improve: allow loot pouch reordering in Store Inbox#3872
improve: allow loot pouch reordering in Store Inbox#3872
Conversation
Special-case gold pouch (ITEM_GOLD_POUCH) moves to allow internal reordering inside Store Inbox containers. Add checks to recognize when both source and destination are Store Inbox (including root parent) and bypass the normal non-movable restriction by passing a move flag (FLAG_IGNORENOTMOVABLE) to internalMoveItem. Update checkMoveItemToCylinder to permit gold pouch internal moves and to apply stricter checks for non-store-movable items only when appropriate (direct Store Inbox drops vs. top-parent Store Inbox reorganizations). Also enable valid-move detection for items moved into Store Inbox parent containers.
dudantas
changed the title
📝 WalkthroughWalkthroughThis change updates item-move validation to recognize store-inbox contexts and adjust movability rules for loot containers (notably GOLD_POUCH). It permits loot-pouch reordering inside store-inbox contexts, propagates store-inbox awareness through movability checks, and applies moveFlags for specific internal reorders. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reformatted a multi-line comment in game.cpp regarding Store Inbox reorganization into a single-line comment. This is a non-functional change intended to improve code readability.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/game/game.cpp (1)
1892-1907: Consider centralizing Store Inbox context detection.The Store Inbox context predicates are duplicated here and in
checkMoveItemToCylinder, which raises drift risk (e.g., slightly differentfromIsStoreInboxlogic). A small helper (e.g.,isStoreInboxContext(Container*)) would keep the rules consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/game/game.cpp` around lines 1892 - 1907, The Store Inbox detection logic is duplicated between this block (using fromContainer/fromRoot and computing fromIsStoreInbox/toIsStoreInbox) and checkMoveItemToCylinder, risking inconsistencies; extract that predicate into a single helper (e.g., isStoreInboxContext(Container* container) or isStoreInboxContext(const Cylinder* cyl)) and replace the local computations here (references: fromContainer, toContainer, fromRoot, toRoot, fromIsStoreInbox, toIsStoreInbox, isLootPouchStoreInboxReorder) and inside checkMoveItemToCylinder to call the helper so both sites share the same rule and reduce drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/game/game.cpp`:
- Around line 1965-1982: playerMoveItem and moveItem use different predicates to
detect a Store Inbox which causes inconsistent acceptance of loot pouch moves;
update moveItem so its fromIsStoreInbox calculation matches playerMoveItem by
checking both fromContainer->isStoreInbox() and (fromRoot &&
fromRoot->isStoreInbox()) instead of only fromRoot, i.e. compute fromContainer,
fromRoot and set fromIsStoreInbox = (fromContainer &&
fromContainer->isStoreInbox()) || (fromRoot && fromRoot->isStoreInbox()); ensure
this change is applied to the fromIsStoreInbox variable used in the
ITEM_GOLD_POUCH and directIsStoreInbox checks in moveItem.
---
Nitpick comments:
In `@src/game/game.cpp`:
- Around line 1892-1907: The Store Inbox detection logic is duplicated between
this block (using fromContainer/fromRoot and computing
fromIsStoreInbox/toIsStoreInbox) and checkMoveItemToCylinder, risking
inconsistencies; extract that predicate into a single helper (e.g.,
isStoreInboxContext(Container* container) or isStoreInboxContext(const Cylinder*
cyl)) and replace the local computations here (references: fromContainer,
toContainer, fromRoot, toRoot, fromIsStoreInbox, toIsStoreInbox,
isLootPouchStoreInboxReorder) and inside checkMoveItemToCylinder to call the
helper so both sites share the same rule and reduce drift.
| const bool topParentIsStoreInbox = topParentContainer && topParentContainer->isStoreInbox(); | ||
| const bool directIsStoreInbox = (containerID == ITEM_STORE_INBOX); | ||
| const auto fromContainer = fromCylinder->getContainer(); | ||
| const auto fromRoot = fromContainer ? fromContainer->getRootContainer() : nullptr; | ||
| const bool fromIsStoreInbox = fromRoot && fromRoot->isStoreInbox(); | ||
| const bool toIsStoreInbox = directIsStoreInbox || topParentIsStoreInbox; | ||
|
|
||
| if (item->getID() == ITEM_GOLD_POUCH && !containerToStow) { | ||
| // Loot pouch can only be moved inside Store Inbox (internal reordering). | ||
| if (!(fromIsStoreInbox && toIsStoreInbox)) { | ||
| return RETURNVALUE_ITEMCANNOTBEMOVEDTHERE; | ||
| } | ||
| } | ||
|
|
||
| if (!item->canBeMovedToStore()) { | ||
| if (directIsStoreInbox) { | ||
| if (!(fromIsStoreInbox && (item->isMovable() || item->getID() == ITEM_GOLD_POUCH))) { | ||
| return RETURNVALUE_NOTBOUGHTINSTORE; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Inspect getRootContainer and isStoreInbox implementations
rg -n "getRootContainer\s*\(" -S src -A 10 --max-count=5Repository: opentibiabr/canary
Length of output: 3927
🏁 Script executed:
# Find isStoreInbox implementation
rg -n "isStoreInbox\s*\(" -S src -A 3 --max-count=10Repository: opentibiabr/canary
Length of output: 3640
🏁 Script executed:
# Find playerMoveItem function
rg -n "playerMoveItem" -S src --max-count=5Repository: opentibiabr/canary
Length of output: 1270
🏁 Script executed:
# Get getTopParentContainer implementation
rg -n "getTopParentContainer\s*\(" -S src -A 10 --max-count=5Repository: opentibiabr/canary
Length of output: 3027
🏁 Script executed:
# Check if Store Inbox can be nested or what its typical parent structure is
rg -n "ITEM_STORE_INBOX" -S src --max-count=15Repository: opentibiabr/canary
Length of output: 1490
Align Store Inbox detection between playerMoveItem and moveItem functions.
A semantic mismatch exists: playerMoveItem (line 1896) checks fromContainer && (fromContainer->isStoreInbox() || (fromRoot && fromRoot->isStoreInbox())), while moveItem (line 1969) checks only fromRoot && fromRoot->isStoreInbox(). Since moveItem's check is more restrictive, it will reject loot pouch reorders (line 1974) that playerMoveItem would allow. Both functions must use consistent predicates for Store Inbox detection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/game/game.cpp` around lines 1965 - 1982, playerMoveItem and moveItem use
different predicates to detect a Store Inbox which causes inconsistent
acceptance of loot pouch moves; update moveItem so its fromIsStoreInbox
calculation matches playerMoveItem by checking both
fromContainer->isStoreInbox() and (fromRoot && fromRoot->isStoreInbox()) instead
of only fromRoot, i.e. compute fromContainer, fromRoot and set fromIsStoreInbox
= (fromContainer && fromContainer->isStoreInbox()) || (fromRoot &&
fromRoot->isStoreInbox()); ensure this change is applied to the fromIsStoreInbox
variable used in the ITEM_GOLD_POUCH and directIsStoreInbox checks in moveItem.
1 participant
This pull request updates the logic for moving items—particularly the gold pouch—between containers, with a focus on special handling for the Store Inbox. The changes ensure that gold pouches can be reordered within the Store Inbox even if they are not normally movable, and strengthen checks to prevent moving items into or within the Store Inbox unless specific conditions are met.
Key changes:
Special handling for gold pouch and Store Inbox:
playerMoveItemto allow the gold pouch (ITEM_GOLD_POUCH) to be reordered within the Store Inbox even if it is not pushable, by setting a special move flag (FLAG_IGNORENOTMOVABLE). This ensures internal organization is possible for this item in the Store Inbox.checkMoveItemToCylinder, added logic to restrict the gold pouch so it can only be moved inside the Store Inbox (i.e., internal reordering), blocking moves to or from other containers.Stricter Store Inbox movement checks:
checkMoveItemToCylinderto ensure items can only be moved to the Store Inbox if they are eligible (movable or gold pouch) and originate from within the Store Inbox, preventing unauthorized moves.These changes collectively reinforce item movement rules, particularly for the Store Inbox and gold pouch, improving both security and user experience for inventory management.
Summary by CodeRabbit