Cleaned up network inventory action reading and core action creation

src/pocketmine/Player.php

@@ -80,6 +80,7 @@ use pocketmine\inventory\PlayerCursorInventory;
 use pocketmine\inventory\PlayerInventory;
 use pocketmine\inventory\ShapedRecipe;
 use pocketmine\inventory\ShapelessRecipe;
+use pocketmine\inventory\transaction\action\InventoryAction;
 use pocketmine\inventory\transaction\SimpleInventoryTransaction;
 use pocketmine\item\Item;
 use pocketmine\item\ItemFactory;
@@ -164,6 +165,7 @@ use pocketmine\resourcepacks\ResourcePack;
 use pocketmine\tile\ItemFrame;
 use pocketmine\tile\Spawnable;
 use pocketmine\tile\Tile;
+use pocketmine\utils\MainLogger;
 use pocketmine\utils\TextFormat;
 use pocketmine\utils\UUID;
 
@@ -2178,20 +2180,23 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 	 * @return bool
 	 */
 	public function handleInventoryTransaction(InventoryTransactionPacket $packet) : bool{
-		switch($packet->transactionData->transactionType){
+		/** @var InventoryAction[] $actions */
-			case InventoryTransactionPacket::TYPE_NORMAL:
+		$actions = [];
-				$transaction = new SimpleInventoryTransaction($this);
+		foreach($packet->actions as $networkInventoryAction){
-
-				foreach($packet->actions as $action){
 			try{
-						$transaction->addAction($action);
+				$action = $networkInventoryAction->createInventoryAction($this);
-					}catch(\InvalidStateException $e){
+				$actions[] = $action;
-						$this->server->getLogger()->debug($e->getMessage());
+			}catch(\Throwable $e){
+				MainLogger::getLogger()->debug("Unhandled inventory action from " . $this->getName() . ": " . $e->getMessage());
 				$this->sendAllInventories();
 				return false;
 			}
 		}
 
+		switch($packet->transactionData->transactionType){
+			case InventoryTransactionPacket::TYPE_NORMAL:
+				$transaction = new SimpleInventoryTransaction($this, $actions);
+
 				if(!$transaction->execute()){
 					foreach($transaction->getInventories() as $inventory){
 						$inventory->sendContents($this);
@@ -2763,7 +2768,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 	 * @param Item $item
 	 * @return bool if the item was dropped or if the item was null
 	 */
-	public function dropItem(Item $item){
+	public function dropItem(Item $item) : bool{
 		if(!$this->spawned or !$this->isAlive()){
 			return false;
 		}
@@ -3762,7 +3767,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 	 *
 	 * @return int
 	 */
-	public function getWindowId(Inventory $inventory){
+	public function getWindowId(Inventory $inventory) : int{
 		if($this->windows->contains($inventory)){
 			/** @var int $id */
 			$id = $this->windows[$inventory];

src/pocketmine/inventory/transaction/SimpleInventoryTransaction.php

@@ -25,7 +25,6 @@ namespace pocketmine\inventory\transaction;
 
 use pocketmine\event\inventory\InventoryTransactionEvent;
 use pocketmine\inventory\Inventory;
-use pocketmine\inventory\transaction\InventoryTransaction;
 use pocketmine\inventory\transaction\action\InventoryAction;
 use pocketmine\inventory\transaction\action\SlotChangeAction;
 use pocketmine\item\Item;
@@ -50,10 +49,14 @@ class SimpleInventoryTransaction implements InventoryTransaction{
 
 	/**
 	 * @param Player            $source
+	 * @param InventoryAction[] $actions
 	 */
-	public function __construct(Player $source = null){
+	public function __construct(Player $source = null, array $actions = []){
 		$this->creationTime = microtime(true);
 		$this->source = $source;
+		foreach($actions as $action){
+			$this->addAction($action);
+		}
 	}
 
 	/**
@@ -87,7 +90,6 @@ class SimpleInventoryTransaction implements InventoryTransaction{
 		}
 
 		if($action instanceof SlotChangeAction){
-			$action->setInventoryFrom($this->source);
 			$this->inventories[spl_object_hash($action->getInventory())] = $action->getInventory();
 		}
 

src/pocketmine/inventory/transaction/action/SlotChangeAction.php

@@ -32,47 +32,32 @@ use pocketmine\Player;
  */
 class SlotChangeAction extends InventoryAction{
 
-	/** @var Inventory|null */
+	/** @var Inventory */
 	protected $inventory;
 	/** @var int */
 	private $inventorySlot;
-	/** @var int */
-	private $containerId;
 
 	/**
+	 * @param Inventory $inventory
+	 * @param int       $inventorySlot
 	 * @param Item      $sourceItem
 	 * @param Item      $targetItem
-	 * @param int  $containerId
-	 * @param int  $inventorySlot
 	 */
-	public function __construct(Item $sourceItem, Item $targetItem, int $containerId, int $inventorySlot){
+	public function __construct(Inventory $inventory, int $inventorySlot, Item $sourceItem, Item $targetItem){
 		parent::__construct($sourceItem, $targetItem);
+		$this->inventory = $inventory;
 		$this->inventorySlot = $inventorySlot;
-		$this->containerId = $containerId;
-	}
-
-	public function getContainerId() : int{
-		return $this->containerId;
 	}
 
 	/**
-	 * Returns the inventory involved in this action. Will return null if the action has not yet been fully initialized.
+	 * Returns the inventory involved in this action.
 	 *
-	 * @return Inventory|null
+	 * @return Inventory
 	 */
-	public function getInventory(){
+	public function getInventory() : Inventory{
 		return $this->inventory;
 	}
 
-	public function setInventoryFrom(Player $player){
-		$inventory = $player->getWindow($this->containerId);
-		if($inventory === null){
-			throw new \InvalidStateException("Player " . $player->getName() . " has no open container with ID " . $this->containerId);
-		}
-
-		$this->inventory = $inventory;
-	}
-
 	/**
 	 * Returns the slot in the inventory which this action modified.
 	 * @return int

src/pocketmine/network/mcpe/protocol/InventoryTransactionPacket.php

@@ -25,12 +25,8 @@ namespace pocketmine\network\mcpe\protocol;
 
 #include <rules/DataPacket.h>
 
-use pocketmine\inventory\transaction\action\CreativeInventoryAction;
-use pocketmine\inventory\transaction\action\DropItemAction;
-use pocketmine\inventory\transaction\action\InventoryAction;
-use pocketmine\inventory\transaction\action\SlotChangeAction;
 use pocketmine\network\mcpe\NetworkSession;
-use pocketmine\network\mcpe\protocol\types\ContainerIds;
+use pocketmine\network\mcpe\protocol\types\NetworkInventoryAction;
 
 class InventoryTransactionPacket extends DataPacket{
 	const NETWORK_ID = ProtocolInfo::INVENTORY_TRANSACTION_PACKET;
@@ -92,7 +88,7 @@ class InventoryTransactionPacket extends DataPacket{
 	const ACTION_MAGIC_SLOT_CREATIVE_DELETE_ITEM = 0;
 	const ACTION_MAGIC_SLOT_CREATIVE_CREATE_ITEM = 1;
 
-	/** @var InventoryAction[] */
+	/** @var NetworkInventoryAction[] */
 	public $actions = [];
 
 	/** @var \stdClass */
@@ -147,58 +143,9 @@ class InventoryTransactionPacket extends DataPacket{
 	}
 
 	protected function decodeInventoryAction(){
-		$sourceType = $this->getUnsignedVarInt();
+		$action = new NetworkInventoryAction();
-		$containerId = ContainerIds::NONE;
+		$action->read($this);
-		$unknown = 0;
+		return $action;
-
-
-		switch($sourceType){
-			case self::SOURCE_CONTAINER:
-				$containerId = $this->getVarInt();
-				break;
-			case self::SOURCE_WORLD:
-				$unknown = $this->getUnsignedVarInt();
-				break;
-			case self::SOURCE_CREATIVE:
-				break;
-			case self::SOURCE_TODO:
-				$containerId = $this->getVarInt();
-				break;
-			default:
-				throw new \UnexpectedValueException("Unexpected inventory source type $sourceType");
-		}
-
-		$inventorySlot = $this->getUnsignedVarInt();
-		$sourceItem = $this->getSlot();
-		$targetItem = $this->getSlot();
-
-		switch($sourceType){
-			case self::SOURCE_CONTAINER:
-				if($containerId === ContainerIds::ARMOR){
-					//TODO: HACK!
-					$inventorySlot += 36;
-					$containerId = ContainerIds::INVENTORY;
-				}
-				return new SlotChangeAction($sourceItem, $targetItem, $containerId, $inventorySlot);
-			case self::SOURCE_WORLD:
-				if($inventorySlot !== self::ACTION_MAGIC_SLOT_DROP_ITEM){
-					throw new \UnexpectedValueException("Only expect drop item world action types from client");
-				}
-
-				return new DropItemAction($sourceItem, $targetItem);
-			case self::SOURCE_CREATIVE:
-				if($inventorySlot === self::ACTION_MAGIC_SLOT_CREATIVE_DELETE_ITEM){
-					return new CreativeInventoryAction($sourceItem, $targetItem, CreativeInventoryAction::TYPE_DELETE_ITEM);
-				}elseif($inventorySlot === self::ACTION_MAGIC_SLOT_CREATIVE_CREATE_ITEM){
-					return new CreativeInventoryAction($sourceItem, $targetItem, CreativeInventoryAction::TYPE_CREATE_ITEM);
-				}else{
-					throw new \UnexpectedValueException("Unknown creative inventory action type $inventorySlot");
-				}
-			case self::SOURCE_TODO:
-				return new SlotChangeAction($sourceItem, $targetItem, $containerId, $inventorySlot);
-			default:
-				throw new \UnexpectedValueException("Unknown source type $sourceType");
-		}
 	}
 
 	public function handle(NetworkSession $session) : bool{

src/pocketmine/network/mcpe/protocol/types/NetworkInventoryAction.php

@@ -0,0 +1,135 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\network\mcpe\protocol\types;
+
+use pocketmine\inventory\transaction\action\CreativeInventoryAction;
+use pocketmine\inventory\transaction\action\DropItemAction;
+use pocketmine\inventory\transaction\action\SlotChangeAction;
+use pocketmine\item\Item;
+use pocketmine\network\mcpe\protocol\InventoryTransactionPacket;
+use pocketmine\Player;
+
+class NetworkInventoryAction{
+	/** @var int */
+	public $sourceType;
+	/** @var int */
+	public $windowId = ContainerIds::NONE;
+	/** @var int */
+	public $unknown = 0;
+	/** @var int */
+	public $inventorySlot;
+	/** @var Item */
+	public $oldItem;
+	/** @var Item */
+	public $newItem;
+
+	/**
+	 * @param InventoryTransactionPacket $packet
+	 */
+	public function read(InventoryTransactionPacket $packet){
+		$this->sourceType = $packet->getUnsignedVarInt();
+
+		switch($this->sourceType){
+			case InventoryTransactionPacket::SOURCE_CONTAINER:
+				$this->windowId = $packet->getVarInt();
+				break;
+			case InventoryTransactionPacket::SOURCE_WORLD:
+				$this->unknown = $packet->getUnsignedVarInt();
+				break;
+			case InventoryTransactionPacket::SOURCE_CREATIVE:
+				break;
+			case InventoryTransactionPacket::SOURCE_TODO:
+				$this->windowId = $packet->getVarInt();
+				break;
+		}
+
+		$this->inventorySlot = $packet->getUnsignedVarInt();
+		$this->oldItem = $packet->getSlot();
+		$this->newItem = $packet->getSlot();
+	}
+
+	/**
+	 * @param InventoryTransactionPacket $packet
+	 */
+	public function write(InventoryTransactionPacket $packet){
+		$packet->putUnsignedVarInt($this->sourceType);
+
+		switch($this->sourceType){
+			case InventoryTransactionPacket::SOURCE_CONTAINER:
+				$packet->putVarInt($this->windowId);
+				break;
+			case InventoryTransactionPacket::SOURCE_WORLD:
+				$packet->putUnsignedVarInt($this->unknown);
+				break;
+			case InventoryTransactionPacket::SOURCE_CREATIVE:
+				break;
+			case InventoryTransactionPacket::SOURCE_TODO:
+				$packet->putVarInt($this->windowId);
+				break;
+		}
+
+		$packet->putUnsignedVarInt($this->inventorySlot);
+		$packet->putSlot($this->oldItem);
+		$packet->putSlot($this->newItem);
+	}
+
+	public function createInventoryAction(Player $player){
+		switch($this->sourceType){
+			case InventoryTransactionPacket::SOURCE_CONTAINER:
+				if($this->windowId === ContainerIds::ARMOR){
+					//TODO: HACK!
+					$this->inventorySlot += 36;
+					$this->windowId = ContainerIds::INVENTORY;
+				}
+
+				$window = $player->getWindow($this->windowId);
+				if($window !== null){
+					return new SlotChangeAction($player->getWindow($this->windowId), $this->inventorySlot, $this->oldItem, $this->newItem);
+				}
+
+				throw new \InvalidStateException("Player " . $player->getName() . " has no open container with window ID $this->windowId");
+			case InventoryTransactionPacket::SOURCE_WORLD:
+				if($this->inventorySlot !== InventoryTransactionPacket::ACTION_MAGIC_SLOT_DROP_ITEM){
+					throw new \UnexpectedValueException("Only expecting drop-item world actions from the client!");
+				}
+
+				return new DropItemAction($this->oldItem, $this->newItem);
+			case InventoryTransactionPacket::SOURCE_CREATIVE:
+				switch($this->inventorySlot){
+					case InventoryTransactionPacket::ACTION_MAGIC_SLOT_CREATIVE_DELETE_ITEM:
+						return new CreativeInventoryAction($this->oldItem, $this->newItem, CreativeInventoryAction::TYPE_DELETE_ITEM);
+					case InventoryTransactionPacket::ACTION_MAGIC_SLOT_CREATIVE_CREATE_ITEM:
+						return new CreativeInventoryAction($this->oldItem, $this->newItem, CreativeInventoryAction::TYPE_CREATE_ITEM);
+				}
+
+				throw new \UnexpectedValueException("Unexpected creative action type $this->inventorySlot");
+			case InventoryTransactionPacket::SOURCE_TODO:
+				//TODO
+				throw new \UnexpectedValueException("Player " . $player->getName() . " has no open container with window ID $this->windowId");
+			default:
+				throw new \UnexpectedValueException("Unknown inventory source type $this->sourceType");
+		}
+	}
+
+}