[Bug]: ItemStack changes not synced if LocalPlayer

[MrCrayfish]

Minecraft Version

1.20.4

What happened?

I am currently updating my mod to 1.20.4, and I've run into a problem. I am trying to update some data to the client using ICurio#writeSyncData and ICurio#readSyncData. The ItemStack passed into the ICurio#readSyncData call, presumably the ItemStack that I should be updating with my custom data, is completely ignored. I've looked into your code and it seem in the handling of SPacketSyncStack that if the target entity is a LocalPlayer, it will never update the item in the stacksHandler.

This behaviour is only present in the NeoForge version of the mod. The check for LocalPlayer does not exist in the Forge version.

NeoForge handling: (The code in question)
https://github.com/TheIllusiveC4/Curios/blob/1.20.4/neoforge/src/main/java/top/theillusivec4/curios/common/network/client/CuriosClientPayloadHandler.java#L296-L302

Forge handling: (No localplayer check)
https://github.com/TheIllusiveC4/Curios/blob/1.20.4/forge/src/main/java/top/theillusivec4/curios/common/network/server/sync/SPacketSyncStack.java#L89-L93

How do you trigger this bug?

1. Write custom sync data with ICurio#writeSyncData and ICurio#readSyncData
2. Update the ItemStack passed in ICurio#readSyncData with the custom data
3. Evaluate the ItemStack in the curios slot with the local player (Minecraft.getMinecraft().player)
4. The ItemStack will not have the custom data

Loader

NeoForge

Loader Version

20.4.190

Mod Version

7.4.0+1.20.4

Relevant Log Outputs

No response

[TheIllusiveC4]

That check exists only on NeoForge because it otherwise caused syncing issues in the curio inventory for the local player once Curios migrated over to their new data attachment system.

Can you provide more specific reproduction steps in regards to the data you're writing and reading on the client? Also, does this occur in a single-player instance or while connected on a dedicated server?

I'm having a hard time reproducing this on my end, as any changes I've attempted are read and synced appropriately, but I suspect this may just be due to the exact nature of the data we're respectively attempting to sync.

[MrCrayfish]

Sorry about the lack of details. I reported it late at night my time so I might have rushed and missed some details. I'm going to do a test today on a clean workspace that I can share with you. But to answer your questions, it was on singleplayer. I haven't tested on a dedicated server.

[MrCrayfish]

Alright, here is a test mod showcasing the issue. This is running on 1.20.6 with NeoForge. I've added some comments on demonstrating the problem. If you have any questions, let me know.
https://github.com/MrCrayfish/CuriosSyncBug

[TheIllusiveC4]

Thanks, that should be enough information for now.

I am able to reproduce this issue now, at least on 1.20.6. I'll need to re-test some of this on 1.20.4, which is showing some strangely divergent behavior. It looks like this check might be unnecessary on 1.20.6, which would solve the problem entirely if I could just remove it, but still causes syncing issues if removed on 1.20.4. I'll do some more digging and testing to figure out what's going on exactly and what the ideal solution is for each version.

[MrCrayfish]

Before I start this off, I reversed your bug fix by adding a non-suspending breakpoint with the condition (livingEntity = null) == null on the local player check in order to bypass it.

I did some digging into the issue. I am going to guess that the sync issue you were fixing is related to putting the item into the curio slot and it disappearing. To make things short, AbstractContainerMenu#carried on the client and server at some point begin to reference the exact same ItemStack in memory.

Here is how to test this:

1. Put a non-suspending breakpoint on this.carried = p_150439_; inside of AbstractContianerMenu#setCarried with log output of (this.synchronizer != null ? "SERVER: " : "CLIENT: ") + p_150439_.hashCode() + " " + p_150439_.getItem().
2. Pick up and place the curio item continuously into the curio slot until it disappears
3. Read the console output, you will notice there is one case where the hashcode is the same on the client and server. (In my tests, the second and third to last prints).

ItemStack doesn't have a hashCode() implementation, so it is just using the JVM hash. Provided they are the same, we can safely assume the client/server are retrieving the same object in memory. If you look at the stacktrace of setCarried() when the duplicate hash occurs, it happens in AbstractContianerMenu#doClick specifically line 418 (hopefully your lines match up).

Above on line 416 is where the item is retrieved, and this is where it starts going into NeoForge's code. Moving into tryRemove and then remove() on line 138 of Slot, we end up at SlotIItemHandler#remove. I some quick tests and the itemHandler in SlotIItemHandler are different objects on the client vs server, so the problem must lie inside them instead.

To push forward further, the ItemStack are being retrieved from ItemStackHandler#stacks. So now we know where they are coming from, how are they being added into it. So it's funny because I end up at CuriosClientPayloadHandler, exactly inside the scope of the local player check (that I am bypassing in this test) at stacksHandler.getStacks().setStackInSlot(slot, stack);.

Now here is what I discovered...

The ItemStack passed into the creation of SPacketSyncStack is exactly the same as the ItemStack in the handler, as in it is not a new object. The reference is the exactly the same (e.g. using ==). Well, turns out serialization to the buffer is not even happening, I don't know if it is NeoForge or Vanilla, but it is simply skipping the writing/reading from the buffer. The SPacketSyncStack object created on the server is just passed to the client handle method. I assume this is for performance since we are just playing singleplayer, writing/reading from buffer is extra work.

And here is the simple fix for the problem. In the handling of SPacketSyncStack replace:
ItemStack stack = data.stack(); with ItemStack stack = data.stack().copy();

and of course, remove the LocalPlayer check.

It's time for me to go to bed.

[TheIllusiveC4]

Hmm, very interesting findings. It does thoroughly explain what's going on, which you are correct to assume that the original bug that appeared was related to putting the item into the curio slot and it disappearing. I had originally assumed there was syncing happening in the data attachment and that it was caused by a race condition in the inventory data when applied on the LocalPlayer in SPacketSyncStack, but that was an incorrect guess on my part.

It also explains why this doesn't appear to happen in 1.20.6 (without just the LocalPlayer check), considering the networking refactors that happened on that version probably addressed this issue inadvertently.

I'm glad to see that it's a very simple fix though, I'll try to have an update out tonight at some point.

Thank you very much for the thorough investigation, I really appreciate it.

[MrCrayfish]

I forgot to mention what finally happens. Since AbstractContianerMenu#carried is the same object on both the client/server. When you place the item into the slot, it shrinks the ItemStack on the client first, and carried becomes empty for both sides. The click slot packet is then sent to the server, but since carried is now empty, the server doesn't place anything.

The remoteSlots on the server are updated with the item placed on the client. The server then runs broadcastChanges and will compare the slots on the server vs the remote slots. Since the slot is empty on the server, but the remote slot has an item, it sends an update to the client to set the curio slot as empty; deleting the item.

Glad I could help, just trying to get my mods updated :')

[MrCrayfish]

So basically make sure you copy objects on the receiving end if you're developing prior to 1.20.6. This is not exclusive to ItemStacks. You can use Connection#isMemoryConnection to determine if you need to copy it.