Skip to content

fix WrapperPlayer.java packet order according to mc protocol #58

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
Tofaa2 merged 3 commits into from
May 13, 2026
Merged

fix WrapperPlayer.java packet order according to mc protocol #58

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4a1b93
Add getEntityId method to EntityMeta
3add May 12, 2026
be460de
Fix packet order: sending tablist info before spawning entity as acco…
3add May 12, 2026
4ae42f9
Revert "Add getEntityId method to EntityMeta"
3add May 12, 2026
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
11 changes: 11 additions & 0 deletions api/src/main/java/me/tofaa/entitylib/wrapper/WrapperPlayer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.github.retrooper.packetevents.protocol.player.GameMode;
import com.github.retrooper.packetevents.protocol.player.TextureProperty;
import com.github.retrooper.packetevents.protocol.player.UserProfile;
import com.github.retrooper.packetevents.protocol.world.Location;
import com.github.retrooper.packetevents.wrapper.PacketWrapper;
import com.github.retrooper.packetevents.wrapper.play.server.WrapperPlayServerDestroyEntities;
import com.github.retrooper.packetevents.wrapper.play.server.WrapperPlayServerEntityHeadLook;
Expand All @@ -13,6 +14,7 @@
import com.github.retrooper.packetevents.wrapper.play.server.WrapperPlayServerPlayerInfoUpdate;
import com.github.retrooper.packetevents.wrapper.play.server.WrapperPlayServerSpawnPlayer;
import me.tofaa.entitylib.EntityLib;
import me.tofaa.entitylib.container.EntityContainer;
import me.tofaa.entitylib.meta.EntityMeta;
import java.util.EnumSet;
import java.util.List;
Expand Down Expand Up @@ -54,6 +56,15 @@ protected PacketWrapper<?> createSpawnPacket() {
return packets;
}

@Override
public boolean spawn(Location location, EntityContainer parent) {
if (isSpawned()) return false;

sendPacketToViewers(tabListPacket());

return super.spawn(location, parent);
Comment on lines +60 to +65
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

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

Copy link
Copy Markdown
Contributor Author

@3add 3add May 12, 2026

Choose a reason for hiding this comment

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

Currently only way spawn could return false if it's already spawned (which I'm checking for), could add this if you want.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

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

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

( ˘ ･ ᴗ･ ˘ )

}

public WrapperPlayServerPlayerInfoUpdate tabListPacket() {
EnumSet<WrapperPlayServerPlayerInfoUpdate.Action> actions = EnumSet.of(
WrapperPlayServerPlayerInfoUpdate.Action.ADD_PLAYER,
Expand Down
Loading