Player attachments not syncing to client properly on respawn
bejker123 opened this issue ยท 5 comments
While developing a mod using the v1 attachments API I've encountered a bug.
Where the server after respawing a player doesn't properly sync attachments to the client.
I believe it only affects client side, generating a stack trace for each attachment, linked below.
I suspect that the packet is sent during the wrong client phase.
I'm happy to provide more relevant information if needed.
Below I include the versions I use, logs, and a workaround I made, that doesn't address the root of the problem, and is only a band-aid solution.
Versions:
Minecraft - 1.21.4
Yarn - 1.21.4+build.8
Fabric Loader - 0.16.10
Fabric API - 0.118.0+1.21.4
A very basic workaround:
I set a constant delay of 100ms which fixes the issue
https://gist.github.com/bejker123/ff2daeeef96fe824d24e4c87b6a2004e
Relevant Logs:
https://gist.github.com/bejker123/a38d08dd622ff4fafd0076616696563d
After doing some digging I believe the ClientPlayNetworkHandlerMixin.copyAttachmentsOnClientRespawn should mixin'ed after the clientPlayerEntity2.setId(...) call. My assumption is that that causes the issue. Because when setting a break point and inspecting the AttachmentChange.apply function I was able to see that targetInfo.getTarget(...) returns null. This implies that the network id for the client player entity is mismatched with the server.
[...]
//onPlayerRespawn snippet
this.startWorldLoading(clientPlayerEntity2, this.world, worldEntryReason);
clientPlayerEntity2.setId(clientPlayerEntity.getId());
this.client.player = clientPlayerEntity2; // I suggest mixing in into here
if (bl) {
this.client.getMusicTracker().stop();
}
[...]I had a look at that code but I think it is? The mixin targets that very field set already: see here.
A minimal code example to reproduce the issue would help.
After giving it a second look it seems like it should work, but doesn't.
I think my IDE pointed me to the wrong line.
Here is the minimal code example:
https://github.com/bejker123/attch-bug-repro/
It still produces the same logs, with the most basic setup.
I still think the target should be moved to a later line.
Edit 3:
I finally understand the issue. Transferring attachments happened before the player is completely respawned on the client. After changing the transfer to be triggered on ServerPlayerEvents.COPY_FROM to ServerPlayerEvents.AFTER_RESPAWN the stack trace is no longer present.
package net.fabricmc.fabric.impl.attachment;
[...]
public class AttachmentEntrypoint implements ModInitializer {
[...]
@Override
public void onInitialize() {
[...]
// Current
ServerPlayerEvents.COPY_FROM.register((oldPlayer, newPlayer, alive) ->
AttachmentTargetImpl.transfer(oldPlayer, newPlayer, !alive)
);
// Suggested change
ServerPlayerEvents.AFTER_RESPAWN.register((oldPlayer, newPlayer, alive) ->
AttachmentTargetImpl.transfer(oldPlayer, newPlayer, !alive)
);
[...]
}Edit 2:
I've tried the change suggested above and it didn't help. At this point my only idea is to add a check to AttachmentChange.apply, if target is null. This wouldn't fix the underlying issue but at least wouldn't produce the stack traces.
Edit:
I'm pretty sure it should be moved after this.world.addEntity(...) call. Since AttachmentChange accesses the entity from the world.
I was clearly wrong as the problem had nothing to do with the mixin I mentioned.
(See comment above)
I'm sorry for the confusion.
Could that change be implemented?
I tried understanding the code structure better, but it's possible that I'm missing something.