Uh oh!
There was an error while loading. Please reload this page.
Fix item command and fix InventoryManager.Add - #576
Conversation
Caution Review failedThe pull request is closed. WalkthroughSeparates drop vs give flows: ItemCommand now uses DropItem and GiveItem helpers with explicit drop rules; InventoryManager.Add is rewritten to multi‑step stacking with a session.Field null guard and explicit ItemDrop.CreateItem usage; field spawn/drop APIs consolidated to new SpawnItem/DropItem signatures and callers updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Command Invoker
participant Cmd as ItemCommand
participant Sess as Session
participant ItemDrop as ItemDrop
participant Field as Field
participant Inv as InventoryManager
Admin->>Cmd: invoke /item (drop mode)
Cmd->>Sess: resolve session, metadata
alt Non-stackable (SlotMax == 1)
Cmd->>Cmd: clamp amount to 100
loop each unit up to cap
Cmd->>ItemDrop: CreateItem(id, rarity, 1)
ItemDrop-->>Cmd: Item
Cmd->>Field: DropItem(position/owner, Item)
end
else Stackable
Cmd->>ItemDrop: CreateItem(id, rarity, clamp(amount, SlotMax))
ItemDrop-->>Cmd: Item
Cmd->>Field: DropItem(position/owner, Item)
end
Cmd-->>Admin: ExitCode 0
sequenceDiagram
autonumber
actor Admin as Command Invoker
participant Cmd as ItemCommand
participant Sess as Session
participant Inv as InventoryManager
participant ItemDrop as ItemDrop
Admin->>Cmd: invoke /item (give mode)
Cmd->>Sess: resolve session, metadata
Cmd->>Inv: GiveItem(ctx, item)
Inv->>Sess: check session.Field != null
alt field null
Inv-->>Cmd: warn + false
Cmd-->>Admin: ExitCode 1
else
Inv->>Inv: determine slotMax, fill partial stacks
Inv->>Inv: compute fullStacks and remainder
alt insufficient free slots
Inv-->>Cmd: send inventory error
Cmd-->>Admin: ExitCode 1
else
loop each stack chunk
Inv->>ItemDrop: CreateItem(id, rarity, chunkAmount)
ItemDrop-->>Inv: Item
Inv-->>Cmd: add Item (success)
end
Cmd-->>Admin: ExitCode 0
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)
179-221: Check Add return in partial-fill loop
In the partial-stack filling loop you callAdd(itemToStack, notifyNew, commit)without checking its return value, so a database failure there would be ignored and lead to lost items—change it toif(!Add(itemToStack,notifyNew,commit))returnfalse;to prevent silent failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/Commands/ItemCommand.cs(2 hunks)Maple2.Server.Game/Manager/Items/InventoryManager.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Maple2.Server.Game/Commands/ItemCommand.cs (4)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (4)
Item(591-599)FreeSlots(579-583)Add(149-276)Discard(730-749)Maple2.Server.Game/Manager/Items/ItemDropManager.cs (1)
Item(294-316)Maple2.Model/ModelExtensions.cs (1)
IsCurrency(87-87)Maple2.Server.Game/Packets/FieldPacket.cs (1)
FieldPacket(19-353)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (4)
Maple2.Server.Game/Manager/Items/ItemDropManager.cs (1)
Item(294-316)Maple2.Model/ModelExtensions.cs (1)
IsExpired(94-94)Maple2.Server.Core/Network/Session.cs (2)
Send(145-145)Send(147-147)Maple2.Server.Game/Packets/ItemInventoryPacket.cs (1)
ItemInventoryPacket(11-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)
151-154: Field null guard prevents additions outside of field context.The guard correctly rejects additions when
session.Fieldis null. This is necessary because the new stacking logic relies onsession.Field.ItemDrop.CreateItemfor item creation (lines 190, 206, 214).Maple2.Server.Game/Commands/ItemCommand.cs (8)
7-7: LGTM!Added import for
ItemInventoryErroris necessary for the error handling at line 85.
45-45: Direct field access is safe due to guard at line 41.The change from optional access (
session.Field?.ItemDrop.CreateItem) to direct access (session.Field.ItemDrop.CreateItem) is safe because of the null check at line 41 that returns early ifsession.Fieldis null.
51-55: Currency handling uses GiveItem helper correctly.The currency path now delegates to
GiveItem, which properly handles addition failures by discarding the item and logging an error.
59-80: Drop logic correctly handles stackable vs non-stackable items.The drop path distinguishes between:
- Non-stackable items (SlotMax == 1): Drops up to 100 individual items
- Stackable items: Drops a single stack clamped to SlotMax
The 100-item cap for non-stackable drops prevents spam/abuse.
82-103: Free slots check for non-stackable items prevents inventory overflow.The logic:
- Checks available free slots before attempting to add non-stackable items
- Clamps the amount to available slots
- Uses
GiveItemwhich aborts on failureHowever, there's a potential issue: the
FreeSlotscheck is performed once at the beginning, but the loop doesn't re-check after each successful addition. If another concurrent operation consumes slots, subsequent additions could fail.This is a minor edge case since the code is wrapped in a lock at the InventoryManager level, but the failure handling via
GiveItemreturning false will abort the operation.
104-109: Stackable item handling correctly delegates to Add method.For stackable items, the amount is set directly on the item and
GiveItemis called once. This delegates the stack splitting logic toInventoryManager.Add, which handles the multi-step stacking.
119-123: DropItem helper correctly spawns and broadcasts item drops.The helper properly:
- Guards against null field
- Spawns the item in the field
- Broadcasts the drop event to all players
125-131: GiveItem helper properly handles inventory addition failures.The helper:
- Attempts to add the item to inventory with
notifyNew = true- On failure, discards the item (preventing leaks)
- Logs an error message to the console
- Sets exit code and returns false to abort
This ensures proper cleanup and error reporting.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (3)
198-205: Open slots check should precede partial stack fills.The
stacksNeededcalculation and open slots validation (lines 198-205) occur after attempting to fill partial stacks (lines 185-196). If the inventory lacks sufficient space, some partial stacks may already have been added before the error is detected.While the error handling in lines 192-195 prevents silent failures during partial fills, moving the capacity check before line 185 would avoid unnecessary work and provide clearer early validation.
Consider this refactor to validate capacity upfront:
+ // Calculate space requirements upfront+ int existingPartialSlots = items.Count(x => x.Id == add.Id && x.Rarity == add.Rarity && x.Amount < slotMax && !x.IsExpired());+ int maxFillableFromPartials = existingPartialSlots > 0 + ? items.Where(x => x.Id == add.Id && x.Rarity == add.Rarity && x.Amount < slotMax && !x.IsExpired())+ .Sum(x => slotMax - x.Amount)+ : 0;+ int amountAfterPartials = Math.Max(0, totalAmount - maxFillableFromPartials);+ int fullStacks = amountAfterPartials / slotMax;+ int remainder = amountAfterPartials % slotMax;+ int stacksNeeded = fullStacks + (remainder > 0 ? 1 : 0);++ if (items.OpenSlots < stacksNeeded) {+ session.Send(ItemInventoryPacket.Error(s_err_inventory));+ return false;+ }+ // 1. Fill partially filled stacks first foreach (Item existing in items.Where(x => x.Id == add.Id && x.Rarity == add.Rarity && x.Amount < slotMax && !x.IsExpired())) {Based on learnings
207-213: Pass amount directly to CreateItem for consistency.Lines 209-211 create an item with the default amount (1) and then manually override it to
slotMax. This is inconsistent with the partial stack creation at line 190, which passes the amount directly:CreateItem(add.Id, add.Rarity, toFill).For clarity and consistency, pass
slotMaxas theamountparameter toCreateItem.Apply this diff:
// 2. Add full stacks for (int i = 0; i < fullStacks; i++) { - Item? stackItem = session.Field.ItemDrop.CreateItem(add.Id, add.Rarity);+ Item? stackItem = session.Field.ItemDrop.CreateItem(add.Id, add.Rarity, slotMax); if (stackItem is null) return false; - stackItem.Amount = slotMax; if (!Add(stackItem, notifyNew, commit)) return false; }
216-221: Pass amount directly to CreateItem for remainder stack.Similar to the full stacks, lines 217-219 create an item and then manually set the amount to
remainder. For consistency with line 190, passremainderdirectly toCreateItem.Apply this diff:
// 3. Add remainder stack if (remainder > 0) { - Item? remainderItem = session.Field.ItemDrop.CreateItem(add.Id, add.Rarity);+ Item? remainderItem = session.Field.ItemDrop.CreateItem(add.Id, add.Rarity, remainder); if (remainderItem is null) return false; - remainderItem.Amount = remainder; if (!Add(remainderItem, notifyNew, commit)) return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Manager/Items/InventoryManager.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (4)
Maple2.Server.Game/Manager/Items/ItemDropManager.cs (1)
Item(294-316)Maple2.Model/ModelExtensions.cs (1)
IsExpired(94-94)Maple2.Server.Core/Network/Session.cs (2)
Send(145-145)Send(147-147)Maple2.Server.Game/Packets/ItemInventoryPacket.cs (1)
ItemInventoryPacket(11-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (2)
151-154: Good defensive guard.The null check for
session.Fieldprevents potential null reference exceptions when callingsession.Field.ItemDrop.CreateItemin the stacking logic below.
185-196: Error handling improved for partial stack fills.The code now properly checks the return value of recursive
Addcalls (lines 192-195) and returnsfalsewith logging on failure, addressing the silent failure concern from previous reviews.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Maple2.Server.Game/Commands/ItemCommand.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs(1 hunks)Maple2.Server.Game/Manager/MasteryManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs(1 hunks)Maple2.Server.Game/PacketHandlers/BreakableHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs(3 hunks)Maple2.Server.Game/PacketHandlers/InteractObjectHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs(1 hunks)Maple2.Server.Game/Trigger/TriggerContext.Field.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (2)
DropItem(76-76)DropItem(77-77)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (4)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (2)
DropItem(76-76)DropItem(77-77)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs (1)
DropItem(91-118)Maple2.Server.Game/Model/Field/Actor/FieldPet.cs (1)
DropItem(92-111)
Maple2.Server.Game/PacketHandlers/InteractObjectHandler.cs (4)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (2)
DropItem(76-76)DropItem(77-77)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs (1)
DropItem(91-118)Maple2.Server.Game/Model/Field/Actor/FieldPet.cs (1)
DropItem(92-111)
Maple2.Server.Game/Manager/MasteryManager.cs (3)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (2)
DropItem(76-76)DropItem(77-77)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs (1)
DropItem(91-118)
Maple2.Server.Game/PacketHandlers/BreakableHandler.cs (3)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (2)
DropItem(76-76)DropItem(77-77)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs (1)
DropItem(91-118)
Maple2.Server.Game/Commands/ItemCommand.cs (5)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (4)
Item(594-602)FreeSlots(582-586)Add(149-279)Discard(733-752)Maple2.Server.Game/Manager/Items/ItemDropManager.cs (1)
Item(294-316)Maple2.Model/ModelExtensions.cs (1)
IsCurrency(87-87)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (2)
DropItem(76-76)DropItem(77-77)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (3)
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (5)
FieldItem(73-73)FieldItem(74-74)DropItem(76-76)DropItem(77-77)Broadcast(66-66)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
Broadcast(695-704)Maple2.Server.Game/Packets/FieldPacket.cs (1)
FieldPacket(19-353)
Maple2.Server.Game/Trigger/TriggerContext.Field.cs (4)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (2)
DropItem(76-76)DropItem(77-77)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs (1)
DropItem(91-118)Maple2.Server.Game/Model/Field/Actor/FieldPet.cs (1)
DropItem(92-111)
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (1)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (4)
FieldItem(247-262)FieldItem(264-276)DropItem(278-281)DropItem(283-286)
Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs (5)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
DropItem(278-281)DropItem(283-286)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (3)
DropItem(76-76)DropItem(77-77)Broadcast(66-66)Maple2.Database/Storage/Game/GameStorage.Nurturing.cs (1)
UpdateNurturing(33-49)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
Broadcast(695-704)Maple2.Server.Game/Packets/FunctionCubePacket.cs (1)
FunctionCubePacket(11-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
Maple2.Server.Game/Manager/MasteryManager.cs (1)
210-211: DropItem usage maintains gather ownership. Passing the characterId into the centralized helper keeps gather rewards instanced while leveraging the shared drop path.Maple2.Server.Game/Trigger/TriggerContext.Field.cs (2)
419-420: DropItem centralization retains fixed-position drops. Routing event spawn rewards throughDropItem(... fixedPosition: true)keeps placement behavior unchanged while reusing the new helper.
445-446: Consistent drop handling for range spawns. This mirrors the earlier loop so range-triggered rewards also benefit from the unified DropItem flow.Maple2.Server.Game/PacketHandlers/InteractObjectHandler.cs (2)
98-101: Drop path alignment looks good. Setting the interact object as owner and forwarding the characterId preserves instanced drops while using the centralized helper.
104-107: Secondary drop path mirrors the new helper. The individual-box rewards get the same ownership treatment, so both flows stay consistent.Maple2.Server.Game/PacketHandlers/BreakableHandler.cs (1)
50-51: Breakable drop now flows through DropItem. Supplying the breakable as owner together with the characterId keeps attribution intact while simplifying the broadcast.Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs (3)
126-127: Stage reward leverages unified drop path. UsingDropItemwith the player owner keeps the reward instanced as before without manual spawn/broadcast plumbing.
150-156: Feed reward flow stays coherent. The drop helper plus reusingrewardItem.Uidfor the feed packet preserves client updates while sharing the new drop logic.
183-184: Play reward drop matches the other branches. This mirrors the nurturing paths so every reward now funnels through the centralized helper.
Uh oh!
There was an error while loading. Please reload this page.
Summary by CodeRabbit
New Features
Bug Fixes