Separate inventory holder info from container & player inventories

[dktapps]

This PR breaks the cyclic dependency between Inventory and its holder, which unblocks a lot of new developments.

Related issues & PRs

• Fixes Breaking cyclic reference between Inventory and its holder #5033
• Removes a blocker for Positionless tiles #6147 (which in turn means that async tasks will eventually be able to work with tiles)
• Removes a blocker for OfflinePlayer inventory access #2684

Changes

API changes

• Player->getCurrentWindow() now returns ?InventoryWindow instead of ?Inventory
• Player->setCurrentWindow() now accepts ?InventoryWindow instead of ?Inventory
• InventoryWindow introduced, which is created for each player viewing the inventory, provides decorative information like holder info for InventoryTransactionEvent, and is destroyed when the window is closed, eliminating cyclic references
• Added:
  • player\InventoryWindow
  • player\PlayerInventoryWindow - wraps all permanent inventories of Player with type info for transactions
  • inventory\Hotbar - replaces all hotbar usages in PlayerInventory
  • Human->getHotbar()
  • Human->getMainHandItem(), Human->setMainHandItem(), Human->getOffHandItem(), Human->setOffHandItem()
  • block\utils\AnimatedContainerLike & block\utils\AnimatedContainerLikeTrait (for chests, shulkerboxes, etc)
  • block\utils\Container & block\utils\ContainerTrait for blocks containing items (chests, etc)
  • block\utils\MenuAccessor implemented by all blocks that can open inventory menus
  • block\utils\MenuAccessorTrait used by blocks with menus but without inventories (anvils, crafting tables etc)
• Removed:
  • inventory\DelegateInventory (only used for ender chests)
  • inventory\PlayerInventory,
  • inventory\PlayerOffHandInventory,
  • inventory\PlayerCraftingInventory,
  • inventory\PlayerCursorInventory - these have all been internally replaced by SimpleInventory & they will appear as PlayerInventoryWindow in transactions (check getType() against the PlayerInventoryWindow::TYPE_* constants to identify them)
  • block\inventory\AnimatedBlockInventoryTrait, (blocks now handle this logic directly using AnimatedContainer and AnimatedContainerTrait)
  • block\inventory\BlockInventoryTrait,
  • block\inventory\BlockInventory
• Most BlockInventory classes have been transitioned to InventoryWindow wrappers
• Tiles now all use SimpleInventory internally (no cyclic references) except for Chest (which uses CombinedInventory, without holder info)
• InventoryOpenEvent and InventoryCloseEvent now provide InventoryWindow instead of Inventory (to provide type information)
• InventoryTransaction and SlotChangeAction now provide InventoryWindow instead of Inventory
• Renamed TransactionBuilderInventory to SlotChangeActionBuilder
• TransactionBuilderInventory->getBuilder() now accepts InventoryWindow instead of Inventory
• DoubleChestInventory superseded by CombinedInventory - this new class allows combining any number of inventories behind a single object; mainly used for double chests but plugins could use it to do lots of fun things

Impacts to plugins

Plugins can now do the following:

$block = $world->getBlockAt($x, $y, $z);
if($block instanceof MenuAccessor){
    $block->openToUnchecked($player);
}

As compared to the old way:

$tile = $world->getTileAt($x, $y, $z);
if($tile instanceof Container){
    $player->setCurrentWindow($tile->getInventory());
}

Advantages

• No tile access needed
• Works for menu blocks without inventories as well as container blocks
• Less code

Behavioural changes

Inventories no longer keep permanent cyclic references to their holders.

Backwards compatibility

This makes significant BC breaks. However, all changes are able to be adapted to and the same amount of information is present on all APIs and events.

Follow-up

• Implement Positionless tiles #6147
• Support inventory inheritance when copying blocks from one position to another

Tests

Briefly tested in-game.

[ShockedPlot7560]

I have no real problem with the current implementation. It seems fine to me and is a first step towards a better inventory system.

Another follow-up I would suggest concerns the viewers of the inventory. I can see the point of including viewers in this system but I still have a sticking point because we're including a reference to the player in the inventory interface. Wouldn't it be good after this to think of a system where we have classes that only contain storage logic / listeners and that's it ?
This may also be a bad idea, potentially making the code into spaghetti for inventories.

[dktapps]

I have no real problem with the current implementation. It seems fine to me and is a first step towards a better inventory system.

Another follow-up I would suggest concerns the viewers of the inventory. I can see the point of including viewers in this system but I still have a sticking point because we're including a reference to the player in the inventory interface. Wouldn't it be good after this to think of a system where we have classes that only contain storage logic / listeners and that's it ? This may also be a bad idea, potentially making the code into spaghetti for inventories.

Yeah, that's the ideal world. Unfortunately we need the Inventory to still track the viewers so that plugins can see all the viewers of an inventory, and also so that the inventory menus get closed when a block is deleted. I don't see another obvious solution to this problem. We could pull the viewer list out of Inventory and have the container itself manage it, but I'm not sure if that would improve anything.

[dktapps]

Few issues remain with the PR:

• BlockInventoryWindow subclasses have minimum 3 constructor parameters - annoying for plugins, lots of room to break things - I'm thinking something like $chest->openTo(Player $player) would be a good idea to simplify the usage
• I haven't figured out a solution for stuff like Furnace which uses an InventoryListener to send itself a scheduled block update on a position. That's a remaining blocker for Positionless tiles #6147.
  Possibly could use Block in BlockInventoryWindow - this is fine now that the windows aren't referenced by holders anymore, we don't need to worry about cyclic references
• No inventory type info in windows for single chest, shulker, barrel & ender chest - this isn't needed for internal use but would probably be desired by plugins; could just let them check the block type but I don't know if this should be bound to blocks

Possibly could use Block in BlockInventoryWindow - this is fine now that the windows aren't referenced by holders anymore, we don't need to worry about cyclic references

[dktapps]

Alright. I think I'm finally done with this

[Copilot]

Pull Request Overview

This PR eliminates cyclic references between inventories and their holders by introducing an InventoryWindow abstraction layer that separates inventory holder information from the container and player inventories.

• Introduces InventoryWindow classes to wrap inventories with view-specific information for players
• Replaces direct inventory access with window-based access for transactions and events
• Creates unified interfaces for blocks with menus (MenuAccessor) and containers (Container) with associated traits

Reviewed Changes

Copilot reviewed 91 out of 91 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/phpunit/inventory/CombinedInventoryProxyTest.php	Test file for new combined inventory proxy functionality
src/player/TemporaryInventoryWindow.php	Interface for temporary inventory windows
src/player/SurvivalBlockBreakHandler.php	Updates to use new main hand item access method
src/player/PlayerInventoryWindow.php	Window wrapper for player-owned inventories with type identification
src/player/Player.php	Major refactoring to use inventory windows instead of direct inventory references
src/player/InventoryWindow.php	Base class for inventory windows providing viewer-inventory abstraction
Various network/mcpe handler files	Updates to handle inventory windows instead of raw inventories
Various inventory files	Updates to support new window-based architecture

Comments suppressed due to low confidence (1)

src/player/Player.php:1

• [nitpick] The TODO comment about a cyclic reference should be addressed or removed if the cyclic reference is acceptable in this context. Consider using a WeakReference if the cycle is problematic.

<?php

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[dktapps]

In the interest of avoiding branch rot I'm going to get this merged, the final result won't look too different from this anyway. Pls let me know if there are issues found after the fact.