Skip to content

Conversation

@niho2
Copy link

@niho2 niho2 commented Oct 31, 2025

Description

Fixes item desync issues when using PlayerItemConsumeEvent#setReplacement(), PlayerBucketFillEvent#setItemStack(), and PlayerBucketEmptyEvent#setItemStack() with items that appear identical to vanilla behavior.

Problem

The optimization introduced in commit a939945 ("Fixup sendAllDataToRemote calls") reduced unnecessary item copies but introduced a side effect: when items appeared "equal" (same type, data, etc.), the sync to clients was skipped. This caused visual desyncs where:

  • The server correctly kept the replacement item
  • The client thought the item was consumed (slot appears empty)
  • Re-opening inventory or relogging revealed the item (proving server had it)

Solution

Force client synchronization via sendAllDataToRemote() when replacement items are explicitly set through these events. This ensures the client always receives updates even if the items appear identical to vanilla behavior.

Changes

  • LivingEntity.java.patch: Added sync call after PlayerItemConsumeEvent replacement is set
  • BucketItem.java.patch: Added sync calls after bucket fill/empty events complete

Testing

Tested with the reproduction cases from the issue:

  1. ✅ Eating cooked beef with setReplacement(item.clone()) - item now stays visible

All cases that previously required updateInventory() or caused desync now work correctly without manual synchronization.

Fixes

Fixes #13253

Related

  • Related to commit a939945 (Fixup sendAllDataToRemote calls)

@niho2 niho2 requested a review from a team as a code owner October 31, 2025 07:38
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Oct 31, 2025
@masmc05
Copy link
Contributor

masmc05 commented Oct 31, 2025

Have you tried replacing that specific remote slot with ContainerSyncronizer#createSlot to force eventual synchronization? Sending the whole inventory is just too expensive for just 1 slot

@masmc05
Copy link
Contributor

masmc05 commented Oct 31, 2025

Or creating a RemoteSlot#flush that clears the hash and item from it, and use that to force synchronization

@niho2
Copy link
Author

niho2 commented Oct 31, 2025

Thank you for the feedback @masmc05!

I've optimized the implementation to address your performance concern. Instead of synchronizing the entire inventory with sendAllDataToRemote(), the fix now uses forceSlot() to synchronize only the specific slot that was modified.

Changes made:

Optimized forceHeldSlot() method (AbstractContainerMenu.java.patch):

  • Now identifies the player inventory from the container menu
  • Determines the specific slot based on the InteractionHand (main hand or offhand)
  • Uses the existing forceSlot() method to sync only that slot via sendSlotChange()
  • Falls back to sendAllDataToRemote() for non-player inventories as a safety measure

Updated event handlers (LivingEntity.java.patch and BucketItem.java.patch):

  • Changed from sendAllDataToRemote() to forceHeldSlot(hand)
  • This ensures only the affected slot is synchronized

Testing:

✅ Item consumption with PlayerItemConsumeEvent#setReplacement() still works correctly

This approach provides the same fix for the desync issue while being significantly more performant by avoiding unnecessary network traffic.

@lynxplay
Copy link
Contributor

lynxplay commented Nov 1, 2025

Superseded by #12960

@lynxplay lynxplay closed this Nov 1, 2025
@github-project-automation github-project-automation bot moved this from Awaiting review to Closed in Paper PR Queue Nov 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

Item desync when using PlayerItemConsumeEvent#setReplacement with same ItemStack

3 participants