Storage Drawers API Update

Question

Storage Drawers API Update

jaquadro opened this issue 7 years ago · 30 comments

jaquadro commented 7 years ago

This is a heads up I've made major API upgrades in the 5.2.0 release (alpha release - https://minecraft.curseforge.com/projects/storage-drawers/files/2446485)

I skimmed across your usage of the API and I think I've made it compatible with existing releases. Most of the carnage was in areas not used by any 1.12 projects.

IItemRepository operations and IDrawer.adjustStoredItemCount may allow you to shorten up the code a bit, as they take into account special cases like voiding, and generally behave like the insert/extract operations in Forge IItemHandler.

IDrawerGroup and IItemRepository are exposed through the capability system. IDrawerGroup is still implemented on the Tiles too, mainly for back-compat with existing RS releases. So that will stick around for the remainder of 1.12's lifetime.

raoulvdberge · Answer 1 · 2017-07-06T13:36:45.000Z

Some questions:

I don't think I can use adjustStoredItemCount because it doesn't support simulation (look at StorageItemDrawer#insert)
Why is IVoidable deprecated?
Why isn't IDrawer a capability?

jaquadro · Answer 2 · 2017-07-06T13:41:38.000Z

In the implementation for IItemRepoistory, I do this:

amount = (simulate)
   ? Math.max(amount - drawer.getAcceptingRemainingCapacity(), 0)
   : drawer.adjustStoredItemCount(amount);

jaquadro · Answer 3 · 2017-07-06T16:02:13.000Z

I've got a little more feedback about your first change set.

You should be calling IDrawerGroup.getAccessibleDrawerSlots() to get the order in which you should enumerate drawers. Otherwise priority is ignored and you might end up populating empty slots instead of adding to a partial drawer elsewhere.

The priority rules are actually a bit more subtle that what can even be done via getAccessibleDrawerSlots which is why IItemRepository is the best insertion endpoint.

raoulvdberge · Answer 4 · 2017-07-06T17:31:39.000Z

Thanks, I guess a predicate would work.

If I were to use IitemRepository at all times I'd still need one method: getCapacity.

jaquadro · Answer 5 · 2017-07-06T18:27:55.000Z

If I'm reading this right, getCapacity does not discriminate item type, it's just raw capacity of everything?

This probably causes unexpected behavior if you slap one of the creative upgrades anywhere -- maxCapacity in that cases returns Integer.MAX_SIZE. And adding anything on top of that will give you a negative result because Java.

raoulvdberge · Answer 6 · 2017-07-06T18:44:44.000Z

It would still care about item type. I just need this for the display on the External Storage GUI.

jaquadro · Answer 7 · 2017-07-06T18:50:58.000Z

I think there's something I'm not getting, so I'm copy/pasting StorageItemDrawerGroup.getCapacity():

    @Override
    public int getCapacity() {
        int capacity = 0;

        IDrawerGroup group = groupSupplier.get();

        if (group == null) {
            return 0;
        }

        for (int i = 0; i < group.getDrawerCount(); ++i) {
            IDrawer drawer = group.getDrawer(i);

            if (drawer.isEnabled()) {
                capacity += drawer.getMaxCapacity();
            }
        }

        return capacity;
    }

I don't see anything that checks item type.

raoulvdberge · Answer 8 · 2017-07-06T18:55:07.000Z

Oh, sorry, but I think that method is broken since it indeed doesn't check item type. Does it matter? It's just for display on the GUI.

jaquadro · Answer 9 · 2017-07-06T18:59:49.000Z

No, it's fine, it's that current implementation that I didn't understand. The item-checked version sounds like a reasonable addition to IItemRepository. I'll put an update together this evening.

raoulvdberge · Answer 10 · 2017-07-06T19:01:24.000Z

Cool, thanks, then I can just use IItemRepository if everything goes right. That should also fix some bugs regarding insertion/extraction order.

raoulvdberge · Answer 11 · 2017-07-06T13:56:56.000Z

Ok @jaquadro done!

Can you validate my changes: f686b3c

Some things I've been noticing:

RS can no longer interact with an empty drawer (for insertions, the adjustStoredItemCount wouldn't obviously work there - is that intended or do I need to work around it?)
There is a desync (single player) when getting a void upgrade out of the upgrade slots - I get the upgrade back but the drawer still stays voidable.

jaquadro · Answer 12 · 2017-07-06T14:49:18.000Z

You'll have to check if it's empty and set the item.

You made a comment about IItemRepository earlier, but it looks like you walked it back. Is there a design constraint in RS that prevents you from using it?

https://github.com/jaquadro/StorageDrawers/blob/1.12/src/com/jaquadro/minecraft/storagedrawers/block/tile/TileEntityController.java#L699

If you check the implementation, and look down to line ~726, your insert implementation would look similar to what's happening in the for loop. But IItemRepository.insert exercises pretty wide latitude in where it places items, ignoring exact slots.

Re desync: Sounds like a bug I'll look into tonight.

raoulvdberge · Answer 13 · 2017-07-06T14:52:50.000Z

Yes, I can't use IItemRepository since there is a "compare" flag for RS extractions.

    public static ItemStack extract(@Nullable IDrawer drawer, @Nonnull ItemStack stack, int size, int flags, boolean simulate) {
        if (drawer != null && API.instance().getComparer().isEqual(stack, drawer.getStoredItemPrototype(), flags)) {

jaquadro · Answer 14 · 2017-07-06T15:10:33.000Z

IItemRepository.extractItem is guaranteed to return the same stack type you passed it (returns the same object instance in fact), so could you not do your filter check in the group wrapper first, and then call extract?

You could also use IItemRepository for the insert side only.

raoulvdberge · Answer 15 · 2017-07-06T15:34:11.000Z

No, RS supports oredict as well. The oredict lookups would be too slow.

Closing this now, thanks for the heads up about the API change.

jaquadro · Answer 16 · 2017-07-06T15:42:53.000Z

I don't follow "the oredict lookups would be too slow". ~~You're going to incur SD's oredict match cost on every lookup regardless (though now that only happens if the appropriate upgrade inst installed), in addition to your own.~~ You're not doing the canExtract check, so never mind. Your extract code is fine.

I was just looking through how you use your comparers though, so I can see where there might be a problem. I considered adding an alternate version of the extract call that would let you pass a custom matcher - would that have been sufficient?

jaquadro · Answer 17 · 2017-07-07T05:29:04.000Z

Alright, IItemRepository is extended in version 5.2.1. Most functions have an alternate version that accepts a Predicate entry, which will pass the stored item prototype to you for matching.

As far as SD's implementation is concerned, passing in a predicate will stop it from doing any of its own item matching (so if dirt to diamonds is your thing ...). Not passing a predicate, or passing one that implements IItemRepository.DefaultPredicate will cause the internal checks to be applied, which is a strict ItemStack match, plus SD's internal ore-dict matching if the conversion upgrade is present.

I didn't do much testing around the predicates so hopefully I didn't do something dumb.

https://github.com/jaquadro/StorageDrawers/blob/1.12/src/com/jaquadro/minecraft/storagedrawers/api/capabilities/IItemRepository.java

raoulvdberge · Answer 18 · 2017-07-07T12:58:48.000Z

Thank you, I will look into it tonight.

raoulvdberge · Answer 19 · 2017-07-07T13:38:42.000Z

@jaquadro Well, felt like coding so I looked into it immediataly.

Can you re-validate? 7669b19

I removed all the drawers API classes and left IItemRepository.

Relevant code is in StorageItemItemRepository.java.

jaquadro · Answer 20 · 2017-07-07T14:17:14.000Z

Two things I see:

In extract(), instead of checking the count first, just try to extract the original size. You'll get back the same amount. getStoredItemCount has a similar runtime cost to the extract call itself.

For getCapacity(), I must have misunderstood again the intention. This implementation will have bad runtime complexity (n^2) and potentially give an inflated answer. The original IDrawerGroup implementation here would be better, being mindful of the edge case where a call to getMaxCapacity() would give back Integer.MAX_VALUE.

raoulvdberge · Answer 21 · 2017-07-07T14:21:15.000Z

I don't understand why I could "just extract the original count"? That's what the size param is for. Besides, I gave it 64 for example while I only had 32 of said item, and it gave me 64 back... Or is that an isssue with the comparer predicate?

Is every IItemRepository guaranteed to be an IDrawerGroup? Currently the code only accounts for a IItemRepository.

jaquadro · Answer 22 · 2017-07-07T14:30:37.000Z

Asking the inventory to extract 64 when it only has 5 is fine. You will get back 5.

It's not really a guarantee, but as a practical matter every tile in SD that exports an IDrawerGroup capability also exports IItemRepository. Capacity could also be inferred through the IItemHandler interface, though not as fast or straightforward as doing it through IDrawerGroup.

raoulvdberge · Answer 23 · 2017-07-07T14:31:56.000Z

Well, the entire point of these changes is that I would only need IItemRepository.

And no, that didn't happen on my side. I gave it 64 and I got back 64 (while I only had 32). Again, might be a comparer thing I have to account for?

jaquadro · Answer 24 · 2017-07-07T14:55:26.000Z

Either the comparer is too broad, or I have a bug (that's probably more likely). The intention is very much that you can request as much as you want and only get back what's available.

If I were to put an inventory-global getCapacity() on IItemRepository, what specifically should that represent? What is the capacity of a partially filled inventory when some items have a stack size of 64 and some 16, for example, and how are empty slots treated?

raoulvdberge · Answer 25 · 2017-07-07T15:19:32.000Z

That is up to you to decide, I'm the API consumer. It's whatever you want the "capacity" value on the GUI to represent.

The comparer doesn't check for stack size, so I don't know if you expect that SD side.

jaquadro · Answer 26 · 2017-07-07T15:43:46.000Z

The comparer is not expected to check stack size.

raoulvdberge · Answer 27 · 2017-07-08T07:59:54.000Z

Aha, very nice. That means RS no longer includes any API in the jar, thank you! :)

raoulvdberge · Answer 28 · 2017-07-08T08:49:13.000Z

@jaquadro I'm trying to load an existing world with drawers and it crashes like this:

[10:48:12] [Server thread/ERROR]: Failed to create block entity storagedrawers:basicdrawers.1
java.lang.StackOverflowError: null
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawers.hasCapability(TileEntityDrawers.java:640) ~[TileEntityDrawers.class:?]
	at com.jaquadro.minecraft.storagedrawers.block.tile.TileEntityDrawersStandard$GroupData.hasCapability(TileEntityDrawersStandard.java:225) ~[TileEntityDrawersStandard$GroupData.class:?]

Edit: The crash also happens on placing a new drawer.

jaquadro · Answer 29 · 2017-07-08T17:26:48.000Z

Thanks, it's fixed now in 5.2.3.

jaquadro · Answer 30 · 2017-07-08T06:13:47.000Z

I've opened a PR with what I think are the final changes needed: #1349

First, the extraction problem was a really egregious bug on my side, from trying to be clever with arithmetic. It would have massively duped with any kind of automation. Thanks for catching it; I forgot to test extraction.

I switched up the implementation a bit to take advantage of all IDrawerGroups also being ICapabilityProviders, and taking the repo capability off of that. Should it be absent for some reason (which would be a third party implementor), it should just no-op without harm. That leaves the slotty IDrawer stuff available where it makes sense.

And lastly I took your recommendation and opened an account on Bintray. API and jars are now being hosted there, so I updated your build file to point there and remove the embedded API. Between the multiple crash reports I've had and KingLemming's ranting about the RF API, I'm concluding that the Forge API system is just broken.

Share to