Some tweaks to AbstractInventory/PlayerInventory, as well as various Tests (#1049)

src/main/java/net/minestom/server/inventory/AbstractInventory.java

@@ -66,7 +66,7 @@ public sealed abstract class AbstractInventory implements InventoryClickHandler,
      * @param itemStack the item to insert (use air instead of null)
      * @throws IllegalArgumentException if the slot {@code slot} does not exist
      */
-    protected final void safeItemInsert(int slot, @NotNull ItemStack itemStack) {
+    protected final void safeItemInsert(int slot, @NotNull ItemStack itemStack, boolean sendPacket) {
         ItemStack previous;
         synchronized (this) {
             Check.argCondition(
@@ -75,7 +75,8 @@ public sealed abstract class AbstractInventory implements InventoryClickHandler,
                     slot
             );
             previous = itemStacks[slot];
-            UNSAFE_itemInsert(slot, itemStack);
+            if (itemStack.equals(previous)) return; // Avoid sending updates if the item has not changed
+            UNSAFE_itemInsert(slot, itemStack, sendPacket);
         }
         if (this instanceof PlayerInventory inv) {
             EventDispatcher.call(new PlayerInventoryItemChangeEvent(inv.player, slot, previous, itemStack));
@@ -84,7 +85,11 @@ public sealed abstract class AbstractInventory implements InventoryClickHandler,
         }
     }
 
-    protected abstract void UNSAFE_itemInsert(int slot, @NotNull ItemStack itemStack);
+    protected final void safeItemInsert(int slot, @NotNull ItemStack itemStack) {
+        safeItemInsert(slot, itemStack, true);
+    }
+
+    protected abstract void UNSAFE_itemInsert(int slot, @NotNull ItemStack itemStack, boolean sendPacket);
 
     public synchronized <T> @NotNull T processItemStack(@NotNull ItemStack itemStack,
                                                         @NotNull TransactionType type,
@@ -162,7 +167,7 @@ public sealed abstract class AbstractInventory implements InventoryClickHandler,
     public synchronized void clear() {
         // Clear the item array
         for (int i = 0; i < size; i++) {
-            safeItemInsert(i, ItemStack.AIR);
+            safeItemInsert(i, ItemStack.AIR, false);
         }
         // Send the cleared inventory to viewers
         update();

src/main/java/net/minestom/server/inventory/Inventory.java

@@ -109,10 +109,8 @@ public non-sealed class Inventory extends AbstractInventory implements Viewable
 
     @Override
     public synchronized void clear() {
+        cursorPlayersItem.clear();
         super.clear();
-        // Clear cursor
-        getViewers().forEach(player ->
-                setCursorItem(player, ItemStack.AIR));
     }
 
     /**
@@ -186,7 +184,7 @@ public non-sealed class Inventory extends AbstractInventory implements Viewable
      */
     public void setCursorItem(@NotNull Player player, @NotNull ItemStack cursorItem) {
         final ItemStack currentCursorItem = cursorPlayersItem.getOrDefault(player, ItemStack.AIR);
-        if (!currentCursorItem.isSimilar(cursorItem)) {
+        if (!currentCursorItem.equals(cursorItem)) {
             player.sendPacket(SetSlotPacket.createCursorPacket(cursorItem));
         }
         if (!cursorItem.isAir()) {
@@ -197,9 +195,11 @@ public non-sealed class Inventory extends AbstractInventory implements Viewable
     }
 
     @Override
-    protected void UNSAFE_itemInsert(int slot, @NotNull ItemStack itemStack) {
+    protected void UNSAFE_itemInsert(int slot, @NotNull ItemStack itemStack, boolean sendPacket) {
         itemStacks[slot] = itemStack;
-        sendPacketToViewers(new SetSlotPacket(getWindowId(), 0, (short) slot, itemStack));
+        if (sendPacket) {
+            sendPacketToViewers(new SetSlotPacket(getWindowId(), 0, (short) slot, itemStack));
+        }
     }
 
     private @NotNull WindowItemsPacket createNewWindowItemsPacket(Player player) {

src/main/java/net/minestom/server/inventory/PlayerInventory.java

@@ -32,9 +32,8 @@ public non-sealed class PlayerInventory extends AbstractInventory implements Equ
 
     @Override
     public synchronized void clear() {
+        cursorItem = ItemStack.AIR;
         super.clear();
-        // Reset cursor
-        setCursorItem(ItemStack.AIR);
         // Update equipments
         this.player.sendPacketToViewersAndSelf(player.getEquipmentsPacket());
     }
@@ -128,17 +127,16 @@ public non-sealed class PlayerInventory extends AbstractInventory implements Equ
      * @param cursorItem the new cursor item
      */
     public void setCursorItem(@NotNull ItemStack cursorItem) {
-        final boolean similar = this.cursorItem.isSimilar(cursorItem);
+        if (this.cursorItem.equals(cursorItem)) return;
+
         this.cursorItem = cursorItem;
 
-        if (!similar) {
+        final SetSlotPacket setSlotPacket = SetSlotPacket.createCursorPacket(cursorItem);
-            final SetSlotPacket setSlotPacket = SetSlotPacket.createCursorPacket(cursorItem);
+        player.getPlayerConnection().sendPacket(setSlotPacket);
-            player.getPlayerConnection().sendPacket(setSlotPacket);
-        }
     }
 
     @Override
-    protected void UNSAFE_itemInsert(int slot, @NotNull ItemStack itemStack) {
+    protected void UNSAFE_itemInsert(int slot, @NotNull ItemStack itemStack, boolean sendPacket) {
         EquipmentSlot equipmentSlot = null;
         if (slot == player.getHeldSlot()) {
             equipmentSlot = EquipmentSlot.MAIN_HAND;
@@ -159,12 +157,16 @@ public non-sealed class PlayerInventory extends AbstractInventory implements Equ
             itemStack = entityEquipEvent.getEquippedItem();
         }
         this.itemStacks[slot] = itemStack;
-        // Sync equipment
+
-        if (equipmentSlot != null) {
+        if (sendPacket) {
-            this.player.syncEquipment(equipmentSlot);
+            // Sync equipment
+            if (equipmentSlot != null) {
+                this.player.syncEquipment(equipmentSlot);
+            }
+
+            // Refresh slot
+            sendSlotRefresh((short) convertToPacketSlot(slot), itemStack);
         }
-        // Refresh slot
-        sendSlotRefresh((short) convertToPacketSlot(slot), itemStack);
     }
 
     /**

src/test/java/net/minestom/server/inventory/InventoryIntegrationTest.java

@@ -0,0 +1,115 @@
+package net.minestom.server.inventory;
+
+import net.kyori.adventure.text.Component;
+import net.minestom.server.api.Env;
+import net.minestom.server.api.EnvTest;
+import net.minestom.server.coordinate.Pos;
+import net.minestom.server.entity.EquipmentSlot;
+import net.minestom.server.item.ItemStack;
+import net.minestom.server.item.Material;
+import net.minestom.server.network.packet.server.play.EntityEquipmentPacket;
+import net.minestom.server.network.packet.server.play.SetSlotPacket;
+import net.minestom.server.network.packet.server.play.WindowItemsPacket;
+import org.junit.jupiter.api.Test;
+
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+@EnvTest
+public class InventoryIntegrationTest {
+
+    private static final ItemStack MAGIC_STACK = ItemStack.of(Material.DIAMOND, 3);
+
+    @Test
+    public void setSlotDuplicateTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connection = env.createConnection();
+        var player = connection.connect(instance, new Pos(0, 42, 0)).join();
+        assertEquals(instance, player.getInstance());
+
+        Inventory inventory = new Inventory(InventoryType.CHEST_6_ROW, Component.empty());
+        player.openInventory(inventory);
+        assertEquals(inventory, player.getOpenInventory());
+
+        var packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        inventory.setItemStack(3, MAGIC_STACK);
+        packetTracker.assertSingle(slot -> assertEquals(MAGIC_STACK, slot.itemStack())); // Setting a slot should send a packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        inventory.setItemStack(3, MAGIC_STACK);
+        packetTracker.assertEmpty(); // Setting the same slot to the same ItemStack should not send another packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        inventory.setItemStack(3, ItemStack.AIR);
+        packetTracker.assertSingle(slot -> assertEquals(ItemStack.AIR, slot.itemStack())); // Setting a slot should send a packet
+    }
+
+    @Test
+    public void setCursorItemDuplicateTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connection = env.createConnection();
+        var player = connection.connect(instance, new Pos(0, 42, 0)).join();
+        assertEquals(instance, player.getInstance());
+
+        Inventory inventory = new Inventory(InventoryType.CHEST_6_ROW, Component.empty());
+        player.openInventory(inventory);
+        assertEquals(inventory, player.getOpenInventory());
+
+        var packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        inventory.setCursorItem(player, MAGIC_STACK);
+        packetTracker.assertSingle(slot -> assertEquals(MAGIC_STACK, slot.itemStack())); // Setting a slot should send a packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        inventory.setCursorItem(player, MAGIC_STACK);
+        packetTracker.assertEmpty(); // Setting the same slot to the same ItemStack should not send another packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        inventory.setCursorItem(player, ItemStack.AIR);
+        packetTracker.assertSingle(slot -> assertEquals(ItemStack.AIR, slot.itemStack())); // Setting a slot should send a packet
+    }
+
+    @Test
+    public void clearInventoryTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connection = env.createConnection();
+        var player = connection.connect(instance, new Pos(0, 42, 0)).join();
+        assertEquals(instance, player.getInstance());
+        
+        Inventory inventory = new Inventory(InventoryType.CHEST_6_ROW, Component.empty());
+        player.openInventory(inventory);
+        assertEquals(inventory, player.getOpenInventory());
+
+        var setSlotTracker = connection.trackIncoming(SetSlotPacket.class);
+
+        inventory.setItemStack(1, MAGIC_STACK);
+        inventory.setItemStack(3, MAGIC_STACK);
+        inventory.setItemStack(19, MAGIC_STACK);
+        inventory.setItemStack(40, MAGIC_STACK);
+        inventory.setCursorItem(player, MAGIC_STACK);
+
+        setSlotTracker.assertCount(5);
+
+        setSlotTracker = connection.trackIncoming(SetSlotPacket.class);
+        var updateWindowTracker = connection.trackIncoming(WindowItemsPacket.class);
+        var equipmentTracker = connection.trackIncoming(EntityEquipmentPacket.class);
+
+        // Perform the clear operation we are testing
+        inventory.clear();
+
+        // Make sure not individual SetSlotPackets get sent
+        setSlotTracker.assertEmpty();
+
+        // Make sure WindowItemsPacket is empty
+        updateWindowTracker.assertSingle(windowItemsPacket -> {
+            assertEquals(ItemStack.AIR, windowItemsPacket.carriedItem());
+            for (ItemStack item : windowItemsPacket.items()) {
+                assertEquals(ItemStack.AIR, item);
+            }
+        });
+
+        // Make sure EntityEquipmentPacket isn't sent (this is an Inventory, not a PlayerInventory)
+        equipmentTracker.assertEmpty();
+    }
+
+}

src/test/java/net/minestom/server/inventory/PlayerInventoryIntegrationTest.java

@@ -0,0 +1,174 @@
+package net.minestom.server.inventory;
+
+import net.minestom.server.api.Env;
+import net.minestom.server.api.EnvTest;
+import net.minestom.server.coordinate.Pos;
+import net.minestom.server.entity.EquipmentSlot;
+import net.minestom.server.item.ItemStack;
+import net.minestom.server.item.Material;
+import net.minestom.server.network.packet.server.play.EntityEquipmentPacket;
+import net.minestom.server.network.packet.server.play.SetSlotPacket;
+import net.minestom.server.network.packet.server.play.WindowItemsPacket;
+import org.junit.jupiter.api.Test;
+
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+@EnvTest
+public class PlayerInventoryIntegrationTest {
+
+    private static final ItemStack MAGIC_STACK = ItemStack.of(Material.DIAMOND, 3);
+
+    @Test
+    public void setSlotDuplicateTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connection = env.createConnection();
+        var player = connection.connect(instance, new Pos(0, 42, 0)).join();
+        assertEquals(instance, player.getInstance());
+
+        var packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        player.getInventory().setItemStack(3, MAGIC_STACK);
+        packetTracker.assertSingle(slot -> assertEquals(MAGIC_STACK, slot.itemStack())); // Setting a slot should send a packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        player.getInventory().setItemStack(3, MAGIC_STACK);
+        packetTracker.assertEmpty(); // Setting the same slot to the same ItemStack should not send another packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        player.getInventory().setItemStack(3, ItemStack.AIR);
+        packetTracker.assertSingle(slot -> assertEquals(ItemStack.AIR, slot.itemStack())); // Setting a slot should send a packet
+    }
+
+    @Test
+    public void setCursorItemDuplicateTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connection = env.createConnection();
+        var player = connection.connect(instance, new Pos(0, 42, 0)).join();
+        assertEquals(instance, player.getInstance());
+
+        var packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        player.getInventory().setCursorItem(MAGIC_STACK);
+        packetTracker.assertSingle(slot -> assertEquals(MAGIC_STACK, slot.itemStack())); // Setting a slot should send a packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        player.getInventory().setCursorItem(MAGIC_STACK);
+        packetTracker.assertEmpty(); // Setting the same slot to the same ItemStack should not send another packet
+
+        packetTracker = connection.trackIncoming(SetSlotPacket.class);
+        player.getInventory().setCursorItem(ItemStack.AIR);
+        packetTracker.assertSingle(slot -> assertEquals(ItemStack.AIR, slot.itemStack())); // Setting a slot should send a packet
+    }
+
+    @Test
+    public void clearInventoryTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connection = env.createConnection();
+        var player = connection.connect(instance, new Pos(0, 42, 0)).join();
+        assertEquals(instance, player.getInstance());
+
+        var setSlotTracker = connection.trackIncoming(SetSlotPacket.class);
+
+        player.getInventory().setItemStack(1, MAGIC_STACK);
+        player.getInventory().setItemStack(3, MAGIC_STACK);
+        player.getInventory().setItemStack(19, MAGIC_STACK);
+        player.getInventory().setItemStack(40, MAGIC_STACK);
+        player.getInventory().setCursorItem(MAGIC_STACK);
+
+        setSlotTracker.assertCount(5);
+
+        setSlotTracker = connection.trackIncoming(SetSlotPacket.class);
+        var updateWindowTracker = connection.trackIncoming(WindowItemsPacket.class);
+        var equipmentTracker = connection.trackIncoming(EntityEquipmentPacket.class);
+
+        // Perform the clear operation we are testing
+        player.getInventory().clear();
+
+        // Make sure not individual SetSlotPackets get sent
+        setSlotTracker.assertEmpty();
+
+        // Make sure WindowItemsPacket is empty
+        updateWindowTracker.assertSingle(windowItemsPacket -> {
+            assertEquals(ItemStack.AIR, windowItemsPacket.carriedItem());
+            for (ItemStack item : windowItemsPacket.items()) {
+                assertEquals(ItemStack.AIR, item);
+            }
+        });
+
+        // Make sure EntityEquipmentPacket is empty
+        equipmentTracker.assertSingle(entityEquipmentPacket -> {
+            assertEquals(6, entityEquipmentPacket.equipments().size());
+            for (Map.Entry<EquipmentSlot, ItemStack> entry : entityEquipmentPacket.equipments().entrySet()) {
+                assertEquals(ItemStack.AIR, entry.getValue());
+            }
+        });
+    }
+
+    @Test
+    public void equipmentViewTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connectionArmored = env.createConnection();
+        var playerArmored = connectionArmored.connect(instance, new Pos(0, 42, 0)).join();
+        var connectionViewer = env.createConnection();
+        var playerViewer = connectionViewer.connect(instance, new Pos(0, 42, 0)).join();
+
+        assertEquals(instance, playerArmored.getInstance());
+        assertEquals(instance, playerViewer.getInstance());
+
+        var equipmentTracker = connectionViewer.trackIncoming(EntityEquipmentPacket.class);
+
+        // Setting to an item should send EntityEquipmentPacket to viewer
+        playerArmored.getInventory().setEquipment(EquipmentSlot.HELMET, MAGIC_STACK);
+        equipmentTracker.assertSingle(entityEquipmentPacket -> {
+            assertEquals(MAGIC_STACK, entityEquipmentPacket.equipments().get(EquipmentSlot.HELMET));
+        });
+
+        // Setting to the same item shouldn't send packet
+        equipmentTracker = connectionViewer.trackIncoming(EntityEquipmentPacket.class);
+        playerArmored.getInventory().setEquipment(EquipmentSlot.HELMET, MAGIC_STACK);
+        equipmentTracker.assertEmpty();
+
+        // Setting to air should send packet
+        equipmentTracker = connectionViewer.trackIncoming(EntityEquipmentPacket.class);
+        playerArmored.getInventory().setEquipment(EquipmentSlot.HELMET, ItemStack.AIR);
+        equipmentTracker.assertSingle(entityEquipmentPacket -> {
+            assertEquals(ItemStack.AIR, entityEquipmentPacket.equipments().get(EquipmentSlot.HELMET));
+        });
+    }
+
+    @Test
+    public void heldItemViewTest(Env env) {
+        var instance = env.createFlatInstance();
+        var connectionHolder = env.createConnection();
+        var playerHolder = connectionHolder.connect(instance, new Pos(0, 42, 0)).join();
+        var connectionViewer = env.createConnection();
+        var playerViewer = connectionViewer.connect(instance, new Pos(0, 42, 0)).join();
+
+        assertEquals(instance, playerHolder.getInstance());
+        assertEquals(instance, playerViewer.getInstance());
+
+        playerHolder.setHeldItemSlot((byte) 0);
+
+        // Setting held item
+        var equipmentTracker = connectionViewer.trackIncoming(EntityEquipmentPacket.class);
+        playerHolder.setItemInMainHand(MAGIC_STACK);
+        equipmentTracker.assertSingle(entityEquipmentPacket -> {
+            assertEquals(MAGIC_STACK, entityEquipmentPacket.equipments().get(EquipmentSlot.MAIN_HAND));
+        });
+
+        // Changing held slot to an empty slot should update MAIN_HAND to empty item
+        equipmentTracker = connectionViewer.trackIncoming(EntityEquipmentPacket.class);
+        playerHolder.setHeldItemSlot((byte) 3);
+        equipmentTracker.assertSingle(entityEquipmentPacket -> {
+            assertEquals(ItemStack.AIR, entityEquipmentPacket.equipments().get(EquipmentSlot.MAIN_HAND));
+        });
+
+        // Changing held slot to the original slot should update MAIN_HAND to original item
+        equipmentTracker = connectionViewer.trackIncoming(EntityEquipmentPacket.class);
+        playerHolder.setHeldItemSlot((byte) 0);
+        equipmentTracker.assertSingle(entityEquipmentPacket -> {
+            assertEquals(MAGIC_STACK, entityEquipmentPacket.equipments().get(EquipmentSlot.MAIN_HAND));
+        });
+    }
+
+}