Prevent 1.16 -> 1.17 cursor desync with dragging (mostly fixes #389) (#445)

common/src/main/java/com/viaversion/viabackwards/protocol/protocol1_16_4to1_17/Protocol1_16_4To1_17.java

@@ -25,6 +25,7 @@ import com.viaversion.viabackwards.api.rewriters.TranslatableRewriter;
 import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.packets.BlockItemPackets1_17;
 import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.packets.EntityPackets1_17;
 import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.storage.PingRequests;
+import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.storage.PlayerLastCursorItem;
 import com.viaversion.viaversion.api.connection.UserConnection;
 import com.viaversion.viaversion.api.minecraft.RegistryType;
 import com.viaversion.viaversion.api.minecraft.TagData;
@@ -249,6 +250,7 @@ public final class Protocol1_16_4To1_17 extends BackwardsProtocol<ClientboundPac
     public void init(UserConnection user) {
         addEntityTracker(user, new EntityTrackerBase(user, Entity1_17Types.PLAYER));
         user.put(new PingRequests());
+        user.put(new PlayerLastCursorItem());
     }
 
     @Override

common/src/main/java/com/viaversion/viabackwards/protocol/protocol1_16_4to1_17/packets/BlockItemPackets1_17.java

@@ -23,10 +23,12 @@ import com.viaversion.viabackwards.api.rewriters.MapColorRewriter;
 import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.Protocol1_16_4To1_17;
 import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.data.MapColorRewrites;
 import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.storage.PingRequests;
+import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.storage.PlayerLastCursorItem;
 import com.viaversion.viaversion.api.data.entity.EntityTracker;
 import com.viaversion.viaversion.api.minecraft.BlockChangeRecord;
 import com.viaversion.viaversion.api.minecraft.chunks.Chunk;
 import com.viaversion.viaversion.api.minecraft.chunks.ChunkSection;
+import com.viaversion.viaversion.api.minecraft.item.Item;
 import com.viaversion.viaversion.api.protocol.packet.PacketWrapper;
 import com.viaversion.viaversion.api.protocol.remapper.PacketRemapper;
 import com.viaversion.viaversion.api.type.Type;
@@ -64,7 +66,6 @@ public final class BlockItemPackets1_17 extends ItemRewriter<Protocol1_16_4To1_1
 
         registerSetCooldown(ClientboundPackets1_17.COOLDOWN);
         registerWindowItems(ClientboundPackets1_17.WINDOW_ITEMS, Type.FLAT_VAR_INT_ITEM_ARRAY);
-        registerSetSlot(ClientboundPackets1_17.SET_SLOT, Type.FLAT_VAR_INT_ITEM);
         registerEntityEquipmentArray(ClientboundPackets1_17.ENTITY_EQUIPMENT, Type.FLAT_VAR_INT_ITEM);
         registerTradeList(ClientboundPackets1_17.TRADE_LIST, Type.FLAT_VAR_INT_ITEM);
         registerAdvancements(ClientboundPackets1_17.ADVANCEMENTS, Type.FLAT_VAR_INT_ITEM);
@@ -82,24 +83,87 @@ public final class BlockItemPackets1_17 extends ItemRewriter<Protocol1_16_4To1_1
             }
         });
 
-        //TODO This will cause desync issues for players under certain circumstances, but mostly works:tm:
+        // TODO Since the carried and modified items are typically set incorrectly, the server sends unnecessary
+        // set slot packets after practically every window click, since it thinks the client and server
+        // inventories are desynchronized. Ideally, we want to store a replica of each container state, and update
+        // it appropriately for both serverbound and clientbound window packets, then fill in the carried
+        // and modified items array as appropriate here. That would be a ton of work and replicated vanilla code,
+        // and the hack below mitigates the worst side effects of this issue, which is an incorrect carried item
+        // sent to the client when a right/left click drag is started. It works, at least for now...
         protocol.registerServerbound(ServerboundPackets1_16_2.CLICK_WINDOW, new PacketRemapper() {
             @Override
             public void registerMap() {
-                map(Type.UNSIGNED_BYTE); // Window Id
-                map(Type.SHORT); // Slot
-                map(Type.BYTE); // Button
-                map(Type.SHORT, Type.NOTHING); // Action id - removed
-                map(Type.VAR_INT); // Mode
+                map(Type.UNSIGNED_BYTE);
                 handler(wrapper -> {
+                    short slot = wrapper.passthrough(Type.SHORT); // Slot
+                    byte button = wrapper.passthrough(Type.BYTE); // Button
+                    wrapper.read(Type.SHORT); // Action id - removed
+                    int mode = wrapper.passthrough(Type.VAR_INT); // Mode
+                    Item clicked = handleItemToServer(wrapper.read(Type.FLAT_VAR_INT_ITEM)); // Clicked item
+
                     // The 1.17 client would check the entire inventory for changes before -> after a click and send the changed slots here
                     wrapper.write(Type.VAR_INT, 0); // Empty array of slot+item
 
-                    // Expected is the carried item after clicking, old clients send the clicked one (*mostly* being the same)
-                    handleItemToServer(wrapper.passthrough(Type.FLAT_VAR_INT_ITEM));
+                    PlayerLastCursorItem state = wrapper.user().get(PlayerLastCursorItem.class);
+                    if (mode == 0 && button == 0 && clicked != null) {
+                        // Left click PICKUP
+                        // Carried item will (usually) be the entire clicked stack
+                        state.setLastCursorItem(clicked);
+                    } else if (mode == 0 && button == 1 && clicked != null) {
+                        // Right click PICKUP
+                        // Carried item will (usually) be half of the clicked stack (rounding up)
+                        // if the clicked slot is empty, otherwise it will (usually) be the whole clicked stack
+                        if (state.isSet()) {
+                            state.setLastCursorItem(clicked);
+                        } else {
+                            state.setLastCursorItem(clicked, (clicked.amount() + 1) / 2);
+                        }
+                    } else if (mode == 5 && slot == -999 && (button == 0 || button == 4)) {
+                        // Start drag (left or right click)
+                        // Preserve guessed carried item and forward to server
+                        // This mostly fixes the click and drag ghost item issue
+
+                        // no-op
+                    } else {
+                        // Carried item unknown (TODO)
+                        state.setLastCursorItem(null);
+                    }
+
+                    Item carried = state.getLastCursorItem();
+                    if (carried == null) {
+                        // Expected is the carried item after clicking, old clients send the clicked one (*mostly* being the same)
+                        wrapper.write(Type.FLAT_VAR_INT_ITEM, clicked);
+                    } else {
+                        wrapper.write(Type.FLAT_VAR_INT_ITEM, carried);
+                    }
                 });
             }
         });
+
+        protocol.registerClientbound(ClientboundPackets1_17.SET_SLOT, new PacketRemapper() {
+            @Override
+            public void registerMap() {
+                handler(wrapper -> {
+                    short windowId = wrapper.passthrough(Type.UNSIGNED_BYTE);
+                    short slot = wrapper.passthrough(Type.SHORT);
+
+                    Item carried = wrapper.read(Type.FLAT_VAR_INT_ITEM);
+                    if (carried != null && windowId == -1 && slot == -1) {
+                        // This is related to the hack to fix click and drag ghost items above.
+                        // After a completed drag, we have no idea how many items remain on the cursor,
+                        // and vanilla logic replication would be required to calculate the value.
+                        // When the click drag complete packet is sent, we will send an incorrect
+                        // carried item, and the server will helpfully send this packet allowing us
+                        // to update the internal state. This is necessary for fixing multiple sequential
+                        // click drag actions without intermittent pickup actions.
+                        wrapper.user().get(PlayerLastCursorItem.class).setLastCursorItem(carried);
+                    }
+
+                    wrapper.write(Type.FLAT_VAR_INT_ITEM, handleItemToClient(carried));
+                });
+            }
+        });
+
         protocol.registerServerbound(ServerboundPackets1_16_2.WINDOW_CONFIRMATION, null, new PacketRemapper() {
             @Override
             public void registerMap() {

common/src/main/java/com/viaversion/viabackwards/protocol/protocol1_16_4to1_17/storage/PlayerLastCursorItem.java

@@ -0,0 +1,52 @@
+/*
+ * This file is part of ViaBackwards - https://github.com/ViaVersion/ViaBackwards
+ * Copyright (C) 2016-2022 ViaVersion and contributors
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.storage;
+
+import com.viaversion.viaversion.api.connection.StorableObject;
+import com.viaversion.viaversion.api.minecraft.item.DataItem;
+import com.viaversion.viaversion.api.minecraft.item.Item;
+
+public class PlayerLastCursorItem implements StorableObject {
+    private Item lastCursorItem;
+
+    public Item getLastCursorItem() {
+        return copyItem(lastCursorItem);
+    }
+
+    public void setLastCursorItem(Item item) {
+        this.lastCursorItem = copyItem(item);
+    }
+
+    public void setLastCursorItem(Item item, int amount) {
+        this.lastCursorItem = copyItem(item);
+        this.lastCursorItem.setAmount(amount);
+    }
+
+    public boolean isSet() {
+        return lastCursorItem != null;
+    }
+
+    private static Item copyItem(Item item) {
+        if (item == null) {
+            return null;
+        }
+        Item copy = new DataItem(item);
+        copy.setTag(copy.tag() == null ? null : copy.tag().clone());
+        return copy;
+    }
+}

common/src/main/java/com/viaversion/viabackwards/protocol/protocol1_17to1_17_1/Protocol1_17To1_17_1.java

@@ -18,6 +18,7 @@
 package com.viaversion.viabackwards.protocol.protocol1_17to1_17_1;
 
 import com.viaversion.viabackwards.api.BackwardsProtocol;
+import com.viaversion.viabackwards.protocol.protocol1_16_4to1_17.storage.PlayerLastCursorItem;
 import com.viaversion.viabackwards.protocol.protocol1_17to1_17_1.storage.InventoryStateIds;
 import com.viaversion.viaversion.api.connection.UserConnection;
 import com.viaversion.viaversion.api.minecraft.item.Item;
@@ -91,7 +92,17 @@ public final class Protocol1_17To1_17_1 extends BackwardsProtocol<ClientboundPac
                     wrapper.write(Type.FLAT_VAR_INT_ITEM_ARRAY, wrapper.read(Type.FLAT_VAR_INT_ITEM_ARRAY_VAR_INT));
 
                     // Carried item - should work without adding it to the array above
-                    wrapper.read(Type.FLAT_VAR_INT_ITEM);
+                    Item carried = wrapper.read(Type.FLAT_VAR_INT_ITEM);
+
+                    PlayerLastCursorItem lastCursorItem = wrapper.user().get(PlayerLastCursorItem.class);
+                    if (lastCursorItem != null) {
+                        // For click drag ghost item fix -- since the state ID is always wrong,
+                        // the server always resends the entire window contents after a drag action,
+                        // which is useful since we need to update the carried item in preparation
+                        // for a subsequent drag
+
+                        lastCursorItem.setLastCursorItem(carried);
+                    }
                 });
             }
         });