Properly handle block change acknowledgements in 1.18.2 -> 1.19

bukkit/src/main/java/com/viaversion/viaversion/bukkit/providers/BukkitAckSequenceProvider.java

@@ -19,6 +19,7 @@ package com.viaversion.viaversion.bukkit.providers;
 
 import com.viaversion.viaversion.ViaVersionPlugin;
 import com.viaversion.viaversion.api.connection.UserConnection;
+import com.viaversion.viaversion.api.minecraft.BlockPosition;
 import com.viaversion.viaversion.api.protocol.version.ProtocolVersion;
 import com.viaversion.viaversion.bukkit.tasks.v1_18_2to1_19.AckSequenceTask;
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.provider.AckSequenceProvider;
@@ -33,7 +34,9 @@ public final class BukkitAckSequenceProvider extends AckSequenceProvider {
     }
 
     @Override
-    public void handleSequence(final UserConnection connection, final int sequence) {
+    public void handleSequence(final UserConnection connection, final BlockPosition position, final int sequence) {
+        if (sequence <= 0) return; // Does not need to be acked
+
         final SequenceStorage sequenceStorage = connection.get(SequenceStorage.class);
         final int previousSequence = sequenceStorage.setSequenceId(sequence);
         if (previousSequence == -1) {
@@ -45,4 +48,10 @@ public final class BukkitAckSequenceProvider extends AckSequenceProvider {
             }
         }
     }
+
+    @Override
+    public void handleBlockChange(final UserConnection connection, final BlockPosition position) {
+        // no op on bukkit
+    }
+
 }

common/src/main/java/com/viaversion/viaversion/protocols/v1_18_2to1_19/provider/AckSequenceProvider.java

@@ -18,18 +18,37 @@
 package com.viaversion.viaversion.protocols.v1_18_2to1_19.provider;
 
 import com.viaversion.viaversion.api.connection.UserConnection;
+import com.viaversion.viaversion.api.minecraft.BlockPosition;
 import com.viaversion.viaversion.api.platform.providers.Provider;
 import com.viaversion.viaversion.api.protocol.packet.PacketWrapper;
 import com.viaversion.viaversion.api.type.Types;
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.Protocol1_18_2To1_19;
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.packet.ClientboundPackets1_19;
+import com.viaversion.viaversion.protocols.v1_18_2to1_19.storage.SequenceStorage;
 
 public class AckSequenceProvider implements Provider {
 
-    public void handleSequence(final UserConnection connection, final int sequence) {
+    public void handleSequence(final UserConnection connection, final BlockPosition position, final int sequence) {
+        if (sequence <= 0) return; // Does not need to be acked
+
+        if (position == null) {
+            // Acknowledge immediately if the position isn't known
             final PacketWrapper ackPacket = PacketWrapper.create(ClientboundPackets1_19.BLOCK_CHANGED_ACK, connection);
             ackPacket.write(Types.VAR_INT, sequence);
             ackPacket.send(Protocol1_18_2To1_19.class);
+        } else {
+            // Store sequence to be acknowledged when the block change is received
+            connection.get(SequenceStorage.class).addPendingBlockChange(position, sequence);
+        }
+    }
+
+    public void handleBlockChange(final UserConnection connection, final BlockPosition position) {
+        final int sequence = connection.get(SequenceStorage.class).removePendingBlockChange(position);
+        if (sequence > 0) { // Acknowledge if pending sequence was found
+            final PacketWrapper ackPacket = PacketWrapper.create(ClientboundPackets1_19.BLOCK_CHANGED_ACK, connection);
+            ackPacket.write(Types.VAR_INT, sequence);
+            ackPacket.scheduleSend(Protocol1_18_2To1_19.class);
+        }
     }
 
 }

common/src/main/java/com/viaversion/viaversion/protocols/v1_18_2to1_19/rewriter/EntityPacketRewriter1_19.java

@@ -26,6 +26,7 @@ import com.viaversion.nbt.tag.NumberTag;
 import com.viaversion.viaversion.api.Via;
 import com.viaversion.viaversion.api.data.ParticleMappings;
 import com.viaversion.viaversion.api.data.entity.DimensionData;
+import com.viaversion.viaversion.api.data.entity.EntityTracker;
 import com.viaversion.viaversion.api.minecraft.BlockPosition;
 import com.viaversion.viaversion.api.minecraft.Particle;
 import com.viaversion.viaversion.api.minecraft.entities.EntityType;
@@ -41,6 +42,7 @@ import com.viaversion.viaversion.protocols.v1_17_1to1_18.packet.ClientboundPacke
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.Protocol1_18_2To1_19;
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.packet.ClientboundPackets1_19;
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.storage.DimensionRegistryStorage;
+import com.viaversion.viaversion.protocols.v1_18_2to1_19.storage.SequenceStorage;
 import com.viaversion.viaversion.rewriter.EntityRewriter;
 import com.viaversion.viaversion.util.Key;
 import com.viaversion.viaversion.util.Pair;
@@ -229,6 +231,13 @@ public final class EntityPacketRewriter1_19 extends EntityRewriter<ClientboundPa
                 map(Types.BOOLEAN); // Flat
                 map(Types.BOOLEAN); // Keep player attributes
                 create(Types.OPTIONAL_GLOBAL_POSITION, null); // Last death location
+                handler(wrapper -> {
+                    final String world = wrapper.get(Types.STRING, 1);
+                    final EntityTracker tracker = tracker(wrapper.user());
+                    if (tracker.currentWorld() != null && !tracker.currentWorld().equals(world)) {
+                        wrapper.user().get(SequenceStorage.class).clearPendingBlockChanges();
+                    }
+                });
                 handler(worldDataTrackerHandlerByKey());
             }
         });

common/src/main/java/com/viaversion/viaversion/protocols/v1_18_2to1_19/rewriter/ItemPacketRewriter1_19.java

@@ -19,7 +19,8 @@ package com.viaversion.viaversion.protocols.v1_18_2to1_19.rewriter;
 
 import com.viaversion.viaversion.api.Via;
 import com.viaversion.viaversion.api.data.ParticleMappings;
-import com.viaversion.viaversion.api.protocol.remapper.PacketHandler;
+import com.viaversion.viaversion.api.minecraft.BlockFace;
+import com.viaversion.viaversion.api.minecraft.BlockPosition;
 import com.viaversion.viaversion.api.protocol.remapper.PacketHandlers;
 import com.viaversion.viaversion.api.type.Types;
 import com.viaversion.viaversion.protocols.v1_17_1to1_18.packet.ClientboundPackets1_18;
@@ -113,7 +114,14 @@ public final class ItemPacketRewriter1_19 extends ItemRewriter<ClientboundPacket
                 map(Types.VAR_INT); // Action
                 map(Types.BLOCK_POSITION1_14); // Block position
                 map(Types.UNSIGNED_BYTE); // Direction
-                handler(sequenceHandler());
+                handler(wrapper -> {
+                    final AckSequenceProvider ackSequenceProvider = Via.getManager().getProviders().get(AckSequenceProvider.class);
+                    final BlockPosition position = wrapper.get(Types.BLOCK_POSITION1_14, 0);
+                    final BlockPosition relativePosition = position.getRelative(getBlockFace(wrapper.get(Types.UNSIGNED_BYTE, 0)));
+                    final int sequence = wrapper.read(Types.VAR_INT);
+                    ackSequenceProvider.handleSequence(wrapper.user(), position, sequence);
+                    ackSequenceProvider.handleSequence(wrapper.user(), relativePosition, sequence);
+                });
             }
         });
         protocol.registerServerbound(ServerboundPackets1_19.USE_ITEM_ON, new PacketHandlers() {
@@ -126,25 +134,42 @@ public final class ItemPacketRewriter1_19 extends ItemRewriter<ClientboundPacket
                 map(Types.FLOAT); // Y
                 map(Types.FLOAT); // Z
                 map(Types.BOOLEAN); // Inside
-                handler(sequenceHandler());
+                handler(wrapper -> {
+                    final AckSequenceProvider ackSequenceProvider = Via.getManager().getProviders().get(AckSequenceProvider.class);
+                    final BlockPosition position = wrapper.get(Types.BLOCK_POSITION1_14, 0);
+                    final BlockPosition relativePosition = position.getRelative(getBlockFace(wrapper.get(Types.VAR_INT, 1)));
+                    final int sequence = wrapper.read(Types.VAR_INT);
+                    ackSequenceProvider.handleSequence(wrapper.user(), position, sequence);
+                    ackSequenceProvider.handleSequence(wrapper.user(), relativePosition, sequence);
+                });
             }
         });
         protocol.registerServerbound(ServerboundPackets1_19.USE_ITEM, new PacketHandlers() {
             @Override
             public void register() {
                 map(Types.VAR_INT); // Hand
-                handler(sequenceHandler());
+                handler(wrapper -> {
+                    final AckSequenceProvider ackSequenceProvider = Via.getManager().getProviders().get(AckSequenceProvider.class);
+                    final int sequence = wrapper.read(Types.VAR_INT);
+                    ackSequenceProvider.handleSequence(wrapper.user(), null, sequence);
+                });
             }
         });
 
         new RecipeRewriter<>(protocol).register(ClientboundPackets1_18.UPDATE_RECIPES);
     }
 
-    private PacketHandler sequenceHandler() {
+    private BlockFace getBlockFace(int direction) {
-        return wrapper -> {
+        direction = Math.abs(direction % 6);
-            final int sequence = wrapper.read(Types.VAR_INT);
+        return switch (direction) {
-            final AckSequenceProvider provider = Via.getManager().getProviders().get(AckSequenceProvider.class);
+            case 0 -> BlockFace.BOTTOM;
-            provider.handleSequence(wrapper.user(), sequence);
+            case 1 -> BlockFace.TOP;
+            case 2 -> BlockFace.NORTH;
+            case 3 -> BlockFace.SOUTH;
+            case 4 -> BlockFace.WEST;
+            case 5 -> BlockFace.EAST;
+            default -> throw new IllegalStateException("Unexpected value: " + direction);
         };
     }
+
 }

common/src/main/java/com/viaversion/viaversion/protocols/v1_18_2to1_19/rewriter/WorldPacketRewriter1_19.java

@@ -17,11 +17,16 @@
  */
 package com.viaversion.viaversion.protocols.v1_18_2to1_19.rewriter;
 
+import com.viaversion.viaversion.api.Via;
+import com.viaversion.viaversion.api.minecraft.BlockChangeRecord;
+import com.viaversion.viaversion.api.minecraft.BlockPosition;
+import com.viaversion.viaversion.api.protocol.remapper.PacketHandlers;
 import com.viaversion.viaversion.api.type.Types;
 import com.viaversion.viaversion.api.type.types.chunk.ChunkType1_18;
 import com.viaversion.viaversion.protocols.v1_17_1to1_18.packet.ClientboundPackets1_18;
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.Protocol1_18_2To1_19;
 import com.viaversion.viaversion.protocols.v1_18_2to1_19.packet.ServerboundPackets1_19;
+import com.viaversion.viaversion.protocols.v1_18_2to1_19.provider.AckSequenceProvider;
 import com.viaversion.viaversion.rewriter.BlockRewriter;
 
 public final class WorldPacketRewriter1_19 {
@@ -29,11 +34,35 @@ public final class WorldPacketRewriter1_19 {
     public static void register(final Protocol1_18_2To1_19 protocol) {
         final BlockRewriter<ClientboundPackets1_18> blockRewriter = BlockRewriter.for1_14(protocol);
         blockRewriter.registerBlockEvent(ClientboundPackets1_18.BLOCK_EVENT);
-        blockRewriter.registerBlockUpdate(ClientboundPackets1_18.BLOCK_UPDATE);
-        blockRewriter.registerSectionBlocksUpdate(ClientboundPackets1_18.SECTION_BLOCKS_UPDATE);
         blockRewriter.registerLevelEvent(ClientboundPackets1_18.LEVEL_EVENT, 1010, 2001);
         blockRewriter.registerLevelChunk1_19(ClientboundPackets1_18.LEVEL_CHUNK_WITH_LIGHT, ChunkType1_18::new);
 
+        protocol.registerClientbound(ClientboundPackets1_18.BLOCK_UPDATE, wrapper -> {
+            final BlockPosition position = wrapper.passthrough(Types.BLOCK_POSITION1_14);
+            wrapper.write(Types.VAR_INT, protocol.getMappingData().getNewBlockStateId(wrapper.read(Types.VAR_INT)));
+            Via.getManager().getProviders().get(AckSequenceProvider.class).handleBlockChange(wrapper.user(), position);
+        });
+        protocol.registerClientbound(ClientboundPackets1_18.SECTION_BLOCKS_UPDATE, new PacketHandlers() {
+            @Override
+            public void register() {
+                map(Types.LONG); // Chunk position
+                map(Types.BOOLEAN); // Suppress light updates
+                handler(wrapper -> {
+                    final AckSequenceProvider ackSequenceProvider = Via.getManager().getProviders().get(AckSequenceProvider.class);
+                    final long chunkPosition = wrapper.get(Types.LONG, 0);
+                    final int x = (int) ((chunkPosition >> 42) & 0x3FFFFFL) << 4;
+                    final int z = (int) ((chunkPosition >> 20) & 0x3FFFFFL) << 4;
+                    final int y = (int) (chunkPosition & 0xFFFL) << 4;
+
+                    for (BlockChangeRecord record : wrapper.passthrough(Types.VAR_LONG_BLOCK_CHANGE_ARRAY)) {
+                        record.setBlockId(protocol.getMappingData().getNewBlockStateId(record.getBlockId()));
+                        final BlockPosition position = new BlockPosition(x + record.getSectionX(), y + record.getSectionY(), z + record.getSectionZ());
+                        ackSequenceProvider.handleBlockChange(wrapper.user(), position);
+                    }
+                });
+            }
+        });
+
         protocol.cancelClientbound(ClientboundPackets1_18.BLOCK_BREAK_ACK);
 
         protocol.registerServerbound(ServerboundPackets1_19.SET_BEACON, wrapper -> {

common/src/main/java/com/viaversion/viaversion/protocols/v1_18_2to1_19/storage/SequenceStorage.java

@@ -18,17 +18,18 @@
 package com.viaversion.viaversion.protocols.v1_18_2to1_19.storage;
 
 import com.viaversion.viaversion.api.connection.StorableObject;
+import com.viaversion.viaversion.api.minecraft.BlockPosition;
+import it.unimi.dsi.fastutil.objects.Object2IntLinkedOpenHashMap;
+import it.unimi.dsi.fastutil.objects.Object2IntSortedMap;
 
 public final class SequenceStorage implements StorableObject {
 
+    // Bukkit fix
     private final Object lock = new Object();
     private int sequenceId = -1;
 
-    public int sequenceId() {
+    // Protocol level fix
-        synchronized (lock) {
+    private final Object2IntSortedMap<BlockPosition> pendingBlockChanges = new Object2IntLinkedOpenHashMap<>();
-            return sequenceId;
-        }
-    }
 
     public int setSequenceId(final int sequenceId) {
         synchronized (lock) {
@@ -37,4 +38,28 @@ public final class SequenceStorage implements StorableObject {
             return previousSequence;
         }
     }
+
+    public void addPendingBlockChange(final BlockPosition position, final int id) {
+        final int lastSequence = this.pendingBlockChanges.isEmpty() ? 0 : this.pendingBlockChanges.getInt(this.pendingBlockChanges.lastKey());
+        if (id > 0 && id >= lastSequence && !this.pendingBlockChanges.containsKey(position)) {
+            // Store the last 200 pending block changes. Some may never get acked, so we need to limit the size.
+            while (this.pendingBlockChanges.size() > 200) {
+                this.pendingBlockChanges.removeInt(this.pendingBlockChanges.firstKey());
+            }
+            this.pendingBlockChanges.put(position, id);
+        }
+    }
+
+    public int removePendingBlockChange(final BlockPosition position) {
+        final int ackedSequence = this.pendingBlockChanges.getInt(position); // 0 if not found
+        if (ackedSequence > 0) {
+            this.pendingBlockChanges.values().removeIf(sequence -> sequence <= ackedSequence); // Remove all previous pending changes
+        }
+        return ackedSequence;
+    }
+
+    public void clearPendingBlockChanges() {
+        this.pendingBlockChanges.clear();
+    }
+
 }