Fix moving items from output slot over multiple actions (#4993)

* Fix moving items from output slot over multiple actions

* Refactor and use temp slot

* Ensure source slot is not cursor

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

@@ -162,6 +162,27 @@ public final class ClickPlan {
         finished = true;
     }
 
+    public Inventory getInventory() {
+        return inventory;
+    }
+
+    /**
+     * Test if the item stacks with another item in the specified slot.
+     * This will check the simulated inventory without copying.
+     */
+    public boolean canStack(int slot, GeyserItemStack item) {
+        GeyserItemStack slotItem = simulatedItems.getOrDefault(slot, inventory.getItem(slot));
+        return InventoryUtils.canStack(slotItem, item);
+    }
+
+    /**
+     * Test if the specified slot is empty.
+     * This will check the simulated inventory without copying.
+     */
+    public boolean isEmpty(int slot) {
+        return simulatedItems.getOrDefault(slot, inventory.getItem(slot)).isEmpty();
+    }
+
     public GeyserItemStack getItem(int slot) {
         return simulatedItems.computeIfAbsent(slot, k -> inventory.getItem(slot).copy());
     }

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

@@ -201,6 +201,9 @@ public abstract class InventoryTranslator {
     public ItemStackResponse translateRequest(GeyserSession session, Inventory inventory, ItemStackRequest request) {
         ClickPlan plan = new ClickPlan(session, this, inventory);
         IntSet affectedSlots = new IntOpenHashSet();
+        int pendingOutput = 0;
+        int savedTempSlot = -1;
+
         for (ItemStackRequestAction action : request.getActions()) {
             GeyserItemStack cursor = session.getPlayerInventory().getCursor();
             switch (action.getType()) {
@@ -241,6 +244,65 @@ public abstract class InventoryTranslator {
                         return rejectRequest(request, false);
                     }
 
+                    // Handle partial transfer of output slot
+                    if (pendingOutput == 0 && !isSourceCursor && getSlotType(sourceSlot) == SlotType.OUTPUT
+                        && transferAction.getCount() < plan.getItem(sourceSlot).getAmount()) {
+                        // Cursor as dest should always be full transfer.
+                        if (isDestCursor) {
+                            return rejectRequest(request);
+                        }
+
+                        if (!plan.getCursor().isEmpty()) {
+                            savedTempSlot = findTempSlot(plan, plan.getCursor(), true);
+                            if (savedTempSlot == -1) {
+                                return rejectRequest(request);
+                            }
+                            plan.add(Click.LEFT, savedTempSlot);
+                        }
+
+                        // Pickup entire stack from output
+                        pendingOutput = plan.getItem(sourceSlot).getAmount();
+                        plan.add(Click.LEFT, sourceSlot);
+                    }
+
+                    // Continue transferring items from output that is currently stored in the cursor
+                    if (pendingOutput > 0) {
+                        if (isSourceCursor || getSlotType(sourceSlot) != SlotType.OUTPUT
+                            || transferAction.getCount() > pendingOutput
+                            || destSlot == savedTempSlot
+                            || isDestCursor) {
+                            return rejectRequest(request);
+                        }
+
+                        // Make sure item can be placed here
+                        GeyserItemStack destItem = plan.getItem(destSlot);
+                        if (!destItem.isEmpty() && !InventoryUtils.canStack(destItem, plan.getCursor())) {
+                            return rejectRequest(request);
+                        }
+
+                        // TODO: Optimize using max stack size
+                        if (pendingOutput == transferAction.getCount()) {
+                            plan.add(Click.LEFT, destSlot);
+                        } else {
+                            for (int i = 0; i < transferAction.getCount(); i++) {
+                                plan.add(Click.RIGHT, destSlot);
+                            }
+                        }
+
+                        pendingOutput -= transferAction.getCount();
+                        if (pendingOutput != plan.getCursor().getAmount()) {
+                            return rejectRequest(request);
+                        }
+
+                        if (pendingOutput == 0 && savedTempSlot != -1) {
+                            plan.add(Click.LEFT, savedTempSlot);
+                            savedTempSlot = -1;
+                        }
+
+                        // Skip to next action
+                        continue;
+                    }
+
                     if (isSourceCursor && isDestCursor) { //???
                         return rejectRequest(request);
                     } else if (isSourceCursor) { //releasing cursor
@@ -271,7 +333,7 @@ public abstract class InventoryTranslator {
                                 return rejectRequest(request);
                             }
                             if (transferAction.getCount() != sourceAmount) {
-                                int tempSlot = findTempSlot(inventory, cursor, false, sourceSlot);
+                                int tempSlot = findTempSlot(plan, cursor, false, sourceSlot);
                                 if (tempSlot == -1) {
                                     return rejectRequest(request);
                                 }
@@ -292,7 +354,7 @@ public abstract class InventoryTranslator {
                     } else { //transfer from one slot to another
                         int tempSlot = -1;
                         if (!plan.getCursor().isEmpty()) {
-                            tempSlot = findTempSlot(inventory, cursor, false, sourceSlot, destSlot);
+                            tempSlot = findTempSlot(plan, cursor, getSlotType(sourceSlot) != SlotType.NORMAL, sourceSlot, destSlot);
                             if (tempSlot == -1) {
                                 return rejectRequest(request);
                             }
@@ -440,6 +502,11 @@ public abstract class InventoryTranslator {
                     return rejectRequest(request);
             }
         }
+
+        if (pendingOutput != 0) {
+            return rejectRequest(request);
+        }
+
         plan.execute(false);
         affectedSlots.addAll(plan.getAffectedSlots());
         return acceptRequest(request, makeContainerEntries(session, inventory, affectedSlots));
@@ -536,7 +603,7 @@ public abstract class InventoryTranslator {
                             }
                         } else {
                             GeyserItemStack cursor = session.getPlayerInventory().getCursor();
-                            int tempSlot = findTempSlot(inventory, cursor, true, sourceSlot, destSlot);
+                            int tempSlot = findTempSlot(plan, cursor, true, sourceSlot, destSlot);
                             if (tempSlot == -1) {
                                 return rejectRequest(request);
                             }
@@ -699,7 +766,7 @@ public abstract class InventoryTranslator {
                     int javaSlot = bedrockSlotToJava(transferAction.getDestination());
                     if (isCursor(transferAction.getDestination())) { //TODO
                         if (timesCrafted > 1) {
-                            tempSlot = findTempSlot(inventory, GeyserItemStack.from(output), true);
+                            tempSlot = findTempSlot(plan, GeyserItemStack.from(output), true);
                             if (tempSlot == -1) {
                                 return rejectRequest(request);
                             }
@@ -836,49 +903,68 @@ public abstract class InventoryTranslator {
     }
 
     /**
-     * Try to find a slot that can temporarily store the given item.
+     * Try to find a slot that is preferably empty, or does not stack with a given item.
      * Only looks in the main inventory and hotbar (excluding offhand).
-     * Only slots that are empty or contain a different type of item are valid.
+     * <p>
+     * Slots are searched in the reverse order that the bedrock client uses for quick moving.
      *
-     * @return java id for the temporary slot, or -1 if no viable slot was found
+     * @param plan used to check the simulated inventory
+     * @param item the item to temporarily store
+     * @param emptyOnly if only empty slots should be considered
+     * @param slotBlacklist list of slots to exclude; the items contained in these slots will also be checked for stacking
+     * @return the temp slot, or -1 if no suitable slot was found
      */
-    //TODO: compatibility for simulated inventory (ClickPlan)
+    private static int findTempSlot(ClickPlan plan, GeyserItemStack item, boolean emptyOnly, int... slotBlacklist) {
-    private static int findTempSlot(Inventory inventory, GeyserItemStack item, boolean emptyOnly, int... slotBlacklist) {
+        IntSortedSet potentialSlots = new IntLinkedOpenHashSet(PLAYER_INVENTORY_SIZE);
-        int offset = inventory.getJavaId() == 0 ? 1 : 0; //offhand is not a viable temp slot
+        int hotbarOffset = plan.getInventory().getOffsetForHotbar(0);
-        HashSet<GeyserItemStack> itemBlacklist = new HashSet<>(slotBlacklist.length + 1);
-        itemBlacklist.add(item);
 
-        IntSet potentialSlots = new IntOpenHashSet(36);
+        // Add main inventory slots in reverse
-        for (int i = inventory.getSize() - (36 + offset); i < inventory.getSize() - offset; i++) {
+        for (int i = hotbarOffset - 1; i >= hotbarOffset - 27; i--) {
             potentialSlots.add(i);
         }
+
+        // Add hotbar slots in reverse
+        for (int i = hotbarOffset + 8; i >= hotbarOffset; i--) {
+            potentialSlots.add(i);
+        }
+
         for (int i : slotBlacklist) {
             potentialSlots.remove(i);
-            GeyserItemStack blacklistedItem = inventory.getItem(i);
+        }
-            if (!blacklistedItem.isEmpty()) {
+
-                itemBlacklist.add(blacklistedItem);
+        // Prefer empty slots
+        IntIterator it = potentialSlots.iterator();
+        while (it.hasNext()) {
+            int slot = it.nextInt();
+            if (plan.isEmpty(slot)) {
+                return slot;
             }
         }
 
-        for (int i : potentialSlots) {
+        if (emptyOnly) {
-            GeyserItemStack testItem = inventory.getItem(i);
+            return -1;
-            if ((emptyOnly && !testItem.isEmpty())) {
+        }
+
+        // No empty slots. Look for a slot that does not stack
+        it = potentialSlots.iterator();
+
+        outer:
+        while (it.hasNext()) {
+            int slot = it.nextInt();
+            if (plan.canStack(slot, item)) {
                 continue;
             }
 
-            boolean viable = true;
+            for (int blacklistedSlot : slotBlacklist) {
-            for (GeyserItemStack blacklistedItem : itemBlacklist) {
+                GeyserItemStack blacklistedItem = plan.getItem(blacklistedSlot);
-                if (InventoryUtils.canStack(testItem, blacklistedItem)) {
+                if (plan.canStack(slot, blacklistedItem)) {
-                    viable = false;
+                    continue outer;
-                    break;
                 }
             }
-            if (!viable) {
+
-                continue;
+            return slot;
         }
-            return i;
+
-        }
-        //could not find a viable temp slot
         return -1;
     }