Proposal: Inventory Rework

[RoyalMCPE]

Description

This proposal aims to help streamline the implementation of blocks and entities with inventories. I'm mainly looking for comments on the API and to start a discussion on this topic.

The basic idea is to let the world manage the inventory's state instead of the block itself. We assign properties to the inventory so the world knows how to handle "player actions"

In the coming weeks, a draft pr will be opened with a rough implementation.

What are the problems with the current implementation?

 - The current implementation suffers from a lot of duplicate code.

Changes

This code may have errors. Its purpose is to give a general idea of implementation on blocks and entities. This is not final so expect many changes as feedback comes in and the implementation is underway.

// Simple reimplementation of Chest
type Chest struct {
    chest,
    transparent
    bass
    sourceWaterDisplacer

    Facing cube.Direction

    paired bool
    pairPos cube.Pos
}

func (c Chest) open(tx *world.Tx, pos cube.Pos) {
    for _,v := range tx.Viewers(pos.Vec3()) {
        if c.paired {
            v.ViewBlockAction(c.pairPos(pos), OpenAction{})
 }
        v.ViewBlockAction(pos, OpenAction{})
 }
    tx.PlaySound(pos.Vec3Centre(), sound.ChestOpen{})
}

func (c Chest) close(tx *world.Tx, pos cube.Pos) {
    for _,v := range tx.Viewers(pos.Vec3()) {
        if c.paired {
            v.ViewBlockAction(c.pairPos(pos), CloseAction{})
 }
        v.ViewBlockAction(pos, CloseAction{})
 }
    tx.PlaySound(pos.Vec3Centre(), sound.ChestClose{})
}

func chestObstructed(pos cube.Pos, u item.User, tx *world.Tx) bool {
    if opener, ok := u.(ContainerOpener); ok {
        if c.paired {
            if d, ok := tx.Block(c.pairPos(pos).Side(cube.FaceUp)).(LightDiffuser); !ok || d.LightDiffusionLevel() > 2 {
                return false
 }
 }
        if d, ok := tx.Block(pos.Side(cube.FaceUp)).(LightDiffuser); ok && d.LightDiffusionLevel() <= 2 {
            return  true
 }
 }
    return false
}

func (c Chest) ContainerInfo() ContainerInfo {
    // arg1: size
    // arg2: can add func
    // arg3: viewable
    // arg4: block container open while sneaking
    return newContainerInfo(27, defaultTransfer, true, true).withOpenHandler(func(pos cube.Pos, tx *world.Tx, u item.User) bool {
        if(!chestObstructed(pos, tx, u)) {
            c.open(tx, pos)
            return true
 }
        return false
 }).withCloseHandler(func(pos cube.Pos, tx *world.Tx, u item.User) {
        // View block actions and play sounds
        c.close(tx, pos)
 })
}

// Entity with inventory
type HorseBehaviour struct {}

func horseTransfer(item.Stack, int) bool {
    // Horse inventory behaviour
    return false
}

func (*HorseBehaviour) ContainerInfo() ContainerInfo {
    return newContainerInfo(2, horseTransfer, false, false)
}

// Setting a block with a inventory
w := srv.World()
<-w.Exec(func(tx *world.Tx) {
    tx.SetBlock(cube.Pos{}, block.Chest{}, SetOpts{
        ContainerOptions: ContainerOptions {
            CustomName: "Name"
 }
 })
})

// Accessing a block inventory
w := srv.World()
<-w.Exec(func(tx *world.Tx) {
    inv := tx.BlockInventory(cube.Pos{})
    inv.AddItem(item.NewStack(item.Apple{}, 1))
})

// Accessing entity inventory
for p := range srv.Accept() {
    inv := p.Tx().EntityInventory(p) // This is just an example. I would still expect (*Player).Inventory() to exist
    inv.AddItem(item.NewStack(item.Apple{}, 1))
}

Drawbacks

  - This implementation means that inventories are no longer valid outside of transactions like entities. That being said you'd have to do this with the current just that user can use it anywhere instead of just a transaction.
  - Interacting with an inventory requires an extra nil check on the inventory before the user uses it.
  - With the API mostly being based on BreakInfo it's possible as Mojang adds new types (especially with entities) this implementation may become messy over time. This is just speculation how ever.

Notes

  - It's possible it's not possible. The way inventories are currently implemented this may require quite a bit of refactors to fix import cycle problems that currently exist. Ex; item.Stack. It may just not be worth the effort to do it this way.
  - Technically this makes inventories more scalable since we should be able to remove the underlying RWMutex. That being said, I don't expect the current implementation to have many scaling problems if any.
  - Because this is expected to handle inventories for entities. It may be best to wait til after the entity rework for final implementation and hopefully develop alongside it.
  - The current implementation requires withOpenHandler & withCloseHandler to handle sounds and block actions. This isn't ideal as most blocks have these features and this is a too turse way to do it. A simplified withOpenOptions(OpenOptions{sound world.Sound, action world.BlockAction}) and withCloseOptions(CloseOptions{sound world.Sound, action world.BlockAction}) may be a better solution while keeping the handler functions for blocks that need them. Ex;chest
  One side goal is to clean up some of the container mess within the session package. Depending on how messy the git diff becomes, there may be a follow-up PR after this.
  - A follow-up to the previous but hopefully with these two prs (rework and session cleanup) with the implementation the goal is to make it generic enough that "virtual inventories"

[Sandertv]

I think you've definitely touched on a lot of the pain points with containers right now and I agree that storing inventories on world level feels like one of the most feasible ideas.

I tried to include changes like these in the world transactions changes, but it has proven to be quite challenging.

All in all, some good concepts here!

[RoyalMCPE]

Yeah trust me, I'm aware of the challenges. This is the third time I've looked into this. In the last year.

I think I have a solution mapped out though. The only thing I'm unsure about is dealing with entities that have multiple inventories like the player (inventory, offhand, enderchest, ui and armor)