diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index cb8c73eb3..ea978d2cb 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -51,6 +51,7 @@ use pocketmine\network\mcpe\protocol\MobEquipmentPacket; use pocketmine\network\mcpe\protocol\types\BlockPosition; use pocketmine\network\mcpe\protocol\types\inventory\ContainerIds; use pocketmine\network\mcpe\protocol\types\inventory\CreativeContentEntry; +use pocketmine\network\mcpe\protocol\types\inventory\ItemStack; use pocketmine\network\mcpe\protocol\types\inventory\ItemStackWrapper; use pocketmine\network\mcpe\protocol\types\inventory\NetworkInventoryAction; use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset; @@ -100,6 +101,8 @@ class InventoryManager{ private ?\Closure $pendingOpenWindowCallback = null; private int $nextItemStackId = 1; + private ?int $currentItemStackRequestId = null; + /** * @var int[][] * @phpstan-var array> @@ -186,7 +189,7 @@ class InventoryManager{ return null; } - public function addPredictedSlotChange(Inventory $inventory, int $slot, Item $item) : void{ + private function addPredictedSlotChange(Inventory $inventory, int $slot, Item $item) : void{ $predictions = ($this->initiatedSlotChanges[spl_object_id($inventory)] ??= new InventoryManagerPredictedChanges($inventory)); $predictions->add($slot, $item); } @@ -223,6 +226,10 @@ class InventoryManager{ } } + public function setCurrentItemStackRequestId(?int $id) : void{ + $this->currentItemStackRequestId = $id; + } + /** * 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, @@ -390,8 +397,25 @@ class InventoryManager{ //BDS (Bedrock Dedicated Server) also seems to work this way. $this->session->sendDataPacket(InventoryContentPacket::create($windowId, [$itemStackWrapper])); }else{ + if($this->currentItemStackRequestId !== null){ + //TODO: HACK! + //When right-clicking to equip armour, the client predicts the content of the armour slot, but + //doesn't report it in the transaction packet. The server then sends an InventorySlotPacket to + //the client, assuming the slot changed for some other reason, since there is no prediction for + //the slot. + //However, later requests involving that itemstack will refer to the request ID in which the + //armour was equipped, instead of the stack ID provided by the server in the outgoing + //InventorySlotPacket. (Perhaps because the item is already the same as the client actually + //predicted, but didn't tell us?) + //We work around this bug by setting the slot to air and then back to the correct item. In + //theory, setting a different count and then back again (or changing any other property) would + //also work, but this is simpler. + $this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, new ItemStackWrapper(0, ItemStack::null()))); + } $this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, $itemStackWrapper)); } + }elseif($this->currentItemStackRequestId !== null){ + $this->trackItemStack($inventory, $slot, $currentItem, $this->currentItemStackRequestId); } $predictions?->remove($slot); } @@ -498,11 +522,15 @@ class InventoryManager{ return $this->nextItemStackId++; } - public function trackItemStack(Inventory $inventory, int $slotId, Item $item, ?int $itemStackRequestId) : ItemStackInfo{ + public function getItemStackInfo(Inventory $inventory, int $slot) : ?ItemStackInfo{ + return $this->itemStackInfos[spl_object_id($inventory)][$slot] ?? null; + } + + private function trackItemStack(Inventory $inventory, int $slotId, Item $item, ?int $itemStackRequestId) : ItemStackInfo{ $existing = $this->itemStackInfos[spl_object_id($inventory)][$slotId] ?? null; $typeConverter = TypeConverter::getInstance(); $itemStack = $typeConverter->coreItemStackToNet($item); - if($existing !== null && $existing->getItemStack()->equals($itemStack)){ + if($existing !== null && $existing->getItemStack()->equals($itemStack) && $existing->getRequestId() === $itemStackRequestId){ return $existing; } @@ -515,7 +543,7 @@ class InventoryManager{ return new ItemStackWrapper($info->getStackId(), $info->getItemStack()); } - public function matchItemStack(Inventory $inventory, int $slotId, int $itemStackId) : bool{ + public function matchItemStack(Inventory $inventory, int $slotId, int $clientItemStackId) : bool{ $inventoryObjectId = spl_object_id($inventory); if(!isset($this->itemStackInfos[$inventoryObjectId])){ $this->session->getLogger()->debug("Attempted to match item preimage unsynced inventory " . get_class($inventory) . "#" . $inventoryObjectId); @@ -527,10 +555,10 @@ class InventoryManager{ return false; } - if(!($itemStackId < 0 ? $info->getRequestId() === $itemStackId : $info->getStackId() === $itemStackId)){ + if(!($clientItemStackId < 0 ? $info->getRequestId() === $clientItemStackId : $info->getStackId() === $clientItemStackId)){ $this->session->getLogger()->debug( "Mismatched expected itemstack: " . get_class($inventory) . "#" . $inventoryObjectId . ", " . - "slot: $slotId, expected: $itemStackId, actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none") + "slot: $slotId, client expected: $clientItemStackId, server actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none") ); return false; } diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 51be22658..c618c2c16 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -97,6 +97,8 @@ use pocketmine\network\mcpe\protocol\types\inventory\MismatchTransactionData; use pocketmine\network\mcpe\protocol\types\inventory\NetworkInventoryAction; use pocketmine\network\mcpe\protocol\types\inventory\NormalTransactionData; use pocketmine\network\mcpe\protocol\types\inventory\ReleaseItemTransactionData; +use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\ItemStackRequest; +use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse; use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset; use pocketmine\network\mcpe\protocol\types\inventory\UseItemOnEntityTransactionData; use pocketmine\network\mcpe\protocol\types\inventory\UseItemTransactionData; @@ -258,6 +260,8 @@ class InGamePacketHandler extends PacketHandler{ if(count($useItemTransaction->getTransactionData()->getActions()) > 100){ throw new PacketHandlingException("Too many actions in item use transaction"); } + + $this->inventoryManager->setCurrentItemStackRequestId($useItemTransaction->getRequestId()); $this->inventoryManager->addRawPredictedSlotChanges($useItemTransaction->getTransactionData()->getActions()); if(!$this->handleUseItemTransaction($useItemTransaction->getTransactionData())){ $packetHandled = false; @@ -265,14 +269,13 @@ class InGamePacketHandler extends PacketHandler{ }else{ $this->inventoryManager->syncMismatchedPredictedSlotChanges(); } + $this->inventoryManager->setCurrentItemStackRequestId(null); } $itemStackRequest = $packet->getItemStackRequest(); if($itemStackRequest !== null){ - $executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $itemStackRequest); - $transaction = $executor->generateInventoryTransaction(); - $result = $this->executeInventoryTransaction($transaction); - $this->session->sendDataPacket(ItemStackResponsePacket::create([$executor->buildItemStackResponse($result)])); + $result = $this->handleSingleItemStackRequest($itemStackRequest); + $this->session->sendDataPacket(ItemStackResponsePacket::create([$result])); } $blockActions = $packet->getBlockActions(); @@ -330,10 +333,11 @@ class InGamePacketHandler extends PacketHandler{ throw new PacketHandlingException("Too many actions in inventory transaction"); } + $this->inventoryManager->setCurrentItemStackRequestId($packet->requestId); $this->inventoryManager->addRawPredictedSlotChanges($packet->trData->getActions()); if($packet->trData instanceof NormalTransactionData){ - $result = $this->handleNormalTransaction($packet->trData); + $result = $this->handleNormalTransaction($packet->trData, $packet->requestId); }elseif($packet->trData instanceof MismatchTransactionData){ $this->session->getLogger()->debug("Mismatch transaction received"); $this->inventoryManager->syncAll(); @@ -349,22 +353,14 @@ class InGamePacketHandler extends PacketHandler{ if($this->craftingTransaction === null){ //don't sync if we're waiting to complete a crafting transaction $this->inventoryManager->syncMismatchedPredictedSlotChanges(); } - if($packet->requestId !== 0){ - $itemStackResponseBuilder = new ItemStackResponseBuilder($packet->requestId, $this->inventoryManager); - foreach($packet->requestChangedSlots as $requestChangedSlotsEntry){ - foreach($requestChangedSlotsEntry->getChangedSlotIndexes() as $slotId){ - $itemStackResponseBuilder->addSlot($requestChangedSlotsEntry->getContainerId(), $slotId); - } - } - $itemStackResponse = $itemStackResponseBuilder->build($result); - $this->session->sendDataPacket(ItemStackResponsePacket::create([$itemStackResponse])); - $this->session->getLogger()->debug("Sent item stack response for fake stack request " . $packet->requestId); - } + $this->inventoryManager->setCurrentItemStackRequestId(null); return $result; } - private function executeInventoryTransaction(InventoryTransaction $transaction) : bool{ + private function executeInventoryTransaction(InventoryTransaction $transaction, int $requestId) : bool{ $this->player->setUsingItem(false); + + $this->inventoryManager->setCurrentItemStackRequestId($requestId); $this->inventoryManager->addTransactionPredictedSlotChanges($transaction); try{ $transaction->execute(); @@ -373,12 +369,15 @@ class InGamePacketHandler extends PacketHandler{ $logger->debug("Failed to execute inventory transaction: " . $e->getMessage()); return false; + }finally{ + $this->inventoryManager->syncMismatchedPredictedSlotChanges(); + $this->inventoryManager->setCurrentItemStackRequestId(null); } return true; } - private function handleNormalTransaction(NormalTransactionData $data) : bool{ + private function handleNormalTransaction(NormalTransactionData $data, int $itemStackRequestId) : bool{ /** @var InventoryAction[] $actions */ $actions = []; @@ -426,7 +425,7 @@ class InGamePacketHandler extends PacketHandler{ return true; } try{ - return $this->executeInventoryTransaction($this->craftingTransaction); + return $this->executeInventoryTransaction($this->craftingTransaction, $itemStackRequestId); }finally{ $this->craftingTransaction = null; } @@ -443,7 +442,7 @@ class InGamePacketHandler extends PacketHandler{ return true; } - return $this->executeInventoryTransaction(new InventoryTransaction($this->player, $actions)); + return $this->executeInventoryTransaction(new InventoryTransaction($this->player, $actions), $itemStackRequestId); } } @@ -558,14 +557,19 @@ class InGamePacketHandler extends PacketHandler{ return false; } + private function handleSingleItemStackRequest(ItemStackRequest $request) : ItemStackResponse{ + $executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $request); + $transaction = $executor->generateInventoryTransaction(); + $result = $this->executeInventoryTransaction($transaction, $request->getRequestId()); + $this->session->getLogger()->debug("Item stack request " . $request->getRequestId() . " result: " . ($result ? "success" : "failure")); + + return $executor->buildItemStackResponse($result); + } + public function handleItemStackRequest(ItemStackRequestPacket $packet) : bool{ $responses = []; foreach($packet->getRequests() as $request){ - $executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $request); - $transaction = $executor->generateInventoryTransaction(); - $result = $this->executeInventoryTransaction($transaction); - $this->session->getLogger()->debug("Item stack request " . $request->getRequestId() . " result: " . ($result ? "success" : "failure")); - $responses[] = $executor->buildItemStackResponse($result); + $responses[] = $this->handleSingleItemStackRequest($request); } $this->session->sendDataPacket(ItemStackResponsePacket::create($responses)); diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index 11315695e..e83a75db4 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -70,14 +70,23 @@ final class ItemStackResponseBuilder{ foreach($slotIds as $slotId){ [$inventory, $slot] = $this->getInventoryAndSlot($containerInterfaceId, $slotId); + $itemStackInfo = $this->inventoryManager->getItemStackInfo($inventory, $slot); + if($itemStackInfo === null){ + //TODO: what if a plugin closes the inventory while the transaction is ongoing? + throw new \LogicException("ItemStackInfo should never be null for an open inventory"); + } + if($itemStackInfo->getRequestId() !== $this->requestId){ + //the itemstack may have been synced due to transaction producing results that the client did not + //predict correctly, which will wipe out the tracked request ID (intentionally) + continue; + } $item = $inventory->getItem($slot); - $info = $this->inventoryManager->trackItemStack($inventory, $slot, $item, $this->requestId); $responseInfosByContainer[$containerInterfaceId][] = new ItemStackResponseSlotInfo( $slotId, $slotId, - $info->getItemStack()->getCount(), - $info->getStackId(), + $item->getCount(), + $itemStackInfo->getStackId(), $item->hasCustomName() ? $item->getCustomName() : "", 0 );