perf/refactor: implement player batch updates#3701
Open
Conversation
dudantas
changed the title
gabrielew
reviewed
Sep 15, 2025
dudantas
force-pushed
the
dudantas/implement-player-batch-updates
branch
from
0141e93 to
1474a48
Compare
Introduces batch update logic for containers and player inventory, refactors NPC sell logic to support batch operations, and adds new Lua talkactions for reward chest and loot pouch management. Improves container item count caching and container closing logic, and updates related tests and documentation.
dudantas
changed the title
dudantas
force-pushed
the
dudantas/implement-player-batch-updates
branch
from
1647bb3 to
37f0883
Compare
Optimized the onPlayerSellAllLoot function by restructuring item sale data handling, improving iteration over loot pouch items, and streamlining currency and sale letter processing. Enhanced error handling for store inbox operations and removed redundant code for better maintainability.
Introduces luaPlayerGetBackpack and luaPlayerGetLootPouch methods to the Player Lua API, allowing scripts to retrieve a player's backpack and loot pouch containers. Updates createitemtest.lua to use addItemBatchToPaginedContainer for improved item addition handling and logging.
Adds a message to inform the player if the sale letter could not be added to their store inbox after selling all loot.
Corrected the variable name from 'returnVale' to 'returnValue' in the internalCollectManagedItems function for clarity and consistency.
Replaces Boost.UT with Google Test in reward_iteration_test.cpp and test_batch_update.cpp. Adds new test cases for reward item iteration and batch update logic, improves test structure, and updates CMakeLists.txt to enable the reward_iteration_test.cpp target. Also enhances test_items.hpp to ensure proper item type initialization for tests.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Updated the item search in the reward chest collection logic to use std::ranges::find instead of std::find for improved readability and modern C++ practices.
Modified sendBatchUpdateContainer to call updateState only if m_batching is false. This prevents unnecessary state updates during batch operations.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
|
This PR is stale because it has been open 45 days with no activity. |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Refactored BatchUpdate to use std::shared_ptr and std::weak_ptr for Player and Container, improving memory safety and deduplication. Updated all usages and Lua bindings accordingly. Enhanced batch update logic in loot and container management, fixed related tests, and improved error handling and logging. Also made minor bug fixes and code cleanups in related files.
dudantas
force-pushed
the
dudantas/implement-player-batch-updates
branch
from
985b21c to
f5fdcd6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Corrects container cache decrement logic to prevent negative values, refactors ContainerIterator to use an internal visited set, and fixes container closing logic in Player. Adds new integration and unit tests for batch update behavior, and updates test configuration to ensure correct working directory and config file usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 765-784: The loop over saleData can leave the player in a partial
state because applySaleProceedsForLoot is called per-item and returns early on
failure; instead aggregate sale results first (e.g., accumulate per-currency
totals and build the log using saleData, totalPrice, totalItemsSold, and item
names from Item::items.getItemType) and only call applySaleProceedsForLoot once
per currency (using getCurrency() and getName() for context) after all removals
succeed; if any applySaleProceedsForLoot fails, avoid mutating player state
further (or perform a single rollback/restore) so the operation is atomic from
the player's perspective.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/players/player.cpp`:
- Around line 8076-8086: The LINK_OWNER equipment callbacks are skipped when
m_batching causes an early return in Player::postAddNotification and
Player::postRemoveNotification; move the LINK_OWNER handling out of the batching
short-circuit so equipment events always fire. Specifically, in
Player::postAddNotification call g_moveEvents().onPlayerEquip(getPlayer(),
thing->getItem(), static_cast<Slots_t>(index), false) before checking
m_batching; likewise in Player::postRemoveNotification invoke
g_moveEvents().onPlayerDeEquip(getPlayer(), thing->getItem(),
static_cast<Slots_t>(index), false) (or the existing de-equip call) prior to the
m_batching return so equip/dequip hooks run even during batch updates.
🧹 Nitpick comments (1)
src/creatures/players/player.cpp (1)
73-95: Test helper registers player globally—ensure cleanup.
initForTests()adds the player tog_game(). Consider adding a teardown helper or documenting required cleanup in tests to avoid cross‑test leakage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 249-253: The CustomSaleContext array used in
Npc::onPlayerSellAllLoot contains a mislabelled context string
"[Npc::onPlayerSellItem]"; update that entry to "[Npc::onPlayerSellAllLoot]" so
the log context accurately reflects the onPlayerSellAllLoot path (locate the
CustomSaleContext initializer in src/creatures/npcs/npc.cpp inside the
Npc::onPlayerSellAllLoot handling and replace the incorrect string).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 234-252: The CustomSaleContext passed from
applySaleProceedsForLoot is missing the fourth field, leaving failureMessage
null and risking a crash in sendTextMessage; update the call in
applySaleProceedsForLoot to provide all four string fields (e.g., first two
context labels consistent with the loot-sale path such as
"[Npc::onPlayerSellLoot]" and "[Npc::onPlayerSellAllLoot]" and a non-null
failureMessage like "An error occurred while completing the sale of your loot.
No items were exchanged.") so that CustomSaleContext is fully initialized before
calling applyCustomSaleProceeds.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 254-257: Remove the stray orphan tokens that break the anonymous
namespace: delete the extraneous ");" and the premature "}" currently at the end
of the block so that applySaleProceedsForItem and sendSaleLetterIfNeeded remain
inside the anonymous namespace and can access helpers like applyGoldSaleProceeds
and applyCustomSaleProceeds; ensure the anonymous namespace is closed only once
after sendSaleLetterIfNeeded (the intended closing brace at the original end of
the helper block).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Updated cost calculation functions in npc.cpp to use uint64_t for bagsCost and totalCost to prevent overflow. Adjusted the cleanup lambda and guard in rsa_test.cpp to use the correct pointer type, improving type safety.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The balance_decrease metric is now only incremented after confirming the player has sufficient funds and the purchase is in gold coins. This prevents logging failed purchase attempts as successful balance decreases.
|
Contributor
|
This PR is stale because it has been open 45 days with no activity. |
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/creatures/npcs/npc.cpp (3)
776-777:⚠️ Potential issue | 🟡 MinorLoot-sale messages still hardcode “gold” for non-gold currencies.
Lines 777 and 800 are misleading when
getCurrency()is notITEM_GOLD_COIN.🛠️ Suggested fix
- const auto &itemName = Item::items.getItemType(itemId).name; - log += fmt::format("Sold {}x {} for {} gold.\n", data.amount, itemName, totalCost); + const auto &itemName = Item::items.getItemType(itemId).name; + const auto ¤cyName = Item::items.getItemType(getCurrency()).name; + log += fmt::format("Sold {}x {} for {} {}.\n", data.amount, itemName, totalCost, currencyName); @@ - finalMessage = fmt::format("You sold {} item{} from your loot pouch for {} gold. A letter with the full list has been sent to your store inbox.", totalItemsSold, (totalItemsSold == 1 ? "" : "s"), totalPrice); + const auto ¤cyName = Item::items.getItemType(getCurrency()).name; + finalMessage = fmt::format("You sold {} item{} from your loot pouch for {} {}. A letter with the full list has been sent to your store inbox.", totalItemsSold, (totalItemsSold == 1 ? "" : "s"), totalPrice, currencyName);Also applies to: 800-800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/creatures/npcs/npc.cpp` around lines 776 - 777, The loot-sale log hardcodes the word "gold" when composing messages (see the format call that uses itemName and totalCost); change the message to use the actual currency name returned by getCurrency() instead of "gold": fetch the currency item/type name (e.g., via Item::items.getItemType(getCurrency()).name or the project's Currency/name accessor) and use that variable in the fmt::format calls that build the "Sold ... for ... gold." lines (references: Item::items.getItemType(itemId).name, data.amount, totalCost, getCurrency(), ITEM_GOLD_COIN).
664-667:⚠️ Potential issue | 🟠 MajorUse 64-bit arithmetic for purchase total to avoid overflow.
Line 664 computes
buyPrice * amountin 32-bit space before any widening. Large values can wrap and undercharge while still passing funds checks.🛠️ Suggested fix
- const uint32_t totalCost = buyPrice * amount; - const uint32_t bagsCost = calculateBagsCost(itemType, amount, inBackpacks); - const uint64_t totalRequired = static_cast<uint64_t>(totalCost) + bagsCost; + const uint64_t totalCost = static_cast<uint64_t>(buyPrice) * static_cast<uint64_t>(amount); + const uint64_t bagsCost = calculateBagsCost(itemType, amount, inBackpacks); + const uint64_t totalRequired = totalCost + bagsCost; if (hasInsufficientFunds(player, itemId, getName(), getCurrency(), totalCost, bagsCost)) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/creatures/npcs/npc.cpp` around lines 664 - 667, The multiplication buyPrice * amount is done in 32-bit and can overflow; change computation to 64-bit by computing totalCost as uint64_t totalCost = static_cast<uint64_t>(buyPrice) * amount (or similar widening), keep bagsCost as computed by calculateBagsCost, compute totalRequired accordingly, and pass the 64-bit totals into hasInsufficientFunds (or cast in the call) so all comparisons use 64-bit arithmetic (update variable types or casts for totalCost/totalRequired and the hasInsufficientFunds invocation to use uint64_t).
828-842:⚠️ Potential issue | 🔴 CriticalCustom-currency sale path can remove items before credit is guaranteed.
Lines 828-830 remove items first, then Lines 841-842 abort when proceeds application fails. In that failure path, items are already gone, but the flow reports the sale as aborted. Please make this transactional (rollback removed items on credit failure, or pre-validate/apply proceeds before destructive removal).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/creatures/npcs/npc.cpp` around lines 828 - 842, The code currently removes items via removeItemsFromLootPouch or removeItemsFromInventory before ensuring currency credit via applySaleProceedsForItem, so if applySaleProceedsForItem fails the items are already gone; change the flow to be transactional by either (A) pre-validating/applying proceeds (call applySaleProceedsForItem first or a new validateAndReserveProceeds step) and only removing items after success, or (B) if you must remove first, capture the removed items (their ids/amounts) and, on applySaleProceedsForItem failure, call a restore function to put items back into the player/loot pouch using the same context and batchUpdate; update the code paths referencing removeItemsFromLootPouch, removeItemsFromInventory, applySaleProceedsForItem, getCurrency, getName, and context to implement one of these transactional approaches so failure does not leave the player missing items.
🧹 Nitpick comments (1)
src/game/game.cpp (1)
3284-3299: Batch source containers lazily after filters to reduce unnecessary updatesAt Lines 3284-3299 all parent containers are added to
BatchUpdatebefore quick-loot filters/capacity checks. Containers for skipped items are still batched, which can trigger avoidable container refresh work at batch end.Also applies to: 3305-3319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/game/game.cpp` around lines 3284 - 3299, The code currently adds every parent container to BatchUpdate up-front (BatchUpdate batchUpdate / loop over rewardItemsVector), causing containers for items that will be skipped by quick-loot filters or capacity checks to be needlessly batched; change the logic to add containers lazily after applying the item-level filters/capacity checks (i.e., only call batchUpdate.add(container) once you know the item will be processed), and ensure you avoid duplicate adds (use the existing BatchUpdate.dedup behavior or maintain a local set keyed by container pointer/ID) so only necessary containers are refreshed at batch end; apply the same change to the analogous loop handling the later rewardItemsVector block.
🤖 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 3325-3330: The code sends a specific lootedItemsMessage via
player->sendTextMessage when maxMoveItems limit is reached but then returns
RETURNVALUE_NOTPOSSIBLE which triggers the caller to send a second generic
cancel message; update the return to a non-canceling success code (e.g.,
RETURNVALUE_NOERROR) from the block that contains maxMoveItems,
movedRewardItems, rewardCount and lootedItemsMessage so the caller does not emit
the duplicate generic message—ensure you only change the return value after the
player->sendTextMessage call for that condition.
- Around line 3273-3274: The code dereferences rewardChest returned from
player->getRewardChest() without null checking; modify the logic around the
rewardChest variable (the local std::shared_ptr<RewardChest> rewardChest =
player->getRewardChest()) to first test rewardChest for null (e.g., if
(!rewardChest) { /* handle absent chest: return, skip, or create one */ })
before calling rewardChest->empty(), ensuring the rest of the branch handles the
absent-chest case consistently with other uses in this file.
---
Duplicate comments:
In `@src/creatures/npcs/npc.cpp`:
- Around line 776-777: The loot-sale log hardcodes the word "gold" when
composing messages (see the format call that uses itemName and totalCost);
change the message to use the actual currency name returned by getCurrency()
instead of "gold": fetch the currency item/type name (e.g., via
Item::items.getItemType(getCurrency()).name or the project's Currency/name
accessor) and use that variable in the fmt::format calls that build the "Sold
... for ... gold." lines (references: Item::items.getItemType(itemId).name,
data.amount, totalCost, getCurrency(), ITEM_GOLD_COIN).
- Around line 664-667: The multiplication buyPrice * amount is done in 32-bit
and can overflow; change computation to 64-bit by computing totalCost as
uint64_t totalCost = static_cast<uint64_t>(buyPrice) * amount (or similar
widening), keep bagsCost as computed by calculateBagsCost, compute totalRequired
accordingly, and pass the 64-bit totals into hasInsufficientFunds (or cast in
the call) so all comparisons use 64-bit arithmetic (update variable types or
casts for totalCost/totalRequired and the hasInsufficientFunds invocation to use
uint64_t).
- Around line 828-842: The code currently removes items via
removeItemsFromLootPouch or removeItemsFromInventory before ensuring currency
credit via applySaleProceedsForItem, so if applySaleProceedsForItem fails the
items are already gone; change the flow to be transactional by either (A)
pre-validating/applying proceeds (call applySaleProceedsForItem first or a new
validateAndReserveProceeds step) and only removing items after success, or (B)
if you must remove first, capture the removed items (their ids/amounts) and, on
applySaleProceedsForItem failure, call a restore function to put items back into
the player/loot pouch using the same context and batchUpdate; update the code
paths referencing removeItemsFromLootPouch, removeItemsFromInventory,
applySaleProceedsForItem, getCurrency, getName, and context to implement one of
these transactional approaches so failure does not leave the player missing
items.
---
Nitpick comments:
In `@src/game/game.cpp`:
- Around line 3284-3299: The code currently adds every parent container to
BatchUpdate up-front (BatchUpdate batchUpdate / loop over rewardItemsVector),
causing containers for items that will be skipped by quick-loot filters or
capacity checks to be needlessly batched; change the logic to add containers
lazily after applying the item-level filters/capacity checks (i.e., only call
batchUpdate.add(container) once you know the item will be processed), and ensure
you avoid duplicate adds (use the existing BatchUpdate.dedup behavior or
maintain a local set keyed by container pointer/ID) so only necessary containers
are refreshed at batch end; apply the same change to the analogous loop handling
the later rewardItemsVector block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f61b2f7-56f2-43d1-afd0-d2581f19d271
📒 Files selected for processing (6)
src/creatures/creature.hppsrc/creatures/npcs/npc.cppsrc/creatures/npcs/npc.hppsrc/game/game.cppsrc/game/game.hppsrc/items/containers/container.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/game/game.hpp
Comment on lines
+3273
to
3274
| std::shared_ptr<RewardChest> rewardChest = player->getRewardChest(); | ||
| if (rewardChest->empty()) { |
There was a problem hiding this comment.
Guard reward chest pointer before dereference
At Line 3274, rewardChest is dereferenced without a null check. player->getRewardChest() is nullable in this file (e.g., guarded at Line 11028), so this can crash.
🔧 Proposed fix
- std::shared_ptr<RewardChest> rewardChest = player->getRewardChest();
- if (rewardChest->empty()) {
+ std::shared_ptr<RewardChest> rewardChest = player->getRewardChest();
+ if (!rewardChest || rewardChest->empty()) {
g_logger().debug("Reward chest is empty");
return RETURNVALUE_REWARDCHESTISEMPTY;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/game/game.cpp` around lines 3273 - 3274, The code dereferences
rewardChest returned from player->getRewardChest() without null checking; modify
the logic around the rewardChest variable (the local
std::shared_ptr<RewardChest> rewardChest = player->getRewardChest()) to first
test rewardChest for null (e.g., if (!rewardChest) { /* handle absent chest:
return, skip, or create one */ }) before calling rewardChest->empty(), ensuring
the rest of the branch handles the absent-chest case consistently with other
uses in this file.
Comment on lines
+3325
to
+3330
| if (maxMoveItems && movedRewardItems == maxMoveItems) { | ||
| lootedItemsMessage = fmt::format("You can only collect {} items at a time. {} of {} objects were picked up.", maxMoveItems, movedRewardItems, rewardCount); | ||
| player->sendTextMessage(MESSAGE_EVENT_ADVANCE, lootedItemsMessage); | ||
| // Already send message here | ||
| return RETURNVALUE_NOTPOSSIBLE; | ||
| } |
There was a problem hiding this comment.
Avoid duplicate client messaging on max-item limit
At Line 3326 you already send a specific message, but at Line 3329 you return RETURNVALUE_NOTPOSSIBLE, and the caller sends another generic cancel message. This creates noisy double feedback.
🔧 Proposed fix
if (maxMoveItems && movedRewardItems == maxMoveItems) {
lootedItemsMessage = fmt::format("You can only collect {} items at a time. {} of {} objects were picked up.", maxMoveItems, movedRewardItems, rewardCount);
player->sendTextMessage(MESSAGE_EVENT_ADVANCE, lootedItemsMessage);
// Already send message here
- return RETURNVALUE_NOTPOSSIBLE;
+ return RETURNVALUE_NOERROR;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/game/game.cpp` around lines 3325 - 3330, The code sends a specific
lootedItemsMessage via player->sendTextMessage when maxMoveItems limit is
reached but then returns RETURNVALUE_NOTPOSSIBLE which triggers the caller to
send a second generic cancel message; update the return to a non-canceling
success code (e.g., RETURNVALUE_NOERROR) from the block that contains
maxMoveItems, movedRewardItems, rewardCount and lootedItemsMessage so the caller
does not emit the duplicate generic message—ensure you only change the return
value after the player->sendTextMessage call for that condition.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors and fixes NPC buy/sell logic, loot pouch selling, and the container batch update system, ensuring correct validation, safer transactions, and reduced client update spam.
Motivation / Context
Behaviour
Actual
Expected
Type of change
How Has This Been Tested
1) NPC Buy (gold and custom currency)
Backpack full
Attempt to buy a non-container item with no free slots.
Expected: purchase is blocked.
Ignore enabled + tile limit
Buy with
ignore=trueuntil exceeding the 30-item tile limit (considering required slots and free slots).Expected: purchase is blocked.
Shopping bag cost
Buy with
inBackpacks=trueand verify bag cost calculation for:Insufficient funds
totalCost→ purchase blocked.totalCost, or insufficient gold/bank to pay for bags → purchase blocked.2) NPC Sell (regular inventory)
Valid item sale
Sell an item without tier or imbuements.
Expected: correct total calculation and credit to bank or inventory gold (depending on
AUTOBANK).Custom currency sale
Validate custom currency delivery in stacks.
If currency creation or insertion fails:
Expected: error message is shown and transaction is aborted.
Invalid or zero-amount items
Attempt to sell non-sellable items or zero quantity.
Expected: no sale occurs and message “You have no items to sell.” is shown when applicable.
3) Loot pouch selling
Loot pouch sale trigger
Use
ITEM_GOLD_POUCH.Expected: triggers batch sale of loot pouch contents.
Tiered / imbued items ignored
Items with
tier > 0or imbuements are not sold.Custom currency during loot sale
Validate correct delivery.
On failure:
Expected: sale is aborted and player is informed.
Store inbox letter
When a sale occurs, confirm a log letter is sent to the store inbox.
4) Container batch update & synchronization
Batch update lifecycle
Verify
beginBatchUpdate/endBatchUpdateare balanced and duplicated containers are not reprocessed.Updates during batching
Ensure item updates are suppressed while
m_batchingis active.Drop destination (gray area)
Move an item to an index outside container capacity.
Expected: destination resolves to
INDEX_WHEREEVER.Parent notification on removal
Removing an item by index correctly notifies the parent container.
Checklist
Summary by CodeRabbit
/addrewardcommand for adding items to timestamped reward chests./createloot,/clearloot,/createtestshop,/countloot, and/addlootfor administrative loot control.