Spongeapi: Current inventory API (and its implementation)

[NeumimTo]

I would like to bring up a discussion related to current state of queryable inventory api.

First of all i understand that inventories, are poorly desinged by mojang and writing an abstraction layer on top of that for sponge and/or forge is not easy task to do, but i belive that current api/impl brings more problems than actual features.

I'm not sure how other plugin developers feel about this, but i can already tell my more complex inventory menus will broke almost every time sponge announces another stable build.

I understand that sometimes changes needs to happen in order to move the project into the next development stage, i understand that in those cases ill have to rewrite parts of my code but i would expect this to only happen during major version transitions (6 -> 7 -> 7.1 -> 8.0)

The Inventory api feels like it was desinged by someone as a pure experiment which was never intented to be implemented into sponge's ecosystem. I fear for my sanity every time i have to update my code related to inventories.

- The api is not intuitive
Current api is simply difficult to read, and even more difficult to write and understand.
Lets have real usecase:

        Inventory build = Inventory.builder()
                .of(InventoryArchetypes.DOUBLE_CHEST)
                .build(pluginInstance);
        Slot query = build.query(QueryOperationTypes.INVENTORY_PROPERTY.of(SlotIndex.of(1)));
        query.offer(myItemStack);
        player.openInventory(build);

Now look at this code and think about what will this code do.

1) It opens a doublechest inventory, but the entire inventory will be empty
2) It opens a doublechest inventory, and the first slot (slot having ordinal number 1) in that inventory will have myItemStack, rest of slots in that inventory will be empty
3) Something else

Click to see the correct answer

3 is correct. The code throws ClassCastException:

java.lang.ClassCastException: org.spongepowered.common.item.inventory.query.result.MinecraftResultAdapterProvider$MinecraftQueryResultAdapter cannot be cast to org.spongepowered.api.item.inventory.Slot

The entire object structure is rather very strange. For example Slot inherits from Inventory.
I cannot find a good reason why is it made that way?

Due to the object structure of Slot this is valid code in sponge.

                Slot s = ...;
        Iterator<Inventory> iterator = s.iterator();
        while (iterator.hasNext()) {
            Inventory next = iterator.next();
        }

What does this code do? I cannot tell or even make an assumption.

I can see you attempted to desingn this part of sponge api with some kind of duck-like typing? Dont do that. This is java, strongly typed language, please leave this crazyness for another languages.
All what this desingn causes is to confuse developers and introduces unnecessary overhead.

Does this strange desingn exist just supposed to ANY modded inventory?
I belive that if a mod introduces some weird inventory structures it shall not be sponge's goal to natively support these mods. Leave that to mod author, or some other developer to implement these things to sponge via third party libraries.

Confusing every plugin developer just to support some edge case scenarios is simply not worth from my point of view.

- Documentation
Another point is that javadocs are written in a strange way. They are simply not clear.

Slot.java:

    /**
     * Transforms this Slot into given Type.
     *
     * <dl>
     *   <dt>Example</dt>
     *   <dd>In a InventoryEvent with a Container to get the actual inventory from Slot,
     *     you may call this with {@link Type#INVENTORY}.</dd>
     * </dl>
     *
     * @param type the type to transform into
     * @return the transformed Slot or itself if already the correct type
     */
    Slot transform(Type type);

    /**
     * Transforms this Slot into the default Type.
     *
     * @return the transformed Slot or itself if already the default type
     */
    Slot transform();

I have literally no idea what any of this means, unfortunetly i needed to call this method to get correct slot id. If faithcaio did not tell me to transform the slot, i would never find this out on my own.

My currect workflow is like: I want to get slot id so i just transform the slot, i have no idea why i am doing it, i have no idea how expensive the call is, i have no idea what is that call even doing, but after i transform it it gives me correct slot id, so i just transform it.
(Pretty much similar how everyone writes datamanipulators)

Another example of unclear documentation could be
https://github.com/SpongePowered/SpongeAPI/blob/d5f1171eb1ede4fe12efe731271eb3ad5a7cf48d/src/main/java/org/spongepowered/api/item/inventory/entity/MainPlayerInventory.java

This inventory inherits from GridInventory, but still has method GridInventory getGrid();
Why? What does it even return

- It breaks too often
As i already wrote above. current implementation breaks too much. Features which i was using few builds back, now with recent stable builds simply crash the server. Due to the nature of the inventory api its hard correctly identify those problems in some more complex scenarios.

There are much more problems related to inventories, I belive that the inventory api needs a major overhaul, this is just to start up a discussion for possible future changes

[NeumimTo]

update: fixed some typos

[XakepSDK]

I think, querying slot indexes is broken.
Just look at these examples:
Code:

        Inventory build = Inventory.builder()
                .of(InventoryArchetypes.CHEST)
                .build(this);
        Inventory query = build.query(QueryOperationTypes.INVENTORY_PROPERTY.of(SlotPos.of(0, 0)), QueryOperationTypes.INVENTORY_PROPERTY.of(SlotPos.of(0, 1)));
        player.sendMessage(Text.of(query.size()));
        for (Inventory inventory : query) {
            inventory.offer(ItemStack.of(ItemTypes.APPLE, 1));
        }
        player.sendMessage(Text.of(query));
        player.openInventory(build);

Result: https://puu.sh/AGT9k/649da70c99.png

Inventory query = build.query(QueryOperationTypes.INVENTORY_PROPERTY.of(SlotPos.of(0, 0)), QueryOperationTypes.INVENTORY_PROPERTY.of(SlotIndex.of(1)));

Result: https://puu.sh/AGTaz/9d2ca8bad8.png

Inventory query = build.query(QueryOperationTypes.INVENTORY_PROPERTY.of(SlotIndex.of(0)), QueryOperationTypes.INVENTORY_PROPERTY.of(SlotIndex.of(1)));

Result: https://puu.sh/AGTbp/8c4a04ff2d.png

Inventory query = build.query(QueryOperationTypes.INVENTORY_PROPERTY.of(SlotIndex.of(0)));

Result: https://puu.sh/AGTcC/01ed6cb87c.png

Inventory query = build.query(QueryOperationTypes.INVENTORY_PROPERTY.of(SlotIndex.of(1)));

Result: https://puu.sh/AGTdw/7cec408a8f.png

So, because it returns not just 1 slot, but set of slots, it returns some internal query adapter, instead of Slot implementation class.

Also, writing this: Inventory query = build.query(QueryOperationTypes.INVENTORY_PROPERTY.of(SlotIndex.of(1)));
to get just one slot - this scares me, so much code for just that simple operation.

Also, QueryOperationTypes contains ITEM_STACK_CUSTOM, ITEM_STACK_EXACT, ITEM_STACK_IGNORE_QUANTITY, ITEM_TYPE - this everything can be replaced with ItemStackComparator, that was introduced in 7.1, i think, these queries may be somewhat rewritten to make less things, that make same job.

As for slot transformations - iirc, sponge's Inventory class is a view for internal minecraft inventories and containers, so, transformation just returns correct slot relative to something, because one inventory slot may have different id's inside container and inventory, so, transforming just returns correct slot relative inventory, not container.
Well, i think, iterating over Slot will just iterate it 1 time.

[ryantheleach]

I have to strongly disagree with 1 part.

I belive that if a mod introduces some weird inventory structures it shall not be sponge's goal to natively support these mods. Leave that to mod author, or some other developer to implement these things to sponge via third party libraries.

IF a mod has supported, identifiable slots, Sponge should absolutely make best effort to support them.

Yes some things are going to be unexpected / impossible to support without building per-mod support, but the large bulk of modded inventories can be easily handled.

[XakepSDK]

@Faithcaio

[Faithcaio]

The api is not intuitive

Yep. Thats why I plan to break it all in API8.

Lets have a real usecase:

I will move methods from OrderedInventory up to Inventory so you will be able to just call getSlot.

Ducktyping

I don't like it either.

Slot structure

Think about it. Slots really are just Inventories of size 1.

Slot#transform

I had to add a method like this so you can deal with a Slot having multiple parents. (Container and the real inventory)
If someone has a good idea, give it to me.
transform with type parameter will be gone. there is only one anyways

MainPlayerInventory

MainPlayerInventory is the 4x9 Grid. It is itself a Grid.
It contains a 3x9 Grid (that is returned by getGrid) and the Hotbar

It breaks to often

Sorry. :/

query for SlotIndex

Queries go into the inventory recursivly and check for every possible match of what you query for.
In a Grid naturally Slot 0 can be the first slot of the Grid but also the first Slot of every Row/Column.

The way to get a single Slot for a SlotIndex is to use the OrderedInventory.
Same for SlotPos - GridInventory

[NeumimTo]

Think about it. Slots really are just Inventories of size 1.

If the slots are inventories of size 1 I still cannot find a reason why they should inherit Iterable. I guess the initial intetion was here to because of duck typing?

Another thing what confuses me is why the Inventory's signatre looks like?

public interface Inventory extends Iterable<Inventory>, Nameable {

I would expect it to be

public interface Inventory extends Iterable<Slot>, Nameable {

[ryantheleach]

If the slots are inventories of size 1 I still cannot find a reason why they should inherit Iterable.

Collections that contain a single element are still iterable.

Collections that are empty are iterable.

Why should Slot be iterable from a users point of view?

For consistencies sake, so the same code that filters an inventory can filter a single slot without adaption.

Another thing what confuses me is why the Inventory's signatre looks like?

public interface Inventory extends Iterable, Nameable {
I would expect it to be

public interface Inventory extends Iterable, Nameable {

Because you are iterating over the inventories, not the slots.

Potentially you could end up iterating through an inventory that doesn't contain slots, but I'm not sure what that would look like.

Additionally you could be iterating over rows or columns, as opposed to the slots that make up that inventory, given the right lens / query (But I don't know if there are any concrete cases of such).

[XakepSDK]

What we currently need:

1. Better api documentation
2. More documentation inside implementation, for experienced users, who want to understand, how it works without thousand hours of reverse engineering
3. Documentation for spongedocs
4. Do something with queries, because they are too big(InventoryQueryTypes.MY_BEST_QUERY.of(MyBestQueryClass.of(MyBestQueryValue)))
   Maybe add reverse mechanism to get query for instances of queryable classes, e.g.
   SlotIndex.of(1).asQuery() Much cleaner, no?

Will be slot transorm more understandable, if it will be named relativize? relativize(Type.INVENTORY), relativize(Type.CONTAINER).
Also, as i think - all these dissatisfactions are because of really BAD documentation, people don't know, why they need these slot transformations and why they can't get directly slot by position or why they need to write inventory queries.

[Faithcaio]

correct ryan.

Best example for that is a queryresult:
If you query for InventoryRow you'll get an Inventory containing all those Rows. The rows then consist of slots.
To get the slots there is Inventory#slots (that is getting changed to actually return Slot in API8`

More documentation

Takes time. That I don't have.

big queries

How about this: https://github.com/SpongePowered/SpongeAPI/blob/2f32e54f3014e2cd309de877727487621c50f337/src/main/java/org/spongepowered/api/item/inventory/InventoryProperty.java#L41 (API8)

Slot transform

I guess I could rename it. If possible I'd like to get rid of it entirely.
But the problem here is the double parent structure of Containers.

relativize(Type.INVENTORY) would not be possible.
The Inventory Slot does not know a Container Slot exists for it. (also inventory slots don't really exists ; in reality its just a list)

BAD documentation

Again it needs time. And sometimes I don't even know what stuff is supposed to be there for.

[NeumimTo]

I think it would be enough to remove deprecated annotation from query(slotindex) query(slotpos) to reduce the boilerplate code for simple yet common queries

[XakepSDK]

My way allows to chain queries.

[Cybermaxke]

@Faithcaio I prefer Inventory query(InventoryProperty property) over Inventory queryIn(Inventory inventory), the query methods should be in the same place IMO (for this case).

[pie-flavor]

I don't really care how the API looks, since verboseness clarifies intent. I will however say that the current behavior of transform() is very unwieldy and non-obvious. The default should be the most common use case, and I can't think of a single use case for the union inventory, whereas a very common interaction is to just get the top inventory. I much preferred Bukkit's system, where InventoryView was a separate class unrelated to Inventory which represented the joined inventories.

[Faithcaio]

Here have some use-cases for having the Container instead of the "top" inventory in the event:
You click in the bottom Inventory. You shift-click into the bottom Inventory

When a Player interacts with an open Container all the Slots he can click are inside that one View.

Bukkit only had to write wrappers for 100% known inventories, we also try to support all modded inventories.
What does "top" inventory really mean? The only thing I can really check for if it is the player inventory or not. But then again that wont work if you open your own inventory.
Also sometimes there are more than 2 inventories.

I agree that Slot#transform is not the best but I don't know how to provide the slot in the underlying inventory otherwise.
Also only the Slots from Containers have a reference back to the viewed Inventory.
It is not possible to get the Container Slot from an Inventory Slot.

[Zidane]

We're not changing the API to be a Vanilla Inventory API.

[Zidane]

Also, if you dislike breakages @NeumimTo then you really aren't going to like API 8. I never, in the history of this project, ever gave implied stability to plugins. Sometimes shit breaks, @Faithcaio is human just like you and the Vanilla inventory system is, as of 1.13, the worst part of the codebase by hundreds of miles. Then once you throw in mods its a miracle that @Mumfrey / @Faithcaio ever managed to get something put together that functions decently.

[Faithcaio]

Check out the Inventory API in bleeding branch.
If you got more stuff you feel needs changing make a new issue.