Skip to content

Roll-max multiple item fix #495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AngeloTadeucci merged 1 commit into from
Jun 18, 2025
Merged

Roll-max multiple item fix #495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 41 additions & 17 deletions Maple2.Server.Game/Commands/ItemCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,34 +43,58 @@ public ItemCommand(GameSession session, ItemMetadataStorage itemStorage) : base(
private void Handle(InvocationContext ctx, int itemId, int amount, int rarity, bool drop, bool rollMax) {
try {
rarity = Math.Clamp(rarity, 1, MAX_RARITY);
Item? item = session.Field.ItemDrop.CreateItem(itemId, rarity, rollMax: rollMax);
if (item == null) {

Item? firstItem = session.Field.ItemDrop.CreateItem(itemId, rarity, rollMax: rollMax);
if (firstItem == null) {
Comment on lines +47 to +48

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

ctx.Console.Error.WriteLine($"Invalid Item: {itemId}");
return;
}

if (!item.IsCurrency()) {
if (item.Metadata.Property.SlotMax == 0) {
ctx.Console.Error.WriteLine($"{itemId} has SlotMax of 0, ignoring...");
amount = Math.Clamp(amount, 1, int.MaxValue);
}
}
item.Amount = amount;

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 (firstItem.IsCurrency()) {
firstItem.Amount = amount;
ProcessSingleItem(ctx, firstItem, drop);
return;
Comment on lines +53 to 56

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}

bool isNonStackable = firstItem.Metadata.Property.SlotMax == 0;
if (isNonStackable) {
ctx.Console.Error.WriteLine($"{itemId} has SlotMax of 0, ignoring...");
amount = Math.Clamp(amount, 1, int.MaxValue);
}

// For non-stackable items or when rollMax is enabled, create individual items
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 {
Comment on lines +66 to +78

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 failure

Or 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.

firstItem.Amount = amount;
ProcessSingleItem(ctx, firstItem, drop);
}

ctx.ExitCode = 0;
} catch (SystemException ex) {
ctx.Console.Error.WriteLine(ex.Message);
ctx.ExitCode = 1;
}
}

private void ProcessSingleItem(InvocationContext ctx, Item item, bool drop) {
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;
}
Comment on lines +91 to +98

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

}
}