CombinedInventory now propagates updates if its backing inventories w…

…ere directly modified this was always lacking with DoubleChestInventory and is a major factor in it being basically useless for custom use cases.
pmmp · Dec 8, 2024 · 9949e38 · 9949e38
1 parent 3ee78e2
commit 9949e38
Show file tree

Hide file tree

Showing 2 changed files with 149 additions and 2 deletions.
diff --git a/src/inventory/CombinedInventory.php b/src/inventory/CombinedInventory.php
@@ -24,8 +24,11 @@
 namespace pocketmine\inventory;
 
 use pocketmine\item\Item;
+use pocketmine\item\VanillaItems;
+use pocketmine\utils\AssumptionFailedError;
 use function array_fill_keys;
 use function array_keys;
+use function count;
 use function spl_object_id;
 
 /**
@@ -52,6 +55,9 @@ final class CombinedInventory extends BaseInventory{
 	 */
 	private array $inventoryToOffsetMap = [];
 
+	private InventoryListener $backingInventoryListener;
+	private bool $modifyingBackingInventory = false;
+
 	/**
 	 * @phpstan-param Inventory[] $backingInventories
 	 */
@@ -74,6 +80,45 @@ public function __construct(
 			$combinedSize += $size;
 		}
 		$this->size = $combinedSize;
+
+		$weakThis = \WeakReference::create($this);
+		$getThis = static fn() => $weakThis->get() ?? throw new AssumptionFailedError("Listener should've been unregistered in __destruct()");
+
+		$this->backingInventoryListener = new CallbackInventoryListener(
+			onSlotChange: static function(Inventory $inventory, int $slot, Item $oldItem) use ($getThis) : void{
+				$strongThis = $getThis();
+				if($strongThis->modifyingBackingInventory){
+					return;
+				}
+
+				$offset = $strongThis->inventoryToOffsetMap[spl_object_id($inventory)];
+				$strongThis->onSlotChange($offset + $slot, $oldItem);
+			},
+			onContentChange: static function(Inventory $inventory, array $oldContents) use ($getThis) : void{
+				$strongThis = $getThis();
+				if($strongThis->modifyingBackingInventory){
+					return;
+				}
+
+				if(count($strongThis->backingInventories) === 1){
+					$strongThis->onContentChange($oldContents);
+				}else{
+					$offset = $strongThis->inventoryToOffsetMap[spl_object_id($inventory)];
+					for($slot = 0, $limit = $inventory->getSize(); $slot < $limit; $slot++){
+						$strongThis->onSlotChange($offset + $slot, $oldContents[$slot] ?? VanillaItems::AIR());
+					}
+				}
+			}
+		);
+		foreach($this->backingInventories as $inventory){
+			$inventory->getListeners()->add($this->backingInventoryListener);
+		}
+	}
+
+	public function __destruct(){
+		foreach($this->backingInventories as $inventory){
+			$inventory->getListeners()->remove($this->backingInventoryListener);
+		}
 	}
 
 	/**
@@ -87,7 +132,14 @@ private function getInventory(int $slot) : array{
 
 	protected function internalSetItem(int $index, Item $item) : void{
 		[$inventory, $actualSlot] = $this->getInventory($index);
-		$inventory->setItem($actualSlot, $item);
+
+		//Make sure our backing listener doesn't dispatch double updates to our own listeners
+		$this->modifyingBackingInventory = true;
+		try{
+			$inventory->setItem($actualSlot, $item);
+		}finally{
+			$this->modifyingBackingInventory = false;
+		}
 	}
 
 	protected function internalSetContents(array $items) : void{
@@ -98,7 +150,14 @@ protected function internalSetContents(array $items) : void{
 		}
 		foreach($contentsByInventory as $splObjectId => $backingInventoryContents){
 			$backingInventory = $this->backingInventories[$splObjectId];
-			$backingInventory->setContents($backingInventoryContents);
+
+			//Make sure our backing listener doesn't dispatch double updates to our own listeners
+			$this->modifyingBackingInventory = true;
+			try{
+				$backingInventory->setContents($backingInventoryContents);
+			}finally{
+				$this->modifyingBackingInventory = false;
+			}
 		}
 	}
 

diff --git a/tests/phpunit/inventory/CombinedInventoryTest.php b/tests/phpunit/inventory/CombinedInventoryTest.php
@@ -177,4 +177,92 @@ public function testIsSlotEmpty() : void{
 		self::assertFalse($inventory->isSlotEmpty(1));
 		self::assertFalse($inventory->isSlotEmpty(3));
 	}
+
+	public function testListenersOnProxySlotUpdate() : void{
+		$inventory = new CombinedInventory($this->createInventories());
+
+		$numChanges = 0;
+		$inventory->getListeners()->add(new CallbackInventoryListener(
+			onSlotChange: function(Inventory $inventory, int $slot, Item $before) use (&$numChanges) : void{
+				$numChanges++;
+			},
+			onContentChange: null
+		));
+		$inventory->setItem(0, VanillaItems::DIAMOND_SWORD());
+		self::assertSame(1, $numChanges, "Inventory listener detected wrong number of changes");
+	}
+
+	public function testListenersOnProxyContentUpdate() : void{
+		$inventory = new CombinedInventory($this->createInventories());
+
+		$numChanges = 0;
+		$inventory->getListeners()->add(new CallbackInventoryListener(
+			onSlotChange: null,
+			onContentChange: function(Inventory $inventory, array $oldItems) use (&$numChanges) : void{
+				$numChanges++;
+			}
+		));
+		$inventory->setContents(self::getAltItems());
+		self::assertSame(1, $numChanges, "Expected onContentChange to be called exactly 1 time");
+	}
+
+	public function testListenersOnBackingSlotUpdate() : void{
+		$backing = $this->createInventories();
+		$inventory = new CombinedInventory($backing);
+
+		$slotChangeDetected = null;
+		$numChanges = 0;
+		$inventory->getListeners()->add(new CallbackInventoryListener(
+			onSlotChange: function(Inventory $inventory, int $slot, Item $before) use (&$slotChangeDetected, &$numChanges) : void{
+				$slotChangeDetected = $slot;
+				$numChanges++;
+			},
+			onContentChange: null
+		));
+		$backing[2]->setItem(0, VanillaItems::DIAMOND_SWORD());
+		self::assertNotNull($slotChangeDetected, "Inventory listener didn't hear about backing inventory update");
+		self::assertSame(2, $slotChangeDetected, "Inventory listener detected unexpected slot change");
+		self::assertSame(1, $numChanges, "Inventory listener detected wrong number of changes");
+	}
+
+	/**
+	 * When a combined inventory has multiple backing inventories, content updates of the backing inventories must be
+	 * turned into slot updates on the proxy, to avoid syncing the entire proxy inventory.
+	 */
+	public function testListenersOnBackingContentUpdate() : void{
+		$backing = $this->createInventories();
+		$inventory = new CombinedInventory($backing);
+
+		$slotChanges = [];
+		$inventory->getListeners()->add(new CallbackInventoryListener(
+			onSlotChange: function(Inventory $inventory, int $slot, Item $before) use (&$slotChanges) : void{
+				$slotChanges[] = $slot;
+			},
+			onContentChange: null
+		));
+		$backing[2]->setContents([VanillaItems::DIAMOND_SWORD(), VanillaItems::DIAMOND()]);
+		self::assertCount(2, $slotChanges, "Inventory listener detected wrong number of changes");
+		self::assertSame([2, 3], $slotChanges, "Incorrect slots updated");
+	}
+
+	/**
+	 * If a combined inventory has only 1 backing inventory, content updates on the backing inventory can be directly
+	 * processed as content updates on the proxy inventory without modification. This allows optimizations when only 1
+	 * backing inventory is used.
+	 * This test verifies that this special case works as expected.
+	 */
+	public function testListenersOnSingleBackingContentUpdate() : void{
+		$backing = new SimpleInventory(2);
+		$inventory = new CombinedInventory([$backing]);
+
+		$numChanges = 0;
+		$inventory->getListeners()->add(new CallbackInventoryListener(
+			onSlotChange: null,
+			onContentChange: function(Inventory $inventory, array $oldItems) use (&$numChanges) : void{
+				$numChanges++;
+			}
+		));
+		$inventory->setContents([VanillaItems::DIAMOND_SWORD(), VanillaItems::DIAMOND()]);
+		self::assertSame(1, $numChanges, "Expected onContentChange to be called exactly 1 time");
+	}
 }