Inventory Rework

[GoldenStack]

Rework of the Inventory system.

Main changes:

• Accurate click handling (except for special slots like result slots)
• Improved event API: allows accessing all click information via the ClickInfo, as well as providing a list of all changes via ClickResult
• Removed InventoryCondition in favor of three events: InventoryPreClickEvent, modifying only click info, like slot IDs; InventoryClickEvent, modifying the list of changes that will occur as a result of the click; and InventoryPostClickEvent, which can view all the click information but happens after handling is finished

Notes:

• All inventory tests pass (75/75)
• The removed inventory tests were replaced with ones that test the same areas

Changes:

• Removed Minestom's custom inventory slots in favor of protocol ones
• Moved a lot of behaviour into AbstractInventory, renamed AbstractInventory to Inventory and Inventory to ContainerInventory, and made ContainerInventory implement Inventory.Typed which includes inventory types and titles
• Refactored inventory events adding #getEventInventory which returns the player inventory when necessary (deprecating #getInventory which can return null)
• Added the tagged union ClickInfo that stores all the information about the slots in a click, and made events use it
• Made all inventories implement Viewable and added methods #handleOpen and #handleClose
• Simplified click handling and implemented vanilla parity other than special slots (like result slots) in ClickHandler and StandardClickHandler
• Moved click preprocessing (like packet parsing & drag handling) into each inventory's ClickPreprocessor instance
• Added ClickResult which stores a map of changes and side effects that could affect an inventory after each click, and allows viewing it through InventoryClickEvent
• Refactored InventoryCondition to allow editing ClickResult instances
• Moved much of the Inventory implementation to InventoryImpl
• Refactored InventoryCondition into InventoryClickEvent, and renamed what was formerly InventoryClickEvent into InventoryPostClickEvent
• More comprehensive tests (due to ClickResult the click API is much more testable)

[mworzala]

Seems interesting I will try to review tomorrow

[GoldenStack]

I remain a bit unconvinced about switching to protocol slots. Being able to iterate over the inner inventory slots and get just the inventory is a pretty useful property in some cases.

i probably don't understand what you're saying but im pretty sure this PR doesn't impede that?

Also, you mention that special slots are not implemented. I think this is fine, but it would be nice to have a little documentation of what the path is to implement those post-this PR. They will have to be implemented at some point, so I would like to ensure it is possible after this PR without another major change.

:+1:

I need to think a bit about removing InventoryCondition. I have used it successfully a few times and want to confirm in my head that I agree its redundant.

You can replace InventoryCondition with a InventoryClickEvent handler (they can do the exact same stuff), and you can use the event filters to make sure it's the right inventory.

[mworzala]

Regarding protocol slots, the following code snippet (currently) will return the items in the players hotbar and 3 rows of inventory:

for (int i = 0; i < PlayerInventory.INNER_INVENTORY_SIZE; i++) {
    var item = player.getInventory().getItemStack(i);
}

After this PR, it will return the crafting grid, armor slots, and 3 rows of inventory.

Generally I believe the protocol slots make no sense and the remapping that Minestom currently does is a significant improvement. Also not unprecedented, spigot also remaps slots (although with a significantly stranger mapping).

[GoldenStack]

that's a fair point. Though I feel like this now puts the spotlight on the fact that accessing specific inventory regions is quite annoying, regardless of how player inventory slot IDs are assigned.

I would suggest features similar to something out of my library Window (that allows you to do stuff like Views120.player().contents() instead of raw slot IDs) but I think they feel a bit too much like a utility, rather than something that would actually be included in base Minestom. What do you think about this?

Otherwise I can just re-add the inventory mappings.

[mworzala]

Ill look some more later.

P.S. tests are failing

[GoldenStack]

Moved to https://github.com/Minestom/Minestom/pull/1992