fix WrapperPlayer.java packet order according to mc protocol#58
Conversation
…rding to mc protocol.
This reverts commit e4a1b93.
📝 WalkthroughWalkthroughThis PR fixes the Minecraft protocol packet ordering in ChangesTab-list packet ordering
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
3add
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/main/java/me/tofaa/entitylib/wrapper/WrapperPlayer.java`:
- Around line 60-65: The code sends the tab-list add packet before calling
spawn, which can leave a ghost entry if super.spawn(Location, EntityContainer)
returns false or throws; wrap the call to super.spawn(...) in a try/catch and
check its boolean result, and if it returns false or an exception occurs send
the corresponding tab-list removal packet to viewers (e.g. a
tabListRemovePacket() or the inverse of tabListPacket()) before
returning/propagating the error, so viewers never keep a stale entry; use the
existing spawn(Location, EntityContainer), isSpawned(), tabListPacket() methods
to locate the spot to add the rollback send.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4f8db28-0c13-4122-9e1c-325205a022f2
📒 Files selected for processing (1)
api/src/main/java/me/tofaa/entitylib/wrapper/WrapperPlayer.java
| public boolean spawn(Location location, EntityContainer parent) { | ||
| if (isSpawned()) return false; | ||
|
|
||
| sendPacketToViewers(tabListPacket()); | ||
|
|
||
| return super.spawn(location, parent); |
There was a problem hiding this comment.
Avoid stale tab-list entries when super.spawn(...) fails.
Line 63 sends player info before spawn (good for protocol), but if super.spawn(location, parent) returns false or errors, viewers can keep a ghost tab-list entry for this wrapper.
Proposed fix
`@Override`
public boolean spawn(Location location, EntityContainer parent) {
if (isSpawned()) return false;
sendPacketToViewers(tabListPacket());
- return super.spawn(location, parent);
+ try {
+ boolean spawned = super.spawn(location, parent);
+ if (!spawned) {
+ sendPacketToViewers(new WrapperPlayServerPlayerInfoRemove(getUuid()));
+ }
+ return spawned;
+ } catch (RuntimeException ex) {
+ sendPacketToViewers(new WrapperPlayServerPlayerInfoRemove(getUuid()));
+ throw ex;
+ }
}📝 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.
| public boolean spawn(Location location, EntityContainer parent) { | |
| if (isSpawned()) return false; | |
| sendPacketToViewers(tabListPacket()); | |
| return super.spawn(location, parent); | |
| public boolean spawn(Location location, EntityContainer parent) { | |
| if (isSpawned()) return false; | |
| sendPacketToViewers(tabListPacket()); | |
| try { | |
| boolean spawned = super.spawn(location, parent); | |
| if (!spawned) { | |
| sendPacketToViewers(new WrapperPlayServerPlayerInfoRemove(getUuid())); | |
| } | |
| return spawned; | |
| } catch (RuntimeException ex) { | |
| sendPacketToViewers(new WrapperPlayServerPlayerInfoRemove(getUuid())); | |
| throw ex; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/main/java/me/tofaa/entitylib/wrapper/WrapperPlayer.java` around lines
60 - 65, The code sends the tab-list add packet before calling spawn, which can
leave a ghost entry if super.spawn(Location, EntityContainer) returns false or
throws; wrap the call to super.spawn(...) in a try/catch and check its boolean
result, and if it returns false or an exception occurs send the corresponding
tab-list removal packet to viewers (e.g. a tabListRemovePacket() or the inverse
of tabListPacket()) before returning/propagating the error, so viewers never
keep a stale entry; use the existing spawn(Location, EntityContainer),
isSpawned(), tabListPacket() methods to locate the spot to add the rollback
send.
There was a problem hiding this comment.
Currently only way spawn could return false if it's already spawned (which I'm checking for), could add this if you want.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the spawn method in WrapperEntity and WrapperLivingEntity
ast-grep --pattern 'public boolean spawn($_, $_) {
$$$
}'
# Also check with rg for context
rg -n "boolean spawn" --type java -A 10
Length of output: 7764
@3add, you're right — after inspecting WrapperEntity.spawn() in WrapperEntity.java (lines 87–96), the only return false path is:
public boolean spawn(Location location, EntityContainer parent) {
if (spawned) return false; // only false path
this.location = location;
this.spawned = true;
// ... always proceeds from here
}Since WrapperPlayer.spawn() already guards with if (isSpawned()) return false; before sending the tab-list packet, super.spawn() is guaranteed to see spawned == false and will never return false again. The current implementation is correct and the extra rollback logic I suggested would be dead code under normal conditions. The suggestion can be safely ignored.
( ˘ ･ ᴗ･ ˘ )
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #57 by ensuring WrapperPlayer sends the player-info update packet to viewers before sending the player spawn packet, aligning WrapperPlayer’s spawn flow with the expected Minecraft protocol ordering (matching the already-correct per-viewer add logic in WrapperEntity.addViewer).
Changes:
- Override
WrapperPlayer.spawn(Location, EntityContainer)to sendtabListPacket()to current viewers before delegating toWrapperEntity.spawn(...).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Is this a logic breaking change? |
|
LGTM |
Fixes #57
Summary by CodeRabbit