feat(world-ptr): WorldPtr<T> / PolyPtr<T> affine pointer pair + migrate Mounts, Outfits, Tile hierarchy

[beats-dh]

Summary

Introduces WorldPtr<T> and PolyPtr<T> — an affine pointer pair on top of std::shared_ptr that pays the atomic refcount only at the world boundary (Lua, network), not on every read traversal. Three storage migrations land on top: Mounts, Outfits, and the Tile hierarchy.

What's in here

API — src/utils/worldpointer.hpp

• WorldPtr<T> (intrusive, 8 B Borrowed): Owning (move-only, single per object) / Borrowed (non-pinning view, zero atomic ops) / Shared (boundary refcount). Block is recovered from the T* via offsetof(Block, value). Per-(T, Allocator) quiescentState() drain.
• PolyPtr<T> (type-erased header, 16 B): polymorphic variant for hierarchies (Tile, Item, Cylinder, Thing). Adds Weak<T> so Item↔Tile cycles can be broken. Single global retire list — one polyPtrQuiescentState() drains every type.
• Lua boundary helpers: pushUserdataAffine / pushUserdataPoly, registerAffineClass / registerPolyClass, matching __gc metamethods.

Migrations

• refactor(mounts): phmap::parallel_flat_hash_set<OwningMount>; Player::hasMount(const Mount*); Lua boundary uses WorldPtr<Mount>::Shared.
• refactor(outfits): std::vector<OwningOutfit> per sex; Player::getOutfitAddons(const Outfit*, uint8_t&).
• refactor(tile): full hierarchy migrated to PolyPtr<Tile>::Owning (Floor) / Borrowed (read paths) / Weak (Item.m_parent, Creature.m_tile to break refcount cycles). Tile::getCylinder() bridges to legacy shared_ptr<Cylinder> callers with a custom-deleter facade that pins via Shared (1 alloc + 1 atomic per call — boundary cost only).

Dispatcher — quiescentState called per migrated type at end-of-tick, plus a final drain after the worker loop exits so destructors don't get skipped on shutdown.

Tests — per-migration suites

• test(utils): 19+ tests covering both type families, move semantics, zero-bump contracts, retire-until-QS, Weak ↔ Shared, offsetof on over-aligned T.
• test(mounts): 28+ tests including MountsTestBase fixture, strict zero-bump lookup, lifetime extension across reload, Lua boundary suite, dispatcher QSBR integration.
• test(outfits): 12 tests mirroring Mounts.
• test(tile): integration callback signature adaptation.

Bug fixes folded in (caught by review + regression-hunter audit)

• fix(world-ptr) Borrowed→Shared resurrection: 5 call sites used a bare fetch_add on strong_refcount — could silently bump from 0, resurrecting a retired block. Replaced with CAS-loop (tryIncrementStrong / Block::tryIncrementReferenceCount) that returns null Shared when expired, same as weak_ptr::lock().
• fix(map): null-guard at Map::moveCreature async branch to handle the new nullable-Shared contract.
• fix(world-ptr) Weak downcast: weakDowncast_ and weakDynamicDowncast_ did fetch_add AND the Weak(Header*, T*) ctor also incremented — every cross-type Weak cast leaked the block. Removed the explicit add.
• fix(dispatcher) shutdown drain.
• fix(tile): five migration misses exposed by Unity build (Map::placeCreature signature, addItemToCylinder bridge, astarnodes.hpp missing include, Floor::setTileCache storage reverted to const BasicTile* since MapCache owns the cache via retainedBasicTiles, Thing::getPosition() const static Position fallback).

Performance impact

• getTile() / Borrowed propagation: −2 atomics per call vs shared_ptr<Tile> return by value.
• Weak<Tile>::borrowIfAlive(): 1 atomic LOAD vs weak_ptr::lock()'s CAS.
• Tile::getCylinder(): +1 alloc + 1 atomic per call vs legacy shared_ptr<Tile> return — accepted bridge cost for un-migrated shared_ptr<Cylinder> APIs.
• PolyPtr header overhead: +16 B per block (weak_refcount + DeleterTable*). For a fully populated map (~10M tiles) that's ~160 MB.

Reviewer guide

• Read in commit order — each commit (refactor / test) is self-contained. The 4 fix(...) commits at the end are the post-review hardening.
• Memory ordering lives in world_ptr_poly_detail::tryIncrementStrong (PolyPtr) and Block::tryIncrementReferenceCount (WorldPtr) — single audit point.
• WorldPtr is single-thread today by explicit design (worldpointer.hpp:63-67). All reload() callers (sighupHandler, GameReload, tests) run on the dispatcher thread. Multi-thread dispatch would need atomic retire push for WorldPtr (PolyPtr already lock-free).

Test plan

• Windows MSVC release — full build green (passed Unity build that originally exposed the Tile migration misses).
• Linux release + debug — pending CI.
• ctest --test-dir build/windows-debug — canary_unittest green.
• AddressSanitizer / UBSanitizer (Linux) — TODO.
• ThreadSanitizer (Linux) — TODO.
• Manual: spawn/despawn loop, mount/dismount, outfit change, house door, SIGHUP reload — no leaks or crashes after several minutes.

Summary by CodeRabbit

• Refactor

  • Backend pointer model migrated to a borrowed/non-owning system, improving performance, reducing runtime refcount churn, and making reloads/cleanup occur at end-of-tick for greater stability.
  • Combat, movement, item, house, mount and outfit flows updated to the new ownership model, improving responsiveness and reliability.

• Tests

  • Added extensive unit and integration tests covering mounts, outfits, pointer semantics, and end-of-tick reclamation.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Migrates ownership from std::shared_ptr to intrusive WorldPtr/PolyPtr (Owning/Borrowed/Shared/Weak), updates tile/map/game/creature/item/house/combat/Lua/protocol APIs to use borrowed views, adds dispatcher QSBR draining, and introduces extensive unit/integration tests and fixtures.

Changes

WorldPointer / Pointer model migration

Layer / File(s)	Summary
WorldPtr/PolyPtr core src/utils/worldpointer.hpp, src/utils/worldpointer.cpp	Adds intrusive WorldPtr and polymorphic PolyPtr pointer types with Owning/Borrowed/Shared/Weak, make_poly, enable_borrowed_from_this, transparent hashing/equality, and QSBR retire/drain tools.
Dispatcher QSBR integration src/game/scheduling/dispatcher.cpp, src/game/scheduling/dispatcher.hpp, tests/unit/game/dispatcher_qsbr_test.cpp	Dispatcher worker loop drains WorldPtr/PolyPtr retire lists end-of-tick; tests verify retire/drain semantics on tick thread.
Tile core changes src/items/tile.hpp, src/items/tile.cpp	Tile now supports borrowed/shared bridging, enable_borrowed_from_this, safeCall overloads, getSurroundingTiles returns borrowed views, and getCylinder pins borrowed lifetime into shared_ptr.
Map / Floor / Cache src/map/map.hpp, src/map/map.cpp, src/map/utils/mapsector.hpp, src/map/mapcache.cpp, src/map/mapcache.hpp	Map APIs now return/accept PolyPtr<Tile>::Borrowed/Owning; Floor storage switched to owning/borrow pair; mapcache and getOrCreateTile build Owning then return Borrowed.
Game and movement src/game/game.hpp, src/game/game.cpp, src/game/movement/teleport.hpp, src/game/movement/teleport.cpp	Game movement/item APIs and internal bookkeeping use borrowed tile handles; added overloads for internalMoveItem/internalAddItem with borrowed tiles; teleport/loop checks updated.
Thing / Item / Creature parent model src/items/thing.hpp, src/items/item.hpp, src/items/item.cpp, src/creatures/creature.hpp, src/creatures/creature.cpp	Thing/Item refactor to ParentRef variant; Item/Thing getTile now return Borrowed; Creature parent wiring moved through setParentImpl and m_tile uses PolyPtr::Weak.
Houses & HouseTile src/map/house/house.hpp, src/map/house/house.cpp, src/map/house/housetile.cpp	House stores PolyPtr::Shared; addTile/transfer helpers accept borrowed/raw HouseTile* or PolyPtr borrows; queryDestination non-null contract adjusted.
Combat & movement events src/creatures/combat/combat.hpp, src/creatures/combat/combat.cpp, src/creatures/monsters/*, src/creatures/npcs/*	Combat area/getList/onTileCombat and various movement/push helpers now use PolyPtr::Borrowed; local tile lists changed to borrowed vectors.
Players / Outfits / Mounts src/creatures/players/*, src/creatures/appearance/outfit/*, src/creatures/appearance/mounts/*	Outfits and Mounts migrated to WorldPtr owning/borrowed models; getters return Borrowed types; Player APIs adjusted to accept raw pointers or borrowed views for mounts/outfits; Lua mount userdata switched to affine model.
Items & Containers / Weapons src/items/containers/*, src/items/weapons/*	Container and weapon APIs updated to accept borrowed tiles; parent lookups use Item::getParent(); hangable/parent casts use raw pointer dynamic_cast where appropriate.
IO & Serialization src/io/iomapserialize.hpp, src/io/iomapserialize.cpp	Tile serialization helper now takes PolyPtr<Tile>::Borrowed and uses tile->getCylinder() for load/save paths.
Lua bindings & Protocol src/lua/functions/*, src/lua/functions/lua_functions_loader.hpp, src/lua/callbacks/*, src/server/network/protocol/*	Lua userdata helpers added for affine/poly types; Tile and Mount userdata switched to poly/affine helpers; ProtocolGame send/GetTileDescription updated to use borrowed tiles; many Lua functions updated to getUserdataPoly/getUserdataAffine and pushUserdataPoly/pushUserdataAffine.
CMake / build CMakePresets.json, src/utils/CMakeLists.txt	CMake preset base gained environment VSLANG=1033; utils CMake source ordering adjusted; worldpointer.cpp placeholder added.
Tests & fixtures tests/unit/utils/worldpointer_test.cpp, tests/unit/game/mounts_test.cpp, tests/unit/game/outfits_test.cpp, tests/unit/game/dispatcher_qsbr_test.cpp, tests/unit/lua/mount_functions_test.cpp, fixtures under tests/fixture/*	Adds comprehensive WorldPtr/PolyPtr unit tests, mounts/outfits tests, dispatcher QSBR integration tests, Lua mount tests, and fixture XML/config files; CMakeLists updated to include new tests.

Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• opentibiabr/canary#3932: Related changes to mount/outfit handling and random-mount substitution; overlaps with mount/outfit API migration.

Suggested reviewers

• dudantas
• majestyotbr

"A rabbit hops with careful cheer,
Borrowed pointers hold things near.
When ticks end, the old are freed,
World pointers hum — no shared_ref need.
Hooray for clean code and carrot cake!" 🐇

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/analyze-feedback-47Stv

[gemini-code-assist]

Code Review

This pull request introduces a high-performance memory management system using WorldPtr and PolyPtr with Quiescent State Based Reclamation (QSBR) to replace std::shared_ptr for core objects like Mount, Outfit, and Tile. The feedback identifies critical thread-safety risks in mapsector.hpp due to missing or disabled locks in setTile and setTileCache, which could result in torn reads or data races. Additionally, the use of offsetof in WorldPtr for non-standard-layout types is noted as a medium-severity issue that will likely trigger compilation errors.

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

src/creatures/monsters/monster.cpp (1)

1583-1588: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Don't enqueue a Borrowed tile into the walk event.

posTile is only a borrowed handle. Capturing it in addWalkEvent() lets the callback outlive the current tile epoch, so it can dereference a retired/replaced tile when the event finally runs.

🐛 Proposed fix

 			if (canPushCreatures()) {
 				if (g_dispatcher().context().getGroup() == TaskGroup::Walk) {
 					Monster::pushCreatures(posTile);
 				} else {
-					g_dispatcher().addWalkEvent([=] {
-						Monster::pushCreatures(posTile);
-					});
+					const Position pushPos = pos;
+					g_dispatcher().addWalkEvent([pushPos] {
+						if (const auto tile = g_game().map.getTile(pushPos)) {
+							Monster::pushCreatures(tile);
+						}
+					});
 				}
 			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/creatures/monsters/monster.cpp` around lines 1583 - 1588, The callback
currently captures the borrowed tile handle posTile and passes it to
Monster::pushCreatures inside g_dispatcher().addWalkEvent, which can outlive the
tile epoch; change the capture to only copy a stable identifier (e.g., tile
coordinates or position) and in the added walk event re-resolve the tile from
the world/map before calling Monster::pushCreatures. Specifically, update the
lambda passed to g_dispatcher().addWalkEvent so it captures a primitive (like
posTile->position() or x/y/z) instead of posTile, then inside the lambda obtain
a fresh tile handle from the dispatcher/world and call Monster::pushCreatures
with that fresh handle.

src/items/weapons/weapons.cpp (1)

321-333: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard empty borrowed tiles before using them.

These overloads now accept PolyPtr<Tile>::Borrowed, but both paths still assume the handle is always live. If a caller passes an empty borrow, this becomes an immediate null dereference on tile->getPosition() or the move path.

Proposed fix

 void Weapon::internalUseWeapon(const std::shared_ptr<Player> &player, const std::shared_ptr<Item> &item, PolyPtr<Tile>::Borrowed tile) const {
+	if (!tile) {
+		return;
+	}
+
 	if (isLoadedScriptId()) {
 		LuaVariant var;
 		var.type = VARIANT_TARGETPOSITION;
@@
 void Weapon::onUsedWeapon(const std::shared_ptr<Player> &player, const std::shared_ptr<Item> &item, PolyPtr<Tile>::Borrowed destTile) const {
+	if (!destTile) {
+		return;
+	}
+
 	if (!player->hasFlag(PlayerFlags_t::NotGainSkill)) {

Also applies to: 335-387

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/items/weapons/weapons.cpp` around lines 321 - 333, The method
Weapon::internalUseWeapon assumes the PolyPtr<Tile>::Borrowed tile is always
valid and dereferences it (tile->getPosition()); add a guard at the start to
handle an empty/expired borrow (e.g., if !tile) and bail out or handle the
missing tile before calling isLoadedScriptId/executeUseWeapon,
Combat::postCombatEffects, g_game().addMagicEffect,
g_game().sendSingleSoundEffect, and onUsedWeapon. Apply the same null-check
pattern to the other overload(s) referenced (lines 335-387) so no code path
dereferences tile when the borrow is empty.

src/creatures/combat/combat.cpp (2)

346-352: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Null-check target->getTile() before tile-based combat checks.

After the tile migration, getTile() can legitimately return a null Borrowed. Lines 347 and 350 dereference it unconditionally, so a detached or already-retired target now turns this path into a crash.

Suggested fix

 	if (target) {
 		PolyPtr<Tile>::Borrowed tile = target->getTile();
-		if (tile->hasProperty(CONST_PROP_BLOCKPROJECTILE)) {
+		if (tile && tile->hasProperty(CONST_PROP_BLOCKPROJECTILE)) {
 			return RETURNVALUE_NOTENOUGHROOM;
 		}
-		if (targetPlayer && tile->hasFlag(TILESTATE_PROTECTIONZONE)) {
+		if (targetPlayer && tile && tile->hasFlag(TILESTATE_PROTECTIONZONE)) {
 			const auto permittedOnPz = targetPlayer->hasPermittedConditionInPZ();
 			return permittedOnPz ? RETURNVALUE_NOERROR : RETURNVALUE_ACTIONNOTPERMITTEDINPROTECTIONZONE;
 		}
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/creatures/combat/combat.cpp` around lines 346 - 352, The code
dereferences the result of target->getTile() (assigned to
PolyPtr<Tile>::Borrowed tile) without null-checking, causing crashes when
getTile() returns a null Borrowed; add a guard after obtaining tile to return an
appropriate value (e.g., RETURNVALUE_NOTPOSSIBLE or RETURNVALUE_NOTENOUGHROOM)
when tile is null before calling tile->hasProperty(CONST_PROP_BLOCKPROJECTILE)
or tile->hasFlag(TILESTATE_PROTECTIONZONE), and only call
targetPlayer->hasPermittedConditionInPZ() when tile is non-null.

---

1048-1134: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Early-return when tile effects receive a null borrow.

combatTileEffects() now accepts PolyPtr<Tile>::Borrowed, but every path assumes tile is valid. doCombatDefault() can pass target->getTile() directly, so the first detached target will crash here as soon as an item/effect/tile callback is processed.

Suggested fix

 void Combat::combatTileEffects(const CreatureVector &spectators, const std::shared_ptr<Creature> &caster, PolyPtr<Tile>::Borrowed tile, const CombatParams &params) {
+	if (!tile) {
+		return;
+	}
+
 	if (params.itemId != 0) {
 		uint16_t itemId = params.itemId;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/creatures/combat/combat.cpp` around lines 1048 - 1134, combatTileEffects
assumes the borrowed tile is non-null and will dereference it unconditionally,
causing a crash when doCombatDefault passes a null target->getTile(); fix by
adding an early null check at the top of Combat::combatTileEffects (the function
taking PolyPtr<Tile>::Borrowed tile) and return immediately if tile is null so
no item creation, tileCallback->onTileCombat, sendCombatEffect, or sound calls
run against a null tile.

src/server/network/protocol/protocolgame.cpp (1)

4090-4105: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Serialize mounts in a stable order.

Mounts::getMounts() now comes from an unordered hash-set-backed store, so iterating it directly makes these packets nondeterministic. That will reshuffle mount lists between runs/reloads and can make the UI and packet-order tests flaky. Snapshot the borrowed mounts and sort them by a stable key such as clientId before encoding.

Also applies to: 7878-7892, 7985-7999, 8108-8118

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/network/protocol/protocolgame.cpp` around lines 4090 - 4105, The
loop iterates g_game().mounts->getMounts() (an unordered set) directly causing
nondeterministic packet order; fix by copying the borrowed mounts into a
std::vector, sort that vector by a stable key such as mount->clientId, then
iterate the sorted vector when calling player->hasMount(mount.get()), reading
mount->type and mount->clientId and serializing with msg.add/...; apply the same
snapshot-and-sort change to the other mount-iteration sites referenced (the
blocks around the other ranges).

src/game/game.cpp (1)

1394-1405: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Don’t capture PolyPtr<Tile>::Borrowed across dispatcher/task boundaries.

These callbacks store a borrowed tile and dereference it later from createPlayerTask(...) / addWalkEvent(...). With the new QSBR-style lifetime rules, that borrowed handle can be invalid by the time the callback runs, turning this into a use-after-free. Capture Position and re-resolve the tile inside the deferred callback, or cross the boundary with an owning/shared facade instead.

Suggested direction

-		PolyPtr<Tile>::Borrowed tile = map.getTile(toPos);
-		if (!tile) {
+		PolyPtr<Tile>::Borrowed tile = map.getTile(toPos);
+		if (!tile) {
 			player->sendCancelMessage(RETURNVALUE_NOTPOSSIBLE);
 			return;
 		}
+		const Position targetPos = tile->getPosition();

 		if (Position::areInRange<1, 1, 0>(movingCreature->getPosition(), player->getPosition())) {
 			const auto &task = createPlayerTask(
 				g_configManager().getNumber(PUSH_DELAY),
-				[this, player, movingCreature, tile] {
-					playerMoveCreatureByID(player->getID(), movingCreature->getID(), movingCreature->getPosition(), tile->getPosition());
+				[this, player, movingCreature, targetPos] {
+					playerMoveCreatureByID(player->getID(), movingCreature->getID(), movingCreature->getPosition(), targetPos);
 				},
 				__FUNCTION__
 			);
 			player->setNextActionPushTask(task);
 		} else {
-			playerMoveCreature(player, movingCreature, movingCreature->getPosition(), tile);
+			playerMoveCreature(player, movingCreature, movingCreature->getPosition(), targetPos);
 		}
 	}

-void Game::playerMoveCreature(const std::shared_ptr<Player> &player, const std::shared_ptr<Creature> &movingCreature, const Position &movingCreatureOrigPos, PolyPtr<Tile>::Borrowed toTile) {
+void Game::playerMoveCreature(const std::shared_ptr<Player> &player, const std::shared_ptr<Creature> &movingCreature, const Position &movingCreatureOrigPos, const Position &targetPos) {
 	metrics::method_latency measure(__METRICS_METHOD_NAME__);

-	g_dispatcher().addWalkEvent([=, this] {
+	g_dispatcher().addWalkEvent([=, this] {
+		const auto toTile = map.getTile(targetPos);
+		if (!toTile) {
+			player->sendCancelMessage(RETURNVALUE_NOTPOSSIBLE);
+			return;
+		}
+
 		if (!player->canDoAction()) {
 			const auto &task = createPlayerTask(
 				600,
-				[this, player, movingCreature, toTile, movingCreatureOrigPos] {
-					playerMoveCreatureByID(player->getID(), movingCreature->getID(), movingCreatureOrigPos, toTile->getPosition());
+				[this, player, movingCreature, movingCreatureOrigPos, targetPos] {
+					playerMoveCreatureByID(player->getID(), movingCreature->getID(), movingCreatureOrigPos, targetPos);
 				},
 				__FUNCTION__
 			);

Also mirror the signature change in src/game/game.hpp.

Also applies to: 1434-1471

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/game/game.cpp` around lines 1394 - 1405, The callback captures a borrowed
PolyPtr<Tile>::Borrowed (from map.getTile(toPos)) which may be invalid when the
deferred task created via createPlayerTask / addWalkEvent runs; instead capture
the tile's Position (e.g., tile->getPosition()) and any IDs you need, then
inside the deferred lambda re-resolve the tile via map.getTile(resolvedPos)
before calling playerMoveCreatureByID(player->getID(), movingCreature->getID(),
movingCreature->getPosition(), tilePos) (or bail out if the tile no longer
exists), and mirror the changed lambda signature in the corresponding
declaration in the header (game.hpp).

🧹 Nitpick comments (4)

src/items/containers/container.hpp (1)

162-162: ⚡ Quick win

Align createBrowseField naming/docs with the borrowed handle API.

The signature now takes PolyPtr<Tile>::Borrowed, but docs still describe a shared pointer and the parameter name type is ambiguous.

Proposed cleanup

- * `@param` tile A shared pointer to the Tile from which items will be sourced.
+ * `@param` tile Borrowed handle to the Tile from which items will be sourced.
...
- * `@note` This function will return nullptr if the newContainer could not be created or if the tile pointer is null.
+ * `@note` This function returns nullptr if the container cannot be created or the tile handle is empty.
...
-static std::shared_ptr<Container> createBrowseField(PolyPtr<Tile>::Borrowed type);
+static std::shared_ptr<Container> createBrowseField(PolyPtr<Tile>::Borrowed tile);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/items/containers/container.hpp` at line 162, The declaration for
createBrowseField currently accepts PolyPtr<Tile>::Borrowed but the docs and
parameter name imply a shared_ptr and are ambiguous; update the function
signature comment and parameter name to reflect the borrowed handle API (e.g.,
change parameter name from type to tileHandle or borrowedTile) and revise the
docblock to state it takes a PolyPtr<Tile>::Borrowed borrowed handle (not a
shared_ptr), plus ensure any related factory/overload names match the
borrowed-handle convention where createBrowseField is used.

tests/unit/game/mounts_test.cpp (2)

56-69: ⚡ Quick win

Drain the retired mount blocks in MountsApiTest too.

This fixture also creates and destroys Mounts instances, so it can leave WorldPtr<Mount> retire-list state behind for later tests. Mirroring GlobalMountsTest::TearDown() here will keep the suite isolated.

Proposed fix

-class MountsApiTest : public test::mounts::MountsTestBase { };
+class MountsApiTest : public test::mounts::MountsTestBase {
+protected:
+	void TearDown() override {
+		MountsTestBase::TearDown();
+		drainMounts();
+	}
+};

As per coding guidelines, C++ changes should "prefer focused tests or the smallest practical build/check that validates the touched code".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/game/mounts_test.cpp` around lines 56 - 69, Add a TearDown
override to MountsApiTest (which inherits test::mounts::MountsTestBase) that
mirrors GlobalMountsTest::TearDown: call MountsTestBase::TearDown() and then
call drainMounts() to clear retired WorldPtr<Mount> blocks left by the fixture;
this ensures MountsApiTest does not leave retire-list state for later tests.

---

224-237: ⚡ Quick win

Reload_DefersOldMountsToQSBR does not actually prove deferral.

The only post-reload assertion is the new container size, so this still passes if old mounts are reclaimed eagerly. Keep a pre-reload borrow/shared handle and assert the old object survives until the quiescent drain boundary.

As per coding guidelines, C++ changes should "prefer focused tests or the smallest practical build/check that validates the touched code".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/game/mounts_test.cpp` around lines 224 - 237, The test
Reload_DefersOldMountsToQSBR must retain a pre-reload borrow/handle to an old
mount so we can prove reclamation is deferred: before calling mounts.reload(),
obtain and hold a shared/borrowed reference to one of the existing mount objects
(via mounts.getMounts() or the API that returns a shared_ptr/borrowed handle),
then call reload() and run drainMounts(); assert that the held handle still
points to a valid/expected old mount (i.e., the object was not destroyed), only
then release the handle and run drainMounts() again and assert final
cleanup/size changes; update assertions to reference Mounts::loadFromXml,
Mounts::reload, Mounts::getMounts, and drainMounts to locate the changes.

tests/unit/utils/worldpointer_test.cpp (1)

241-269: ⚡ Quick win

Add a regression case for post-retire Borrowed::share().

These tests cover the successful promotion path, but not the bug this PR is fixing: borrowing first, dropping the last strong ref, and then verifying that share() returns null before quiescentState(). Without that case, the resurrection fix can regress silently.

As per coding guidelines, "For C++ changes, prefer focused tests or the smallest practical build/check that validates the touched code".

Also applies to: 585-604

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/utils/worldpointer_test.cpp` around lines 241 - 269, Add a focused
regression test that exercises calling Borrowed::share() after the owner has
been dropped but before WorldPtr<Probe>::quiescentState() to ensure it returns
null; specifically create an Owning via Owning::make(value), obtain a Borrowed
view = owner.borrow(), reset/drop the owner, then call auto promoted =
view.share() and ASSERT_FALSE(promoted) (or EXPECT_EQ(nullptr, promoted.get()))
before calling WorldPtr<Probe>::quiescentState(), finally verify that after
quiescentState() promotion behaves as expected if needed; place this alongside
the existing Borrowed->Shared tests and reference Borrowed::share, Owning::make,
and WorldPtr::quiescentState to locate the code.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/game/game.hpp`:
- Around line 623-627: addTileToClean currently calls
tilesToClean.emplace(tile.share()) which can insert a null Shared when
Borrowed::share() fails; change addTileToClean to promote once into a
PolyPtr<Tile>::Shared local (e.g. auto shared = tile.share()) and only call
tilesToClean.emplace(shared) if shared is non-null, so null promotions are never
inserted and removeTileToClean can correctly match/erase entries.

In `@src/items/tile.cpp`:
- Around line 1977-2007: The safeCall overloads assume Borrowed::share() always
returns a valid pin and only assert, which disappears in release builds; change
both Tile::safeCall functions to perform a runtime check after calling
borrowedFromThis().share() (for the Borrowed overload check the pinned variable,
for the void overload check pin) and if the returned shared pin is null simply
return/do not call g_dispatcher().addEvent (i.e., drop the work) instead of
enqueueing; remove or keep the assert as a diagnostic but ensure the null-check
prevents scheduling when the tile is retired. Use the existing symbols
borrowedFromThis(), Borrowed::share(), g_dispatcher().addEvent and the lambda
captures (pinned/pin) when implementing the guard.

In `@src/map/map.cpp`:
- Around line 151-169: Reject out-of-range layer indices early in
Map::getOrCreateTile by checking z against MAP_MAX_LAYERS before constructing
any PolyPtr<Tile>::Owning or calling borrow(); if z is invalid, return an empty
PolyPtr<Tile>::Borrowed immediately instead of creating an Owning (so
getTile()/setTile() cannot drop a newly-created Owning and leave a dangling
borrowed). Move the z validation to the top of getOrCreateTile, and only create
the make_poly<DynamicTile>/make_poly<StaticTile>, call owning.borrow(), and
setTile(...) after the z check and after confirming getTile(...) did not already
return a tile.

In `@src/map/mapcache.cpp`:
- Around line 317-325: The code creates tile = make_poly<HouseTile>(...) only
when a house is found but then unconditionally calls
tile.borrow()->safeCall(...), which dereferences a null handle if
getHouse(cachedTile->houseId) missed; fix by checking that the house
pointer/result of getHouse(cachedTile->houseId) is non-null before
creating/using tile and move the tile.borrow()->safeCall(...) inside that branch
(or return/continue after logging the missing house), and apply the same guard
for the similar block around the code referenced at lines 333-347; reference
symbols: getHouse, HouseTile, make_poly<HouseTile>, tile,
PolyPtr<Tile>::Borrowed, safeCall, cachedTile->houseId.

In `@src/map/utils/mapsector.hpp`:
- Around line 40-45: Writes to tiles in setTile and setTileCache must be
protected by the same exclusive mutex used with readers (getTile/getTileCache
use shared_lock); restore an exclusive lock (e.g.
std::unique_lock<std::shared_mutex> or equivalent scoped exclusive lock on
mutex) at the start of both setTile and setTileCache before mutating tiles[x &
SECTOR_MASK][y & SECTOR_MASK].first/second so concurrent readers cannot observe
a torn PolyPtr<Tile>::Owning state.

In `@tests/shared/mounts/mounts_test_fixture.hpp`:
- Around line 47-50: In TearDown() ensure the config restoration cannot silently
fail by checking the return value of
g_configManager().setConfigFileLua(previousConfigFile_) and
g_configManager().reload() (or otherwise verifying reload succeeded) and failing
the test if either call fails; update the TearDown implementation in
mounts_test_fixture.hpp to assert or EXPECT_TRUE/ASSERT_TRUE on the results of
setConfigFileLua and reload (while still restoring
std::filesystem::current_path(previousPath_)) so any config restore failure
causes an immediate test failure and prevents state leakage into later tests.

---

Outside diff comments:
In `@src/creatures/combat/combat.cpp`:
- Around line 346-352: The code dereferences the result of target->getTile()
(assigned to PolyPtr<Tile>::Borrowed tile) without null-checking, causing
crashes when getTile() returns a null Borrowed; add a guard after obtaining tile
to return an appropriate value (e.g., RETURNVALUE_NOTPOSSIBLE or
RETURNVALUE_NOTENOUGHROOM) when tile is null before calling
tile->hasProperty(CONST_PROP_BLOCKPROJECTILE) or
tile->hasFlag(TILESTATE_PROTECTIONZONE), and only call
targetPlayer->hasPermittedConditionInPZ() when tile is non-null.
- Around line 1048-1134: combatTileEffects assumes the borrowed tile is non-null
and will dereference it unconditionally, causing a crash when doCombatDefault
passes a null target->getTile(); fix by adding an early null check at the top of
Combat::combatTileEffects (the function taking PolyPtr<Tile>::Borrowed tile) and
return immediately if tile is null so no item creation,
tileCallback->onTileCombat, sendCombatEffect, or sound calls run against a null
tile.

In `@src/creatures/monsters/monster.cpp`:
- Around line 1583-1588: The callback currently captures the borrowed tile
handle posTile and passes it to Monster::pushCreatures inside
g_dispatcher().addWalkEvent, which can outlive the tile epoch; change the
capture to only copy a stable identifier (e.g., tile coordinates or position)
and in the added walk event re-resolve the tile from the world/map before
calling Monster::pushCreatures. Specifically, update the lambda passed to
g_dispatcher().addWalkEvent so it captures a primitive (like posTile->position()
or x/y/z) instead of posTile, then inside the lambda obtain a fresh tile handle
from the dispatcher/world and call Monster::pushCreatures with that fresh
handle.

In `@src/game/game.cpp`:
- Around line 1394-1405: The callback captures a borrowed
PolyPtr<Tile>::Borrowed (from map.getTile(toPos)) which may be invalid when the
deferred task created via createPlayerTask / addWalkEvent runs; instead capture
the tile's Position (e.g., tile->getPosition()) and any IDs you need, then
inside the deferred lambda re-resolve the tile via map.getTile(resolvedPos)
before calling playerMoveCreatureByID(player->getID(), movingCreature->getID(),
movingCreature->getPosition(), tilePos) (or bail out if the tile no longer
exists), and mirror the changed lambda signature in the corresponding
declaration in the header (game.hpp).

In `@src/items/weapons/weapons.cpp`:
- Around line 321-333: The method Weapon::internalUseWeapon assumes the
PolyPtr<Tile>::Borrowed tile is always valid and dereferences it
(tile->getPosition()); add a guard at the start to handle an empty/expired
borrow (e.g., if !tile) and bail out or handle the missing tile before calling
isLoadedScriptId/executeUseWeapon, Combat::postCombatEffects,
g_game().addMagicEffect, g_game().sendSingleSoundEffect, and onUsedWeapon. Apply
the same null-check pattern to the other overload(s) referenced (lines 335-387)
so no code path dereferences tile when the borrow is empty.

In `@src/server/network/protocol/protocolgame.cpp`:
- Around line 4090-4105: The loop iterates g_game().mounts->getMounts() (an
unordered set) directly causing nondeterministic packet order; fix by copying
the borrowed mounts into a std::vector, sort that vector by a stable key such as
mount->clientId, then iterate the sorted vector when calling
player->hasMount(mount.get()), reading mount->type and mount->clientId and
serializing with msg.add/...; apply the same snapshot-and-sort change to the
other mount-iteration sites referenced (the blocks around the other ranges).

---

Nitpick comments:
In `@src/items/containers/container.hpp`:
- Line 162: The declaration for createBrowseField currently accepts
PolyPtr<Tile>::Borrowed but the docs and parameter name imply a shared_ptr and
are ambiguous; update the function signature comment and parameter name to
reflect the borrowed handle API (e.g., change parameter name from type to
tileHandle or borrowedTile) and revise the docblock to state it takes a
PolyPtr<Tile>::Borrowed borrowed handle (not a shared_ptr), plus ensure any
related factory/overload names match the borrowed-handle convention where
createBrowseField is used.

In `@tests/unit/game/mounts_test.cpp`:
- Around line 56-69: Add a TearDown override to MountsApiTest (which inherits
test::mounts::MountsTestBase) that mirrors GlobalMountsTest::TearDown: call
MountsTestBase::TearDown() and then call drainMounts() to clear retired
WorldPtr<Mount> blocks left by the fixture; this ensures MountsApiTest does not
leave retire-list state for later tests.
- Around line 224-237: The test Reload_DefersOldMountsToQSBR must retain a
pre-reload borrow/handle to an old mount so we can prove reclamation is
deferred: before calling mounts.reload(), obtain and hold a shared/borrowed
reference to one of the existing mount objects (via mounts.getMounts() or the
API that returns a shared_ptr/borrowed handle), then call reload() and run
drainMounts(); assert that the held handle still points to a valid/expected old
mount (i.e., the object was not destroyed), only then release the handle and run
drainMounts() again and assert final cleanup/size changes; update assertions to
reference Mounts::loadFromXml, Mounts::reload, Mounts::getMounts, and
drainMounts to locate the changes.

In `@tests/unit/utils/worldpointer_test.cpp`:
- Around line 241-269: Add a focused regression test that exercises calling
Borrowed::share() after the owner has been dropped but before
WorldPtr<Probe>::quiescentState() to ensure it returns null; specifically create
an Owning via Owning::make(value), obtain a Borrowed view = owner.borrow(),
reset/drop the owner, then call auto promoted = view.share() and
ASSERT_FALSE(promoted) (or EXPECT_EQ(nullptr, promoted.get())) before calling
WorldPtr<Probe>::quiescentState(), finally verify that after quiescentState()
promotion behaves as expected if needed; place this alongside the existing
Borrowed->Shared tests and reference Borrowed::share, Owning::make, and
WorldPtr::quiescentState to locate the code.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 58be47eb-4b2b-48c5-aba0-e10e823efa1a

📥 Commits

Reviewing files that changed from the base of the PR and between bd5b402 and 4c4ae7d.

📒 Files selected for processing (91)

• CMakePresets.json
• src/creatures/appearance/mounts/mounts.cpp
• src/creatures/appearance/mounts/mounts.hpp
• src/creatures/appearance/outfit/outfit.cpp
• src/creatures/appearance/outfit/outfit.hpp
• src/creatures/combat/combat.cpp
• src/creatures/combat/combat.hpp
• src/creatures/creature.cpp
• src/creatures/creature.hpp
• src/creatures/monsters/monster.cpp
• src/creatures/monsters/monster.hpp
• src/creatures/npcs/npc.cpp
• src/creatures/npcs/npc.hpp
• src/creatures/players/components/player_title.cpp
• src/creatures/players/player.cpp
• src/creatures/players/player.hpp
• src/game/game.cpp
• src/game/game.hpp
• src/game/movement/teleport.cpp
• src/game/movement/teleport.hpp
• src/game/scheduling/dispatcher.cpp
• src/game/scheduling/dispatcher.hpp
• src/io/iomapserialize.cpp
• src/io/iomapserialize.hpp
• src/items/containers/container.cpp
• src/items/containers/container.hpp
• src/items/containers/depot/depotchest.cpp
• src/items/containers/depot/depotchest.hpp
• src/items/containers/depot/depotlocker.cpp
• src/items/containers/inbox/inbox.cpp
• src/items/containers/inbox/inbox.hpp
• src/items/containers/rewards/reward.cpp
• src/items/containers/rewards/reward.hpp
• src/items/containers/rewards/rewardchest.cpp
• src/items/item.cpp
• src/items/item.hpp
• src/items/thing.cpp
• src/items/thing.hpp
• src/items/tile.cpp
• src/items/tile.hpp
• src/items/trashholder.cpp
• src/items/weapons/weapons.cpp
• src/items/weapons/weapons.hpp
• src/lua/callbacks/event_callback.cpp
• src/lua/callbacks/event_callback.hpp
• src/lua/creature/actions.cpp
• src/lua/creature/events.cpp
• src/lua/creature/events.hpp
• src/lua/creature/movement.cpp
• src/lua/creature/movement.hpp
• src/lua/functions/core/game/game_functions.cpp
• src/lua/functions/creatures/creature_functions.cpp
• src/lua/functions/creatures/npc/npc_functions.cpp
• src/lua/functions/creatures/player/mount_functions.cpp
• src/lua/functions/creatures/player/player_functions.cpp
• src/lua/functions/creatures/player/player_functions.hpp
• src/lua/functions/items/item_functions.cpp
• src/lua/functions/lua_functions_loader.cpp
• src/lua/functions/lua_functions_loader.hpp
• src/lua/functions/map/house_functions.cpp
• src/lua/functions/map/position_functions.cpp
• src/lua/functions/map/tile_functions.cpp
• src/map/house/house.cpp
• src/map/house/house.hpp
• src/map/house/housetile.cpp
• src/map/map.cpp
• src/map/map.hpp
• src/map/mapcache.cpp
• src/map/mapcache.hpp
• src/map/utils/astarnodes.cpp
• src/map/utils/astarnodes.hpp
• src/map/utils/mapsector.hpp
• src/server/network/protocol/protocolgame.cpp
• src/server/network/protocol/protocolgame.hpp
• src/utils/CMakeLists.txt
• src/utils/worldpointer.cpp
• src/utils/worldpointer.hpp
• tests/fixture/config/mounts_test.lua
• tests/fixture/core/XML/mounts.xml
• tests/fixture/core/XML/outfits.xml
• tests/integration/event_callbacks/event_callbacks_it.cpp
• tests/shared/mounts/mounts_test_fixture.hpp
• tests/unit/game/CMakeLists.txt
• tests/unit/game/dispatcher_qsbr_test.cpp
• tests/unit/game/mounts_test.cpp
• tests/unit/game/outfits_test.cpp
• tests/unit/game/random_mount_test.cpp
• tests/unit/lua/CMakeLists.txt
• tests/unit/lua/mount_functions_test.cpp
• tests/unit/utils/CMakeLists.txt
• tests/unit/utils/worldpointer_test.cpp

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/utils/worldpointer.hpp (3)

402-424: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Make nullable Borrowed -> Shared promotion explicit.

These conversions now have lock()-style failure semantics, but they still happen implicitly. That makes the new null path easy to miss at call sites that accept Shared, and the resulting bugs show up later as null dereferences instead of compile-time friction.

Suggested fix

-		[[nodiscard]] operator Shared<Allocator>() const noexcept {
+		[[nodiscard]] explicit operator Shared<Allocator>() const noexcept {
 			if (!this->tryIncrementReferenceCount()) {
 				return Shared<Allocator> {};
 			}
 			return Shared<Allocator>(this->get());
 		}

-		[[nodiscard]] operator Shared() const noexcept {
+		[[nodiscard]] explicit operator Shared() const noexcept {
 			if (!world_ptr_poly_detail::tryIncrementStrong(this->header_)) {
 				return Shared {};
 			}
 			return Shared(this->header_, this->value_);
 		}

Also applies to: 1090-1113

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/worldpointer.hpp` around lines 402 - 424, The implicit conversion
from Borrowed to Shared (operator Shared<Allocator>() and share()) can now fail
and return a null Shared but remains implicit, causing silent nulls at call
sites expecting a non-null Shared; update the API to make nullable promotion
explicit by removing or making operator Shared<Allocator>() explicit (or marking
it as explicit operator Shared<Allocator>() noexcept) and ensure share() is
clearly named (e.g., lock() or try_share()) and documents/returns an
optional-like result so callers must handle nulls; change usages that relied on
implicit conversion to call the explicit method (try_share()/lock()) and add
checks for null before dereference, referencing Borrowed::operator
Shared<Allocator>(), Borrowed::share(), and tryIncrementReferenceCount() to
locate and update all conversions (also apply same change to the other
occurrence at the noted range).

---

869-874: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Hide the raw control-block fields.

header_ and value_ are inherited by every public pointer type here, so external code can retarget them without the matching refcount transition and corrupt ownership state.

Suggested fix

 class Base {
+	template <typename>
+	friend struct ::PolyPtr;
+
 public:
 	// Public to allow cross-type (PolyPtr<Derived> → PolyPtr<Base>)
 	// constructors to read the source pair without a maze of friend
 	// declarations across nested templates. Treat as internal — direct
 	// mutation breaks invariants.
-	Header* header_;
-	T* value_;
+protected:
+	Header* header_;
+	T* value_;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/worldpointer.hpp` around lines 869 - 874, Make the raw
control-block fields header_ and value_ non-public to prevent external
retargeting: move Header* header_ and T* value_ to private, add protected
read-only accessors (e.g. Header* header() const and T* value() const) and a
single protected helper (e.g. set_control(Header*, T*) or a protected
conversion-constructor) that performs the required refcount transition; update
PolyPtr conversion constructors to use those helpers so only controlled code can
change the control pointers.

---

1739-1745: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

AnyPtrSelector reintroduces incomplete-type hazard via std::is_polymorphic_v<T>

AnyPtrSelector<T> probes std::is_polymorphic_v<T> at alias instantiation time (src/utils/worldpointer.hpp ~1739-1745). That runs the trait even when T may be forward-declared/incomplete, which the header earlier explicitly avoided for PolyPtr (the code comment calls out that MSVC rejects incomplete records as arguments to __is_polymorphic). The standard/library wording treats std::is_polymorphic on an incomplete non-union class type as undefined behavior, so this is a portability/design boundary.

Fix by guarding completeness at the selector boundary with a clear diagnostic (e.g. require sizeof(T)/complete type before evaluating is_polymorphic_v), or by restructuring so polymorphic detection only happens at a point where T is complete (similar to PolyPtr<T>::Owning::make).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/worldpointer.hpp` around lines 1739 - 1745, AnyPtrSelector
currently evaluates std::is_polymorphic_v<T> even for
incomplete/forward-declared T, reintroducing the incomplete-type UB; replace the
direct use with a completeness-guarded trait (e.g. add a helper template
is_polymorphic_if_complete<T> that defaults to false and uses
std::void_t<decltype(sizeof(T))> specialization to evaluate
std::is_polymorphic_v<T> only when sizeof(T) is well-formed) and then change
AnyPtrSelector to use that helper (keep symbols AnyPtrSelector, AnyPtr, PolyPtr,
WorldPtr unchanged so selection logic is identical but safe on incomplete
types).

🧹 Nitpick comments (1)

src/utils/worldpointer.hpp (1)

173-196: ⚡ Quick win

Add a focused check around the non-standard-layout offsetof path.

This logic now intentionally depends on compiler-specific offsetof behavior for payloads like std::string. A tiny compile/runtime test that instantiates WorldPtr with a non-standard-layout T would lock in the exact contract this change relies on.

As per coding guidelines, **/*.{cpp,hpp,h,cc,cxx}: For C++ changes, prefer focused tests or the smallest practical build/check that validates the touched code.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/worldpointer.hpp` around lines 173 - 196, Add a focused
unit/compile-time test that instantiates WorldPtr with a non-standard-layout
payload (e.g., T = std::string) and forces the offsetof path to execute (create
a Block-like wrapper, set inner_ non-null and call the conversion that uses
offsetof(Block, value)) so the code path in WorldPtr, Block, inner_, and the
offsetof usage is compiled and exercised; ensure the test is minimal (single TU)
and runs under GCC to validate the pragma suppression and that both compilation
and a simple runtime check (returned pointer/ref is correct) succeed.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/utils/worldpointer.hpp`:
- Around line 402-424: The implicit conversion from Borrowed to Shared (operator
Shared<Allocator>() and share()) can now fail and return a null Shared but
remains implicit, causing silent nulls at call sites expecting a non-null
Shared; update the API to make nullable promotion explicit by removing or making
operator Shared<Allocator>() explicit (or marking it as explicit operator
Shared<Allocator>() noexcept) and ensure share() is clearly named (e.g., lock()
or try_share()) and documents/returns an optional-like result so callers must
handle nulls; change usages that relied on implicit conversion to call the
explicit method (try_share()/lock()) and add checks for null before dereference,
referencing Borrowed::operator Shared<Allocator>(), Borrowed::share(), and
tryIncrementReferenceCount() to locate and update all conversions (also apply
same change to the other occurrence at the noted range).
- Around line 869-874: Make the raw control-block fields header_ and value_
non-public to prevent external retargeting: move Header* header_ and T* value_
to private, add protected read-only accessors (e.g. Header* header() const and
T* value() const) and a single protected helper (e.g. set_control(Header*, T*)
or a protected conversion-constructor) that performs the required refcount
transition; update PolyPtr conversion constructors to use those helpers so only
controlled code can change the control pointers.
- Around line 1739-1745: AnyPtrSelector currently evaluates
std::is_polymorphic_v<T> even for incomplete/forward-declared T, reintroducing
the incomplete-type UB; replace the direct use with a completeness-guarded trait
(e.g. add a helper template is_polymorphic_if_complete<T> that defaults to false
and uses std::void_t<decltype(sizeof(T))> specialization to evaluate
std::is_polymorphic_v<T> only when sizeof(T) is well-formed) and then change
AnyPtrSelector to use that helper (keep symbols AnyPtrSelector, AnyPtr, PolyPtr,
WorldPtr unchanged so selection logic is identical but safe on incomplete
types).

---

Nitpick comments:
In `@src/utils/worldpointer.hpp`:
- Around line 173-196: Add a focused unit/compile-time test that instantiates
WorldPtr with a non-standard-layout payload (e.g., T = std::string) and forces
the offsetof path to execute (create a Block-like wrapper, set inner_ non-null
and call the conversion that uses offsetof(Block, value)) so the code path in
WorldPtr, Block, inner_, and the offsetof usage is compiled and exercised;
ensure the test is minimal (single TU) and runs under GCC to validate the pragma
suppression and that both compilation and a simple runtime check (returned
pointer/ref is correct) succeed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bca1d882-3717-4bb5-bcb2-54c85055ed83

📥 Commits

Reviewing files that changed from the base of the PR and between 4c4ae7d and 58e2b49.

📒 Files selected for processing (1)

• src/utils/worldpointer.hpp

[beats-dh]

@coderabbitai re: the outside-diff findings in your latest review — addressing each here since they're not inline-replyable:

1. explicit operator Shared (worldpointer.hpp:402-424, 1090-1113) — deferred. The implicit conversion is load-bearing for ergonomics at many call sites (function-arg passing of Borrowed where Shared is expected, container insertion, etc.); making it explicit would force an API-wide audit for marginal safety gain. share() and lock() already exist as explicit entry points. Will revisit as part of a dedicated API-hardening pass once we have data on which call sites genuinely need the nullable path.

2. Privatize header_/value_ (worldpointer.hpp:869-874) — won't fix. The comment in-place already documents the explicit trade-off the original author made: "Public to allow cross-type (PolyPtr → PolyPtr) constructors to read the source pair without a maze of friend declarations across nested templates." Moving them private would require friending every sibling type (Borrowed/Shared/Owning/Weak) AND every PolyPtr for cross-type access. The "treat as internal" contract is documented.

3. AnyPtrSelector incomplete-type hazard (worldpointer.hpp:1739-1745) — deferred. AnyPtr<T> is only used by the test suite today (tests/unit/utils/worldpointer_test.cpp:661-668) with complete types (Probe, PolyBase); no production code instantiates it. The UB is theoretical, not exercised. Will add the is_polymorphic_if_complete guard if/when production adopts AnyPtr.

4. Nitpick: focused test for non-standard-layout offsetof (worldpointer.hpp:173-196) — skip. WorldPtr<Mount> and WorldPtr<Outfit> (both contain std::string members → non-standard-layout) are already instantiated and exercised end-to-end by the mounts/outfits test suites that ship in this PR. A dedicated test of the offsetof path would duplicate that coverage.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@beats-dh: Thanks for the thorough write-up — acknowledged on all four points.

1. explicit operator Shared — Understood. The implicit conversion is clearly load-bearing at many call sites. Will defer until the API-hardening pass with production call-site data.

2. header_/value_ visibility — Makes sense. The cross-type upcasts (PolyPtr<Derived> → PolyPtr<Base>) would require a combinatorial explosion of friend declarations if those fields were private. The in-code documentation of the "treat as internal" contract is a reasonable substitute.

3. AnyPtrSelector incomplete-type hazard — Fair. Theoretical-only for now given all current instantiation sites use complete types. Adding the is_polymorphic_if_complete guard at production adoption time is the right call.

4. offsetof nitpick — Agreed; the WorldPtr<Mount>/WorldPtr<Outfit> end-to-end suites already exercise the non-standard-layout path, so a dedicated unit test would be redundant.

---

✏️ Learnings added

Learnt from: beats-dh
URL: https://github.com/opentibiabr/canary/pull/3975

Timestamp: 2026-05-24T07:02:41.894Z
Learning: In `src/utils/worldpointer.hpp`, `PolyPtr<T>::ConcreteBlock` members `header_` and `value_` are intentionally public (not private) to allow cross-type `PolyPtr<Derived> → PolyPtr<Base>` constructors to access them without requiring a combinatorial set of `friend` declarations across all nested template siblings. This is a documented design trade-off; do not flag them as requiring privatization.

---

Learnt from: beats-dh
URL: https://github.com/opentibiabr/canary/pull/3975

Timestamp: 2026-05-24T07:02:41.894Z
Learning: In `src/utils/worldpointer.hpp`, the `operator Shared` conversion on `WorldPtr<T>::Borrowed` and `PolyPtr<T>::Borrowed` is intentionally implicit (not `explicit`) because it is load-bearing for ergonomics at many call sites (function-arg passing, container insertion, etc.). `share()` and `lock()` exist as explicit entry points. Do not flag the implicit conversion as a safety concern without new data from a production call-site audit.

---

Learnt from: beats-dh
URL: https://github.com/opentibiabr/canary/pull/3975

Timestamp: 2026-05-24T07:02:41.894Z
Learning: In `src/utils/worldpointer.hpp`, `AnyPtrSelector<T>` / `AnyPtr<T>` (around line 1739-1745) has a theoretical incomplete-type UB hazard, but as of PR `#3975` it is only instantiated in the test suite (`tests/unit/utils/worldpointer_test.cpp`) with complete types. The `is_polymorphic_if_complete` guard should be added only when `AnyPtr` is adopted in production code.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[coderabbitai]

♻️ Duplicate comments (1)

tests/shared/mounts/mounts_test_fixture.hpp (1)

47-52: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Also assert setConfigFileLua() succeeds during teardown.

If Line 48 fails, Line 51 can still reload the mounts test config and leak global state into later suites.

Suggested fix

 void TearDown() override {
-	(void)g_configManager().setConfigFileLua(previousConfigFile_);
+	EXPECT_TRUE(g_configManager().setConfigFileLua(previousConfigFile_)) << "Failed to restore previous Lua config path";
 	// Surface restore failures — otherwise broken config can leak
 	// into subsequent tests and produce flaky cross-test errors.
 	EXPECT_TRUE(g_configManager().reload()) << "Failed to restore previous Lua config";
 	std::filesystem::current_path(previousPath_);
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/shared/mounts/mounts_test_fixture.hpp` around lines 47 - 52, In
TearDown(), assert that g_configManager().setConfigFileLua(previousConfigFile_)
returns true before calling reload and restoring current_path: replace the void
cast with an EXPECT_TRUE or equivalent assertion on the result of
g_configManager().setConfigFileLua(previousConfigFile_) (inside the TearDown()
method) so failures are surfaced immediately for setConfigFileLua, then proceed
to call g_configManager().reload() and
std::filesystem::current_path(previousPath_).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@tests/shared/mounts/mounts_test_fixture.hpp`:
- Around line 47-52: In TearDown(), assert that
g_configManager().setConfigFileLua(previousConfigFile_) returns true before
calling reload and restoring current_path: replace the void cast with an
EXPECT_TRUE or equivalent assertion on the result of
g_configManager().setConfigFileLua(previousConfigFile_) (inside the TearDown()
method) so failures are surfaced immediately for setConfigFileLua, then proceed
to call g_configManager().reload() and
std::filesystem::current_path(previousPath_).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aaa85c11-01d1-46ee-b9bd-5a17fa275943

📥 Commits

Reviewing files that changed from the base of the PR and between 58e2b49 and 37420d3.

📒 Files selected for processing (7)

• src/creatures/monsters/monster.cpp
• src/game/game.cpp
• src/game/game.hpp
• src/items/tile.cpp
• src/map/map.cpp
• src/map/mapcache.cpp
• tests/shared/mounts/mounts_test_fixture.hpp

[beats-dh]

@coderabbitai fixed in 7753211 — TearDown now EXPECT_TRUE's both setConfigFileLua() and reload() so a failed path restore surfaces immediately instead of letting the subsequent reload re-apply the fixture config.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@beats-dh: Confirmed — asserting both setConfigFileLua() and reload() in TearDown means a bad path restore now fails fast and visibly rather than silently poisoning subsequent tests. Clean fix, thanks for following through on it!

(ᵔ ᗢ ᵔ)🐇✨

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[sonarqubecloud]

Quality Gate passed

Issues
175 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.1% Duplication on New Code

See analysis details on SonarQube Cloud