PaperMC
fix: inventory desync when interaction is cancelled
Fixes to a9399451481e3f8b376484e63909ff6c274f03db that caused inventory desyncs when the client predicted an interaction would result in an item being added to its inventory
Fixes #12947 Fixes #12954 Fixes #13253
I think Kenny said he was working on some fixes for issues related to this but still had to test them
Okay so I've added a clientPredictsInteraction method to keep track of which block & item interactions actually need this. I couldn't really find a good place to put it, so I just slapped it into MCUtil for now. Maybe this should go in its own util class, something like ClientPredictions? idk
I scrolled through the creative inventory and tested some blocks that looked like they might be affected by this, but I didn't find anything else. So if anyone knows of any I missed, please let me know.
I added a check to this method on both interact & spawn protection to send the correct update, but I'm a bit confused by
Only call this for the spawn protection cases
Isn't that what's already being done? Should I split this if check up into something like
if (awaitingPositionFromClient == null) {
if (!mayInteract && !...) {
send correct update
return;
}
rest of the code
}
It's fine in there, looks good; cake plus candle would also be one I think. for the spawn prot one it should just be
} else if (spawnprot && clientPredictsInteraction) {
sendAllDataToRemote()
}
the other cases should be ignored
Implemented the spawn prot thing.
From my testing and understanding you can only place a candle on a cake and not remove it, unless you eat or destroy the cake (but this drops the candle on the ground). I did find something else tho: picking up fluids or honey from a bee hive/nest using a bucket or glass bottle with a count > 1. I found no good way to get the fluid they're interacting with that early in the interact, other than using Level#clip. but calling that an extra time per interaction just for this seemed unnecessary. So I decided to just check if they're holding a bucket/glass bottle with a count > 1. That conveniently also covers the bee nest.
Screenshot from my test world, with other stuff I tested
You can't really rely on tags, since datapacks may remove/change their content and you still need to resend held item within the spawn protection when clientPredictsInteraction = false
right.. datapacks.. using the tag just would've been too simple. yanked all the blocks that are added to the tag in vanilla and put them in a Set.
added back sending the held item in spawn prot
I'm honestly not sure anymore what I was cooking there with the set. My thought process must've been something like "I can't check if it's in the tag, but I can check if it's in a set" without thinking further about it. This is much better. Also applied your other suggestions.
I haven't looked into the PlayerBucketEntityEvent yet, but changing the max stack size of items is something I have not considered at all yet, so there is probably more there even (I'm thinking emptying buckets with a count > 1). I'll do some more testing and check other places where the commit changed stuff.
I went through all the changes of the commit and tested a lot of different events here is what I now changed:
- Sending a full update in Bucketable if the player is in creative or holding a stacked bucket, to fix what Lulu has mentioned. When looking into this I also checked out the PlayerInteractEntityEvent, where I found that it was always sending a full inventory update. So I changed that to follow the same logic as in Bucketable.
- Applied the same logic as in
ServerGamePacketListnerImpl#useItemOntoServerGamePacketListenerImpl#useItem. This would desync when for example picking up a fluid without looking at a block - Checking if the held item is instance of BucketItem in
MCUtil#clientPredictsInteractionto fix cancelling stacked water/lava bucket interaction
All other changes seem to behave correctly and I couldn't find anymore edge cases when testing and looking. If anyone can think of anything else I missed please let me know.
EDIT: found something else: milking a cow in creative, or with a stacked bucket, with a cancelled PlayerInteractEntityEvent EDIT2: okay also mooshroom cows & bowls
Maybe this time I won't think of 5 more things the instant I click ready for review. More changes:
- Added a case for cancelling converting mud, either though PlayerInteractEvent or EntityChangeBlockEvent
- Added a separate clientPredictsInteraction method for entity interactions
- Milking AbstractCow
- Getting soup from MushroomCow
- Bucketing Bucketables
- Refactored the clientPredictsInteraction method into multiple if statements, instead of just one big one, to improve readability. Also checking for creative mode now in places where it was previously only item count > 1
Can you look at the InventoryDragEvent, i believe the above commit broke the ability for plugins to set the cursor on the event when the carried item was empty (which might happens at the end of the action when all the items have been placed).
Specifically, this comment is not really accurate: https://github.com/PaperMC/Paper/blob/main/paper-server/patches/sources/net/minecraft/world/inventory/AbstractContainerMenu.java.patch#L152
You were right, this broke setting the carried item when it's empty. I'm not sure what behavior was even fixed with that if statement, I did some testing by just removing it and everything seemed to work as expected when the inventory is closed server side. To me it doesn't look like getCarried can return null anyways so this is basically what was done before the commit.
https://github.com/user-attachments/assets/c7e34756-f29a-4aa3-bca5-62dfcd18d8b4
In the first test I kept holding mouse one not letting the event fire and it dropped the old item stack. The second test I release mouse 1 and the event changes my carried item to a diamond sword and that is dropped when the inventory is closed. Am I missing something obvious here?
Has there been any movement on this? Checked on the latest build of 1.21.8 as of writing (build 60), and the underlying issue still seems to be present.
If you want to handle https://github.com/PaperMC/Paper/issues/13253 since it's still the same commit.
I included a fix for setting the replacement item in the consume event, but I haven't had time to fully look into the bucket events also mentioned in that issue. So far I believe that it might be related to https://github.com/PaperMC/Paper/commit/a9399451481e3f8b376484e63909ff6c274f03db#diff-041b13ef604c2316fbe5d887eea7c155cedf23bdfccd031a839f75a8c8384bbaR315, but I will look into it further when I find more time
That took a bit, sorry about that. I finally found time to look into the bucket events and the diff I suspected was indeed the cause to the issue. It is now changed to behave like the other places