From 1420cd1fa5fc94356acb800d5dcd6252457e1355 Mon Sep 17 00:00:00 2001
From: "Dylan K. Taylor" <odigiman@gmail.com>
Date: Thu, 29 Mar 2018 11:21:48 +0100
Subject: [PATCH] InventoryTransaction: Added a more robust action flattening
 mechanism This now handles the case where there are multiple options which
 could be taken, and opts for the first result which successfully ties all the
 actions together. Previously it would be entirely down to chance (ordering)
 whether the actions would get ordered successfully.

---
 .../transaction/InventoryTransaction.php      | 82 ++++++++++---------
 1 file changed, 45 insertions(+), 37 deletions(-)

diff --git a/src/pocketmine/inventory/transaction/InventoryTransaction.php b/src/pocketmine/inventory/transaction/InventoryTransaction.php
index f708be992..acb5af902 100644
--- a/src/pocketmine/inventory/transaction/InventoryTransaction.php
+++ b/src/pocketmine/inventory/transaction/InventoryTransaction.php
@@ -165,65 +165,73 @@ class InventoryTransaction{
 	protected function squashDuplicateSlotChanges() : bool{
 		/** @var SlotChangeAction[][] $slotChanges */
 		$slotChanges = [];
+		/** @var Inventory[] $inventories */
+		$inventories = [];
+		/** @var int[] $slots */
+		$slots = [];
+
 		foreach($this->actions as $key => $action){
 			if($action instanceof SlotChangeAction){
-				$slotChanges[spl_object_hash($action->getInventory()) . "@" . $action->getSlot()][] = $action;
+				$slotChanges[$h = (spl_object_hash($action->getInventory()) . "@" . $action->getSlot())][] = $action;
+				$inventories[$h] = $action->getInventory();
+				$slots[$h] = $action->getSlot();
 			}
 		}
 
 		foreach($slotChanges as $hash => $list){
 			if(count($list) === 1){ //No need to compact slot changes if there is only one on this slot
-				unset($slotChanges[$hash]);
 				continue;
 			}
 
-			$originalList = $list;
+			$inventory = $inventories[$hash];
+			$slot = $slots[$hash];
+			$sourceItem = $inventory->getItem($slot);
 
-			/** @var SlotChangeAction|null $originalAction */
-			$originalAction = null;
-			/** @var Item|null $lastTargetItem */
-			$lastTargetItem = null;
-
-			foreach($list as $i => $action){
-				if($action->isValid($this->source)){
-					$originalAction = $action;
-					$lastTargetItem = $action->getTargetItem();
-					unset($list[$i]);
-					break;
-				}
-			}
-
-			if($originalAction === null){
-				return false; //Couldn't find any actions that had a source-item matching the current inventory slot
-			}
-
-			do{
-				$sortedThisLoop = 0;
-				foreach($list as $i => $action){
-					$actionSource = $action->getSourceItem();
-					if($actionSource->equalsExact($lastTargetItem)){
-						$lastTargetItem = $action->getTargetItem();
-						unset($list[$i]);
-						$sortedThisLoop++;
-					}
-				}
-			}while($sortedThisLoop > 0);
-
-			if(count($list) > 0){ //couldn't chain all the actions together
-				MainLogger::getLogger()->debug("Failed to compact " . count($originalList) . " actions for " . $this->source->getName());
+			$targetItem = $this->findResultItem($sourceItem, $list);
+			if($targetItem === null){
+				MainLogger::getLogger()->debug("Failed to compact " . count($list) . " actions for " . $this->source->getName());
 				return false;
 			}
 
-			foreach($originalList as $action){
+			foreach($list as $action){
 				unset($this->actions[spl_object_hash($action)]);
 			}
 
-			$this->addAction(new SlotChangeAction($originalAction->getInventory(), $originalAction->getSlot(), $originalAction->getSourceItem(), $lastTargetItem));
+			if(!$targetItem->equalsExact($sourceItem)){
+				//sometimes we get actions on the crafting grid whose source and target items are the same, so dump them
+				$this->addAction(new SlotChangeAction($inventory, $slot, $sourceItem, $targetItem));
+			}
 		}
 
 		return true;
 	}
 
+	/**
+	 * @param Item               $needOrigin
+	 * @param SlotChangeAction[] $possibleActions
+	 *
+	 * @return null|Item
+	 */
+	protected function findResultItem(Item $needOrigin, array $possibleActions) : ?Item{
+		assert(!empty($possibleActions));
+
+		foreach($possibleActions as $i => $action){
+			if($action->getSourceItem()->equalsExact($needOrigin)){
+				$newList = $possibleActions;
+				unset($newList[$i]);
+				if(empty($newList)){
+					return $action->getTargetItem();
+				}
+				$result = $this->findResultItem($action->getTargetItem(), $newList);
+				if($result !== null){
+					return $result;
+				}
+			}
+		}
+
+		return null;
+	}
+
 	/**
 	 * @return bool
 	 */