Roll-max multiple item fix - #495
Conversation
Fixes it so roll-max applies to multiple items rather than just 1
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ItemCommand
participant ItemFactory
participant Inventory
participant World
User->>ItemCommand: Invoke Handle()
ItemCommand->>ItemFactory: Create Item(s)
alt Item is currency
ItemCommand->>Inventory: Add currency
else Non-currency
alt Non-stackable or rollMax
loop For each item
ItemCommand->>ItemFactory: Create Item
ItemCommand->>ItemCommand: ProcessSingleItem()
end
else
ItemCommand->>ItemCommand: ProcessSingleItem()
end
end
ItemCommand->>User: Set exit code (0 or 1)
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Maple2.Server.Game/Commands/ItemCommand.cs (1)
59-64:SlotMax == 0path clamps only the lower bound.
Math.Clamp(amount, 1, int.MaxValue)does nothing except force a minimum of 1.
If the caller mistakenly passes a negative value the earlier check already covers it; otherwise the call is redundant.
You can drop the clamp (or clamp to a reasonable upper bound, e.g. player inventory slots).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Commands/ItemCommand.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: format
- GitHub Check: build
- GitHub Check: build
| if (drop && session.Field != null) { | ||
| FieldItem fieldItem = session.Field.SpawnItem(session.Player, item); | ||
| session.Field.Broadcast(FieldPacket.DropItem(fieldItem)); | ||
| } else if (!session.Item.Inventory.Add(item, true)) { | ||
| session.Item.Inventory.Discard(item); | ||
| ctx.Console.Error.WriteLine($"Failed to add item:{item.Id} to inventory"); | ||
| ctx.ExitCode = 1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Early-exit after inventory failure to prevent partial success confusion.
If adding to inventory fails, the command keeps processing subsequent items, producing a mix of success & failure with a single exit code.
Consider aborting once an error is detected.
- session.Item.Inventory.Discard(item);
- ctx.Console.Error.WriteLine($"Failed to add item:{item.Id} to inventory");
- ctx.ExitCode = 1;
+ session.Item.Inventory.Discard(item);
+ ctx.Console.Error.WriteLine($"Failed to add item:{item.Id} to inventory");
+ ctx.ExitCode = 1;
+ return; // stop further processing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (drop && session.Field != null) { | |
| FieldItem fieldItem = session.Field.SpawnItem(session.Player, item); | |
| session.Field.Broadcast(FieldPacket.DropItem(fieldItem)); | |
| } else if (!session.Item.Inventory.Add(item, true)) { | |
| session.Item.Inventory.Discard(item); | |
| ctx.Console.Error.WriteLine($"Failed to add item:{item.Id} to inventory"); | |
| ctx.ExitCode = 1; | |
| } | |
| if (drop && session.Field != null) { | |
| FieldItem fieldItem = session.Field.SpawnItem(session.Player, item); | |
| session.Field.Broadcast(FieldPacket.DropItem(fieldItem)); | |
| } else if (!session.Item.Inventory.Add(item, true)) { | |
| session.Item.Inventory.Discard(item); | |
| ctx.Console.Error.WriteLine($"Failed to add item:{item.Id} to inventory"); | |
| ctx.ExitCode = 1; | |
| return; // stop further processing | |
| } |
🤖 Prompt for AI Agents
In Maple2.Server.Game/Commands/ItemCommand.cs around lines 91 to 98, after
failing to add an item to the inventory, the code continues processing further
items, causing mixed success and failure states. To fix this, add an early exit
(e.g., return or break) immediately after setting the error exit code and
logging the failure to stop further processing and avoid partial success
confusion.
| Item? firstItem = session.Field.ItemDrop.CreateItem(itemId, rarity, rollMax: rollMax); | ||
| if (firstItem == null) { |
There was a problem hiding this comment.
Possible NullReferenceException when session.Field is null.
session.Field.ItemDrop.CreateItem(...) dereferences session.Field unconditionally.
While the command is usually executed in-field, a GM can trigger it from a lobby / loading screen, making session.Field null and crashing the server.
+if (session.Field == null) {
+ ctx.Console.Error.WriteLine("Cannot spawn items: player is not in a field.");
+ ctx.ExitCode = 1;
+ return;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Item? firstItem = session.Field.ItemDrop.CreateItem(itemId, rarity, rollMax: rollMax); | |
| if (firstItem == null) { | |
| if (session.Field == null) { | |
| ctx.Console.Error.WriteLine("Cannot spawn items: player is not in a field."); | |
| ctx.ExitCode = 1; | |
| return; | |
| } | |
| Item? firstItem = session.Field.ItemDrop.CreateItem(itemId, rarity, rollMax: rollMax); | |
| if (firstItem == null) { |
🤖 Prompt for AI Agents
In Maple2.Server.Game/Commands/ItemCommand.cs around lines 47 to 48, the code
accesses session.Field without checking if it is null, which can cause a
NullReferenceException when the command is triggered outside a field context.
Add a null check for session.Field before accessing ItemDrop.CreateItem, and
handle the case where session.Field is null gracefully, such as by returning
early or sending an error message to prevent the server crash.
| if (firstItem.IsCurrency()) { | ||
| firstItem.Amount = amount; | ||
| ProcessSingleItem(ctx, firstItem, drop); | ||
| return; |
There was a problem hiding this comment.
Validate amount for currency items.
firstItem.Amount = amount; accepts negative or zero values, silently minting / removing mesos.
Add a sanity-check to reject non-positive amounts.
-if (firstItem.IsCurrency()) {
- firstItem.Amount = amount;
+if (firstItem.IsCurrency()) {
+ if (amount <= 0) {
+ ctx.Console.Error.WriteLine("Amount must be positive.");
+ ctx.ExitCode = 1;
+ return;
+ }
+ firstItem.Amount = amount;🤖 Prompt for AI Agents
In Maple2.Server.Game/Commands/ItemCommand.cs around lines 53 to 56, the
assignment of amount to firstItem.Amount allows zero or negative values, which
can incorrectly mint or remove currency. Add a validation check before this
assignment to ensure amount is positive, and reject or handle cases where amount
is zero or negative to prevent unintended currency manipulation.
| if (isNonStackable || (rollMax && amount > 1)) { | ||
| ProcessSingleItem(ctx, firstItem, drop); | ||
|
|
||
| for (int i = 1; i < amount; i++) { | ||
| Item? additionalItem = session.Field.ItemDrop.CreateItem(itemId, rarity, rollMax: rollMax); | ||
| if (additionalItem == null) { | ||
| ctx.Console.Error.WriteLine($"Failed to create additional item {i + 1}/{amount}"); | ||
| continue; | ||
| } | ||
|
|
||
| ProcessSingleItem(ctx, additionalItem, drop); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Error propagation from ProcessSingleItem is lost.
When ProcessSingleItem sets ctx.ExitCode = 1, it is overwritten by the unconditional
ctx.ExitCode = 0 at line 83.
A batch spawn in which a single item fails still reports success.
- ctx.ExitCode = 0;
+ // Preserve a non-zero exit code set by ProcessSingleItem
+ if (ctx.ExitCode == 0)
+ ctx.ExitCode = 0; // success only if no previous failureOr simply delete line 83 and rely on the default exit code (0) unless an error was signalled.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Maple2.Server.Game/Commands/ItemCommand.cs around lines 66 to 78, the exit
code set by ProcessSingleItem to indicate failure is overwritten later by an
unconditional ctx.ExitCode = 0 at line 83, causing errors to be ignored. To fix
this, modify the code to preserve any non-zero exit code set by
ProcessSingleItem by only setting ctx.ExitCode to 0 if it is not already an
error code, or remove the line 83 assignment entirely to rely on the default
exit code unless an error occurs.
Fixes it so roll-max applies to multiple items rather than just 1
Summary by CodeRabbit