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

[1.20.1] EvilCraft & GTCEu-Modern Potion Bottle Added NBT #1084

Closed
loving2 opened this issue Jan 8, 2025 · 8 comments
Closed

[1.20.1] EvilCraft & GTCEu-Modern Potion Bottle Added NBT #1084

loving2 opened this issue Jan 8, 2025 · 8 comments

Comments

@loving2
Copy link

loving2 commented Jan 8, 2025

Issue type:

  • 🐛 Bug

Short description:

Installing both GTCEu-Modern v1.6.2 and EvilCraft 1.2.49 results in Minecraft:Potion items getting an additional NBT on them. This causes issues with any other mod that adds crafting recipes utilizing a Minecraft:Potion because they expect the one NBT specifying the Potion type.

Steps to reproduce the problem:

  1. Install GTCEu-Modern 1.6.2, EvilCraft 1.2.49, JEI v15.20.0.105, and for example Draconic Evolution v3.1.2.604
  2. Start a world in Creative
  3. Use JEI to bookmark the ingredients for the Damage Module from Draconic Evolution
  4. Give yourself the ingredients
  5. Hit F3+H and confirm that there are 2 NBT tags on the strength potion
  6. Place a Crafting Table down and try to craft the Damage Module

Expected behaviour:

No added NBT to a Minecraft:Potion item so that crafting recipes using the potion still work


Versions:

  • This mod: 1.2.49
  • GTCEu-Modern: 1.6.2
  • Minecraft: 1.20.1
  • Mod loader version: Forge 47.3.11

Additional Information:

You don't need to install JEI or Draconic Evolution to reproduce this. Having just GTCEu-Modern 1.6.2 and EvilCraft 1.2.49 is enough to see that potions have an additional NBT stored on them.

@rubensworks
Copy link
Member

Thanks for reporting!

@rubensworks
Copy link
Member

AFAIK, EvilCraft does not add any tags to potions.
Can you report this to the GTCEu-Modern issue tracker and link back here?
Happy to make changes here if the GTCEu-Modern mod author considers it relevant.

@rubensworks rubensworks moved this from To Do to On hold (awaiting input) in Maintenance Jan 9, 2025
@loving2
Copy link
Author

loving2 commented Jan 9, 2025

Sure thing - I reported it here first since it doesn't happen with just GTCEu-Modern installed and appears to be a mod interaction, but I'll make an issue there too. Issue here

GTCEu-Modern recently added functionality where Potions are now a Fluid, and my thought was the additional NBT could be getting added so the Dark Tank would work properly with it.

@loving2
Copy link
Author

loving2 commented Jan 13, 2025

What causes the poison bottle to have the NBT Fluid: {FluidName: "evilcraft:poison", Amount: 1000}? With just EvilCraft installed and no other mods. JEI shows a poison bottle with no NBT, cheating one into your inventory changes it to have that additional NBT.

This is what the added NBT looks like on the potions when GTCEu-Modern is also installed.

@loving2
Copy link
Author

loving2 commented Jan 21, 2025

I suspect something in EvilCraft may be calling the setFluid method on items that have a FluidHandlerItemStack capability, which is writing the Fluid tag to the item. I've made a PR to GTM that overrides the setFluid and getFluid methods here. This resolved the issue in my local testing instance.

@rubensworks
Copy link
Member

I suspect something in EvilCraft may be calling the setFluid method on items that have a FluidHandlerItemStack capability

AFAIK, we don't call this method anywhere: https://github.com/search?q=repo%3ACyclopsMC%2FEvilCraft%20setFluid&type=code
But if you see something I don't, I'd be happy to hear about it.

@loving2
Copy link
Author

loving2 commented Jan 21, 2025

Alright, I believe I've found where this is stemming from. I'm still not entirely sure I understand everything, but hopefully the below makes sense.

GTM uses the AttachCapabilitiesEvent<ItemStack> event to attach a Fluid Handler Item capability to potions.

if (itemStack.getItem() instanceof PotionItem) {
    LazyOptional<IFluidHandlerItem> handler = LazyOptional.of(() -> {
        var fluidHandler = new FluidHandlerItemStack.SwapEmpty(itemStack, new ItemStack(Items.GLASS_BOTTLE),
                PotionFluidHelper.BOTTLE_AMOUNT);
        fluidHandler.fill(PotionFluidHelper.getFluidFromPotionItem(itemStack, PotionFluidHelper.BOTTLE_AMOUNT),
                IFluidHandler.FluidAction.EXECUTE);
        return fluidHandler;
    });
    event.addCapability(GTCEu.id("potion_item_handler"), new ICapabilityProvider() {

        @Override
        public @NotNull <T> LazyOptional<T> getCapability(@NotNull Capability<T> cap,
                                                          @Nullable Direction side) {
            return ForgeCapabilities.FLUID_HANDLER_ITEM.orEmpty(cap, handler);
        }
    });
}

With just GTM installed, this is fine and does not set the NBT despite the fill method calling the setFluid method in FluidHandlerItemStack because of an attached capability to glass bottles that overrides the setFluid method. You'll notice in particular, after the NBT is set from super.setFluid(fluid), that it overwrites container and thus deletes the NBT data. I believe the general usage scenario here is that you take glass bottles from your inventory and click on a fluid inventory slot in a machine to fill the bottle with the potion. As well as being able to take potion bottles from your inventory and click on a fluid inventory slot in a machine to fill the machine with a potion and give you the glass bottle back.

else if (itemStack.is(Items.GLASS_BOTTLE)) {
    LazyOptional<IFluidHandlerItem> handler = LazyOptional.of(() -> new FilteredFluidHandlerItemStack(itemStack,
            250, s -> s.getFluid().is(CustomTags.POTION_FLUIDS)) {

        @Override
        protected void setFluid(FluidStack fluid) {
            super.setFluid(fluid);
            if (!fluid.isEmpty()) {
                container = PotionUtils.setPotion(new ItemStack(Items.POTION),
                        PotionUtils.getPotion(fluid.getTag()));
            }
        }
    });
    event.addCapability(GTCEu.id("bottle_item_handler"), new ICapabilityProvider() {

        @Override
        public @NotNull <T> LazyOptional<T> getCapability(@NotNull Capability<T> cap,
                                                          @Nullable Direction side) {
            return ForgeCapabilities.FLUID_HANDLER_ITEM.orEmpty(cap, handler);
        }
    });

}

From what I can gather, EvilCraft uses the RenderOverlayEventHook and iterates over the inventory getting the fluid handlers (if present) for the items. As seen above, the attached fluid handler for potions is a LazyOptional and includes the fill method. In this scenario, it will not call the setFluid override provided by the glass bottle's fluid handler, and thus the NBT will remain on the potion.

My PR to GTM resolves this by adding overrides to the setFluid and getFluid methods for the fluid handler attached to potions.

@rubensworks
Copy link
Member

Aha, good catch!

So it looks like this issue would have occurred with other mods as well.

But I'm glad to hear the issue is resolved now :-)

@github-project-automation github-project-automation bot moved this from On hold (awaiting input) to Done in Maintenance Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

2 participants