From 4ba57f2b039173b238f115bbd912cf091fc45b50 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 16 Apr 2023 00:40:43 +0100 Subject: [PATCH 1/9] RegisteredListener: use try...finally to stop timings While event handlers should not throw exceptions, we need to make sure the timings get stopped in the correct order, because the parent Event timer will be stopped due to using a finally block. If this happens while the handler timing is still running, a second exception will occur, obscuring the real error. --- src/event/RegisteredListener.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/event/RegisteredListener.php b/src/event/RegisteredListener.php index d227fc2cb..6b29dfec3 100644 --- a/src/event/RegisteredListener.php +++ b/src/event/RegisteredListener.php @@ -57,8 +57,11 @@ class RegisteredListener{ return; } $this->timings->startTiming(); - ($this->handler)($event); - $this->timings->stopTiming(); + try{ + ($this->handler)($event); + }finally{ + $this->timings->stopTiming(); + } } public function isHandlingCancelled() : bool{ From b5dc72b0ee4fffba7be91e022a804f255bb58c98 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 16 Apr 2023 16:47:17 +0100 Subject: [PATCH 2/9] tools/simulate-chunk-selector: fixed the script being completely broken getopt() behaviour is really, really dumb --- tools/simulate-chunk-selector.php | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tools/simulate-chunk-selector.php b/tools/simulate-chunk-selector.php index 81beb6bb3..0b279268a 100644 --- a/tools/simulate-chunk-selector.php +++ b/tools/simulate-chunk-selector.php @@ -24,12 +24,10 @@ declare(strict_types=1); namespace pocketmine\tools\simulate_chunk_selector; use pocketmine\player\ChunkSelector; -use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\Utils; use pocketmine\world\format\Chunk; use pocketmine\world\World; use Symfony\Component\Filesystem\Path; -use function assert; use function count; use function dirname; use function fwrite; @@ -128,7 +126,12 @@ if(count(getopt("", ["help"])) !== 0){ exit(0); } -foreach(Utils::stringifyKeys(getopt("", ["radius:", "baseX:", "baseZ:", "scale:", "chunksPerStep:"])) as $name => $value){ +$opts = getopt("", ["radius:", "baseX:", "baseZ:", "scale:", "chunksPerStep:", "output:"]); +foreach(["radius", "baseX", "baseZ", "scale", "chunksPerStep"] as $name){ + $value = $opts[$name] ?? null; + if($value === null){ + continue; + } if(!is_string($value) || (string) ((int) $value) !== $value){ fwrite(STDERR, "Value for --$name must be an integer\n"); exit(1); @@ -139,8 +142,7 @@ foreach(Utils::stringifyKeys(getopt("", ["radius:", "baseX:", "baseZ:", "scale:" "baseX" => ($baseX = $value), "baseZ" => ($baseZ = $value), "scale" => ($scale = $value), - "chunksPerStep" => ($nChunksPerStep = $value), - default => throw new AssumptionFailedError("getopt() returned unknown option") + "chunksPerStep" => ($nChunksPerStep = $value) }; } if($radius === null){ @@ -149,10 +151,10 @@ if($radius === null){ } $outputDirectory = null; -foreach(Utils::stringifyKeys(getopt("", ["output:"])) as $name => $value){ - assert($name === "output"); +if(isset($opts["output"])){ + $value = $opts["output"]; if(!is_string($value)){ - fwrite(STDERR, "Value for --$name must be a string\n"); + fwrite(STDERR, "Value for --output be a string\n"); exit(1); } if(!@mkdir($value) && !is_dir($value)){ From 56fbd45dd5790a9bbc7182b800933c5f38e1700a Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 16 Apr 2023 17:38:26 +0100 Subject: [PATCH 3/9] Entity: avoid double-checking block intersections for moving entities fixes #1824 --- src/entity/Entity.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/entity/Entity.php b/src/entity/Entity.php index 69fc684a1..346f196e4 100644 --- a/src/entity/Entity.php +++ b/src/entity/Entity.php @@ -115,6 +115,8 @@ abstract class Entity{ /** @var Block[]|null */ protected $blocksAround; + private bool $checkBlockIntersectionsNextTick = true; + /** @var Location */ protected $location; /** @var Location */ @@ -649,7 +651,10 @@ abstract class Entity{ $hasUpdate = false; - $this->checkBlockIntersections(); + if($this->checkBlockIntersectionsNextTick){ + $this->checkBlockIntersections(); + } + $this->checkBlockIntersectionsNextTick = true; if($this->location->y <= World::Y_MIN - 16 && $this->isAlive()){ $ev = new EntityDamageEvent($this, EntityDamageEvent::CAUSE_VOID, 10); @@ -1308,6 +1313,7 @@ abstract class Entity{ } protected function checkBlockIntersections() : void{ + $this->checkBlockIntersectionsNextTick = false; $vectors = []; foreach($this->getBlocksAroundWithEntityInsideActions() as $block){ From 9561ae5af7d679974ef5eef66317936c2e5563eb Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 16 Apr 2023 17:57:51 +0100 Subject: [PATCH 4/9] Entity: micro optimisation for checkBlockIntersections() --- src/entity/Entity.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/entity/Entity.php b/src/entity/Entity.php index 346f196e4..0266407c9 100644 --- a/src/entity/Entity.php +++ b/src/entity/Entity.php @@ -1325,10 +1325,12 @@ abstract class Entity{ } } - $vector = Vector3::sum(...$vectors); - if($vector->lengthSquared() > 0){ - $d = 0.014; - $this->motion = $this->motion->addVector($vector->normalize()->multiply($d)); + if(count($vectors) > 0){ + $vector = Vector3::sum(...$vectors); + if($vector->lengthSquared() > 0){ + $d = 0.014; + $this->motion = $this->motion->addVector($vector->normalize()->multiply($d)); + } } } From d07acd0013fa3093154de365f9574dd2350f4957 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 17 Apr 2023 14:05:46 +0100 Subject: [PATCH 5/9] RakLibInterface: split error ID into 4-character chunks this makes it easier to read, since the error ID can't be copy-pasted from the disconnection screen on the client. --- src/network/mcpe/raklib/RakLibInterface.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/network/mcpe/raklib/RakLibInterface.php b/src/network/mcpe/raklib/RakLibInterface.php index 46927b4f6..f420d4352 100644 --- a/src/network/mcpe/raklib/RakLibInterface.php +++ b/src/network/mcpe/raklib/RakLibInterface.php @@ -53,6 +53,7 @@ use function implode; use function mt_rand; use function random_bytes; use function rtrim; +use function str_split; use function substr; use const PHP_INT_MAX; @@ -196,7 +197,7 @@ class RakLibInterface implements ServerEventListener, AdvancedNetworkInterface{ try{ $session->handleEncoded($buf); }catch(PacketHandlingException $e){ - $errorId = bin2hex(random_bytes(6)); + $errorId = implode("-", str_split(bin2hex(random_bytes(6)), 4)); $logger = $session->getLogger(); $logger->error("Bad packet (error ID $errorId): " . $e->getMessage()); From 40168a457e8b129e763f3bc33fc458904cba2cca Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 18 Apr 2023 14:43:25 +0100 Subject: [PATCH 6/9] TypeConverter: fixed coreItemStackToNet() causing item NBT to be prepared twice hasNamedTag() calls getNamedTag(), which calls serializeCompoundTag(), which writes the item's properties into the given NBT tag. --- src/network/mcpe/convert/TypeConverter.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/network/mcpe/convert/TypeConverter.php b/src/network/mcpe/convert/TypeConverter.php index e8826e591..3fe679196 100644 --- a/src/network/mcpe/convert/TypeConverter.php +++ b/src/network/mcpe/convert/TypeConverter.php @@ -135,9 +135,11 @@ class TypeConverter{ if($itemStack->isNull()){ return ItemStack::null(); } - $nbt = null; - if($itemStack->hasNamedTag()){ - $nbt = clone $itemStack->getNamedTag(); + $nbt = $itemStack->getNamedTag(); + if($nbt->count() === 0){ + $nbt = null; + }else{ + $nbt = clone $nbt; } $isBlockItem = $itemStack->getId() < 256; From 6102740ee35c2f03387748bb795fe74b7724655f Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 18 Apr 2023 15:00:34 +0100 Subject: [PATCH 7/9] TypeConverter: use a less slow hack to restore meta values on items sent by the client this isn't even really needed anymore, since we don't decode items from the client since 4.18. However, this may still be useful for tools. --- src/network/mcpe/convert/TypeConverter.php | 26 +++++++--------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/network/mcpe/convert/TypeConverter.php b/src/network/mcpe/convert/TypeConverter.php index 3fe679196..09d5d4a2e 100644 --- a/src/network/mcpe/convert/TypeConverter.php +++ b/src/network/mcpe/convert/TypeConverter.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace pocketmine\network\mcpe\convert; +use pocketmine\block\Block; use pocketmine\block\BlockLegacyIds; use pocketmine\item\Durable; use pocketmine\item\Item; @@ -142,8 +143,6 @@ class TypeConverter{ $nbt = clone $nbt; } - $isBlockItem = $itemStack->getId() < 256; - $idMeta = ItemTranslator::getInstance()->toNetworkIdQuiet($itemStack->getId(), $itemStack->getMeta()); if($idMeta === null){ //Display unmapped items as INFO_UPDATE, but stick something in their NBT to make sure they don't stack with @@ -168,23 +167,15 @@ class TypeConverter{ } $nbt->setInt(self::DAMAGE_TAG, $itemStack->getDamage()); $meta = 0; - }elseif($isBlockItem && $itemStack->getMeta() !== 0){ - //TODO HACK: This foul-smelling code ensures that we can correctly deserialize an item when the - //client sends it back to us, because as of 1.16.220, blockitems quietly discard their metadata - //client-side. Aside from being very annoying, this also breaks various server-side behaviours. - if($nbt === null){ - $nbt = new CompoundTag(); - } - $nbt->setInt(self::PM_META_TAG, $itemStack->getMeta()); - $meta = 0; } } $blockRuntimeId = 0; - if($isBlockItem){ + if($itemStack->getId() < 256){ $block = $itemStack->getBlock(); if($block->getId() !== BlockLegacyIds::AIR){ $blockRuntimeId = RuntimeBlockMapping::getInstance()->toRuntimeId($block->getFullId()); + $meta = 0; } } @@ -210,6 +201,11 @@ class TypeConverter{ $compound = $itemStack->getNbt(); [$id, $meta] = ItemTranslator::getInstance()->fromNetworkId($itemStack->getId(), $itemStack->getMeta()); + if($itemStack->getBlockRuntimeId() !== 0){ + //blockitem meta is zeroed out by the client, so we have to infer it from the block runtime ID + $blockFullId = RuntimeBlockMapping::getInstance()->fromRuntimeId($itemStack->getBlockRuntimeId()); + $meta = $blockFullId & Block::INTERNAL_METADATA_MASK; + } if($compound !== null){ $compound = clone $compound; @@ -224,12 +220,6 @@ class TypeConverter{ $compound->removeTag(self::DAMAGE_TAG_CONFLICT_RESOLUTION); $compound->setTag(self::DAMAGE_TAG, $conflicted); } - }elseif(($metaTag = $compound->getTag(self::PM_META_TAG)) instanceof IntTag){ - //TODO HACK: This foul-smelling code ensures that we can correctly deserialize an item when the - //client sends it back to us, because as of 1.16.220, blockitems quietly discard their metadata - //client-side. Aside from being very annoying, this also breaks various server-side behaviours. - $meta = $metaTag->getValue(); - $compound->removeTag(self::PM_META_TAG); } if($compound->count() === 0){ $compound = null; From a77fc8109f669a7859fe46edc5a0bdb73c0d4b46 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 18 Apr 2023 15:02:52 +0100 Subject: [PATCH 8/9] TypeConverter: avoid repeated calls to getId() and getMeta() --- src/network/mcpe/convert/TypeConverter.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/network/mcpe/convert/TypeConverter.php b/src/network/mcpe/convert/TypeConverter.php index 09d5d4a2e..cfc2454d9 100644 --- a/src/network/mcpe/convert/TypeConverter.php +++ b/src/network/mcpe/convert/TypeConverter.php @@ -143,7 +143,9 @@ class TypeConverter{ $nbt = clone $nbt; } - $idMeta = ItemTranslator::getInstance()->toNetworkIdQuiet($itemStack->getId(), $itemStack->getMeta()); + $internalId = $itemStack->getId(); + $internalMeta = $itemStack->getMeta(); + $idMeta = ItemTranslator::getInstance()->toNetworkIdQuiet($internalId, $internalMeta); if($idMeta === null){ //Display unmapped items as INFO_UPDATE, but stick something in their NBT to make sure they don't stack with //other unmapped items. @@ -151,8 +153,8 @@ class TypeConverter{ if($nbt === null){ $nbt = new CompoundTag(); } - $nbt->setInt(self::PM_ID_TAG, $itemStack->getId()); - $nbt->setInt(self::PM_META_TAG, $itemStack->getMeta()); + $nbt->setInt(self::PM_ID_TAG, $internalId); + $nbt->setInt(self::PM_META_TAG, $internalMeta); }else{ [$id, $meta] = $idMeta; @@ -171,7 +173,7 @@ class TypeConverter{ } $blockRuntimeId = 0; - if($itemStack->getId() < 256){ + if($internalId < 256){ $block = $itemStack->getBlock(); if($block->getId() !== BlockLegacyIds::AIR){ $blockRuntimeId = RuntimeBlockMapping::getInstance()->toRuntimeId($block->getFullId()); From 674b65f789c193cb221027c7f71a37d83074d877 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 18 Apr 2023 16:18:34 +0100 Subject: [PATCH 9/9] Item: optimise serializeCompoundTag() a little --- src/item/Item.php | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/item/Item.php b/src/item/Item.php index 785a38ca0..d59e15cd7 100644 --- a/src/item/Item.php +++ b/src/item/Item.php @@ -353,30 +353,35 @@ class Item implements \JsonSerializable{ } protected function serializeCompoundTag(CompoundTag $tag) : void{ - $display = $tag->getCompoundTag(self::TAG_DISPLAY) ?? new CompoundTag(); + $display = $tag->getCompoundTag(self::TAG_DISPLAY); - $this->hasCustomName() ? - $display->setString(self::TAG_DISPLAY_NAME, $this->getCustomName()) : - $display->removeTag(self::TAG_DISPLAY_NAME); + if($this->customName !== ""){ + $display ??= new CompoundTag(); + $display->setString(self::TAG_DISPLAY_NAME, $this->customName); + }else{ + $display?->removeTag(self::TAG_DISPLAY_NAME); + } if(count($this->lore) > 0){ $loreTag = new ListTag(); foreach($this->lore as $line){ $loreTag->push(new StringTag($line)); } + $display ??= new CompoundTag(); $display->setTag(self::TAG_DISPLAY_LORE, $loreTag); }else{ - $display->removeTag(self::TAG_DISPLAY_LORE); + $display?->removeTag(self::TAG_DISPLAY_LORE); } - $display->count() > 0 ? + $display !== null && $display->count() > 0 ? $tag->setTag(self::TAG_DISPLAY, $display) : $tag->removeTag(self::TAG_DISPLAY); - if($this->hasEnchantments()){ + if(count($this->enchantments) > 0){ $ench = new ListTag(); - foreach($this->getEnchantments() as $enchantmentInstance){ + $enchantmentIdMap = EnchantmentIdMap::getInstance(); + foreach($this->enchantments as $enchantmentInstance){ $ench->push(CompoundTag::create() - ->setShort(self::TAG_ENCH_ID, EnchantmentIdMap::getInstance()->toId($enchantmentInstance->getType())) + ->setShort(self::TAG_ENCH_ID, $enchantmentIdMap->toId($enchantmentInstance->getType())) ->setShort(self::TAG_ENCH_LVL, $enchantmentInstance->getLevel()) ); } @@ -385,8 +390,8 @@ class Item implements \JsonSerializable{ $tag->removeTag(self::TAG_ENCH); } - ($blockData = $this->getCustomBlockData()) !== null ? - $tag->setTag(self::TAG_BLOCK_ENTITY_TAG, clone $blockData) : + $this->blockEntityTag !== null ? + $tag->setTag(self::TAG_BLOCK_ENTITY_TAG, clone $this->blockEntityTag) : $tag->removeTag(self::TAG_BLOCK_ENTITY_TAG); if(count($this->canPlaceOn) > 0){