Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support fabric context update mutated stack #728

Closed

Conversation

ZurrTum
Copy link

@ZurrTum ZurrTum commented Nov 25, 2024

Add ContainerItemContext support to EnergyStorageAdapter
related to TechReborn/TechReborn#3302

Question:

EnergyStorage.ITEM.registerForItems has context parameter but is not used.

private void registerEnergyItemProviders() {
EnergyStorage.ITEM.registerForItems(
(stack, context) -> new EnergyStorageAdapter(Items.INSTANCE.getWirelessGrid().createEnergyStorage(stack)),
Items.INSTANCE.getWirelessGrid()
);
Items.INSTANCE.getControllers().forEach(controller -> EnergyStorage.ITEM.registerForItems(
(stack, context) -> new EnergyStorageAdapter(controller.get().createEnergyStorage(stack)),
controller.get()
));
EnergyStorage.ITEM.registerForItems(
(stack, context) -> new EnergyStorageAdapter(PortableGridBlockItem.createEnergyStorage(stack)),
Items.INSTANCE.getPortableGrid()
);
EnergyStorage.ITEM.registerForItems(
(stack, context) ->
new EnergyStorageAdapter(Items.INSTANCE.getWirelessAutocraftingMonitor().createEnergyStorage(stack)),
Items.INSTANCE.getWirelessAutocraftingMonitor()
);
}

Discovered by comparing TechReborn:
https://github.com/TechReborn/Energy/blob/da313914b1efe29632300900ba06f95e8c1d60b8/src/main/java/team/reborn/energy/impl/EnergyImpl.java#L23-L29

EnergyStorage.ITEM.registerFallback((stack, ctx) -> {
        if (stack.getItem() instanceof SimpleEnergyItem energyItem) {
                return SimpleEnergyItem.createStorage(ctx, energyItem.getEnergyCapacity(stack), energyItem.getEnergyMaxInput(stack), energyItem.getEnergyMaxOutput(stack));
        } else {
                return null;
        }
});

https://github.com/TechReborn/Energy/blob/da313914b1efe29632300900ba06f95e8c1d60b8/src/main/java/team/reborn/energy/impl/SimpleItemEnergyStorageImpl.java#L48-L62

private final ContainerItemContext ctx;
...
private boolean trySetEnergy(long energyAmountPerCount, long count, TransactionContext transaction) {
        ItemStack newStack = ctx.getItemVariant().toStack();
        SimpleEnergyItem.setStoredEnergyUnchecked(newStack, energyAmountPerCount);
        ItemVariant newVariant = ItemVariant.of(newStack);

        // Try to convert exactly `count` items.
        try (Transaction nested = transaction.openNested()) {
                if (ctx.extract(ctx.getItemVariant(), count, nested) == count && ctx.insert(newVariant, count, nested) == count) {
                        nested.commit();
                        return true;
                }
        }

        return false;
}

Simple fix:

if (ctx != null) {
        ItemStack itemStack = null;
        if (energyStorage instanceof ItemBlockEnergyStorage itemBlockEnergyStorage) {
                itemStack = itemBlockEnergyStorage.getStack();
        } else if (energyStorage instanceof ItemEnergyStorage itemEnergyStorage) {
                itemStack = itemEnergyStorage.getStack();
        }
        if (itemStack != null) {
                ctx.exchange(ItemVariant.of(itemStack), 1, transaction);
        }
}

screenshots

Because the code structure is too nested, itemStack cannot be accessed directly, so it is obtained through instanceof.
Maybe the data structure needs to be optimized

@ZurrTum
Copy link
Author

ZurrTum commented Nov 25, 2024

More

https://github.com/TechReborn/TechReborn/blob/1.21/RebornCore/src/main/java/reborncore/common/powerSystem/PowerAcceptorBlockEntity.java#L192

ContainerItemContext.ofSingleSlot(InventoryStorage.of(inventory, null).getSlots().get(slot)).find(EnergyStorage.ITEM)

find function call:

EnergyStorage.ITEM.find(containerItemContext.getItemVariant().toStack(), containerItemContext)

EnergyStorage.ITEM.registerForItems((stack, context) -> new EnergyStorageAdapter...

toStack will create a new copy:

this.isBlank() ? ItemStack.EMPTY : new ItemStack(this.getRegistryEntry(), count, this.getComponents());

@ZurrTum ZurrTum force-pushed the fix/GH-696/fabric-energy branch from a0b104e to 48ea4a8 Compare November 25, 2024 06:26
@ZurrTum ZurrTum force-pushed the fix/GH-696/fabric-energy branch from ee25b19 to c1fe01f Compare November 26, 2024 03:51
@raoulvdberge
Copy link
Contributor

Thanks, I will review this this weekend.

@ZurrTum ZurrTum closed this by deleting the head repository Dec 7, 2024
@raoulvdberge
Copy link
Contributor

Why did you close this PR?

@ZurrTum
Copy link
Author

ZurrTum commented Dec 7, 2024

Because I thought it was inactive and it was a simple fix. Code structure may also need to be adjusted to support ContainerItemContext.

@raoulvdberge
Copy link
Contributor

I've been inactive the past 2 weeks yes, but was still planning to merge this...

@raoulvdberge
Copy link
Contributor

Thank you for the fix, I've integrated this in PR #741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants