Hotbar/inventory bugfixes (#399)

- Fixed most issues with item equipment in creative
- Added save and restore of currently-held item
- Reset hotbar on death, added API method PlayerInventory->resetHotbar()
- Creative players now have more leeway to get items, alleviates issues with item equipment in desktop GUI
- Fixed creative players wearing armour
- Found unknown field in ContainerSetSlotPacket
- Removed outdated/redundant constants
- Use a case statement in ContainerSetSlotPacket handler, added handling for 0x7a hotbar slot link update

src/pocketmine/Player.php

@@ -152,9 +152,6 @@ class Player extends Human implements CommandSender, InventoryHolder, ChunkLoade
 	const SPECTATOR = 3;
 	const VIEW = Player::SPECTATOR;
 
-	const SURVIVAL_SLOTS = 36;
-	const CREATIVE_SLOTS = 112;
-
 	/** @var SourceInterface */
 	protected $interface;
 
@@ -804,6 +801,7 @@ class Player extends Human implements CommandSender, InventoryHolder, ChunkLoade
 		$this->sendData($this);
 		$this->inventory->sendContents($this);
 		$this->inventory->sendArmorContents($this);
+		$this->inventory->sendHeldItem($this);
 
 		$pk = new SetTimePacket();
 		$pk->time = $this->level->getTime();
@@ -1756,12 +1754,6 @@ class Player extends Human implements CommandSender, InventoryHolder, ChunkLoade
 			return;
 		}
 
-		if($this->isCreative()){
-			$this->inventory->setHeldItemSlot(0);
-		}else{
-			$this->inventory->setHeldItemSlot($this->inventory->getHotbarSlotIndex(0));
-		}
-
 		$this->dataPacket(new ResourcePacksInfoPacket());
 
 		if(!$this->hasValidSpawnPosition() and isset($this->namedtag->SpawnLevel) and ($level = $this->server->getLevelByName($this->namedtag["SpawnLevel"])) instanceof Level){
@@ -1987,64 +1979,20 @@ class Player extends Human implements CommandSender, InventoryHolder, ChunkLoade
 					break;
 				}
 
-				if($packet->slot === 0x28 or $packet->slot === 0 or $packet->slot === 255){ //0 for 0.8.0 compatibility
+				if($packet->inventorySlot === 255){
-					$packet->slot = -1; //Air
+					$packet->inventorySlot = -1; //Cleared slot
 				}else{
-					$packet->slot -= 9; //Get real block slot
+					$packet->inventorySlot -= 9; //Get real inventory slot
-				}
+					$item = $this->inventory->getItem($packet->inventorySlot);
 
-				/** @var Item $item */
+					if(!$item->equals($packet->item)){
-				$item = null;
-
-				if($this->isCreative()){ //Creative mode match
-					$item = $packet->item;
-					$slot = Item::getCreativeItemIndex($item);
-				}else{
-					$item = $this->inventory->getItem($packet->slot);
-					$slot = $packet->slot;
-				}
-
-				if($packet->slot === -1){ //Air
-					if($this->isCreative()){
-						$found = false;
-						for($i = 0; $i < $this->inventory->getHotbarSize(); ++$i){
-							if($this->inventory->getHotbarSlotIndex($i) === -1){
-								$this->inventory->setHeldItemIndex($i);
-								$found = true;
-								break;
-							}
-						}
-
-						if(!$found){ //couldn't find a empty slot (error)
-							$this->inventory->sendContents($this);
-							break;
-						}
-					}else{
-						if($packet->selectedSlot >= 0 and $packet->selectedSlot < 9){
-							$this->inventory->setHeldItemIndex($packet->selectedSlot, false);
-							$this->inventory->setHeldItemSlot($packet->slot);
-						}else{
-							$this->inventory->sendContents($this);
-							break;
-						}
-					}
-				}elseif($item === null or $slot === -1 or !$item->deepEquals($packet->item)){ // packet error or not implemented
-					$this->inventory->sendContents($this);
-					break;
-				}elseif($this->isCreative()){
-					$this->inventory->setHeldItemIndex($packet->selectedSlot, false);
-					$this->inventory->setItem($packet->selectedSlot, $item);
-					$this->inventory->setHeldItemSlot($packet->selectedSlot);
-				}else{
-					if($packet->selectedSlot >= 0 and $packet->selectedSlot < $this->inventory->getHotbarSize()){
-						$this->inventory->setHeldItemIndex($packet->selectedSlot, false);
-						$this->inventory->setHeldItemSlot($slot);
-					}else{
 						$this->inventory->sendContents($this);
 						break;
 					}
 				}
 
+				$this->inventory->equipItem($packet->hotbarSlot, $packet->inventorySlot);
+
 				$this->setDataFlag(self::DATA_FLAGS, self::DATA_FLAG_ACTION, false);
 				break;
 			case ProtocolInfo::USE_ITEM_PACKET:
@@ -2854,33 +2802,38 @@ class Player extends Human implements CommandSender, InventoryHolder, ChunkLoade
 					break;
 				}
 
-				if($packet->windowid === 0){ //Our inventory
+				switch($packet->windowid){
-					if($packet->slot >= $this->inventory->getSize()){
+					case ContainerSetContentPacket::SPECIAL_INVENTORY: //Normal inventory change
-						break;
+						if($packet->slot >= $this->inventory->getSize()){
-					}
+							break 2;
-					if($this->isCreative()){
-						if(Item::getCreativeItemIndex($packet->item) !== -1){
-							$this->inventory->setItem($packet->slot, $packet->item);
-							$this->inventory->setHotbarSlotIndex($packet->slot, $packet->slot); //links $hotbar[$packet->slot] to $slots[$packet->slot]
 						}
-					}
-					$transaction = new BaseTransaction($this->inventory, $packet->slot, $this->inventory->getItem($packet->slot), $packet->item);
-				}elseif($packet->windowid === ContainerSetContentPacket::SPECIAL_ARMOR){ //Our armor
-					if($packet->slot >= 4){
-						break;
-					}
 
-					$transaction = new BaseTransaction($this->inventory, $packet->slot + $this->inventory->getSize(), $this->inventory->getArmorItem($packet->slot), $packet->item);
+						$transaction = new BaseTransaction($this->inventory, $packet->slot, $this->inventory->getItem($packet->slot), $packet->item);
-				}elseif(isset($this->windowIndex[$packet->windowid])){
+						break;
-					$this->craftingType = 0;
+					case ContainerSetContentPacket::SPECIAL_ARMOR: //Armour change
-					$inv = $this->windowIndex[$packet->windowid];
+						if($packet->slot >= 4){
-					$transaction = new BaseTransaction($inv, $packet->slot, $inv->getItem($packet->slot), $packet->item);
+							break 2;
-				}else{
+						}
-					break;
+
+						$transaction = new BaseTransaction($this->inventory, $packet->slot + $this->inventory->getSize(), $this->inventory->getArmorItem($packet->slot), $packet->item);
+						break;
+					case ContainerSetContentPacket::SPECIAL_HOTBAR: //Hotbar link update
+						//hotbarSlot 0-8, slot 9-44
+						$this->inventory->setHotbarSlotIndex($packet->hotbarSlot, $packet->slot - 9);
+						break 2;
+					default:
+						if(!isset($this->windowIndex[$packet->windowid])){
+							break 2; //unknown windowID and/or not matching any open windows
+						}
+
+						$this->craftingType = 0;
+						$inv = $this->windowIndex[$packet->windowid];
+						$transaction = new BaseTransaction($inv, $packet->slot, $inv->getItem($packet->slot), $packet->item);
+						break;
 				}
 
 				if($transaction->getSourceItem()->deepEquals($transaction->getTargetItem()) and $transaction->getTargetItem()->getCount() === $transaction->getSourceItem()->getCount()){ //No changes!
-					//No changes, just a local inventory update sent by the server
+					//No changes, just a local inventory update sent by the client
 					break;
 				}
 
@@ -3372,6 +3325,8 @@ class Player extends Human implements CommandSender, InventoryHolder, ChunkLoade
 
 			if($this->inventory !== null){
 				$this->inventory->clearAll();
+				$this->inventory->setHeldItemIndex(0);
+				$this->inventory->resetHotbar(true);
 			}
 		}
 

src/pocketmine/entity/Human.php

@@ -285,6 +285,12 @@ class Human extends Creature implements ProjectileSource, InventoryHolder{
 			}
 		}
 
+		if(isset($this->namedtag->SelectedInventorySlot) and $this->namedtag->SelectedInventorySlot instanceof IntTag){
+			$this->inventory->setHeldItemIndex($this->namedtag->SelectedInventorySlot->getValue(), false);
+		}else{
+			$this->inventory->setHeldItemIndex(0, false);
+		}
+
 		parent::initEntity();
 
 		if(!isset($this->namedtag->foodLevel) or !($this->namedtag->foodLevel instanceof IntTag)){
@@ -428,9 +434,8 @@ class Human extends Creature implements ProjectileSource, InventoryHolder{
 			}
 
 			//Normal inventory
-			$slotCount = Player::SURVIVAL_SLOTS + 9;
+			$slotCount = $this->inventory->getSize() + $this->inventory->getHotbarSize();
-			//$slotCount = (($this instanceof Player and ($this->gamemode & 0x01) === 1) ? Player::CREATIVE_SLOTS : Player::SURVIVAL_SLOTS) + 9;
+			for($slot = $this->inventory->getHotbarSize(); $slot < $slotCount; ++$slot){
-			for($slot = 9; $slot < $slotCount; ++$slot){
 				$item = $this->inventory->getItem($slot - 9);
 				$this->namedtag->Inventory[$slot] = $item->nbtSerialize($slot);
 			}
@@ -442,6 +447,8 @@ class Human extends Creature implements ProjectileSource, InventoryHolder{
 					$this->namedtag->Inventory[$slot] = $item->nbtSerialize($slot);
 				}
 			}
+
+			$this->namedtag->SelectedInventorySlot = new IntTag("SelectedInventorySlot", $this->inventory->getHeldItemIndex());
 		}
 
 		if(strlen($this->getSkinData()) > 0){

src/pocketmine/inventory/PlayerInventory.php

@@ -40,7 +40,7 @@ class PlayerInventory extends BaseInventory{
 	protected $hotbar;
 
 	public function __construct(Human $player){
-		$this->hotbar = range(0, $this->getHotbarSize() - 1, 1);
+		$this->resetHotbar(false);
 		parent::__construct($player, InventoryType::get(InventoryType::PLAYER));
 	}
 
@@ -53,20 +53,93 @@ class PlayerInventory extends BaseInventory{
 		$this->sendContents($this->getViewers());
 	}
 
-	public function getHotbarSlotIndex($index){
+	/**
-		return ($index >= 0 and $index < $this->getHotbarSize()) ? $this->hotbar[$index] : -1;
+	 * Called when a client equips a hotbar slot. This method should not be used by plugins.
+	 * This method will call PlayerItemHeldEvent.
+	 *
+	 * @param int      $hotbarSlot Number of the hotbar slot to equip.
+	 * @param int|null $inventorySlot Inventory slot to map to the specified hotbar slot. Supply null to make no change to the link.
+	 *
+	 * @return bool if the equipment change was successful, false if not.
+	 */
+	public function equipItem(int $hotbarSlot, $inventorySlot = null) : bool{
+		if($inventorySlot === null){
+			$inventorySlot = $this->getHotbarSlotIndex($this->getHeldItemIndex());
+		}
+		if($hotbarSlot < 0 or $hotbarSlot >= $this->getHotbarSize() or $inventorySlot < -1 or $inventorySlot >= $this->getSize()){
+			$this->sendContents($this->getHolder());
+			return false;
+		}
+
+		$this->getHolder()->getLevel()->getServer()->getPluginManager()->callEvent($ev = new PlayerItemHeldEvent($this->getHolder(), $this->getItem($inventorySlot), $inventorySlot, $hotbarSlot));
+		if($ev->isCancelled()){
+			$this->sendContents($this->getHolder());
+			return false;
+		}
+
+		$this->setHotbarSlotIndex($hotbarSlot, $inventorySlot);
+		$this->setHeldItemIndex($hotbarSlot, false);
+
+		return true;
 	}
 
-	public function setHotbarSlotIndex($index, $slot){
+	/**
-		if($index >= 0 and $index < $this->getHotbarSize() and $slot >= -1 and $slot < $this->getSize()){
+	 * Returns the index of the inventory slot mapped to the specified hotbar slot, or -1 if the hotbar slot does not exist.
-			$this->hotbar[$index] = $slot;
+	 * @param int $index
+	 *
+	 * @return int
+	 */
+	public function getHotbarSlotIndex($index){
+		return $this->hotbar[$index] ?? -1;
+	}
+
+	/**
+	 * @internal This method is intended for use in network interaction with clients only.
+	 * @deprecated Do not change hotbar slot mapping with plugins, this will cause myriad client-sided bugs, especially with desktop GUI clients.
+	 *
+	 * @param int $hotbarSlot
+	 * @param int $inventorySlot
+	 */
+	public function setHotbarSlotIndex($hotbarSlot, $inventorySlot){
+		if($hotbarSlot >= 0 and $hotbarSlot < $this->getHotbarSize() and $inventorySlot >= -1 and $inventorySlot < $this->getSize()){
+			if($inventorySlot !== -1 and ($alreadyEquippedIndex = array_search($inventorySlot, $this->hotbar)) !== false){
+				/* Swap the slots
+				 * This assumes that the equipped slot can only be equipped in one other slot
+				 * it will not account for ancient bugs where the same slot ended up linked to several hotbar slots.
+				 * Such bugs will require a hotbar reset to default.
+				 */
+				$this->hotbar[$alreadyEquippedIndex] = $this->hotbar[$hotbarSlot];
+			}
+			$this->hotbar[$hotbarSlot] = $inventorySlot;
 		}
 	}
 
+	/**
+	 * Resets hotbar links to their original defaults.
+	 * @param bool $send Whether to send changes to the holder.
+	 */
+	public function resetHotbar(bool $send = true){
+		$this->hotbar = range(0, $this->getHotbarSize() - 1, 1);
+		if($send){
+			$this->sendContents($this->getHolder());
+		}
+	}
+
+	/**
+	 * Returns the hotbar slot number the holder is currently holding.
+	 * @return int
+	 */
 	public function getHeldItemIndex(){
 		return $this->itemInHandIndex;
 	}
 
+	/**
+	 * Sets which hotbar slot the player is currently loading.
+	 *
+	 * @param int  $index 0-8 index of the hotbar slot to hold
+	 * @param bool $send  Whether to send updates back to the inventory holder. This should usually be true for plugin calls.
+	 *                    It should only be false to prevent feedback loops of equipment packets between client and server.
+	 */
 	public function setHeldItemIndex($index, $send = true){
 		if($index >= 0 and $index < $this->getHotbarSize()){
 			$this->itemInHandIndex = $index;
@@ -79,6 +152,11 @@ class PlayerInventory extends BaseInventory{
 		}
 	}
 
+	/**
+	 * Returns the currently-held item.
+	 *
+	 * @return Item
+	 */
 	public function getItemInHand(){
 		$item = $this->getItem($this->getHeldItemSlot());
 		if($item instanceof Item){
@@ -89,6 +167,7 @@ class PlayerInventory extends BaseInventory{
 	}
 
 	/**
+	 * Sets the item in the currently-held slot to the specified item.
 	 * @param Item $item
 	 *
 	 * @return bool
@@ -97,29 +176,29 @@ class PlayerInventory extends BaseInventory{
 		return $this->setItem($this->getHeldItemSlot(), $item);
 	}
 
+	/**
+	 * Returns the hotbar slot number currently held.
+	 *
+	 * @return int
+	 */
 	public function getHeldItemSlot(){
 		return $this->getHotbarSlotIndex($this->itemInHandIndex);
 	}
 
+	/**
+	 * Sets the hotbar slot link of the currently-held hotbar slot.
+	 * @deprecated Do not change hotbar slot mapping with plugins, this will cause myriad client-sided bugs, especially with desktop GUI clients.
+	 *
+	 * @param int $slot
+	 */
 	public function setHeldItemSlot($slot){
 		if($slot >= -1 and $slot < $this->getSize()){
-			$item = $this->getItem($slot);
+			$this->setHotbarSlotIndex($this->getHeldItemIndex(), $slot);
-
-			$itemIndex = $this->getHeldItemIndex();
-
-			if($this->getHolder() instanceof Player){
-				Server::getInstance()->getPluginManager()->callEvent($ev = new PlayerItemHeldEvent($this->getHolder(), $item, $slot, $itemIndex));
-				if($ev->isCancelled()){
-					$this->sendContents($this->getHolder());
-					return;
-				}
-			}
-
-			$this->setHotbarSlotIndex($itemIndex, $slot);
 		}
 	}
 
 	/**
+	 * Sends the currently-held item to specified targets.
 	 * @param Player|Player[] $target
 	 */
 	public function sendHeldItem($target){
@@ -128,8 +207,8 @@ class PlayerInventory extends BaseInventory{
 		$pk = new MobEquipmentPacket();
 		$pk->eid = $this->getHolder()->getId();
 		$pk->item = $item;
-		$pk->slot = $this->getHeldItemSlot();
+		$pk->inventorySlot = $this->getHeldItemSlot();
-		$pk->selectedSlot = $this->getHeldItemIndex();
+		$pk->hotbarSlot = $this->getHeldItemIndex();
 
 		if(!is_array($target)){
 			$target->dataPacket($pk);
@@ -161,6 +240,10 @@ class PlayerInventory extends BaseInventory{
 		}
 	}
 
+	/**
+	 * Returns the number of slots in the hotbar.
+	 * @return int
+	 */
 	public function getHotbarSize(){
 		return 9;
 	}

src/pocketmine/inventory/SimpleTransactionGroup.php

@@ -130,7 +130,21 @@ class SimpleTransactionGroup implements TransactionGroup{
 		$haveItems = [];
 		$needItems = [];
 
-		return $this->matchItems($haveItems, $needItems) and count($haveItems) === 0 and count($needItems) === 0 and count($this->transactions) > 0;
+		if($this->matchItems($needItems, $haveItems) and count($this->transactions) > 0){
+			if(count($haveItems) === 0 and count($needItems) === 0){
+				return true;
+			}elseif($this->source->isCreative(true) and count($needItems) > 0){ //Added items from creative inventory
+				foreach($needItems as $item){
+					if(Item::getCreativeItemIndex($item) === -1 and $item->getId() !== Item::AIR){
+						return false;
+					}
+				}
+
+				return true;
+			}
+		}
+
+		return false;
 	}
 
 	public function execute(){

src/pocketmine/network/protocol/ContainerSetSlotPacket.php

@@ -33,14 +33,14 @@ class ContainerSetSlotPacket extends DataPacket{
 	public $hotbarSlot;
 	/** @var Item */
 	public $item;
-	public $unknown;
+	public $selectSlot;
 
 	public function decode(){
 		$this->windowid = $this->getByte();
 		$this->slot = $this->getVarInt();
 		$this->hotbarSlot = $this->getVarInt();
 		$this->item = $this->getSlot();
-		$this->unknown = $this->getByte();
+		$this->selectSlot = $this->getByte();
 	}
 
 	public function encode(){
@@ -49,7 +49,7 @@ class ContainerSetSlotPacket extends DataPacket{
 		$this->putVarInt($this->slot);
 		$this->putVarInt($this->hotbarSlot);
 		$this->putSlot($this->item);
-		$this->putByte($this->unknown);
+		$this->putByte($this->selectSlot);
 	}
 
 }

src/pocketmine/network/protocol/MobEquipmentPacket.php

@@ -29,15 +29,15 @@ class MobEquipmentPacket extends DataPacket{
 
 	public $eid;
 	public $item;
-	public $slot;
+	public $inventorySlot;
-	public $selectedSlot;
+	public $hotbarSlot;
 	public $unknownByte;
 
 	public function decode(){
 		$this->eid = $this->getEntityId(); //EntityRuntimeID
 		$this->item = $this->getSlot();
-		$this->slot = $this->getByte();
+		$this->inventorySlot = $this->getByte();
-		$this->selectedSlot = $this->getByte();
+		$this->hotbarSlot = $this->getByte();
 		$this->unknownByte = $this->getByte();
 	}
 
@@ -45,8 +45,8 @@ class MobEquipmentPacket extends DataPacket{
 		$this->reset();
 		$this->putEntityId($this->eid); //EntityRuntimeID
 		$this->putSlot($this->item);
-		$this->putByte($this->slot);
+		$this->putByte($this->inventorySlot);
-		$this->putByte($this->selectedSlot);
+		$this->putByte($this->hotbarSlot);
 		$this->putByte($this->unknownByte);
 	}