From 2b7510945a3efd97836ae85cc08bfe44581169a9 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 18 Aug 2022 17:38:57 +0100 Subject: [PATCH 01/32] First look at ItemStackRequest usage (very unstable) --- composer.json | 2 +- composer.lock | 38 +-- src/network/mcpe/InventoryManager.php | 145 ++++++--- .../mcpe/InventoryManagerPredictedChanges.php | 61 ++++ src/network/mcpe/ItemStackInfo.php | 41 +++ .../mcpe/handler/InGamePacketHandler.php | 77 +++-- .../mcpe/handler/ItemStackRequestExecutor.php | 288 ++++++++++++++++++ .../mcpe/handler/PreSpawnPacketHandler.php | 2 +- 8 files changed, 560 insertions(+), 94 deletions(-) create mode 100644 src/network/mcpe/InventoryManagerPredictedChanges.php create mode 100644 src/network/mcpe/ItemStackInfo.php create mode 100644 src/network/mcpe/handler/ItemStackRequestExecutor.php diff --git a/composer.json b/composer.json index 5334687cf..8d5d7adad 100644 --- a/composer.json +++ b/composer.json @@ -35,7 +35,7 @@ "fgrosse/phpasn1": "^2.3", "netresearch/jsonmapper": "^4.0", "pocketmine/bedrock-data": "~1.10.0+bedrock-1.19.20", - "pocketmine/bedrock-protocol": "~12.0.0+bedrock-1.19.20", + "pocketmine/bedrock-protocol": "~12.1.0+bedrock-1.19.20", "pocketmine/binaryutils": "^0.2.1", "pocketmine/callback-validator": "^1.0.2", "pocketmine/classloader": "^0.2.0", diff --git a/composer.lock b/composer.lock index 34cce5d63..8eb97af45 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "80afa24adf37096a23643e051d6128ce", + "content-hash": "df65cc2917f6ab13745a48a6eb2fe48a", "packages": [ { "name": "adhocore/json-comment", @@ -63,16 +63,16 @@ }, { "name": "brick/math", - "version": "0.10.1", + "version": "0.10.2", "source": { "type": "git", "url": "https://github.com/brick/math.git", - "reference": "de846578401f4e58f911b3afeb62ced56365ed87" + "reference": "459f2781e1a08d52ee56b0b1444086e038561e3f" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/brick/math/zipball/de846578401f4e58f911b3afeb62ced56365ed87", - "reference": "de846578401f4e58f911b3afeb62ced56365ed87", + "url": "https://api.github.com/repos/brick/math/zipball/459f2781e1a08d52ee56b0b1444086e038561e3f", + "reference": "459f2781e1a08d52ee56b0b1444086e038561e3f", "shasum": "" }, "require": { @@ -107,7 +107,7 @@ ], "support": { "issues": "https://github.com/brick/math/issues", - "source": "https://github.com/brick/math/tree/0.10.1" + "source": "https://github.com/brick/math/tree/0.10.2" }, "funding": [ { @@ -115,7 +115,7 @@ "type": "github" } ], - "time": "2022-08-01T22:54:31+00:00" + "time": "2022-08-10T22:54:19+00:00" }, { "name": "fgrosse/phpasn1", @@ -271,16 +271,16 @@ }, { "name": "pocketmine/bedrock-protocol", - "version": "12.0.0+bedrock-1.19.20", + "version": "12.1.0+bedrock-1.19.20", "source": { "type": "git", "url": "https://github.com/pmmp/BedrockProtocol.git", - "reference": "c2778039544fa0c7c5bd3af7963149e7552f4215" + "reference": "f754df1c7becfad89599052e3ac07852a02dc4dd" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/c2778039544fa0c7c5bd3af7963149e7552f4215", - "reference": "c2778039544fa0c7c5bd3af7963149e7552f4215", + "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/f754df1c7becfad89599052e3ac07852a02dc4dd", + "reference": "f754df1c7becfad89599052e3ac07852a02dc4dd", "shasum": "" }, "require": { @@ -312,9 +312,9 @@ "description": "An implementation of the Minecraft: Bedrock Edition protocol in PHP", "support": { "issues": "https://github.com/pmmp/BedrockProtocol/issues", - "source": "https://github.com/pmmp/BedrockProtocol/tree/bedrock-1.19.20" + "source": "https://github.com/pmmp/BedrockProtocol/tree/12.1.0+bedrock-1.19.20" }, - "time": "2022-08-09T17:57:29+00:00" + "time": "2022-08-16T19:55:35+00:00" }, { "name": "pocketmine/binaryutils", @@ -532,16 +532,16 @@ }, { "name": "pocketmine/locale-data", - "version": "2.8.3", + "version": "2.8.4", "source": { "type": "git", "url": "https://github.com/pmmp/Language.git", - "reference": "113c115a3b8976917eb22b74dccab464831b6483" + "reference": "6709467487d270c962deee16972c7786949d1511" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/pmmp/Language/zipball/113c115a3b8976917eb22b74dccab464831b6483", - "reference": "113c115a3b8976917eb22b74dccab464831b6483", + "url": "https://api.github.com/repos/pmmp/Language/zipball/6709467487d270c962deee16972c7786949d1511", + "reference": "6709467487d270c962deee16972c7786949d1511", "shasum": "" }, "type": "library", @@ -549,9 +549,9 @@ "description": "Language resources used by PocketMine-MP", "support": { "issues": "https://github.com/pmmp/Language/issues", - "source": "https://github.com/pmmp/Language/tree/2.8.3" + "source": "https://github.com/pmmp/Language/tree/2.8.4" }, - "time": "2022-05-11T13:51:37+00:00" + "time": "2022-08-16T17:47:52+00:00" }, { "name": "pocketmine/log", diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 323136086..b831ef486 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -87,7 +87,7 @@ class InventoryManager{ /** * @var Item[][] - * @phpstan-var array> + * @phpstan-var array */ private array $initiatedSlotChanges = []; private int $clientSelectedHotbarSlot = -1; @@ -99,6 +99,13 @@ class InventoryManager{ /** @phpstan-var \Closure() : void */ private ?\Closure $pendingOpenWindowCallback = null; + private int $nextItemStackId = 1; + /** + * @var int[][] + * @phpstan-var array> + */ + private array $itemStackInfos = []; + public function __construct( private Player $player, private NetworkSession $session @@ -141,11 +148,14 @@ class InventoryManager{ private function remove(int $id) : void{ $inventory = $this->windowMap[$id]; - $splObjectId = spl_object_id($inventory); - unset($this->windowMap[$id], $this->initiatedSlotChanges[$id], $this->complexWindows[$splObjectId]); - foreach($this->complexSlotToWindowMap as $netSlot => $entry){ - if($entry->getInventory() === $inventory){ - unset($this->complexSlotToWindowMap[$netSlot]); + unset($this->windowMap[$id]); + if($this->getWindowId($inventory) === null){ + $splObjectId = spl_object_id($inventory); + unset($this->initiatedSlotChanges[$splObjectId], $this->itemStackInfos[$splObjectId], $this->complexWindows[$splObjectId]); + foreach($this->complexSlotToWindowMap as $netSlot => $entry){ + if($entry->getInventory() === $inventory){ + unset($this->complexSlotToWindowMap[$netSlot]); + } } } } @@ -159,7 +169,7 @@ class InventoryManager{ } /** - * @phpstan-return array{Inventory, int} + * @phpstan-return array{Inventory, int}|null */ public function locateWindowAndSlot(int $windowId, int $netSlotId) : ?array{ if($windowId === ContainerIds::UI){ @@ -176,11 +186,15 @@ class InventoryManager{ return null; } - public function onTransactionStart(InventoryTransaction $tx) : void{ + public function addPredictedSlotChange(Inventory $inventory, int $slot, Item $item) : void{ + $predictions = ($this->initiatedSlotChanges[spl_object_id($inventory)] ??= new InventoryManagerPredictedChanges($inventory)); + $predictions->add($slot, $item); + } + + public function addTransactionPredictedSlotChanges(InventoryTransaction $tx) : void{ foreach($tx->getActions() as $action){ - if($action instanceof SlotChangeAction && ($windowId = $this->getWindowId($action->getInventory())) !== null){ - //in some cases the inventory might not have a window ID, but still be referenced by a transaction (e.g. crafting grid changes), so we can't unconditionally record the change here or we might leak things - $this->initiatedSlotChanges[$windowId][$action->getSlot()] = $action->getTargetItem(); + if($action instanceof SlotChangeAction){ + $this->addPredictedSlotChange($action->getInventory(), $action->getSlot(), $action->getTargetItem()); } } } @@ -189,17 +203,23 @@ class InventoryManager{ * @param NetworkInventoryAction[] $networkInventoryActions * @throws PacketHandlingException */ - public function addPredictedSlotChanges(array $networkInventoryActions) : void{ + public function addRawPredictedSlotChanges(array $networkInventoryActions) : void{ foreach($networkInventoryActions as $action){ - if($action->sourceType === NetworkInventoryAction::SOURCE_CONTAINER && isset($this->windowMap[$action->windowId])){ - //this won't cover stuff like crafting grid due to too much magic - try{ - $item = TypeConverter::getInstance()->netItemStackToCore($action->newItem->getItemStack()); - }catch(TypeConversionException $e){ - throw new PacketHandlingException($e->getMessage(), 0, $e); - } - $this->initiatedSlotChanges[$action->windowId][$action->inventorySlot] = $item; + if($action->sourceType !== NetworkInventoryAction::SOURCE_CONTAINER){ + continue; } + $info = $this->locateWindowAndSlot($action->windowId, $action->inventorySlot); + if($info === null){ + continue; + } + + [$inventory, $slot] = $info; + try{ + $item = TypeConverter::getInstance()->netItemStackToCore($action->newItem->getItemStack()); + }catch(TypeConversionException $e){ + throw new PacketHandlingException($e->getMessage(), 0, $e); + } + $this->addPredictedSlotChange($inventory, $slot, $item); } } @@ -356,9 +376,10 @@ class InventoryManager{ } if($windowId !== null && $netSlot !== null){ $currentItem = $inventory->getItem($slot); - $clientSideItem = $this->initiatedSlotChanges[$windowId][$netSlot] ?? null; + $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; + $clientSideItem = $predictions?->getSlot($slot); if($clientSideItem === null || !$clientSideItem->equalsExact($currentItem)){ - $itemStackWrapper = ItemStackWrapper::legacy(TypeConverter::getInstance()->coreItemStackToNet($currentItem)); + $itemStackWrapper = $this->wrapItemStack($inventory, $slot, $currentItem); if($windowId === ContainerIds::OFFHAND){ //TODO: HACK! //The client may sometimes ignore the InventorySlotPacket for the offhand slot. @@ -370,7 +391,7 @@ class InventoryManager{ $this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, $itemStackWrapper)); } } - unset($this->initiatedSlotChanges[$windowId][$netSlot]); + $predictions?->remove($slot); } } @@ -381,26 +402,28 @@ class InventoryManager{ }else{ $windowId = $this->getWindowId($inventory); } - $typeConverter = TypeConverter::getInstance(); if($windowId !== null){ if($slotMap !== null){ + $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; foreach($inventory->getContents(true) as $slotId => $item){ $packetSlot = $slotMap->mapCoreToNet($slotId) ?? null; if($packetSlot === null){ continue; } - unset($this->initiatedSlotChanges[$windowId][$packetSlot]); + $predictions?->remove($slotId); $this->session->sendDataPacket(InventorySlotPacket::create( $windowId, $packetSlot, - ItemStackWrapper::legacy($typeConverter->coreItemStackToNet($inventory->getItem($slotId))) + $this->wrapItemStack($inventory, $slotId, $inventory->getItem($slotId)) )); } }else{ - unset($this->initiatedSlotChanges[$windowId]); - $this->session->sendDataPacket(InventoryContentPacket::create($windowId, array_map(function(Item $itemStack) use ($typeConverter) : ItemStackWrapper{ - return ItemStackWrapper::legacy($typeConverter->coreItemStackToNet($itemStack)); - }, $inventory->getContents(true)))); + unset($this->initiatedSlotChanges[spl_object_id($inventory)]); + $contents = []; + foreach($inventory->getContents(true) as $slotId => $item){ + $contents[] = $this->wrapItemStack($inventory, $slotId, $item); + } + $this->session->sendDataPacket(InventoryContentPacket::create($windowId, $contents)); } } } @@ -415,14 +438,9 @@ class InventoryManager{ } public function syncMismatchedPredictedSlotChanges() : void{ - foreach($this->initiatedSlotChanges as $windowId => $slots){ - foreach($slots as $netSlot => $expectedItem){ - $located = $this->locateWindowAndSlot($windowId, $netSlot); - if($located === null){ - continue; - } - [$inventory, $slot] = $located; - + foreach($this->initiatedSlotChanges as $predictions){ + $inventory = $predictions->getInventory(); + foreach($predictions->getSlots() as $slot => $expectedItem){ if(!$inventory->slotExists($slot)){ continue; //TODO: size desync ??? } @@ -449,11 +467,14 @@ class InventoryManager{ } public function syncSelectedHotbarSlot() : void{ - $selected = $this->player->getInventory()->getHeldItemIndex(); + $playerInventory = $this->player->getInventory(); + $selected = $playerInventory->getHeldItemIndex(); if($selected !== $this->clientSelectedHotbarSlot){ + $itemStackInfo = $this->itemStackInfos[spl_object_id($playerInventory)][$selected]; + $this->session->sendDataPacket(MobEquipmentPacket::create( $this->player->getId(), - ItemStackWrapper::legacy(TypeConverter::getInstance()->coreItemStackToNet($this->player->getInventory()->getItemInHand())), + new ItemStackWrapper($itemStackInfo->getStackId(), $itemStackInfo->getItemStack()), $selected, $selected, ContainerIds::INVENTORY @@ -470,4 +491,48 @@ class InventoryManager{ return new CreativeContentEntry($nextEntryId++, $typeConverter->coreItemStackToNet($item)); }, $this->player->isSpectator() ? [] : CreativeInventory::getInstance()->getAll()))); } + + private function newItemStackId() : int{ + return $this->nextItemStackId++; + } + + public 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)){ + return $existing; + } + + $info = new ItemStackInfo($itemStackRequestId, $item->isNull() ? 0 : $this->newItemStackId(), $itemStack); + return $this->itemStackInfos[spl_object_id($inventory)][$slotId] = $info; + } + + private function wrapItemStack(Inventory $inventory, int $slotId, Item $item) : ItemStackWrapper{ + $info = $this->trackItemStack($inventory, $slotId, $item, null); + return new ItemStackWrapper($info->getStackId(), $info->getItemStack()); + } + + public function matchItemStack(Inventory $inventory, int $slotId, int $itemStackId) : 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); + return false; + } + $info = $this->itemStackInfos[$inventoryObjectId][$slotId] ?? null; + if($info === null){ + $this->session->getLogger()->debug("Attempted to match item preimage for unsynced slot $slotId in " . get_class($inventory) . "#$inventoryObjectId that isn't synced"); + return false; + } + + if(!($itemStackId < 0 ? $info->getRequestId() === $itemStackId : $info->getStackId() === $itemStackId)){ + $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") + ); + return false; + } + + return true; + } } diff --git a/src/network/mcpe/InventoryManagerPredictedChanges.php b/src/network/mcpe/InventoryManagerPredictedChanges.php new file mode 100644 index 000000000..a381234a4 --- /dev/null +++ b/src/network/mcpe/InventoryManagerPredictedChanges.php @@ -0,0 +1,61 @@ + + */ + private array $slots = []; + + public function __construct( + private Inventory $inventory + ){} + + public function getInventory() : Inventory{ return $this->inventory; } + + /** + * @return Item[] + * @phpstan-return array + */ + public function getSlots() : array{ + return $this->slots; + } + + public function getSlot(int $slot) : ?Item{ + return $this->slots[$slot] ?? null; + } + + public function add(int $slot, Item $item) : void{ + $this->slots[$slot] = $item; + } + + public function remove(int $slot) : void{ + unset($this->slots[$slot]); + } +} diff --git a/src/network/mcpe/ItemStackInfo.php b/src/network/mcpe/ItemStackInfo.php new file mode 100644 index 000000000..630765bfa --- /dev/null +++ b/src/network/mcpe/ItemStackInfo.php @@ -0,0 +1,41 @@ +requestId; } + + public function getStackId() : int{ return $this->stackId; } + + public function getItemStack() : ItemStack{ return $this->itemStack; } +} diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 7a8caabfe..3b44cd027 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -65,6 +65,8 @@ use pocketmine\network\mcpe\protocol\EmotePacket; use pocketmine\network\mcpe\protocol\InteractPacket; use pocketmine\network\mcpe\protocol\InventoryTransactionPacket; use pocketmine\network\mcpe\protocol\ItemFrameDropItemPacket; +use pocketmine\network\mcpe\protocol\ItemStackRequestPacket; +use pocketmine\network\mcpe\protocol\ItemStackResponsePacket; use pocketmine\network\mcpe\protocol\LabTablePacket; use pocketmine\network\mcpe\protocol\LecternUpdatePacket; use pocketmine\network\mcpe\protocol\LevelSoundEventPacket; @@ -119,7 +121,6 @@ use function is_bool; use function is_infinite; use function is_nan; use function json_decode; -use function json_encode; use function max; use function mb_strlen; use function microtime; @@ -263,6 +264,14 @@ class InGamePacketHandler extends PacketHandler{ } } + $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)])); + } + $blockActions = $packet->getBlockActions(); if($blockActions !== null){ foreach($blockActions as $k => $blockAction){ @@ -333,6 +342,21 @@ class InGamePacketHandler extends PacketHandler{ return $result; } + private function executeInventoryTransaction(InventoryTransaction $transaction) : bool{ + $this->player->setUsingItem(false); + $this->inventoryManager->addTransactionPredictedSlotChanges($transaction); + try{ + $transaction->execute(); + }catch(TransactionException $e){ + $logger = $this->session->getLogger(); + $logger->debug("Failed to execute inventory transaction: " . $e->getMessage()); + + return false; + } + + return true; + } + private function handleNormalTransaction(NormalTransactionData $data) : bool{ /** @var InventoryAction[] $actions */ $actions = []; @@ -380,18 +404,8 @@ class InGamePacketHandler extends PacketHandler{ //all of the parts before we can execute it return true; } - $this->player->setUsingItem(false); try{ - $this->inventoryManager->onTransactionStart($this->craftingTransaction); - $this->craftingTransaction->execute(); - }catch(TransactionException $e){ - $this->session->getLogger()->debug("Failed to execute crafting transaction: " . $e->getMessage()); - - //TODO: only sync slots that the client tried to change - foreach($this->craftingTransaction->getInventories() as $inventory){ - $this->inventoryManager->syncContents($inventory); - } - return false; + return $this->executeInventoryTransaction($this->craftingTransaction); }finally{ $this->craftingTransaction = null; } @@ -408,30 +422,13 @@ class InGamePacketHandler extends PacketHandler{ return true; } - $this->player->setUsingItem(false); - $transaction = new InventoryTransaction($this->player, $actions); - $this->inventoryManager->onTransactionStart($transaction); - try{ - $transaction->execute(); - }catch(TransactionException $e){ - $logger = $this->session->getLogger(); - $logger->debug("Failed to execute inventory transaction: " . $e->getMessage()); - $logger->debug("Actions: " . json_encode($data->getActions())); - - foreach($transaction->getInventories() as $inventory){ - $this->inventoryManager->syncContents($inventory); - } - - return false; - } + return $this->executeInventoryTransaction(new InventoryTransaction($this->player, $actions)); } - - return true; } private function handleUseItemTransaction(UseItemTransactionData $data) : bool{ $this->player->selectHotbarSlot($data->getHotbarSlot()); - $this->inventoryManager->addPredictedSlotChanges($data->getActions()); + $this->inventoryManager->addRawPredictedSlotChanges($data->getActions()); switch($data->getActionType()){ case UseItemTransactionData::ACTION_CLICK_BLOCK: @@ -517,7 +514,7 @@ class InGamePacketHandler extends PacketHandler{ } $this->player->selectHotbarSlot($data->getHotbarSlot()); - $this->inventoryManager->addPredictedSlotChanges($data->getActions()); + $this->inventoryManager->addRawPredictedSlotChanges($data->getActions()); //TODO: use transactiondata for rollbacks here switch($data->getActionType()){ @@ -534,7 +531,7 @@ class InGamePacketHandler extends PacketHandler{ private function handleReleaseItemTransaction(ReleaseItemTransactionData $data) : bool{ $this->player->selectHotbarSlot($data->getHotbarSlot()); - $this->inventoryManager->addPredictedSlotChanges($data->getActions()); + $this->inventoryManager->addRawPredictedSlotChanges($data->getActions()); //TODO: use transactiondata for rollbacks here (resending entire inventory is very wasteful) switch($data->getActionType()){ @@ -548,6 +545,20 @@ class InGamePacketHandler extends PacketHandler{ return false; } + 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); + $responses[] = $executor->buildItemStackResponse($result); + } + + $this->session->sendDataPacket(ItemStackResponsePacket::create($responses)); + + return true; + } + public function handleMobEquipment(MobEquipmentPacket $packet) : bool{ if($packet->windowId === ContainerIds::OFFHAND){ return true; //this happens when we put an item into the offhand diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php new file mode 100644 index 000000000..52c46f919 --- /dev/null +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -0,0 +1,288 @@ +builder = new TransactionBuilder(); + } + + private function translateContainerId(int $containerInterfaceId) : int{ + return match($containerInterfaceId){ + ContainerUIIds::ARMOR => ContainerIds::ARMOR, + + ContainerUIIds::HOTBAR, + ContainerUIIds::INVENTORY, + ContainerUIIds::COMBINED_HOTBAR_AND_INVENTORY => ContainerIds::INVENTORY, + + ContainerUIIds::OFFHAND => ContainerIds::OFFHAND, + + ContainerUIIds::ANVIL_INPUT, + ContainerUIIds::ANVIL_MATERIAL, + ContainerUIIds::BEACON_PAYMENT, + ContainerUIIds::CARTOGRAPHY_ADDITIONAL, + ContainerUIIds::CARTOGRAPHY_INPUT, + ContainerUIIds::COMPOUND_CREATOR_INPUT, + ContainerUIIds::CRAFTING_INPUT, + ContainerUIIds::CREATED_OUTPUT, + ContainerUIIds::CURSOR, + ContainerUIIds::ENCHANTING_INPUT, + ContainerUIIds::ENCHANTING_MATERIAL, + ContainerUIIds::GRINDSTONE_ADDITIONAL, + ContainerUIIds::GRINDSTONE_INPUT, + ContainerUIIds::LAB_TABLE_INPUT, + ContainerUIIds::LOOM_DYE, + ContainerUIIds::LOOM_INPUT, + ContainerUIIds::LOOM_MATERIAL, + ContainerUIIds::MATERIAL_REDUCER_INPUT, + ContainerUIIds::MATERIAL_REDUCER_OUTPUT, + ContainerUIIds::SMITHING_TABLE_INPUT, + ContainerUIIds::SMITHING_TABLE_MATERIAL, + ContainerUIIds::STONECUTTER_INPUT, + ContainerUIIds::TRADE2_INGREDIENT1, + ContainerUIIds::TRADE2_INGREDIENT2, + ContainerUIIds::TRADE_INGREDIENT1, + ContainerUIIds::TRADE_INGREDIENT2 => ContainerIds::UI, + + ContainerUIIds::BARREL, + ContainerUIIds::BLAST_FURNACE_INGREDIENT, + ContainerUIIds::BREWING_STAND_FUEL, + ContainerUIIds::BREWING_STAND_INPUT, + ContainerUIIds::BREWING_STAND_RESULT, + ContainerUIIds::FURNACE_FUEL, + ContainerUIIds::FURNACE_INGREDIENT, + ContainerUIIds::FURNACE_RESULT, + ContainerUIIds::LEVEL_ENTITY, //chest + ContainerUIIds::SHULKER_BOX, + ContainerUIIds::SMOKER_INGREDIENT => $this->inventoryManager->getCurrentWindowId(), + + //all preview slots are ignored, since the client shouldn't be modifying those directly + + default => throw new PacketHandlingException("Unexpected container UI ID $containerInterfaceId") + }; + } + + /** + * @phpstan-return array{Inventory, int} + */ + private function getInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{ + $windowId = $this->translateContainerId($info->getContainerId()); + $info = $this->inventoryManager->locateWindowAndSlot($windowId, $info->getSlotId()); + if($info === null){ + throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); + } + [$inventory, $slot] = $info; + if(!$inventory->slotExists($slot)){ + throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); + } + + return [$inventory, $slot]; + } + + /** + * @phpstan-return array{TransactionBuilderInventory, int} + */ + private function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{ + [$inventory, $slot] = $this->getInventoryAndSlot($info); + + if( + $info->getStackId() !== $this->request->getRequestId() && //using TransactionBuilderInventory enables this to work + !$this->inventoryManager->matchItemStack($inventory, $slot, $info->getStackId()) + ){ + throw new PacketHandlingException("Inventory " . $info->getContainerId() . ", slot " . $slot . ": server-side item does not match expected"); + } + + return [$this->builder->getInventory($inventory), $slot]; + } + + private function transferItems(ItemStackRequestSlotInfo $source, ItemStackRequestSlotInfo $destination, int $count) : void{ + [$sourceInventory, $sourceSlot] = $this->getBuilderInventoryAndSlot($source); + [$targetInventory, $targetSlot] = $this->getBuilderInventoryAndSlot($destination); + + $oldSourceItem = $sourceInventory->getItem($sourceSlot); + $oldTargetItem = $targetInventory->getItem($targetSlot); + + if(!$targetInventory->isSlotEmpty($targetSlot) && !$oldTargetItem->canStackWith($oldSourceItem)){ + throw new PacketHandlingException("Can only transfer items into an empty slot, or a slot containing the same item"); + } + [$newSourceItem, $newTargetItem] = $this->splitStack($oldSourceItem, $count, $oldTargetItem->getCount()); + + $sourceInventory->setItem($sourceSlot, $newSourceItem); + $targetInventory->setItem($targetSlot, $newTargetItem); + } + + /** + * @phpstan-return array{Item, Item} + */ + private function splitStack(Item $item, int $transferredCount, int $targetCount) : array{ + if($item->getCount() < $transferredCount){ + throw new PacketHandlingException("Cannot take $transferredCount items from a stack of " . $item->getCount()); + } + + $leftover = clone $item; + $removed = $leftover->pop($transferredCount); + $removed->setCount($removed->getCount() + $targetCount); + if($leftover->isNull()){ + $leftover = VanillaItems::AIR(); + } + + return [$leftover, $removed]; + } + + private function processItemStackRequestAction(ItemStackRequestAction $action) : void{ + if( + $action instanceof TakeStackRequestAction || + $action instanceof PlaceStackRequestAction + ){ + $this->requestSlotInfos[] = $action->getSource(); + $this->requestSlotInfos[] = $action->getDestination(); + $this->transferItems($action->getSource(), $action->getDestination(), $action->getCount()); + }elseif($action instanceof SwapStackRequestAction){ + $this->requestSlotInfos[] = $action->getSlot1(); + $this->requestSlotInfos[] = $action->getSlot2(); + + [$inventory1, $slot1] = $this->getBuilderInventoryAndSlot($action->getSlot1()); + [$inventory2, $slot2] = $this->getBuilderInventoryAndSlot($action->getSlot2()); + + $item1 = $inventory1->getItem($slot1); + $item2 = $inventory2->getItem($slot2); + $inventory1->setItem($slot1, $item2); + $inventory2->setItem($slot2, $item1); + }elseif($action instanceof DropStackRequestAction){ + $this->requestSlotInfos[] = $action->getSource(); + [$inventory, $slot] = $this->getBuilderInventoryAndSlot($action->getSource()); + + $oldItem = $inventory->getItem($slot); + [$leftover, $dropped] = $this->splitStack($oldItem, $action->getCount(), 0); + + //TODO: this action has a "randomly" field, I have no idea what it's used for + $inventory->setItem($slot, $leftover); + $this->builder->addAction(new DropItemAction($dropped)); + }elseif($action instanceof DestroyStackRequestAction){ + $this->requestSlotInfos[] = $action->getSource(); + [$inventory, $slot] = $this->getBuilderInventoryAndSlot($action->getSource()); + + $oldItem = $inventory->getItem($slot); + [$leftover, $destroyed] = $this->splitStack($oldItem, $action->getCount(), 0); + + $inventory->setItem($slot, $leftover); + $this->builder->addAction(new DestroyItemAction($destroyed)); + }elseif($action instanceof CraftingConsumeInputStackRequestAction){ + //we don't need this for the PM system + $this->requestSlotInfos[] = $action->getSource(); + $this->crafting = true; + }elseif( + $action instanceof CraftRecipeStackRequestAction || //TODO + $action instanceof CraftRecipeAutoStackRequestAction || //TODO + $action instanceof CraftingMarkSecondaryResultStackRequestAction || //no obvious use + $action instanceof DeprecatedCraftingResultsStackRequestAction //no obvious use + ){ + $this->crafting = true; + }else{ + throw new PacketHandlingException("Unhandled item stack request action: " . get_class($action)); + } + } + + public function generateInventoryTransaction() : InventoryTransaction{ + foreach($this->request->getActions() as $action){ + $this->processItemStackRequestAction($action); + } + $inventoryActions = $this->builder->generateActions(); + + return $this->crafting ? + new CraftingTransaction($this->player, $this->player->getServer()->getCraftingManager(), $inventoryActions) : + new InventoryTransaction($this->player, $inventoryActions); + } + + public function buildItemStackResponse(bool $success) : ItemStackResponse{ + $responseInfosByContainer = []; + foreach($this->requestSlotInfos as $requestInfo){ + [$inventory, $slot] = $this->getInventoryAndSlot($requestInfo); + + $item = $inventory->getItem($slot); + $info = $this->inventoryManager->trackItemStack($inventory, $slot, $item, $this->request->getRequestId()); + + $responseInfosByContainer[$requestInfo->getContainerId()][] = new ItemStackResponseSlotInfo( + $requestInfo->getSlotId(), + $requestInfo->getSlotId(), + $info->getItemStack()->getCount(), + $info->getStackId(), + $item->hasCustomName() ? $item->getCustomName() : "", + 0 + ); + } + + $responseContainerInfos = []; + foreach($responseInfosByContainer as $containerId => $responseInfos){ + $responseContainerInfos[] = new ItemStackResponseContainerInfo($containerId, $responseInfos); + } + + return new ItemStackResponse($success ? ItemStackResponse::RESULT_OK : ItemStackResponse::RESULT_ERROR, $this->request->getRequestId(), $responseContainerInfos); + } +} diff --git a/src/network/mcpe/handler/PreSpawnPacketHandler.php b/src/network/mcpe/handler/PreSpawnPacketHandler.php index 92e502351..cf7c9b0f3 100644 --- a/src/network/mcpe/handler/PreSpawnPacketHandler.php +++ b/src/network/mcpe/handler/PreSpawnPacketHandler.php @@ -96,7 +96,7 @@ class PreSpawnPacketHandler extends PacketHandler{ 0, 0, "", - false, + true, sprintf("%s %s", VersionInfo::NAME, VersionInfo::VERSION()->getFullVersion(true)), Uuid::fromString(Uuid::NIL), false, From 3235d128e5f9e5cb58e596eb8c8131b2a71bde28 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 18 Aug 2022 18:25:49 +0100 Subject: [PATCH 02/32] Fixed handling of fake requests during block placement and other actions --- .../mcpe/handler/InGamePacketHandler.php | 12 ++ .../ItemStackContainerIdTranslator.php | 90 ++++++++++++++ .../mcpe/handler/ItemStackRequestExecutor.php | 112 ++---------------- .../mcpe/handler/ItemStackResponseBuilder.php | 94 +++++++++++++++ 4 files changed, 208 insertions(+), 100 deletions(-) create mode 100644 src/network/mcpe/handler/ItemStackContainerIdTranslator.php create mode 100644 src/network/mcpe/handler/ItemStackResponseBuilder.php diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 3b44cd027..252eddb2e 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -339,6 +339,17 @@ class InGamePacketHandler extends PacketHandler{ }else{ $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); + } return $result; } @@ -551,6 +562,7 @@ class InGamePacketHandler extends PacketHandler{ $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); } diff --git a/src/network/mcpe/handler/ItemStackContainerIdTranslator.php b/src/network/mcpe/handler/ItemStackContainerIdTranslator.php new file mode 100644 index 000000000..a0ad3b26c --- /dev/null +++ b/src/network/mcpe/handler/ItemStackContainerIdTranslator.php @@ -0,0 +1,90 @@ + ContainerIds::ARMOR, + + ContainerUIIds::HOTBAR, + ContainerUIIds::INVENTORY, + ContainerUIIds::COMBINED_HOTBAR_AND_INVENTORY => ContainerIds::INVENTORY, + + ContainerUIIds::OFFHAND => ContainerIds::OFFHAND, + + ContainerUIIds::ANVIL_INPUT, + ContainerUIIds::ANVIL_MATERIAL, + ContainerUIIds::BEACON_PAYMENT, + ContainerUIIds::CARTOGRAPHY_ADDITIONAL, + ContainerUIIds::CARTOGRAPHY_INPUT, + ContainerUIIds::COMPOUND_CREATOR_INPUT, + ContainerUIIds::CRAFTING_INPUT, + ContainerUIIds::CREATED_OUTPUT, + ContainerUIIds::CURSOR, + ContainerUIIds::ENCHANTING_INPUT, + ContainerUIIds::ENCHANTING_MATERIAL, + ContainerUIIds::GRINDSTONE_ADDITIONAL, + ContainerUIIds::GRINDSTONE_INPUT, + ContainerUIIds::LAB_TABLE_INPUT, + ContainerUIIds::LOOM_DYE, + ContainerUIIds::LOOM_INPUT, + ContainerUIIds::LOOM_MATERIAL, + ContainerUIIds::MATERIAL_REDUCER_INPUT, + ContainerUIIds::MATERIAL_REDUCER_OUTPUT, + ContainerUIIds::SMITHING_TABLE_INPUT, + ContainerUIIds::SMITHING_TABLE_MATERIAL, + ContainerUIIds::STONECUTTER_INPUT, + ContainerUIIds::TRADE2_INGREDIENT1, + ContainerUIIds::TRADE2_INGREDIENT2, + ContainerUIIds::TRADE_INGREDIENT1, + ContainerUIIds::TRADE_INGREDIENT2 => ContainerIds::UI, + + ContainerUIIds::BARREL, + ContainerUIIds::BLAST_FURNACE_INGREDIENT, + ContainerUIIds::BREWING_STAND_FUEL, + ContainerUIIds::BREWING_STAND_INPUT, + ContainerUIIds::BREWING_STAND_RESULT, + ContainerUIIds::FURNACE_FUEL, + ContainerUIIds::FURNACE_INGREDIENT, + ContainerUIIds::FURNACE_RESULT, + ContainerUIIds::LEVEL_ENTITY, //chest + ContainerUIIds::SHULKER_BOX, + ContainerUIIds::SMOKER_INGREDIENT => $currentWindowId, + + //all preview slots are ignored, since the client shouldn't be modifying those directly + + default => throw new PacketHandlingException("Unexpected container UI ID $containerInterfaceId") + }; + } +} diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index 52c46f919..4b7923205 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -23,8 +23,6 @@ declare(strict_types=1); namespace pocketmine\network\mcpe\handler; -use pocketmine\block\inventory\CraftingTableInventory; -use pocketmine\inventory\Inventory; use pocketmine\inventory\transaction\action\DestroyItemAction; use pocketmine\inventory\transaction\action\DropItemAction; use pocketmine\inventory\transaction\CraftingTransaction; @@ -34,8 +32,6 @@ use pocketmine\inventory\transaction\TransactionBuilderInventory; use pocketmine\item\Item; use pocketmine\item\VanillaItems; use pocketmine\network\mcpe\InventoryManager; -use pocketmine\network\mcpe\protocol\types\inventory\ContainerIds; -use pocketmine\network\mcpe\protocol\types\inventory\ContainerUIIds; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingConsumeInputStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingMarkSecondaryResultStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftRecipeAutoStackRequestAction; @@ -50,12 +46,9 @@ use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\PlaceStackRequ use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\SwapStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\TakeStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse; -use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseContainerInfo; -use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseSlotInfo; use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; use function get_class; -use function var_dump; final class ItemStackRequestExecutor{ private TransactionBuilder $builder; @@ -73,83 +66,19 @@ final class ItemStackRequestExecutor{ $this->builder = new TransactionBuilder(); } - private function translateContainerId(int $containerInterfaceId) : int{ - return match($containerInterfaceId){ - ContainerUIIds::ARMOR => ContainerIds::ARMOR, - - ContainerUIIds::HOTBAR, - ContainerUIIds::INVENTORY, - ContainerUIIds::COMBINED_HOTBAR_AND_INVENTORY => ContainerIds::INVENTORY, - - ContainerUIIds::OFFHAND => ContainerIds::OFFHAND, - - ContainerUIIds::ANVIL_INPUT, - ContainerUIIds::ANVIL_MATERIAL, - ContainerUIIds::BEACON_PAYMENT, - ContainerUIIds::CARTOGRAPHY_ADDITIONAL, - ContainerUIIds::CARTOGRAPHY_INPUT, - ContainerUIIds::COMPOUND_CREATOR_INPUT, - ContainerUIIds::CRAFTING_INPUT, - ContainerUIIds::CREATED_OUTPUT, - ContainerUIIds::CURSOR, - ContainerUIIds::ENCHANTING_INPUT, - ContainerUIIds::ENCHANTING_MATERIAL, - ContainerUIIds::GRINDSTONE_ADDITIONAL, - ContainerUIIds::GRINDSTONE_INPUT, - ContainerUIIds::LAB_TABLE_INPUT, - ContainerUIIds::LOOM_DYE, - ContainerUIIds::LOOM_INPUT, - ContainerUIIds::LOOM_MATERIAL, - ContainerUIIds::MATERIAL_REDUCER_INPUT, - ContainerUIIds::MATERIAL_REDUCER_OUTPUT, - ContainerUIIds::SMITHING_TABLE_INPUT, - ContainerUIIds::SMITHING_TABLE_MATERIAL, - ContainerUIIds::STONECUTTER_INPUT, - ContainerUIIds::TRADE2_INGREDIENT1, - ContainerUIIds::TRADE2_INGREDIENT2, - ContainerUIIds::TRADE_INGREDIENT1, - ContainerUIIds::TRADE_INGREDIENT2 => ContainerIds::UI, - - ContainerUIIds::BARREL, - ContainerUIIds::BLAST_FURNACE_INGREDIENT, - ContainerUIIds::BREWING_STAND_FUEL, - ContainerUIIds::BREWING_STAND_INPUT, - ContainerUIIds::BREWING_STAND_RESULT, - ContainerUIIds::FURNACE_FUEL, - ContainerUIIds::FURNACE_INGREDIENT, - ContainerUIIds::FURNACE_RESULT, - ContainerUIIds::LEVEL_ENTITY, //chest - ContainerUIIds::SHULKER_BOX, - ContainerUIIds::SMOKER_INGREDIENT => $this->inventoryManager->getCurrentWindowId(), - - //all preview slots are ignored, since the client shouldn't be modifying those directly - - default => throw new PacketHandlingException("Unexpected container UI ID $containerInterfaceId") - }; - } - - /** - * @phpstan-return array{Inventory, int} - */ - private function getInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{ - $windowId = $this->translateContainerId($info->getContainerId()); - $info = $this->inventoryManager->locateWindowAndSlot($windowId, $info->getSlotId()); - if($info === null){ - throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); - } - [$inventory, $slot] = $info; - if(!$inventory->slotExists($slot)){ - throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); - } - - return [$inventory, $slot]; - } - /** * @phpstan-return array{TransactionBuilderInventory, int} */ private function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{ - [$inventory, $slot] = $this->getInventoryAndSlot($info); + $windowId = ItemStackContainerIdTranslator::translate($info->getContainerId(), $this->inventoryManager->getCurrentWindowId()); + $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $info->getSlotId()); + if($windowAndSlot === null){ + throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); + } + [$inventory, $slot] = $windowAndSlot; + if(!$inventory->slotExists($slot)){ + throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); + } if( $info->getStackId() !== $this->request->getRequestId() && //using TransactionBuilderInventory enables this to work @@ -261,28 +190,11 @@ final class ItemStackRequestExecutor{ } public function buildItemStackResponse(bool $success) : ItemStackResponse{ - $responseInfosByContainer = []; + $builder = new ItemStackResponseBuilder($this->request->getRequestId(), $this->inventoryManager); foreach($this->requestSlotInfos as $requestInfo){ - [$inventory, $slot] = $this->getInventoryAndSlot($requestInfo); - - $item = $inventory->getItem($slot); - $info = $this->inventoryManager->trackItemStack($inventory, $slot, $item, $this->request->getRequestId()); - - $responseInfosByContainer[$requestInfo->getContainerId()][] = new ItemStackResponseSlotInfo( - $requestInfo->getSlotId(), - $requestInfo->getSlotId(), - $info->getItemStack()->getCount(), - $info->getStackId(), - $item->hasCustomName() ? $item->getCustomName() : "", - 0 - ); + $builder->addSlot($requestInfo->getContainerId(), $requestInfo->getSlotId()); } - $responseContainerInfos = []; - foreach($responseInfosByContainer as $containerId => $responseInfos){ - $responseContainerInfos[] = new ItemStackResponseContainerInfo($containerId, $responseInfos); - } - - return new ItemStackResponse($success ? ItemStackResponse::RESULT_OK : ItemStackResponse::RESULT_ERROR, $this->request->getRequestId(), $responseContainerInfos); + return $builder->build($success); } } diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php new file mode 100644 index 000000000..11315695e --- /dev/null +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -0,0 +1,94 @@ +> + */ + private array $changedSlots = []; + + public function __construct( + private int $requestId, + private InventoryManager $inventoryManager + ){} + + public function addSlot(int $containerInterfaceId, int $slotId) : void{ + $this->changedSlots[$containerInterfaceId][$slotId] = $slotId; + } + + /** + * @phpstan-return array{Inventory, int} + */ + private function getInventoryAndSlot(int $containerInterfaceId, int $slotId) : array{ + $windowId = ItemStackContainerIdTranslator::translate($containerInterfaceId, $this->inventoryManager->getCurrentWindowId()); + $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slotId); + if($windowAndSlot === null){ + throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); + } + [$inventory, $slot] = $windowAndSlot; + if(!$inventory->slotExists($slot)){ + throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); + } + + return [$inventory, $slot]; + } + + public function build(bool $success) : ItemStackResponse{ + $responseInfosByContainer = []; + foreach($this->changedSlots as $containerInterfaceId => $slotIds){ + foreach($slotIds as $slotId){ + [$inventory, $slot] = $this->getInventoryAndSlot($containerInterfaceId, $slotId); + + $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->hasCustomName() ? $item->getCustomName() : "", + 0 + ); + } + } + + $responseContainerInfos = []; + foreach($responseInfosByContainer as $containerInterfaceId => $responseInfos){ + $responseContainerInfos[] = new ItemStackResponseContainerInfo($containerInterfaceId, $responseInfos); + } + + return new ItemStackResponse($success ? ItemStackResponse::RESULT_OK : ItemStackResponse::RESULT_ERROR, $this->requestId, $responseContainerInfos); + } +} From d8d236842fb18f3f59d7669b0b398932f39fef12 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 3 Jan 2023 19:54:41 +0000 Subject: [PATCH 03/32] Fixed merge error --- src/network/mcpe/handler/InGamePacketHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index c617d1598..51be22658 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -258,7 +258,7 @@ class InGamePacketHandler extends PacketHandler{ if(count($useItemTransaction->getTransactionData()->getActions()) > 100){ throw new PacketHandlingException("Too many actions in item use transaction"); } - $this->inventoryManager->addPredictedSlotChanges($useItemTransaction->getTransactionData()->getActions()); + $this->inventoryManager->addRawPredictedSlotChanges($useItemTransaction->getTransactionData()->getActions()); if(!$this->handleUseItemTransaction($useItemTransaction->getTransactionData())){ $packetHandled = false; $this->session->getLogger()->debug("Unhandled transaction in PlayerAuthInputPacket (type " . $useItemTransaction->getTransactionData()->getActionType() . ")"); From 5fdbb19852dec121aa61e4e84f97983887dbb96e Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 4 Jan 2023 00:13:51 +0000 Subject: [PATCH 04/32] Fixed a whole bunch of issues with legacy transactions --- src/network/mcpe/InventoryManager.php | 40 +++++++++++--- .../mcpe/handler/InGamePacketHandler.php | 54 ++++++++++--------- .../mcpe/handler/ItemStackResponseBuilder.php | 15 ++++-- 3 files changed, 75 insertions(+), 34 deletions(-) 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 ); From 2e9a3f9160f2dbdcdf9632129221c7408ed69b86 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 4 Jan 2023 22:29:29 +0000 Subject: [PATCH 05/32] Working crafting :woohoo: --- src/crafting/CraftingManager.php | 16 ++ .../transaction/CraftingTransaction.php | 51 +++-- src/network/mcpe/cache/CraftingDataCache.php | 23 +-- .../mcpe/handler/ItemStackRequestExecutor.php | 177 ++++++++++++------ .../mcpe/handler/ItemStackResponseBuilder.php | 4 + 5 files changed, 187 insertions(+), 84 deletions(-) diff --git a/src/crafting/CraftingManager.php b/src/crafting/CraftingManager.php index 8edfaf3aa..d1421c837 100644 --- a/src/crafting/CraftingManager.php +++ b/src/crafting/CraftingManager.php @@ -45,6 +45,12 @@ class CraftingManager{ */ protected $shapelessRecipes = []; + /** + * @var CraftingRecipe[] + * @phpstan-var array + */ + private array $craftingRecipeIndex = []; + /** * @var FurnaceRecipeManager[] * @phpstan-var array @@ -153,6 +159,14 @@ class CraftingManager{ return $this->shapedRecipes; } + /** + * @return CraftingRecipe[] + * @phpstan-return array + */ + public function getCraftingRecipeIndex() : array{ + return $this->craftingRecipeIndex; + } + public function getFurnaceRecipeManager(FurnaceType $furnaceType) : FurnaceRecipeManager{ return $this->furnaceRecipeManagers[$furnaceType->id()]; } @@ -175,6 +189,7 @@ class CraftingManager{ public function registerShapedRecipe(ShapedRecipe $recipe) : void{ $this->shapedRecipes[self::hashOutputs($recipe->getResults())][] = $recipe; + $this->craftingRecipeIndex[] = $recipe; foreach($this->recipeRegisteredCallbacks as $callback){ $callback(); @@ -183,6 +198,7 @@ class CraftingManager{ public function registerShapelessRecipe(ShapelessRecipe $recipe) : void{ $this->shapelessRecipes[self::hashOutputs($recipe->getResults())][] = $recipe; + $this->craftingRecipeIndex[] = $recipe; foreach($this->recipeRegisteredCallbacks as $callback){ $callback(); diff --git a/src/inventory/transaction/CraftingTransaction.php b/src/inventory/transaction/CraftingTransaction.php index 792984e0f..5771038ed 100644 --- a/src/inventory/transaction/CraftingTransaction.php +++ b/src/inventory/transaction/CraftingTransaction.php @@ -60,9 +60,11 @@ class CraftingTransaction extends InventoryTransaction{ private CraftingManager $craftingManager; - public function __construct(Player $source, CraftingManager $craftingManager, array $actions = []){ + public function __construct(Player $source, CraftingManager $craftingManager, array $actions = [], ?CraftingRecipe $recipe = null, ?int $repetitions = null){ parent::__construct($source, $actions); $this->craftingManager = $craftingManager; + $this->recipe = $recipe; + $this->repetitions = $repetitions; } /** @@ -123,6 +125,18 @@ class CraftingTransaction extends InventoryTransaction{ return $iterations; } + private function validateRecipe(CraftingRecipe $recipe, ?int $expectedRepetitions) : int{ + //compute number of times recipe was crafted + $repetitions = $this->matchRecipeItems($this->outputs, $recipe->getResultsFor($this->source->getCraftingGrid()), false); + if($expectedRepetitions !== null && $repetitions !== $expectedRepetitions){ + throw new TransactionValidationException("Expected $expectedRepetitions repetitions, got $repetitions"); + } + //assert that $repetitions x recipe ingredients should be consumed + $this->matchRecipeItems($this->inputs, $recipe->getIngredientList(), true, $repetitions); + + return $repetitions; + } + public function validate() : void{ $this->squashDuplicateSlotChanges(); if(count($this->actions) < 1){ @@ -131,25 +145,24 @@ class CraftingTransaction extends InventoryTransaction{ $this->matchItems($this->outputs, $this->inputs); - $failed = 0; - foreach($this->craftingManager->matchRecipeByOutputs($this->outputs) as $recipe){ - try{ - //compute number of times recipe was crafted - $this->repetitions = $this->matchRecipeItems($this->outputs, $recipe->getResultsFor($this->source->getCraftingGrid()), false); - //assert that $repetitions x recipe ingredients should be consumed - $this->matchRecipeItems($this->inputs, $recipe->getIngredientList(), true, $this->repetitions); - - //Success! - $this->recipe = $recipe; - break; - }catch(TransactionValidationException $e){ - //failed - ++$failed; - } - } - if($this->recipe === null){ - throw new TransactionValidationException("Unable to match a recipe to transaction (tried to match against $failed recipes)"); + $failed = 0; + foreach($this->craftingManager->matchRecipeByOutputs($this->outputs) as $recipe){ + try{ + $this->repetitions = $this->validateRecipe($recipe, $this->repetitions); + $this->recipe = $recipe; + break; + }catch(TransactionValidationException $e){ + //failed + ++$failed; + } + } + + if($this->recipe === null){ + throw new TransactionValidationException("Unable to match a recipe to transaction (tried to match against $failed recipes)"); + } + }else{ + $this->repetitions = $this->validateRecipe($this->recipe, $this->repetitions); } } diff --git a/src/network/mcpe/cache/CraftingDataCache.php b/src/network/mcpe/cache/CraftingDataCache.php index 8589f1881..c1dc4820c 100644 --- a/src/network/mcpe/cache/CraftingDataCache.php +++ b/src/network/mcpe/cache/CraftingDataCache.php @@ -25,6 +25,8 @@ namespace pocketmine\network\mcpe\cache; use pocketmine\crafting\CraftingManager; use pocketmine\crafting\FurnaceType; +use pocketmine\crafting\ShapedRecipe; +use pocketmine\crafting\ShapelessRecipe; use pocketmine\crafting\ShapelessRecipeType; use pocketmine\item\Item; use pocketmine\network\mcpe\convert\ItemTranslator; @@ -76,12 +78,12 @@ final class CraftingDataCache{ private function buildCraftingDataCache(CraftingManager $manager) : CraftingDataPacket{ Timings::$craftingDataCacheRebuild->startTiming(); - $counter = 0; $nullUUID = Uuid::fromString(Uuid::NIL); $converter = TypeConverter::getInstance(); $recipesWithTypeIds = []; - foreach($manager->getShapelessRecipes() as $list){ - foreach($list as $recipe){ + + foreach($manager->getCraftingRecipeIndex() as $index => $recipe){ + if($recipe instanceof ShapelessRecipe){ $typeTag = match($recipe->getType()->id()){ ShapelessRecipeType::CRAFTING()->id() => CraftingRecipeBlockName::CRAFTING_TABLE, ShapelessRecipeType::STONECUTTER()->id() => CraftingRecipeBlockName::STONECUTTER, @@ -89,7 +91,7 @@ final class CraftingDataCache{ }; $recipesWithTypeIds[] = new ProtocolShapelessRecipe( CraftingDataPacket::ENTRY_SHAPELESS, - Binary::writeInt(++$counter), + Binary::writeInt($index), array_map(function(Item $item) use ($converter) : RecipeIngredient{ return $converter->coreItemStackToRecipeIngredient($item); }, $recipe->getIngredientList()), @@ -99,12 +101,9 @@ final class CraftingDataCache{ $nullUUID, $typeTag, 50, - $counter + $index ); - } - } - foreach($manager->getShapedRecipes() as $list){ - foreach($list as $recipe){ + }elseif($recipe instanceof ShapedRecipe){ $inputs = []; for($row = 0, $height = $recipe->getHeight(); $row < $height; ++$row){ @@ -114,7 +113,7 @@ final class CraftingDataCache{ } $recipesWithTypeIds[] = $r = new ProtocolShapedRecipe( CraftingDataPacket::ENTRY_SHAPED, - Binary::writeInt(++$counter), + Binary::writeInt($index), $inputs, array_map(function(Item $item) use ($converter) : ItemStack{ return $converter->coreItemStackToNet($item); @@ -122,8 +121,10 @@ final class CraftingDataCache{ $nullUUID, CraftingRecipeBlockName::CRAFTING_TABLE, 50, - $counter + $index ); + }else{ + //TODO: probably special recipe types } } diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index 4b7923205..60edbab29 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace pocketmine\network\mcpe\handler; +use pocketmine\crafting\CraftingGrid; use pocketmine\inventory\transaction\action\DestroyItemAction; use pocketmine\inventory\transaction\action\DropItemAction; use pocketmine\inventory\transaction\CraftingTransaction; @@ -30,8 +31,8 @@ use pocketmine\inventory\transaction\InventoryTransaction; use pocketmine\inventory\transaction\TransactionBuilder; use pocketmine\inventory\transaction\TransactionBuilderInventory; use pocketmine\item\Item; -use pocketmine\item\VanillaItems; use pocketmine\network\mcpe\InventoryManager; +use pocketmine\network\mcpe\protocol\types\inventory\ContainerUIIds; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingConsumeInputStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingMarkSecondaryResultStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftRecipeAutoStackRequestAction; @@ -46,8 +47,10 @@ use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\PlaceStackRequ use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\SwapStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\TakeStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse; +use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset; use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; +use pocketmine\utils\AssumptionFailedError; use function get_class; final class ItemStackRequestExecutor{ @@ -56,7 +59,11 @@ final class ItemStackRequestExecutor{ /** @var ItemStackRequestSlotInfo[] */ private array $requestSlotInfos = []; - private bool $crafting = false; + private ?InventoryTransaction $specialTransaction = null; + + /** @var Item[] */ + private array $craftingResults = []; + private int $craftingOutputIndex = 0; public function __construct( private Player $player, @@ -91,37 +98,91 @@ final class ItemStackRequestExecutor{ } private function transferItems(ItemStackRequestSlotInfo $source, ItemStackRequestSlotInfo $destination, int $count) : void{ - [$sourceInventory, $sourceSlot] = $this->getBuilderInventoryAndSlot($source); - [$targetInventory, $targetSlot] = $this->getBuilderInventoryAndSlot($destination); - - $oldSourceItem = $sourceInventory->getItem($sourceSlot); - $oldTargetItem = $targetInventory->getItem($targetSlot); - - if(!$targetInventory->isSlotEmpty($targetSlot) && !$oldTargetItem->canStackWith($oldSourceItem)){ - throw new PacketHandlingException("Can only transfer items into an empty slot, or a slot containing the same item"); - } - [$newSourceItem, $newTargetItem] = $this->splitStack($oldSourceItem, $count, $oldTargetItem->getCount()); - - $sourceInventory->setItem($sourceSlot, $newSourceItem); - $targetInventory->setItem($targetSlot, $newTargetItem); + $removed = $this->removeItemFromSlot($source, $count); + $this->addItemToSlot($destination, $removed, $count); } /** - * @phpstan-return array{Item, Item} + * Deducts items from an inventory slot, returning a stack containing the removed items. */ - private function splitStack(Item $item, int $transferredCount, int $targetCount) : array{ - if($item->getCount() < $transferredCount){ - throw new PacketHandlingException("Cannot take $transferredCount items from a stack of " . $item->getCount()); + private function removeItemFromSlot(ItemStackRequestSlotInfo $slotInfo, int $count) : Item{ + $this->requestSlotInfos[] = $slotInfo; + [$inventory, $slot] = $this->getBuilderInventoryAndSlot($slotInfo); + + $existingItem = $inventory->getItem($slot); + if($existingItem->getCount() < $count){ + throw new PacketHandlingException("Cannot take $count items from a stack of " . $existingItem->getCount()); } + $removed = $existingItem->pop($count); + $inventory->setItem($slot, $existingItem); + + return $removed; + } + + /** + * Adds items to the target slot, if they are stackable. + */ + private function addItemToSlot(ItemStackRequestSlotInfo $slotInfo, Item $item, int $count) : Item{ + $this->requestSlotInfos[] = $slotInfo; + [$inventory, $slot] = $this->getBuilderInventoryAndSlot($slotInfo); + + $existingItem = $inventory->getItem($slot); + if(!$existingItem->isNull() && !$existingItem->canStackWith($item)){ + throw new PacketHandlingException("Can only add items to an empty slot, or a slot containing the same item"); + } + + //we can't use the existing item here; it may be an empty stack + $newItem = clone $item; + $newItem->setCount($existingItem->getCount() + $count); + $inventory->setItem($slot, $newItem); + $leftover = clone $item; - $removed = $leftover->pop($transferredCount); - $removed->setCount($removed->getCount() + $targetCount); - if($leftover->isNull()){ - $leftover = VanillaItems::AIR(); + $leftover->pop($count); + + return $leftover; + } + + private function beginCrafting(int $recipeId, int $repetitions) : void{ + if($this->specialTransaction !== null){ + throw new PacketHandlingException("Cannot perform more than 1 special action per request"); + } + if($repetitions < 1){ //TODO: upper bound? + throw new PacketHandlingException("Cannot craft a recipe less than 1 time"); + } + $craftingManager = $this->player->getServer()->getCraftingManager(); + $recipe = $craftingManager->getCraftingRecipeIndex()[$recipeId] ?? null; + if($recipe === null){ + throw new PacketHandlingException("Unknown crafting recipe ID $recipeId"); } - return [$leftover, $removed]; + $this->specialTransaction = new CraftingTransaction($this->player, $craftingManager, [], $recipe, $repetitions); + + $currentWindow = $this->player->getCurrentWindow(); + if($currentWindow !== null && !($currentWindow instanceof CraftingGrid)){ + throw new PacketHandlingException("Cannot complete crafting when the player's current window is not a crafting grid"); + } + $craftingGrid = $currentWindow ?? $this->player->getCraftingGrid(); + + $craftingResults = $recipe->getResultsFor($craftingGrid); + foreach($craftingResults as $k => $craftingResult){ + $craftingResult->setCount($craftingResult->getCount() * $repetitions); + $this->craftingResults[$k] = $craftingResult; + } + } + + private function takeCraftingResult(ItemStackRequestSlotInfo $destination, int $count) : void{ + $recipeResultItem = $this->craftingResults[$this->craftingOutputIndex] ?? null; + if($recipeResultItem === null){ + throw new PacketHandlingException("Cannot refer to nonexisting crafting output index " . $this->craftingOutputIndex); + } + + $availableCount = $recipeResultItem->getCount(); + if($availableCount < $count){ + throw new PacketHandlingException("Tried to take too many results from crafting"); + } + + $this->craftingResults[$this->craftingOutputIndex] = $this->addItemToSlot($destination, $recipeResultItem, $count); } private function processItemStackRequestAction(ItemStackRequestAction $action) : void{ @@ -129,9 +190,14 @@ final class ItemStackRequestExecutor{ $action instanceof TakeStackRequestAction || $action instanceof PlaceStackRequestAction ){ - $this->requestSlotInfos[] = $action->getSource(); - $this->requestSlotInfos[] = $action->getDestination(); - $this->transferItems($action->getSource(), $action->getDestination(), $action->getCount()); + $source = $action->getSource(); + $destination = $action->getDestination(); + + if($source->getContainerId() === ContainerUIIds::CREATED_OUTPUT && $source->getSlotId() === UIInventorySlotOffset::CREATED_ITEM_OUTPUT){ + $this->takeCraftingResult($destination, $action->getCount()); + }else{ + $this->transferItems($source, $destination, $action->getCount()); + } }elseif($action instanceof SwapStackRequestAction){ $this->requestSlotInfos[] = $action->getSlot1(); $this->requestSlotInfos[] = $action->getSlot2(); @@ -144,35 +210,35 @@ final class ItemStackRequestExecutor{ $inventory1->setItem($slot1, $item2); $inventory2->setItem($slot2, $item1); }elseif($action instanceof DropStackRequestAction){ - $this->requestSlotInfos[] = $action->getSource(); - [$inventory, $slot] = $this->getBuilderInventoryAndSlot($action->getSource()); - - $oldItem = $inventory->getItem($slot); - [$leftover, $dropped] = $this->splitStack($oldItem, $action->getCount(), 0); - //TODO: this action has a "randomly" field, I have no idea what it's used for - $inventory->setItem($slot, $leftover); + $dropped = $this->removeItemFromSlot($action->getSource(), $action->getCount()); $this->builder->addAction(new DropItemAction($dropped)); + }elseif($action instanceof DestroyStackRequestAction){ - $this->requestSlotInfos[] = $action->getSource(); - [$inventory, $slot] = $this->getBuilderInventoryAndSlot($action->getSource()); - - $oldItem = $inventory->getItem($slot); - [$leftover, $destroyed] = $this->splitStack($oldItem, $action->getCount(), 0); - - $inventory->setItem($slot, $leftover); + $destroyed = $this->removeItemFromSlot($action->getSource(), $action->getCount()); $this->builder->addAction(new DestroyItemAction($destroyed)); + + }elseif($action instanceof CraftRecipeStackRequestAction){ + $this->beginCrafting($action->getRecipeId(), 1); + }elseif($action instanceof CraftRecipeAutoStackRequestAction){ + $this->beginCrafting($action->getRecipeId(), $action->getRepetitions()); }elseif($action instanceof CraftingConsumeInputStackRequestAction){ - //we don't need this for the PM system - $this->requestSlotInfos[] = $action->getSource(); - $this->crafting = true; - }elseif( - $action instanceof CraftRecipeStackRequestAction || //TODO - $action instanceof CraftRecipeAutoStackRequestAction || //TODO - $action instanceof CraftingMarkSecondaryResultStackRequestAction || //no obvious use - $action instanceof DeprecatedCraftingResultsStackRequestAction //no obvious use - ){ - $this->crafting = true; + if(!$this->specialTransaction instanceof CraftingTransaction){ + throw new PacketHandlingException("Cannot consume crafting input when no crafting transaction is in progress"); + } + $this->removeItemFromSlot($action->getSource(), $action->getCount()); //output discarded - we allow CraftingTransaction to verify the balance + + }elseif($action instanceof CraftingMarkSecondaryResultStackRequestAction){ + if(!$this->specialTransaction instanceof CraftingTransaction){ + throw new AssumptionFailedError("Cannot mark crafting result index when no crafting transaction is in progress"); + } + $outputIndex = $action->getCraftingGridSlot(); + if($outputIndex < 0){ + throw new PacketHandlingException("Crafting result index cannot be negative"); + } + $this->craftingOutputIndex = $outputIndex; + }elseif($action instanceof DeprecatedCraftingResultsStackRequestAction){ + //no obvious use }else{ throw new PacketHandlingException("Unhandled item stack request action: " . get_class($action)); } @@ -184,9 +250,12 @@ final class ItemStackRequestExecutor{ } $inventoryActions = $this->builder->generateActions(); - return $this->crafting ? - new CraftingTransaction($this->player, $this->player->getServer()->getCraftingManager(), $inventoryActions) : - new InventoryTransaction($this->player, $inventoryActions); + $transaction = $this->specialTransaction ?? new InventoryTransaction($this->player); + foreach($inventoryActions as $action){ + $transaction->addAction($action); + } + + return $transaction; } public function buildItemStackResponse(bool $success) : ItemStackResponse{ diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index e83a75db4..722ed39bf 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -25,6 +25,7 @@ namespace pocketmine\network\mcpe\handler; use pocketmine\inventory\Inventory; use pocketmine\network\mcpe\InventoryManager; +use pocketmine\network\mcpe\protocol\types\inventory\ContainerUIIds; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseContainerInfo; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseSlotInfo; @@ -67,6 +68,9 @@ final class ItemStackResponseBuilder{ public function build(bool $success) : ItemStackResponse{ $responseInfosByContainer = []; foreach($this->changedSlots as $containerInterfaceId => $slotIds){ + if($containerInterfaceId === ContainerUIIds::CREATED_OUTPUT){ + continue; + } foreach($slotIds as $slotId){ [$inventory, $slot] = $this->getInventoryAndSlot($containerInterfaceId, $slotId); From c6e11a845320af4bfa102cc012e0db57bdea2357 Mon Sep 17 00:00:00 2001 From: IvanCraft623 <57236932+IvanCraft623@users.noreply.github.com> Date: Wed, 4 Jan 2023 18:22:31 -0500 Subject: [PATCH 06/32] Remove unnecessary ternary operator (#5493) --- src/network/mcpe/handler/ItemStackResponseBuilder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index 722ed39bf..782404ebe 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -91,7 +91,7 @@ final class ItemStackResponseBuilder{ $slotId, $item->getCount(), $itemStackInfo->getStackId(), - $item->hasCustomName() ? $item->getCustomName() : "", + $item->getCustomName(), 0 ); } From eedc943766ed310bccbf3ee039ff2cce3f501e95 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 5 Jan 2023 17:23:50 +0000 Subject: [PATCH 07/32] Confine legacy transaction handling to dropping items only --- .../mcpe/handler/InGamePacketHandler.php | 100 +++++++----------- 1 file changed, 38 insertions(+), 62 deletions(-) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index c618c2c16..b6b7c68be 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -31,11 +31,11 @@ use pocketmine\entity\animation\ConsumingItemAnimation; use pocketmine\entity\Attribute; use pocketmine\entity\InvalidSkinException; use pocketmine\event\player\PlayerEditBookEvent; -use pocketmine\inventory\transaction\action\InventoryAction; +use pocketmine\inventory\transaction\action\DropItemAction; use pocketmine\inventory\transaction\CraftingTransaction; use pocketmine\inventory\transaction\InventoryTransaction; +use pocketmine\inventory\transaction\TransactionBuilder; use pocketmine\inventory\transaction\TransactionException; -use pocketmine\inventory\transaction\TransactionValidationException; use pocketmine\item\VanillaItems; use pocketmine\item\WritableBook; use pocketmine\item\WritableBookPage; @@ -99,7 +99,6 @@ 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; use pocketmine\network\mcpe\protocol\types\PlayerAction; @@ -378,72 +377,49 @@ class InGamePacketHandler extends PacketHandler{ } private function handleNormalTransaction(NormalTransactionData $data, int $itemStackRequestId) : bool{ - /** @var InventoryAction[] $actions */ - $actions = []; + //When the ItemStackRequest system is used, this transaction type is only used for dropping items by pressing Q. + //I don't know why they don't just use ItemStackRequest for that too, which already supports dropping items by + //clicking them outside an open inventory menu, but for now it is what it is. + //Fortunately, this means we can be extremely strict about the validation criteria. + + if(count($data->getActions()) > 2){ + throw new PacketHandlingException("Expected exactly 2 actions for dropping an item"); + } - $isCraftingPart = false; - $converter = TypeConverter::getInstance(); foreach($data->getActions() as $networkInventoryAction){ - if( - $networkInventoryAction->sourceType === NetworkInventoryAction::SOURCE_TODO || ( - $this->craftingTransaction !== null && - !$networkInventoryAction->oldItem->getItemStack()->equals($networkInventoryAction->newItem->getItemStack()) && - $networkInventoryAction->sourceType === NetworkInventoryAction::SOURCE_CONTAINER && - $networkInventoryAction->windowId === ContainerIds::UI && - $networkInventoryAction->inventorySlot === UIInventorySlotOffset::CREATED_ITEM_OUTPUT - ) - ){ - $isCraftingPart = true; - } + if($networkInventoryAction->sourceType === NetworkInventoryAction::SOURCE_WORLD){ + //drop item - we don't need to validate this, we only care about the count + //if the resulting actions don't match the client for some reason, it will trigger an automatic + //prediction rollback anyway. + //it's technically possible to see this more than once, but a normal client should never do that. + $inventory = $this->player->getInventory(); + $heldItem = $inventory->getItemInHand(); - try{ - $action = $converter->createInventoryAction($networkInventoryAction, $this->player, $this->inventoryManager); - if($action !== null){ - $actions[] = $action; + try{ + $droppedItem = TypeConverter::getInstance()->netItemStackToCore($networkInventoryAction->newItem->getItemStack()); + }catch(TypeConversionException $e){ + throw PacketHandlingException::wrap($e); } - }catch(TypeConversionException $e){ - $this->session->getLogger()->debug("Error unpacking inventory action: " . $e->getMessage()); - return false; + + //TODO: if we can avoid decoding incoming item NBT, it will be faster to compare network ItemStacks + //rather than converting to internal itemstacks and using canStackWith() here. + if(!$heldItem->canStackWith($droppedItem) || $heldItem->getCount() < $droppedItem->getCount()){ + return false; + } + + //purposely overwritten here - this allows any immutable internal references to be shared + $droppedItem = $heldItem->pop($droppedItem->getCount()); + + $builder = new TransactionBuilder(); + $builder->getInventory($inventory)->setItem($inventory->getHeldItemIndex(), $heldItem); + $builder->addAction(new DropItemAction($droppedItem)); + + $transaction = new InventoryTransaction($this->player, $builder->generateActions()); + return $this->executeInventoryTransaction($transaction, $itemStackRequestId); } } - if($isCraftingPart){ - if($this->craftingTransaction === null){ - //TODO: this might not be crafting if there is a special inventory open (anvil, enchanting, loom etc) - $this->craftingTransaction = new CraftingTransaction($this->player, $this->player->getServer()->getCraftingManager(), $actions); - }else{ - foreach($actions as $action){ - $this->craftingTransaction->addAction($action); - } - } - - try{ - $this->craftingTransaction->validate(); - }catch(TransactionValidationException $e){ - //transaction is incomplete - crafting transaction comes in lots of little bits, so we have to collect - //all of the parts before we can execute it - return true; - } - try{ - return $this->executeInventoryTransaction($this->craftingTransaction, $itemStackRequestId); - }finally{ - $this->craftingTransaction = null; - } - }else{ - //normal transaction fallthru - if($this->craftingTransaction !== null){ - $this->session->getLogger()->debug("Got unexpected normal inventory action with incomplete crafting transaction, refusing to execute crafting"); - $this->craftingTransaction = null; - return false; - } - - if(count($actions) === 0){ - //TODO: 1.13+ often sends transactions with nothing but useless crap in them, no need for the debug noise - return true; - } - - return $this->executeInventoryTransaction(new InventoryTransaction($this->player, $actions), $itemStackRequestId); - } + throw new PacketHandlingException("Legacy 'normal' transactions should only be used for dropping items"); } private function handleUseItemTransaction(UseItemTransactionData $data) : bool{ From 30d3869eea998dbe9a1ff6c98fb9782383404a5c Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 5 Jan 2023 17:26:58 +0000 Subject: [PATCH 08/32] Remove dead code --- src/network/mcpe/convert/TypeConverter.php | 66 ---------------------- 1 file changed, 66 deletions(-) diff --git a/src/network/mcpe/convert/TypeConverter.php b/src/network/mcpe/convert/TypeConverter.php index 2c7a3415e..e8826e591 100644 --- a/src/network/mcpe/convert/TypeConverter.php +++ b/src/network/mcpe/convert/TypeConverter.php @@ -24,11 +24,6 @@ declare(strict_types=1); namespace pocketmine\network\mcpe\convert; use pocketmine\block\BlockLegacyIds; -use pocketmine\inventory\transaction\action\CreateItemAction; -use pocketmine\inventory\transaction\action\DestroyItemAction; -use pocketmine\inventory\transaction\action\DropItemAction; -use pocketmine\inventory\transaction\action\InventoryAction; -use pocketmine\inventory\transaction\action\SlotChangeAction; use pocketmine\item\Durable; use pocketmine\item\Item; use pocketmine\item\ItemFactory; @@ -37,17 +32,12 @@ use pocketmine\item\VanillaItems; use pocketmine\nbt\NbtException; use pocketmine\nbt\tag\CompoundTag; use pocketmine\nbt\tag\IntTag; -use pocketmine\network\mcpe\InventoryManager; use pocketmine\network\mcpe\protocol\types\GameMode as ProtocolGameMode; -use pocketmine\network\mcpe\protocol\types\inventory\ContainerIds; use pocketmine\network\mcpe\protocol\types\inventory\ItemStack; -use pocketmine\network\mcpe\protocol\types\inventory\NetworkInventoryAction; -use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset; use pocketmine\network\mcpe\protocol\types\recipe\IntIdMetaItemDescriptor; use pocketmine\network\mcpe\protocol\types\recipe\RecipeIngredient; use pocketmine\network\mcpe\protocol\types\recipe\StringIdMetaItemDescriptor; use pocketmine\player\GameMode; -use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\SingletonTrait; @@ -261,60 +251,4 @@ class TypeConverter{ throw TypeConversionException::wrap($e, "Bad itemstack NBT data"); } } - - /** - * @throws TypeConversionException - */ - public function createInventoryAction(NetworkInventoryAction $action, Player $player, InventoryManager $inventoryManager) : ?InventoryAction{ - if($action->oldItem->getItemStack()->equals($action->newItem->getItemStack())){ - //filter out useless noise in 1.13 - return null; - } - try{ - $old = $this->netItemStackToCore($action->oldItem->getItemStack()); - }catch(TypeConversionException $e){ - throw TypeConversionException::wrap($e, "Inventory action: oldItem"); - } - try{ - $new = $this->netItemStackToCore($action->newItem->getItemStack()); - }catch(TypeConversionException $e){ - throw TypeConversionException::wrap($e, "Inventory action: newItem"); - } - switch($action->sourceType){ - case NetworkInventoryAction::SOURCE_CONTAINER: - if($action->windowId === ContainerIds::UI && $action->inventorySlot === UIInventorySlotOffset::CREATED_ITEM_OUTPUT){ - return null; //useless noise - } - $located = $inventoryManager->locateWindowAndSlot($action->windowId, $action->inventorySlot); - if($located !== null){ - [$window, $slot] = $located; - return new SlotChangeAction($window, $slot, $old, $new); - } - - throw new TypeConversionException("No open container with window ID $action->windowId"); - case NetworkInventoryAction::SOURCE_WORLD: - if($action->inventorySlot !== NetworkInventoryAction::ACTION_MAGIC_SLOT_DROP_ITEM){ - throw new TypeConversionException("Only expecting drop-item world actions from the client!"); - } - - return new DropItemAction($new); - case NetworkInventoryAction::SOURCE_CREATIVE: - switch($action->inventorySlot){ - case NetworkInventoryAction::ACTION_MAGIC_SLOT_CREATIVE_DELETE_ITEM: - return new DestroyItemAction($new); - case NetworkInventoryAction::ACTION_MAGIC_SLOT_CREATIVE_CREATE_ITEM: - return new CreateItemAction($old); - default: - throw new TypeConversionException("Unexpected creative action type $action->inventorySlot"); - - } - case NetworkInventoryAction::SOURCE_TODO: - //These are used to balance a transaction that involves special actions, like crafting, enchanting, etc. - //The vanilla server just accepted these without verifying them. We don't need to care about them since - //we verify crafting by checking for imbalances anyway. - return null; - default: - throw new TypeConversionException("Unknown inventory source type $action->sourceType"); - } - } } From 3d6baa8a55ca3c2ce1870a6e67ab8adf13deb9db Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 5 Jan 2023 18:09:57 +0000 Subject: [PATCH 09/32] Working creative inventory, with a few more hacks than I'd like --- src/network/mcpe/InventoryManager.php | 13 ++-- .../mcpe/handler/ItemStackRequestExecutor.php | 78 ++++++++++++++----- 2 files changed, 67 insertions(+), 24 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index ea978d2cb..d710d1271 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -60,7 +60,6 @@ use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\ObjectSet; -use function array_map; use function array_search; use function get_class; use function is_int; @@ -512,10 +511,14 @@ class InventoryManager{ public function syncCreative() : void{ $typeConverter = TypeConverter::getInstance(); - $nextEntryId = 1; - $this->session->sendDataPacket(CreativeContentPacket::create(array_map(function(Item $item) use($typeConverter, &$nextEntryId) : CreativeContentEntry{ - return new CreativeContentEntry($nextEntryId++, $typeConverter->coreItemStackToNet($item)); - }, $this->player->isSpectator() ? [] : CreativeInventory::getInstance()->getAll()))); + $entries = []; + if(!$this->player->isSpectator()){ + //creative inventory may have holes if items were unregistered - ensure network IDs used are always consistent + foreach(CreativeInventory::getInstance()->getAll() as $k => $item){ + $entries[] = new CreativeContentEntry($k, $typeConverter->coreItemStackToNet($item)); + } + } + $this->session->sendDataPacket(CreativeContentPacket::create($entries)); } private function newItemStackId() : int{ diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index 60edbab29..46e9caf7f 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -24,6 +24,8 @@ declare(strict_types=1); namespace pocketmine\network\mcpe\handler; use pocketmine\crafting\CraftingGrid; +use pocketmine\inventory\CreativeInventory; +use pocketmine\inventory\transaction\action\CreateItemAction; use pocketmine\inventory\transaction\action\DestroyItemAction; use pocketmine\inventory\transaction\action\DropItemAction; use pocketmine\inventory\transaction\CraftingTransaction; @@ -37,6 +39,7 @@ use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingConsum use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingMarkSecondaryResultStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftRecipeAutoStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftRecipeStackRequestAction; +use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CreativeCreateStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\DeprecatedCraftingResultsStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\DestroyStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\DropStackRequestAction; @@ -51,6 +54,7 @@ use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset; use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; +use function array_key_first; use function get_class; final class ItemStackRequestExecutor{ @@ -63,7 +67,10 @@ final class ItemStackRequestExecutor{ /** @var Item[] */ private array $craftingResults = []; - private int $craftingOutputIndex = 0; + + private ?Item $nextCreatedItem = null; + private bool $createdItemFromCreativeInventory = false; + private int $createdItemsTakenCount = 0; public function __construct( private Player $player, @@ -123,7 +130,7 @@ final class ItemStackRequestExecutor{ /** * Adds items to the target slot, if they are stackable. */ - private function addItemToSlot(ItemStackRequestSlotInfo $slotInfo, Item $item, int $count) : Item{ + private function addItemToSlot(ItemStackRequestSlotInfo $slotInfo, Item $item, int $count) : void{ $this->requestSlotInfos[] = $slotInfo; [$inventory, $slot] = $this->getBuilderInventoryAndSlot($slotInfo); @@ -136,11 +143,25 @@ final class ItemStackRequestExecutor{ $newItem = clone $item; $newItem->setCount($existingItem->getCount() + $count); $inventory->setItem($slot, $newItem); + } - $leftover = clone $item; - $leftover->pop($count); - - return $leftover; + private function setNextCreatedItem(?Item $item, bool $creative = false) : void{ + if($item !== null && $item->isNull()){ + $item = null; + } + if($this->nextCreatedItem !== null){ + //while this is more complicated than simply adding the action when the item is taken, this ensures that + //plugins can tell the difference between 1 item that got split into 2 slots, vs 2 separate items. + if($this->createdItemFromCreativeInventory && $this->createdItemsTakenCount > 0){ + $this->nextCreatedItem->setCount($this->createdItemsTakenCount); + $this->builder->addAction(new CreateItemAction($this->nextCreatedItem)); + }elseif($this->createdItemsTakenCount < $this->nextCreatedItem->getCount()){ + throw new PacketHandlingException("Not all of the previous created item was taken"); + } + } + $this->nextCreatedItem = $item; + $this->createdItemFromCreativeInventory = $creative; + $this->createdItemsTakenCount = 0; } private function beginCrafting(int $recipeId, int $repetitions) : void{ @@ -169,20 +190,27 @@ final class ItemStackRequestExecutor{ $craftingResult->setCount($craftingResult->getCount() * $repetitions); $this->craftingResults[$k] = $craftingResult; } + $this->setNextCreatedItem($this->craftingResults[array_key_first($this->craftingResults)]); } - private function takeCraftingResult(ItemStackRequestSlotInfo $destination, int $count) : void{ - $recipeResultItem = $this->craftingResults[$this->craftingOutputIndex] ?? null; - if($recipeResultItem === null){ - throw new PacketHandlingException("Cannot refer to nonexisting crafting output index " . $this->craftingOutputIndex); + private function takeCreatedItem(ItemStackRequestSlotInfo $destination, int $count) : void{ + $createdItem = $this->nextCreatedItem; + if($createdItem === null){ + throw new PacketHandlingException("No created item is waiting to be taken"); } - $availableCount = $recipeResultItem->getCount(); - if($availableCount < $count){ - throw new PacketHandlingException("Tried to take too many results from crafting"); + if(!$this->createdItemFromCreativeInventory){ + $availableCount = $createdItem->getCount() - $this->createdItemsTakenCount; + if($count > $availableCount){ + throw new PacketHandlingException("Not enough created items available to be taken (have $availableCount, tried to take $count)"); + } } - $this->craftingResults[$this->craftingOutputIndex] = $this->addItemToSlot($destination, $recipeResultItem, $count); + $this->createdItemsTakenCount += $count; + $this->addItemToSlot($destination, $createdItem, $count); + if(!$this->createdItemFromCreativeInventory && $this->createdItemsTakenCount >= $createdItem->getCount()){ + $this->setNextCreatedItem(null); + } } private function processItemStackRequestAction(ItemStackRequestAction $action) : void{ @@ -194,7 +222,7 @@ final class ItemStackRequestExecutor{ $destination = $action->getDestination(); if($source->getContainerId() === ContainerUIIds::CREATED_OUTPUT && $source->getSlotId() === UIInventorySlotOffset::CREATED_ITEM_OUTPUT){ - $this->takeCraftingResult($destination, $action->getCount()); + $this->takeCreatedItem($destination, $action->getCount()); }else{ $this->transferItems($source, $destination, $action->getCount()); } @@ -218,6 +246,16 @@ final class ItemStackRequestExecutor{ $destroyed = $this->removeItemFromSlot($action->getSource(), $action->getCount()); $this->builder->addAction(new DestroyItemAction($destroyed)); + }elseif($action instanceof CreativeCreateStackRequestAction){ + $item = CreativeInventory::getInstance()->getItem($action->getCreativeItemId()); + if($item === null){ + //TODO: the item may have been unregistered after the client was sent the creative contents, leaving a + //gap in the creative item list. This probably shouldn't be a violation, but I'm not sure how else to + //handle it right now. + throw new PacketHandlingException("Tried to create nonexisting creative item " . $action->getCreativeItemId()); + } + + $this->setNextCreatedItem($item, true); }elseif($action instanceof CraftRecipeStackRequestAction){ $this->beginCrafting($action->getRecipeId(), 1); }elseif($action instanceof CraftRecipeAutoStackRequestAction){ @@ -232,11 +270,12 @@ final class ItemStackRequestExecutor{ if(!$this->specialTransaction instanceof CraftingTransaction){ throw new AssumptionFailedError("Cannot mark crafting result index when no crafting transaction is in progress"); } - $outputIndex = $action->getCraftingGridSlot(); - if($outputIndex < 0){ - throw new PacketHandlingException("Crafting result index cannot be negative"); + + $nextResultItem = $this->craftingResults[$action->getCraftingGridSlot()] ?? null; + if($nextResultItem === null){ + throw new PacketHandlingException("No such crafting result index " . $action->getCraftingGridSlot()); } - $this->craftingOutputIndex = $outputIndex; + $this->setNextCreatedItem($nextResultItem); }elseif($action instanceof DeprecatedCraftingResultsStackRequestAction){ //no obvious use }else{ @@ -248,6 +287,7 @@ final class ItemStackRequestExecutor{ foreach($this->request->getActions() as $action){ $this->processItemStackRequestAction($action); } + $this->setNextCreatedItem(null); $inventoryActions = $this->builder->generateActions(); $transaction = $this->specialTransaction ?? new InventoryTransaction($this->player); From 36525d905543849379badd051ceac9609bf1872c Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 5 Jan 2023 20:41:44 +0000 Subject: [PATCH 10/32] Fixed multi-output recipe handling --- src/network/mcpe/handler/ItemStackRequestExecutor.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index 46e9caf7f..ce9fe2d9a 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -190,7 +190,10 @@ final class ItemStackRequestExecutor{ $craftingResult->setCount($craftingResult->getCount() * $repetitions); $this->craftingResults[$k] = $craftingResult; } - $this->setNextCreatedItem($this->craftingResults[array_key_first($this->craftingResults)]); + if(count($this->craftingResults) === 1){ + //for multi-output recipes, later actions will tell us which result to create and when + $this->setNextCreatedItem($this->craftingResults[array_key_first($this->craftingResults)]); + } } private function takeCreatedItem(ItemStackRequestSlotInfo $destination, int $count) : void{ From b24eb153f9d3478b4159d6e904971f9e6c267118 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 5 Jan 2023 21:18:30 +0000 Subject: [PATCH 11/32] Constrain inventory transaction predictions these are now only used for actions done with a closed inventory window. This means that they can only predict the slots of inventory, offhand and armor (total 41 slots) and perhaps include some DropItem actions. --- src/network/mcpe/InventoryManager.php | 9 +++++++++ src/network/mcpe/handler/InGamePacketHandler.php | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index d710d1271..b9cb99262 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -210,6 +210,15 @@ class InventoryManager{ if($action->sourceType !== NetworkInventoryAction::SOURCE_CONTAINER){ continue; } + + //legacy transactions should not modify or predict anything other than these inventories, since these are + //the only ones accessible when not in-game (ItemStackRequest is used for everything else) + if(match($action->windowId){ + ContainerIds::INVENTORY, ContainerIds::OFFHAND, ContainerIds::ARMOR => false, + default => true + }){ + throw new PacketHandlingException("Legacy transactions cannot predict changes to inventory with ID " . $action->windowId); + } $info = $this->locateWindowAndSlot($action->windowId, $action->inventorySlot); if($info === null){ continue; diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index b6b7c68be..80a0f2015 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -328,7 +328,7 @@ class InGamePacketHandler extends PacketHandler{ public function handleInventoryTransaction(InventoryTransactionPacket $packet) : bool{ $result = true; - if(count($packet->trData->getActions()) > 100){ + if(count($packet->trData->getActions()) > 50){ throw new PacketHandlingException("Too many actions in inventory transaction"); } From d3cea2ca7cc16561df483981cab8e86c59b98ee5 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 6 Jan 2023 02:07:31 +0000 Subject: [PATCH 12/32] CS --- src/network/mcpe/handler/ItemStackRequestExecutor.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index ce9fe2d9a..37ba047f1 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -55,6 +55,7 @@ use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; use function array_key_first; +use function count; use function get_class; final class ItemStackRequestExecutor{ From 8633804f156817eb4db9f44ed2125e44ded01667 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 6 Jan 2023 20:26:19 +0000 Subject: [PATCH 13/32] InventoryManager: disentangle slot tracking from slot syncing --- src/inventory/BaseInventory.php | 2 +- src/network/mcpe/InventoryManager.php | 117 ++++++++++++++------------ 2 files changed, 62 insertions(+), 57 deletions(-) diff --git a/src/inventory/BaseInventory.php b/src/inventory/BaseInventory.php index 06b3b9800..daf41c579 100644 --- a/src/inventory/BaseInventory.php +++ b/src/inventory/BaseInventory.php @@ -348,7 +348,7 @@ abstract class BaseInventory implements Inventory{ if($invManager === null){ continue; } - $invManager->syncSlot($this, $index); + $invManager->onSlotChange($this, $index); } } diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index b9cb99262..cdc588180 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -382,51 +382,63 @@ class InventoryManager{ } } + public function onSlotChange(Inventory $inventory, int $slot) : void{ + $currentItem = $inventory->getItem($slot); + $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; + $clientSideItem = $predictions?->getSlot($slot); + if($clientSideItem === null || !$clientSideItem->equalsExact($currentItem)){ + //no prediction or incorrect - do not associate this with the currently active itemstack request + $this->trackItemStack($inventory, $slot, $currentItem, null); + $this->syncSlot($inventory, $slot); + }else{ + //correctly predicted - associate the change with the currently active itemstack request + $this->trackItemStack($inventory, $slot, $currentItem, $this->currentItemStackRequestId); + } + $predictions?->remove($slot); + } + public function syncSlot(Inventory $inventory, int $slot) : void{ + $itemStackInfo = $this->getItemStackInfo($inventory, $slot); + if($itemStackInfo === null){ + throw new \LogicException("Cannot sync an untracked inventory slot"); + } $slotMap = $this->complexWindows[spl_object_id($inventory)] ?? null; if($slotMap !== null){ $windowId = ContainerIds::UI; - $netSlot = $slotMap->mapCoreToNet($slot) ?? null; + $netSlot = $slotMap->mapCoreToNet($slot) ?? throw new AssumptionFailedError("We already have an ItemStackInfo, so this should not be null"); }else{ - $windowId = $this->getWindowId($inventory); + $windowId = $this->getWindowId($inventory) ?? throw new AssumptionFailedError("We already have an ItemStackInfo, so this should not be null"); $netSlot = $slot; } - if($windowId !== null && $netSlot !== null){ - $currentItem = $inventory->getItem($slot); - $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; - $clientSideItem = $predictions?->getSlot($slot); - if($clientSideItem === null || !$clientSideItem->equalsExact($currentItem)){ - $itemStackWrapper = $this->wrapItemStack($inventory, $slot, $currentItem); - if($windowId === ContainerIds::OFFHAND){ - //TODO: HACK! - //The client may sometimes ignore the InventorySlotPacket for the offhand slot. - //This can cause a lot of problems (totems, arrows, and more...). - //The workaround is to send an InventoryContentPacket instead - //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); + + $itemStackWrapper = new ItemStackWrapper($itemStackInfo->getStackId(), $itemStackInfo->getItemStack()); + if($windowId === ContainerIds::OFFHAND){ + //TODO: HACK! + //The client may sometimes ignore the InventorySlotPacket for the offhand slot. + //This can cause a lot of problems (totems, arrows, and more...). + //The workaround is to send an InventoryContentPacket instead + //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()))); } - $predictions?->remove($slot); + $this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, $itemStackWrapper)); } + $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; + $predictions?->remove($slot); } public function syncContents(Inventory $inventory) : void{ @@ -437,26 +449,25 @@ class InventoryManager{ $windowId = $this->getWindowId($inventory); } if($windowId !== null){ + unset($this->initiatedSlotChanges[spl_object_id($inventory)]); + $contents = []; + foreach($inventory->getContents(true) as $slot => $item){ + $info = $this->trackItemStack($inventory, $slot, $item, null); + $contents[] = new ItemStackWrapper($info->getStackId(), $info->getItemStack()); + } if($slotMap !== null){ - $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; - foreach($inventory->getContents(true) as $slotId => $item){ + foreach($contents as $slotId => $info){ $packetSlot = $slotMap->mapCoreToNet($slotId) ?? null; if($packetSlot === null){ continue; } - $predictions?->remove($slotId); $this->session->sendDataPacket(InventorySlotPacket::create( $windowId, $packetSlot, - $this->wrapItemStack($inventory, $slotId, $inventory->getItem($slotId)) + $info )); } }else{ - unset($this->initiatedSlotChanges[spl_object_id($inventory)]); - $contents = []; - foreach($inventory->getContents(true) as $slotId => $item){ - $contents[] = $this->wrapItemStack($inventory, $slotId, $item); - } $this->session->sendDataPacket(InventoryContentPacket::create($windowId, $contents)); } } @@ -475,14 +486,13 @@ class InventoryManager{ foreach($this->initiatedSlotChanges as $predictions){ $inventory = $predictions->getInventory(); foreach($predictions->getSlots() as $slot => $expectedItem){ - if(!$inventory->slotExists($slot)){ + if(!$inventory->slotExists($slot) || $this->getItemStackInfo($inventory, $slot) === null){ continue; //TODO: size desync ??? } - $actualItem = $inventory->getItem($slot); - if(!$actualItem->equalsExact($expectedItem)){ - $this->session->getLogger()->debug("Detected prediction mismatch in inventory " . get_class($inventory) . "#" . spl_object_id($inventory) . " slot $slot"); - $this->syncSlot($inventory, $slot); - } + + //any prediction that still exists at this point is a slot that was predicted to change but didn't + $this->session->getLogger()->debug("Detected prediction mismatch in inventory " . get_class($inventory) . "#" . spl_object_id($inventory) . " slot $slot"); + $this->syncSlot($inventory, $slot); } } @@ -550,11 +560,6 @@ class InventoryManager{ return $this->itemStackInfos[spl_object_id($inventory)][$slotId] = $info; } - private function wrapItemStack(Inventory $inventory, int $slotId, Item $item) : ItemStackWrapper{ - $info = $this->trackItemStack($inventory, $slotId, $item, null); - return new ItemStackWrapper($info->getStackId(), $info->getItemStack()); - } - public function matchItemStack(Inventory $inventory, int $slotId, int $clientItemStackId) : bool{ $inventoryObjectId = spl_object_id($inventory); if(!isset($this->itemStackInfos[$inventoryObjectId])){ From 1123a5aa23f8e9ab925ca0dad13aa94337f5a651 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 6 Jan 2023 20:45:08 +0000 Subject: [PATCH 14/32] InventoryManager: Track predictions using ItemStack directly, instead of internal Item this removes the need for deserializing network itemstacks to core items, thereby eliminating a whole bunch of potential security issues. --- src/network/mcpe/InventoryManager.php | 28 ++++++++----------- .../mcpe/InventoryManagerPredictedChanges.php | 14 +++++----- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index cdc588180..a0a28a3a2 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -38,7 +38,6 @@ use pocketmine\inventory\Inventory; use pocketmine\inventory\transaction\action\SlotChangeAction; use pocketmine\inventory\transaction\InventoryTransaction; use pocketmine\item\Item; -use pocketmine\network\mcpe\convert\TypeConversionException; use pocketmine\network\mcpe\convert\TypeConverter; use pocketmine\network\mcpe\protocol\ClientboundPacket; use pocketmine\network\mcpe\protocol\ContainerClosePacket; @@ -188,7 +187,7 @@ class InventoryManager{ return null; } - private function addPredictedSlotChange(Inventory $inventory, int $slot, Item $item) : void{ + private function addPredictedSlotChange(Inventory $inventory, int $slot, ItemStack $item) : void{ $predictions = ($this->initiatedSlotChanges[spl_object_id($inventory)] ??= new InventoryManagerPredictedChanges($inventory)); $predictions->add($slot, $item); } @@ -196,7 +195,9 @@ class InventoryManager{ public function addTransactionPredictedSlotChanges(InventoryTransaction $tx) : void{ foreach($tx->getActions() as $action){ if($action instanceof SlotChangeAction){ - $this->addPredictedSlotChange($action->getInventory(), $action->getSlot(), $action->getTargetItem()); + //TODO: ItemStackRequestExecutor can probably build these predictions with much lower overhead + $itemStack = TypeConverter::getInstance()->coreItemStackToNet($action->getTargetItem()); + $this->addPredictedSlotChange($action->getInventory(), $action->getSlot(), $itemStack); } } } @@ -225,12 +226,7 @@ class InventoryManager{ } [$inventory, $slot] = $info; - try{ - $item = TypeConverter::getInstance()->netItemStackToCore($action->newItem->getItemStack()); - }catch(TypeConversionException $e){ - throw new PacketHandlingException($e->getMessage(), 0, $e); - } - $this->addPredictedSlotChange($inventory, $slot, $item); + $this->addPredictedSlotChange($inventory, $slot, $action->newItem->getItemStack()); } } @@ -383,10 +379,10 @@ class InventoryManager{ } public function onSlotChange(Inventory $inventory, int $slot) : void{ - $currentItem = $inventory->getItem($slot); + $currentItem = TypeConverter::getInstance()->coreItemStackToNet($inventory->getItem($slot)); $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; $clientSideItem = $predictions?->getSlot($slot); - if($clientSideItem === null || !$clientSideItem->equalsExact($currentItem)){ + if($clientSideItem === null || !$clientSideItem->equals($currentItem)){ //no prediction or incorrect - do not associate this with the currently active itemstack request $this->trackItemStack($inventory, $slot, $currentItem, null); $this->syncSlot($inventory, $slot); @@ -452,7 +448,8 @@ class InventoryManager{ unset($this->initiatedSlotChanges[spl_object_id($inventory)]); $contents = []; foreach($inventory->getContents(true) as $slot => $item){ - $info = $this->trackItemStack($inventory, $slot, $item, null); + $itemStack = TypeConverter::getInstance()->coreItemStackToNet($item); + $info = $this->trackItemStack($inventory, $slot, $itemStack, null); $contents[] = new ItemStackWrapper($info->getStackId(), $info->getItemStack()); } if($slotMap !== null){ @@ -548,15 +545,14 @@ class InventoryManager{ return $this->itemStackInfos[spl_object_id($inventory)][$slot] ?? null; } - private function trackItemStack(Inventory $inventory, int $slotId, Item $item, ?int $itemStackRequestId) : ItemStackInfo{ + private function trackItemStack(Inventory $inventory, int $slotId, ItemStack $itemStack, ?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) && $existing->getRequestId() === $itemStackRequestId){ return $existing; } - $info = new ItemStackInfo($itemStackRequestId, $item->isNull() ? 0 : $this->newItemStackId(), $itemStack); + //TODO: ItemStack->isNull() would be nice to have here + $info = new ItemStackInfo($itemStackRequestId, $itemStack->getId() === 0 ? 0 : $this->newItemStackId(), $itemStack); return $this->itemStackInfos[spl_object_id($inventory)][$slotId] = $info; } diff --git a/src/network/mcpe/InventoryManagerPredictedChanges.php b/src/network/mcpe/InventoryManagerPredictedChanges.php index a381234a4..8264e83fb 100644 --- a/src/network/mcpe/InventoryManagerPredictedChanges.php +++ b/src/network/mcpe/InventoryManagerPredictedChanges.php @@ -24,12 +24,12 @@ declare(strict_types=1); namespace pocketmine\network\mcpe; use pocketmine\inventory\Inventory; -use pocketmine\item\Item; +use pocketmine\network\mcpe\protocol\types\inventory\ItemStack; final class InventoryManagerPredictedChanges{ /** - * @var Item[] - * @phpstan-var array + * @var ItemStack[] + * @phpstan-var array */ private array $slots = []; @@ -40,18 +40,18 @@ final class InventoryManagerPredictedChanges{ public function getInventory() : Inventory{ return $this->inventory; } /** - * @return Item[] - * @phpstan-return array + * @return ItemStack[] + * @phpstan-return array */ public function getSlots() : array{ return $this->slots; } - public function getSlot(int $slot) : ?Item{ + public function getSlot(int $slot) : ?ItemStack{ return $this->slots[$slot] ?? null; } - public function add(int $slot, Item $item) : void{ + public function add(int $slot, ItemStack $item) : void{ $this->slots[$slot] = $item; } From faaec12aaf56e42672287913bfb41d51a86a6a4e Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 6 Jan 2023 22:16:29 +0000 Subject: [PATCH 15/32] Update BedrockProtocol --- composer.json | 2 +- composer.lock | 117 ++++++++++-------- .../mcpe/handler/ItemStackRequestExecutor.php | 8 +- 3 files changed, 69 insertions(+), 58 deletions(-) diff --git a/composer.json b/composer.json index 68a22e7f1..183f0e1f7 100644 --- a/composer.json +++ b/composer.json @@ -35,7 +35,7 @@ "fgrosse/phpasn1": "^2.3", "netresearch/jsonmapper": "^4.0", "pocketmine/bedrock-data": "~1.13.0+bedrock-1.19.50", - "pocketmine/bedrock-protocol": "~17.1.0+bedrock-1.19.50", + "pocketmine/bedrock-protocol": "~18.0.0+bedrock-1.19.50", "pocketmine/binaryutils": "^0.2.1", "pocketmine/callback-validator": "^1.0.2", "pocketmine/classloader": "^0.2.0", diff --git a/composer.lock b/composer.lock index a2f6c207e..41db16407 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "76c6b5521d8f88d9070e8dec1c0ae144", + "content-hash": "9c80c749cc8f464806f8164d6ec670e0", "packages": [ { "name": "adhocore/json-comment", @@ -276,16 +276,16 @@ }, { "name": "pocketmine/bedrock-protocol", - "version": "17.1.0+bedrock-1.19.50", + "version": "18.0.0+bedrock-1.19.50", "source": { "type": "git", "url": "https://github.com/pmmp/BedrockProtocol.git", - "reference": "c572706cf5e3202718dd35a35dd30fe08cd671de" + "reference": "b558ec883bd967dd3339f513cba62d2fbcd63349" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/c572706cf5e3202718dd35a35dd30fe08cd671de", - "reference": "c572706cf5e3202718dd35a35dd30fe08cd671de", + "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/b558ec883bd967dd3339f513cba62d2fbcd63349", + "reference": "b558ec883bd967dd3339f513cba62d2fbcd63349", "shasum": "" }, "require": { @@ -293,13 +293,13 @@ "netresearch/jsonmapper": "^4.0", "php": "^8.0", "pocketmine/binaryutils": "^0.2.0", - "pocketmine/color": "^0.2.0", + "pocketmine/color": "^0.2.0 || ^0.3.0", "pocketmine/math": "^0.3.0 || ^0.4.0", "pocketmine/nbt": "^0.3.0", "ramsey/uuid": "^4.1" }, "require-dev": { - "phpstan/phpstan": "1.9.3", + "phpstan/phpstan": "1.9.4", "phpstan/phpstan-phpunit": "^1.0.0", "phpstan/phpstan-strict-rules": "^1.0.0", "phpunit/phpunit": "^9.5" @@ -317,9 +317,9 @@ "description": "An implementation of the Minecraft: Bedrock Edition protocol in PHP", "support": { "issues": "https://github.com/pmmp/BedrockProtocol/issues", - "source": "https://github.com/pmmp/BedrockProtocol/tree/17.1.0+bedrock-1.19.50" + "source": "https://github.com/pmmp/BedrockProtocol/tree/18.0.0+bedrock-1.19.50" }, - "time": "2022-12-15T20:34:49+00:00" + "time": "2023-01-06T21:47:35+00:00" }, { "name": "pocketmine/binaryutils", @@ -852,42 +852,53 @@ }, { "name": "ramsey/collection", - "version": "1.2.2", + "version": "1.3.0", "source": { "type": "git", "url": "https://github.com/ramsey/collection.git", - "reference": "cccc74ee5e328031b15640b51056ee8d3bb66c0a" + "reference": "ad7475d1c9e70b190ecffc58f2d989416af339b4" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/ramsey/collection/zipball/cccc74ee5e328031b15640b51056ee8d3bb66c0a", - "reference": "cccc74ee5e328031b15640b51056ee8d3bb66c0a", + "url": "https://api.github.com/repos/ramsey/collection/zipball/ad7475d1c9e70b190ecffc58f2d989416af339b4", + "reference": "ad7475d1c9e70b190ecffc58f2d989416af339b4", "shasum": "" }, "require": { - "php": "^7.3 || ^8", + "php": "^7.4 || ^8.0", "symfony/polyfill-php81": "^1.23" }, "require-dev": { - "captainhook/captainhook": "^5.3", - "dealerdirect/phpcodesniffer-composer-installer": "^0.7.0", - "ergebnis/composer-normalize": "^2.6", - "fakerphp/faker": "^1.5", - "hamcrest/hamcrest-php": "^2", - "jangregor/phpstan-prophecy": "^0.8", - "mockery/mockery": "^1.3", + "captainhook/plugin-composer": "^5.3", + "ergebnis/composer-normalize": "^2.28.3", + "fakerphp/faker": "^1.21", + "hamcrest/hamcrest-php": "^2.0", + "jangregor/phpstan-prophecy": "^1.0", + "mockery/mockery": "^1.5", + "php-parallel-lint/php-console-highlighter": "^1.0", + "php-parallel-lint/php-parallel-lint": "^1.3", + "phpcsstandards/phpcsutils": "^1.0.0-rc1", "phpspec/prophecy-phpunit": "^2.0", - "phpstan/extension-installer": "^1", - "phpstan/phpstan": "^0.12.32", - "phpstan/phpstan-mockery": "^0.12.5", - "phpstan/phpstan-phpunit": "^0.12.11", - "phpunit/phpunit": "^8.5 || ^9", - "psy/psysh": "^0.10.4", - "slevomat/coding-standard": "^6.3", - "squizlabs/php_codesniffer": "^3.5", - "vimeo/psalm": "^4.4" + "phpstan/extension-installer": "^1.2", + "phpstan/phpstan": "^1.9", + "phpstan/phpstan-mockery": "^1.1", + "phpstan/phpstan-phpunit": "^1.3", + "phpunit/phpunit": "^9.5", + "psalm/plugin-mockery": "^1.1", + "psalm/plugin-phpunit": "^0.18.4", + "ramsey/coding-standard": "^2.0.3", + "ramsey/conventional-commits": "^1.3", + "vimeo/psalm": "^5.4" }, "type": "library", + "extra": { + "captainhook": { + "force-install": true + }, + "ramsey/conventional-commits": { + "configFile": "conventional-commits.json" + } + }, "autoload": { "psr-4": { "Ramsey\\Collection\\": "src/" @@ -915,7 +926,7 @@ ], "support": { "issues": "https://github.com/ramsey/collection/issues", - "source": "https://github.com/ramsey/collection/tree/1.2.2" + "source": "https://github.com/ramsey/collection/tree/1.3.0" }, "funding": [ { @@ -927,27 +938,27 @@ "type": "tidelift" } ], - "time": "2021-10-10T03:01:02+00:00" + "time": "2022-12-27T19:12:24+00:00" }, { "name": "ramsey/uuid", - "version": "4.6.0", + "version": "4.7.1", "source": { "type": "git", "url": "https://github.com/ramsey/uuid.git", - "reference": "ad63bc700e7d021039e30ce464eba384c4a1d40f" + "reference": "a1acf96007170234a8399586a6e2ab8feba109d1" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/ramsey/uuid/zipball/ad63bc700e7d021039e30ce464eba384c4a1d40f", - "reference": "ad63bc700e7d021039e30ce464eba384c4a1d40f", + "url": "https://api.github.com/repos/ramsey/uuid/zipball/a1acf96007170234a8399586a6e2ab8feba109d1", + "reference": "a1acf96007170234a8399586a6e2ab8feba109d1", "shasum": "" }, "require": { "brick/math": "^0.8.8 || ^0.9 || ^0.10", "ext-json": "*", "php": "^8.0", - "ramsey/collection": "^1.0" + "ramsey/collection": "^1.2 || ^2.0" }, "replace": { "rhumsaa/uuid": "self.version" @@ -1007,7 +1018,7 @@ ], "support": { "issues": "https://github.com/ramsey/uuid/issues", - "source": "https://github.com/ramsey/uuid/tree/4.6.0" + "source": "https://github.com/ramsey/uuid/tree/4.7.1" }, "funding": [ { @@ -1019,7 +1030,7 @@ "type": "tidelift" } ], - "time": "2022-11-05T23:03:38+00:00" + "time": "2022-12-31T22:20:34+00:00" }, { "name": "symfony/filesystem", @@ -1525,30 +1536,30 @@ "packages-dev": [ { "name": "doctrine/instantiator", - "version": "1.4.1", + "version": "1.5.0", "source": { "type": "git", "url": "https://github.com/doctrine/instantiator.git", - "reference": "10dcfce151b967d20fde1b34ae6640712c3891bc" + "reference": "0a0fa9780f5d4e507415a065172d26a98d02047b" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/doctrine/instantiator/zipball/10dcfce151b967d20fde1b34ae6640712c3891bc", - "reference": "10dcfce151b967d20fde1b34ae6640712c3891bc", + "url": "https://api.github.com/repos/doctrine/instantiator/zipball/0a0fa9780f5d4e507415a065172d26a98d02047b", + "reference": "0a0fa9780f5d4e507415a065172d26a98d02047b", "shasum": "" }, "require": { "php": "^7.1 || ^8.0" }, "require-dev": { - "doctrine/coding-standard": "^9", + "doctrine/coding-standard": "^9 || ^11", "ext-pdo": "*", "ext-phar": "*", "phpbench/phpbench": "^0.16 || ^1", "phpstan/phpstan": "^1.4", "phpstan/phpstan-phpunit": "^1", "phpunit/phpunit": "^7.5 || ^8.5 || ^9.5", - "vimeo/psalm": "^4.22" + "vimeo/psalm": "^4.30 || ^5.4" }, "type": "library", "autoload": { @@ -1575,7 +1586,7 @@ ], "support": { "issues": "https://github.com/doctrine/instantiator/issues", - "source": "https://github.com/doctrine/instantiator/tree/1.4.1" + "source": "https://github.com/doctrine/instantiator/tree/1.5.0" }, "funding": [ { @@ -1591,7 +1602,7 @@ "type": "tidelift" } ], - "time": "2022-03-03T08:28:38+00:00" + "time": "2022-12-30T00:15:36+00:00" }, { "name": "myclabs/deep-copy", @@ -1980,16 +1991,16 @@ }, { "name": "phpunit/php-code-coverage", - "version": "9.2.22", + "version": "9.2.23", "source": { "type": "git", "url": "https://github.com/sebastianbergmann/php-code-coverage.git", - "reference": "e4bf60d2220b4baaa0572986b5d69870226b06df" + "reference": "9f1f0f9a2fbb680b26d1cf9b61b6eac43a6e4e9c" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sebastianbergmann/php-code-coverage/zipball/e4bf60d2220b4baaa0572986b5d69870226b06df", - "reference": "e4bf60d2220b4baaa0572986b5d69870226b06df", + "url": "https://api.github.com/repos/sebastianbergmann/php-code-coverage/zipball/9f1f0f9a2fbb680b26d1cf9b61b6eac43a6e4e9c", + "reference": "9f1f0f9a2fbb680b26d1cf9b61b6eac43a6e4e9c", "shasum": "" }, "require": { @@ -2045,7 +2056,7 @@ ], "support": { "issues": "https://github.com/sebastianbergmann/php-code-coverage/issues", - "source": "https://github.com/sebastianbergmann/php-code-coverage/tree/9.2.22" + "source": "https://github.com/sebastianbergmann/php-code-coverage/tree/9.2.23" }, "funding": [ { @@ -2053,7 +2064,7 @@ "type": "github" } ], - "time": "2022-12-18T16:40:55+00:00" + "time": "2022-12-28T12:41:10+00:00" }, { "name": "phpunit/php-file-iterator", diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index 37ba047f1..be7e472ac 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -36,7 +36,7 @@ use pocketmine\item\Item; use pocketmine\network\mcpe\InventoryManager; use pocketmine\network\mcpe\protocol\types\inventory\ContainerUIIds; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingConsumeInputStackRequestAction; -use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingMarkSecondaryResultStackRequestAction; +use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftingCreateSpecificResultStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftRecipeAutoStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CraftRecipeStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\CreativeCreateStackRequestAction; @@ -270,14 +270,14 @@ final class ItemStackRequestExecutor{ } $this->removeItemFromSlot($action->getSource(), $action->getCount()); //output discarded - we allow CraftingTransaction to verify the balance - }elseif($action instanceof CraftingMarkSecondaryResultStackRequestAction){ + }elseif($action instanceof CraftingCreateSpecificResultStackRequestAction){ if(!$this->specialTransaction instanceof CraftingTransaction){ throw new AssumptionFailedError("Cannot mark crafting result index when no crafting transaction is in progress"); } - $nextResultItem = $this->craftingResults[$action->getCraftingGridSlot()] ?? null; + $nextResultItem = $this->craftingResults[$action->getResultIndex()] ?? null; if($nextResultItem === null){ - throw new PacketHandlingException("No such crafting result index " . $action->getCraftingGridSlot()); + throw new PacketHandlingException("No such crafting result index " . $action->getResultIndex()); } $this->setNextCreatedItem($nextResultItem); }elseif($action instanceof DeprecatedCraftingResultsStackRequestAction){ From 34ced382db077cc23bd01a0feade7421917204c2 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 14 Mar 2023 22:56:11 +0000 Subject: [PATCH 16/32] Eliminate final remaining usage of TypeConverter::netItemStackToCore() instead, we can verify that the held items match by comparing the received ItemStack with the one cached in InventoryManager, which is more cost effective and closes off internal item deserializers to external attacks. --- .../mcpe/handler/InGamePacketHandler.php | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 1fd0f584c..dbf2aa5ab 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -46,7 +46,6 @@ use pocketmine\math\Vector3; use pocketmine\nbt\tag\CompoundTag; use pocketmine\nbt\tag\StringTag; use pocketmine\network\mcpe\convert\SkinAdapterSingleton; -use pocketmine\network\mcpe\convert\TypeConversionException; use pocketmine\network\mcpe\convert\TypeConverter; use pocketmine\network\mcpe\InventoryManager; use pocketmine\network\mcpe\NetworkSession; @@ -392,25 +391,23 @@ class InGamePacketHandler extends PacketHandler{ //prediction rollback anyway. //it's technically possible to see this more than once, but a normal client should never do that. $inventory = $this->player->getInventory(); - $heldItem = $inventory->getItemInHand(); - try{ - $droppedItem = TypeConverter::getInstance()->netItemStackToCore($networkInventoryAction->newItem->getItemStack()); - }catch(TypeConversionException $e){ - throw PacketHandlingException::wrap($e); + $heldItemStack = $this->inventoryManager->getItemStackInfo($inventory, $inventory->getHeldItemIndex())?->getItemStack(); + if($heldItemStack === null){ + throw new AssumptionFailedError("Missing itemstack info for held item"); } - - //TODO: if we can avoid decoding incoming item NBT, it will be faster to compare network ItemStacks - //rather than converting to internal itemstacks and using canStackWith() here. - if(!$heldItem->canStackWith($droppedItem) || $heldItem->getCount() < $droppedItem->getCount()){ + $droppedItemStack = $networkInventoryAction->newItem->getItemStack(); + //because the client doesn't tell us the expected itemstack ID, we have to deep-compare our known + //itemstack info with the one the client sent. This is costly, but we don't have any other option :( + if(!$heldItemStack->equalsWithoutCount($droppedItemStack) || $heldItemStack->getCount() < $droppedItemStack->getCount()){ return false; } - //purposely overwritten here - this allows any immutable internal references to be shared - $droppedItem = $heldItem->pop($droppedItem->getCount()); + $newHeldItem = $inventory->getItemInHand(); + $droppedItem = $newHeldItem->pop($droppedItemStack->getCount()); $builder = new TransactionBuilder(); - $builder->getInventory($inventory)->setItem($inventory->getHeldItemIndex(), $heldItem); + $builder->getInventory($inventory)->setItem($inventory->getHeldItemIndex(), $newHeldItem); $builder->addAction(new DropItemAction($droppedItem)); $transaction = new InventoryTransaction($this->player, $builder->generateActions()); From 4864444440d993c1bb3c1a17ed2ac0163fb5357e Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 19 Mar 2023 22:14:23 +0000 Subject: [PATCH 17/32] Added CraftingManager::getCraftingRecipeFromIndex() --- src/crafting/CraftingManager.php | 4 ++++ src/network/mcpe/handler/ItemStackRequestExecutor.php | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/crafting/CraftingManager.php b/src/crafting/CraftingManager.php index d1421c837..3b2bd173a 100644 --- a/src/crafting/CraftingManager.php +++ b/src/crafting/CraftingManager.php @@ -167,6 +167,10 @@ class CraftingManager{ return $this->craftingRecipeIndex; } + public function getCraftingRecipeFromIndex(int $index) : ?CraftingRecipe{ + return $this->craftingRecipeIndex[$index] ?? null; + } + public function getFurnaceRecipeManager(FurnaceType $furnaceType) : FurnaceRecipeManager{ return $this->furnaceRecipeManagers[$furnaceType->id()]; } diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index be7e472ac..b904f279d 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -173,7 +173,7 @@ final class ItemStackRequestExecutor{ throw new PacketHandlingException("Cannot craft a recipe less than 1 time"); } $craftingManager = $this->player->getServer()->getCraftingManager(); - $recipe = $craftingManager->getCraftingRecipeIndex()[$recipeId] ?? null; + $recipe = $craftingManager->getCraftingRecipeFromIndex($recipeId); if($recipe === null){ throw new PacketHandlingException("Unknown crafting recipe ID $recipeId"); } From 7b0816e42fd61d7a1ec580920ac35a98831ccdf5 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 00:52:26 +0000 Subject: [PATCH 18/32] Properly handle transaction building errors instead of kicking the player --- .../TransactionBuilderInventory.php | 4 + src/network/mcpe/InventoryManager.php | 23 ---- .../mcpe/handler/InGamePacketHandler.php | 13 ++- .../mcpe/handler/ItemStackRequestExecutor.php | 108 +++++++++++++----- .../mcpe/handler/ItemStackResponseBuilder.php | 19 +-- 5 files changed, 104 insertions(+), 63 deletions(-) diff --git a/src/inventory/transaction/TransactionBuilderInventory.php b/src/inventory/transaction/TransactionBuilderInventory.php index 4995284a0..95b6c4a14 100644 --- a/src/inventory/transaction/TransactionBuilderInventory.php +++ b/src/inventory/transaction/TransactionBuilderInventory.php @@ -50,6 +50,10 @@ final class TransactionBuilderInventory extends BaseInventory{ $this->changedSlots = new \SplFixedArray($this->actualInventory->getSize()); } + public function getActualInventory() : Inventory{ + return $this->actualInventory; + } + protected function internalSetContents(array $items) : void{ for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ if(!isset($items[$i])){ diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index a0a28a3a2..1ea14b33c 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -555,27 +555,4 @@ class InventoryManager{ $info = new ItemStackInfo($itemStackRequestId, $itemStack->getId() === 0 ? 0 : $this->newItemStackId(), $itemStack); return $this->itemStackInfos[spl_object_id($inventory)][$slotId] = $info; } - - 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); - return false; - } - $info = $this->itemStackInfos[$inventoryObjectId][$slotId] ?? null; - if($info === null){ - $this->session->getLogger()->debug("Attempted to match item preimage for unsynced slot $slotId in " . get_class($inventory) . "#$inventoryObjectId that isn't synced"); - return false; - } - - if(!($clientItemStackId < 0 ? $info->getRequestId() === $clientItemStackId : $info->getStackId() === $clientItemStackId)){ - $this->session->getLogger()->debug( - "Mismatched expected itemstack: " . get_class($inventory) . "#" . $inventoryObjectId . ", " . - "slot: $slotId, client expected: $clientItemStackId, server actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none") - ); - return false; - } - - return true; - } } diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index dbf2aa5ab..70bb827a2 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -110,11 +110,13 @@ use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\Limits; use pocketmine\utils\TextFormat; +use pocketmine\utils\Utils; use pocketmine\world\format\Chunk; use function array_push; use function base64_encode; use function count; use function fmod; +use function implode; use function in_array; use function is_bool; use function is_infinite; @@ -531,9 +533,14 @@ class InGamePacketHandler extends PacketHandler{ 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")); + try{ + $transaction = $executor->generateInventoryTransaction(); + $result = $this->executeInventoryTransaction($transaction, $request->getRequestId()); + }catch(ItemStackRequestProcessException $e){ + $result = false; + $this->session->getLogger()->debug("ItemStackRequest #" . $request->getRequestId() . " failed: " . $e->getMessage()); + $this->session->getLogger()->debug(implode("\n", Utils::printableExceptionInfo($e))); + } return $executor->buildItemStackResponse($result); } diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index b904f279d..c2ae8e305 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -25,6 +25,7 @@ namespace pocketmine\network\mcpe\handler; use pocketmine\crafting\CraftingGrid; use pocketmine\inventory\CreativeInventory; +use pocketmine\inventory\Inventory; use pocketmine\inventory\transaction\action\CreateItemAction; use pocketmine\inventory\transaction\action\DestroyItemAction; use pocketmine\inventory\transaction\action\DropItemAction; @@ -81,25 +82,49 @@ final class ItemStackRequestExecutor{ $this->builder = new TransactionBuilder(); } + private function prettyInventoryAndSlot(Inventory $inventory, int $slot) : string{ + if($inventory instanceof TransactionBuilderInventory){ + $inventory = $inventory->getActualInventory(); + } + return (new \ReflectionClass($inventory))->getShortName() . "#" . spl_object_id($inventory) . ", slot: $slot"; + } + + /** + * @throws ItemStackRequestProcessException + */ + private function matchItemStack(Inventory $inventory, int $slotId, int $clientItemStackId) : void{ + $info = $this->inventoryManager->getItemStackInfo($inventory, $slotId); + if($info === null){ + throw new AssumptionFailedError("The inventory is tracked and the slot is valid, so this should not be null"); + } + + if(!($clientItemStackId < 0 ? $info->getRequestId() === $clientItemStackId : $info->getStackId() === $clientItemStackId)){ + throw new ItemStackRequestProcessException( + $this->prettyInventoryAndSlot($inventory, $slotId) . ": " . + "Mismatched expected itemstack, " . + "client expected: $clientItemStackId, server actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none") + ); + } + } + /** * @phpstan-return array{TransactionBuilderInventory, int} + * + * @throws ItemStackRequestProcessException */ private function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{ $windowId = ItemStackContainerIdTranslator::translate($info->getContainerId(), $this->inventoryManager->getCurrentWindowId()); $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $info->getSlotId()); if($windowAndSlot === null){ - throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); + throw new ItemStackRequestProcessException("No open inventory matches container UI ID: " . $info->getContainerId() . ", slot ID: " . $info->getSlotId()); } [$inventory, $slot] = $windowAndSlot; if(!$inventory->slotExists($slot)){ - throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); + throw new ItemStackRequestProcessException("No such inventory slot :" . $this->prettyInventoryAndSlot($inventory, $slot)); } - if( - $info->getStackId() !== $this->request->getRequestId() && //using TransactionBuilderInventory enables this to work - !$this->inventoryManager->matchItemStack($inventory, $slot, $info->getStackId()) - ){ - throw new PacketHandlingException("Inventory " . $info->getContainerId() . ", slot " . $slot . ": server-side item does not match expected"); + if($info->getStackId() !== $this->request->getRequestId()){ //the itemstack may have been modified by the current request + $this->matchItemStack($inventory, $slot, $info->getStackId()); } return [$this->builder->getInventory($inventory), $slot]; @@ -112,6 +137,7 @@ final class ItemStackRequestExecutor{ /** * Deducts items from an inventory slot, returning a stack containing the removed items. + * @throws ItemStackRequestProcessException */ private function removeItemFromSlot(ItemStackRequestSlotInfo $slotInfo, int $count) : Item{ $this->requestSlotInfos[] = $slotInfo; @@ -119,7 +145,7 @@ final class ItemStackRequestExecutor{ $existingItem = $inventory->getItem($slot); if($existingItem->getCount() < $count){ - throw new PacketHandlingException("Cannot take $count items from a stack of " . $existingItem->getCount()); + throw new ItemStackRequestProcessException($this->prettyInventoryAndSlot($inventory, $slot) . ": Cannot take $count items from a stack of " . $existingItem->getCount()); } $removed = $existingItem->pop($count); @@ -137,7 +163,7 @@ final class ItemStackRequestExecutor{ $existingItem = $inventory->getItem($slot); if(!$existingItem->isNull() && !$existingItem->canStackWith($item)){ - throw new PacketHandlingException("Can only add items to an empty slot, or a slot containing the same item"); + throw new ItemStackRequestProcessException($this->prettyInventoryAndSlot($inventory, $slot) . ": Can only add items to an empty slot, or a slot containing the same item"); } //we can't use the existing item here; it may be an empty stack @@ -146,6 +172,9 @@ final class ItemStackRequestExecutor{ $inventory->setItem($slot, $newItem); } + /** + * @throws ItemStackRequestProcessException + */ private function setNextCreatedItem(?Item $item, bool $creative = false) : void{ if($item !== null && $item->isNull()){ $item = null; @@ -157,7 +186,7 @@ final class ItemStackRequestExecutor{ $this->nextCreatedItem->setCount($this->createdItemsTakenCount); $this->builder->addAction(new CreateItemAction($this->nextCreatedItem)); }elseif($this->createdItemsTakenCount < $this->nextCreatedItem->getCount()){ - throw new PacketHandlingException("Not all of the previous created item was taken"); + throw new ItemStackRequestProcessException("Not all of the previous created item was taken"); } } $this->nextCreatedItem = $item; @@ -165,24 +194,27 @@ final class ItemStackRequestExecutor{ $this->createdItemsTakenCount = 0; } + /** + * @throws ItemStackRequestProcessException + */ private function beginCrafting(int $recipeId, int $repetitions) : void{ if($this->specialTransaction !== null){ - throw new PacketHandlingException("Cannot perform more than 1 special action per request"); + throw new ItemStackRequestProcessException("Another special transaction is already in progress"); } if($repetitions < 1){ //TODO: upper bound? - throw new PacketHandlingException("Cannot craft a recipe less than 1 time"); + throw new ItemStackRequestProcessException("Cannot craft a recipe less than 1 time"); } $craftingManager = $this->player->getServer()->getCraftingManager(); $recipe = $craftingManager->getCraftingRecipeFromIndex($recipeId); if($recipe === null){ - throw new PacketHandlingException("Unknown crafting recipe ID $recipeId"); + throw new ItemStackRequestProcessException("No such crafting recipe index: $recipeId"); } $this->specialTransaction = new CraftingTransaction($this->player, $craftingManager, [], $recipe, $repetitions); $currentWindow = $this->player->getCurrentWindow(); if($currentWindow !== null && !($currentWindow instanceof CraftingGrid)){ - throw new PacketHandlingException("Cannot complete crafting when the player's current window is not a crafting grid"); + throw new ItemStackRequestProcessException("Player's current window is not a crafting grid"); } $craftingGrid = $currentWindow ?? $this->player->getCraftingGrid(); @@ -200,13 +232,13 @@ final class ItemStackRequestExecutor{ private function takeCreatedItem(ItemStackRequestSlotInfo $destination, int $count) : void{ $createdItem = $this->nextCreatedItem; if($createdItem === null){ - throw new PacketHandlingException("No created item is waiting to be taken"); + throw new ItemStackRequestProcessException("No created item is waiting to be taken"); } if(!$this->createdItemFromCreativeInventory){ $availableCount = $createdItem->getCount() - $this->createdItemsTakenCount; if($count > $availableCount){ - throw new PacketHandlingException("Not enough created items available to be taken (have $availableCount, tried to take $count)"); + throw new ItemStackRequestProcessException("Not enough created items available to be taken (have $availableCount, tried to take $count)"); } } @@ -217,6 +249,22 @@ final class ItemStackRequestExecutor{ } } + /** + * @throws ItemStackRequestProcessException + */ + private function assertDoingCrafting() : void{ + if(!$this->specialTransaction instanceof CraftingTransaction){ + if($this->specialTransaction === null){ + throw new ItemStackRequestProcessException("Expected CraftRecipe or CraftRecipeAuto action to precede this action"); + }else{ + throw new ItemStackRequestProcessException("A different special transaction is already in progress"); + } + } + } + + /** + * @throws ItemStackRequestProcessException + */ private function processItemStackRequestAction(ItemStackRequestAction $action) : void{ if( $action instanceof TakeStackRequestAction || @@ -253,10 +301,7 @@ final class ItemStackRequestExecutor{ }elseif($action instanceof CreativeCreateStackRequestAction){ $item = CreativeInventory::getInstance()->getItem($action->getCreativeItemId()); if($item === null){ - //TODO: the item may have been unregistered after the client was sent the creative contents, leaving a - //gap in the creative item list. This probably shouldn't be a violation, but I'm not sure how else to - //handle it right now. - throw new PacketHandlingException("Tried to create nonexisting creative item " . $action->getCreativeItemId()); + throw new ItemStackRequestProcessException("No such creative item index: " . $action->getCreativeItemId()); } $this->setNextCreatedItem($item, true); @@ -265,31 +310,34 @@ final class ItemStackRequestExecutor{ }elseif($action instanceof CraftRecipeAutoStackRequestAction){ $this->beginCrafting($action->getRecipeId(), $action->getRepetitions()); }elseif($action instanceof CraftingConsumeInputStackRequestAction){ - if(!$this->specialTransaction instanceof CraftingTransaction){ - throw new PacketHandlingException("Cannot consume crafting input when no crafting transaction is in progress"); - } + $this->assertDoingCrafting(); $this->removeItemFromSlot($action->getSource(), $action->getCount()); //output discarded - we allow CraftingTransaction to verify the balance }elseif($action instanceof CraftingCreateSpecificResultStackRequestAction){ - if(!$this->specialTransaction instanceof CraftingTransaction){ - throw new AssumptionFailedError("Cannot mark crafting result index when no crafting transaction is in progress"); - } + $this->assertDoingCrafting(); $nextResultItem = $this->craftingResults[$action->getResultIndex()] ?? null; if($nextResultItem === null){ - throw new PacketHandlingException("No such crafting result index " . $action->getResultIndex()); + throw new ItemStackRequestProcessException("No such crafting result index: " . $action->getResultIndex()); } $this->setNextCreatedItem($nextResultItem); }elseif($action instanceof DeprecatedCraftingResultsStackRequestAction){ //no obvious use }else{ - throw new PacketHandlingException("Unhandled item stack request action: " . get_class($action)); + throw new ItemStackRequestProcessException("Unhandled item stack request action"); } } + /** + * @throws ItemStackRequestProcessException + */ public function generateInventoryTransaction() : InventoryTransaction{ - foreach($this->request->getActions() as $action){ - $this->processItemStackRequestAction($action); + foreach($this->request->getActions() as $k => $action){ + try{ + $this->processItemStackRequestAction($action); + }catch(ItemStackRequestProcessException $e){ + throw new ItemStackRequestProcessException("Error processing action $k (" . (new \ReflectionClass($action))->getShortName() . "): " . $e->getMessage(), 0, $e); + } } $this->setNextCreatedItem(null); $inventoryActions = $this->builder->generateActions(); diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index 782404ebe..35a07425e 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -29,7 +29,7 @@ use pocketmine\network\mcpe\protocol\types\inventory\ContainerUIIds; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseContainerInfo; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseSlotInfo; -use pocketmine\network\PacketHandlingException; +use pocketmine\utils\AssumptionFailedError; final class ItemStackResponseBuilder{ @@ -51,15 +51,15 @@ final class ItemStackResponseBuilder{ /** * @phpstan-return array{Inventory, int} */ - private function getInventoryAndSlot(int $containerInterfaceId, int $slotId) : array{ + private function getInventoryAndSlot(int $containerInterfaceId, int $slotId) : ?array{ $windowId = ItemStackContainerIdTranslator::translate($containerInterfaceId, $this->inventoryManager->getCurrentWindowId()); $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slotId); if($windowAndSlot === null){ - throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); + return null; } [$inventory, $slot] = $windowAndSlot; if(!$inventory->slotExists($slot)){ - throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); + return null; } return [$inventory, $slot]; @@ -72,16 +72,21 @@ final class ItemStackResponseBuilder{ continue; } foreach($slotIds as $slotId){ - [$inventory, $slot] = $this->getInventoryAndSlot($containerInterfaceId, $slotId); + $inventoryAndSlot = $this->getInventoryAndSlot($containerInterfaceId, $slotId); + if($inventoryAndSlot === null){ + //a plugin may have closed the inventory during an event, or the slot may have been invalid + continue; + } + [$inventory, $slot] = $inventoryAndSlot; $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"); + throw new AssumptionFailedError("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) + //TODO: is this the correct behaviour? continue; } $item = $inventory->getItem($slot); From d57aca1367dc42ddbc35f36e524e80bed912e438 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 00:53:00 +0000 Subject: [PATCH 19/32] CS --- src/network/mcpe/handler/ItemStackRequestExecutor.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index c2ae8e305..5ba92e4aa 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -52,12 +52,11 @@ use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\SwapStackReque use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\TakeStackRequestAction; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse; use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset; -use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; use function array_key_first; use function count; -use function get_class; +use function spl_object_id; final class ItemStackRequestExecutor{ private TransactionBuilder $builder; From 804feedb67adfa894be310e5823d92647632e01f Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 00:54:33 +0000 Subject: [PATCH 20/32] Added some dumb limits --- src/network/mcpe/handler/InGamePacketHandler.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 70bb827a2..f4f882c80 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -532,6 +532,10 @@ class InGamePacketHandler extends PacketHandler{ } private function handleSingleItemStackRequest(ItemStackRequest $request) : ItemStackResponse{ + if(count($request->getActions()) > 20){ + //TODO: we can probably lower this limit, but this will do for now + throw new PacketHandlingException("Too many actions in ItemStackRequest"); + } $executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $request); try{ $transaction = $executor->generateInventoryTransaction(); @@ -547,6 +551,10 @@ class InGamePacketHandler extends PacketHandler{ public function handleItemStackRequest(ItemStackRequestPacket $packet) : bool{ $responses = []; + if(count($packet->getRequests()) > 80){ + //TODO: we can probably lower this limit, but this will do for now + throw new PacketHandlingException("Too many requests in ItemStackRequestPacket"); + } foreach($packet->getRequests() as $request){ $responses[] = $this->handleSingleItemStackRequest($request); } From 67b7b60d18ecc188c8003a101b5eff9db4cc4918 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 01:19:07 +0000 Subject: [PATCH 21/32] .............. --- .../ItemStackRequestProcessException.php | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 src/network/mcpe/handler/ItemStackRequestProcessException.php diff --git a/src/network/mcpe/handler/ItemStackRequestProcessException.php b/src/network/mcpe/handler/ItemStackRequestProcessException.php new file mode 100644 index 000000000..7d5c667cf --- /dev/null +++ b/src/network/mcpe/handler/ItemStackRequestProcessException.php @@ -0,0 +1,31 @@ + Date: Mon, 20 Mar 2023 01:28:18 +0000 Subject: [PATCH 22/32] InGamePacketHandler: remove dead code --- src/network/mcpe/handler/InGamePacketHandler.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index f4f882c80..f05e99607 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -136,9 +136,6 @@ use const JSON_THROW_ON_ERROR; class InGamePacketHandler extends PacketHandler{ private const MAX_FORM_RESPONSE_DEPTH = 2; //modal/simple will be 1, custom forms 2 - they will never contain anything other than string|int|float|bool|null - /** @var CraftingTransaction|null */ - protected $craftingTransaction = null; - /** @var float */ protected $lastRightClickTime = 0.0; /** @var UseItemTransactionData|null */ @@ -349,9 +346,7 @@ class InGamePacketHandler extends PacketHandler{ $result = $this->handleReleaseItemTransaction($packet->trData); } - if($this->craftingTransaction === null){ //don't sync if we're waiting to complete a crafting transaction - $this->inventoryManager->syncMismatchedPredictedSlotChanges(); - } + $this->inventoryManager->syncMismatchedPredictedSlotChanges(); $this->inventoryManager->setCurrentItemStackRequestId(null); return $result; } From 4e55433ed804bf3fa378b20d44a750bf4d0f2262 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 01:35:03 +0000 Subject: [PATCH 23/32] Fixed request rejecting --- composer.json | 2 +- composer.lock | 16 ++++++++-------- src/network/mcpe/handler/InGamePacketHandler.php | 5 ++++- .../mcpe/handler/ItemStackRequestExecutor.php | 4 ++-- .../mcpe/handler/ItemStackResponseBuilder.php | 4 ++-- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/composer.json b/composer.json index 23bf5f126..4a6ac4f5e 100644 --- a/composer.json +++ b/composer.json @@ -37,7 +37,7 @@ "pocketmine/bedrock-block-upgrade-schema": "~1.1.1+bedrock-1.19.70", "pocketmine/bedrock-data": "~2.1.1+bedrock-1.19.70", "pocketmine/bedrock-item-upgrade-schema": "~1.1.0+bedrock-1.19.70", - "pocketmine/bedrock-protocol": "~20.0.0+bedrock-1.19.70", + "pocketmine/bedrock-protocol": "~20.1.0+bedrock-1.19.70", "pocketmine/binaryutils": "^0.2.1", "pocketmine/callback-validator": "^1.0.2", "pocketmine/classloader": "^0.2.0", diff --git a/composer.lock b/composer.lock index 4f149eee9..33fb06dfd 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "1d0c1d2fe668d85ae87110a1e3cfac05", + "content-hash": "01afa65b40f95ad9378c8cd999e6098d", "packages": [ { "name": "adhocore/json-comment", @@ -328,16 +328,16 @@ }, { "name": "pocketmine/bedrock-protocol", - "version": "20.0.0+bedrock-1.19.70", + "version": "20.1.0+bedrock-1.19.70", "source": { "type": "git", "url": "https://github.com/pmmp/BedrockProtocol.git", - "reference": "4892a5020187da805d7b46ab522d8185b0283726" + "reference": "91d67c8b1bced3c82d0841b1041c0c1f4e93eb68" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/4892a5020187da805d7b46ab522d8185b0283726", - "reference": "4892a5020187da805d7b46ab522d8185b0283726", + "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/91d67c8b1bced3c82d0841b1041c0c1f4e93eb68", + "reference": "91d67c8b1bced3c82d0841b1041c0c1f4e93eb68", "shasum": "" }, "require": { @@ -351,7 +351,7 @@ "ramsey/uuid": "^4.1" }, "require-dev": { - "phpstan/phpstan": "1.10.1", + "phpstan/phpstan": "1.10.7", "phpstan/phpstan-phpunit": "^1.0.0", "phpstan/phpstan-strict-rules": "^1.0.0", "phpunit/phpunit": "^9.5" @@ -369,9 +369,9 @@ "description": "An implementation of the Minecraft: Bedrock Edition protocol in PHP", "support": { "issues": "https://github.com/pmmp/BedrockProtocol/issues", - "source": "https://github.com/pmmp/BedrockProtocol/tree/20.0.0+bedrock-1.19.70" + "source": "https://github.com/pmmp/BedrockProtocol/tree/20.1.0+bedrock-1.19.70" }, - "time": "2023-03-14T17:06:38+00:00" + "time": "2023-03-20T01:17:00+00:00" }, { "name": "pocketmine/binaryutils", diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index f05e99607..e6e7d0916 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -541,7 +541,10 @@ class InGamePacketHandler extends PacketHandler{ $this->session->getLogger()->debug(implode("\n", Utils::printableExceptionInfo($e))); } - return $executor->buildItemStackResponse($result); + if(!$result){ + return new ItemStackResponse(ItemStackResponse::RESULT_ERROR, $request->getRequestId()); + } + return $executor->buildItemStackResponse(); } public function handleItemStackRequest(ItemStackRequestPacket $packet) : bool{ diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index 5ba92e4aa..15f1776f0 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -349,12 +349,12 @@ final class ItemStackRequestExecutor{ return $transaction; } - public function buildItemStackResponse(bool $success) : ItemStackResponse{ + public function buildItemStackResponse() : ItemStackResponse{ $builder = new ItemStackResponseBuilder($this->request->getRequestId(), $this->inventoryManager); foreach($this->requestSlotInfos as $requestInfo){ $builder->addSlot($requestInfo->getContainerId(), $requestInfo->getSlotId()); } - return $builder->build($success); + return $builder->build(); } } diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index 35a07425e..1c742077f 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -65,7 +65,7 @@ final class ItemStackResponseBuilder{ return [$inventory, $slot]; } - public function build(bool $success) : ItemStackResponse{ + public function build() : ItemStackResponse{ $responseInfosByContainer = []; foreach($this->changedSlots as $containerInterfaceId => $slotIds){ if($containerInterfaceId === ContainerUIIds::CREATED_OUTPUT){ @@ -107,6 +107,6 @@ final class ItemStackResponseBuilder{ $responseContainerInfos[] = new ItemStackResponseContainerInfo($containerInterfaceId, $responseInfos); } - return new ItemStackResponse($success ? ItemStackResponse::RESULT_OK : ItemStackResponse::RESULT_ERROR, $this->requestId, $responseContainerInfos); + return new ItemStackResponse(ItemStackResponse::RESULT_OK, $this->requestId, $responseContainerInfos); } } From c91168db66f58a14c51e8a1d33f12b9f1a4a24eb Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 01:35:15 +0000 Subject: [PATCH 24/32] ... --- src/network/mcpe/handler/InGamePacketHandler.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index e6e7d0916..682cc0d4e 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -33,7 +33,6 @@ use pocketmine\entity\Attribute; use pocketmine\entity\InvalidSkinException; use pocketmine\event\player\PlayerEditBookEvent; use pocketmine\inventory\transaction\action\DropItemAction; -use pocketmine\inventory\transaction\CraftingTransaction; use pocketmine\inventory\transaction\InventoryTransaction; use pocketmine\inventory\transaction\TransactionBuilder; use pocketmine\inventory\transaction\TransactionException; From 59bae9b0774976c9f449f6aa1c70676b23d563dd Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 16:53:57 +0000 Subject: [PATCH 25/32] Give InventoryManager internals clearer names and stop mixing 'window' and 'inventory' terminology... --- src/network/mcpe/ComplexWindowMapEntry.php | 64 ---------------------- src/network/mcpe/InventoryManager.php | 62 ++++++++++++--------- 2 files changed, 35 insertions(+), 91 deletions(-) delete mode 100644 src/network/mcpe/ComplexWindowMapEntry.php diff --git a/src/network/mcpe/ComplexWindowMapEntry.php b/src/network/mcpe/ComplexWindowMapEntry.php deleted file mode 100644 index c2792297b..000000000 --- a/src/network/mcpe/ComplexWindowMapEntry.php +++ /dev/null @@ -1,64 +0,0 @@ - - */ - private array $reverseSlotMap = []; - - /** - * @param int[] $slotMap - * @phpstan-param array $slotMap - */ - public function __construct( - private Inventory $inventory, - private array $slotMap - ){ - foreach($slotMap as $slot => $index){ - $this->reverseSlotMap[$index] = $slot; - } - } - - public function getInventory() : Inventory{ return $this->inventory; } - - /** - * @return int[] - * @phpstan-return array - */ - public function getSlotMap() : array{ return $this->slotMap; } - - public function mapNetToCore(int $slot) : ?int{ - return $this->slotMap[$slot] ?? null; - } - - public function mapCoreToNet(int $slot) : ?int{ - return $this->reverseSlotMap[$slot] ?? null; - } -} diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 1ea14b33c..8a40762b4 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -69,18 +69,21 @@ use function spl_object_id; * @phpstan-type ContainerOpenClosure \Closure(int $id, Inventory $inventory) : (list|null) */ class InventoryManager{ - /** @var Inventory[] */ - private array $windowMap = []; /** - * @var ComplexWindowMapEntry[] - * @phpstan-var array + * @var Inventory[] network window ID => Inventory + * @phpstan-var array */ - private array $complexWindows = []; + private array $networkIdToInventoryMap = []; /** - * @var ComplexWindowMapEntry[] - * @phpstan-var array + * @var ComplexInventoryMapEntry[] spl_object_id(Inventory) => ComplexWindowMapEntry + * @phpstan-var array */ - private array $complexSlotToWindowMap = []; + private array $complexInventorySlotMaps = []; + /** + * @var ComplexInventoryMapEntry[] net slot ID => ComplexWindowMapEntry + * @phpstan-var array + */ + private array $complexSlotToInventoryMap = []; private int $lastInventoryNetworkId = ContainerIds::FIRST; @@ -126,7 +129,7 @@ class InventoryManager{ } private function add(int $id, Inventory $inventory) : void{ - $this->windowMap[$id] = $inventory; + $this->networkIdToInventoryMap[$id] = $inventory; } private function addDynamic(Inventory $inventory) : int{ @@ -140,29 +143,34 @@ class InventoryManager{ * @phpstan-param array|int $slotMap */ private function addComplex(array|int $slotMap, Inventory $inventory) : void{ - $entry = new ComplexWindowMapEntry($inventory, is_int($slotMap) ? [$slotMap => 0] : $slotMap); - $this->complexWindows[spl_object_id($inventory)] = $entry; + $entry = new ComplexInventoryMapEntry($inventory, is_int($slotMap) ? [$slotMap => 0] : $slotMap); + $this->complexInventorySlotMaps[spl_object_id($inventory)] = $entry; foreach($entry->getSlotMap() as $netSlot => $coreSlot){ - $this->complexSlotToWindowMap[$netSlot] = $entry; + $this->complexSlotToInventoryMap[$netSlot] = $entry; } } private function remove(int $id) : void{ - $inventory = $this->windowMap[$id]; - unset($this->windowMap[$id]); + $inventory = $this->networkIdToInventoryMap[$id]; + unset($this->networkIdToInventoryMap[$id]); if($this->getWindowId($inventory) === null){ $splObjectId = spl_object_id($inventory); - unset($this->initiatedSlotChanges[$splObjectId], $this->itemStackInfos[$splObjectId], $this->complexWindows[$splObjectId]); - foreach($this->complexSlotToWindowMap as $netSlot => $entry){ + unset( + $this->initiatedSlotChanges[$splObjectId], + $this->itemStackInfos[$splObjectId], + $this->complexInventorySlotMaps[$splObjectId], + $this->pendingSlotSyncs[$splObjectId] + ); + foreach($this->complexSlotToInventoryMap as $netSlot => $entry){ if($entry->getInventory() === $inventory){ - unset($this->complexSlotToWindowMap[$netSlot]); + unset($this->complexSlotToInventoryMap[$netSlot]); } } } } public function getWindowId(Inventory $inventory) : ?int{ - return ($id = array_search($inventory, $this->windowMap, true)) !== false ? $id : null; + return ($id = array_search($inventory, $this->networkIdToInventoryMap, true)) !== false ? $id : null; } public function getCurrentWindowId() : int{ @@ -174,15 +182,15 @@ class InventoryManager{ */ public function locateWindowAndSlot(int $windowId, int $netSlotId) : ?array{ if($windowId === ContainerIds::UI){ - $entry = $this->complexSlotToWindowMap[$netSlotId] ?? null; + $entry = $this->complexSlotToInventoryMap[$netSlotId] ?? null; if($entry === null){ return null; } $coreSlotId = $entry->mapNetToCore($netSlotId); return $coreSlotId !== null ? [$entry->getInventory(), $coreSlotId] : null; } - if(isset($this->windowMap[$windowId])){ - return [$this->windowMap[$windowId], $netSlotId]; + if(isset($this->networkIdToInventoryMap[$windowId])){ + return [$this->networkIdToInventoryMap[$windowId], $netSlotId]; } return null; } @@ -344,7 +352,7 @@ class InventoryManager{ } public function onCurrentWindowRemove() : void{ - if(isset($this->windowMap[$this->lastInventoryNetworkId])){ + if(isset($this->networkIdToInventoryMap[$this->lastInventoryNetworkId])){ $this->remove($this->lastInventoryNetworkId); $this->session->sendDataPacket(ContainerClosePacket::create($this->lastInventoryNetworkId, true)); if($this->pendingCloseWindowId !== null){ @@ -356,7 +364,7 @@ class InventoryManager{ public function onClientRemoveWindow(int $id) : void{ if($id === $this->lastInventoryNetworkId){ - if(isset($this->windowMap[$id]) && $id !== $this->pendingCloseWindowId){ + if(isset($this->networkIdToInventoryMap[$id]) && $id !== $this->pendingCloseWindowId){ $this->remove($id); $this->player->removeCurrentWindow(); } @@ -398,7 +406,7 @@ class InventoryManager{ if($itemStackInfo === null){ throw new \LogicException("Cannot sync an untracked inventory slot"); } - $slotMap = $this->complexWindows[spl_object_id($inventory)] ?? null; + $slotMap = $this->complexInventorySlotMaps[spl_object_id($inventory)] ?? null; if($slotMap !== null){ $windowId = ContainerIds::UI; $netSlot = $slotMap->mapCoreToNet($slot) ?? throw new AssumptionFailedError("We already have an ItemStackInfo, so this should not be null"); @@ -438,7 +446,7 @@ class InventoryManager{ } public function syncContents(Inventory $inventory) : void{ - $slotMap = $this->complexWindows[spl_object_id($inventory)] ?? null; + $slotMap = $this->complexInventorySlotMaps[spl_object_id($inventory)] ?? null; if($slotMap !== null){ $windowId = ContainerIds::UI; }else{ @@ -471,10 +479,10 @@ class InventoryManager{ } public function syncAll() : void{ - foreach($this->windowMap as $inventory){ + foreach($this->networkIdToInventoryMap as $inventory){ $this->syncContents($inventory); } - foreach($this->complexWindows as $entry){ + foreach($this->complexInventorySlotMaps as $entry){ $this->syncContents($entry->getInventory()); } } From 6ccb8f737305059038d695c82db1bfba3047d514 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 16:57:38 +0000 Subject: [PATCH 26/32] git --- src/network/mcpe/ComplexInventoryMapEntry.php | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 src/network/mcpe/ComplexInventoryMapEntry.php diff --git a/src/network/mcpe/ComplexInventoryMapEntry.php b/src/network/mcpe/ComplexInventoryMapEntry.php new file mode 100644 index 000000000..dfd3e999a --- /dev/null +++ b/src/network/mcpe/ComplexInventoryMapEntry.php @@ -0,0 +1,64 @@ + + */ + private array $reverseSlotMap = []; + + /** + * @param int[] $slotMap + * @phpstan-param array $slotMap + */ + public function __construct( + private Inventory $inventory, + private array $slotMap + ){ + foreach($slotMap as $slot => $index){ + $this->reverseSlotMap[$index] = $slot; + } + } + + public function getInventory() : Inventory{ return $this->inventory; } + + /** + * @return int[] + * @phpstan-return array + */ + public function getSlotMap() : array{ return $this->slotMap; } + + public function mapNetToCore(int $slot) : ?int{ + return $this->slotMap[$slot] ?? null; + } + + public function mapCoreToNet(int $slot) : ?int{ + return $this->reverseSlotMap[$slot] ?? null; + } +} From 3d70a169e11df3301aebd84d35bedc8447667cf9 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 17:31:54 +0000 Subject: [PATCH 27/32] Reduce chaos in InventoryManager the information in these arrays is usually needed all at the same time, so it doesn't make sense to force multiple array lookups for it. in addition, this (obviously) cleans up the code quite a lot. --- src/network/mcpe/InventoryManager.php | 106 +++++++++--------- ...dChanges.php => InventoryManagerEntry.php} | 33 ++---- 2 files changed, 60 insertions(+), 79 deletions(-) rename src/network/mcpe/{InventoryManagerPredictedChanges.php => InventoryManagerEntry.php} (63%) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 8a40762b4..868ccebdf 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -69,16 +69,17 @@ use function spl_object_id; * @phpstan-type ContainerOpenClosure \Closure(int $id, Inventory $inventory) : (list|null) */ class InventoryManager{ + /** + * @var InventoryManagerEntry[] spl_object_id(Inventory) => InventoryManagerEntry + * @phpstan-var array + */ + private array $inventories = []; + /** * @var Inventory[] network window ID => Inventory * @phpstan-var array */ private array $networkIdToInventoryMap = []; - /** - * @var ComplexInventoryMapEntry[] spl_object_id(Inventory) => ComplexWindowMapEntry - * @phpstan-var array - */ - private array $complexInventorySlotMaps = []; /** * @var ComplexInventoryMapEntry[] net slot ID => ComplexWindowMapEntry * @phpstan-var array @@ -87,11 +88,6 @@ class InventoryManager{ private int $lastInventoryNetworkId = ContainerIds::FIRST; - /** - * @var Item[][] - * @phpstan-var array - */ - private array $initiatedSlotChanges = []; private int $clientSelectedHotbarSlot = -1; /** @phpstan-var ObjectSet */ @@ -104,12 +100,6 @@ class InventoryManager{ private int $nextItemStackId = 1; private ?int $currentItemStackRequestId = null; - /** - * @var int[][] - * @phpstan-var array> - */ - private array $itemStackInfos = []; - public function __construct( private Player $player, private NetworkSession $session @@ -129,10 +119,12 @@ class InventoryManager{ } private function add(int $id, Inventory $inventory) : void{ + $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry($inventory); $this->networkIdToInventoryMap[$id] = $inventory; } private function addDynamic(Inventory $inventory) : int{ + $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry($inventory); $this->lastInventoryNetworkId = max(ContainerIds::FIRST, ($this->lastInventoryNetworkId + 1) % ContainerIds::LAST); $this->add($this->lastInventoryNetworkId, $inventory); return $this->lastInventoryNetworkId; @@ -144,7 +136,10 @@ class InventoryManager{ */ private function addComplex(array|int $slotMap, Inventory $inventory) : void{ $entry = new ComplexInventoryMapEntry($inventory, is_int($slotMap) ? [$slotMap => 0] : $slotMap); - $this->complexInventorySlotMaps[spl_object_id($inventory)] = $entry; + $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry( + $inventory, + $entry + ); foreach($entry->getSlotMap() as $netSlot => $coreSlot){ $this->complexSlotToInventoryMap[$netSlot] = $entry; } @@ -154,13 +149,7 @@ class InventoryManager{ $inventory = $this->networkIdToInventoryMap[$id]; unset($this->networkIdToInventoryMap[$id]); if($this->getWindowId($inventory) === null){ - $splObjectId = spl_object_id($inventory); - unset( - $this->initiatedSlotChanges[$splObjectId], - $this->itemStackInfos[$splObjectId], - $this->complexInventorySlotMaps[$splObjectId], - $this->pendingSlotSyncs[$splObjectId] - ); + unset($this->inventories[spl_object_id($inventory)]); foreach($this->complexSlotToInventoryMap as $netSlot => $entry){ if($entry->getInventory() === $inventory){ unset($this->complexSlotToInventoryMap[$netSlot]); @@ -196,8 +185,7 @@ class InventoryManager{ } private function addPredictedSlotChange(Inventory $inventory, int $slot, ItemStack $item) : void{ - $predictions = ($this->initiatedSlotChanges[spl_object_id($inventory)] ??= new InventoryManagerPredictedChanges($inventory)); - $predictions->add($slot, $item); + $this->inventories[spl_object_id($inventory)]->predictions[$slot] = $item; } public function addTransactionPredictedSlotChanges(InventoryTransaction $tx) : void{ @@ -388,8 +376,8 @@ class InventoryManager{ public function onSlotChange(Inventory $inventory, int $slot) : void{ $currentItem = TypeConverter::getInstance()->coreItemStackToNet($inventory->getItem($slot)); - $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; - $clientSideItem = $predictions?->getSlot($slot); + $inventoryEntry = $this->inventories[spl_object_id($inventory)]; + $clientSideItem = $inventoryEntry->predictions[$slot] ?? null; if($clientSideItem === null || !$clientSideItem->equals($currentItem)){ //no prediction or incorrect - do not associate this with the currently active itemstack request $this->trackItemStack($inventory, $slot, $currentItem, null); @@ -398,18 +386,22 @@ class InventoryManager{ //correctly predicted - associate the change with the currently active itemstack request $this->trackItemStack($inventory, $slot, $currentItem, $this->currentItemStackRequestId); } - $predictions?->remove($slot); + + unset($inventoryEntry->predictions[$slot]); } public function syncSlot(Inventory $inventory, int $slot) : void{ - $itemStackInfo = $this->getItemStackInfo($inventory, $slot); + $entry = $this->inventories[spl_object_id($inventory)] ?? null; + if($entry === null){ + throw new \LogicException("Cannot sync an untracked inventory"); + } + $itemStackInfo = $entry->itemStackInfos[$slot]; if($itemStackInfo === null){ throw new \LogicException("Cannot sync an untracked inventory slot"); } - $slotMap = $this->complexInventorySlotMaps[spl_object_id($inventory)] ?? null; - if($slotMap !== null){ + if($entry->complexSlotMap !== null){ $windowId = ContainerIds::UI; - $netSlot = $slotMap->mapCoreToNet($slot) ?? throw new AssumptionFailedError("We already have an ItemStackInfo, so this should not be null"); + $netSlot = $entry->complexSlotMap->mapCoreToNet($slot) ?? throw new AssumptionFailedError("We already have an ItemStackInfo, so this should not be null"); }else{ $windowId = $this->getWindowId($inventory) ?? throw new AssumptionFailedError("We already have an ItemStackInfo, so this should not be null"); $netSlot = $slot; @@ -441,28 +433,27 @@ class InventoryManager{ } $this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, $itemStackWrapper)); } - $predictions = $this->initiatedSlotChanges[spl_object_id($inventory)] ?? null; - $predictions?->remove($slot); + unset($entry->predictions[$slot]); } public function syncContents(Inventory $inventory) : void{ - $slotMap = $this->complexInventorySlotMaps[spl_object_id($inventory)] ?? null; - if($slotMap !== null){ + $entry = $this->inventories[spl_object_id($inventory)]; + if($entry->complexSlotMap !== null){ $windowId = ContainerIds::UI; }else{ $windowId = $this->getWindowId($inventory); } if($windowId !== null){ - unset($this->initiatedSlotChanges[spl_object_id($inventory)]); + $entry->predictions = []; $contents = []; foreach($inventory->getContents(true) as $slot => $item){ $itemStack = TypeConverter::getInstance()->coreItemStackToNet($item); $info = $this->trackItemStack($inventory, $slot, $itemStack, null); $contents[] = new ItemStackWrapper($info->getStackId(), $info->getItemStack()); } - if($slotMap !== null){ + if($entry->complexSlotMap !== null){ foreach($contents as $slotId => $info){ - $packetSlot = $slotMap->mapCoreToNet($slotId) ?? null; + $packetSlot = $entry->complexSlotMap->mapCoreToNet($slotId) ?? null; if($packetSlot === null){ continue; } @@ -479,19 +470,16 @@ class InventoryManager{ } public function syncAll() : void{ - foreach($this->networkIdToInventoryMap as $inventory){ - $this->syncContents($inventory); - } - foreach($this->complexInventorySlotMaps as $entry){ - $this->syncContents($entry->getInventory()); + foreach($this->inventories as $entry){ + $this->syncContents($entry->inventory); } } public function syncMismatchedPredictedSlotChanges() : void{ - foreach($this->initiatedSlotChanges as $predictions){ - $inventory = $predictions->getInventory(); - foreach($predictions->getSlots() as $slot => $expectedItem){ - if(!$inventory->slotExists($slot) || $this->getItemStackInfo($inventory, $slot) === null){ + foreach($this->inventories as $entry){ + $inventory = $entry->inventory; + foreach($entry->predictions as $slot => $expectedItem){ + if(!$inventory->slotExists($slot) || $entry->itemStackInfos[$slot] === null){ continue; //TODO: size desync ??? } @@ -499,9 +487,9 @@ class InventoryManager{ $this->session->getLogger()->debug("Detected prediction mismatch in inventory " . get_class($inventory) . "#" . spl_object_id($inventory) . " slot $slot"); $this->syncSlot($inventory, $slot); } - } - $this->initiatedSlotChanges = []; + $entry->predictions = []; + } } public function syncData(Inventory $inventory, int $propertyId, int $value) : void{ @@ -519,7 +507,10 @@ class InventoryManager{ $playerInventory = $this->player->getInventory(); $selected = $playerInventory->getHeldItemIndex(); if($selected !== $this->clientSelectedHotbarSlot){ - $itemStackInfo = $this->itemStackInfos[spl_object_id($playerInventory)][$selected]; + $itemStackInfo = $this->getItemStackInfo($playerInventory, $selected); + if($itemStackInfo === null){ + throw new AssumptionFailedError("Player inventory slots should always be tracked"); + } $this->session->sendDataPacket(MobEquipmentPacket::create( $this->player->getId(), @@ -550,17 +541,22 @@ class InventoryManager{ } public function getItemStackInfo(Inventory $inventory, int $slot) : ?ItemStackInfo{ - return $this->itemStackInfos[spl_object_id($inventory)][$slot] ?? null; + $entry = $this->inventories[spl_object_id($inventory)] ?? null; + return $entry?->itemStackInfos[$slot] ?? null; } private function trackItemStack(Inventory $inventory, int $slotId, ItemStack $itemStack, ?int $itemStackRequestId) : ItemStackInfo{ - $existing = $this->itemStackInfos[spl_object_id($inventory)][$slotId] ?? null; + $entry = $this->inventories[spl_object_id($inventory)] ?? null; + if($entry === null){ + throw new \LogicException("Cannot track an item stack for an untracked inventory"); + } + $existing = $entry->itemStackInfos[$slotId] ?? null; if($existing !== null && $existing->getItemStack()->equals($itemStack) && $existing->getRequestId() === $itemStackRequestId){ return $existing; } //TODO: ItemStack->isNull() would be nice to have here $info = new ItemStackInfo($itemStackRequestId, $itemStack->getId() === 0 ? 0 : $this->newItemStackId(), $itemStack); - return $this->itemStackInfos[spl_object_id($inventory)][$slotId] = $info; + return $entry->itemStackInfos[$slotId] = $info; } } diff --git a/src/network/mcpe/InventoryManagerPredictedChanges.php b/src/network/mcpe/InventoryManagerEntry.php similarity index 63% rename from src/network/mcpe/InventoryManagerPredictedChanges.php rename to src/network/mcpe/InventoryManagerEntry.php index 8264e83fb..7b97a45b0 100644 --- a/src/network/mcpe/InventoryManagerPredictedChanges.php +++ b/src/network/mcpe/InventoryManagerEntry.php @@ -26,36 +26,21 @@ namespace pocketmine\network\mcpe; use pocketmine\inventory\Inventory; use pocketmine\network\mcpe\protocol\types\inventory\ItemStack; -final class InventoryManagerPredictedChanges{ +final class InventoryManagerEntry{ /** * @var ItemStack[] * @phpstan-var array */ - private array $slots = []; - - public function __construct( - private Inventory $inventory - ){} - - public function getInventory() : Inventory{ return $this->inventory; } + public array $predictions; /** - * @return ItemStack[] - * @phpstan-return array + * @var ItemStackInfo[] + * @phpstan-var array */ - public function getSlots() : array{ - return $this->slots; - } + public array $itemStackInfos = []; - public function getSlot(int $slot) : ?ItemStack{ - return $this->slots[$slot] ?? null; - } - - public function add(int $slot, ItemStack $item) : void{ - $this->slots[$slot] = $item; - } - - public function remove(int $slot) : void{ - unset($this->slots[$slot]); - } + public function __construct( + public Inventory $inventory, + public ?ComplexInventoryMapEntry $complexSlotMap = null + ){} } From a83fc85f1e80a113cd2e4c59eb195cba02640d15 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 17:32:44 +0000 Subject: [PATCH 28/32] InventoryManagerEntry: fixed missing default --- src/network/mcpe/InventoryManagerEntry.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/mcpe/InventoryManagerEntry.php b/src/network/mcpe/InventoryManagerEntry.php index 7b97a45b0..b87650e29 100644 --- a/src/network/mcpe/InventoryManagerEntry.php +++ b/src/network/mcpe/InventoryManagerEntry.php @@ -31,7 +31,7 @@ final class InventoryManagerEntry{ * @var ItemStack[] * @phpstan-var array */ - public array $predictions; + public array $predictions = []; /** * @var ItemStackInfo[] From e8085e22a00074638f7808e30647d69f7ddd9792 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 18:40:18 +0000 Subject: [PATCH 29/32] Fixed crash when opening main inventory the InventoryManagerEntry was getting overwritten, since we don't expect to open the same inventory with two different window IDs. --- src/network/mcpe/InventoryManager.php | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 868ccebdf..e93f2bc39 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -118,30 +118,48 @@ class InventoryManager{ }); } - private function add(int $id, Inventory $inventory) : void{ - $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry($inventory); + private function associateIdWithInventory(int $id, Inventory $inventory) : void{ $this->networkIdToInventoryMap[$id] = $inventory; } - private function addDynamic(Inventory $inventory) : int{ - $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry($inventory); + private function getNewWindowId() : int{ $this->lastInventoryNetworkId = max(ContainerIds::FIRST, ($this->lastInventoryNetworkId + 1) % ContainerIds::LAST); - $this->add($this->lastInventoryNetworkId, $inventory); return $this->lastInventoryNetworkId; } + private function add(int $id, Inventory $inventory) : void{ + if(isset($this->inventories[spl_object_id($inventory)])){ + throw new \InvalidArgumentException("Inventory " . get_class($inventory) . " is already tracked"); + } + $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry($inventory); + $this->associateIdWithInventory($id, $inventory); + } + + private function addDynamic(Inventory $inventory) : int{ + if(isset($this->inventories[spl_object_id($inventory)])){ + throw new \InvalidArgumentException("Inventory " . get_class($inventory) . " is already tracked"); + } + $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry($inventory); + $id = $this->getNewWindowId(); + $this->add($id, $inventory); + return $id; + } + /** * @param int[]|int $slotMap * @phpstan-param array|int $slotMap */ private function addComplex(array|int $slotMap, Inventory $inventory) : void{ - $entry = new ComplexInventoryMapEntry($inventory, is_int($slotMap) ? [$slotMap => 0] : $slotMap); + if(isset($this->inventories[spl_object_id($inventory)])){ + throw new \InvalidArgumentException("Inventory " . get_class($inventory) . " is already tracked"); + } + $complexSlotMap = new ComplexInventoryMapEntry($inventory, is_int($slotMap) ? [$slotMap => 0] : $slotMap); $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry( $inventory, - $entry + $complexSlotMap ); - foreach($entry->getSlotMap() as $netSlot => $coreSlot){ - $this->complexSlotToInventoryMap[$netSlot] = $entry; + foreach($complexSlotMap->getSlotMap() as $netSlot => $coreSlot){ + $this->complexSlotToInventoryMap[$netSlot] = $complexSlotMap; } } @@ -329,7 +347,8 @@ class InventoryManager{ $this->onCurrentWindowRemove(); $this->openWindowDeferred(function() : void{ - $windowId = $this->addDynamic($this->player->getInventory()); + $windowId = $this->getNewWindowId(); + $this->associateIdWithInventory($windowId, $this->player->getInventory()); $this->session->sendDataPacket(ContainerOpenPacket::entityInv( $windowId, From ca6d51498f12427a947467da8fcad7811418e6cc Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 19:16:00 +0000 Subject: [PATCH 30/32] Buffer slot and content syncing until the end of the tick we may receive multiple requests in one tick (e.g. crafting in a batch) --- src/network/mcpe/InventoryManager.php | 36 +++++++++++++++++-- src/network/mcpe/InventoryManagerEntry.php | 6 ++++ src/network/mcpe/NetworkSession.php | 1 + .../mcpe/handler/InGamePacketHandler.php | 15 +++++--- 4 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index e93f2bc39..225b6cdec 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -59,8 +59,11 @@ use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\ObjectSet; +use function array_keys; use function array_search; +use function count; use function get_class; +use function implode; use function is_int; use function max; use function spl_object_id; @@ -100,6 +103,8 @@ class InventoryManager{ private int $nextItemStackId = 1; private ?int $currentItemStackRequestId = null; + private bool $fullSyncRequested = false; + public function __construct( private Player $player, private NetworkSession $session @@ -400,7 +405,7 @@ class InventoryManager{ if($clientSideItem === null || !$clientSideItem->equals($currentItem)){ //no prediction or incorrect - do not associate this with the currently active itemstack request $this->trackItemStack($inventory, $slot, $currentItem, null); - $this->syncSlot($inventory, $slot); + $inventoryEntry->pendingSyncs[$slot] = $slot; }else{ //correctly predicted - associate the change with the currently active itemstack request $this->trackItemStack($inventory, $slot, $currentItem, $this->currentItemStackRequestId); @@ -452,7 +457,7 @@ class InventoryManager{ } $this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, $itemStackWrapper)); } - unset($entry->predictions[$slot]); + unset($entry->predictions[$slot], $entry->pendingSyncs[$slot]); } public function syncContents(Inventory $inventory) : void{ @@ -464,6 +469,7 @@ class InventoryManager{ } if($windowId !== null){ $entry->predictions = []; + $entry->pendingSyncs = []; $contents = []; foreach($inventory->getContents(true) as $slot => $item){ $itemStack = TypeConverter::getInstance()->coreItemStackToNet($item); @@ -494,6 +500,10 @@ class InventoryManager{ } } + public function requestSyncAll() : void{ + $this->fullSyncRequested = true; + } + public function syncMismatchedPredictedSlotChanges() : void{ foreach($this->inventories as $entry){ $inventory = $entry->inventory; @@ -504,13 +514,33 @@ class InventoryManager{ //any prediction that still exists at this point is a slot that was predicted to change but didn't $this->session->getLogger()->debug("Detected prediction mismatch in inventory " . get_class($inventory) . "#" . spl_object_id($inventory) . " slot $slot"); - $this->syncSlot($inventory, $slot); + $entry->pendingSyncs[$slot] = $slot; } $entry->predictions = []; } } + public function flushPendingUpdates() : void{ + if($this->fullSyncRequested){ + $this->fullSyncRequested = false; + $this->session->getLogger()->debug("Full inventory sync requested, sending contents of " . count($this->inventories) . " inventories"); + $this->syncAll(); + }else{ + foreach($this->inventories as $entry){ + if(count($entry->pendingSyncs) === 0){ + continue; + } + $inventory = $entry->inventory; + $this->session->getLogger()->debug("Syncing slots " . implode(", ", array_keys($entry->pendingSyncs)) . " in inventory " . get_class($inventory) . "#" . spl_object_id($inventory)); + foreach($entry->pendingSyncs as $slot){ + $this->syncSlot($inventory, $slot); + } + $entry->pendingSyncs = []; + } + } + } + public function syncData(Inventory $inventory, int $propertyId, int $value) : void{ $windowId = $this->getWindowId($inventory); if($windowId !== null){ diff --git a/src/network/mcpe/InventoryManagerEntry.php b/src/network/mcpe/InventoryManagerEntry.php index b87650e29..fa49baf87 100644 --- a/src/network/mcpe/InventoryManagerEntry.php +++ b/src/network/mcpe/InventoryManagerEntry.php @@ -39,6 +39,12 @@ final class InventoryManagerEntry{ */ public array $itemStackInfos = []; + /** + * @var int[] + * @phpstan-var array + */ + public array $pendingSyncs = []; + public function __construct( public Inventory $inventory, public ?ComplexInventoryMapEntry $complexSlotMap = null diff --git a/src/network/mcpe/NetworkSession.php b/src/network/mcpe/NetworkSession.php index 56b0622d8..697795f1c 100644 --- a/src/network/mcpe/NetworkSession.php +++ b/src/network/mcpe/NetworkSession.php @@ -1219,6 +1219,7 @@ class NetworkSession{ $attribute->markSynchronized(); } } + $this->invManager?->flushPendingUpdates(); $this->flushSendBuffer(); } diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 682cc0d4e..68e939195 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -35,7 +35,8 @@ use pocketmine\event\player\PlayerEditBookEvent; use pocketmine\inventory\transaction\action\DropItemAction; use pocketmine\inventory\transaction\InventoryTransaction; use pocketmine\inventory\transaction\TransactionBuilder; -use pocketmine\inventory\transaction\TransactionException; +use pocketmine\inventory\transaction\TransactionCancelledException; +use pocketmine\inventory\transaction\TransactionValidationException; use pocketmine\item\VanillaItems; use pocketmine\item\WritableBook; use pocketmine\item\WritableBookPage; @@ -335,7 +336,7 @@ class InGamePacketHandler extends PacketHandler{ $result = $this->handleNormalTransaction($packet->trData, $packet->requestId); }elseif($packet->trData instanceof MismatchTransactionData){ $this->session->getLogger()->debug("Mismatch transaction received"); - $this->inventoryManager->syncAll(); + $this->inventoryManager->requestSyncAll(); $result = true; }elseif($packet->trData instanceof UseItemTransactionData){ $result = $this->handleUseItemTransaction($packet->trData); @@ -357,9 +358,14 @@ class InGamePacketHandler extends PacketHandler{ $this->inventoryManager->addTransactionPredictedSlotChanges($transaction); try{ $transaction->execute(); - }catch(TransactionException $e){ + }catch(TransactionValidationException $e){ + $this->inventoryManager->requestSyncAll(); $logger = $this->session->getLogger(); - $logger->debug("Failed to execute inventory transaction: " . $e->getMessage()); + $logger->debug("Invalid inventory transaction $requestId: " . $e->getMessage()); + + return false; + }catch(TransactionCancelledException){ + $this->session->getLogger()->debug("Inventory transaction $requestId cancelled by a plugin"); return false; }finally{ @@ -538,6 +544,7 @@ class InGamePacketHandler extends PacketHandler{ $result = false; $this->session->getLogger()->debug("ItemStackRequest #" . $request->getRequestId() . " failed: " . $e->getMessage()); $this->session->getLogger()->debug(implode("\n", Utils::printableExceptionInfo($e))); + $this->inventoryManager->requestSyncAll(); } if(!$result){ From 758b5ee500b2860d4f9997f90120ebd048c9d95f Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 21:27:56 +0000 Subject: [PATCH 31/32] InventoryManager: fixed armor slots hack the correct condition for this should be an unsynced armor slot changed during a transaction, but conveying this information to syncSlot() is a bit of a hassle, so this will do for now. --- src/network/mcpe/InventoryManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 225b6cdec..d86821c2f 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -440,7 +440,7 @@ class InventoryManager{ //BDS (Bedrock Dedicated Server) also seems to work this way. $this->session->sendDataPacket(InventoryContentPacket::create($windowId, [$itemStackWrapper])); }else{ - if($this->currentItemStackRequestId !== null){ + if($windowId === ContainerIds::ARMOR){ //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 From c9601ae67dd35434817dc3429e6e35aac57e5997 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 22:00:38 +0000 Subject: [PATCH 32/32] Fixed crash when opening crafting table and other 'UI' inventories --- src/network/mcpe/InventoryManager.php | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index d86821c2f..5b5708b8a 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -141,10 +141,6 @@ class InventoryManager{ } private function addDynamic(Inventory $inventory) : int{ - if(isset($this->inventories[spl_object_id($inventory)])){ - throw new \InvalidArgumentException("Inventory " . get_class($inventory) . " is already tracked"); - } - $this->inventories[spl_object_id($inventory)] = new InventoryManagerEntry($inventory); $id = $this->getNewWindowId(); $this->add($id, $inventory); return $id; @@ -168,6 +164,17 @@ class InventoryManager{ } } + /** + * @param int[]|int $slotMap + * @phpstan-param array|int $slotMap + */ + private function addComplexDynamic(array|int $slotMap, Inventory $inventory) : int{ + $this->addComplex($slotMap, $inventory); + $id = $this->getNewWindowId(); + $this->associateIdWithInventory($id, $inventory); + return $id; + } + private function remove(int $id) : void{ $inventory = $this->networkIdToInventoryMap[$id]; unset($this->networkIdToInventoryMap[$id]); @@ -296,9 +303,10 @@ class InventoryManager{ $this->onCurrentWindowRemove(); $this->openWindowDeferred(function() use ($inventory) : void{ - $windowId = $this->addDynamic($inventory); if(($slotMap = $this->createComplexSlotMapping($inventory)) !== null){ - $this->addComplex($slotMap, $inventory); + $windowId = $this->addComplexDynamic($slotMap, $inventory); + }else{ + $windowId = $this->addDynamic($inventory); } foreach($this->containerOpenCallbacks as $callback){