Open
Conversation
WalkthroughRefactors event argument constructors, introduces sequence-aware BlockBreak event handling moved into MainEventHandler, updates PlayerAction packet flow and PlayerActionStatus enum, refactors velocity packing/encoding, and applies minor lifecycle and numeric adjustments across server/world/player code. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PacketHandler as PlayerActionPacket
participant Events as MainEventHandler
participant World
participant ItemEntity
Client->>PacketHandler: FinishedDigging (includes Sequence)
PacketHandler->>Events: Construct BlockBreakEventArgs(server, player, block, loc, world)
Events->>Events: OnBlockBreak(args)
Events->>PacketHandler: Send BlockChangedAckPacket(sequence)
alt event cancelled
Events->>PacketHandler: Send BlockUpdatePacket(original block)
else event allowed
Events->>World: SetBlock(location, Air)
Events->>World: Broadcast BlockUpdate(Air)
Events->>ItemEntity: Create dropped item entity (id, pos, vel)
Events->>World: Add & Spawn ItemEntity
Events->>World: Broadcast BlockDestructionPacket
end
PacketHandler->>Client: Ack / updates delivered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 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 |
Tides
added
server
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (1)
129-143:⚠️ Potential issue | 🔴 CriticalRemove the
Stabenum value — it does not exist in the Minecraft 1.21 protocol.The
PlayerActionStatusenum values 0–6 correctly map to the protocol (StartedDigging, CancelledDigging, FinishedDigging, DropItemStack, DropItem, ReleaseUseItem, SwapItemInHand). However, theStabvalue at index 7 is not defined in the Minecraft Java Edition 1.21 protocol specification. Since this enum is deserialized from network packets, having undefined enum values will cause deserialization failures when the protocol sends valid values.🤖 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 129 - 143, The PlayerActionStatus enum contains an invalid member "Stab" that doesn't exist in the Minecraft 1.21 protocol and will break deserialization; remove the "Stab" enum value from the PlayerActionStatus declaration in PlayerActionPacket.cs so the enum values map 0–6 to StartedDigging..SwapItemInHand, and scan for any code referencing PlayerActionStatus.Stab (e.g., switch/case or comparisons) and remove or handle those code paths appropriately.
🧹 Nitpick comments (5)
Obsidian/Events/MainEventHandler.World.cs (2)
58-63:GetRandDropVelocityis defined but never used.This helper method is dead code. Either use it when spawning the dropped item entity, or remove it to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian/Events/MainEventHandler.World.cs` around lines 58 - 63, GetRandDropVelocity is dead code; either remove the unused helper or apply it when setting the velocity for spawned dropped-item entities. Locate the item spawn logic (the method that creates the dropped item entity — search for calls that instantiate or spawn dropped items, e.g., CreateDroppedItem, SpawnItemEntity, or the code block that sets entity.Velocity) and replace the current hardcoded/absent velocity assignment with a call to GetRandDropVelocity() (or remove the helper if you prefer deletion). Ensure the chosen change updates the entity's velocity property when spawning and remove the private GetRandDropVelocity method if it remains unused.
55-55: Item spawns withVelocity.Zero- should it useGetRandDropVelocity?The item is spawned with
Velocity.Zero, butGetRandDropVelocityis defined below and appears intended to add random motion to dropped items. This mimics vanilla Minecraft behavior where dropped items have slight random velocity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian/Events/MainEventHandler.World.cs` at line 55, The item is being spawned with Velocity.Zero instead of applying the intended random drop motion; replace the zero velocity with a call to the existing GetRandDropVelocity helper so dropped items get the random motion (i.e., change the SpawnEntity call that uses Velocity.Zero to use GetRandDropVelocity(...) and pass any required position/seed/context that GetRandDropVelocity expects). Update any surrounding logic that assumed zero velocity (e.g., collision or immediate pickup) to work with the new randomized velocity if necessary, and ensure GetRandDropVelocity is accessible from MainEventHandler.World.Obsidian/WorldData/World.cs (1)
202-202: CastingIPlayertoPlayeris fragile if non-Playerimplementations exist.The
.Cast<Player>()call assumes allIPlayerinstances arePlayertypes. While this matches the existing pattern inPacketBroadcaster.cs, it will throwInvalidCastExceptionif a plugin or future change introduces a differentIPlayerimplementation.Consider using
OfType<Player>()for a safer alternative that filters rather than throws:💡 Suggested fix
-foreach (Player player in PlayersInRange(location).Cast<Player>()) +foreach (var player in PlayersInRange(location).OfType<Player>())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian/WorldData/World.cs` at line 202, The foreach is using PlayersInRange(location).Cast<Player>() which will throw InvalidCastException if any IPlayer is not a Player; change the iteration to filter safe Player instances (use OfType<Player>() instead of Cast<Player>()) so non-Player IPlayer implementations are skipped; update the foreach in World.cs where PlayersInRange(location) is enumerated (same pattern as in PacketBroadcaster.cs) to use OfType<Player>() to avoid brittle casting.Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (1)
73-78: EmptyFinishedDiggingcase - consider removing or documenting.The
FinishedDiggingcase contains only whitespace. Since block breaking is now handled by the event system beforeBroadcastPlayerActionis called, this case appears unnecessary.💡 Suggested fix
case PlayerActionStatus.StartedDigging: case PlayerActionStatus.CancelledDigging: player.Client.SendPacket(new BlockChangedAckPacket { SequenceID = this.Sequence }); break; - case PlayerActionStatus.FinishedDigging: - { - - - break; - }🤖 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 73 - 78, The PlayerActionStatus.FinishedDigging case in PlayerActionPacket.cs is an empty block and should be removed or clarified; either delete the entire case branch for PlayerActionStatus.FinishedDigging (so the switch no longer contains an empty branch) or replace the empty block with a concise comment explaining why no action is needed (e.g., "Handled by event system before BroadcastPlayerAction"), keeping the break/flow consistent; locate the switch on PlayerActionStatus in the method handling BroadcastPlayerAction/PlayerActionPacket and apply the change to avoid dead/empty code.Obsidian.API/Events/BlockEventArgs.cs (1)
20-21: Consider adding XML documentation for theSequenceproperty.The other properties have documentation comments, but
Sequencelacks context about its purpose (block change acknowledgment ID).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian.API/Events/BlockEventArgs.cs` around lines 20 - 21, Add an XML documentation comment for the Sequence property on the BlockEventArgs class: insert a /// <summary> that explains this is the block change acknowledgment ID (used to correlate/acknowledge changes or ordering of block events) and, if appropriate, a <remarks> or <value> line consistent with the existing docs on the other properties to match style and clarify usage.
🤖 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.API/Events/BlockEventArgs.cs`:
- Line 18: The World property on BlockEventArgs is hardcoded to
server.DefaultWorld; change BlockEventArgs so World is set via a constructor
parameter (e.g., add a constructor accepting IWorld world and assign it to the
World property) instead of the inline initializer, and update derived classes
such as BlockBreakEventArgs and any callers (like the OnBlockBreak handler) to
pass the correct world (use player.World or block.World) into the new base
constructor so args.World matches the actual dimension rather than
server.DefaultWorld.
In `@Obsidian/Client.cs`:
- Around line 330-334: Remove the redundant world removal in the client
disconnect path: in the code block inside Client.cs where DisconnectAsync
triggers PlayerLeaveEventArgs (DisconnectAsync) and
MainEventHandler.OnPlayerLeave already calls
player.World.TryRemovePlayer(player), delete the call to
this.Player.World.TryRemovePlayer(this.Player) and keep only the canonical
removal handled by MainEventHandler.OnPlayerLeave (you may leave
this.Server.RemovePlayer(this.Player) if server-side bookkeeping remains
required).
In `@Obsidian/Events/MainEventHandler.World.cs`:
- Around line 23-27: The handler currently only checks args.IsCancelled before
reverting the block and returning; update the condition to also respect
args.Handled (e.g., treat Handled like a short-circuit to skip processing) so
that if another handler set Handled = true this handler will also send the
BlockUpdatePacket and return. Locate the block in MainEventHandler.World where
you check args.IsCancelled (the same place referenced from
PlayerActionPacket.HandleAsync) and change the guard to include args.Handled
(args.IsCancelled || args.Handled) so custom handlers can stop further
destruction logic.
---
Outside diff comments:
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Around line 129-143: The PlayerActionStatus enum contains an invalid member
"Stab" that doesn't exist in the Minecraft 1.21 protocol and will break
deserialization; remove the "Stab" enum value from the PlayerActionStatus
declaration in PlayerActionPacket.cs so the enum values map 0–6 to
StartedDigging..SwapItemInHand, and scan for any code referencing
PlayerActionStatus.Stab (e.g., switch/case or comparisons) and remove or handle
those code paths appropriately.
---
Nitpick comments:
In `@Obsidian.API/Events/BlockEventArgs.cs`:
- Around line 20-21: Add an XML documentation comment for the Sequence property
on the BlockEventArgs class: insert a /// <summary> that explains this is the
block change acknowledgment ID (used to correlate/acknowledge changes or
ordering of block events) and, if appropriate, a <remarks> or <value> line
consistent with the existing docs on the other properties to match style and
clarify usage.
In `@Obsidian/Events/MainEventHandler.World.cs`:
- Around line 58-63: GetRandDropVelocity is dead code; either remove the unused
helper or apply it when setting the velocity for spawned dropped-item entities.
Locate the item spawn logic (the method that creates the dropped item entity —
search for calls that instantiate or spawn dropped items, e.g.,
CreateDroppedItem, SpawnItemEntity, or the code block that sets entity.Velocity)
and replace the current hardcoded/absent velocity assignment with a call to
GetRandDropVelocity() (or remove the helper if you prefer deletion). Ensure the
chosen change updates the entity's velocity property when spawning and remove
the private GetRandDropVelocity method if it remains unused.
- Line 55: The item is being spawned with Velocity.Zero instead of applying the
intended random drop motion; replace the zero velocity with a call to the
existing GetRandDropVelocity helper so dropped items get the random motion
(i.e., change the SpawnEntity call that uses Velocity.Zero to use
GetRandDropVelocity(...) and pass any required position/seed/context that
GetRandDropVelocity expects). Update any surrounding logic that assumed zero
velocity (e.g., collision or immediate pickup) to work with the new randomized
velocity if necessary, and ensure GetRandDropVelocity is accessible from
MainEventHandler.World.
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Around line 73-78: The PlayerActionStatus.FinishedDigging case in
PlayerActionPacket.cs is an empty block and should be removed or clarified;
either delete the entire case branch for PlayerActionStatus.FinishedDigging (so
the switch no longer contains an empty branch) or replace the empty block with a
concise comment explaining why no action is needed (e.g., "Handled by event
system before BroadcastPlayerAction"), keeping the break/flow consistent; locate
the switch on PlayerActionStatus in the method handling
BroadcastPlayerAction/PlayerActionPacket and apply the change to avoid
dead/empty code.
In `@Obsidian/WorldData/World.cs`:
- Line 202: The foreach is using PlayersInRange(location).Cast<Player>() which
will throw InvalidCastException if any IPlayer is not a Player; change the
iteration to filter safe Player instances (use OfType<Player>() instead of
Cast<Player>()) so non-Player IPlayer implementations are skipped; update the
foreach in World.cs where PlayersInRange(location) is enumerated (same pattern
as in PacketBroadcaster.cs) to use OfType<Player>() to avoid brittle casting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86dbafe0-3d97-4d2d-a756-c3e56142752a
📒 Files selected for processing (7)
Obsidian.API/Events/BlockEventArgs.csObsidian/Client.csObsidian/Events/MainEventHandler.World.csObsidian/Net/NetworkBuffer.Writing.csObsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.csObsidian/Utilities/NumericsHelper.csObsidian/WorldData/World.cs
💤 Files with no reviewable changes (1)
- Obsidian/Utilities/NumericsHelper.cs
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 (1)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (1)
52-79:⚠️ Potential issue | 🟠 MajorNew
PlayerActionStatusvalues are silent no-ops.
ReleaseUseItem,SwapItemInHand, andStabare now deserializable, but the switch still has no branches for them, so those packets are ignored. At minimum that leaves the new statuses unusable, andSwapItemInHandlooks like a player-visible regression.Also applies to: 138-142
🤖 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 52 - 79, The switch in BroadcastPlayerAction ignores newly deserializable PlayerActionStatus values (ReleaseUseItem, SwapItemInHand, Stab), causing silent no-ops and user-visible regressions; update BroadcastPlayerAction to handle these enum members explicitly (add case branches for ReleaseUseItem, SwapItemInHand, and Stab) and invoke the appropriate server logic or client responses (e.g., call the corresponding player methods or send the matching packets used elsewhere in the class for other actions) so the actions are processed instead of dropped; also mirror the same added cases where the player-action switch is duplicated (the other Broadcast/handling block in this file) to keep behavior consistent.
🧹 Nitpick comments (1)
Obsidian.API/Events/BlockEventArgs.cs (1)
21-21: KeepSequenceoff the abstract base if possible.Right now this looks specific to the digging/ack flow in
PlayerActionPacket. Putting it onBlockEventArgsleaks a transport detail into every block event;BlockBreakEventArgsor a small interface would keep the base API cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian.API/Events/BlockEventArgs.cs` at line 21, The Sequence property on the abstract BlockEventArgs leaks transport/ack details (used by PlayerActionPacket) into the base event API; remove Sequence from BlockEventArgs and move it to a more specific location such as adding Sequence to BlockBreakEventArgs (or create a small interface like ISequencedEvent) so only events that participate in the digging/ack flow expose it; update any consumers that cast or rely on BlockEventArgs.Sequence to use the new specific type or interface and adjust serialization/packet handling in PlayerActionPacket accordingly.
🤖 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/Server.cs`:
- Line 309: The current return expression short-circuits and skips world cleanup
when OnlinePlayers.Remove(player.Uuid, out _) is false; change the method to
call player.World.TryRemovePlayer(player) unconditionally (best-effort) and then
return the combination of both results (e.g., store the result of
OnlinePlayers.Remove(...) and the result of player.World.TryRemovePlayer(...) in
local bools and return their &&). Ensure you still pass out _ to
OnlinePlayers.Remove and reference the same player variable so world removal
always runs even if the dictionary removal failed.
---
Outside diff comments:
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Around line 52-79: The switch in BroadcastPlayerAction ignores newly
deserializable PlayerActionStatus values (ReleaseUseItem, SwapItemInHand, Stab),
causing silent no-ops and user-visible regressions; update BroadcastPlayerAction
to handle these enum members explicitly (add case branches for ReleaseUseItem,
SwapItemInHand, and Stab) and invoke the appropriate server logic or client
responses (e.g., call the corresponding player methods or send the matching
packets used elsewhere in the class for other actions) so the actions are
processed instead of dropped; also mirror the same added cases where the
player-action switch is duplicated (the other Broadcast/handling block in this
file) to keep behavior consistent.
---
Nitpick comments:
In `@Obsidian.API/Events/BlockEventArgs.cs`:
- Line 21: The Sequence property on the abstract BlockEventArgs leaks
transport/ack details (used by PlayerActionPacket) into the base event API;
remove Sequence from BlockEventArgs and move it to a more specific location such
as adding Sequence to BlockBreakEventArgs (or create a small interface like
ISequencedEvent) so only events that participate in the digging/ack flow expose
it; update any consumers that cast or rely on BlockEventArgs.Sequence to use the
new specific type or interface and adjust serialization/packet handling in
PlayerActionPacket accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2cc9c61-cc92-4a44-b62b-01876af81062
📒 Files selected for processing (6)
Obsidian.API/Events/BlockBreakEventArgs.csObsidian.API/Events/BlockEventArgs.csObsidian.API/Events/PlayerLeaveEventArgs.csObsidian/Events/MainEventHandler.csObsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.csObsidian/Server.cs
💤 Files with no reviewable changes (1)
- Obsidian/Events/MainEventHandler.cs
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/Entities/Player.cs (1)
117-133:⚠️ Potential issue | 🟡 MinorGetter with side-effect silently masks invalid state.
The getter mutates
fieldwhen it's out of range, which is an anti-pattern:
- Reading a property shouldn't have side effects—this can cause subtle bugs in debugging, logging, or multi-threaded access.
- Silent correction hides the root cause if
fieldbecomes invalid (e.g., a bug in deserialization or reflection-based assignment).- Per context snippet 4 (
Player.Helpers.cs:49), serialization reads this property—iffieldwas corrupted, it will be silently "fixed" to 36 and persisted, losing diagnostic information.Consider throwing or logging when the backing field is invalid, or validating at write time (setter, deserialization) rather than on read.
Suggested approach: validate at the source, not the getter
public short CurrentHeldItemSlot { - get - { - if (field < 36 || field > 44) - field = 36; - - return field; - } + get => field; set { if (value is < 0 or > 8) throw new IndexOutOfRangeException("Value must be >= 0 or <= 8"); field = (short)(value + 36); } }If defensive fallback is truly needed, consider logging a warning instead of silently mutating:
get { if (field < 36 || field > 44) { Logger.LogWarning("CurrentHeldItemSlot backing field was invalid ({Field}), resetting to 36", field); field = 36; } return field; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Obsidian/Entities/Player.cs` around lines 117 - 133, The getter CurrentHeldItemSlot mutates its backing field when out of range—remove that side effect: make the getter only return the backing field (if it's in the valid range) and if it's invalid throw a clear exception (e.g., InvalidOperationException or IndexOutOfRangeException) or log a warning instead of silently correcting it; keep the setter validation (it already checks value and writes field = (short)(value + 36)) and add explicit validation/fixing at the source(s) that can corrupt the backing field (e.g., the deserialization logic referenced in Player.Helpers.cs around where the player is read) so any corruption is detected and handled during write/deserialization rather than on read.Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (1)
128-142:⚠️ Potential issue | 🔴 CriticalRemove
StabfromPlayerActionStatusenum—it does not exist in Minecraft 1.21 protocol.The Minecraft 1.21 protocol defines
PlayerActionas a 7-value enum (ordinals 0–6):START_DESTROY_BLOCK,ABORT_DESTROY_BLOCK,STOP_DESTROY_BLOCK,DROP_ALL_ITEMS,DROP_ITEM,RELEASE_USE_ITEM, andSWAP_ITEM_WITH_OFFHAND. TheStabentry at ordinal 7 is not part of the official specification and will cause packet serialization/deserialization failures. Additionally, consider renamingSwapItemInHandtoSwapItemWithOffhandto align with the protocol specification.🤖 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 128 - 142, The enum PlayerActionStatus contains an extra entry "Stab" that must be removed and the entry SwapItemInHand should be renamed to SwapItemWithOffhand to match the Minecraft 1.21 protocol ordinals (0–6: StartedDigging, CancelledDigging, FinishedDigging, DropItemStack, DropItem, ReleaseUseItem, SwapItemWithOffhand); update the enum declaration by removing Stab and renaming SwapItemInHand, then review and adjust any serialization/deserialization code, switch statements, unit tests, or references that rely on PlayerActionStatus (including methods that map ordinals or write/read the enum) so the ordinal ordering and names align with the protocol.
🧹 Nitpick comments (1)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (1)
73-78: Empty switch case appears incomplete or should have a comment.The
FinishedDiggingcase body is empty (Line 75 has only whitespace). If this is intentional because the block destruction is now handled byMainEventHandler.OnBlockBreak, add a clarifying comment. Otherwise, this looks like unfinished refactoring.Add explanatory comment
case PlayerActionStatus.FinishedDigging: { - - + // Block destruction is handled by OnBlockBreak event handler break; }🤖 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 73 - 78, The empty case for PlayerActionStatus.FinishedDigging in PlayerActionPacket looks accidental; either add a clarifying comment indicating that block destruction/handling is intentionally moved to MainEventHandler.OnBlockBreak (e.g., "Handled by MainEventHandler.OnBlockBreak — no-op here") or restore the intended logic for finishing a dig if refactoring left it out; update the case body in PlayerActionPacket to include that comment or reintroduce the necessary handling so the behavior is explicit.
🤖 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`:
- Around line 36-49: PlayerActionPacket is causing duplicate
BlockChangedAckPacket for StartedDigging in Creative because
MainEventHandler.OnBlockBreak sends an ack but doesn't mark the
BlockBreakEventArgs as handled so BroadcastPlayerAction sends another; fix by
either setting args.Handled = true inside MainEventHandler.OnBlockBreak after it
sends the BlockChangedAck (so ExecuteEventAsync results in no further
broadcast), or change BroadcastPlayerAction (the method referenced from
PlayerActionPacket.BroadcastPlayerAction) to skip sending the
StartedDigging/creative BlockChangedAck case when the event was meant to handle
it; update the code in MainEventHandler.OnBlockBreak or in BroadcastPlayerAction
accordingly (references: PlayerActionPacket, BroadcastPlayerAction,
MainEventHandler.OnBlockBreak, BlockBreakEventArgs).
---
Outside diff comments:
In `@Obsidian/Entities/Player.cs`:
- Around line 117-133: The getter CurrentHeldItemSlot mutates its backing field
when out of range—remove that side effect: make the getter only return the
backing field (if it's in the valid range) and if it's invalid throw a clear
exception (e.g., InvalidOperationException or IndexOutOfRangeException) or log a
warning instead of silently correcting it; keep the setter validation (it
already checks value and writes field = (short)(value + 36)) and add explicit
validation/fixing at the source(s) that can corrupt the backing field (e.g., the
deserialization logic referenced in Player.Helpers.cs around where the player is
read) so any corruption is detected and handled during write/deserialization
rather than on read.
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Around line 128-142: The enum PlayerActionStatus contains an extra entry
"Stab" that must be removed and the entry SwapItemInHand should be renamed to
SwapItemWithOffhand to match the Minecraft 1.21 protocol ordinals (0–6:
StartedDigging, CancelledDigging, FinishedDigging, DropItemStack, DropItem,
ReleaseUseItem, SwapItemWithOffhand); update the enum declaration by removing
Stab and renaming SwapItemInHand, then review and adjust any
serialization/deserialization code, switch statements, unit tests, or references
that rely on PlayerActionStatus (including methods that map ordinals or
write/read the enum) so the ordinal ordering and names align with the protocol.
---
Nitpick comments:
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Around line 73-78: The empty case for PlayerActionStatus.FinishedDigging in
PlayerActionPacket looks accidental; either add a clarifying comment indicating
that block destruction/handling is intentionally moved to
MainEventHandler.OnBlockBreak (e.g., "Handled by MainEventHandler.OnBlockBreak —
no-op here") or restore the intended logic for finishing a dig if refactoring
left it out; update the case body in PlayerActionPacket to include that comment
or reintroduce the necessary handling so the behavior is explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2feb436-5317-414b-8e3c-33ba4310ea64
📒 Files selected for processing (6)
Obsidian.API/_Types/Velocity.csObsidian/Entities/Player.csObsidian/Events/MainEventHandler.World.csObsidian/Net/NetworkBuffer.Writing.csObsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.csObsidian/Server.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Obsidian/Events/MainEventHandler.World.cs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (1)
127-141:⚠️ Potential issue | 🔴 CriticalRemove the
Stabenum value—it does not exist in Minecraft 1.21.x protocol.The protocol defines only 7 status values (0–6). Values 0–6 in the enum correctly correspond to
START_DESTROY_BLOCK,ABORT_DESTROY_BLOCK,STOP_DESTROY_BLOCK,DROP_ALL_ITEMS,DROP_ITEM,RELEASE_USE_ITEM, andSWAP_ITEM_WITH_OFFHANDrespectively. TheStabentry at index 7 has no equivalent in the official protocol specification and must be removed.🤖 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 127 - 141, The PlayerActionStatus enum contains an extra value "Stab" that does not exist in the Minecraft 1.21.x protocol; remove the "Stab" member from the PlayerActionStatus enum so the remaining entries (StartedDigging, CancelledDigging, FinishedDigging, DropItemStack, DropItem, ReleaseUseItem, SwapItemInHand) map to values 0–6 exactly and ensure any code using PlayerActionStatus (parsing/serializing in PlayerActionPacket) still treats the enum as 0–6 only.
🧹 Nitpick comments (1)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs (1)
72-77: Remove unreachableFinishedDiggingcase.This case is unreachable because
FinishedDiggingis handled at line 36 and returns beforeBroadcastPlayerActionis called. Dead code creates confusion about the intended flow.♻️ Proposed fix
case PlayerActionStatus.StartedDigging: case PlayerActionStatus.CancelledDigging: player.Client.SendPacket(new BlockChangedAckPacket { SequenceID = this.Sequence }); break; - case PlayerActionStatus.FinishedDigging: - { - - - break; - } }🤖 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 72 - 77, Remove the unreachable case for PlayerActionStatus.FinishedDigging from the switch inside PlayerActionPacket (the switch handling player actions) because FinishedDigging is already handled earlier and returns before BroadcastPlayerAction is invoked; delete the empty case block referencing PlayerActionStatus.FinishedDigging to avoid dead code and ensure the switch only contains actionable cases (verify no other logic depends on that case and that BroadcastPlayerAction remains the single handler for finished-digging).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Around line 127-141: The PlayerActionStatus enum contains an extra value
"Stab" that does not exist in the Minecraft 1.21.x protocol; remove the "Stab"
member from the PlayerActionStatus enum so the remaining entries
(StartedDigging, CancelledDigging, FinishedDigging, DropItemStack, DropItem,
ReleaseUseItem, SwapItemInHand) map to values 0–6 exactly and ensure any code
using PlayerActionStatus (parsing/serializing in PlayerActionPacket) still
treats the enum as 0–6 only.
---
Nitpick comments:
In `@Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs`:
- Around line 72-77: Remove the unreachable case for
PlayerActionStatus.FinishedDigging from the switch inside PlayerActionPacket
(the switch handling player actions) because FinishedDigging is already handled
earlier and returns before BroadcastPlayerAction is invoked; delete the empty
case block referencing PlayerActionStatus.FinishedDigging to avoid dead code and
ensure the switch only contains actionable cases (verify no other logic depends
on that case and that BroadcastPlayerAction remains the single handler for
finished-digging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fefa0f3-6d42-4814-bd01-a68ade2ffd17
📒 Files selected for processing (1)
Obsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.cs
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
1 participant
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.
Fixes the block breaking bug. This bug occurs on reconnect because the player object was not being removed from the world causing a stale "player" to receive the packets instead of the new player.
Closes #500
Summary by CodeRabbit