Issue / Bug

Duping items

https://clips.twitch.tv/PluckyPlayfulPanFUNgineer

Expected Behavior

Backpack interface to close when put into offhand or when dropped

Possible Solution

Steps to Reproduce (for bugs)

1.open a backpack
2.put backpack into off hand with keyboard shortcut
3.put backpack into inventory with keyboard shortcut in a different spot
4.drop backpack
5.take items from interface

Client Information

Modpack Version:3.0.6
Java Version:8
Launcher Used:ATLauncher
Memory Allocated: 4GB

From Darkosto's SevTech bugtracker: DarkPacks/SevTech-Ages#1865

You can only remove the bag from your inventory using the new open backpack keybind.

More duping https://clips.twitch.tv/BlightedSpicyFennelDerp

I believe it is both due to Quark, specifically: https://github.com/Vazkii/Quark/blob/master/src/main/java/vazkii/quark/management/feature/FToSwitchItems.java

can confirm disabling in the quark.cfg on the server solved it for us
management > B:"Press F in the inventory to switch item to main hand"=false

It's a case of Quark not checking to see if the slot can be interacted with before yanking it's item out.

Absolutely, quark should check first before doing slot interact

Also should be fixable in mod using public boolean canInteractWith(EntityPlayer playerIn) inside ContainerBackpack so the bag can close inventory right away the second its not in the slot anymore

EDIT:
Gif with fix in place on my bag
https://i.imgur.com/nNUJobe.gif

My solution linked below, feel free to use if you like. I probably went a bit overboard, i set a UUID into the item stack when its used to open the bag, on the edge case the user puts a second bag in offhand and switches between. So the second the "player held bag" is not the right thing it closes.

Lothrazar/Cyclic@4e18064#diff-e5c7a95a7e3ff7f7f53b4705f016ec5cR82

It should be enough to save the ItemStack-instance in the Container and check whether it reference-equal to the one in the players hand - this means it cant be swapped with another stack while allowing nbt to change

As this issue been reported to Quark already?

Summoning @Vazkii - as no, it has not yet been reported. Would this issue be best fixed on your end, or mine? I get the impression that you should be checking validity first anyway, but I can just do my own fix if needed as well.

Something mentioned by Soaryn... he suggested storing the contents the same way something like an ender pouch does - store the UUID of the player and store the actual contents in a data file.

If you can fix what you can that would be great. Quark hasnt really had any attention in months now.

@justinrusso not UUID of player, UUID of bag. Have inventory storage separate from the item (global for world for example) and have the bag store just a pointer to that storage. That way dupe bug disappears.

External storage raise the risk of desynchronization. EU2 golden bag of holding will close the GUI as soon as the bag moves. Can we achieve something similar in IronBackpacks?

I used to have the UUID for bags in the 1.10 version...and I'd like to stay away from it if possible. While it solved some issues it also caused quite a few. For example, initializing the UUID becomes surprisingly tricky with how many ways there are to open a bag. Synchronization is another pain point.

I will likely just close the GUI as soon as it is moved. I would like to first do a quick PR to Quark though, as ideally it should be fixed there anyway. Vazkii pushed out a build yesterday so the repository should be active enough for the fix to go through.

This isn't just caused by Quark though. For example, a player interface with a hopper extracting the backpack from the player inventory while the backpack is opened.

Good point. I will definitely add in the GUI closing then 👍

So you can either plug every single way a backpack can be moved around in players inventory or make one change that prevents all movement-based dupe bugs AND makes duping backpacks pointless (aside from making them equivalent to enderpouches).

From a quick look at the code, only the BackpackInfo.fromStack method would need to be updated to read from external file (world nbt, custom file, whatever) based on the NBT of the pack - instead of having the packInv tag contain a list of items, have it contain GUID of the pack.

Yep @justinrusso good find. You can improve the canInteractWith check to also protect against dupe bugs with AA Player Interface block. Linking fix for cyclic storage bag in case it helps anyone, i used betweenlands code as a reference.

Lothrazar/Cyclic@6a60672#diff-e5c7a95a7e3ff7f7f53b4705f016ec5cR90

It should auto close as soon as AA player interface with a hopper below pulls it out , steps to reproduce in gif.
before fix i was able to dupe the redstone after it got pulled out and stayed open
https://cdn.discordapp.com/attachments/443078370721267712/450018006064562206/AA_betweenlands_fix.gif

read from external file (world nbt, custom file, whatever)

Haha no.

Then just use global World Saved Data. And I forgot about IronBackpacksAPI.applyPackInfo that would need to write/mark the data as dirty.

My main point is that no matter what switching to global inventory storage would take, it will always be easier than trying to plug all methods of moving items in players inventory. From basic ones like switching to offhand, player interfaces (historically provided by multiple mods, I know of Actually Additions, Random Things, some OpenComputers/ComputerCraft addons and at least one dedicated mod) to more esoteric like rats stealing items in Invasion (and didn't at least one of the magic mods had a spell that messed with inventory?).

just close the inventory and cancel current action if the backpack is no longer there. External saving is just adding complexity and its own set of issues.

The Betweenlands interaction seems satisfactory... https://github.com/Angry-Pixel/The-Betweenlands/blob/b3fc6f38982be87f67bddd1042ef4986a631d3bd/src/main/java/thebetweenlands/common/inventory/container/ContainerPouch.java#L62-L91

In addition, after thinking about the global inventory concept... it would be quite a pain to implement especially without a Minecraft version change. This would depreciate the old backpacks and cause the need to for methods to transfer the inventory over if it detects its the old bag.