From 1bdc50f4a0c3e7b03d1e8249e0e861dc1a99d670 Mon Sep 17 00:00:00 2001 From: TheMode Date: Fri, 13 Aug 2021 20:41:59 +0200 Subject: [PATCH] Trust client's inventory prediction when possible --- .../net/minestom/server/entity/Player.java | 1 - .../minestom/server/inventory/Inventory.java | 114 ++++++------------ .../server/inventory/PlayerInventory.java | 112 +++++++---------- .../click/InventoryClickProcessor.java | 21 ++-- .../inventory/click/InventoryClickResult.java | 30 ++--- .../CreativeInventoryActionListener.java | 11 +- 6 files changed, 97 insertions(+), 192 deletions(-) diff --git a/src/main/java/net/minestom/server/entity/Player.java b/src/main/java/net/minestom/server/entity/Player.java index 52b9e5672..b29720506 100644 --- a/src/main/java/net/minestom/server/entity/Player.java +++ b/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(); } /** diff --git a/src/main/java/net/minestom/server/inventory/Inventory.java b/src/main/java/net/minestom/server/inventory/Inventory.java index 65261a8d1..1debb3311 100644 --- a/src/main/java/net/minestom/server/inventory/Inventory.java +++ b/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()) { - updateFromClick(clickResult, player); + if (clickResult.isCancel()) { + updateAll(player); + return false; } - if (isInWindow) { setItemStack(slot, clickResult.getClicked()); } else { playerInventory.setItemStack(clickSlot, clickResult.getClicked()); } refreshPlayerCursorItem(player, clickResult.getCursor()); - - if (!clickResult.isCancel()) - callClickEvent(player, isInWindow ? this : null, slot, ClickType.LEFT_CLICK, clicked, cursor); - - return !clickResult.isCancel(); + callClickEvent(player, isInWindow ? this : null, slot, ClickType.LEFT_CLICK, clicked, cursor); + return true; } @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()) { - updateFromClick(clickResult, player); + if (clickResult.isCancel()) { + updateAll(player); + return false; } - if (isInWindow) { setItemStack(slot, clickResult.getClicked()); } else { playerInventory.setItemStack(clickSlot, clickResult.getClicked()); } refreshPlayerCursorItem(player, clickResult.getCursor()); - - if (!clickResult.isCancel()) - callClickEvent(player, isInWindow ? this : null, slot, ClickType.RIGHT_CLICK, clicked, cursor); - - return !clickResult.isCancel(); + callClickEvent(player, isInWindow ? this : null, slot, ClickType.RIGHT_CLICK, clicked, cursor); + return true; } @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()); } - - if (clickResult.doRefresh()) { - update(player); - } - + updateAll(player); // FIXME: currently not properly client-predicted refreshPlayerCursorItem(player, clickResult.getCursor()); - - return !clickResult.isCancel(); + return true; } @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()) { - updateFromClick(clickResult, player); + if (clickResult.isCancel()) { + updateAll(player); + return false; } - if (isInWindow) { setItemStack(slot, clickResult.getClicked()); } else { playerInventory.setItemStack(clickSlot, clickResult.getClicked()); } playerInventory.setItemStack(key, clickResult.getCursor()); - - if (!clickResult.isCancel()) - callClickEvent(player, isInWindow ? this : null, slot, ClickType.CHANGE_HELD, clicked, getCursorItem(player)); - - // Weird synchronization issue when omitted - updateFromClick(clickResult, player); - - return !clickResult.isCancel(); + callClickEvent(player, isInWindow ? this : null, slot, ClickType.CHANGE_HELD, clicked, getCursorItem(player)); + return true; } @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.doRefresh()) { - updateFromClick(clickResult, player); + if (clickResult.isCancel()) { + updateAll(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 !clickResult.isCancel(); + return true; } @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) { + if (clickResult == null || clickResult.isCancel()) { + updateAll(player); return false; } - - if (clickResult.doRefresh()) { - updateFromClick(clickResult, player); - } - refreshPlayerCursorItem(player, clickResult.getCursor()); - - return !clickResult.isCancel(); + return true; } @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 == null) + if (clickResult.isCancel()) { + updateAll(player); return false; - - if (clickResult.doRefresh()) - updateFromClick(clickResult, player); - + } refreshPlayerCursorItem(player, clickResult.getCursor()); - - return !clickResult.isCancel(); + return true; } - /** - * Used to update the inventory for a specific player in order to fix his cancelled actions - * - * @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); - } + private void updateAll(Player player) { + player.getInventory().update(); + update(player); } } diff --git a/src/main/java/net/minestom/server/inventory/PlayerInventory.java b/src/main/java/net/minestom/server/inventory/PlayerInventory.java index 3e4bdafca..2df6226dc 100644 --- a/src/main/java/net/minestom/server/inventory/PlayerInventory.java +++ b/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(); - // 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); + sendSlotRefresh((short) convertToPacketSlot(slot), itemStack); } /** @@ -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()) - sendSlotRefresh((short) slot, clicked); - + if (clickResult.isCancel()) { + update(); + return false; + } setItemStack(convertedSlot, clickResult.getClicked()); setCursorItem(clickResult.getCursor()); - - if (!clickResult.isCancel()) - callClickEvent(player, null, convertedSlot, ClickType.LEFT_CLICK, clicked, cursor); - - return !clickResult.isCancel(); + callClickEvent(player, null, convertedSlot, ClickType.LEFT_CLICK, clicked, cursor); + return true; } @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()) - sendSlotRefresh((short) slot, clicked); - + if (clickResult.isCancel()) { + update(); + return false; + } setItemStack(convertedSlot, clickResult.getClicked()); setCursorItem(clickResult.getCursor()); - - if (!clickResult.isCancel()) - callClickEvent(player, null, convertedSlot, ClickType.RIGHT_CLICK, clicked, cursor); - - return !clickResult.isCancel(); + callClickEvent(player, null, convertedSlot, ClickType.RIGHT_CLICK, clicked, cursor); + return true; } @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()) - sendSlotRefresh((short) slot, clicked); - + if (clickResult.isCancel()) { + update(); + return false; + } final ItemStack resultClicked = clickResult.getClicked(); - if (resultClicked != null && !outsideDrop) + if (resultClicked != null && !outsideDrop) { setItemStack(slot, OFFSET, resultClicked); + } setCursorItem(clickResult.getCursor()); - - return !clickResult.isCancel(); + return true; } @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()); - - return !clickResult.isCancel(); + update(); // FIXME: currently not properly client-predicted + return true; } @Override public boolean changeHeld(@NotNull Player player, int slot, int key) { - if (!getCursorItem().isAir()) - return false; - + final ItemStack cursorItem = getCursorItem(); + 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()) { - sendSlotRefresh((short) slot, clicked); + if (clickResult.isCancel()) { + update(); + return false; } - setItemStack(slot, OFFSET, clickResult.getClicked()); setItemStack(key, clickResult.getCursor()); - - if (!clickResult.isCancel()) - callClickEvent(player, null, slot, ClickType.CHANGE_HELD, clicked, getCursorItem()); - - // Weird synchronization issue when omitted - update(); - - return !clickResult.isCancel(); + callClickEvent(player, null, slot, ClickType.CHANGE_HELD, clicked, cursorItem); + return true; } @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 !clickResult.isCancel(); + return true; } @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) - return false; - if (clickResult.doRefresh()) + if (clickResult.isCancel()) { update(); + return false; + } setCursorItem(clickResult.getCursor()); - return !clickResult.isCancel(); + return true; } } diff --git a/src/main/java/net/minestom/server/inventory/click/InventoryClickProcessor.java b/src/main/java/net/minestom/server/inventory/click/InventoryClickProcessor.java index 964da85e2..ba7be2039 100644 --- a/src/main/java/net/minestom/server/inventory/click/InventoryClickProcessor.java +++ b/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, - int start, int end, int step, - @NotNull Player player, int slot, - @NotNull ItemStack clicked, @NotNull ItemStack cursor) { + public @NotNull InventoryClickResult shiftClick(@NotNull AbstractInventory inventory, @NotNull AbstractInventory targetInventory, + int start, int end, int step, + @NotNull Player player, int slot, + @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, - @NotNull final ItemStack cursor) { + public @NotNull InventoryClickResult doubleClick(@NotNull AbstractInventory clickedInventory, @NotNull AbstractInventory inventory, @NotNull Player player, int slot, + @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); } } diff --git a/src/main/java/net/minestom/server/inventory/click/InventoryClickResult.java b/src/main/java/net/minestom/server/inventory/click/InventoryClickResult.java index 9b29192ba..5a5384565 100644 --- a/src/main/java/net/minestom/server/inventory/click/InventoryClickResult.java +++ b/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() { - return refresh; - } - - protected void setRefresh(boolean refresh) { - this.refresh = refresh; + InventoryClickResult cancelled() { + setCancel(true); + return this; } } diff --git a/src/main/java/net/minestom/server/listener/CreativeInventoryActionListener.java b/src/main/java/net/minestom/server/listener/CreativeInventoryActionListener.java index 02d42a6c2..04353db7d 100644 --- a/src/main/java/net/minestom/server/listener/CreativeInventoryActionListener.java +++ b/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) - return; - + if (!player.isCreative()) 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); } - } - }