From 5a54d098690d1c46a3dd56382928c75426c055cd Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 13:18:28 +0100 Subject: [PATCH 01/25] InventoryManager: verify slot existence in locateWindowAndSlot() previously, this would happily return invalid slot IDs, potentially leading to a crash. --- src/network/mcpe/InventoryManager.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index a651b9213..c6d83c65e 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -205,11 +205,13 @@ class InventoryManager{ if($entry === null){ return null; } + $inventory = $entry->getInventory(); $coreSlotId = $entry->mapNetToCore($netSlotId); - return $coreSlotId !== null ? [$entry->getInventory(), $coreSlotId] : null; + return $coreSlotId !== null && $inventory->slotExists($coreSlotId) ? [$inventory, $coreSlotId] : null; } - if(isset($this->networkIdToInventoryMap[$windowId])){ - return [$this->networkIdToInventoryMap[$windowId], $netSlotId]; + $inventory = $this->networkIdToInventoryMap[$windowId] ?? null; + if($inventory !== null && $inventory->slotExists($netSlotId)){ + return [$inventory, $netSlotId]; } return null; } From 7c19f14cf5d0e55a2d38ec60ae489f8c867969f5 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 13:25:08 +0100 Subject: [PATCH 02/25] Fixed up offhand handling for ItemStackRequest, fixes #5723 --- .../mcpe/handler/InGamePacketHandler.php | 4 ++-- .../handler/ItemStackContainerIdTranslator.php | 18 ++++++++++++------ .../mcpe/handler/ItemStackRequestExecutor.php | 7 +------ .../mcpe/handler/ItemStackResponseBuilder.php | 6 +----- 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 732c8817d..329bcd6ef 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -356,8 +356,8 @@ class InGamePacketHandler extends PacketHandler{ //rejects the transaction. The most common example of this is equipping armor by right-click, which doesn't send //a legacy prediction action for the destination armor slot. foreach($packet->requestChangedSlots as $containerInfo){ - $windowId = ItemStackContainerIdTranslator::translate($containerInfo->getContainerId(), $this->inventoryManager->getCurrentWindowId()); - foreach($containerInfo->getChangedSlotIndexes() as $slot){ + foreach($containerInfo->getChangedSlotIndexes() as $netSlot){ + [$windowId, $slot] = ItemStackContainerIdTranslator::translate($containerInfo->getContainerId(), $this->inventoryManager->getCurrentWindowId(), $netSlot); $inventoryAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slot); if($inventoryAndSlot !== null){ //trigger the normal slot sync logic $this->inventoryManager->onSlotChange($inventoryAndSlot[0], $inventoryAndSlot[1]); diff --git a/src/network/mcpe/handler/ItemStackContainerIdTranslator.php b/src/network/mcpe/handler/ItemStackContainerIdTranslator.php index a0ad3b26c..f9ea9ef5f 100644 --- a/src/network/mcpe/handler/ItemStackContainerIdTranslator.php +++ b/src/network/mcpe/handler/ItemStackContainerIdTranslator.php @@ -33,15 +33,21 @@ final class ItemStackContainerIdTranslator{ //NOOP } - public static function translate(int $containerInterfaceId, int $currentWindowId) : int{ + /** + * @return int[] + * @phpstan-return array{int, int} + * @throws PacketHandlingException + */ + public static function translate(int $containerInterfaceId, int $currentWindowId, int $slotId) : array{ return match($containerInterfaceId){ - ContainerUIIds::ARMOR => ContainerIds::ARMOR, + ContainerUIIds::ARMOR => [ContainerIds::ARMOR, $slotId], ContainerUIIds::HOTBAR, ContainerUIIds::INVENTORY, - ContainerUIIds::COMBINED_HOTBAR_AND_INVENTORY => ContainerIds::INVENTORY, + ContainerUIIds::COMBINED_HOTBAR_AND_INVENTORY => [ContainerIds::INVENTORY, $slotId], - ContainerUIIds::OFFHAND => ContainerIds::OFFHAND, + //TODO: HACK! The client sends an incorrect slot ID for the offhand as of 1.19.70 (though this doesn't really matter since the offhand has only 1 slot anyway) + ContainerUIIds::OFFHAND => [ContainerIds::OFFHAND, 0], ContainerUIIds::ANVIL_INPUT, ContainerUIIds::ANVIL_MATERIAL, @@ -68,7 +74,7 @@ final class ItemStackContainerIdTranslator{ ContainerUIIds::TRADE2_INGREDIENT1, ContainerUIIds::TRADE2_INGREDIENT2, ContainerUIIds::TRADE_INGREDIENT1, - ContainerUIIds::TRADE_INGREDIENT2 => ContainerIds::UI, + ContainerUIIds::TRADE_INGREDIENT2 => [ContainerIds::UI, $slotId], ContainerUIIds::BARREL, ContainerUIIds::BLAST_FURNACE_INGREDIENT, @@ -80,7 +86,7 @@ final class ItemStackContainerIdTranslator{ ContainerUIIds::FURNACE_RESULT, ContainerUIIds::LEVEL_ENTITY, //chest ContainerUIIds::SHULKER_BOX, - ContainerUIIds::SMOKER_INGREDIENT => $currentWindowId, + ContainerUIIds::SMOKER_INGREDIENT => [$currentWindowId, $slotId], //all preview slots are ignored, since the client shouldn't be modifying those directly diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index b7f9b1604..f9532291c 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -111,12 +111,7 @@ class ItemStackRequestExecutor{ * @throws ItemStackRequestProcessException */ protected function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{ - $windowId = ItemStackContainerIdTranslator::translate($info->getContainerId(), $this->inventoryManager->getCurrentWindowId()); - $slotId = $info->getSlotId(); - if($info->getContainerId() === ContainerUIIds::OFFHAND && $slotId === 1){ - //TODO: HACK! The client sends an incorrect slot ID for the offhand as of 1.19.70 - $slotId = 0; - } + [$windowId, $slotId] = ItemStackContainerIdTranslator::translate($info->getContainerId(), $this->inventoryManager->getCurrentWindowId(), $info->getSlotId()); $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slotId); if($windowAndSlot === null){ throw new ItemStackRequestProcessException("No open inventory matches container UI ID: " . $info->getContainerId() . ", slot ID: " . $info->getSlotId()); diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index 2a55c2d95..09af69f2a 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -53,11 +53,7 @@ final class ItemStackResponseBuilder{ * @phpstan-return array{Inventory, int} */ private function getInventoryAndSlot(int $containerInterfaceId, int $slotId) : ?array{ - if($containerInterfaceId === ContainerUIIds::OFFHAND && $slotId === 1){ - //TODO: HACK! The client sends an incorrect slot ID for the offhand as of 1.19.70 - $slotId = 0; - } - $windowId = ItemStackContainerIdTranslator::translate($containerInterfaceId, $this->inventoryManager->getCurrentWindowId()); + [$windowId, $slotId] = ItemStackContainerIdTranslator::translate($containerInterfaceId, $this->inventoryManager->getCurrentWindowId(), $slotId); $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slotId); if($windowAndSlot === null){ return null; From fea820a99e3fba8c91c0dc770fdef502ff8a6517 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 13:31:02 +0100 Subject: [PATCH 03/25] Release 4.20.1 --- changelogs/4.20.md | 10 ++++++++++ src/VersionInfo.php | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/changelogs/4.20.md b/changelogs/4.20.md index 357cdaced..d8f4c66b0 100644 --- a/changelogs/4.20.md +++ b/changelogs/4.20.md @@ -34,3 +34,13 @@ Released 26th April 2023. ### `pocketmine\player` - The following API methods have been added: - `public Player->openSignEditor(Vector3 $position) : void` - opens the client-side sign editor GUI for the given position + +# 4.20.1 +Released 27th April 2023. + +## Fixes +- Fixed server crash when firing a bow while holding arrows in the offhand slot. + +## Internals +- `ItemStackContainerIdTranslator::translate()` now requires an additional `int $slotId` parameter and returns `array{int, int}` (translated window ID, translated slot ID) to be used with `InventoryManager->locateWindowAndSlot()`. +- `InventoryManager->locateWindowAndSlot()` now checks if the translated slot actually exists in the requested inventory, and returns `null` if not. Previously, it would return potentially invalid slot IDs without checking them, potentially leading to crashes. diff --git a/src/VersionInfo.php b/src/VersionInfo.php index 8f31f6509..b2189ed23 100644 --- a/src/VersionInfo.php +++ b/src/VersionInfo.php @@ -32,7 +32,7 @@ use function str_repeat; final class VersionInfo{ public const NAME = "PocketMine-MP"; public const BASE_VERSION = "4.20.1"; - public const IS_DEVELOPMENT_BUILD = true; + public const IS_DEVELOPMENT_BUILD = false; public const BUILD_CHANNEL = "stable"; private function __construct(){ From 2910ffebf4f455baded6dd35574e7a44eb44d627 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 13:31:02 +0100 Subject: [PATCH 04/25] 4.20.2 is next --- src/VersionInfo.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/VersionInfo.php b/src/VersionInfo.php index b2189ed23..08582cc58 100644 --- a/src/VersionInfo.php +++ b/src/VersionInfo.php @@ -31,8 +31,8 @@ use function str_repeat; final class VersionInfo{ public const NAME = "PocketMine-MP"; - public const BASE_VERSION = "4.20.1"; - public const IS_DEVELOPMENT_BUILD = false; + public const BASE_VERSION = "4.20.2"; + public const IS_DEVELOPMENT_BUILD = true; public const BUILD_CHANNEL = "stable"; private function __construct(){ From 023460db2c2887233190565fd8b1bfae45cbe1fb Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 14:53:54 +0100 Subject: [PATCH 05/25] BaseInventory: fixed internalAddItem() doing useless canStackWith() checks on null items if the item is null, it's never going to stack with anything given to this function, because addItem() already discards null items. --- src/inventory/BaseInventory.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/inventory/BaseInventory.php b/src/inventory/BaseInventory.php index daf41c579..9968c283e 100644 --- a/src/inventory/BaseInventory.php +++ b/src/inventory/BaseInventory.php @@ -219,6 +219,7 @@ abstract class BaseInventory implements Inventory{ $item = $this->getItem($i); if($item->isNull()){ $emptySlots[] = $i; + continue; } if($slot->canStackWith($item) && $item->getCount() < $item->getMaxStackSize()){ From 7f6269c432e5341c7ad128c065a3bcda5060cf17 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 16:52:52 +0100 Subject: [PATCH 06/25] Introduce and use optimised versions of Inventory->isSlotEmpty() this avoids useless cloning, improving the performance of several functions. --- src/inventory/BaseInventory.php | 26 ++++++++++++++------------ src/inventory/DelegateInventory.php | 4 ++++ src/inventory/SimpleInventory.php | 4 ++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/inventory/BaseInventory.php b/src/inventory/BaseInventory.php index 9968c283e..d92ee92db 100644 --- a/src/inventory/BaseInventory.php +++ b/src/inventory/BaseInventory.php @@ -152,8 +152,8 @@ abstract class BaseInventory implements Inventory{ } public function firstEmpty() : int{ - foreach($this->getContents(true) as $i => $slot){ - if($slot->isNull()){ + for($i = 0, $size = $this->getSize(); $i < $size; $i++){ + if($this->isSlotEmpty($i)){ return $i; } } @@ -172,13 +172,15 @@ abstract class BaseInventory implements Inventory{ public function getAddableItemQuantity(Item $item) : int{ $count = $item->getCount(); for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ - $slot = $this->getItem($i); - if($item->canStackWith($slot)){ - if(($diff = min($slot->getMaxStackSize(), $item->getMaxStackSize()) - $slot->getCount()) > 0){ - $count -= $diff; - } - }elseif($slot->isNull()){ + if($this->isSlotEmpty($i)){ $count -= min($this->getMaxStackSize(), $item->getMaxStackSize()); + }else{ + $slot = $this->getItem($i); + if($item->canStackWith($slot)){ + if(($diff = min($slot->getMaxStackSize(), $item->getMaxStackSize()) - $slot->getCount()) > 0){ + $count -= $diff; + } + } } if($count <= 0){ @@ -216,11 +218,11 @@ abstract class BaseInventory implements Inventory{ $emptySlots = []; for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ - $item = $this->getItem($i); - if($item->isNull()){ + if($this->isSlotEmpty($i)){ $emptySlots[] = $i; continue; } + $item = $this->getItem($i); if($slot->canStackWith($item) && $item->getCount() < $item->getMaxStackSize()){ $amount = min($item->getMaxStackSize() - $item->getCount(), $slot->getCount(), $this->getMaxStackSize()); @@ -273,10 +275,10 @@ abstract class BaseInventory implements Inventory{ } for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ - $item = $this->getItem($i); - if($item->isNull()){ + if($this->isSlotEmpty($i)){ continue; } + $item = $this->getItem($i); foreach($itemSlots as $index => $slot){ if($slot->equals($item, !$slot->hasAnyDamageValue(), $slot->hasNamedTag())){ diff --git a/src/inventory/DelegateInventory.php b/src/inventory/DelegateInventory.php index ba9e5a983..a211732cf 100644 --- a/src/inventory/DelegateInventory.php +++ b/src/inventory/DelegateInventory.php @@ -85,6 +85,10 @@ class DelegateInventory extends BaseInventory{ $this->backingInventory->setContents($items); } + public function isSlotEmpty(int $index) : bool{ + return $this->backingInventory->isSlotEmpty($index); + } + protected function onSlotChange(int $index, Item $before) : void{ if($this->backingInventoryChanging){ parent::onSlotChange($index, $before); diff --git a/src/inventory/SimpleInventory.php b/src/inventory/SimpleInventory.php index aae11c84c..19fbdbb17 100644 --- a/src/inventory/SimpleInventory.php +++ b/src/inventory/SimpleInventory.php @@ -83,4 +83,8 @@ class SimpleInventory extends BaseInventory{ } } } + + public function isSlotEmpty(int $index) : bool{ + return $this->slots[$index] === null || $this->slots[$index]->isNull(); + } } From 07dc10d6e6a06ccc941627f4eff7c6d982be7244 Mon Sep 17 00:00:00 2001 From: Dylan T Date: Thu, 27 Apr 2023 16:59:29 +0100 Subject: [PATCH 07/25] World: improve performance of tickChunks() selection process (#5721) Since light population is required to make a chunk tickable, a chunk may not be tickable for some time if the async workers get backlogged. The previous version of this system only cached the eligibility result if the result was a "yes", but we can also track it when it's a "no", rather than rechecking it every tick. This change should improve performance in factions and similar gamemodes, which involve large maps with sparsely distributed players, where each player likely has an independent, non-overlapping ticking chunk circle. We also ditch TickingChunkEntry in favour of multiple arrays to track the eligibility states. This allows us to avoid rechecking the (even cached) readiness of potentially thousands of chunks. If there are no ticking chunks to recheck, this reduces the cost of the selection process to zero. --- src/world/TickingChunkEntry.php | 37 ---------- src/world/World.php | 116 +++++++++++++++++++------------- 2 files changed, 71 insertions(+), 82 deletions(-) delete mode 100644 src/world/TickingChunkEntry.php diff --git a/src/world/TickingChunkEntry.php b/src/world/TickingChunkEntry.php deleted file mode 100644 index ca965463d..000000000 --- a/src/world/TickingChunkEntry.php +++ /dev/null @@ -1,37 +0,0 @@ - ChunkTicker - * @phpstan-var array - */ - public array $tickers = []; - - public bool $ready = false; -} diff --git a/src/world/World.php b/src/world/World.php index 3a09be176..1803c03f1 100644 --- a/src/world/World.php +++ b/src/world/World.php @@ -224,10 +224,25 @@ class World implements ChunkManager{ private array $tickingLoaderCounter = []; /** - * @var TickingChunkEntry[] chunkHash => TickingChunkEntry - * @phpstan-var array + * @var ChunkTicker[][] chunkHash => [spl_object_id => ChunkTicker] + * @phpstan-var array> */ - private array $tickingChunks = []; + private array $registeredTickingChunks = []; + + /** + * Set of chunks which are definitely ready for ticking. + * + * @var int[] + * @phpstan-var array + */ + private array $validTickingChunks = []; + + /** + * Set of chunks which might be ready for ticking. These will be checked at the next tick. + * @var int[] + * @phpstan-var array + */ + private array $recheckTickingChunks = []; /** * @var ChunkLoader[][] chunkHash => [spl_object_id => ChunkLoader] @@ -1157,14 +1172,14 @@ class World implements ChunkManager{ } /** - * Returns a list of chunk position hashes (as returned by World::chunkHash()) which are currently registered for + * Returns a list of chunk position hashes (as returned by World::chunkHash()) which are currently valid for * ticking. * * @return int[] * @phpstan-return list */ public function getTickingChunks() : array{ - return array_keys($this->tickingChunks); + return array_keys($this->validTickingChunks); } /** @@ -1173,11 +1188,8 @@ class World implements ChunkManager{ */ public function registerTickingChunk(ChunkTicker $ticker, int $chunkX, int $chunkZ) : void{ $chunkPosHash = World::chunkHash($chunkX, $chunkZ); - $entry = $this->tickingChunks[$chunkPosHash] ?? null; - if($entry === null){ - $entry = $this->tickingChunks[$chunkPosHash] = new TickingChunkEntry(); - } - $entry->tickers[spl_object_id($ticker)] = $ticker; + $this->registeredTickingChunks[$chunkPosHash][spl_object_id($ticker)] = $ticker; + $this->recheckTickingChunks[$chunkPosHash] = $chunkPosHash; } /** @@ -1187,10 +1199,14 @@ class World implements ChunkManager{ public function unregisterTickingChunk(ChunkTicker $ticker, int $chunkX, int $chunkZ) : void{ $chunkHash = World::chunkHash($chunkX, $chunkZ); $tickerId = spl_object_id($ticker); - if(isset($this->tickingChunks[$chunkHash]->tickers[$tickerId])){ - unset($this->tickingChunks[$chunkHash]->tickers[$tickerId]); - if(count($this->tickingChunks[$chunkHash]->tickers) === 0){ - unset($this->tickingChunks[$chunkHash]); + if(isset($this->registeredTickingChunks[$chunkHash][$tickerId])){ + unset($this->registeredTickingChunks[$chunkHash][$tickerId]); + if(count($this->registeredTickingChunks[$chunkHash]) === 0){ + unset( + $this->registeredTickingChunks[$chunkHash], + $this->recheckTickingChunks[$chunkHash], + $this->validTickingChunks[$chunkHash] + ); } } } @@ -1234,37 +1250,37 @@ class World implements ChunkManager{ } private function tickChunks() : void{ - if($this->chunkTickRadius <= 0 || (count($this->tickingChunks) === 0 && count($this->tickingLoaders) === 0)){ + if($this->chunkTickRadius <= 0 || (count($this->registeredTickingChunks) === 0 && count($this->tickingLoaders) === 0)){ return; } - $this->timings->randomChunkUpdatesChunkSelection->startTiming(); + if(count($this->recheckTickingChunks) > 0 || count($this->tickingLoaders) > 0){ + $this->timings->randomChunkUpdatesChunkSelection->startTiming(); - /** @var bool[] $chunkTickList chunkhash => dummy */ - $chunkTickList = []; + $chunkTickableCache = []; - $chunkTickableCache = []; - - foreach($this->tickingChunks as $hash => $entry){ - if(!$entry->ready){ + foreach($this->recheckTickingChunks as $hash => $_){ World::getXZ($hash, $chunkX, $chunkZ); if($this->isChunkTickable($chunkX, $chunkZ, $chunkTickableCache)){ - $entry->ready = true; - }else{ - //the chunk has been flagged as temporarily not tickable, so we don't want to tick it this time - continue; + $this->validTickingChunks[$hash] = $hash; } } - $chunkTickList[$hash] = true; - } + $this->recheckTickingChunks = []; - //TODO: REMOVE THIS - //backwards compatibility for TickingChunkLoader, although I'm not sure this is really necessary in practice - if(count($this->tickingLoaders) !== 0){ - $this->selectTickableChunksLegacy($chunkTickList, $chunkTickableCache); - } + //TODO: REMOVE THIS - we need a local var to add extra chunks to if we have legacy ticking loaders + //this is copy-on-write, so it won't have any performance impact if there are no legacy ticking loaders + $chunkTickList = $this->validTickingChunks; - $this->timings->randomChunkUpdatesChunkSelection->stopTiming(); + //TODO: REMOVE THIS + //backwards compatibility for TickingChunkLoader, although I'm not sure this is really necessary in practice + if(count($this->tickingLoaders) !== 0){ + $this->selectTickableChunksLegacy($chunkTickList, $chunkTickableCache); + } + + $this->timings->randomChunkUpdatesChunkSelection->stopTiming(); + }else{ + $chunkTickList = $this->validTickingChunks; + } foreach($chunkTickList as $index => $_){ World::getXZ($index, $chunkX, $chunkZ); @@ -1315,16 +1331,23 @@ class World implements ChunkManager{ } /** - * Marks the 3x3 chunks around the specified chunk as not ready to be ticked. This is used to prevent chunk ticking - * while a chunk is being populated, light-populated, or unloaded. - * Each chunk will be rechecked every tick until it is ready to be ticked again. + * Marks the 3x3 square of chunks centered on the specified chunk for chunk ticking eligibility recheck. + * + * This should be used whenever the chunk's eligibility to be ticked is changed. This includes: + * - Loading/unloading the chunk (the chunk may be registered for ticking before it is loaded) + * - Locking/unlocking the chunk (e.g. world population) + * - Light populated state change (i.e. scheduled for light population, or light population completed) + * - Arbitrary chunk replacement (i.e. setChunk() or similar) */ - private function markTickingChunkUnavailable(int $chunkX, int $chunkZ) : void{ + private function markTickingChunkForRecheck(int $chunkX, int $chunkZ) : void{ for($cx = -1; $cx <= 1; ++$cx){ for($cz = -1; $cz <= 1; ++$cz){ $chunkHash = World::chunkHash($chunkX + $cx, $chunkZ + $cz); - if(isset($this->tickingChunks[$chunkHash])){ - $this->tickingChunks[$chunkHash]->ready = false; + unset($this->validTickingChunks[$chunkHash]); + if(isset($this->registeredTickingChunks[$chunkHash])){ + $this->recheckTickingChunks[$chunkHash] = $chunkHash; + }else{ + unset($this->recheckTickingChunks[$chunkHash]); } } } @@ -1335,7 +1358,7 @@ class World implements ChunkManager{ $lightPopulatedState = $this->chunks[$chunkHash]->isLightPopulated(); if($lightPopulatedState === false){ $this->chunks[$chunkHash]->setLightPopulated(null); - $this->markTickingChunkUnavailable($chunkX, $chunkZ); + $this->markTickingChunkForRecheck($chunkX, $chunkZ); $this->workerPool->submitTask(new LightPopulationTask( $this->chunks[$chunkHash], @@ -1359,6 +1382,7 @@ class World implements ChunkManager{ $chunk->getSubChunk($y)->setBlockSkyLightArray($lightArray); } $chunk->setLightPopulated(true); + $this->markTickingChunkForRecheck($chunkX, $chunkZ); } )); } @@ -2403,7 +2427,7 @@ class World implements ChunkManager{ throw new \InvalidArgumentException("Chunk $chunkX $chunkZ is already locked"); } $this->chunkLock[$chunkHash] = $lockId; - $this->markTickingChunkUnavailable($chunkX, $chunkZ); + $this->markTickingChunkForRecheck($chunkX, $chunkZ); } /** @@ -2418,6 +2442,7 @@ class World implements ChunkManager{ $chunkHash = World::chunkHash($chunkX, $chunkZ); if(isset($this->chunkLock[$chunkHash]) && ($lockId === null || $this->chunkLock[$chunkHash] === $lockId)){ unset($this->chunkLock[$chunkHash]); + $this->markTickingChunkForRecheck($chunkX, $chunkZ); return true; } return false; @@ -2469,7 +2494,7 @@ class World implements ChunkManager{ unset($this->blockCache[$chunkHash]); unset($this->changedBlocks[$chunkHash]); $chunk->setTerrainDirty(); - $this->markTickingChunkUnavailable($chunkX, $chunkZ); //this replacement chunk may not meet the conditions for ticking + $this->markTickingChunkForRecheck($chunkX, $chunkZ); //this replacement chunk may not meet the conditions for ticking if(!$this->isChunkInUse($chunkX, $chunkZ)){ $this->unloadChunkRequest($chunkX, $chunkZ); @@ -2751,6 +2776,7 @@ class World implements ChunkManager{ foreach($this->getChunkListeners($x, $z) as $listener){ $listener->onChunkLoaded($x, $z, $this->chunks[$chunkHash]); } + $this->markTickingChunkForRecheck($x, $z); //tickers may have been registered before the chunk was loaded $this->timings->syncChunkLoad->stopTiming(); @@ -2912,8 +2938,8 @@ class World implements ChunkManager{ unset($this->chunks[$chunkHash]); unset($this->blockCache[$chunkHash]); unset($this->changedBlocks[$chunkHash]); - unset($this->tickingChunks[$chunkHash]); - $this->markTickingChunkUnavailable($x, $z); + unset($this->registeredTickingChunks[$chunkHash]); + $this->markTickingChunkForRecheck($x, $z); if(array_key_exists($chunkHash, $this->chunkPopulationRequestMap)){ $this->logger->debug("Rejecting population promise for chunk $x $z"); From 709d874204845fb8831b17da29d382643391af0d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 20:27:05 +0100 Subject: [PATCH 08/25] BaseInventory: clean up max stack size handling we can safely assume that: - the inventory's max stack size won't change during the operation - two items which stack together have the same max stack size - the item's max stack size won't change during the operation --- src/inventory/BaseInventory.php | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/inventory/BaseInventory.php b/src/inventory/BaseInventory.php index d92ee92db..a90a4e2b9 100644 --- a/src/inventory/BaseInventory.php +++ b/src/inventory/BaseInventory.php @@ -171,13 +171,15 @@ abstract class BaseInventory implements Inventory{ public function getAddableItemQuantity(Item $item) : int{ $count = $item->getCount(); + $maxStackSize = min($this->getMaxStackSize(), $item->getMaxStackSize()); + for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ if($this->isSlotEmpty($i)){ - $count -= min($this->getMaxStackSize(), $item->getMaxStackSize()); + $count -= $maxStackSize; }else{ $slot = $this->getItem($i); if($item->canStackWith($slot)){ - if(($diff = min($slot->getMaxStackSize(), $item->getMaxStackSize()) - $slot->getCount()) > 0){ + if(($diff = $maxStackSize - $slot->getCount()) > 0){ $count -= $diff; } } @@ -217,6 +219,8 @@ abstract class BaseInventory implements Inventory{ private function internalAddItem(Item $slot) : Item{ $emptySlots = []; + $maxStackSize = min($this->getMaxStackSize(), $slot->getMaxStackSize()); + for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ if($this->isSlotEmpty($i)){ $emptySlots[] = $i; @@ -224,8 +228,8 @@ abstract class BaseInventory implements Inventory{ } $item = $this->getItem($i); - if($slot->canStackWith($item) && $item->getCount() < $item->getMaxStackSize()){ - $amount = min($item->getMaxStackSize() - $item->getCount(), $slot->getCount(), $this->getMaxStackSize()); + if($slot->canStackWith($item) && $item->getCount() < $maxStackSize){ + $amount = min($maxStackSize - $item->getCount(), $slot->getCount()); if($amount > 0){ $slot->setCount($slot->getCount() - $amount); $item->setCount($item->getCount() + $amount); @@ -239,7 +243,7 @@ abstract class BaseInventory implements Inventory{ if(count($emptySlots) > 0){ foreach($emptySlots as $slotIndex){ - $amount = min($slot->getMaxStackSize(), $slot->getCount(), $this->getMaxStackSize()); + $amount = min($maxStackSize, $slot->getCount()); $slot->setCount($slot->getCount() - $amount); $item = clone $slot; $item->setCount($amount); From 42288805097dae66aeec288030b8767ac5b36c1e Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 20:29:02 +0100 Subject: [PATCH 09/25] BaseInventory: change dumb variable names in internalAddItem() --- src/inventory/BaseInventory.php | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/inventory/BaseInventory.php b/src/inventory/BaseInventory.php index a90a4e2b9..6154bcb90 100644 --- a/src/inventory/BaseInventory.php +++ b/src/inventory/BaseInventory.php @@ -216,25 +216,25 @@ abstract class BaseInventory implements Inventory{ return $returnSlots; } - private function internalAddItem(Item $slot) : Item{ + private function internalAddItem(Item $newItem) : Item{ $emptySlots = []; - $maxStackSize = min($this->getMaxStackSize(), $slot->getMaxStackSize()); + $maxStackSize = min($this->getMaxStackSize(), $newItem->getMaxStackSize()); for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ if($this->isSlotEmpty($i)){ $emptySlots[] = $i; continue; } - $item = $this->getItem($i); + $slotItem = $this->getItem($i); - if($slot->canStackWith($item) && $item->getCount() < $maxStackSize){ - $amount = min($maxStackSize - $item->getCount(), $slot->getCount()); + if($newItem->canStackWith($slotItem) && $slotItem->getCount() < $maxStackSize){ + $amount = min($maxStackSize - $slotItem->getCount(), $newItem->getCount()); if($amount > 0){ - $slot->setCount($slot->getCount() - $amount); - $item->setCount($item->getCount() + $amount); - $this->setItem($i, $item); - if($slot->getCount() <= 0){ + $newItem->setCount($newItem->getCount() - $amount); + $slotItem->setCount($slotItem->getCount() + $amount); + $this->setItem($i, $slotItem); + if($newItem->getCount() <= 0){ break; } } @@ -243,18 +243,18 @@ abstract class BaseInventory implements Inventory{ if(count($emptySlots) > 0){ foreach($emptySlots as $slotIndex){ - $amount = min($maxStackSize, $slot->getCount()); - $slot->setCount($slot->getCount() - $amount); - $item = clone $slot; - $item->setCount($amount); - $this->setItem($slotIndex, $item); - if($slot->getCount() <= 0){ + $amount = min($maxStackSize, $newItem->getCount()); + $newItem->setCount($newItem->getCount() - $amount); + $slotItem = clone $newItem; + $slotItem->setCount($amount); + $this->setItem($slotIndex, $slotItem); + if($newItem->getCount() <= 0){ break; } } } - return $slot; + return $newItem; } public function remove(Item $item) : void{ From eb136e60c8011548e0d84f5def1cc1063e83559e Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 21:08:35 +0100 Subject: [PATCH 10/25] BaseInventory: added getMatchingItemCount() helper this eliminates the performance issues described by #5719. closes #5719 we may want to consider exposing a public API for this in the future, since it might be useful for plugins. --- src/inventory/BaseInventory.php | 83 ++++++++++++++++++------------- src/inventory/SimpleInventory.php | 5 ++ 2 files changed, 54 insertions(+), 34 deletions(-) diff --git a/src/inventory/BaseInventory.php b/src/inventory/BaseInventory.php index 6154bcb90..433fcbf87 100644 --- a/src/inventory/BaseInventory.php +++ b/src/inventory/BaseInventory.php @@ -108,13 +108,23 @@ abstract class BaseInventory implements Inventory{ $this->onContentChange($oldContents); } + /** + * Helper for utility functions which search the inventory. + * TODO: make this abstract instead of providing a slow default implementation (BC break) + */ + protected function getMatchingItemCount(int $slot, Item $test, bool $checkDamage, bool $checkTags) : int{ + $item = $this->getItem($slot); + return $item->equals($test, $checkDamage, $checkTags) ? $item->getCount() : 0; + } + public function contains(Item $item) : bool{ $count = max(1, $item->getCount()); $checkDamage = !$item->hasAnyDamageValue(); $checkTags = $item->hasNamedTag(); - foreach($this->getContents() as $i){ - if($item->equals($i, $checkDamage, $checkTags)){ - $count -= $i->getCount(); + for($i = 0, $size = $this->getSize(); $i < $size; $i++){ + $slotCount = $this->getMatchingItemCount($i, $item, $checkDamage, $checkTags); + if($slotCount > 0){ + $count -= $slotCount; if($count <= 0){ return true; } @@ -128,9 +138,9 @@ abstract class BaseInventory implements Inventory{ $slots = []; $checkDamage = !$item->hasAnyDamageValue(); $checkTags = $item->hasNamedTag(); - foreach($this->getContents() as $index => $i){ - if($item->equals($i, $checkDamage, $checkTags)){ - $slots[$index] = $i; + for($i = 0, $size = $this->getSize(); $i < $size; $i++){ + if($this->getMatchingItemCount($i, $item, $checkDamage, $checkTags) > 0){ + $slots[$i] = $this->getItem($i); } } @@ -142,9 +152,10 @@ abstract class BaseInventory implements Inventory{ $checkDamage = $exact || !$item->hasAnyDamageValue(); $checkTags = $exact || $item->hasNamedTag(); - foreach($this->getContents() as $index => $i){ - if($item->equals($i, $checkDamage, $checkTags) && ($i->getCount() === $count || (!$exact && $i->getCount() > $count))){ - return $index; + for($i = 0, $size = $this->getSize(); $i < $size; $i++){ + $slotCount = $this->getMatchingItemCount($i, $item, $checkDamage, $checkTags); + if($slotCount > 0 && ($slotCount === $count || (!$exact && $slotCount > $count))){ + return $i; } } @@ -177,11 +188,9 @@ abstract class BaseInventory implements Inventory{ if($this->isSlotEmpty($i)){ $count -= $maxStackSize; }else{ - $slot = $this->getItem($i); - if($item->canStackWith($slot)){ - if(($diff = $maxStackSize - $slot->getCount()) > 0){ - $count -= $diff; - } + $slotCount = $this->getMatchingItemCount($i, $item, true, true); + if($slotCount > 0 && ($diff = $maxStackSize - $slotCount) > 0){ + $count -= $diff; } } @@ -226,12 +235,16 @@ abstract class BaseInventory implements Inventory{ $emptySlots[] = $i; continue; } - $slotItem = $this->getItem($i); + $slotCount = $this->getMatchingItemCount($i, $newItem, true, true); + if($slotCount === 0){ + continue; + } - if($newItem->canStackWith($slotItem) && $slotItem->getCount() < $maxStackSize){ - $amount = min($maxStackSize - $slotItem->getCount(), $newItem->getCount()); + if($slotCount < $maxStackSize){ + $amount = min($maxStackSize - $slotCount, $newItem->getCount()); if($amount > 0){ $newItem->setCount($newItem->getCount() - $amount); + $slotItem = $this->getItem($i); $slotItem->setCount($slotItem->getCount() + $amount); $this->setItem($i, $slotItem); if($newItem->getCount() <= 0){ @@ -261,20 +274,20 @@ abstract class BaseInventory implements Inventory{ $checkDamage = !$item->hasAnyDamageValue(); $checkTags = $item->hasNamedTag(); - foreach($this->getContents() as $index => $i){ - if($item->equals($i, $checkDamage, $checkTags)){ - $this->clear($index); + for($i = 0, $size = $this->getSize(); $i < $size; $i++){ + if($this->getMatchingItemCount($i, $item, $checkDamage, $checkTags) > 0){ + $this->clear($i); } } } public function removeItem(Item ...$slots) : array{ - /** @var Item[] $itemSlots */ + /** @var Item[] $searchItems */ /** @var Item[] $slots */ - $itemSlots = []; + $searchItems = []; foreach($slots as $slot){ if(!$slot->isNull()){ - $itemSlots[] = clone $slot; + $searchItems[] = clone $slot; } } @@ -282,26 +295,28 @@ abstract class BaseInventory implements Inventory{ if($this->isSlotEmpty($i)){ continue; } - $item = $this->getItem($i); - foreach($itemSlots as $index => $slot){ - if($slot->equals($item, !$slot->hasAnyDamageValue(), $slot->hasNamedTag())){ - $amount = min($item->getCount(), $slot->getCount()); - $slot->setCount($slot->getCount() - $amount); - $item->setCount($item->getCount() - $amount); - $this->setItem($i, $item); - if($slot->getCount() <= 0){ - unset($itemSlots[$index]); + foreach($searchItems as $index => $search){ + $slotCount = $this->getMatchingItemCount($i, $search, !$search->hasAnyDamageValue(), $search->hasNamedTag()); + if($slotCount > 0){ + $amount = min($slotCount, $search->getCount()); + $search->setCount($search->getCount() - $amount); + + $slotItem = $this->getItem($i); + $slotItem->setCount($slotItem->getCount() - $amount); + $this->setItem($i, $slotItem); + if($search->getCount() <= 0){ + unset($searchItems[$index]); } } } - if(count($itemSlots) === 0){ + if(count($searchItems) === 0){ break; } } - return $itemSlots; + return $searchItems; } public function clear(int $index) : void{ diff --git a/src/inventory/SimpleInventory.php b/src/inventory/SimpleInventory.php index 19fbdbb17..c1f352b7b 100644 --- a/src/inventory/SimpleInventory.php +++ b/src/inventory/SimpleInventory.php @@ -84,6 +84,11 @@ class SimpleInventory extends BaseInventory{ } } + protected function getMatchingItemCount(int $slot, Item $test, bool $checkDamage, bool $checkTags) : int{ + $slotItem = $this->slots[$slot]; + return $slotItem !== null && $slotItem->equals($test, $checkDamage, $checkTags) ? $slotItem->getCount() : 0; + } + public function isSlotEmpty(int $index) : bool{ return $this->slots[$index] === null || $this->slots[$index]->isNull(); } From 73bf5d4b29f6e64edc28aee900614b78a62192bb Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Apr 2023 21:17:55 +0100 Subject: [PATCH 11/25] DoubleChestInventory: specialize isSlotEmpty() and getMatchingItemCount() --- src/block/inventory/DoubleChestInventory.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/block/inventory/DoubleChestInventory.php b/src/block/inventory/DoubleChestInventory.php index 92c75ef9f..8c38e190d 100644 --- a/src/block/inventory/DoubleChestInventory.php +++ b/src/block/inventory/DoubleChestInventory.php @@ -85,6 +85,20 @@ class DoubleChestInventory extends BaseInventory implements BlockInventory, Inve $this->right->setContents($rightContents); } + protected function getMatchingItemCount(int $slot, Item $test, bool $checkDamage, bool $checkTags) : int{ + $leftSize = $this->left->getSize(); + return $slot < $leftSize ? + $this->left->getMatchingItemCount($slot, $test, $checkDamage, $checkTags) : + $this->right->getMatchingItemCount($slot - $leftSize, $test, $checkDamage, $checkTags); + } + + public function isSlotEmpty(int $index) : bool{ + $leftSize = $this->left->getSize(); + return $index < $leftSize ? + $this->left->isSlotEmpty($index) : + $this->right->isSlotEmpty($index - $leftSize); + } + protected function getOpenSound() : Sound{ return new ChestOpenSound(); } protected function getCloseSound() : Sound{ return new ChestCloseSound(); } From b70ff325488093f18899ac9935551220334bd4b9 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 28 Apr 2023 13:54:40 +0100 Subject: [PATCH 12/25] ItemTranslator: Fixed log items not displaying correctly on the client closes #5724 this uses a (potentially bogus) assumption that the lowest mapped meta value associated with an ID is valid. I don't want to break this during a patch release, and this works for now. In the future it would probably make more sense to bypass ItemTranslator entirely, and rely solely on the blockstate returned by RuntimeBlockMapping to fetch the correct ID. This is similar to how we serialize items for saving on disk in PM5. --- src/network/mcpe/convert/ItemTranslator.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/network/mcpe/convert/ItemTranslator.php b/src/network/mcpe/convert/ItemTranslator.php index 8342db61f..e3e901e66 100644 --- a/src/network/mcpe/convert/ItemTranslator.php +++ b/src/network/mcpe/convert/ItemTranslator.php @@ -110,6 +110,12 @@ final class ItemTranslator{ //new item without a fixed legacy ID - we can't handle this right now continue; } + if(isset($complexMappings[$newId]) && $complexMappings[$newId][0] === $intId && $complexMappings[$newId][1] <= $meta){ + //TODO: HACK! Multiple legacy ID/meta pairs can be mapped to the same new ID (see minecraft:log) + //Assume that the first one is the most relevant for now + //However, this could catch fire in the future if this assumption is broken + continue; + } $complexMappings[$newId] = [$intId, (int) $meta]; } } From 5dcd8bf2898b49e43f2cc0e0b35fcabf4de09136 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 1 May 2023 13:30:02 +0100 Subject: [PATCH 13/25] Bump symfony/filesystem from 5.4.21 to 5.4.23 (#5730) Bumps [symfony/filesystem](https://github.com/symfony/filesystem) from 5.4.21 to 5.4.23. - [Release notes](https://github.com/symfony/filesystem/releases) - [Changelog](https://github.com/symfony/filesystem/blob/6.2/CHANGELOG.md) - [Commits](https://github.com/symfony/filesystem/compare/v5.4.21...v5.4.23) --- updated-dependencies: - dependency-name: symfony/filesystem dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- composer.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/composer.lock b/composer.lock index ee994f84a..bad0235cb 100644 --- a/composer.lock +++ b/composer.lock @@ -1085,16 +1085,16 @@ }, { "name": "symfony/filesystem", - "version": "v5.4.21", + "version": "v5.4.23", "source": { "type": "git", "url": "https://github.com/symfony/filesystem.git", - "reference": "e75960b1bbfd2b8c9e483e0d74811d555ca3de9f" + "reference": "b2f79d86cd9e7de0fff6d03baa80eaed7a5f38b5" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/filesystem/zipball/e75960b1bbfd2b8c9e483e0d74811d555ca3de9f", - "reference": "e75960b1bbfd2b8c9e483e0d74811d555ca3de9f", + "url": "https://api.github.com/repos/symfony/filesystem/zipball/b2f79d86cd9e7de0fff6d03baa80eaed7a5f38b5", + "reference": "b2f79d86cd9e7de0fff6d03baa80eaed7a5f38b5", "shasum": "" }, "require": { @@ -1129,7 +1129,7 @@ "description": "Provides basic utilities for the filesystem", "homepage": "https://symfony.com", "support": { - "source": "https://github.com/symfony/filesystem/tree/v5.4.21" + "source": "https://github.com/symfony/filesystem/tree/v5.4.23" }, "funding": [ { @@ -1145,7 +1145,7 @@ "type": "tidelift" } ], - "time": "2023-02-14T08:03:56+00:00" + "time": "2023-03-02T11:38:35+00:00" }, { "name": "symfony/polyfill-ctype", From f04151dbe65a4dce6d844bb674d7cd0716f1ef70 Mon Sep 17 00:00:00 2001 From: Lee Siu San <15855635+leolee3914@users.noreply.github.com> Date: Mon, 1 May 2023 21:08:20 +0800 Subject: [PATCH 14/25] README: next-major branch was renamed (#5731) [ci skip] --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index f91fdd31d..fb3a09459 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ * [Building and running from source](BUILDING.md) * [Developer documentation](https://devdoc.pmmp.io) - General documentation for PocketMine-MP plugin developers * [Latest release API documentation](https://apidoc.pmmp.io) - Doxygen API documentation generated for each release - * [Latest bleeding-edge API documentation](https://apidoc-dev.pmmp.io) - Doxygen API documentation generated weekly from `next-major` branch + * [Latest bleeding-edge API documentation](https://apidoc-dev.pmmp.io) - Doxygen API documentation generated weekly from `major-next` branch * [DevTools](https://github.com/pmmp/DevTools/) - Development tools plugin for creating plugins * [ExamplePlugin](https://github.com/pmmp/ExamplePlugin/) - Example plugin demonstrating some basic API features * [Contributing Guidelines](CONTRIBUTING.md) From 22f8623e176e6cff5d87069fc300c191b78c2ceb Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 4 May 2023 21:06:27 +0100 Subject: [PATCH 15/25] Release 4.20.2 --- changelogs/4.20.md | 7 +++++++ src/VersionInfo.php | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/changelogs/4.20.md b/changelogs/4.20.md index d8f4c66b0..4a91ab440 100644 --- a/changelogs/4.20.md +++ b/changelogs/4.20.md @@ -44,3 +44,10 @@ Released 27th April 2023. ## Internals - `ItemStackContainerIdTranslator::translate()` now requires an additional `int $slotId` parameter and returns `array{int, int}` (translated window ID, translated slot ID) to be used with `InventoryManager->locateWindowAndSlot()`. - `InventoryManager->locateWindowAndSlot()` now checks if the translated slot actually exists in the requested inventory, and returns `null` if not. Previously, it would return potentially invalid slot IDs without checking them, potentially leading to crashes. + +# 4.20.2 +Released 4th May 2023. + +## Fixes +- Fixed all types of wooden logs appearing as oak in the inventory. +- Fixed a performance issue in `BaseInventory->canAddItem()` (missing `continue` causing useless logic to run). diff --git a/src/VersionInfo.php b/src/VersionInfo.php index 08582cc58..0307d7845 100644 --- a/src/VersionInfo.php +++ b/src/VersionInfo.php @@ -32,7 +32,7 @@ use function str_repeat; final class VersionInfo{ public const NAME = "PocketMine-MP"; public const BASE_VERSION = "4.20.2"; - public const IS_DEVELOPMENT_BUILD = true; + public const IS_DEVELOPMENT_BUILD = false; public const BUILD_CHANNEL = "stable"; private function __construct(){ From c09390d20fd10a3e3671195f86d895cf696ce645 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 4 May 2023 21:06:30 +0100 Subject: [PATCH 16/25] 4.20.3 is next --- src/VersionInfo.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/VersionInfo.php b/src/VersionInfo.php index 0307d7845..df44e6982 100644 --- a/src/VersionInfo.php +++ b/src/VersionInfo.php @@ -31,8 +31,8 @@ use function str_repeat; final class VersionInfo{ public const NAME = "PocketMine-MP"; - public const BASE_VERSION = "4.20.2"; - public const IS_DEVELOPMENT_BUILD = false; + public const BASE_VERSION = "4.20.3"; + public const IS_DEVELOPMENT_BUILD = true; public const BUILD_CHANNEL = "stable"; private function __construct(){ From 092d130c963bb2deedc9a10bc178def407b58ef2 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 4 May 2023 23:01:10 +0100 Subject: [PATCH 17/25] RuntimeBlockMapping: borrow a hack from PM5 to reduce memory footprint we can't change the internals of this on a patch release, but this hack provides a 12 MB memory usage reduction, which is very significant. --- .../mcpe/convert/RuntimeBlockMapping.php | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/network/mcpe/convert/RuntimeBlockMapping.php b/src/network/mcpe/convert/RuntimeBlockMapping.php index dd396af9b..64984c3c1 100644 --- a/src/network/mcpe/convert/RuntimeBlockMapping.php +++ b/src/network/mcpe/convert/RuntimeBlockMapping.php @@ -27,7 +27,10 @@ use pocketmine\block\Block; use pocketmine\block\BlockLegacyIds; use pocketmine\data\bedrock\BedrockDataFiles; use pocketmine\data\bedrock\LegacyBlockIdToStringIdMap; +use pocketmine\nbt\tag\ByteTag; use pocketmine\nbt\tag\CompoundTag; +use pocketmine\nbt\tag\IntTag; +use pocketmine\nbt\tag\StringTag; use pocketmine\network\mcpe\protocol\serializer\NetworkNbtSerializer; use pocketmine\utils\BinaryStream; use pocketmine\utils\Filesystem; @@ -53,15 +56,45 @@ final class RuntimeBlockMapping{ ); } + /** + * @param string[] $keyIndex + * @param (ByteTag|StringTag|IntTag)[][] $valueIndex + * @phpstan-param array $keyIndex + * @phpstan-param array> $valueIndex + */ + private static function deduplicateCompound(CompoundTag $tag, array &$keyIndex, array &$valueIndex) : CompoundTag{ + if($tag->count() === 0){ + return $tag; + } + + $newTag = CompoundTag::create(); + foreach($tag as $key => $value){ + $key = $keyIndex[$key] ??= $key; + + if($value instanceof CompoundTag){ + $value = self::deduplicateCompound($value, $keyIndex, $valueIndex); + }elseif($value instanceof ByteTag || $value instanceof IntTag || $value instanceof StringTag){ + $value = $valueIndex[$value->getType()][$value->getValue()] ??= $value; + } + + $newTag->setTag($key, $value); + } + + return $newTag; + } + public function __construct(string $canonicalBlockStatesFile, string $r12ToCurrentBlockMapFile){ $stream = new BinaryStream(Filesystem::fileGetContents($canonicalBlockStatesFile)); $list = []; $nbtReader = new NetworkNbtSerializer(); + + $keyIndex = []; + $valueIndex = []; while(!$stream->feof()){ $offset = $stream->getOffset(); $blockState = $nbtReader->read($stream->getBuffer(), $offset)->mustGetCompoundTag(); $stream->setOffset($offset); - $list[] = $blockState; + $list[] = self::deduplicateCompound($blockState, $keyIndex, $valueIndex); } $this->bedrockKnownStates = $list; From 633e77a34cf9741a48fd9b5f19e3c83d24fdb02b Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 4 May 2023 23:21:54 +0100 Subject: [PATCH 18/25] RuntimeBlockMapping: share states CompoundTags if they are the same this allows saving about 4 MB of memory, because there are many blocks which have identical states, although they have different IDs. this relies on a potentially risky assumption that the tags in knownStates won't be modified. If they are modified, the changes will influence all blockstates which share the tag. However, I don't expect this to happen, and the 4 MB memory saving is substantial enough to be worth the risk. --- src/network/mcpe/convert/RuntimeBlockMapping.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/network/mcpe/convert/RuntimeBlockMapping.php b/src/network/mcpe/convert/RuntimeBlockMapping.php index 64984c3c1..c2123364b 100644 --- a/src/network/mcpe/convert/RuntimeBlockMapping.php +++ b/src/network/mcpe/convert/RuntimeBlockMapping.php @@ -27,10 +27,12 @@ use pocketmine\block\Block; use pocketmine\block\BlockLegacyIds; use pocketmine\data\bedrock\BedrockDataFiles; use pocketmine\data\bedrock\LegacyBlockIdToStringIdMap; +use pocketmine\nbt\LittleEndianNbtSerializer; use pocketmine\nbt\tag\ByteTag; use pocketmine\nbt\tag\CompoundTag; use pocketmine\nbt\tag\IntTag; use pocketmine\nbt\tag\StringTag; +use pocketmine\nbt\TreeRoot; use pocketmine\network\mcpe\protocol\serializer\NetworkNbtSerializer; use pocketmine\utils\BinaryStream; use pocketmine\utils\Filesystem; @@ -72,7 +74,7 @@ final class RuntimeBlockMapping{ $key = $keyIndex[$key] ??= $key; if($value instanceof CompoundTag){ - $value = self::deduplicateCompound($value, $keyIndex, $valueIndex); + $value = $valueIndex[$value->getType()][(new LittleEndianNbtSerializer())->write(new TreeRoot($value))] ??= self::deduplicateCompound($value, $keyIndex, $valueIndex); }elseif($value instanceof ByteTag || $value instanceof IntTag || $value instanceof StringTag){ $value = $valueIndex[$value->getType()][$value->getValue()] ??= $value; } From 84a943bcec2d3f79a5edbc4acc345f7c20ab8b9d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 5 May 2023 16:06:37 +0100 Subject: [PATCH 19/25] BaseInventory: slap a TODO on isSlotEmpty() --- src/inventory/BaseInventory.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/inventory/BaseInventory.php b/src/inventory/BaseInventory.php index 433fcbf87..308850ffd 100644 --- a/src/inventory/BaseInventory.php +++ b/src/inventory/BaseInventory.php @@ -172,6 +172,10 @@ abstract class BaseInventory implements Inventory{ return -1; } + /** + * TODO: make this abstract and force implementations to implement it properly (BC break) + * This default implementation works, but is slow. + */ public function isSlotEmpty(int $index) : bool{ return $this->getItem($index)->isNull(); } From 02cf5ed388c246e1177e4b3fe07eed0d5a3ac405 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 5 May 2023 16:18:03 +0100 Subject: [PATCH 20/25] RuntimeBlockMapping: lazy-load NBT blockstates this saves a considerable amount of memory. we don't actually need this state array in PM4 anyway, since we don't support the client-side chunk cache yet. when the time comes to support it, it'll be much more practical to cache binary states and copy bytes anyway, instead of doing it the current way, which is both slow and memory-intensive. Measured footprint change: 9 MB -> 400 KB. --- .../mcpe/convert/RuntimeBlockMapping.php | 58 +++++++++++-------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/src/network/mcpe/convert/RuntimeBlockMapping.php b/src/network/mcpe/convert/RuntimeBlockMapping.php index c2123364b..1ad2c5ff7 100644 --- a/src/network/mcpe/convert/RuntimeBlockMapping.php +++ b/src/network/mcpe/convert/RuntimeBlockMapping.php @@ -48,8 +48,8 @@ final class RuntimeBlockMapping{ private array $legacyToRuntimeMap = []; /** @var int[] */ private array $runtimeToLegacyMap = []; - /** @var CompoundTag[] */ - private array $bedrockKnownStates; + /** @var CompoundTag[]|null */ + private ?array $bedrockKnownStates = null; private static function make() : self{ return new self( @@ -85,25 +85,13 @@ final class RuntimeBlockMapping{ return $newTag; } - public function __construct(string $canonicalBlockStatesFile, string $r12ToCurrentBlockMapFile){ - $stream = new BinaryStream(Filesystem::fileGetContents($canonicalBlockStatesFile)); - $list = []; - $nbtReader = new NetworkNbtSerializer(); + public function __construct( + private string $canonicalBlockStatesFile, + string $r12ToCurrentBlockMapFile + ){ + //do not cache this - we only need it to set up mappings under normal circumstances + $bedrockKnownStates = $this->loadBedrockKnownStates(); - $keyIndex = []; - $valueIndex = []; - while(!$stream->feof()){ - $offset = $stream->getOffset(); - $blockState = $nbtReader->read($stream->getBuffer(), $offset)->mustGetCompoundTag(); - $stream->setOffset($offset); - $list[] = self::deduplicateCompound($blockState, $keyIndex, $valueIndex); - } - $this->bedrockKnownStates = $list; - - $this->setupLegacyMappings($r12ToCurrentBlockMapFile); - } - - private function setupLegacyMappings(string $r12ToCurrentBlockMapFile) : void{ $legacyIdMap = LegacyBlockIdToStringIdMap::getInstance(); /** @var R12ToCurrentBlockMapEntry[] $legacyStateMap */ $legacyStateMap = []; @@ -123,7 +111,7 @@ final class RuntimeBlockMapping{ * @var int[][] $idToStatesMap string id -> int[] list of candidate state indices */ $idToStatesMap = []; - foreach($this->bedrockKnownStates as $k => $state){ + foreach($bedrockKnownStates as $k => $state){ $idToStatesMap[$state->getString("name")][] = $k; } foreach($legacyStateMap as $pair){ @@ -142,7 +130,7 @@ final class RuntimeBlockMapping{ throw new \RuntimeException("Mapped new state does not appear in network table"); } foreach($idToStatesMap[$mappedName] as $k){ - $networkState = $this->bedrockKnownStates[$k]; + $networkState = $bedrockKnownStates[$k]; if($mappedState->equals($networkState)){ $this->registerMapping($k, $id, $data); continue 2; @@ -152,6 +140,25 @@ final class RuntimeBlockMapping{ } } + /** + * @return CompoundTag[] + */ + private function loadBedrockKnownStates() : array{ + $stream = new BinaryStream(Filesystem::fileGetContents($this->canonicalBlockStatesFile)); + $list = []; + $nbtReader = new NetworkNbtSerializer(); + + $keyIndex = []; + $valueIndex = []; + while(!$stream->feof()){ + $offset = $stream->getOffset(); + $blockState = $nbtReader->read($stream->getBuffer(), $offset)->mustGetCompoundTag(); + $stream->setOffset($offset); + $list[] = self::deduplicateCompound($blockState, $keyIndex, $valueIndex); + } + return $list; + } + public function toRuntimeId(int $internalStateId) : int{ return $this->legacyToRuntimeMap[$internalStateId] ?? $this->legacyToRuntimeMap[BlockLegacyIds::INFO_UPDATE << Block::INTERNAL_METADATA_BITS]; } @@ -166,9 +173,14 @@ final class RuntimeBlockMapping{ } /** + * WARNING: This method may load the palette from disk, which is a slow operation. + * Afterwards, it will cache the palette in memory, which requires (in some cases) tens of MB of memory. + * Avoid using this where possible. + * + * @deprecated * @return CompoundTag[] */ public function getBedrockKnownStates() : array{ - return $this->bedrockKnownStates; + return $this->bedrockKnownStates ??= $this->loadBedrockKnownStates(); } } From d04da9b1d8292e8f5b8ca699ba68917d3b3a8f7c Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 6 May 2023 15:42:52 +0100 Subject: [PATCH 21/25] Reuse timings handlers for event handlers of the same events due to direct repeated usage of registerEvent() with closures, we've seen some libraries like muqsit/SimplePacketHandler generate very large timings reports, because a new timings handler gets created every time a plugin registers or unregisters a new packet handler callback. This change fixes the problem by ensuring that any handlers derived from the same function, handling the same event class, will share the same timer. --- src/plugin/PluginManager.php | 4 ++-- src/timings/Timings.php | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/plugin/PluginManager.php b/src/plugin/PluginManager.php index d109b23fa..725c915a1 100644 --- a/src/plugin/PluginManager.php +++ b/src/plugin/PluginManager.php @@ -37,7 +37,7 @@ use pocketmine\permission\DefaultPermissions; use pocketmine\permission\PermissionManager; use pocketmine\permission\PermissionParser; use pocketmine\Server; -use pocketmine\timings\TimingsHandler; +use pocketmine\timings\Timings; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\Utils; use Symfony\Component\Filesystem\Path; @@ -651,7 +651,7 @@ class PluginManager{ throw new PluginException("Plugin attempted to register event handler " . $handlerName . "() to event " . $event . " while not enabled"); } - $timings = new TimingsHandler($handlerName . "(" . (new \ReflectionClass($event))->getShortName() . ")", group: $plugin->getDescription()->getFullName()); + $timings = Timings::getEventHandlerTimings($event, $handlerName, $plugin->getDescription()->getFullName()); $registeredListener = new RegisteredListener($handler, $priority, $plugin, $handleCancelled, $timings); HandlerListManager::global()->getListFor($event)->register($registeredListener); diff --git a/src/timings/Timings.php b/src/timings/Timings.php index a400a73b4..74fa40dde 100644 --- a/src/timings/Timings.php +++ b/src/timings/Timings.php @@ -166,6 +166,8 @@ abstract class Timings{ /** @var TimingsHandler[] */ private static array $events = []; + /** @var TimingsHandler[][] */ + private static array $eventHandlers = []; public static function init() : void{ if(self::$initialized){ @@ -336,4 +338,16 @@ abstract class Timings{ return self::$events[$eventClass]; } + + /** + * @phpstan-template TEvent of Event + * @phpstan-param class-string $event + */ + public static function getEventHandlerTimings(string $event, string $handlerName, string $group) : TimingsHandler{ + if(!isset(self::$eventHandlers[$event][$handlerName])){ + self::$eventHandlers[$event][$handlerName] = new TimingsHandler($handlerName . "(" . self::shortenCoreClassName($event, "pocketmine\\event\\") . ")", group: $group); + } + + return self::$eventHandlers[$event][$handlerName]; + } } From 4caa2c7690464d1be77f53294f6644087f8e3e39 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 6 May 2023 15:54:11 +0100 Subject: [PATCH 22/25] NetworkSession: send FLYING flag on spectator ability layer fixes #5722 I'm not very clear why this works. PM doesn't use real spectator mode yet (we're still using the faux spectator mode PM has had for years, because I haven't yet assessed how real spectator mode will affect stuff like block interactions), so this ability layer shouldn't have any effect. thank you @Alemiz112 --- src/network/mcpe/NetworkSession.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/network/mcpe/NetworkSession.php b/src/network/mcpe/NetworkSession.php index bff5875b7..1b90547e7 100644 --- a/src/network/mcpe/NetworkSession.php +++ b/src/network/mcpe/NetworkSession.php @@ -894,6 +894,12 @@ class NetworkSession{ [ //TODO: dynamic flying speed! FINALLY!!!!!!!!!!!!!!!!! new AbilitiesLayer(AbilitiesLayer::LAYER_BASE, $boolAbilities, 0.05, 0.1), + + //TODO: HACK! In 1.19.80, the client starts falling in our faux spectator mode when it clips into a + //block. I have no idea why this works, since we don't actually use the real spectator mode. + new AbilitiesLayer(AbilitiesLayer::LAYER_SPECTATOR, [ + AbilitiesLayer::ABILITY_FLYING => true, + ], null, null) ] ))); } From fa715a074ab8af61bae4c0b754785d5f91288563 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 6 May 2023 16:56:39 +0100 Subject: [PATCH 23/25] Fixed TimingsHandler depth not getting reset when timings is disabled When timings was disabled, internalStopTiming is not called, and timer depth is not decremented. If timings is later reenabled, the next call to internalStartTiming will think the timer is already running, and won't generate any new records for the timer. This has led to broken timings reports with missing Full Server Tick entries, amongst other things. --- src/timings/TimingsHandler.php | 5 +++-- src/timings/TimingsRecord.php | 7 +++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/timings/TimingsHandler.php b/src/timings/TimingsHandler.php index be4979f15..9a8234310 100644 --- a/src/timings/TimingsHandler.php +++ b/src/timings/TimingsHandler.php @@ -111,7 +111,7 @@ class TimingsHandler{ } public static function reload() : void{ - TimingsRecord::clearRecords(); + TimingsRecord::reset(); if(self::$enabled){ self::$timingStart = hrtime(true); } @@ -219,8 +219,9 @@ class TimingsHandler{ /** * @internal */ - public function destroyCycles() : void{ + public function reset() : void{ $this->rootRecord = null; $this->recordsByParent = []; + $this->timingDepth = 0; } } diff --git a/src/timings/TimingsRecord.php b/src/timings/TimingsRecord.php index 5254d2e7d..f09984b5b 100644 --- a/src/timings/TimingsRecord.php +++ b/src/timings/TimingsRecord.php @@ -42,9 +42,12 @@ final class TimingsRecord{ private static ?self $currentRecord = null; - public static function clearRecords() : void{ + /** + * @internal + */ + public static function reset() : void{ foreach(self::$records as $record){ - $record->handler->destroyCycles(); + $record->handler->reset(); } self::$records = []; self::$currentRecord = null; From 325ffec1be433cd0ad6321044d23f0efd3e6efee Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 6 May 2023 17:01:49 +0100 Subject: [PATCH 24/25] Release 4.20.3 --- changelogs/4.20.md | 13 +++++++++++++ src/VersionInfo.php | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/changelogs/4.20.md b/changelogs/4.20.md index 4a91ab440..bc8c931b2 100644 --- a/changelogs/4.20.md +++ b/changelogs/4.20.md @@ -51,3 +51,16 @@ Released 4th May 2023. ## Fixes - Fixed all types of wooden logs appearing as oak in the inventory. - Fixed a performance issue in `BaseInventory->canAddItem()` (missing `continue` causing useless logic to run). + +# 4.20.3 +Released 6th May 2023. + +## Improvements +- Reduced memory usage of `RuntimeBlockMapping` from 25 MB to 9 MB. Since every thread has its own copy of the block map, this saves a substantial amount of memory. + +## Fixes +- Fixed players falling through blocks in spectator mode. +- Fixed timings reports getting bloated by prolific usage of `PluginManager->registerEvent()`. + - This was caused by creating a new timings handler for each call, regardless of whether a timer already existed for the given event and callback. +- Fixed `Full Server Tick` and other records being missing from timings reports. + - This was caused by timings handler depth not getting reset when timings was disabled and later re-enabled. diff --git a/src/VersionInfo.php b/src/VersionInfo.php index df44e6982..879ea817c 100644 --- a/src/VersionInfo.php +++ b/src/VersionInfo.php @@ -32,7 +32,7 @@ use function str_repeat; final class VersionInfo{ public const NAME = "PocketMine-MP"; public const BASE_VERSION = "4.20.3"; - public const IS_DEVELOPMENT_BUILD = true; + public const IS_DEVELOPMENT_BUILD = false; public const BUILD_CHANNEL = "stable"; private function __construct(){ From 3b893961e4414f66686dfbb2a6860043e0235552 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 6 May 2023 17:01:49 +0100 Subject: [PATCH 25/25] 4.20.4 is next --- src/VersionInfo.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/VersionInfo.php b/src/VersionInfo.php index 879ea817c..0b72d7f9e 100644 --- a/src/VersionInfo.php +++ b/src/VersionInfo.php @@ -31,8 +31,8 @@ use function str_repeat; final class VersionInfo{ public const NAME = "PocketMine-MP"; - public const BASE_VERSION = "4.20.3"; - public const IS_DEVELOPMENT_BUILD = false; + public const BASE_VERSION = "4.20.4"; + public const IS_DEVELOPMENT_BUILD = true; public const BUILD_CHANNEL = "stable"; private function __construct(){