Duplication
Danekjovax opened this issue · 11 comments
Mekanism logistical pipes (mainly advanced/elite) cause lootbag duplication when connected to Lootbag Storage block.
Steps to reproduce:
- Get 1 Lootbag Storage block, 1 advanced/elite logistical pipe, 1 vanilla chest, and a few lootbags to put into storage (just to get something to pull out).
- Note the lootbag value. Set storage to a bag you can pull out given the lootbag value you've placed into it.
- Hook up the pipe between the storage block and chest. Set the side of the pipe facing the storage to pull.
- Observe the multiple bags of the selected type being pulled into the chest and compare to the value being deducted from the storage block.
Version (make sure you are on the latest version before reporting):
Forge: 1.12.2-14.23.5.2807-universal
Mekanism: 1.12.2-9.7.9.380
Other relevant version: LootBags-1.12.2-2.5.8.5
[gist/pastebin/etc link here]
Lootbag: Malorolam/LootBags#155
Thanks for your time.
I will be the first to admit I'm not the most up to date on other updates including additions to Forge, so that information is likely no longer valid. I haven't tested this since February or with anything other than whichever version of Mekanism that defaultly downloads from Curse, so if there has been an overhaul I may have not seen it.
You will need to re-test it then, we no longer use IInventory for logistical pipes, only IItemHandler
I'm also fairly certain the bug from the video was fixed; it shouldn't get extracted if it won't fit. But in the case it does get extracted it will usually be sent back where it came from.
Just tested it with all default configs with Mekanism 9.8.0.381 and Lootbags 2.5.8.5, there's still issues but some things seem better.
- logistic transporters will only insert into a bag storage with bags if the current bag is different from the one being inserted or the storage is empty. I think this has to do with inventory size and what's being reported. It seems like you're doing the can I insert logic on your end instead of trusting the target inventory to report the correct information.
- logistic transporters pull from the bag storage the reported stack instead of the item's maximum stack and decrement only 1. I'm not sure if I'm the only person who reports a non-correct stack size, but poking around a bit and I think if I update to using IItemHandler to control inventory that's fixable on my end so even if a pipe is saying to extract a larger stack I can make it conform to the stack size.
- Testing TE itemducts, they do not exhibit either of these issues.
- Testing EnderIO conduits, they have the second issue, but don't have the first issue.
I will look some into the first issue of that, though if I remember to code correctly the part where we are checking the inventory stuff our self instead of simulating it, is a partial check for performance reasons of also checking all the en-route itemstacks. So I am unsure how much this can be changed, though I have done a good bit of performance work since then so maybe it doesn't matter quite as much. Though honestly you may want to decide to override the int getSlotLimit(int slot)
in the IItemHandler to report the actual limit of loot bags which would probably fix the insertion for us (and if anyone else happens to be doing their own partial simulations/prechecks).
In terms of the second issue, I believe that is on your end (or maybe in forge's wrapper) as this is the javadocs for the extractItem method:
/**
* Extracts an ItemStack from the given slot.
* <p>
* The returned value must be empty if nothing is extracted,
* otherwise its stack size must be less than or equal to {@code amount} and {@link ItemStack#getMaxStackSize()}.
* </p>
*
* @param slot Slot to extract from.
* @param amount Amount to extract (may be greater than the current stack's max limit)
* @param simulate If true, the extraction is only simulated
* @return ItemStack extracted from the slot, must be empty if nothing can be extracted.
* The returned ItemStack can be safely modified after, so item handlers should return a new or copied stack.
**/
@Nonnull
ItemStack extractItem(int slot, int amount, boolean simulate);
Which states that the amount "may be greater than the current stack's limit".
video example: https://0x0.st/zfzK.mp4
quote from LootBags issue report:
Looking at the code, it seems that they're using getStackInSlot, which is for querying the item in the slot and not calling decrStackSize or removeStackFromSlot at all. Then they're just telling the tile entity to set that slot to the appropriate reduction (which for me is empty in all cases) instead of asking the tile entity to reduce or empty the slot using the decrStackSize or removeStackFromSlot.
I will try to look into this sometime this weekend. That comment of theirs isn't strictly true however, as various subversions of Mekanism 9.7.x worked on changing how Item transferring works. I am not sure about back when that issue was originally opened there (as I was not part of the Mekanism team then), but I do know that what we currently are using is IItemHandler#extractItem
. Would you mind checking on a newer version of forge (2833 or higher) if this is still an issue?
Looking at LootBags code for how they implement IItemHandlers they use Forge's SidedInvWrapper
which in versions less than forge 2833 has a bug in the implementation of one of the methods. MinecraftForge/MinecraftForge@45cadab
I am not sure if the isItemValid method propagates up and would cause our extractItem
call to not extract from the correct slot, but given you already have a setup that you know is reproducible for testing the dupe bug it would be helpful if you could quickly check to see if it still happens in newer versions of forge. If it does I will be more than happy to continue trying to figure out the problem.
the server is using forge-1.12.2-14.23.5.2836-universal, which version would you like it tested on?
That version includes the fixes to the wrapper. So that should be fine, I will try to get a chance to look into this further sometime this weekend/week.
Due to this issue existing in an outdated MC version, the mass changes and improvements we’ve made to inventory handling and transporter mechanics, and the Loot Bags dev being inactive for over a year now, I’m marking this as closed. If it recurs similarly in recent versions we’ll look into this again.