Some documentation, include pending inventories in re-use check

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

@@ -80,7 +80,7 @@ public abstract class Inventory {
     protected final GeyserItemStack[] items;
 
     /**
-     * The location of the inventory block. Will either be a fake block above the player's head, or the actual block location
+     * The location of the inventory block. Will either be a fake block above the player's head, or the actual block location.
      */
     @Getter
     @Setter
@@ -94,18 +94,33 @@ public abstract class Inventory {
     @Setter
     protected long holderId = -1;
 
+    /**
+     * Whether this inventory is currently pending.
+     * It can be pending if this inventory was opened while another inventory was still open,
+     * or because opening this inventory takes more time (e.g. virtual inventories).
+     */
     @Getter
     @Setter
     private boolean pending = false;
 
+    /**
+     * Whether this inventory is currently shown to the Bedrock player.
+     */
     @Getter
     @Setter
     private boolean displayed = false;
 
+    /**
+     * Whether this inventory is currently being opened with a delay.
+     * Only applicable for virtual inventories.
+     */
     @Getter
     @Setter
     private boolean delayed = false;
 
+    /**
+     * The translator for this inventory. Stored here to avoid de-syncs of the inventory & translator used.
+     */
     @Getter
     private final InventoryTranslator translator;
 
@@ -133,8 +148,10 @@ public abstract class Inventory {
         this.bedrockId = javaId <= 100 ? javaId : (javaId % 100) + 1;
 
         // We occasionally need to re-open inventories with a delay in cases where
-        // Java wouldn't - e.g. for virtual chest menus that switch pages
-        if (session.getOpenInventory() != null && session.getOpenInventory().getBedrockId() == bedrockId) {
+        // Java wouldn't - e.g. for virtual chest menus that switch pages.
+        // And, well, we want to avoid reusing Bedrock inventory id's that are currently being used in a closing inventory;
+        // so to be safe we just deviate in that case as well.
+        if ((session.getOpenInventory() != null && session.getOpenInventory().getBedrockId() == bedrockId) || session.isClosingInventory()) {
             this.bedrockId += 1;
         }
     }

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

@@ -78,7 +78,7 @@ public abstract class AbstractBlockInventoryTranslator extends BaseInventoryTran
     }
 
     @Override
-    public boolean shouldDelayInventoryOpen(GeyserSession session, Inventory inventory) {
+    public boolean requiresOpeningDelay(GeyserSession session, Inventory inventory) {
         return inventory instanceof Container container && !container.isUsingRealBlock();
     }
 

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

@@ -137,7 +137,7 @@ public abstract class InventoryTranslator {
     public final int size;
 
     // Whether the inventory open should be delayed.
-    public boolean shouldDelayInventoryOpen(GeyserSession session, Inventory inventory) {
+    public boolean requiresOpeningDelay(GeyserSession session, Inventory inventory) {
         return false;
     }
 
@@ -145,10 +145,10 @@ public abstract class InventoryTranslator {
      * Whether a new inventory should be prepared - or if we can re-use the previous one.
      */
     public boolean canReuseInventory(GeyserSession session, @NonNull Inventory inventory, @NonNull Inventory previous) {
-        // Filter for mismatches that require a new inventory. Further, if we're closing a current inventory, we cannot reuse it.
+        // Filter for mismatches that require a new inventory.
         if (inventory.getContainerType() == null || previous.getContainerType() == null
             || !Objects.equals(inventory.getContainerType(), previous.getContainerType())
-            || inventory.getJavaId() != previous.getJavaId() || session.isClosingInventory()
+            || inventory.getJavaId() != previous.getJavaId()
         ) {
             return false;
         }

core/src/main/java/org/geysermc/geyser/translator/inventory/chest/ChestInventoryTranslator.java

@@ -53,7 +53,7 @@ public abstract class ChestInventoryTranslator extends BaseInventoryTranslator {
     }
 
     @Override
-    public boolean shouldDelayInventoryOpen(GeyserSession session, Inventory inventory) {
+    public boolean requiresOpeningDelay(GeyserSession session, Inventory inventory) {
         return inventory instanceof Container container && !container.isUsingRealBlock();
     }
 

core/src/main/java/org/geysermc/geyser/translator/protocol/bedrock/BedrockContainerCloseTranslator.java

@@ -28,7 +28,6 @@ package org.geysermc.geyser.translator.protocol.bedrock;
 import org.cloudburstmc.protocol.bedrock.packet.ContainerClosePacket;
 import org.cloudburstmc.protocol.bedrock.packet.NetworkStackLatencyPacket;
 import org.geysermc.geyser.GeyserImpl;
-import org.geysermc.geyser.inventory.Container;
 import org.geysermc.geyser.inventory.Inventory;
 import org.geysermc.geyser.inventory.MerchantContainer;
 import org.geysermc.geyser.session.GeyserSession;
@@ -61,18 +60,23 @@ public class BedrockContainerCloseTranslator extends PacketTranslator<ContainerC
 
             // If virtual inventories are opened too quickly, they can be occasionally rejected
             // We just try and queue a new one.
-            if (openInventory instanceof Container container && !(container instanceof MerchantContainer) && !container.isUsingRealBlock()) {
+            if (openInventory.getTranslator().requiresOpeningDelay(session, openInventory)) {
                 if (session.getContainerOpenAttempts() < 3) {
-                    container.setPending(true);
-                    container.setDelayed(true);
-                    session.setPendingInventoryId(container.getBedrockId());
+                    openInventory.setPending(true);
+                    openInventory.setDelayed(true);
+                    session.setPendingInventoryId(openInventory.getBedrockId());
 
+                    byte finalBedrockId = bedrockId;
                     session.scheduleInEventLoop(() -> {
-                        NetworkStackLatencyPacket latencyPacket = new NetworkStackLatencyPacket();
-                        latencyPacket.setFromServer(true);
-                        latencyPacket.setTimestamp(MAGIC_VIRTUAL_INVENTORY_HACK);
-                        session.sendUpstreamPacket(latencyPacket);
-                        GeyserImpl.getInstance().getLogger().debug(session, "Unable to open a virtual inventory, sending another latency packet!");
+                        if (InventoryUtils.shouldQueueRejectedInventory(session)) {
+                            NetworkStackLatencyPacket latencyPacket = new NetworkStackLatencyPacket();
+                            latencyPacket.setFromServer(true);
+                            latencyPacket.setTimestamp(MAGIC_VIRTUAL_INVENTORY_HACK);
+                            session.sendUpstreamPacket(latencyPacket);
+                            GeyserImpl.getInstance().getLogger().debug(session, "Unable to open a virtual inventory, sending another latency packet!");
+                        } else {
+                            closeCurrentOrOpenPending(session, finalBedrockId, session.getOpenInventory());
+                        }
                     }, 200, TimeUnit.MILLISECONDS);
                     return;
                 } else {
@@ -83,7 +87,10 @@ public class BedrockContainerCloseTranslator extends PacketTranslator<ContainerC
         }
 
         session.setContainerOpenAttempts(0);
+        closeCurrentOrOpenPending(session, bedrockId, openInventory);
+    }
 
+    private void closeCurrentOrOpenPending(GeyserSession session, byte bedrockId, Inventory openInventory) {
         if (openInventory != null) {
             if (bedrockId == openInventory.getBedrockId()) {
                 InventoryUtils.sendJavaContainerClose(session, openInventory);

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

@@ -77,7 +77,8 @@ public class JavaOpenScreenTranslator extends PacketTranslator<ClientboundOpenSc
 
         Inventory newInventory = newTranslator.createInventory(session, name, packet.getContainerId(), packet.getType(), session.getPlayerInventory());
         if (openInventory != null) {
-            // Attempt to re-use existing open inventories, if possible
+            // Attempt to re-use existing open inventories, if possible.
+            // Pending inventories are also considered, as a Java server can re-request the same inventory.
             if (newTranslator.canReuseInventory(session, newInventory, openInventory)) {
                 // Use the same Bedrock id. The java id is already confirmed to match
                 // in the reuse inventory check.

core/src/main/java/org/geysermc/geyser/util/InventoryUtils.java

@@ -137,6 +137,16 @@ public class InventoryUtils {
         openInventory(session, currentInventory);
     }
 
+    public static boolean shouldQueueRejectedInventory(GeyserSession session) {
+        Inventory currentInventory = session.getOpenInventory();
+        if (currentInventory == null || !currentInventory.isDelayed() || currentInventory.getBedrockId() != session.getPendingInventoryId()) {
+            GeyserImpl.getInstance().getLogger().debug(session, "Aborting NetworkStackLatency hack as the inventory has changed!");
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * Prepares and displays the current inventory. If necessary, it will queue the opening of virtual inventories.
      * @param inventory the inventory to display
@@ -144,7 +154,7 @@ public class InventoryUtils {
     public static void displayInventory(GeyserSession session, Inventory inventory) {
         InventoryTranslator translator = inventory.getTranslator();
         if (translator.prepareInventory(session, inventory)) {
-            if (translator.shouldDelayInventoryOpen(session, inventory)) {
+            if (translator.requiresOpeningDelay(session, inventory)) {
                 inventory.setPending(true);
                 inventory.setDelayed(true);
                 session.setPendingInventoryId(inventory.getBedrockId());