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 #5033
Opens the gates for #6147 (which in turn means that async tasks will eventually be able to work with tiles)

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 & held-item usages in PlayerInventory
  • Human->getHotbar()
  • block\utils\AnimatedContainer & block\utils\AnimatedContainerTrait (for chests, shulkerboxes, 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

I am aware that the current changeset makes it rather more annoying to open inventory menus to the player. This PR is still under active development and will change in the future.

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
• Add getInventory() APIs to blocks
• 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

[dktapps]

Double chests have also proven to be a pain in the ass and will likely be an obstacle for #6147 as well

[dktapps]

Only remaining major issue is around the lengthened usage for opening inventory windows to players.

I didn't quite decide how to deal with it. There are a few things to consider:

• Not all blocks which open windows have inventories of their own (e.g. anvils, ender chests, crafting tables, etc.)
• Blocks which do have inventories of their own may not have those inventories present (e.g. due to missing tiles) and so a mode of allowed failure would need to be introduced (e.g. Chest->openTo() would need to be allowed to fail). This sucks, since it will not fail in 99.9% of cases (only those special cases where something hasn't been done correct by a plugin, or old worlds).
• Blocks which have inventories also usually have a feature to test for container lock. The player must use an item with a matching custom name to open such containers. Should openTo() check this kind of logic? If not, how do we avoid duplicated logic fetching tile in onInteract() vs openTo()? (Side note: I suppose plugins already need this boilerplate duplicated logic on stable branches, since we don't offer a simple way to get the inventory anyway.)