Inventory interface

[viveleroi]

Adding core inventory interface with essential add/get/clear methods. Includes
empty ItemStack interface - awaiting Material interface in pending PRs.

---

Edit by handler:

YouTrack

[Kobata]

I'm not sure it makes much sense to use slot numbers in the API -- they vary a bit too much in what they represent.

Of course, getting a better idea of what is in each slot would require additions on the Forge side, so that's not very optimal either.

[progwml6]

We also need to keep in mind that vanilla supposidely has a new inventory system in 1.8

[viveleroi]

@progwml6 Can you dig up any information on that? If they've made any changes, now's the time to accommodate them.

[octylFractal]

Hard to do without MCP.

[Mumfrey]

Next question, should inventories be iteratable, comparable or clonable?

[octylFractal]

Iterable for sure, that would be really helpful.

[viveleroi]

I don't think I've ever needed to clone or compare a full inventory - items, yes, but never an inventory. I suppose both could be useful in certain situations though. Someone's welcome to sell me on it.

[Mumfrey]

Well iterable is the one with arguably the weakest case, since a sensible contract is to terate over Inventory.getContents() so that would boil down purely to syntactic sugar. I'm just opening the util classes up to debate basically.

[Kobata]

@botskonet 1.8 Container appears the same largely as 1.7's. IInventory looks somewhat different however. That needs looked at more once MCP is out.

Edit: On second look it's largely similar, it just implements a second interface and a few extra methods -- I can match most of it fairly well with 1.7

[Zidane]

@botskonet

There won't be a Material interface lol. Just Block or Item.

[viveleroi]

Updating with the recent Item interface commit, and adjusting to match recent interface syntax tweaks.

[Zidane]

@botskonet + everyone else

Could we even have Inventory as a list? Would be nice to have some java collection with all those handy utilities that gives us.

[DarkArc]

Perhaps this should be a collection of items, or

<T extends Collection<ItemStack>>

[viveleroi]

@Zidane The contents are already a List, can you clarify what you mean for me?

[DarkArc]

Perhaps this should also be:

List<ItemStack> addItem(ItemStack... item);

[Mumfrey]

If adding then the signature should be changed to throw IllegalArgumentException since it's possible to call varargs with an empty array.

[Mumfrey]

Sorry I missed that above. Changing to Collection is a daft step backwards, and whoever suggested Set... no.

List represents an ordered collection of elements, since the order may be important (based on the type of container represented by this Inventory) and the returned collection may be sparse, List was the best solution from the beginning. I agree that (for example) the addItem method should return Collection, this is a good pattern because the items returned from the function may have changed order relative to their supplied order, etc. so that's a good choice.

However getContents should return a List because it can then represent a "view" of the Inventory's contents (including empy cells), thus making positioned accessors like get (which are not available as part of Collection since Collection supports both ordered and un-ordered collections) work as expected.

If you're going to have a getContents which returns a Collection, then it should return a compact view of the contents with a contract which doesn't guarantee return order, but the usefulness of such a function is reduced, save for returning an iterable collection which need not contain null checks, which was the proposed usefulness of implementing Iterable mentioned above.

On review, it may also be worth adding methods to:

• "consume" an item from the inventory, essentially return and remove an item stack or partial item stack if the inventory contains it, essentially returning an itemstack representing the amount of the original itemstack it is possible to consume from this inventory
• check if the inventory contains an item stack or partial item stack: .contains()
• remove a particular item stack from the inventory regardless of its index
• remove an item stack at a particular index (suggesting index rather than slot because it slot indices are very inventory-dependent)

Just thoughts.

[DarkArc]

My suggestion was aimed at making the addItems method more accepting, but as @Mumfrey stated the return types should be List.

[viveleroi]

Updated with the recent discussion, and @Mumfrey I've added a few new methods based on your suggestions.

[Mumfrey]

Looks good, sorry to nitpick but you should probably declare public on either nothing or everything (current consensus is nothing) since at the moment the access modifiers vary from method to method.

[octylFractal]

I thought convention was public where possible? Explicit not implicit? Never mind, Mumfrey is right, although that can be a bit confusing sometimes.

[spaceemotion]

Maybe you could replace the dashes with an unordered list... Also the } is wrong ;)

[viveleroi]

You're right, I made a habit of always using public but the standard of the repo seems to be leave them off the interface methods.

[keir-nellyer]

This could also do with a get(int slot) and set(int slot, ItemStack itemStack)

[viveleroi]

@iKeirNez I had those originally, but it seems the consensus was to move away from numbered slots in the interface methods.

[Mumfrey]

@kenzierocks don't worry. My own personal preference is actually to use public abstract on all interface methods because I like code to say what it is :) I'm unusual that way though.

@iKeirNez I'm not sure that that makes sense for base inventory. I've been thinking about how semantics for sub inventory interfaces could be implemented though, thinking of essentially a CategorisedInventory (and a corresponding InventoryCategory interface) for example where slot semantics could be more explicity defined by the category, and then

CategorisedInventory ci = new WhateverImplementingInventory();
InventoryCategory cat = InventoryCategories.get("minecraft:hotbar");
ci.put(item, cat, 4); // put item in the 4th slot of the hotbar category.

I'm trying to decide if that's a really dumb idea or not. It's well understood that slots in an inventory don't necessarily constitute a contiguous set, and for example may represent input, output, fuel slots in a furnace for example. It seems better to somehow capture that metadata so that it's clear by the invokation semantics on the interface exactly what is meant by "put item in slot with index 4".

Another option would be to allow inventories to have a parent/child relationship, or have "sub inventories" or even use an InventoryLocation object as a handle, for example:

InventoryPosition pos = new InventoryPosition("minecraft:fuel", 0);
inventory.put(pos, item);

There are lots of possibilities. Raw indices - while prevalent elsewhere - kind of suck since the mapping to internal slots can vary wildly between implementations, and make a supposedly abstract interface very sensitive to underlying changes in implementation, which is bad, and also make calling the same function on different types of interface vary wildly in actual effect.

Forcing the intended target to be passed through to the put handling in some way allows the inventory to either discard, raise an error condition, or provide other special handling in a "put" or "get" is attempted when that category is not supported or has other contractual requirements.

[Kobata]

@Mumfrey Yeah, the best bet with Inventory stuff, is probably to set up a system for categorizing slots -- what you've laid out there is pretty similar to how I do that in Inventory Tweaks (1, 2)

[Mumfrey]

@Kobata yeah, that was more just thinking out loud. I'm wondering whether we should just pull this as it stands as a starter for 10, and then build abstractions from there? It's still my belief that this interface should be abstract though, since I can see a use case down the line for Inventories which take other things (eg. virtual inventories for offline stuff, and such)

[viveleroi]

I think it's smart for us to avoid exposing the idea of "slots" to devs, I've personally hated being on the other side because I never know what slot number refers to which slot. Chests and similar bulk-grid-slot invs may be the only place an index slot number makes sense.

[spaceemotion]

That position/category idea sounds really great! I would change the different positions depending on their purpose though.

Maybe have a general InventoryPosition interface and then just a method to get the internal slot ID.
One inventoryPosition (maybe ChestInventoryPosition) could then have a Chest-Slot ID as a parameter, while a FurnaceInventoryPosition could use an enum instead (e.g. FurnaceInventorySlot). That way we limit the developer and can avoid confusions whether or not slot 1 in the furnace is X or Y.

[viveleroi]

So can we merge this PR, and build off of it, or should I open a new PR with the ItemStack interface, which is blocking some additional submissions?

[bensku]

Can't do any inventory related event stuff before this gets merged/added in different way by core devs... And chest, furnace etc... API will also need inventory API.

If this isn't getting merged soon, @botskonet you probably should make separate PR for ItemStack, as its also needed in many API features which currently cannot written.

[ryantheleach]

For the usecase of a shop plugin for example, the developer is going to want a way to add/remove ItemStacks to an inventory, and fail if they wern't added.

At the moment the add and remove methods, removing or adding "Up to" an amount seems indeterministic and would require the shop plugin to analyse the contents before and after in order to be sure of what events happened.

Instead if there was some way to not allow partial removing, it would make things a lot easier.

Additionally a source of duplication for shop plugins in the past has been due to errors such as these.

Additionally are we exposing ItemStack UUIDs? should it be possible to remove / fetch / contains an itemstack by UUID?

[viveleroi]

My idea is that a developer could "ask" an inventory for X amt of Y item, and the inventory will return as many of Y item as it has. The developer can then check the amount returned. We could make these checks more explicit, the dev could use some sort of int countItems(Item item)-style method before removing the items. The goal is to allow removing multiple stacks of a single Item

[kitskub]

@botskonet I've looked over this and overall it looks pretty good. I'm going to mark this as reviewed. However, as it is a large change, I'm going to wait until we have a clear PR system before pulling. Thanks!

[DarkArc]

If you're comfortable rebasing this, that would be awesome since we have an ItemStack class now. :)
https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/inventory/ItemStack.java

[viveleroi]

Yes, I wrote both, I shall rebase.

[viveleroi]

@DarkArc Rebased.

[turt2live]

Capitalization (x3)

[viveleroi]

FYI I'll revise with formatting corrections today. Thanks

[IDragonfire]

Youtrack: http://issues.spongepowered.org/youtrack/issue/SPONGE-33

[turt2live]

@viveleroi If you are still interested in this, the following issues will need to be resolved before review can begin:

• Comparison between other inventory PRs

Because other inventory systems exist (#182, #9 - yours, and #197), please provide justification for how you believe yours is better (or worse) than the others. This will help determine which of the three (or combination thereof) will be pulled, if any.

Thanks!

[turt2live]

Closing this as #197 is currently more active and up to date. Thank you for your contribution!