From 554f96bc249b50698229d525025a32b4275c46f1 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 20 May 2022 16:54:06 +0100 Subject: [PATCH] InventoryManager: Defer opening new windows to the client until the window close handshake has been completed fixes #5021 and probably a bunch of other inventory related glitches When the server initiates a window close, it does so by sending a ContainerClose to the client, which causes the client to behave as if it initiated the close itself. It responds by sending a ContainerClose back to the server, which the server is then expected to respond to. Sending the client a new window before sending this final response creates buggy behaviour on the client, which is problematic when switching windows. Therefore, we defer sending any new windows until after the client responds to our window close instruction, so that we can complete the window handshake correctly. This is a pile of complicated garbage that only exists because Mojang overengineered the process of opening and closing inventory windows. --- src/network/mcpe/InventoryManager.php | 102 ++++++++++++++++---------- src/player/Player.php | 1 - 2 files changed, 65 insertions(+), 38 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index dc7a9cc2e..d727a0312 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -67,13 +67,6 @@ use function spl_object_id; * @phpstan-type ContainerOpenClosure \Closure(int $id, Inventory $inventory) : (list|null) */ class InventoryManager{ - - //TODO: HACK! - //these IDs are used for 1.16 to restore 1.14ish crafting & inventory behaviour; since they don't seem to have any - //effect on the behaviour of inventory transactions I don't currently plan to integrate these into the main system. - private const RESERVED_WINDOW_ID_RANGE_START = ContainerIds::LAST - 10; - private const HARDCODED_INVENTORY_WINDOW_ID = self::RESERVED_WINDOW_ID_RANGE_START + 2; - /** @var Player */ private $player; /** @var NetworkSession */ @@ -84,15 +77,6 @@ class InventoryManager{ /** @var int */ private $lastInventoryNetworkId = ContainerIds::FIRST; - /** - * TODO: HACK! This tracks GUIs for inventories that the server considers "always open" so that the client can't - * open them twice. (1.16 hack) - * @var true[] - * @phpstan-var array - * @internal - */ - protected $openHardcodedWindows = []; - /** * @var Item[][] * @phpstan-var array> @@ -104,6 +88,10 @@ class InventoryManager{ /** @phpstan-var ObjectSet */ private ObjectSet $containerOpenCallbacks; + private ?int $pendingCloseWindowId = null; + /** @phpstan-var \Closure() : void */ + private ?\Closure $pendingOpenWindowCallback = null; + public function __construct(Player $player, NetworkSession $session){ $this->player = $player; $this->session = $session; @@ -125,6 +113,12 @@ class InventoryManager{ $this->windowMap[$id] = $inventory; } + private function addDynamic(Inventory $inventory) : int{ + $this->lastInventoryNetworkId = max(ContainerIds::FIRST, ($this->lastInventoryNetworkId + 1) % ContainerIds::LAST); + $this->add($this->lastInventoryNetworkId, $inventory); + return $this->lastInventoryNetworkId; + } + private function remove(int $id) : void{ unset($this->windowMap[$id], $this->initiatedSlotChanges[$id]); } @@ -168,21 +162,47 @@ class InventoryManager{ } } + /** + * When the server initiates a window close, it does so by sending a ContainerClose to the client, which causes the + * client to behave as if it initiated the close itself. It responds by sending a ContainerClose back to the server, + * which the server is then expected to respond to. + * + * Sending the client a new window before sending this final response creates buggy behaviour on the client, which + * is problematic when switching windows. Therefore, we defer sending any new windows until after the client + * responds to our window close instruction, so that we can complete the window handshake correctly. + * + * This is a pile of complicated garbage that only exists because Mojang overengineered the process of opening and + * closing inventory windows. + * + * @phpstan-param \Closure() : void $func + */ + private function openWindowDeferred(\Closure $func) : void{ + if($this->pendingCloseWindowId !== null){ + $this->session->getLogger()->debug("Deferring opening of new window, waiting for close ack of window $this->pendingCloseWindowId"); + $this->pendingOpenWindowCallback = $func; + }else{ + $func(); + } + } + public function onCurrentWindowChange(Inventory $inventory) : void{ $this->onCurrentWindowRemove(); - $this->add($this->lastInventoryNetworkId = max(ContainerIds::FIRST, ($this->lastInventoryNetworkId + 1) % self::RESERVED_WINDOW_ID_RANGE_START), $inventory); - foreach($this->containerOpenCallbacks as $callback){ - $pks = $callback($this->lastInventoryNetworkId, $inventory); - if($pks !== null){ - foreach($pks as $pk){ - $this->session->sendDataPacket($pk); + $this->openWindowDeferred(function() use ($inventory) : void{ + $windowId = $this->addDynamic($inventory); + + foreach($this->containerOpenCallbacks as $callback){ + $pks = $callback($windowId, $inventory); + if($pks !== null){ + foreach($pks as $pk){ + $this->session->sendDataPacket($pk); + } + $this->syncContents($inventory); + return; } - $this->syncContents($inventory); - return; } - } - throw new \LogicException("Unsupported inventory type"); + throw new \LogicException("Unsupported inventory type"); + }); } /** @phpstan-return ObjectSet */ @@ -218,31 +238,32 @@ class InventoryManager{ } public function onClientOpenMainInventory() : void{ - $id = self::HARDCODED_INVENTORY_WINDOW_ID; - if(!isset($this->openHardcodedWindows[$id])){ - //TODO: HACK! this restores 1.14ish behaviour, but this should be able to be listened to and - //controlled by plugins. However, the player is always a subscriber to their own inventory so it - //doesn't integrate well with the regular container system right now. - $this->openHardcodedWindows[$id] = true; + $this->onCurrentWindowRemove(); + + $this->openWindowDeferred(function() : void{ + $windowId = $this->addDynamic($this->player->getInventory()); + $this->session->sendDataPacket(ContainerOpenPacket::entityInv( - InventoryManager::HARDCODED_INVENTORY_WINDOW_ID, + $windowId, WindowTypes::INVENTORY, $this->player->getId() )); - } + }); } public function onCurrentWindowRemove() : void{ if(isset($this->windowMap[$this->lastInventoryNetworkId])){ $this->remove($this->lastInventoryNetworkId); $this->session->sendDataPacket(ContainerClosePacket::create($this->lastInventoryNetworkId, true)); + if($this->pendingCloseWindowId !== null){ + throw new AssumptionFailedError("We should not have opened a new window while a window was waiting to be closed"); + } + $this->pendingCloseWindowId = $this->lastInventoryNetworkId; } } public function onClientRemoveWindow(int $id) : void{ - if(isset($this->openHardcodedWindows[$id])){ - unset($this->openHardcodedWindows[$id]); - }elseif($id === $this->lastInventoryNetworkId){ + if($id === $this->lastInventoryNetworkId){ $this->remove($id); $this->player->removeCurrentWindow(); }else{ @@ -252,6 +273,13 @@ class InventoryManager{ //Always send this, even if no window matches. If we told the client to close a window, it will behave as if it //initiated the close and expect an ack. $this->session->sendDataPacket(ContainerClosePacket::create($id, false)); + + if($this->pendingOpenWindowCallback !== null && $id === $this->pendingCloseWindowId){ + $this->session->getLogger()->debug("Opening deferred window after close ack of window $id"); + $this->pendingCloseWindowId = null; + ($this->pendingOpenWindowCallback)(); + $this->pendingOpenWindowCallback = null; + } } public function syncSlot(Inventory $inventory, int $slot) : void{ diff --git a/src/player/Player.php b/src/player/Player.php index 61922be88..112baf821 100644 --- a/src/player/Player.php +++ b/src/player/Player.php @@ -2481,7 +2481,6 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ return false; } - //TODO: client side race condition here makes the opening work incorrectly $this->removeCurrentWindow(); if(($inventoryManager = $this->getNetworkSession()->getInvManager()) === null){