Breaking cyclic reference between Inventory and its holder

[dktapps]

Description

Many places in the PM core (such as here) feature hacks to destroy the inventory to destroy cyclic references and prevent memory leaks.

This is problematic for static analysis, makes post-dispose use more problematic (technically not supported use case, but it should still work), and is caused by an entirely unnecessary requirement.

The only practical purpose for inventories to contain a reference to its holder is to allow users of events such as InventoryTransactionEvent to identify the owner of an inventory. Outside of events, it has no practical purpose, since you typically have to go through the holder of an inventory to fetch the inventory in the first place - e.g. getting a Player's inventory requires use of Player->getInventory(), which requires a Player reference to begin with, so why would you use getHolder()?

This issue proposes that:

• Inventories will no longer contain a reference to their holder. Instead, actors with inventories will hold SimpleInventory directly.
• When a transaction occurs, these inventories will be wrapped in a new InventoryWrapper(inventory, holder) to allow plugins using events to identify them.

Going further, we could also move window type information to InventoryWrapper, enabling completely dynamic window types to be used for inventories from any source.

[dktapps]

Furthermore, this permanent holder requirement presents unnecessary and annoying obstacles for copying sections of terrain, since objects like tiles are difficult to copy to different locations due to their inventories containing their block positions.

[dktapps]

For posterity: One of the main obstacles to introducing an InventoryWrapper type like this was the problem of how to track inventory viewers. Unless the wrapper was cached, every inventory's getViewers() would return only one player - the player who the wrapper belongs to.

It's not yet clear if this would be a problem for any real use case, or how much of a problem it would be. It would be nice to be able to get rid of getViewers() (less Player references to leak), but I suspect it won't be that easy.

[dktapps]

Wrapping inventories in this manner would require a redesign of the handling of hotbar and held item