Following on from fixed issue #2593, attempting some crafting orders results in a GUI where all items are available, enough crafting storage is available, but the Crafting CPU and Start buttons are not enabled.

Description

Attempting to craft Solar Panel IV from Solar Flux Reborn works normally. It can be crafted in multiples just fine up to 8x panels (craft results in 2x panels), but 9x panels and beyond (i.e. 5+ crafting operations) fail as described above.

Crafting details:
Utilises 11 patterns across 6 interfaces (5 connected to molecular assembler, 1 connected to XU2 Furnace)

Checked that it wasn't incorrectly displaying crafting size by adding 3x extra 64k crafting CPUs and 2x coprocessors, to no effect.

Checked that it wasn't an off-by-one error on needed items by starting 2 independent 8x crafting orders.

Screenshots

Crafting Recipe pt 1

Crafting Recipe pt 2

Working recipe & crafting CPU stats

Environment

Testing done on SSP: Logs at https://gist.github.com/predakanga/bca965be18561f7d61d866fa2fe19c7e

• Minecraft Version: 1.10.2
• AE2 Version: commit 89609a8
• Forge Version: 12.18.2.2118

Storage Drawers?

Ah yes, forgot to mention; Storage Drawers 3.4.2.

I can try to replicate without Storage Drawers in the network if you like

Replacing the storage drawer with a Creative Storage Cell configured with the raw materials lets it craft successfully.

Trying with a non-creative now

Successful with the items held in a non-creative storage cell too.

Autocrafting is a two pass system, it first tries to satisfy everything but fails due to not having enough items. Which causes a second pass to calculate the missing items, but here storage drawers now report having enough item stored (as they basically return an infinite amount when simulating).

@jaquadro I think we will not be able to avoid adding a custom handler for storage drawers.
The compacting ones will probably always be an issue. But we should be able to solve it for the normal ones.

As extractItem() will never return more than 1 full stack, regardless of what was requested and even in simulate mode, our handler fails as it will loop over the slot until it is has satisfied the request. Which will always happen as long as the stored amount is lower then the requested one as there are no transactions and the next request will extract the same amount again.

But I still want to keep it mostly generic approach. While we have an IStorageMonitorableAccessor, which could be sufficient, but is way more complex to implement for most other mods. Hence I would like to introduce a slightly better IItemHandler for our use cases. So it certainly should be part of AE2 so other mods can integrate it more easily.

I currently would propose this:

/**
 * An interface to interact with an inventory.
 * 
 * In contrast to {@link IItemHandler} this is not specifiying a slot
 * and it is up to the inventory to route items in the most efficient way.
 * 
 * Any {@link ItemStack} can violate {@link ItemStack#getMaxStackSize()}
 *
 */
public interface IInventoryHandler
{
    /**
     * Represents the unmodifiable content of an inventory.
     * 
     * Any {@link ItemStack} can violate {@link ItemStack#getMaxStackSize()}
     * 
     * @return An unmodifiable collection of the inventory contents.
     */
    @Nonnull
    Collection<ItemStack> inventory();

    /**
     * @param stack The stack to insert into the inventory, must not be null.
     * @param simulate True, if it should not modify the inventory.
     * @return The remaining {@link ItemStack} or null.
     */
    @Nullable
    ItemStack insertItem( @Nonnull ItemStack stack, boolean simulate );

    /**
     * @param stack The stack to extract from the inventory, must not be null.
     * @param simulate True, if it should not modify the inventory.
     * @return The {@link ItemStack} with the available stacksize or null.
     */
    @Nullable
    ItemStack extractItem( @Nonnull ItemStack stack, boolean simulate );

}

Technically we could replace the simulate with a Transaction class containing simulate and a transaction id. But the amount of effort to keep a transaction valid over multipe ticks is probably not worth the performance/memory requirements. Violating getMaxStackSize() is also making it obsolete for requesting a large stack without looping.

It would also prevent us from having to iterate over the full inventory to find the item we need.

The IInventoryHandler API looks good to me.

Speaking of max stack size, one of the weirdest parts of IItemHandler is that insertion is allowed to handle oversized stack sizes, but extract is not allowed to return them (according to my interpretation of its contract, anyway). That limitation creates more trouble for simulation than I originally imagined.

We briefly discussed transactions earlier, but I agree that they would be a lot of trouble for AE2's use, given that crafting simulation happens in the background on parallel threads. Transactions would be okay if they were limited to server-thread single-tick use only.

Yeah, the IItemHandler really looks like an IChestGUIItemHandler and not some common interface. Very few mods actually work directly with slots. Usually it is just extract or insert stuff into a side exposing a subset of all internal slots. But having the slots requires all sorts of workarounds like virtual slots to split it internally or whatever.

Technically there is also the IMEInventory, which is very similar but uses IAEItemStack, which is certainly different to normal ones (shared NBT and so on). So that is certainly nothing I want to expose or require for very simple integrations.

I was thinking about replacing the Collection with an Iterable or Iterator. We never really need more than being able to iterate over the content. Hence it should be way easier to expose an iterator. For example based on your internal one or even just an iterator wrapping getStackInSlot() without having to assemble a fresh collection or maintain an additional one internally. Even if someone maintains a flat collection, exposing an iterator is always easy and fast. Converting an internal iterator into a collection is way more work.

Further we have the idea of adding another cap between IInventoryHandler and IStorageMonitorableAccessor, which mostly extends the inventory() part with an observer pattern. So instead of us having to keep a valid cache by scanning the full inventory, the inventory can just post that it has stored 2k cobble (instead of voiding it). For simple inventories like chests or even normal barrels/DSU there is certainly not difference between us scanning 1 slot or the inventory scanning 1 slot. Or even a couple like a chest. Worst case a chest would even send 27x64 cobble stored instead of us once 1728 cobble.

The idea for two (or even more?) is mostly to make it as easy as possible. So nobody has to understand the observer pattern including all the pitfalls. But still allow a faster approach for certain inventories. For example that storage drawers has not to expose millions of slots just to conform with the contract defined by IItemHandler by having no slot exceed the max stacksize.

@yueh please correct me, but wasn't there issues with inventory display during the "optimization of inventory handling" in the past where you already tried to get rid of the collections and rolled back because items were randomly vanishing from the inventory view when taking or inserting items ?

but yeah okay your further explanation shows clearly the pitfall with those collections and the gazillion virtual slots

That was a different issue. Mostly because Minecraft does not know the concept of trashcan or enderchests. So even if an inventory says they stored X, you cannot be sure about in and have to verify the change manually as well as any other inventory because it might have teleported it around. In terms of storage drawers even the case that inserting 81 iron nuggets into a compacting drawer suddenly reports 81 nuggets + 9 ingots + 1 block instead of just 81 nuggets.

The problem was, that it was done a network level in 1.7.10. 1.10 already makes it more local and works with deltas instead of rebuilding everything. Currently the main performance problem I see is that creating ItemStack is really really slow in 1.10 (or maybe since 1.8). As storage drawer (or any other inventory which can store more than 1 stacksize per slot) has to pretty much do it constantly on every extract.

Further we still have to convert a normal ItemStack into the ones we use internally. For example to reduce NBT usage. As all equal items inside the network share the same NBT tag. So compared to other solutions, inserting 100 items with an equal NBT tag with 2MB will only consume 2MB memory (plus some usual overhead) instead of 200 MB. At least until they leave the network. Then every item has their own one again.
Hence there is no difference between using an Iterator or Collection on our side. Just maintaining a useless collection puts more work on the other mod.

+1 for the iterator. There's not really a way to store my internal data in a ready-made collection so this would avoid the need to create a temporary one. As a bonus it's naturally immutable.

The observer capability is something I've been wanting to support since 1.7.10, but I've got more work I need to do before I can support it.

immutable yes, failsafe no!
if it is a fast iterator it will crash with a concurrent access exception when the source list is modified somewhere else while iterating, you have to program carefull with this, although i have no idea if there is a slight chance of somebody modifying it in some sort of multithread manner!
if it is an concurrent safe iterator it is most likely backed by a collection thus destroying every benefit you hope to get

I updated the ItemHandlerAdapter to limit the extracted amount per slot to the stacksize of the specific slot. This seems to fix most issues.

But still not ideal as it potentially has to scan over the full inventory, should it not be able to satisfy the request within the first found slot (as well as having to scan to find the first matching slot).

Thus I still want to add an AE2 specific handler for an observer pattern. Which should be pretty similar to the other proposal, but might drop the Iterator/Iterable in favour of a contract to notify a newly registered listener with a delta containing the full inventory. Either immediately or during a later tick, depending on what might fit better.

Can confirm that c74166b fixes my instance of this issue.

Thanks!