Fix/issue 512 item dropping action#513
Fix/issue 512 item dropping action#513GabrielaGavi wants to merge 2 commits intoObsidianMC:1.21.xfrom
Conversation
WalkthroughThe PR modifies item drop handling in the PlayerActionPacket to add validation checks ensuring a held item exists before dropping and uses the actual held item count instead of a fixed value for the drop amount. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01e72645ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case PlayerActionStatus.DropItemStack: | ||
| { | ||
| DropItem(player, 64); | ||
| var held = player.getHeldItem(); |
There was a problem hiding this comment.
This switch case now calls player.getHeldItem(), but IPlayer only exposes GetHeldItem() (Obsidian.API/_Interfaces/IPlayer.cs:131). In C#, method names are case-sensitive, so this introduces a compile-time failure in PlayerActionPacket and blocks building/running the server until corrected.
Useful? React with 👍 / 👎.
| private static void DropItem(IPlayer player, sbyte amountToRemove) | ||
| { | ||
| var droppedItem = player.GetHeldItem(); | ||
| var heldItem = player.GetHeldItem(); |
There was a problem hiding this comment.
The method renames droppedItem to heldItem, but the ItemEntity initializer still uses droppedItem, leaving an unresolved identifier in this method. That creates another compile-time error in the packet handler, so the change cannot be built as committed.
Useful? React with 👍 / 👎.
| if (held is null or { Type: Material.Air }) | ||
| break; | ||
|
|
||
| DropItem(player, (sbyte)held.Count); |
There was a problem hiding this comment.
Casting held.Count (an int) to sbyte can overflow for oversized stacks, producing a negative amountToRemove; DropItem then computes a non-positive dropCount and returns without dropping anything. This is reachable because /give accepts arbitrary int amount and stores it directly (Obsidian/Commands/Modules/MainCommandModule.cs:216-231), so stacks above 127 become impossible to drop with the stack-drop action.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (2)
141-141:⚠️ Potential issue | 🟠 MajorUse
dropCountinstead ofamountToRemovefor inventory removal.The
dropCountvariable (line 119) computes the safe drop amount asMath.Min(heldItem.Count, amountToRemove). However, line 141 removesamountToRemovefrom inventory, which could attempt to remove more items than actually dropped or available. This breaks the sync between the dropped entity and inventory update described in the PR objectives.🐛 Proposed fix
- player.Inventory.RemoveItem(player.CurrentHeldItemSlot, amountToRemove); + player.Inventory.RemoveItem(player.CurrentHeldItemSlot, (sbyte)dropCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs` at line 141, The inventory removal uses amountToRemove but should use the computed safe value dropCount; in the PlayerActionPacket handling code replace the argument to player.Inventory.RemoveItem (currently amountToRemove) with dropCount so the number removed matches Math.Min(heldItem.Count, amountToRemove) and stays in sync with the dropped entity update (refer to variables dropCount, amountToRemove, heldItem.Count and the call player.Inventory.RemoveItem).
125-131:⚠️ Potential issue | 🔴 CriticalCreate a new ItemStack for the dropped item to avoid undefined variable and side effects.
Line 128 references undefined variable
droppedItem. The held item variable is namedheldItem(line 114), anddropCountis available (line 119). The dropped entity should use a newItemStackinstance withdropCountto prevent side effects when the inventory is modified.🐛 Fix: Create a new ItemStack for the dropped entity
var loc = new VectorF(player.Position.X, (float)player.HeadY - 0.3f, player.Position.Z); + var droppedItem = new ItemStack(heldItem.Holder, dropCount); + var item = new ItemEntity { EntityId = Server.GetNextEntityId(), Item = droppedItem, World = player.World, Position = loc };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs` around lines 125 - 131, The code creates an ItemEntity referencing an undefined/incorrect variable and risks side effects by reusing the held item; in PlayerActionPacket replace the undefined droppedItem with a newly constructed ItemStack using the existing heldItem as the item type and dropCount as the quantity, then assign that new ItemStack to ItemEntity.Item while keeping EntityId = Server.GetNextEntityId(), World = player.World and Position = loc so the dropped entity uses an independent stack and the inventory mutations won't affect it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Line 64: The code calls player.getHeldItem() using incorrect casing which will
not match the defined method GetHeldItem(); update the call in
PlayerActionPacket (replace player.getHeldItem() with player.GetHeldItem()) so
it matches the interface/implementation, rebuild to confirm compilation, and
ensure no other incorrect-cased calls to GetHeldItem exist in the same class.
---
Outside diff comments:
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Line 141: The inventory removal uses amountToRemove but should use the
computed safe value dropCount; in the PlayerActionPacket handling code replace
the argument to player.Inventory.RemoveItem (currently amountToRemove) with
dropCount so the number removed matches Math.Min(heldItem.Count, amountToRemove)
and stays in sync with the dropped entity update (refer to variables dropCount,
amountToRemove, heldItem.Count and the call player.Inventory.RemoveItem).
- Around line 125-131: The code creates an ItemEntity referencing an
undefined/incorrect variable and risks side effects by reusing the held item; in
PlayerActionPacket replace the undefined droppedItem with a newly constructed
ItemStack using the existing heldItem as the item type and dropCount as the
quantity, then assign that new ItemStack to ItemEntity.Item while keeping
EntityId = Server.GetNextEntityId(), World = player.World and Position = loc so
the dropped entity uses an independent stack and the inventory mutations won't
affect it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09af9556-11c6-4ab9-9e4d-efa7cac1951a
📒 Files selected for processing (1)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs
| case PlayerActionStatus.DropItemStack: | ||
| { | ||
| DropItem(player, 64); | ||
| var held = player.getHeldItem(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which method name exists on IPlayer interface
rg -n 'GetHeldItem|getHeldItem' --type=cs -C2Repository: ObsidianMC/Obsidian
Length of output: 5254
Method name casing inconsistency: getHeldItem() vs GetHeldItem().
Line 64 uses player.getHeldItem() (lowercase 'g'), but the method is defined as GetHeldItem() with uppercase 'G' in both the interface and implementation. C# is case-sensitive, so this will cause a compilation error.
Fix
- var held = player.getHeldItem();
+ var held = player.GetHeldItem();📝 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.
| var held = player.getHeldItem(); | |
| var held = player.GetHeldItem(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs` at line 64, The
code calls player.getHeldItem() using incorrect casing which will not match the
defined method GetHeldItem(); update the call in PlayerActionPacket (replace
player.getHeldItem() with player.GetHeldItem()) so it matches the
interface/implementation, rebuild to confirm compilation, and ensure no other
incorrect-cased calls to GetHeldItem exist in the same class.
1 participant
Updated the item drop flow so it now uses the actual stack amount the player is holding, instead of relying on a fixed value in the stack-drop action. I also adjusted DropItem() to compute a safe drop amount before spawning the entity, so the dropped item and the inventory update stay in sync.
Another important change was creating the dropped entity item from a separate ItemStack instance (with the drop amount), instead of reusing the same in-inventory object reference. This avoids side effects when the inventory count is reduced right after spawning the dropped entity.
Finally, the inventory slot update packet flow remains the same, but now it reflects the corrected drop/removal behavior more consistently from the player’s point of view.
PS: I really enjoyed your project! :)
Summary by CodeRabbit