Redesign ItemStack extraData handling, nuke PacketSerializerContext

the shield stuff can be handled on the PM side by TypeConverter if this is needed.

src/serializer/PacketBatch.php

@@ -60,13 +60,13 @@ final public static function encodeRaw(BinaryStream $stream, array $packets) : v
 	 * @phpstan-return \Generator<int, Packet, void, void>
 	 * @throws PacketDecodeException
 	 */
-	final public static function decodePackets(BinaryStream $stream, PacketSerializerContext $context, PacketPool $packetPool) : \Generator{
+	final public static function decodePackets(BinaryStream $stream, PacketPool $packetPool) : \Generator{
 		$c = 0;
 		foreach(self::decodeRaw($stream) as $packetBuffer){
 			$packet = $packetPool->getPacket($packetBuffer);
 			if($packet !== null){
 				try{
-					$packet->decode(PacketSerializer::decoder($packetBuffer, 0, $context));
+					$packet->decode(PacketSerializer::decoder($packetBuffer, 0));
 				}catch(PacketDecodeException $e){
 					throw new PacketDecodeException("Error decoding packet $c in batch: " . $e->getMessage(), 0, $e);
 				}
@@ -82,9 +82,9 @@ final public static function decodePackets(BinaryStream $stream, PacketSerialize
 	 * @param Packet[]       $packets
 	 * @phpstan-param list<Packet> $packets
 	 */
-	final public static function encodePackets(BinaryStream $stream, PacketSerializerContext $context, array $packets) : void{
+	final public static function encodePackets(BinaryStream $stream, array $packets) : void{
 		foreach($packets as $packet){
-			$serializer = PacketSerializer::encoder($context);
+			$serializer = PacketSerializer::encoder();
 			$packet->encode($serializer);
 			$stream->putUnsignedVarInt(strlen($serializer->getBuffer()));
 			$stream->put($serializer->getBuffer());

src/serializer/PacketSerializer.php

@@ -15,7 +15,6 @@
 namespace pocketmine\network\mcpe\protocol\serializer;
 
 use pocketmine\math\Vector3;
-use pocketmine\nbt\LittleEndianNbtSerializer;
 use pocketmine\nbt\NbtDataException;
 use pocketmine\nbt\tag\CompoundTag;
 use pocketmine\nbt\TreeRoot;
@@ -40,7 +39,6 @@
 use pocketmine\network\mcpe\protocol\types\GameRule;
 use pocketmine\network\mcpe\protocol\types\IntGameRule;
 use pocketmine\network\mcpe\protocol\types\inventory\ItemStack;
-use pocketmine\network\mcpe\protocol\types\inventory\ItemStackExtraData;
 use pocketmine\network\mcpe\protocol\types\inventory\ItemStackWrapper;
 use pocketmine\network\mcpe\protocol\types\recipe\ComplexAliasItemDescriptor;
 use pocketmine\network\mcpe\protocol\types\recipe\IntIdMetaItemDescriptor;
@@ -67,22 +65,17 @@
 use function substr;
 
 class PacketSerializer extends BinaryStream{
-
-	private int $shieldItemRuntimeId;
-	private PacketSerializerContext $context;
-
-	protected function __construct(PacketSerializerContext $context, string $buffer = "", int $offset = 0){
+	protected function __construct(string $buffer = "", int $offset = 0){
+		//overridden to change visibility
 		parent::__construct($buffer, $offset);
-		$this->context = $context;
-		$this->shieldItemRuntimeId = $context->getItemDictionary()->fromStringId("minecraft:shield");
 	}
 
-	public static function encoder(PacketSerializerContext $context) : self{
-		return new self($context);
+	public static function encoder() : self{
+		return new self();
 	}
 
-	public static function decoder(string $buffer, int $offset, PacketSerializerContext $context) : self{
-		return new self($context, $buffer, $offset);
+	public static function decoder(string $buffer, int $offset) : self{
+		return new self($buffer, $offset);
 	}
 
 	/**
@@ -281,77 +274,6 @@ private function putItemStackHeader(ItemStack $itemStack) : bool{
 		return true;
 	}
 
-	public function getItemStackExtraData(int $id) : ItemStackExtraData{
-		$nbtLen = $this->getLShort();
-
-		/** @var CompoundTag|null $compound */
-		$compound = null;
-		if($nbtLen === 0xffff){
-			$nbtDataVersion = $this->getByte();
-			if($nbtDataVersion !== 1){
-				throw new PacketDecodeException("Unexpected NBT data version $nbtDataVersion");
-			}
-			$offset = $this->getOffset();
-			try{
-				$compound = (new LittleEndianNbtSerializer())->read($this->getBuffer(), $offset, 512)->mustGetCompoundTag();
-			}catch(NbtDataException $e){
-				throw PacketDecodeException::wrap($e, "Failed decoding NBT root");
-			}finally{
-				$this->setOffset($offset);
-			}
-		}elseif($nbtLen !== 0){
-			throw new PacketDecodeException("Unexpected fake NBT length $nbtLen");
-		}
-
-		$canPlaceOn = [];
-		for($i = 0, $canPlaceOnCount = $this->getLInt(); $i < $canPlaceOnCount; ++$i){
-			$canPlaceOn[] = $this->get($this->getLShort());
-		}
-
-		$canDestroy = [];
-		for($i = 0, $canDestroyCount = $this->getLInt(); $i < $canDestroyCount; ++$i){
-			$canDestroy[] = $this->get($this->getLShort());
-		}
-
-		$shieldBlockingTick = null;
-		if($id === $this->shieldItemRuntimeId){
-			$shieldBlockingTick = $this->getLLong();
-		}
-
-		if(!$this->feof()){
-			throw new PacketDecodeException("Unexpected trailing extradata for network item $id");
-		}
-
-		return new ItemStackExtraData($compound, $canPlaceOn, $canDestroy, $shieldBlockingTick);
-	}
-
-	public function putItemStackExtraData(int $id, ItemStackExtraData $extraData) : void{
-		$nbt = $extraData->getNbt();
-		if($nbt !== null){
-			$this->putLShort(0xffff);
-			$this->putByte(1); //TODO: NBT data version (?)
-			$this->put((new LittleEndianNbtSerializer())->write(new TreeRoot($nbt)));
-		}else{
-			$this->putLShort(0);
-		}
-
-		$this->putLInt(count($extraData->getCanPlaceOn()));
-		foreach($extraData->getCanPlaceOn() as $entry){
-			$this->putLShort(strlen($entry));
-			$this->put($entry);
-		}
-		$this->putLInt(count($extraData->getCanDestroy()));
-		foreach($extraData->getCanDestroy() as $entry){
-			$this->putLShort(strlen($entry));
-			$this->put($entry);
-		}
-
-		$blockingTick = $extraData->getShieldBlockingTick();
-		if($id === $this->shieldItemRuntimeId){
-			$this->putLLong($blockingTick ?? 0);
-		}
-	}
-
 	private function getItemStackFooter(int $id, int $meta, int $count) : ItemStack{
 		$blockRuntimeId = $this->getVarInt();
 		$rawExtraData = $this->getString();

src/types/inventory/ItemStack.php

@@ -14,13 +14,12 @@
 
 namespace pocketmine\network\mcpe\protocol\types\inventory;
 
-use pocketmine\network\mcpe\protocol\serializer\PacketSerializer;
 use function base64_encode;
 
 final class ItemStack implements \JsonSerializable{
 	/**
-	 * @param string $rawExtraData Serialized ItemStackExtraData as encoded by PacketSerializer::putItemStackExtraData()
-	 * @see PacketSerializer::putItemStackExtraData()
+	 * @param string $rawExtraData Serialized ItemStackExtraData (use ItemStackExtraData->write())
+	 * @see ItemStackExtraData::write()
 	 */
 	public function __construct(
 		private int $id,
@@ -53,9 +52,11 @@ public function getCount() : int{
 	public function getBlockRuntimeId() : int{ return $this->blockRuntimeId; }
 
 	/**
-	 * Decode this into ItemStackExtraData using PacketSerializer::getItemStackExtraData()
-	 * It's provided in raw form to avoid unnecessary decoding when the data is not needed.
-	 * @see PacketSerializer::getItemStackExtraData()
+	 * Decode this into ItemStackExtraData using ItemStackExtraData::read() (or ItemStackExtraDataShield::read() if this
+	 * data is for a shield item)
+	 * This isn't automatically decoded because it's usually not needed and is sometimes expensive to decode.
+	 * @see ItemStackExtraData::read()
+	 * @see ItemStackExtraDataShield::read()
 	 */
 	public function getRawExtraData() : string{ return $this->rawExtraData; }
 

src/types/inventory/ItemStackExtraData.php

@@ -14,23 +14,29 @@
 
 namespace pocketmine\network\mcpe\protocol\types\inventory;
 
+use pocketmine\nbt\LittleEndianNbtSerializer;
+use pocketmine\nbt\NbtDataException;
 use pocketmine\nbt\tag\CompoundTag;
+use pocketmine\nbt\TreeRoot;
+use pocketmine\network\mcpe\protocol\PacketDecodeException;
+use pocketmine\network\mcpe\protocol\serializer\PacketSerializer;
+use function count;
+use function strlen;
 
 /**
  * Wrapper class for extra data on ItemStacks.
  * The data is normally provided as a raw string (not automatically decoded).
  * This class is just a DTO for PacketSerializer to use when encoding/decoding ItemStacks.
  */
-final class ItemStackExtraData{
+class ItemStackExtraData{
 	/**
 	 * @param string[] $canPlaceOn
 	 * @param string[] $canDestroy
 	 */
 	public function __construct(
 		private ?CompoundTag $nbt,
 		private array $canPlaceOn,
-		private array $canDestroy,
-		private ?int $shieldBlockingTick = null
+		private array $canDestroy
 	){}
 
 	/**
@@ -51,7 +57,59 @@ public function getNbt() : ?CompoundTag{
 		return $this->nbt;
 	}
 
-	public function getShieldBlockingTick() : ?int{
-		return $this->shieldBlockingTick;
+	public static function read(PacketSerializer $in) : self{
+		$nbtLen = $in->getLShort();
+
+		/** @var CompoundTag|null $compound */
+		$compound = null;
+		if($nbtLen === 0xffff){
+			$nbtDataVersion = $in->getByte();
+			if($nbtDataVersion !== 1){
+				throw new PacketDecodeException("Unexpected NBT data version $nbtDataVersion");
+			}
+			$offset = $in->getOffset();
+			try{
+				$compound = (new LittleEndianNbtSerializer())->read($in->getBuffer(), $offset, 512)->mustGetCompoundTag();
+			}catch(NbtDataException $e){
+				throw PacketDecodeException::wrap($e, "Failed decoding NBT root");
+			}finally{
+				$in->setOffset($offset);
+			}
+		}elseif($nbtLen !== 0){
+			throw new PacketDecodeException("Unexpected fake NBT length $nbtLen");
+		}
+
+		$canPlaceOn = [];
+		for($i = 0, $canPlaceOnCount = $in->getLInt(); $i < $canPlaceOnCount; ++$i){
+			$canPlaceOn[] = $in->get($in->getLShort());
+		}
+
+		$canDestroy = [];
+		for($i = 0, $canDestroyCount = $in->getLInt(); $i < $canDestroyCount; ++$i){
+			$canDestroy[] = $in->get($in->getLShort());
+		}
+
+		return new self($compound, $canPlaceOn, $canDestroy);
+	}
+
+	public function write(PacketSerializer $out) : void{
+		if($this->nbt !== null){
+			$out->putLShort(0xffff);
+			$out->putByte(1); //TODO: NBT data version (?)
+			$out->put((new LittleEndianNbtSerializer())->write(new TreeRoot($this->nbt)));
+		}else{
+			$out->putLShort(0);
+		}
+
+		$out->putLInt(count($this->canPlaceOn));
+		foreach($this->canPlaceOn as $entry){
+			$out->putLShort(strlen($entry));
+			$out->put($entry);
+		}
+		$out->putLInt(count($this->canDestroy));
+		foreach($this->canDestroy as $entry){
+			$out->putLShort(strlen($entry));
+			$out->put($entry);
+		}
 	}
 }

src/types/inventory/ItemStackExtraDataShield.php

@@ -0,0 +1,51 @@
+<?php
+
+/*
+ * This file is part of BedrockProtocol.
+ * Copyright (C) 2014-2022 PocketMine Team <https://github.com/pmmp/BedrockProtocol>
+ *
+ * BedrockProtocol 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.
+ */
+
+declare(strict_types=1);
+
+namespace pocketmine\network\mcpe\protocol\types\inventory;
+
+use pocketmine\nbt\tag\CompoundTag;
+use pocketmine\network\mcpe\protocol\serializer\PacketSerializer;
+
+/**
+ * Extension of ItemStackExtraData for shield items, which have an additional field for the blocking tick.
+ */
+final class ItemStackExtraDataShield extends ItemStackExtraData{
+
+	/**
+	 * @param string[] $canPlaceOn
+	 * @param string[] $canDestroy
+	 */
+	public function __construct(
+		?CompoundTag $nbt,
+		array $canPlaceOn,
+		array $canDestroy,
+		private int $blockingTick
+	){
+		parent::__construct($nbt, $canPlaceOn, $canDestroy);
+	}
+
+	public function getBlockingTick() : int{ return $this->blockingTick; }
+
+	public static function read(PacketSerializer $in) : self{
+		$base = parent::read($in);
+		$blockingTick = $in->getLLong();
+
+		return new self($base->getNbt(), $base->getCanPlaceOn(), $base->getCanDestroy(), $blockingTick);
+	}
+
+	public function write(PacketSerializer $out) : void{
+		parent::write($out);
+		$out->putLLong($this->blockingTick);
+	}
+}

tests/phpunit/DataPacketTest.php

@@ -36,12 +36,11 @@ public function testHeaderFidelity() : void{
 		$pk->senderSubId = 3;
 		$pk->recipientSubId = 2;
 
-		$context = new PacketSerializerContext(new ItemTypeDictionary([new ItemTypeEntry("minecraft:shield", 0, false)]));
-		$serializer = PacketSerializer::encoder($context);
+		$serializer = PacketSerializer::encoder();
 		$pk->encode($serializer);
 
 		$pk2 = new TestPacket();
-		$pk2->decode(PacketSerializer::decoder($serializer->getBuffer(), 0, $context));
+		$pk2->decode(PacketSerializer::decoder($serializer->getBuffer(), 0));
 		self::assertSame($pk2->senderSubId, 3);
 		self::assertSame($pk2->recipientSubId, 2);
 	}

tests/phpunit/LoginPacketTest.php

@@ -34,13 +34,12 @@
 class LoginPacketTest extends TestCase{
 
 	public function testInvalidChainDataJsonHandling() : void{
-		$context = new PacketSerializerContext(new ItemTypeDictionary([new ItemTypeEntry("minecraft:shield", 0, false)]));
-		$stream = PacketSerializer::encoder($context);
+		$stream = PacketSerializer::encoder();
 		$stream->putUnsignedVarInt(ProtocolInfo::LOGIN_PACKET);
 		$payload = '{"chain":[]'; //intentionally malformed
 		$stream->putInt(ProtocolInfo::CURRENT_PROTOCOL);
 
-		$stream2 = PacketSerializer::encoder($context);
+		$stream2 = PacketSerializer::encoder();
 		$stream2->putLInt(strlen($payload));
 		$stream2->put($payload);
 		$stream->putString($stream2->getBuffer());
@@ -49,6 +48,6 @@ public function testInvalidChainDataJsonHandling() : void{
 		self::assertInstanceOf(LoginPacket::class, $pk);
 
 		$this->expectException(PacketDecodeException::class);
-		$pk->decode(PacketSerializer::decoder($stream->getBuffer(), 0, $context)); //bang
+		$pk->decode(PacketSerializer::decoder($stream->getBuffer(), 0)); //bang
 	}
 }