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

@@ -69,16 +69,17 @@ use function spl_object_id;
  * @phpstan-type ContainerOpenClosure \Closure(int $id, Inventory $inventory) : (list<ClientboundPacket>|null)
  */
 class InventoryManager{
+	/**
+	 * @var InventoryManagerEntry[] spl_object_id(Inventory) => InventoryManagerEntry
+	 * @phpstan-var array<int, InventoryManagerEntry>
+	 */
+	private array $inventories = [];
+
 	/**
 	 * @var Inventory[] network window ID => Inventory
 	 * @phpstan-var array<int, Inventory>
 	 */
 	private array $networkIdToInventoryMap = [];
-	/**
-	 * @var ComplexInventoryMapEntry[] spl_object_id(Inventory) => ComplexWindowMapEntry
-	 * @phpstan-var array<int, ComplexInventoryMapEntry>
-	 */
-	private array $complexInventorySlotMaps = [];
 	/**
 	 * @var ComplexInventoryMapEntry[] net slot ID => ComplexWindowMapEntry
 	 * @phpstan-var array<int, ComplexInventoryMapEntry>
@@ -87,11 +88,6 @@ class InventoryManager{
 
 	private int $lastInventoryNetworkId = ContainerIds::FIRST;
 
-	/**
-	 * @var Item[][]
-	 * @phpstan-var array<int, InventoryManagerPredictedChanges>
-	 */
-	private array $initiatedSlotChanges = [];
 	private int $clientSelectedHotbarSlot = -1;
 
 	/** @phpstan-var ObjectSet<ContainerOpenClosure> */
@@ -104,12 +100,6 @@ class InventoryManager{
 	private int $nextItemStackId = 1;
 	private ?int $currentItemStackRequestId = null;
 
-	/**
-	 * @var int[][]
-	 * @phpstan-var array<int, array<int, ItemStackInfo>>
-	 */
-	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->inventories[spl_object_id($inventory)]);
-			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->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));
+		$this->inventories[spl_object_id($inventory)]->predictions[$slot] = $item;
-		$predictions->add($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;
+		$inventoryEntry = $this->inventories[spl_object_id($inventory)];
-		$clientSideItem = $predictions?->getSlot($slot);
+		$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($entry->complexSlotMap !== 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");
+			$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;
+		unset($entry->predictions[$slot]);
-		$predictions?->remove($slot);
 	}
 
 	public function syncContents(Inventory $inventory) : void{
-		$slotMap = $this->complexInventorySlotMaps[spl_object_id($inventory)] ?? null;
+		$entry = $this->inventories[spl_object_id($inventory)];
-		if($slotMap !== null){
+		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){
+		foreach($this->inventories as $entry){
-			$this->syncContents($inventory);
+			$this->syncContents($entry->inventory);
-		}
-		foreach($this->complexInventorySlotMaps as $entry){
-			$this->syncContents($entry->getInventory());
 		}
 	}
 
 	public function syncMismatchedPredictedSlotChanges() : void{
-		foreach($this->initiatedSlotChanges as $predictions){
+		foreach($this->inventories as $entry){
-			$inventory = $predictions->getInventory();
+			$inventory = $entry->inventory;
-			foreach($predictions->getSlots() as $slot => $expectedItem){
+			foreach($entry->predictions as $slot => $expectedItem){
-				if(!$inventory->slotExists($slot) || $this->getItemStackInfo($inventory, $slot) === null){
+				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;
 	}
 }

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<int, ItemStack>
 	 */
-	private array $slots = [];
+	public array $predictions;
-
-	public function __construct(
-		private Inventory $inventory
-	){}
-
-	public function getInventory() : Inventory{ return $this->inventory; }
 
 	/**
-	 * @return ItemStack[]
+	 * @var ItemStackInfo[]
-	 * @phpstan-return array<int, ItemStack>
+	 * @phpstan-var array<int, ItemStackInfo>
 	 */
-	public function getSlots() : array{
+	public array $itemStackInfos = [];
-		return $this->slots;
-	}
 
-	public function getSlot(int $slot) : ?ItemStack{
+	public function __construct(
-		return $this->slots[$slot] ?? null;
+		public Inventory $inventory,
-	}
+		public ?ComplexInventoryMapEntry $complexSlotMap = null
-
+	){}
-	public function add(int $slot, ItemStack $item) : void{
-		$this->slots[$slot] = $item;
-	}
-
-	public function remove(int $slot) : void{
-		unset($this->slots[$slot]);
-	}
 }