ComponentItemHandler not working correctly with AbstractContainerMenu#moveItemStackTo

[will-y]

Minecraft Version: 1.21

NeoForge Version: 21.0.42-beta

Steps to Reproduce:

1. Have an item container using a ComponentItemHandler
2. Implement quickMoveStack using AbstractContainerMenu#moveItemStackTo (EX: Chest implementation)
3. Shift click a stack into a container with a not full matching stack
4. The amount that was supposed to be added to that stack will be voided Before: After:

Description of issue: I think it is because ComponentItemHandler creates a copy of the item stack when retrieving from a slot. Then AbstractContainerMenu#moveItemStackTo modifies the stack in place and expects it to be saved. I was able to get around this issue by overriding AbstractContainerMenu#moveItemStackTo and changing the calls Slot#setChanged to Slot#set. Here is a simple example mod that shows this issue: https://github.com/will-y/QuickMoveTest

[Shadows-of-Fire]

I don't see a very good solution to this problem. There's an immutable vs mutable contract problem going on here, where the Component must be immutable and is unable to expose mutable data, but Slot expects the result of getItem to be mutable.

Realistically, solving this requires addressing that mutability disaster. The best-case scenario would be to deprecate Slot#setChanged and tell people to call Slot#set with the new ItemStack, and patch the vanilla cases where it might be problematic. The cases that might need updating are:

1. AbstractContainerMenu#doClick, where slot7.setChanged(); should be changed to slot7.set(itemstack9);
2. AbstractContainerMenu#moveItemStackTo, which has three sites (detailed in descending order): a. slot.setChanged(); >> slot.set(itemstack); b. slot.setChanged(); >> slot.set(itemstack); c. slot1.setChanged(); >> slot.set(itemstack1);

The most appropriate solution would be for vanilla to change Slot#setChanged() to protected, forcing external calls to go through Slot#set, but we can't impose that here.

[will-y]

I tried to write up a quick PR for this but it doesn't quite work.

1. This itemstack9 is the item already existing in the slot. If you click a stack into an empty slot, it will then set the slot to an empty stack and void the item.
2. The third instance here is the same sort of thing, itemstack1 is the item already in the slot.

The first one will be a problem for the ItemStackedOnOtherEvent mutating the stack or for people who override ItemStack#overrideOtherStackedOnMe

The second one seems like a redundant call because the slot1.setByPlayer calls setChanged() so it is probably fine to leave alone?

[Technici4n]

Some inspiration maybe: https://github.com/AztechMC/Modern-Industrialization/blob/1.21.x/src/main/java/aztech/modern_industrialization/inventory/HackySlot.java. This can be used with immutable stack storage.

[Shadows-of-Fire]

I had thought of doing that kind of cursed slot-stack-caching but was worried about what terrible side effects that might incur under various conditions.

[Technici4n]

I've been doing it for years :D

[will-y]

Would just adding this logic to SlotItemHandler be the best solution, or applying similar caching logic in ComponentItemHandler because that's where the problem really is.

[Technici4n]

IMO this should be as self-contained as possible. I would maybe reuse the HackySlot directly? This should certainly not leak to the item handler itself.

[will-y]

So create a new Slot that should be used for immutable ItemHandlers and then have SlotItemHandler only work with mutable ItemHandlers? Like maybe this? https://gist.github.com/will-y/c0e35adcedfe80f149bb9de9fa266d46

[Shadows-of-Fire]

Actually, looking closer at it, we only need to set itemstack9 to the slot if the stack was actually mutated (by way of ItemStackedOnOtherEvent or tryItemClickBehaviourOverride). The other code will never modify the stack, and instead always sets the new stack directly to the slot.

[Shadows-of-Fire]

Though trying to work that out I realize there's an issue with attempting to resolve the case where the stack has been changed (which should trigger a write-back) and a new stack has been set to the slot.

So it seems like we're left with the nonsense that is HackySlot as the only workable solution.

[Direwolf20-MC]

I'm running into this one myself - Should I implement my own Hackyslot, or is this something thats liable to be patched into Neo? :)

[will-y]

Created a PR for this. It seems to fix it here: https://github.com/will-y/QuickMoveTest, but if it's not what you meant I can change it