Trust client's inventory prediction when possible

src/main/java/net/minestom/server/entity/Player.java

@@ -282,7 +282,6 @@ public class Player extends LivingEntity implements CommandSender, Localizable,
         this.playerConnection.sendPacket(getPropertiesPacket()); // Send default properties
         refreshHealth(); // Heal and send health packet
         refreshAbilities(); // Send abilities packet
-        getInventory().update();
     }
 
     /**

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

@@ -267,23 +267,19 @@ public class Inventory extends AbstractInventory implements Viewable {
         final boolean isInWindow = isClickInWindow(slot);
         final int clickSlot = isInWindow ? slot : PlayerInventoryUtils.convertSlot(slot, offset);
         final ItemStack clicked = isInWindow ? getItemStack(slot) : playerInventory.getItemStack(clickSlot);
-
         final InventoryClickResult clickResult = clickProcessor.leftClick(player, this, slot, clicked, cursor);
-        if (clickResult.doRefresh()) {
+        if (clickResult.isCancel()) {
-            updateFromClick(clickResult, player);
+            updateAll(player);
+            return false;
         }
-
         if (isInWindow) {
             setItemStack(slot, clickResult.getClicked());
         } else {
             playerInventory.setItemStack(clickSlot, clickResult.getClicked());
         }
         refreshPlayerCursorItem(player, clickResult.getCursor());
-
+        callClickEvent(player, isInWindow ? this : null, slot, ClickType.LEFT_CLICK, clicked, cursor);
-        if (!clickResult.isCancel())
+        return true;
-            callClickEvent(player, isInWindow ? this : null, slot, ClickType.LEFT_CLICK, clicked, cursor);
-
-        return !clickResult.isCancel();
     }
 
     @Override
@@ -293,23 +289,19 @@ public class Inventory extends AbstractInventory implements Viewable {
         final boolean isInWindow = isClickInWindow(slot);
         final int clickSlot = isInWindow ? slot : PlayerInventoryUtils.convertSlot(slot, offset);
         final ItemStack clicked = isInWindow ? getItemStack(slot) : playerInventory.getItemStack(clickSlot);
-
         final InventoryClickResult clickResult = clickProcessor.rightClick(player, this, slot, clicked, cursor);
-        if (clickResult.doRefresh()) {
+        if (clickResult.isCancel()) {
-            updateFromClick(clickResult, player);
+            updateAll(player);
+            return false;
         }
-
         if (isInWindow) {
             setItemStack(slot, clickResult.getClicked());
         } else {
             playerInventory.setItemStack(clickSlot, clickResult.getClicked());
         }
         refreshPlayerCursorItem(player, clickResult.getCursor());
-
+        callClickEvent(player, isInWindow ? this : null, slot, ClickType.RIGHT_CLICK, clicked, cursor);
-        if (!clickResult.isCancel())
+        return true;
-            callClickEvent(player, isInWindow ? this : null, slot, ClickType.RIGHT_CLICK, clicked, cursor);
-
-        return !clickResult.isCancel();
     }
 
     @Override
@@ -319,28 +311,23 @@ public class Inventory extends AbstractInventory implements Viewable {
         final int clickSlot = isInWindow ? slot : PlayerInventoryUtils.convertSlot(slot, offset);
         final ItemStack clicked = isInWindow ? getItemStack(slot) : playerInventory.getItemStack(clickSlot);
         final ItemStack cursor = getCursorItem(player); // Isn't used in the algorithm
-
         final InventoryClickResult clickResult = clickProcessor.shiftClick(
                 isInWindow ? this : playerInventory,
                 isInWindow ? playerInventory : this,
                 0, isInWindow ? playerInventory.getInnerSize() : getInnerSize(), 1,
                 player, slot, clicked, cursor);
-        if (clickResult == null)
+        if (clickResult.isCancel()) {
+            updateAll(player);
             return false;
-
+        }
         if (isInWindow) {
             setItemStack(slot, clickResult.getClicked());
         } else {
             playerInventory.setItemStack(clickSlot, clickResult.getClicked());
         }
-
+        updateAll(player); // FIXME: currently not properly client-predicted
-        if (clickResult.doRefresh()) {
-            update(player);
-        }
-
         refreshPlayerCursorItem(player, clickResult.getCursor());
-
+        return true;
-        return !clickResult.isCancel();
     }
 
     @Override
@@ -350,31 +337,25 @@ public class Inventory extends AbstractInventory implements Viewable {
         final int clickSlot = isInWindow ? slot : PlayerInventoryUtils.convertSlot(slot, offset);
         final ItemStack clicked = isInWindow ? getItemStack(slot) : playerInventory.getItemStack(clickSlot);
         final ItemStack heldItem = playerInventory.getItemStack(key);
-
         final InventoryClickResult clickResult = clickProcessor.changeHeld(player, this, slot, key, clicked, heldItem);
-        if (clickResult.doRefresh()) {
+        if (clickResult.isCancel()) {
-            updateFromClick(clickResult, player);
+            updateAll(player);
+            return false;
         }
-
         if (isInWindow) {
             setItemStack(slot, clickResult.getClicked());
         } else {
             playerInventory.setItemStack(clickSlot, clickResult.getClicked());
         }
         playerInventory.setItemStack(key, clickResult.getCursor());
-
+        callClickEvent(player, isInWindow ? this : null, slot, ClickType.CHANGE_HELD, clicked, getCursorItem(player));
-        if (!clickResult.isCancel())
+        return true;
-            callClickEvent(player, isInWindow ? this : null, slot, ClickType.CHANGE_HELD, clicked, getCursorItem(player));
-
-        // Weird synchronization issue when omitted
-        updateFromClick(clickResult, player);
-
-        return !clickResult.isCancel();
     }
 
     @Override
     public boolean middleClick(@NotNull Player player, int slot) {
         // TODO
+        update(player);
         return false;
     }
 
@@ -387,14 +368,12 @@ public class Inventory extends AbstractInventory implements Viewable {
         final ItemStack clicked = outsideDrop ?
                 ItemStack.AIR : (isInWindow ? getItemStack(slot) : playerInventory.getItemStack(clickSlot));
         final ItemStack cursor = getCursorItem(player);
-
         final InventoryClickResult clickResult = clickProcessor.drop(player, this,
                 all, slot, button, clicked, cursor);
-
+        if (clickResult.isCancel()) {
-        if (clickResult.doRefresh()) {
+            updateAll(player);
-            updateFromClick(clickResult, player);
+            return false;
         }
-
         final ItemStack resultClicked = clickResult.getClicked();
         if (!outsideDrop && resultClicked != null) {
             if (isInWindow) {
@@ -403,10 +382,8 @@ public class Inventory extends AbstractInventory implements Viewable {
                 playerInventory.setItemStack(clickSlot, resultClicked);
             }
         }
-
         refreshPlayerCursorItem(player, clickResult.getCursor());
-
+        return true;
-        return !clickResult.isCancel();
     }
 
     @Override
@@ -418,7 +395,6 @@ public class Inventory extends AbstractInventory implements Viewable {
                 (isInWindow ? getItemStack(slot) : playerInventory.getItemStack(clickSlot)) :
                 ItemStack.AIR;
         final ItemStack cursor = getCursorItem(player);
-
         final InventoryClickResult clickResult = clickProcessor.dragging(player, this,
                 slot, button,
                 clicked, cursor,
@@ -433,18 +409,12 @@ public class Inventory extends AbstractInventory implements Viewable {
                         playerInventory.setItemStack(PlayerInventoryUtils.convertSlot(s, offset), item);
                     }
                 });
-
+        if (clickResult == null || clickResult.isCancel()) {
-        if (clickResult == null) {
+            updateAll(player);
             return false;
         }
-
-        if (clickResult.doRefresh()) {
-            updateFromClick(clickResult, player);
-        }
-
         refreshPlayerCursorItem(player, clickResult.getCursor());
-
+        return true;
-        return !clickResult.isCancel();
     }
 
     @Override
@@ -452,32 +422,18 @@ public class Inventory extends AbstractInventory implements Viewable {
         final PlayerInventory playerInventory = player.getInventory();
         final ItemStack cursor = getCursorItem(player);
         final boolean isInWindow = isClickInWindow(slot);
-
         final InventoryClickResult clickResult = clickProcessor.doubleClick(isInWindow ? this : playerInventory,
                 this, player, slot, cursor);
-
+        if (clickResult.isCancel()) {
-        if (clickResult == null)
+            updateAll(player);
             return false;
-
+        }
-        if (clickResult.doRefresh())
-            updateFromClick(clickResult, player);
-
         refreshPlayerCursorItem(player, clickResult.getCursor());
-
+        return true;
-        return !clickResult.isCancel();
     }
 
-    /**
+    private void updateAll(Player player) {
-     * Used to update the inventory for a specific player in order to fix his cancelled actions
+        player.getInventory().update();
-     *
+        update(player);
-     * @param clickResult the action result
-     * @param player      the player who did the action
-     */
-    private void updateFromClick(InventoryClickResult clickResult, Player player) {
-        if (clickResult.isPlayerInventory()) {
-            player.getInventory().update();
-        } else {
-            update(player);
-        }
     }
 }

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

@@ -11,6 +11,7 @@ import net.minestom.server.item.ItemStack;
 import net.minestom.server.network.packet.server.play.SetSlotPacket;
 import net.minestom.server.network.packet.server.play.WindowItemsPacket;
 import net.minestom.server.utils.MathUtils;
+import net.minestom.server.utils.debug.DebugUtils;
 import net.minestom.server.utils.validate.Check;
 import org.jetbrains.annotations.NotNull;
 
@@ -20,7 +21,6 @@ import static net.minestom.server.utils.inventory.PlayerInventoryUtils.*;
  * Represents the inventory of a {@link Player}, retrieved with {@link Player#getInventory()}.
  */
 public class PlayerInventory extends AbstractInventory implements EquipmentHandler {
-
     public static final int INVENTORY_SIZE = 46;
     public static final int INNER_INVENTORY_SIZE = 36;
 
@@ -129,7 +129,8 @@ public class PlayerInventory extends AbstractInventory implements EquipmentHandl
      */
     @Override
     public void update() {
-        player.getPlayerConnection().sendPacket(createWindowItemsPacket());
+        DebugUtils.printStackTrace();
+        this.player.getPlayerConnection().sendPacket(createWindowItemsPacket());
     }
 
     /**
@@ -172,7 +173,6 @@ public class PlayerInventory extends AbstractInventory implements EquipmentHandl
         Check.notNull(itemStack, "The ItemStack cannot be null, you can set air instead");
 
         EquipmentSlot equipmentSlot = null;
-
         if (slot == player.getHeldSlot()) {
             equipmentSlot = EquipmentSlot.MAIN_HAND;
         } else if (slot == OFFHAND_SLOT) {
@@ -186,25 +186,18 @@ public class PlayerInventory extends AbstractInventory implements EquipmentHandl
         } else if (slot == BOOTS_SLOT) {
             equipmentSlot = EquipmentSlot.BOOTS;
         }
-
         if (equipmentSlot != null) {
             EntityEquipEvent entityEquipEvent = new EntityEquipEvent(player, itemStack, equipmentSlot);
-
             EventDispatcher.call(entityEquipEvent);
             itemStack = entityEquipEvent.getEquippedItem();
         }
-
         this.itemStacks[slot] = itemStack;
-
         // Sync equipment
         if (equipmentSlot != null) {
-            player.syncEquipment(equipmentSlot);
+            this.player.syncEquipment(equipmentSlot);
         }
-
         // Refresh slot
-        update();
+        sendSlotRefresh((short) convertToPacketSlot(slot), itemStack);
-        // FIXME: replace update() to refreshSlot, currently not possible because our inventory click handling is not exactly the same as what the client expects
-        //refreshSlot((short) slot);
     }
 
     /**
@@ -261,18 +254,15 @@ public class PlayerInventory extends AbstractInventory implements EquipmentHandl
         final int convertedSlot = convertPlayerInventorySlot(slot, OFFSET);
         final ItemStack cursor = getCursorItem();
         final ItemStack clicked = getItemStack(convertedSlot);
-
         final InventoryClickResult clickResult = clickProcessor.leftClick(player, this, convertedSlot, clicked, cursor);
-        if (clickResult.doRefresh())
+        if (clickResult.isCancel()) {
-            sendSlotRefresh((short) slot, clicked);
+            update();
-
+            return false;
+        }
         setItemStack(convertedSlot, clickResult.getClicked());
         setCursorItem(clickResult.getCursor());
-
+        callClickEvent(player, null, convertedSlot, ClickType.LEFT_CLICK, clicked, cursor);
-        if (!clickResult.isCancel())
+        return true;
-            callClickEvent(player, null, convertedSlot, ClickType.LEFT_CLICK, clicked, cursor);
-
-        return !clickResult.isCancel();
     }
 
     @Override
@@ -280,23 +270,21 @@ public class PlayerInventory extends AbstractInventory implements EquipmentHandl
         final int convertedSlot = convertPlayerInventorySlot(slot, OFFSET);
         final ItemStack cursor = getCursorItem();
         final ItemStack clicked = getItemStack(convertedSlot);
-
         final InventoryClickResult clickResult = clickProcessor.rightClick(player, this, convertedSlot, clicked, cursor);
-        if (clickResult.doRefresh())
+        if (clickResult.isCancel()) {
-            sendSlotRefresh((short) slot, clicked);
+            update();
-
+            return false;
+        }
         setItemStack(convertedSlot, clickResult.getClicked());
         setCursorItem(clickResult.getCursor());
-
+        callClickEvent(player, null, convertedSlot, ClickType.RIGHT_CLICK, clicked, cursor);
-        if (!clickResult.isCancel())
+        return true;
-            callClickEvent(player, null, convertedSlot, ClickType.RIGHT_CLICK, clicked, cursor);
-
-        return !clickResult.isCancel();
     }
 
     @Override
     public boolean middleClick(@NotNull Player player, int slot) {
         // TODO
+        update();
         return false;
     }
 
@@ -305,18 +293,18 @@ public class PlayerInventory extends AbstractInventory implements EquipmentHandl
         final ItemStack cursor = getCursorItem();
         final boolean outsideDrop = slot == -999;
         final ItemStack clicked = outsideDrop ? ItemStack.AIR : getItemStack(slot, OFFSET);
-
         final InventoryClickResult clickResult = clickProcessor.drop(player, this,
                 all, slot, button, clicked, cursor);
-        if (clickResult.doRefresh())
+        if (clickResult.isCancel()) {
-            sendSlotRefresh((short) slot, clicked);
+            update();
-
+            return false;
+        }
         final ItemStack resultClicked = clickResult.getClicked();
-        if (resultClicked != null && !outsideDrop)
+        if (resultClicked != null && !outsideDrop) {
             setItemStack(slot, OFFSET, resultClicked);
+        }
         setCursorItem(clickResult.getCursor());
-
+        return true;
-        return !clickResult.isCancel();
     }
 
     @Override
@@ -330,70 +318,58 @@ public class PlayerInventory extends AbstractInventory implements EquipmentHandl
                 this, this,
                 start, end, 1,
                 player, slot, clicked, cursor);
-        if (clickResult == null)
+        if (clickResult.isCancel()) {
+            update();
             return false;
-
+        }
         setItemStack(slot, OFFSET, clickResult.getClicked());
         setCursorItem(clickResult.getCursor());
-
+        update(); // FIXME: currently not properly client-predicted
-        return !clickResult.isCancel();
+        return true;
     }
 
     @Override
     public boolean changeHeld(@NotNull Player player, int slot, int key) {
-        if (!getCursorItem().isAir())
+        final ItemStack cursorItem = getCursorItem();
-            return false;
+        if (!cursorItem.isAir()) return false;
-
         final ItemStack heldItem = getItemStack(key);
         final ItemStack clicked = getItemStack(slot, OFFSET);
-
         final InventoryClickResult clickResult = clickProcessor.changeHeld(player, this, slot, key, clicked, heldItem);
-        if (clickResult.doRefresh()) {
+        if (clickResult.isCancel()) {
-            sendSlotRefresh((short) slot, clicked);
+            update();
+            return false;
         }
-
         setItemStack(slot, OFFSET, clickResult.getClicked());
         setItemStack(key, clickResult.getCursor());
-
+        callClickEvent(player, null, slot, ClickType.CHANGE_HELD, clicked, cursorItem);
-        if (!clickResult.isCancel())
+        return true;
-            callClickEvent(player, null, slot, ClickType.CHANGE_HELD, clicked, getCursorItem());
-
-        // Weird synchronization issue when omitted
-        update();
-
-        return !clickResult.isCancel();
     }
 
     @Override
     public boolean dragging(@NotNull Player player, int slot, int button) {
         final ItemStack cursor = getCursorItem();
         final ItemStack clicked = slot != -999 ? getItemStack(slot, OFFSET) : ItemStack.AIR;
-
         final InventoryClickResult clickResult = clickProcessor.dragging(player, this,
                 slot, button,
                 clicked, cursor, s -> getItemStack(s, OFFSET),
                 (s, item) -> setItemStack(s, OFFSET, item));
-        if (clickResult == null) {
+        if (clickResult == null || clickResult.isCancel()) {
+            update();
             return false;
         }
-
-        if (clickResult.doRefresh())
-            update();
-
         setCursorItem(clickResult.getCursor());
-
+        return true;
-        return !clickResult.isCancel();
     }
 
     @Override
     public boolean doubleClick(@NotNull Player player, int slot) {
         final ItemStack cursor = getCursorItem();
         final InventoryClickResult clickResult = clickProcessor.doubleClick(this, this, player, slot, cursor);
-        if (clickResult == null)
+        if (clickResult.isCancel()) {
-            return false;
-        if (clickResult.doRefresh())
             update();
+            return false;
+        }
         setCursorItem(clickResult.getCursor());
-        return !clickResult.isCancel();
+        return true;
     }
 }

src/main/java/net/minestom/server/inventory/click/InventoryClickProcessor.java

@@ -118,19 +118,19 @@ public final class InventoryClickProcessor {
         return clickResult;
     }
 
-    public @Nullable InventoryClickResult shiftClick(@NotNull AbstractInventory inventory, @NotNull AbstractInventory targetInventory,
+    public @NotNull InventoryClickResult shiftClick(@NotNull AbstractInventory inventory, @NotNull AbstractInventory targetInventory,
-                                                     int start, int end, int step,
+                                                    int start, int end, int step,
-                                                     @NotNull Player player, int slot,
+                                                    @NotNull Player player, int slot,
-                                                     @NotNull ItemStack clicked, @NotNull ItemStack cursor) {
+                                                    @NotNull ItemStack clicked, @NotNull ItemStack cursor) {
         InventoryClickResult clickResult = startCondition(player, inventory, slot, ClickType.START_SHIFT_CLICK, clicked, cursor);
         if (clickResult.isCancel()) return clickResult;
-        if (clicked.isAir()) return null;
+        if (clicked.isAir()) return clickResult.cancelled();
         final var pair = TransactionType.ADD.process(targetInventory, clicked, (index, itemStack) -> {
             if (inventory == targetInventory && index == slot)
                 return false; // Prevent item lose/duplication
             InventoryClickResult result = startCondition(player, targetInventory, index, ClickType.SHIFT_CLICK, itemStack, cursor);
             if (result.isCancel()) {
-                clickResult.setRefresh(true);
+                clickResult.setCancel(true);
                 return false;
             }
             return true;
@@ -264,11 +264,11 @@ public final class InventoryClickProcessor {
         return clickResult;
     }
 
-    public @Nullable InventoryClickResult doubleClick(@NotNull AbstractInventory clickedInventory, @NotNull AbstractInventory inventory, @NotNull Player player, int slot,
+    public @NotNull InventoryClickResult doubleClick(@NotNull AbstractInventory clickedInventory, @NotNull AbstractInventory inventory, @NotNull Player player, int slot,
-                                                      @NotNull final ItemStack cursor) {
+                                                     @NotNull final ItemStack cursor) {
         InventoryClickResult clickResult = startCondition(player, inventory, slot, ClickType.START_DOUBLE_CLICK, ItemStack.AIR, cursor);
         if (clickResult.isCancel()) return clickResult;
-        if (cursor.isAir()) return null;
+        if (cursor.isAir()) return clickResult.cancelled();
 
         final StackingRule cursorRule = cursor.getStackingRule();
         final int amount = cursorRule.getAmount(cursor);
@@ -391,7 +391,6 @@ public final class InventoryClickProcessor {
                                                          @NotNull ItemStack clicked, @NotNull ItemStack cursor) {
         final InventoryClickResult clickResult = new InventoryClickResult(clicked, cursor);
         final Inventory eventInventory = inventory instanceof Inventory ? (Inventory) inventory : null;
-        clickResult.setPlayerInventory(eventInventory == null);
 
         // Reset the didCloseInventory field
         // Wait for inventory conditions + events to possibly close the inventory
@@ -404,7 +403,6 @@ public final class InventoryClickProcessor {
             clickResult.setCursor(inventoryPreClickEvent.getCursorItem());
             clickResult.setClicked(inventoryPreClickEvent.getClickedItem());
             if (inventoryPreClickEvent.isCancelled()) {
-                clickResult.setRefresh(true);
                 clickResult.setCancel(true);
             }
         }
@@ -419,7 +417,6 @@ public final class InventoryClickProcessor {
                     clickResult.setCursor(result.getCursorItem());
                     clickResult.setClicked(result.getClickedItem());
                     if (result.isCancel()) {
-                        clickResult.setRefresh(true);
                         clickResult.setCancel(true);
                     }
                 }

src/main/java/net/minestom/server/inventory/click/InventoryClickResult.java

@@ -2,15 +2,10 @@ package net.minestom.server.inventory.click;
 
 import net.minestom.server.item.ItemStack;
 
-public class InventoryClickResult {
+public final class InventoryClickResult {
-
     private ItemStack clicked;
     private ItemStack cursor;
-
-    private boolean playerInventory;
-
     private boolean cancel;
-    private boolean refresh;
 
     public InventoryClickResult(ItemStack clicked, ItemStack cursor) {
         this.clicked = clicked;
@@ -21,7 +16,7 @@ public class InventoryClickResult {
         return clicked;
     }
 
-    protected void setClicked(ItemStack clicked) {
+    void setClicked(ItemStack clicked) {
         this.clicked = clicked;
     }
 
@@ -29,31 +24,20 @@ public class InventoryClickResult {
         return cursor;
     }
 
-    protected void setCursor(ItemStack cursor) {
+    void setCursor(ItemStack cursor) {
         this.cursor = cursor;
     }
 
-    public boolean isPlayerInventory() {
-        return playerInventory;
-    }
-
-    protected void setPlayerInventory(boolean playerInventory) {
-        this.playerInventory = playerInventory;
-    }
-
     public boolean isCancel() {
         return cancel;
     }
 
-    protected void setCancel(boolean cancel) {
+    void setCancel(boolean cancel) {
         this.cancel = cancel;
     }
 
-    public boolean doRefresh() {
+    InventoryClickResult cancelled() {
-        return refresh;
+        setCancel(true);
-    }
+        return this;
-
-    protected void setRefresh(boolean refresh) {
-        this.refresh = refresh;
     }
 }

src/main/java/net/minestom/server/listener/CreativeInventoryActionListener.java

@@ -1,21 +1,16 @@
 package net.minestom.server.listener;
 
-import net.minestom.server.entity.GameMode;
 import net.minestom.server.entity.Player;
 import net.minestom.server.inventory.PlayerInventory;
 import net.minestom.server.item.ItemStack;
 import net.minestom.server.network.packet.client.play.ClientCreativeInventoryActionPacket;
 import net.minestom.server.utils.inventory.PlayerInventoryUtils;
 
-public class CreativeInventoryActionListener {
+public final class CreativeInventoryActionListener {
-
     public static void listener(ClientCreativeInventoryActionPacket packet, Player player) {
-        if (player.getGameMode() != GameMode.CREATIVE)
+        if (!player.isCreative()) return;
-            return;
-
         short slot = packet.slot;
         final ItemStack item = packet.item;
-
         if (slot != -1) {
             // Set item
             slot = (short) PlayerInventoryUtils.convertPlayerInventorySlot(slot, PlayerInventoryUtils.OFFSET);
@@ -25,7 +20,5 @@ public class CreativeInventoryActionListener {
             // Drop item
             player.dropItem(item);
         }
-
     }
-
 }