diff --git a/src/main/java/net/minestom/server/inventory/AbstractInventory.java b/src/main/java/net/minestom/server/inventory/AbstractInventory.java index 0719cc7f6..99cfc2fb0 100644 --- a/src/main/java/net/minestom/server/inventory/AbstractInventory.java +++ b/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 @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(); diff --git a/src/main/java/net/minestom/server/inventory/Inventory.java b/src/main/java/net/minestom/server/inventory/Inventory.java index 02325327f..932ced402 100644 --- a/src/main/java/net/minestom/server/inventory/Inventory.java +++ b/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) { diff --git a/src/main/java/net/minestom/server/inventory/PlayerInventory.java b/src/main/java/net/minestom/server/inventory/PlayerInventory.java index 07717b39d..7743f0b1a 100644 --- a/src/main/java/net/minestom/server/inventory/PlayerInventory.java +++ b/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); - player.getPlayerConnection().sendPacket(setSlotPacket); - } + final SetSlotPacket setSlotPacket = SetSlotPacket.createCursorPacket(cursorItem); + 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) { - this.player.syncEquipment(equipmentSlot); + + if (sendPacket) { + // Sync equipment + if (equipmentSlot != null) { + this.player.syncEquipment(equipmentSlot); + } + + // Refresh slot + sendSlotRefresh((short) convertToPacketSlot(slot), itemStack); } - // Refresh slot - sendSlotRefresh((short) convertToPacketSlot(slot), itemStack); } /** diff --git a/src/test/java/net/minestom/server/inventory/InventoryIntegrationTest.java b/src/test/java/net/minestom/server/inventory/InventoryIntegrationTest.java new file mode 100644 index 000000000..d204f3962 --- /dev/null +++ b/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(); + } + +} diff --git a/src/test/java/net/minestom/server/inventory/PlayerInventoryIntegrationTest.java b/src/test/java/net/minestom/server/inventory/PlayerInventoryIntegrationTest.java new file mode 100644 index 000000000..19e6cf6df --- /dev/null +++ b/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 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)); + }); + } + +}