[Bug] All items disappeared from the backpack (ProjectRed Exploration)

[KarmyDev]

Minecraft version

1.20.1

CodeChickenLib version

4.4.0.516

CBMultipart version

3.3.0.146

ProjectRed version

4.20.1-beta+4

What happened?

I was playing on a server, I've had an orange backpack that I've put a few valuable things in, it mainly had create stuff, but also some iron ingots and compressed iron ingot (mekanism).
I remember putting basic fluid tank inside, one of which contained 6x buckets of lava.
I think I opened backpack near cauldron with lava, I was probably looking at JEI? then I pondered on chute from create by holding W key and opening pondering menu, then I closed it by hitting E, when I opened the backpack again, it was empty.
Everything was gone, I relogged to the server but it was still gone.

Crash log

There was no crash present, I went to prism log, but nothing was thrown when this happened.
I've also asked the owner of the server, but server's console was empty as well.

How do you make it happen again?

My guesses are that the issue lies within either Mekanism's Basic Fluid Tank, JEI, Create Pondering or some unknown bug?
Or possibly dropping all the contents of backpack into lava in cauldron? But that's very unlikely? Considering it was filled with items, and all I did was just look at chute pondering (moved with keys like W, D, and E to close pondering).

[GreenateGreenworld]

hungry.backpack.mp4

I managed to recreate this bug.
On servers with enough lag, if you right click the backpack and swap to another item slot before the backpack GUI comes up, the backpack's data seems to be sent to limbo.

[KarmyDev]

After looking at the BackpackInventory.java code, I've noticed that the code responsible for opening gui... just straight up removes items from backpack? That's a very.. odd way to handle it.

ProjectRed/exploration/src/main/java/mrtjp/projectred/exploration/inventory/BackpackInventory.java

Lines 15 to 25 in 7195120

	public void loadInventoryFromMainHand(Inventory playerInventory) {
	ItemStack backpack = playerInventory.player.getMainHandItem();
	if (BackpackItem.isBackpack(backpack)) {
	CompoundTag inventoryTag = BackpackItem.getBackpackInventoryTag(backpack);
	load(inventoryTag);
	// Delete inventory from stack. It will be saved back once container closes
	BackpackItem.deleteBackpackInventory(backpack);
	BackpackItem.setBackpackOpenedFlag(backpack, true);
	}
	}

And it relies on player having correct item in hand, since items are being copied to temporary inventory and deleted from original backpack.

ProjectRed/exploration/src/main/java/mrtjp/projectred/exploration/inventory/BackpackInventory.java

Lines 27 to 35 in 7195120

	public void saveInventoryToMainHand(Inventory playerInventory) {
	ItemStack backpack = playerInventory.player.getMainHandItem();
	if (BackpackItem.isBackpack(backpack)) {
	CompoundTag inventoryTag = new CompoundTag();
	save(inventoryTag);
	BackpackItem.saveBackpackInventory(backpack, inventoryTag);
	BackpackItem.setBackpackOpenedFlag(backpack, false);
	}
	}

However if player is not holding anything (client quickly swapped item being held in hand), there is a race condition with server, causing the server to open backpack gui, switch the item being held by player, and then close it because said item being held in the hand is not a BackpackItem.

Server then tries to save inventory back to the backpack that was held by player but uh oh... player is not holding any backpack in their hand! So it won't save anything.. And the items inside backpack were already voided by loadInventoryFromMainHand method!

Which further confirms the bug..

There needs to be better solution to loading items / modifying items in a backpack.

One thing that could be done to fix this issue is to keep a reference to an item that was being held by the player (and not the player's hand slot!!) and save items to it upon closing the gui.

Additionally server should change player's selected hotbar slot back to the one where backpack was held (which kinda should fix the issue with saving, but if there is already a reference being kept to the opened backpack, it may be better to just save it to the referenced item).

[MrTJP]

Thanks for digging into this. This drastic measure of clearing the backpack on open was an attempt to fix many duplication exploits that kept getting discovered. Open to hearing alternative solutions. Let me try to understand your logic:

1. Client opens the backpack UI with rightclick. Packet gets sent to server to open
2. Client quickly swaps slots before the UI comes up. Packet also sends to server.
3. Server opens backpack as planned, but is then also forced to process the slot swap while the backpack is open
4. When backpack is closed, the "held" slot is no longer a valid target to save the inventory contents, and so it is voided.

I think holding an item reference is a good plan. I did not do that initially because I wasn't sure under what conditions the item reference stays valid (like what if the player's state gets reloaded, etc). But should be safe to assume the inventory contents will stay safe until backpack is closed.

For additional robustness, I can also save the slot number and use item reference as a fallback (or vice-versa).

[KarmyDev]

Mhm, sounds good.
However I've noticed many other mods we had installed behaved differently upon trying to recreate this bug.
Mekanism just didn't allow player to swap inventory after they rightclicked.
Satchel mod from Thermal Series just closed itself when slot was changed, iirc.

Akin to this mod, I've found that generally inventories mods don't store items in their item form, one thing is that the data inventory holds gets send with player, and this may eventually lead to player not being able to connect to server because they hold too much data (however I'm not sure how true is that in practice).

For instance Mekanism has it's own PersonalStorageManager, that allows it to create inventories and associate them with certain item, as well as deleting inventories as far as I know.

This moves the need to store any data from the backpack item, onto the custom inventory manager.
The backpack then is used as a "facade" for the actual backpack inventory.

This is how I see it:

BackpackInventoryManager stores a Map between ItemStack, and a custom Inventory containing items, and provides methods for Creating, Deleting inventories as well as getting inventory by ItemStack. (and obviously methods for saving and loading that data.)

BackpackItem has a method upon use, that:
  if it is called on client of server, asks server for the inventory associated with this ItemStack.
  if it is called on server, tries to create inventory if none is present in BackpackInventoryManager,
  otherwise returns the inventory from said manager to the client.

and, BackpackItem also overrides onDestoryed method (i think that's it?), however it should only be called on server and call delete inventory from BackpackInventoryManager by ItemStack.

(Apparentally ItemStack provides item.level().isClientSide, which allows for checking if it's called on client.)

 
At least this is how Mekanism manages things.
I think you could move the OpenInventory method onto BackpackInventoryManager itself, and just call it from item.