Fabric API

Fabric API

106M Downloads

URGENT: shift-clicking any stack in a custom Container causes the game to freeze without crashing

LemmaEOF opened this issue ยท 4 comments

commented

Container#transferSlot and Container#insertItem both use while(true) loops, and one or both of them cause infinite loops in custom containers if any item is shift-clicked. This doesn't leave a stacktrace, and I haven't opened a profiler yet to inspect where things go wrong, but we may need to make some changes to either of those methods. This could come in the form of avoiding the loops entirely, or by adding extra breakpoints that modded containers are more likely to run into. In either case, this feels like something that Fabric API should absolutely deal with to make custom containers feasible.

commented

It doesn't help that (in my decompile) transferSlot is 277 lines, and insertItem is 123. The while(true) loops are in the middle of each, too, so we can't just inject, fix the loop, and cancel, and overwriting is a bad idea too.

commented

I've fixed this crash/hang on my end by overriding Container#transferSlot with a copy-pasted implementation from HopperContainer. This one works for all the inventories I have registered (with the exception of armor slots, but thats something i have to do).

    @Override
    public ItemStack transferSlot(PlayerEntity playerEntity_1, int int_1) {
        ItemStack itemStack_1 = ItemStack.EMPTY;
        Slot slot_1 = this.slotList.get(int_1);
        if (slot_1 != null && slot_1.hasStack()) {
            ItemStack itemStack_2 = slot_1.getStack();
            itemStack_1 = itemStack_2.copy();
            if (int_1 < this.inventory.getInvSize()) {
                if (!this.insertItem(itemStack_2, this.inventory.getInvSize(), this.slotList.size(), true)) {
                    return ItemStack.EMPTY;
                }
            } else if (!this.insertItem(itemStack_2, 0, this.inventory.getInvSize(), false)) {
                return ItemStack.EMPTY;
            }

            if (itemStack_2.isEmpty()) {
                slot_1.setStack(ItemStack.EMPTY);
            } else {
                slot_1.markDirty();
            }
        }

        return itemStack_1;
    }

However, if theres anything fabric can do to have this work OOTB, that'd be great.

commented

We've talked in the Discord about blocks extending GenericContainer, but I feel that it might be a nice idea to create a FabricContainer so we already have it there to add hooks in the future if needed.

commented

Almost all containers override the transferSlot method, because you need to implement the logic of "from which slots to which slots you can shift-click" there, and that is quite specific to each container. I don't deny there is some common logic that could be abstracted, if you make some assumptions about the container (I have my own generic container class).
And the insertItem should not get stuck in the infinite loop if your slots don't have any strange logic and work as expected by the minecraft code. Do you have any custom slots?