ExposedStorage

[Sefiraat]

Description

This purpose of this PR is to provide a way for specific SlimefunItems to provide a summary of their contents. This would be used when Transport Slots and/or #getContents simply doesn't cut the mustard! Examples of blocks that could implement this interface would be Barrels to expose their full content as opposed to just the one stack in the output slot, Multi-page inventories to return all held items not just the currently selected one and the likes of DankPacks and likely many more possible applications I've not thought of just yet.

Proposed changes

• New Interface extending ItemAttribute.
• A method getStoredItems that will return Map<ItemStack, Integer> used to tell calling methods what the SlimefunItems currently contains.
• A method withdrawItem that takes a template ItemStack and a requested amount and returns how many items were successfully removed for the calling method to then handle. (I have swapped between ItemStack, ItemStack Array and int a few times and this one felt the most useful in the end - very open to input as to the return type for this method)
• A method depositItemStacks that takes an ItemStack array for the SlimefunItem to try to consume into the storage. Currently returns nothing as the calling method can just look at the array after calling to check what's left - open to having the return the modified array instead?)
• A method depositItemStack that just calls depositItemStacks. Return type may change based on feedback to the above.
• A method contains to return a boolean if the SlimefunItem contains a matching ItemStack within itself.

See Sefiraat/Networks@3e36b9f in which I changed my Network Shells (v e r y r o u g h l y) to use this interface and had the network read the items in a much neater way than I was doing previously. Working perfectly from testing undertaken.

Related Issues (if applicable)

Checklist

• [x] I have fully tested the proposed changes and promise that they will not break everything into chaos.
• [ ] I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
• [x] I followed the existing code standards and didn't mess up the formatting.
• [x] I did my best to add documentation to any public classes or methods I added.
• [x] I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
• [ ] I added sufficient Unit Tests to cover my code.

[WalshyDev]

First thoughts: This is an interesting PR and I understand the reasoning why. I have a few questions however:

1. Do you believe this is better than just improving the current inv/content system?

   • Sub question: What do we do about the current methods for cargo and such

2. Getting the contents returns a Map<Item, Quantity> however deposit takes an array. I think this should probably be consistent but not sure in what way. Not a fan of either honestly (performance + tedious). Any suggestions there?

3. Is there any implementation inside Slimefun which should be done before this is landed?

4. What do you think the performance implications are and should implementations be considered thread-safe or not?

[Sefiraat]

First thoughts: This is an interesting PR and I understand the reasoning why. I have a few questions however:

1. Do you believe this is better than just improving the current inv/content system?

Likely not, but any improvements to the existing system would likely unify how any Slimefun inventory is looked at by anything which would, in turn, mean a big change to cargo. Given cargo is due an overhaul in the future I'd assume this is better smaller in scope for now but potentially used for direction/scoping later on?

• Sub question: What do we do about the current methods for cargo and such

Nothing, yet :)

2. Getting the contents returns a Map<Item, Quantity> however deposit takes an array. I think this should probably be consistent but not sure in what way. Not a fan of either honestly (performance + tedious). Any suggestions there?

Yeah true, I fell into step here because of the way Networks works but deposit can easily be changed to Map<ItemStack, Inventory>, contents is kinda locked in as a Map however, unless there is another data structure that can be used that may be faster?

3. Is there any implementation inside Slimefun which should be done before this is landed?

Not really, most esoterically designed storages live in addons, those I know of:

• Personal Storages (may be tricky)
• FluffyMachines barrels
• Fluffy's foundry (deposit only?)
• Infinity Expansion barrels
• SimpleStorage multi page inventories
• DankTech packs
• Tinkers smeltery (perhaps not)
• Network memory cells

The only one I have found within core is the Trash can which could implement this to return empty content and deposit items could trigger voiding directly or put items into the inventory to be ticked away, but this is barely an improvement over the exposed cargo slots currently so i'd assume no

4. What do you think the performance implications are and should implementations be considered thread-safe or not?

This leaves any comparison/retrieval methods up to the addon implementing the interface which could lead to some issues but, in most cases, these addons must have something similar enough to be able to interact with the block internally. It should be easy enough to tell within a profiler if - for example - a Networks tick is taking longer due to a method being called from another addon to fix it at source. Annnnnd.... yeah this would need to be treadsafe - i'll add something to the documentation but is there an annotation or something else that may make it more prevalent?

[WalshyDev]

Likely not, but any improvements to the existing system would likely unify how any Slimefun inventory is looked at by anything which would, in turn, mean a big change to cargo. Given cargo is due an overhaul in the future I'd assume this is better smaller in scope for now but potentially used for direction/scoping later on?

Yeah makes sense

Yeah true, I fell into step here because of the way Networks works but deposit can easily be changed to Map<ItemStack, Inventory>, contents is kinda locked in as a Map however, unless there is another data structure that can be used that may be faster?

Map is awkward because you need to do like:

final Map<ItemStack, Integer> map = new HashMap<>();
final ItemStack item = // grab item
map.put(item, // grab qantity)

return map

then grabbing and using too likely is gonna be a bit awkward.

Now what is better? No idea frankly. I think we should definitely think more about this though

This leaves any comparison/retrieval methods up to the addon implementing the interface which could lead to some issues but, in most cases, these addons must have something similar enough to be able to interact with the block internally. It should be easy enough to tell within a profiler if - for example - a Networks tick is taking longer due to a method being called from another addon to fix it at source. Annnnnd.... yeah this would need to be treadsafe - i'll add something to the documentation but is there an annotation or something else that may make it more prevalent?

@ThreadSafe. If this needs to be thread-safe then that should definitely be documented and made clear. I was suspecting as such but I was sure given the current code someone would do it in a non-thread-safe way.

I think when the storage rewrite happens everything will be made a lot nicer in this API too.

[TheBusyBiscuit]

Closing this as stale.