Workaround for slot IDs not changing client side when old item == new item

this is a really dumb bug and seems similar to the armor bug I fixed a while ago.

fixes #5987

it's unlikely that #5727 will be solved by this, but one can hope...

src/network/mcpe/InventoryManager.php

@@ -59,6 +59,7 @@ use pocketmine\network\PacketHandlingException;
 use pocketmine\player\Player;
 use pocketmine\utils\AssumptionFailedError;
 use pocketmine\utils\ObjectSet;
+use function array_fill_keys;
 use function array_keys;
 use function array_search;
 use function count;
@@ -429,6 +430,50 @@ class InventoryManager{
 		unset($inventoryEntry->predictions[$slot]);
 	}
 
+	private function sendInventorySlotPackets(int $windowId, int $netSlot, ItemStackWrapper $itemStackWrapper) : void{
+		/*
+		 * TODO: HACK!
+		 * As of 1.20.12, the client ignores change of itemstackID in some cases when the old item == the new item.
+		 * Notably, this happens with armor, offhand and enchanting tables, but not with main inventory.
+		 * While we could track the items previously sent to the client, that's a waste of memory and would
+		 * cost performance. Instead, clear the slot(s) first, then send the new item(s).
+		 * The network cost of doing this is fortunately minimal, as an air itemstack is only 1 byte.
+		 */
+		if($itemStackWrapper->getStackId() !== 0){
+			$this->session->sendDataPacket(InventorySlotPacket::create(
+				$windowId,
+				$netSlot,
+				new ItemStackWrapper(0, ItemStack::null())
+			));
+		}
+		//now send the real contents
+		$this->session->sendDataPacket(InventorySlotPacket::create(
+			$windowId,
+			$netSlot,
+			$itemStackWrapper
+		));
+	}
+
+	/**
+	 * @param ItemStackWrapper[] $itemStackWrappers
+	 */
+	private function sendInventoryContentPackets(int $windowId, array $itemStackWrappers) : void{
+		/*
+		 * TODO: HACK!
+		 * As of 1.20.12, the client ignores change of itemstackID in some cases when the old item == the new item.
+		 * Notably, this happens with armor, offhand and enchanting tables, but not with main inventory.
+		 * While we could track the items previously sent to the client, that's a waste of memory and would
+		 * cost performance. Instead, clear the slot(s) first, then send the new item(s).
+		 * The network cost of doing this is fortunately minimal, as an air itemstack is only 1 byte.
+		 */
+		$this->session->sendDataPacket(InventoryContentPacket::create(
+			$windowId,
+			array_fill_keys(array_keys($itemStackWrappers), new ItemStackWrapper(0, ItemStack::null()))
+		));
+		//now send the real contents
+		$this->session->sendDataPacket(InventoryContentPacket::create($windowId, $itemStackWrappers));
+	}
+
 	public function syncSlot(Inventory $inventory, int $slot, ItemStack $itemStack) : void{
 		$entry = $this->inventories[spl_object_id($inventory)] ?? null;
 		if($entry === null){
@@ -453,24 +498,9 @@ class InventoryManager{
 			//This can cause a lot of problems (totems, arrows, and more...).
 			//The workaround is to send an InventoryContentPacket instead
 			//BDS (Bedrock Dedicated Server) also seems to work this way.
-			$this->session->sendDataPacket(InventoryContentPacket::create($windowId, [$itemStackWrapper]));
+			$this->sendInventoryContentPackets($windowId, [$itemStackWrapper]);
 		}else{
-			if($windowId === ContainerIds::ARMOR){
+			$this->sendInventorySlotPackets($windowId, $netSlot, $itemStackWrapper);
-				//TODO: HACK!
-				//When right-clicking to equip armour, the client predicts the content of the armour slot, but
-				//doesn't report it in the transaction packet. The server then sends an InventorySlotPacket to
-				//the client, assuming the slot changed for some other reason, since there is no prediction for
-				//the slot.
-				//However, later requests involving that itemstack will refer to the request ID in which the
-				//armour was equipped, instead of the stack ID provided by the server in the outgoing
-				//InventorySlotPacket. (Perhaps because the item is already the same as the client actually
-				//predicted, but didn't tell us?)
-				//We work around this bug by setting the slot to air and then back to the correct item. In
-				//theory, setting a different count and then back again (or changing any other property) would
-				//also work, but this is simpler.
-				$this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, new ItemStackWrapper(0, ItemStack::null())));
-			}
-			$this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, $itemStackWrapper));
 		}
 		unset($entry->predictions[$slot], $entry->pendingSyncs[$slot]);
 	}
@@ -497,20 +527,17 @@ class InventoryManager{
 				$info = $this->trackItemStack($entry, $slot, $itemStack, null);
 				$contents[] = new ItemStackWrapper($info->getStackId(), $itemStack);
 			}
+			$clearSlotWrapper = new ItemStackWrapper(0, ItemStack::null());
 			if($entry->complexSlotMap !== null){
 				foreach($contents as $slotId => $info){
 					$packetSlot = $entry->complexSlotMap->mapCoreToNet($slotId) ?? null;
 					if($packetSlot === null){
 						continue;
 					}
-					$this->session->sendDataPacket(InventorySlotPacket::create(
+					$this->sendInventorySlotPackets($windowId, $packetSlot, $info);
-						$windowId,
-						$packetSlot,
-						$info
-					));
 				}
 			}else{
-				$this->session->sendDataPacket(InventoryContentPacket::create($windowId, $contents));
+				$this->sendInventoryContentPackets($windowId, $contents);
 			}
 		}
 	}