RFTools

RFTools

74M Downloads

[1.10.2-5.06] Item duping with Storage Drawers mod

misterg1 opened this issue ยท 27 comments

commented

If you craft something in the storage scanner that has an routable drawer from the Storage Drawers mod v3.2.1 by jaquadro, items that are used from the drawer get duplicated in the drawer itself. So after using one single piece from the drawer, the drawer gets filled up to its potential limit (defined by upgrades).

I don't know if thats RFTools' fault or the Storage Drawers' but there's obviously something wrong with the remaining items calculation.

EDIT: watched it closely again.. You don't even have to craft something. Hovering over the buttons (craft 1, 4, 8, stack) is enough to duplicate the items in the drawer. So it's more likely to be RFTools' issue..

commented

Yeah somehow my emeralds, diamonds and so on now decided to dupe, too.. I realized that it counts exponentially upwards.. About every 4 ticks your amount doubles..

commented

I can confirm that I've seen this behavior as well, but I've only actually noticed it with compacting drawers.

commented

Tested it with all: compacting, normal, 1x2, 2x2... Same thing... The interesting part is, that it doesn't work with all items.. Diamonds, emeralds and lapis don't dupe, iron, gold and redstone do (and many more)..

commented

Weird. For me, only compacting drawers have duped anything, including diamonds, emeralds, and lapis. It does't always go instantly to max capacity though. I saw my emeralds go from 100-ish to 1000-ish then like 6k, then suddenly 600k.

Edit: Also, I was seeing this on 5.03 as well as 5.06

commented

Confirming that I've had this issue too, specifically involving the Storage Scanner and Compacting Drawers. I've had the inventory numbers suddenly shoot to the drawer's max capacity, and I've also had inventories disappear to zero when extracting from the drawer.

commented

I've noticed the same issue, I suspected the storage scanner as well. Not all items seemed to do it. But hard to pin it down..

Item in storage drawer, routable. Tested obsidian and wood logs. Crafted with those items. the drawer shot to maximum.

I was able to dupe diamonds as well. Crafted a pick axe a few times. got nearly double diamonds. strange that it didn't goto max capacity to me.

commented

I don't see how this can be RFTools fault (although it is always possible) because I just access the storage drawer like I access any inventory (chest or whatever). My guess is storage drawer is doing something weird in its inventory handling. Was this reported to them too?

commented

Not yet, but as I mentioned above: hovering over your craft 1,4,8,stack buttons is enough to dupe the items so there should be virtually no item handling until then unless your code stores the items somewhere while hovering over these buttons...

commented

There is already item handling then as the crafting system will actually extract the items to see if it has enough and then places them back in exactly the same slots. That's how the hover operation works so even that does work on the item handler

commented

That's weird.. I never had any duping issues with the drawers and other methods of extracting and inserting items like pipes or applied energistics storage busses respectively refined storages external storage (raoulvdberge writes in his wiki for refined storage, that "There is special integration for Storage Drawers.")... Maybe they need special handling but otherwise everything works just fine

commented

The code starts here where the storage scanner gets a request to test if a crafting operation is possible: https://github.com/McJty/RFTools/blob/1.10/src/main/java/mcjty/rftools/blocks/storagemonitor/StorageScannerTileEntity.java#L105-L120

This creates an item source which is basically a collection of all routable inventories (and will include the storage drawer)

This will go to:
https://github.com/McJty/RFTools/blob/1.10/src/main/java/mcjty/rftools/craftinggrid/StorageCraftingTools.java#L193-L235

The actual inventory handling is done in the item source:
https://github.com/McJty/RFTools/blob/1.10/src/main/java/mcjty/rftools/craftinggrid/TileEntityItemSource.java

That code implements an RFTools interface to abstract away ItemHandlers and IInventories. It will always try to use the item handler first. This is probably the code you want to look at. I might be doing something wrong there but I wouldn't know what

commented

Alright, who can describe how to create a repro case (preferably with pictures?). Assume I have no working knowledge of RFTools.

Tangentially related: in my five minutes of fumbling around I put down a storage scanner, made a drawer routable, double-clicked an item shown, and crashed out to the launch screen. Large wall of stacktrace here: https://gist.github.com/jaquadro/b6060e3aaea37d198501d01270351d27

commented

@jaquadro I can add pictures in a minute, but basically:

  • Place down some drawers (any type, doesn't matter)
  • Put a drawer controller on them
  • Place a storage scanner
  • Set the radius on the scanner to include the drawer controller, but not the actual drawers
  • Put over 1 stack of items in the drawer
  • Go to the storage scanner, make sure the drawer controller is starred, and set a recipe for something that uses whatever you put in the drawers (e.g. cobble stairs if you put cobblestone in the drawer)
  • Hover over any of the craft buttons
  • You should now be duplicating items

Edit: Pictures

I tried replicating the bug using an Ender IO inventory panel, without any success
Also tried to replicate it using Storage Network, without any success

commented

Note that the storage scanner requires power. A creative powercell from rftools is the easiest way to give that

commented

@McJty I think the issue is https://github.com/McJty/RFTools/blob/1.10/src/main/java/mcjty/rftools/craftinggrid/StorageCraftingTools.java#L135

I changed that line to
undo.put(key, new ItemStack(input.getItem(), input.stackSize)); and the bug appears to be fixed

Edit: Made a PR for this

commented

a) I don't get why that would fix the bug
b) That's not a good fix as it will destroy NBT on items that are in a chest
c) input.copy() should make a proper copy of the itemstack WITH NBT

commented

a) Looking at this again, I realize I shouldn't code while I'm this tired. >_<
b) Ah, yes. I'm really new to modding MC. Didn't think about NBT issues. Good to know.

Could the issue be related to itemStacks that have stackSize > 64? I noticed that if I log the undo map, diamond shows up with a stack size > 64 when using a storage drawer. However, if I have a modular storage with an equal amount of diamonds, it shows up as < 64

I don't know the specifics about how MC handles item stacks, but seems like storage drawers might be reporting items as aggregate stacks, instead of multiple stacks <= 64 in size, which then causes duping when you call .putStack on line 160.

Anyways, I don't have any more time to test this tonight. Hopefully this is helpful. Like I said, I'm new to MC modding, but I'm glad to help out with testing this further :) I've learned a lot about how item stacks/storage work from investigating this bug

commented

Does storage drawers perhaps have a custom ItemStack implementation that doesn't properly support copy() or so? Just guessing here as I don't really know what is going wrong

commented

Please link me to the section of code that does this item handling.

Without any prior knowledge of what's going on, my guess is that you may be making assumptions that are valid for normal stack-limited inventories, but are not enforced by the contract of IItemHandler. I can try to review that interaction.

Unlike 1.7.10, I don't believe it's necessary to use my API to take full advantage of deep storage interactions; there are ways to get the necessary information with just IItemHandler.

commented

Put a drawer controller on them
Set the radius on the scanner to include the drawer controller, but not the actual drawers

I wanted to note that I saw the duping behavior in a setup without any Drawer Controller, and with the drawers (Compacting Drawers, to be specific) included in the Storage Scanner listing.

commented

I guess we can safely say that either rftools is making an assumption on how to use IItemHandler that isn't valid or else that storage drawers is implementing its ItemHandler wrong

commented

I need to wait until evening to look at this again. I could craft theories if it were just the compacting drawers, or just the controller at fault. I don't have anything for the general case right now.

commented

Alright, I think I've got to the bottom of it, and it's the kind of squishy ambiguous semantics business I thought it might be.

I think the problem is in TileEntityItemSource.putStackInSlot. A call is made to handler.getStackInSlot(), which returns an ItemStack for what's in that slot. If we check the API, its documentation states:

 * The result's stack size may be greater than the itemstacks max size.

So SD happily returns you an arbitrarily large stack. Then you attempt to call handler.extractItem() with the same count. Now here's where the problem creeps in. Let's review the documentation for that API entry:

 * Extracts an ItemStack from the given slot. The returned value must be null
 * if nothing is extracted, otherwise it's stack size must not be greater than amount or the
 * itemstacks getMaxStackSize().
 *
 * @param slot     Slot to extract from.
 * @param amount   Amount to extract (may be greater than the current stacks max limit)
 * @param simulate If true, the extraction is only simulated
 * @return ItemStack extracted from the slot, must be null, if nothing can be extracted

Based on the wording and some of the conversation in the PR that introduced IItemHandler, I interpret it as extractItem only allowing the extraction of stacks up to getMaxStackSize(), even if you ask for more. And thus repeated calls would be needed to drain the inventory. On the other hand it's a stupid-looking constraints and not consistent with with the rest of the API. So it's conceivable I didn't understand this right, and I'd be happy to make a change if that's the case. Ultimately I'm trying to stick to spec on this implementation.

So jumping back to your code, the extract didn't pull out as many items as you assumed it would, and then you try to insert a much larger stack that I assume was derived from another call to getStackInSlot. Pretty easy to see how the item count goes into runaway after repeated calls.

Happy to have your input on this, and any other authoritative info on IItemHandler that you may by privy to.

commented

On a side note, I'd suggest calculating a diff and adding or subtracting that amount rather than removing the entire inventory and then re-applying the original amount. Some inventories like SD do react immediately to an inventory going to 0, although I don't think it will do anything harmful currently.

commented

Thanks for the investigation. I will see if I can make my code safer to avoid the problem

commented

I'm changing this code to only extract amounts like you suggested. That should make it a lot more stable and robust

commented

I can no longer reproduce the dupe with storage drawers. Will be fixed next release