Inventory Fixes (#602)

* Fix edge case when shift clicking an output slot

* Don't send window close packet if window is already closed

* Limit amount of window close packets sent to the client

Fixes hidden inventory bar bug

* Restrict user from unusable chest inventory slots

* Fix crafting table slot mappings

* Always send cursor update

connector/src/main/java/org/geysermc/connector/network/session/GeyserSession.java

@@ -166,6 +166,9 @@ public class GeyserSession implements CommandSender {
     @Setter
     private int craftSlot = 0;
 
+    @Setter
+    private long lastWindowCloseTime = 0;
+
     private MinecraftProtocol protocol;
 
     public GeyserSession(GeyserConnector connector, BedrockServerSession bedrockServerSession) {

connector/src/main/java/org/geysermc/connector/network/translators/bedrock/BedrockContainerCloseTranslator.java

@@ -38,17 +38,23 @@ public class BedrockContainerCloseTranslator extends PacketTranslator<ContainerC
 
     @Override
     public void translate(ContainerClosePacket packet, GeyserSession session) {
+        session.setLastWindowCloseTime(0);
         byte windowId = packet.getWindowId();
+        Inventory openInventory = session.getInventoryCache().getOpenInventory();
         if (windowId == -1) { //player inventory or crafting table
-            Inventory openInventory = session.getInventoryCache().getOpenInventory();
             if (openInventory != null) {
                 windowId = (byte) openInventory.getId();
             } else {
                 windowId = 0;
             }
         }
-        ClientCloseWindowPacket closeWindowPacket = new ClientCloseWindowPacket(windowId);
+
-        session.sendDownstreamPacket(closeWindowPacket);
+        if (windowId == 0 || (openInventory != null && openInventory.getId() == windowId)) {
-        InventoryUtils.closeInventory(session, windowId);
+            ClientCloseWindowPacket closeWindowPacket = new ClientCloseWindowPacket(windowId);
+            session.getDownstream().getSession().send(closeWindowPacket);
+            InventoryUtils.closeInventory(session, windowId);
+        } else if (openInventory != null && openInventory.getId() != windowId) {
+            InventoryUtils.openInventory(session, openInventory);
+        }
     }
 }

connector/src/main/java/org/geysermc/connector/network/translators/inventory/ChestInventoryTranslator.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2019-2020 GeyserMC. http://geysermc.org
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ * @author GeyserMC
+ * @link https://github.com/GeyserMC/Geyser
+ */
+
+package org.geysermc.connector.network.translators.inventory;
+
+import com.nukkitx.protocol.bedrock.data.InventoryActionData;
+import org.geysermc.connector.inventory.Inventory;
+import org.geysermc.connector.network.session.GeyserSession;
+import org.geysermc.connector.network.translators.inventory.updater.ChestInventoryUpdater;
+import org.geysermc.connector.network.translators.inventory.updater.InventoryUpdater;
+import org.geysermc.connector.utils.InventoryUtils;
+
+import java.util.List;
+
+public abstract class ChestInventoryTranslator extends BaseInventoryTranslator {
+    private final InventoryUpdater updater;
+
+    public ChestInventoryTranslator(int size, int paddedSize) {
+        super(size);
+        this.updater = new ChestInventoryUpdater(paddedSize);
+    }
+
+    @Override
+    public void updateInventory(GeyserSession session, Inventory inventory) {
+        updater.updateInventory(this, session, inventory);
+    }
+
+    @Override
+    public void updateSlot(GeyserSession session, Inventory inventory, int slot) {
+        updater.updateSlot(this, session, inventory, slot);
+    }
+
+    @Override
+    public void translateActions(GeyserSession session, Inventory inventory, List<InventoryActionData> actions) {
+        for (InventoryActionData action : actions) {
+            if (action.getSource().getContainerId() == inventory.getId()) {
+                if (action.getSlot() >= size) {
+                    updateInventory(session, inventory);
+                    InventoryUtils.updateCursor(session);
+                    return;
+                }
+            }
+        }
+
+        super.translateActions(session, inventory, actions);
+    }
+}

connector/src/main/java/org/geysermc/connector/network/translators/inventory/CraftingInventoryTranslator.java

@@ -92,7 +92,10 @@ public class CraftingInventoryTranslator extends BaseInventoryTranslator {
 
     @Override
     public int javaSlotToBedrock(int slot) {
-        return slot == 0 ? 50 : slot + 31;
+        if (slot < size) {
+            return slot == 0 ? 50 : slot + 31;
+        }
+        return super.javaSlotToBedrock(slot);
     }
 
     @Override

connector/src/main/java/org/geysermc/connector/network/translators/inventory/DoubleChestInventoryTranslator.java

@@ -39,15 +39,13 @@ import org.geysermc.connector.network.translators.world.block.BlockTranslator;
 import org.geysermc.connector.network.translators.inventory.updater.ChestInventoryUpdater;
 import org.geysermc.connector.network.translators.inventory.updater.InventoryUpdater;
 
-public class DoubleChestInventoryTranslator extends BaseInventoryTranslator {
+public class DoubleChestInventoryTranslator extends ChestInventoryTranslator {
     private final int blockId;
-    private final InventoryUpdater updater;
 
     public DoubleChestInventoryTranslator(int size) {
-        super(size);
+        super(size, 54);
         BlockState javaBlockState = BlockTranslator.getJavaBlockState("minecraft:chest[facing=north,type=single,waterlogged=false]");
         this.blockId = BlockTranslator.getBedrockBlockId(javaBlockState);
-        this.updater = new ChestInventoryUpdater(54);
     }
 
     @Override
@@ -128,14 +126,4 @@ public class DoubleChestInventoryTranslator extends BaseInventoryTranslator {
         blockPacket.setRuntimeId(BlockTranslator.getBedrockBlockId(realBlock));
         session.sendUpstreamPacket(blockPacket);
     }
-
-    @Override
-    public void updateInventory(GeyserSession session, Inventory inventory) {
-        updater.updateInventory(this, session, inventory);
-    }
-
-    @Override
-    public void updateSlot(GeyserSession session, Inventory inventory, int slot) {
-        updater.updateSlot(this, session, inventory, slot);
-    }
 }

connector/src/main/java/org/geysermc/connector/network/translators/inventory/SingleChestInventoryTranslator.java

@@ -25,11 +25,35 @@
 
 package org.geysermc.connector.network.translators.inventory;
 
+import com.github.steveice10.mc.protocol.data.game.world.block.BlockState;
 import com.nukkitx.protocol.bedrock.data.ContainerType;
-import org.geysermc.connector.network.translators.inventory.updater.ChestInventoryUpdater;
+import org.geysermc.connector.inventory.Inventory;
+import org.geysermc.connector.network.session.GeyserSession;
+import org.geysermc.connector.network.translators.inventory.holder.BlockInventoryHolder;
+import org.geysermc.connector.network.translators.inventory.holder.InventoryHolder;
+import org.geysermc.connector.network.translators.world.block.BlockTranslator;
+
+public class SingleChestInventoryTranslator extends ChestInventoryTranslator {
+    private final InventoryHolder holder;
 
-public class SingleChestInventoryTranslator extends BlockInventoryTranslator {
     public SingleChestInventoryTranslator(int size) {
-        super(size, "minecraft:chest[facing=north,type=single,waterlogged=false]", ContainerType.CONTAINER, new ChestInventoryUpdater(27));
+        super(size, 27);
+        BlockState javaBlockState = BlockTranslator.getJavaBlockState("minecraft:chest[facing=north,type=single,waterlogged=false]");
+        this.holder = new BlockInventoryHolder(BlockTranslator.getBedrockBlockId(javaBlockState), ContainerType.CONTAINER);
+    }
+
+    @Override
+    public void prepareInventory(GeyserSession session, Inventory inventory) {
+        holder.prepareInventory(this, session, inventory);
+    }
+
+    @Override
+    public void openInventory(GeyserSession session, Inventory inventory) {
+        holder.openInventory(this, session, inventory);
+    }
+
+    @Override
+    public void closeInventory(GeyserSession session, Inventory inventory) {
+        holder.closeInventory(this, session, inventory);
     }
 }

connector/src/main/java/org/geysermc/connector/network/translators/inventory/action/InventoryActionDataTranslator.java

@@ -187,11 +187,12 @@ public class InventoryActionDataTranslator {
                     } else if (translator.getSlotType(javaSlot) == SlotType.OUTPUT) {
                         plan.add(Click.LEFT, javaSlot);
                     } else {
-                        int cursorSlot = findTempSlot(inventory, session.getInventory().getCursor(), Collections.singletonList(javaSlot));
+                        int cursorSlot = findTempSlot(inventory, session.getInventory().getCursor(), Collections.singletonList(javaSlot), false);
                         if (cursorSlot != -1) {
                             plan.add(Click.LEFT, cursorSlot);
                         } else {
                             translator.updateInventory(session, inventory);
+                            InventoryUtils.updateCursor(session);
                             return;
                         }
                         plan.add(Click.LEFT, javaSlot);
@@ -245,11 +246,15 @@ public class InventoryActionDataTranslator {
 
             int cursorSlot = -1;
             if (session.getInventory().getCursor() != null) { //move cursor contents to a temporary slot
-                cursorSlot = findTempSlot(inventory, session.getInventory().getCursor(), Arrays.asList(fromSlot, toSlot));
+                cursorSlot = findTempSlot(inventory,
+                        session.getInventory().getCursor(),
+                        Arrays.asList(fromSlot, toSlot),
+                        translator.getSlotType(fromSlot) == SlotType.OUTPUT);
                 if (cursorSlot != -1) {
                     plan.add(Click.LEFT, cursorSlot);
                 } else {
                     translator.updateInventory(session, inventory);
+                    InventoryUtils.updateCursor(session);
                     return;
                 }
             }
@@ -298,7 +303,7 @@ public class InventoryActionDataTranslator {
         InventoryUtils.updateCursor(session);
     }
 
-    private static int findTempSlot(Inventory inventory, ItemStack item, List<Integer> slotBlacklist) {
+    private static int findTempSlot(Inventory inventory, ItemStack item, List<Integer> slotBlacklist, boolean emptyOnly) {
         /*try and find a slot that can temporarily store the given item
         only look in the main inventory and hotbar
         only slots that are empty or contain a different type of item are valid*/
@@ -314,6 +319,9 @@ public class InventoryActionDataTranslator {
             ItemStack testItem = inventory.getItem(i);
             boolean acceptable = true;
             if (testItem != null) {
+                if (emptyOnly) {
+                    continue;
+                }
                 for (ItemStack blacklistItem : itemBlacklist) {
                     if (InventoryUtils.canStack(testItem, blacklistItem)) {
                         acceptable = false;

connector/src/main/java/org/geysermc/connector/network/translators/java/window/JavaCloseWindowTranslator.java

@@ -26,7 +26,6 @@
 package org.geysermc.connector.network.translators.java.window;
 
 import com.github.steveice10.mc.protocol.packet.ingame.server.window.ServerCloseWindowPacket;
-import com.nukkitx.protocol.bedrock.packet.ContainerClosePacket;
 import org.geysermc.connector.network.session.GeyserSession;
 import org.geysermc.connector.network.translators.PacketTranslator;
 import org.geysermc.connector.network.translators.Translator;
@@ -37,9 +36,7 @@ public class JavaCloseWindowTranslator extends PacketTranslator<ServerCloseWindo
 
     @Override
     public void translate(ServerCloseWindowPacket packet, GeyserSession session) {
-        ContainerClosePacket closePacket = new ContainerClosePacket();
+        InventoryUtils.closeWindow(session, packet.getWindowId());
-        closePacket.setWindowId((byte)packet.getWindowId());
-        session.sendUpstreamPacket(closePacket);
         InventoryUtils.closeInventory(session, packet.getWindowId());
     }
 }

connector/src/main/java/org/geysermc/connector/network/translators/java/window/JavaOpenWindowTranslator.java

@@ -38,8 +38,6 @@ import org.geysermc.connector.network.translators.Translator;
 import org.geysermc.connector.network.translators.inventory.InventoryTranslator;
 import org.geysermc.connector.utils.InventoryUtils;
 
-import java.util.concurrent.TimeUnit;
-
 @Translator(packet = ServerOpenWindowPacket.class)
 public class JavaOpenWindowTranslator extends PacketTranslator<ServerOpenWindowPacket> {
 
@@ -80,8 +78,11 @@ public class JavaOpenWindowTranslator extends PacketTranslator<ServerOpenWindowP
         if (openInventory != null) {
             InventoryTranslator openTranslator = InventoryTranslator.INVENTORY_TRANSLATORS.get(openInventory.getWindowType());
             if (!openTranslator.getClass().equals(newTranslator.getClass())) {
+                InventoryUtils.closeWindow(session, openInventory.getId());
                 InventoryUtils.closeInventory(session, openInventory.getId());
-                GeyserConnector.getInstance().getGeneralThreadPool().schedule(() -> InventoryUtils.openInventory(session, newInventory), 500, TimeUnit.MILLISECONDS);
+                session.getInventoryCache().setOpenInventory(newInventory);
+                //The new window will be opened when the bedrock client sends the
+                //window close confirmation in BedrockContainerCloseTranslator
                 return;
             }
         }

connector/src/main/java/org/geysermc/connector/network/translators/java/window/JavaSetSlotTranslator.java

@@ -41,8 +41,6 @@ public class JavaSetSlotTranslator extends PacketTranslator<ServerSetSlotPacket>
     @Override
     public void translate(ServerSetSlotPacket packet, GeyserSession session) {
         if (packet.getWindowId() == 255 && packet.getSlot() == -1) { //cursor
-            if (Objects.equals(session.getInventory().getCursor(), packet.getItem()))
-                return;
             if (session.getCraftSlot() != 0)
                 return;
 

connector/src/main/java/org/geysermc/connector/utils/InventoryUtils.java

@@ -31,6 +31,7 @@ import com.nukkitx.nbt.CompoundTagBuilder;
 import com.nukkitx.nbt.tag.StringTag;
 import com.nukkitx.protocol.bedrock.data.ContainerId;
 import com.nukkitx.protocol.bedrock.data.ItemData;
+import com.nukkitx.protocol.bedrock.packet.ContainerClosePacket;
 import com.nukkitx.protocol.bedrock.packet.InventorySlotPacket;
 import org.geysermc.common.ChatColor;
 import org.geysermc.connector.GeyserConnector;
@@ -69,19 +70,34 @@ public class InventoryUtils {
     public static void closeInventory(GeyserSession session, int windowId) {
         if (windowId != 0) {
             Inventory inventory = session.getInventoryCache().getInventories().get(windowId);
-            if (inventory != null) {
+            Inventory openInventory = session.getInventoryCache().getOpenInventory();
+            session.getInventoryCache().uncacheInventory(windowId);
+            if (inventory != null && openInventory != null && inventory.getId() == openInventory.getId()) {
                 InventoryTranslator translator = InventoryTranslator.INVENTORY_TRANSLATORS.get(inventory.getWindowType());
                 translator.closeInventory(session, inventory);
-                session.getInventoryCache().uncacheInventory(windowId);
                 session.getInventoryCache().setOpenInventory(null);
+            } else {
+                return;
             }
         } else {
             Inventory inventory = session.getInventory();
             InventoryTranslator translator = InventoryTranslator.INVENTORY_TRANSLATORS.get(inventory.getWindowType());
             translator.updateInventory(session, inventory);
         }
+
         session.setCraftSlot(0);
         session.getInventory().setCursor(null);
+        updateCursor(session);
+    }
+
+    public static void closeWindow(GeyserSession session, int windowId) {
+        //Spamming close window packets can bug the client
+        if (System.currentTimeMillis() - session.getLastWindowCloseTime() > 500) {
+            ContainerClosePacket closePacket = new ContainerClosePacket();
+            closePacket.setWindowId((byte) windowId);
+            session.sendUpstreamPacket(closePacket);
+            session.setLastWindowCloseTime(System.currentTimeMillis());
+        }
     }
 
     public static void updateCursor(GeyserSession session) {