Inventory fixes and state ID emulation

- Introduce a state ID incrementation emulation. This prevents the server from spamming back with tons of set content packets, and can instead reply with set slot packets.
- Fix what we were sending as a carried item in the ServerboundContainerClickPacket.

core/src/main/java/org/geysermc/geyser/inventory/Inventory.java

@@ -33,23 +33,31 @@
 import lombok.Getter;
 import lombok.NonNull;
 import lombok.Setter;
+import lombok.ToString;
 import org.geysermc.geyser.GeyserImpl;
 import org.geysermc.geyser.session.GeyserSession;
 
 import java.util.Arrays;
 
+@ToString
 public class Inventory {
 
     @Getter
     protected final int id;
 
     /**
-     * If this is out of sync with the server, the server will resync items.
-     * Since Java Edition 1.17.1.
+     * The Java inventory state ID from the server. As of Java Edition 1.18.1 this value has one instance per player.
+     * If this is out of sync with the server when a packet containing it is handled, the server will resync items.
+     * This field has existed since Java Edition 1.17.1.
      */
     @Getter
     @Setter
     private int stateId;
+    /**
+     * See {@link org.geysermc.geyser.inventory.click.ClickPlan#execute(boolean)}; used as a hack
+     */
+    @Getter
+    private int nextStateId = -1;
 
     @Getter
     protected final int size;
@@ -123,7 +131,7 @@ public void setItem(int slot, @NonNull GeyserItemStack newItem, GeyserSession se
         }
     }
 
-    protected static void updateItemNetId(GeyserItemStack oldItem, GeyserItemStack newItem, GeyserSession session) {
+    protected void updateItemNetId(GeyserItemStack oldItem, GeyserItemStack newItem, GeyserSession session) {
         if (!newItem.isEmpty()) {
             if (newItem.getItemData(session).equals(oldItem.getItemData(session), false, false, false)) {
                 newItem.setNetId(oldItem.getNetId());
@@ -133,15 +141,15 @@ protected static void updateItemNetId(GeyserItemStack oldItem, GeyserItemStack n
         }
     }
 
-    @Override
-    public String toString() {
-        return "Inventory{" +
-                "id=" + id +
-                ", size=" + size +
-                ", title='" + title + '\'' +
-                ", items=" + Arrays.toString(items) +
-                ", holderPosition=" + holderPosition +
-                ", holderId=" + holderId +
-                '}';
+    /**
+     * See {@link org.geysermc.geyser.inventory.click.ClickPlan#execute(boolean)} for more details.
+     */
+    public void incrementStateId(int count) {
+        // nextStateId == -1 means that it was not needed until now
+        nextStateId = (nextStateId == -1 ? stateId : nextStateId) + count & Short.MAX_VALUE;
+    }
+
+    public void resetNextStateId() {
+        nextStateId = -1;
     }
 }

core/src/main/java/org/geysermc/geyser/inventory/PlayerInventory.java

@@ -32,7 +32,6 @@
 import org.geysermc.geyser.session.GeyserSession;
 
 public class PlayerInventory extends Inventory {
-
     /**
      * Stores the held item slot, starting at index 0.
      * Add 36 in order to get the network item slot.

core/src/main/java/org/geysermc/geyser/inventory/click/ClickPlan.java

@@ -27,22 +27,24 @@
 
 import com.github.steveice10.mc.protocol.data.game.entity.metadata.ItemStack;
 import com.github.steveice10.mc.protocol.data.game.inventory.ContainerActionType;
+import com.github.steveice10.mc.protocol.data.game.inventory.ContainerType;
 import com.github.steveice10.mc.protocol.packet.ingame.serverbound.inventory.ServerboundContainerClickPacket;
 import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
 import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
 import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
 import it.unimi.dsi.fastutil.ints.IntSet;
-import lombok.Value;
 import org.geysermc.geyser.inventory.GeyserItemStack;
 import org.geysermc.geyser.inventory.Inventory;
-import org.geysermc.geyser.session.GeyserSession;
-import org.geysermc.geyser.translator.inventory.InventoryTranslator;
 import org.geysermc.geyser.inventory.SlotType;
+import org.geysermc.geyser.session.GeyserSession;
 import org.geysermc.geyser.translator.inventory.CraftingInventoryTranslator;
+import org.geysermc.geyser.translator.inventory.InventoryTranslator;
 import org.geysermc.geyser.translator.inventory.PlayerInventoryTranslator;
 import org.geysermc.geyser.util.InventoryUtils;
+import org.jetbrains.annotations.Contract;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.ListIterator;
 
@@ -108,32 +110,30 @@ public void execute(boolean refresh) {
                 refresh = true;
             }
 
+            int stateId = stateIdHack(action);
+
+            simulateAction(action);
+
             ItemStack clickedItemStack;
             if (!planIter.hasNext() && refresh) {
                 clickedItemStack = InventoryUtils.REFRESH_ITEM;
             } else if (action.click.actionType == ContainerActionType.DROP_ITEM || action.slot == Click.OUTSIDE_SLOT) {
                 clickedItemStack = null;
             } else {
-                clickedItemStack = getItem(action.slot).getItemStack();
-            }
-
-            Int2ObjectMap<ItemStack> affectedSlots = new Int2ObjectOpenHashMap<>();
-            for (Int2ObjectMap.Entry<GeyserItemStack> simulatedSlot : simulatedItems.int2ObjectEntrySet()) {
-                affectedSlots.put(simulatedSlot.getIntKey(), simulatedSlot.getValue().getItemStack());
+                // The action must be simulated first as Java expects the new contents of the cursor (as of 1.18.1)
+                clickedItemStack = simulatedCursor.getItemStack();
             }
 
             ServerboundContainerClickPacket clickPacket = new ServerboundContainerClickPacket(
                     inventory.getId(),
-                    inventory.getStateId(),
+                    stateId,
                     action.slot,
                     action.click.actionType,
                     action.click.action,
                     clickedItemStack,
-                    affectedSlots
+                    Collections.emptyMap() // Anything else we change, at this time, should have a packet sent to address
             );
 
-            simulateAction(action);
-
             session.sendDownstreamPacket(clickPacket);
         }
 
@@ -243,6 +243,67 @@ private void simulateAction(ClickAction action) {
         }
     }
 
+    private int stateIdHack(ClickAction action) {
+        int stateId;
+        if (inventory.getNextStateId() != -1) {
+            stateId = inventory.getNextStateId();
+        } else {
+            stateId = inventory.getStateId();
+        }
+
+        // This is a hack.
+        // Java will never ever send more than one container click packet per set of actions.
+        // Bedrock might, and this would generally fall into one of two categories:
+        // - Bedrock is sending an item directly from one slot to another, without picking it up, that cannot
+        //   be expressed with a shift click
+        // - Bedrock wants to pick up or place an arbitrary amount of items that cannot be expressed from
+        //   one left/right click action.
+        // When Bedrock does one of these actions and sends multiple packets, a 1.17.1+ server will
+        // increment the state ID on each confirmation packet it sends back (I.E. set slot). Then when it
+        // reads our next packet, because we kept the same state ID but the server incremented it, it'll be
+        // desynced and send the entire inventory contents back at us.
+        // This hack therefore increments the state ID to what the server will presumably send back to us.
+        // (This won't be perfect, but should get us through most vanilla situations, and if this is wrong the
+        // server will just send a set content packet back at us)
+        if (inventory.getContainerType() == ContainerType.CRAFTING && CraftingInventoryTranslator.isCraftingGrid(action.slot)) {
+            // 1.18.1 sends a second set slot update for any action in the crafting grid
+            // And an additional packet if something is removed (Mojmap: CraftingContainer#removeItem)
+            //TODO this code kind of really sucks; it's potentially possible to see what Bedrock sends us and send a PlaceRecipePacket
+            int stateIdIncrements;
+            GeyserItemStack clicked = getItem(action.slot);
+            if (action.click == Click.LEFT) {
+                if (!clicked.isEmpty() && !InventoryUtils.canStack(simulatedCursor, clicked)) {
+                    // An item is removed from the crafting table; yes deletion
+                    stateIdIncrements = 3;
+                } else {
+                    // We can stack and we add all the items to the crafting slot; no deletion
+                    stateIdIncrements = 2;
+                }
+            } else if (action.click == Click.RIGHT) {
+                if (simulatedCursor.isEmpty() && !clicked.isEmpty()) {
+                    // Items are taken; yes deletion
+                    stateIdIncrements = 3;
+                } else if ((!simulatedCursor.isEmpty() && clicked.isEmpty()) || InventoryUtils.canStack(simulatedCursor, clicked)) {
+                    // Adding our cursor item to the slot; no deletion
+                    stateIdIncrements = 2;
+                } else {
+                    // ?? nothing I guess
+                    stateIdIncrements = 2;
+                }
+            } else {
+                if (session.getGeyser().getConfig().isDebugMode()) {
+                    session.getGeyser().getLogger().debug("Not sure how to handle state ID hack in crafting table: " + plan);
+                }
+                stateIdIncrements = 2;
+            }
+            inventory.incrementStateId(stateIdIncrements);
+        } else {
+            inventory.incrementStateId(1);
+        }
+
+        return stateId;
+    }
+
     //TODO
     private void reduceCraftingGrid(boolean makeAll) {
         if (gridSize == -1)
@@ -272,8 +333,9 @@ private void reduceCraftingGrid(boolean makeAll) {
     }
 
     /**
-     * @return a new set of all affected slots. This isn't a constant variable; it's newly generated each time it is run.
+     * @return a new set of all affected slots.
      */
+    @Contract("-> new")
     public IntSet getAffectedSlots() {
         IntSet affectedSlots = new IntOpenHashSet();
         for (ClickAction action : plan) {
@@ -284,13 +346,6 @@ public IntSet getAffectedSlots() {
         return affectedSlots;
     }
 
-    @Value
-    private static class ClickAction {
-        Click click;
-        /**
-         * Java slot
-         */
-        int slot;
-        boolean force;
+    private record ClickAction(Click click, int slot, boolean force) {
     }
 }

core/src/main/java/org/geysermc/geyser/translator/inventory/CraftingInventoryTranslator.java

@@ -47,7 +47,7 @@ public SlotType getSlotType(int javaSlot) {
 
     @Override
     public BedrockContainerSlot javaSlotToBedrockContainer(int slot) {
-        if (slot >= 1 && slot <= 9) {
+        if (isCraftingGrid(slot)) {
             return new BedrockContainerSlot(ContainerSlotType.CRAFTING_INPUT, slot + 31);
         }
         if (slot == 0) {
@@ -76,4 +76,8 @@ public int javaSlotToBedrock(int slot) {
         }
         return super.javaSlotToBedrock(slot);
     }
+
+    public static boolean isCraftingGrid(int slot) {
+        return slot >= 1 && slot <= 9;
+    }
 }

core/src/main/java/org/geysermc/geyser/translator/inventory/InventoryTranslator.java

@@ -184,6 +184,9 @@ public void translateRequests(GeyserSession session, Inventory inventory, List<I
             InventoryUtils.updateCursor(session);
             updateInventory(session, inventory);
         }
+
+        // We're done with our batch of inventory requests so this hack should be reset
+        inventory.resetNextStateId();
     }
 
     public ItemStackResponsePacket.Response translateRequest(GeyserSession session, Inventory inventory, ItemStackRequest request) {

core/src/main/java/org/geysermc/geyser/translator/protocol/java/inventory/JavaContainerSetSlotTranslator.java

@@ -71,6 +71,7 @@ public void translate(GeyserSession session, ClientboundContainerSetSlotPacket p
         if (inventory == null)
             return;
 
+        // Intentional behavior here below the cursor; Minecraft 1.18.1 also does this.
         inventory.setStateId(packet.getStateId());
 
         InventoryTranslator translator = session.getInventoryTranslator();