Fix Item Dupe #234

[ghost]

This fix handles two issues that cause item duplication:

1) The view was lazy, it updated the model when the windows was closed, rather than when changes occured. Because of this, items where effectively duplicated as soon as the container was changed, and rolled back to not duped when the view was closed, unless the backpack was somehow removed before the save would happen.

2) Minecraft needs to know when the inventory is no longer accessible to the player and should forcibly close the window, that's what canInteractWith() is for. Its implementation is what prevents item duplication in vanilla minecraft containers, and should be properly implemented in modded containers.

[ghost]

PS: Because Soaryn

[ghost]

@TehNut

Yes, it's required, it's one of the things that causes an item duplication. The problem here is that the view (the container) is not updating the model (the item stack). There are many ways the backpack can be removed from the inventory before the container is closed, and as soon as this happens the items get duped. There's no reliable way to stop this from happening, not without a coremod or changes to Forge.

In any case, writing to the NBT actually has no impact network wise, well maybe perhaps to the players nearby that receive an SPacketEntityEquipment every time the backpack's inventory changes, because the server thinks that because the NBT of the item in hand changed, everybody needs to know about it. For the player, the way GUI work in minecraft, they will not receive updates on the backpack unless they interact with its slot. Once the window closes and the loop goes back to checking the inventory, it notices that it's dirty and sends an update for the backpack.

But... I rather have no unnecessary updates altogether. Clients don't need to know about the backpacks inventory until they open it, so I made a few changes to implement just that. Needs more testing though.

[ghost]

Well... bugs have been introduced by the last commit I've made, and as far as I could see they are a consequence of using getNBTShareTag() to reduce the amount of data sent by the server:

1) This one is a showstopper, there are some cases where the backpack GUI would not stop the player from clicking on the opened backpack when it was opened using the open backpack key rather than right clicking it. This would actually cause a desync issue between client and server, and instead of duping, the backpack would be voided if the player attempts to throw it on the world.

2) This one was rather obvious but not serious, there are desync issues now while using the damage bar upgrade, caused by the fact that the client doesn't know about the contents of the backpack until it is opened.

I'm going to rollback that change until I can find a solution to these. In any case, this isn't required to fix the dupe bug, but I also don't enjoy taxing the network unnecessarily, even if it seems to be the norm among items with inventory stored in NBT in both modded and vanilla minecraft (shulker boxes), it simply doesn't feel like the right solution to leave it at that. What I'm pondering is if this is the right place to keep working on this, since the point of this was to fix the item dupe.

[gr8pefish]

Hehe welcome to inventories; it's stupid interconnected.

I'll review your commits later this week/weekend; I've just been stupid busy recently.

[ghost]

@gr8pefish no prob, I'm on a similar situation myself, I want to keep exploring options to reduce the network impact of items that have inventory, but hadn't had time to do it. From what I've seen neither other mods nor mojang code seem to care much about it (well, you might argue that mojang's solution to that is forcing you to place the shulker box to change it but still). The real challenge here is that, having inventories within items that can't be used for duping and at the same time are lightweight for the player network wise.

[ghost]

I was working on fixing the bugs that I found while trying to improve the syncing by reducing the size of the payloads by removing the inventory contents from the NBT data sent for backpack items, and I found out that one of the issues was already present due to the change I've made of eagerly saving the backpack contents when a slot is changed, but it wasn't noticeable because the server immediately sent an update for the backpack slot when it received a click on blocked slot (an invalid click causes the server to send a resync of the container), so even if the client didn't block the click on the slot, it would then receive a resync from the server that undid the wrong state.

What was happening was that the blocked stack that is set when using a key to open the backpack eventually failed the item stack equality check because of NBT differences of the empty slots. This difference happens because an empty stack is created with a stack size of 1, but when sent in a network packet a stack size of 0 is used. Moving items around eventually causes a desync between one or more empty slots, which makes the NBT comparison fail. Just another run-of-the-mill Minecraft weirdness.

The changes I pushed make it so that blocked stack isn't used but instead the blocked slot is set by number, as it happens when a backpack is opened with a right click.

I'm tempted to do a refactoring of the backpack container. I think the handling of locking the backpack slot and preventing blacklisted items to enter a backpack should be done on the slot setup, overriding the appropriate methods of each slot. And that would also help solve making the container close to prevent item duping in a more elegant way. It could also fix some other issues like the one with Inventory Sorter.