Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Inventory desync & block update happening when canceling lily pad in BlockPlaceEvent

Open AnttiMK opened this issue 2 years ago • 3 comments

Expected behavior

The lily pad item to remain in my inventory (without ghosting) & block updates to not happen when canceling the placement of a lily pad on water

Observed/Actual behavior

When a lily pad is placed on water and the BlockPlaceEvent is canceled, the item ghosts out of the player's inventory and a block update happens. https://github.com/PaperMC/Paper/assets/13612979/78b1558e-c8f5-4d33-bdf3-5e8f53f74295

Steps/models to reproduce

  1. Install test plugin
  2. Create a floating body of water
  3. Place a lily pad on the water, observe item ghosted out of inventory & block update happened, causing water to flow
  4. Place a block on top of the water, no block update happened nor did the item go invisible

Plugin and Datapack List

WorldEdit + test plugin:

@EventHandler(ignoreCancelled = true)
public void onBlockPlace(BlockPlaceEvent event) {
    if (event.getBlock().getType().equals(Material.LILY_PAD)) {
        event.getPlayer().sendPlainMessage("Lilypad placed and canceled");
        event.setCancelled(true);
    }
    if (event.getBlock().getType().equals(Material.STONE)) {
        event.getPlayer().sendPlainMessage("Stone placed and canceled");
        event.setCancelled(true);
    }
}

Paper version

This server is running Paper version git-Paper-83 (MC: 1.20.1) (Implementing API version 1.20.1-R0.1-SNAPSHOT) (Git: c793bd9) You are running the latest version

Other

No response

AnttiMK avatar Jul 17 '23 17:07 AnttiMK

Replicable, generally PlaceOnWaterBlockItem's always return InteractionResult.PASS. Inventory re-sync only happens on either SUCCESS or CONSUME, which I honestly do not know why this isn't one of those, given the item is consumed.

lynxplay avatar Jul 17 '23 17:07 lynxplay

For documentation purposes,

diff --git a/src/main/java/net/minecraft/world/item/BlockItem.java b/src/main/java/net/minecraft/world/item/BlockItem.java
index ebee8de2e..8b76244f5 100644
--- a/src/main/java/net/minecraft/world/item/BlockItem.java
+++ b/src/main/java/net/minecraft/world/item/BlockItem.java
@@ -116,7 +116,7 @@ public class BlockItem extends Item {
                             if (placeEvent != null && (placeEvent.isCancelled() || !placeEvent.canBuild())) {
                                 blockstate.update(true, false);
 
-                                if (this instanceof SolidBucketItem) {
+                                if (true || this instanceof SolidBucketItem) {
                                     ((ServerPlayer) entityhuman).connection.send(new net.minecraft.network.protocol.game.ClientboundBlockUpdatePacket(world, blockposition.below())); // Paper - update block below
                                     ((ServerPlayer) entityhuman).getBukkitEntity().updateInventory(); // SPIGOT-4541
                                 }

should fix this. When I find time to properly test and open a PR, I'll get around to this.

lynxplay avatar Jul 18 '23 09:07 lynxplay

image It seems the linked PR only fixed the inventory desync? Although the desync doesn't happen anymore, placing a lily pad and canceling the place event still triggers a block update

AnttiMK avatar Aug 25 '23 16:08 AnttiMK