From f9318bf286844829b27ac56bc08d4a1bcfedc138 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 21 Apr 2023 15:38:11 +0100 Subject: [PATCH 1/8] TimingsHandler: stop throwing exceptions when timers aren't stopped in the right order this is usually because of an uncaught exception interacting with a try...finally block. This will normally result in a crash anyway, and we don't want to obscure the real error. --- src/timings/TimingsHandler.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/timings/TimingsHandler.php b/src/timings/TimingsHandler.php index b77610a65..be4979f15 100644 --- a/src/timings/TimingsHandler.php +++ b/src/timings/TimingsHandler.php @@ -189,12 +189,12 @@ class TimingsHandler{ } $record = TimingsRecord::getCurrentRecord(); - if($record !== null){ - if($record->getTimerId() !== spl_object_id($this)){ - throw new \LogicException("Timer \"" . $record->getName() . "\" should have been stopped before stopping timer \"" . $this->name . "\""); - } + $timerId = spl_object_id($this); + for(; $record !== null && $record->getTimerId() !== $timerId; $record = TimingsRecord::getCurrentRecord()){ + \GlobalLogger::get()->error("Timer \"" . $record->getName() . "\" should have been stopped before stopping timer \"" . $this->name . "\""); $record->stopTiming($now); } + $record?->stopTiming($now); if($this->parent !== null){ $this->parent->internalStopTiming($now); } From 11e34b3e5cdab7639031d8af53aeacf0242ab487 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 21 Apr 2023 15:53:02 +0100 Subject: [PATCH 2/8] Release 4.19.3 --- changelogs/4.19.md | 15 +++++++++++++++ src/VersionInfo.php | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/changelogs/4.19.md b/changelogs/4.19.md index b8127b335..89ef3140b 100644 --- a/changelogs/4.19.md +++ b/changelogs/4.19.md @@ -94,3 +94,18 @@ Released 14th April 2023. - Fixed player timings duplication leading to extremely large timings reports when timings runs for a long time with many players. - Packet timings are now indexed by class FQN instead of packet ID. This prevents erroneous timer reuse on packet ID reuse (e.g. multi version servers). - Fixed entity timings being shared by different classes with the same short name. This led to incorrect timings being reported for some entities when custom entities were used. + +# 4.19.3 +Released 21st April 2023. + +## General +- Error IDs for `Packet processing error` disconnects are now split into 4-character chunks to make them easier to type (since they can't be copied from the disconnection screen of a client). + +## Fixes +- Fixed entity-block intersections being checked twice per tick. Besides wasting CPU time, this may have caused unexpected behaviour during entity-block interactions with blocks like water or cacti. +- Fixed performance issue in network inventory synchronization due item NBT being prepared twice. +- Fixed `tools/simulate-chunk-selector.php` argument parsing being completely broken (weird behaviour of PHP `getopt()`). + +## Internals +- `TimingsHandler->stopTiming()` now logs an error message if a subtimer wasn't stopped, rather than throwing an exception. + - Due to interactions between `try...finally` and unexpected errors, throwing exceptions made it difficult for plugin developers to debug errors in their plugins, since it obscured the original error. diff --git a/src/VersionInfo.php b/src/VersionInfo.php index 42ead9ee5..0858d617b 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.19.3"; - public const IS_DEVELOPMENT_BUILD = true; + public const IS_DEVELOPMENT_BUILD = false; public const BUILD_CHANNEL = "stable"; private function __construct(){ From d06d3bc87173df6340c1e5aa3a8995a149300149 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 21 Apr 2023 15:53:05 +0100 Subject: [PATCH 3/8] 4.19.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 0858d617b..75b43e6f2 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.19.3"; - public const IS_DEVELOPMENT_BUILD = false; + public const BASE_VERSION = "4.19.4"; + public const IS_DEVELOPMENT_BUILD = true; public const BUILD_CHANNEL = "stable"; private function __construct(){ From 84a16ce69ae8b8880b2f3ff4e86fbedbe18348fe Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 21 Apr 2023 16:19:06 +0100 Subject: [PATCH 4/8] HandlerList: fixed crash on getting unused priority these sub-arrays are no longer allocated if no handlers are registered. fixes #5713 --- src/event/HandlerList.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/event/HandlerList.php b/src/event/HandlerList.php index 692e8ac70..efbb20a1b 100644 --- a/src/event/HandlerList.php +++ b/src/event/HandlerList.php @@ -102,7 +102,7 @@ class HandlerList{ * @return RegisteredListener[] */ public function getListenersByPriority(int $priority) : array{ - return $this->handlerSlots[$priority]; + return $this->handlerSlots[$priority] ?? []; } public function getParent() : ?HandlerList{ From f86fde064d27decd89c1ede0fbee7466468ef216 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 24 Apr 2023 12:34:34 +0100 Subject: [PATCH 5/8] CraftingManager: fixed uninitialized field I'm having deja vu about this ... --- src/crafting/CraftingManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crafting/CraftingManager.php b/src/crafting/CraftingManager.php index 3b2bd173a..bb062382a 100644 --- a/src/crafting/CraftingManager.php +++ b/src/crafting/CraftingManager.php @@ -55,7 +55,7 @@ class CraftingManager{ * @var FurnaceRecipeManager[] * @phpstan-var array */ - protected $furnaceRecipeManagers; + protected $furnaceRecipeManagers = []; /** * @var PotionTypeRecipe[][] From 107b56154b3b75e9269eb002e4b77d75c3596c48 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 24 Apr 2023 13:34:55 +0100 Subject: [PATCH 6/8] ItemStackRequestExecutor: fixed stonecutter recipes this uses the same dodgy hack used by CraftingTransaction, which assumes that getResultsFor() does not care about the crafting inputs. While this is currently OK, since none of the currently-implemented recipes care about inputs anyway, it will become a problem when we implement shulker box recipes, so this needs to be addressed. However, it can't be addressed without BC breaks, so this will have to be dealt with in PM5. closes #5715 --- .../mcpe/handler/ItemStackRequestExecutor.php | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index 124606112..b7f9b1604 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -23,7 +23,6 @@ declare(strict_types=1); namespace pocketmine\network\mcpe\handler; -use pocketmine\crafting\CraftingGrid; use pocketmine\inventory\CreativeInventory; use pocketmine\inventory\Inventory; use pocketmine\inventory\transaction\action\CreateItemAction; @@ -246,13 +245,11 @@ class ItemStackRequestExecutor{ $this->specialTransaction = new CraftingTransaction($this->player, $craftingManager, [], $recipe, $repetitions); - $currentWindow = $this->player->getCurrentWindow(); - if($currentWindow !== null && !($currentWindow instanceof CraftingGrid)){ - throw new ItemStackRequestProcessException("Player's current window is not a crafting grid"); - } - $craftingGrid = $currentWindow ?? $this->player->getCraftingGrid(); - - $craftingResults = $recipe->getResultsFor($craftingGrid); + //TODO: Since the system assumes that crafting can only be done in the crafting grid, we have to give it a + //crafting grid to make the API happy. No implementation of getResultsFor() actually uses the crafting grid + //right now, so this will work, but this will become a problem in the future for things like shulker boxes and + //custom crafting recipes. + $craftingResults = $recipe->getResultsFor($this->player->getCraftingGrid()); foreach($craftingResults as $k => $craftingResult){ $craftingResult->setCount($craftingResult->getCount() * $repetitions); $this->craftingResults[$k] = $craftingResult; From 3ecc980bc4331874852bd14b2720b6703900b90e Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 24 Apr 2023 13:42:11 +0100 Subject: [PATCH 7/8] =?UTF-8?q?=C3=82InventoryManagerEntry:=20fixed=20inco?= =?UTF-8?q?rrect=20PHPDoc=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/network/mcpe/InventoryManagerEntry.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/mcpe/InventoryManagerEntry.php b/src/network/mcpe/InventoryManagerEntry.php index ac2a40dfe..deb2e8e4d 100644 --- a/src/network/mcpe/InventoryManagerEntry.php +++ b/src/network/mcpe/InventoryManagerEntry.php @@ -40,7 +40,7 @@ final class InventoryManagerEntry{ public array $itemStackInfos = []; /** - * @var int[] + * @var ItemStack[] * @phpstan-var array */ public array $pendingSyncs = []; From a4f2b99ed55c97180531a4086bbbfb5df3c56177 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 24 Apr 2023 14:06:29 +0100 Subject: [PATCH 8/8] InGamePacketHandler: queue slots for syncing if they appear in requestChangedSlots this is essentially a prediction without the actual predicted item. We have to sync these regardless of what happens. fixes #5708 --- .../mcpe/handler/InGamePacketHandler.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 37f23e158..12859be46 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -328,6 +328,9 @@ class InGamePacketHandler extends PacketHandler{ if(count($packet->trData->getActions()) > 50){ throw new PacketHandlingException("Too many actions in inventory transaction"); } + if(count($packet->requestChangedSlots) > 10){ + throw new PacketHandlingException("Too many slot sync requests in inventory transaction"); + } $this->inventoryManager->setCurrentItemStackRequestId($packet->requestId); $this->inventoryManager->addRawPredictedSlotChanges($packet->trData->getActions()); @@ -347,6 +350,21 @@ class InGamePacketHandler extends PacketHandler{ } $this->inventoryManager->syncMismatchedPredictedSlotChanges(); + + //requestChangedSlots asks the server to always send out the contents of the specified slots, even if they + //haven't changed. Handling these is necessary to ensure the client inventory stays in sync if the server + //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){ + $inventoryAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slot); + if($inventoryAndSlot !== null){ //trigger the normal slot sync logic + $this->inventoryManager->onSlotChange($inventoryAndSlot[0], $inventoryAndSlot[1]); + } + } + } + $this->inventoryManager->setCurrentItemStackRequestId(null); return $result; }