Refined Storage

Refined Storage

77M Downloads

external_storage dupe bug(1.12)

CatAndHorse opened this issue · 12 comments

commented

1.build an rs system with 2 external storage bus connect to 2 different chest
2.put 32 item into each chest
3.try to get 64 item by shift clicking in terminal or by export bus
4.you get 64 items and 32 remain in rs sys

Version (make sure you are on the latest version before reporting):

  • Minecraft: 1.12
  • Forge:2420
  • Refined Storage:1513

Does this issue occur on a server? [yes/no]
yes

commented

Are you using Colossal chests? If so, are you connecting External Storage to Chest core?

commented

Nope ,i tried vanilla chest , extrautil tiny chest and acatully addition wooden crate,all of them can reproduce this dupe

commented

I can't reproduce this.
x

I have yet to test it on a server, though.

commented

bug

need 1 chest from mod and have higher prority

tested extrautils small chest and actually addons crate they both can reproduce this

commented

In other words: this only happens with ExU mini chest? In that case, it might be an ExU dupe bug issue. Will still investigate.

commented

This appears to be a ExU issue.

I tested this with 2 external storages both connected to its own Mini-Chest.

The problem is as follows:

  • When shift clicking, RS asks 64 of that item
  • Then, ExU gives back a stack of 64 items even though it only has 32

Proof:
x

  • requested is 64, because we want 64 items for shift click
  • We got back a stack of 64 even though the Mini-Chest only has 32 of that item (look at took)

My testing setup:

x

Both Mini-Chests have 32 items. But ExU just gives back 64 items for the first chest, even though it only has 32. So RS stops and doesn't extract from the second one.

In conclusion: this is a ExU issue. They appear to trust the "amount" parameter without checking if it doesn't exceed their own stack size.

commented

And the problem is - @rwtema never reads github issues... ExtraUtils 2 issue tracker just drowned in issues

commented

I just pinged him on IRC, he might fix it.

commented

@raoulvdberge confirmed.

This bug can be reproduced. @CatAndHorse just forgot to mention about priority.

commented

But, as far as i can see it can be reproduced only with one-slot inventories. It just doesn't happen with regular chests only nor quark chests. Only Extra Utils mini chest can trigger that bug.
2017-08-09_07 52 46
2017-08-09_07 52 47
2017-08-09_07 52 50
2017-08-09_07 52 52
2017-08-09_07 53 01
2017-08-09_07 53 05
2017-08-09_07 53 08

commented

Actually additions' storage crates can reproduce this too @raoulvdberge
okay i will open issue there too

commented

I just looked into again some more and I was wrong: this is not an ExU or ActAdd issue. It's in fact an RS issue.

What's going on: I assumed IItemHandler#extract is guaranteed to make copies of the stack, and I could just safely modify the stack returned by it.

Apparantly it does copy the stack, but not when requesting more than the stack size.

    @Nonnull
    public ItemStack extractItem(int slot, int amount, boolean simulate) {
        if (amount == 0) {
            return ItemStack.EMPTY;
        } else {
            this.validateSlotIndex(slot);
            ItemStack existing = (ItemStack)this.stacks.get(slot);
            if (existing.isEmpty()) {
                return ItemStack.EMPTY;
            } else {
                int toExtract = Math.min(amount, existing.getMaxStackSize());
                if (existing.getCount() <= toExtract) {
                    if (!simulate) {
                        this.stacks.set(slot, ItemStack.EMPTY);
                        this.onContentsChanged(slot);
                    }

                    return existing;
                } else {
                    if (!simulate) {
                        this.stacks.set(slot, ItemHandlerHelper.copyStackWithSize(existing, existing.getCount() - toExtract));
                        this.onContentsChanged(slot);
                    }

                    return ItemHandlerHelper.copyStackWithSize(existing, toExtract);
                }
            }
        }
    }

Look at the return existing;.
Thanks to that, RS just modifies the stack when extracting and false assumptions are made.