Fabricators-of-Create / Create

[Fabric Mod] Building Tools and Aesthetic Technology
MIT License
834 stars 188 forks source link

Putting a Diamond Crack Hammer from Resourceful Tools mod in a Create Mixer causes crash #418

Closed xRebellion closed 1 year ago

xRebellion commented 1 year ago

Describe the Bug

As the title suggests, putting a diamond crack hammer (Resourceful Tools) inside a Create mixer causes crash.

Reproduction Steps

  1. Build a powered Create mixer with basin
  2. Insert Diamond Crack Hammer from Resourceful Tools to basin
  3. Crash happens, and the world can no longer be opened as long as the crash remains

Expected Result

It should not have crashed

Screenshots and Videos

No response

Crash Report or Log

https://pastebin.com/XCmkmcyj

Operating System

Ubuntu 20.04, Windows 11

Mod Version

0.4.1

Minecraft Version

1.18.2

Other Mods

A lot, I was using AQM2 modpack while doing this. But the crash happened when doing it with Resourceful Tools. I also mentioned it in https://github.com/itsdinkd/AQM2/issues/69 but it seems like the modpack author cannot do anything about it, so I try to report it here.

Additional Context

It only crashes once the mixer runs on power. When it doesn't run, it doesn't crash.

Platymemo commented 1 year ago

This is due to Resourceful Tools (and gobber and any other kwpugh mod that relies on their pugh_lib for the recipe remainder API, which I cannot find source code for) using a custom recipe remainder implementation that breaks expectations (has a recipe remainder but returns null on the vanilla recipe remainder call). I would report this to them instead. Their private library means it is very hard or likely impossible to work around.

xRebellion commented 1 year ago

Hmmm, I see. I'll try reporting to Resourceful Tools' author. Thank you

kwpugh commented 1 year ago

@Platymemo Can I request your insight on this?

Pugh_lib provides a mixin into Recipe.class:

@Mixin(Recipe.class)
public interface RecipeMixin<C extends Inventory>
{
    @Inject(at = @At("HEAD"), method = "getRemainder", cancellable = true)
    default void getRemainder(C inventory, CallbackInfoReturnable<DefaultedList<ItemStack>> cir)
    {
        DefaultedList<ItemStack> defaultedList = DefaultedList.ofSize(inventory.size(), ItemStack.EMPTY);

        for(int i = 0; i < defaultedList.size(); ++i)
        {
            Item item = inventory.getStack(i).getItem();
            ItemStack stack = item.getDefaultStack();

            if(item instanceof CustomRecipeRemainder && item.hasRecipeRemainder())
            {
                defaultedList.set(i,((CustomRecipeRemainder) item).getRecipeRemainder(inventory.getStack(i)));
                cir.setReturnValue(defaultedList);
            }
        }
    }
}

where CustomRecipeRemainder is just an empty interface.

Then the CrackHammer:

public class CrackHammer extends Item implements CustomRecipeRemainder
{
    public CrackHammer(Settings properties)
    {
        super(properties);
    }

    @Override
    public boolean hasRecipeRemainder()
    {
        return true;
    }

    @Override
    public ItemStack getRecipeRemainder(ItemStack stackIn)
    {
        ItemStack stack = stackIn.copy();
        stack.setDamage(stack.getDamage() + 1);

        if(stack.getDamage() >= stack.getMaxDamage())
        {
            stack.decrement(1);
        }

        return stack;
    }

    @Override
    public void appendTooltip(ItemStack stack, World world, List<Text> tooltip, TooltipContext context) {
        super.appendTooltip(stack, world, tooltip, context);
        tooltip.add((new TranslatableText("item.resourceful_tools.crack_hammer.line1").formatted(Formatting.GREEN)));
    }
}

The CrackHammer allows for a crafting action, takes some durability, and remains in the crafting grid.

I'm not sure why it would return null.

Any insight would be greatly appreciated.

Regards.

TropheusJ commented 1 year ago

The problem is the vanilla recipe remainder getting isn’t overridden and returns null. When hasRecipeRemainder is true, the result of get should never be null. It has no parameters which is why this is a problematic situation and can’t really be fixed easily.

TropheusJ commented 1 year ago

A workaround would be to add a hasCustomRecipeRemainer, and use that instead. This would make stuff treat it as if there’s no remainder unless support is explicitly added for custom stuff.

kwpugh commented 1 year ago

If the mixins conditions are true a DefaultedList is returned that at worst contains ItemStack.EMPTY which isn't null.

I do not understand why that would be null.

      if(item instanceof CustomRecipeRemainder && item.hasRecipeRemainder())
            {
                defaultedList.set(i,((CustomRecipeRemainder) item).getRecipeRemainder(inventory.getStack(i)));
                cir.setReturnValue(defaultedList);
            }
Platymemo commented 1 year ago

Vanilla's Item#getRecipeRemainder() is a parameter-less method. That method you're using is a custom implementation. Create's basin recipe directly uses the item's hasRecipeRemainder() and getRecipeRemainder() instead of the Recipe#getRemainder(C inv), and it's Item#getRecipeRemainder() returning null. I'd suggest doing as Troph said and using your own hasCustomRecipeRemainder() method instead of overriding hasRecipeRemainder() to prevent this conflict. Alternatively, you can mixin to Create for compat :ioa:

kwpugh commented 1 year ago

Honestly, I do not understand what a hasCustomRecipeRemainder would do and how it would hook into crafting system.

This was meant to be a temp solution until fabric put in the proper functionality, which is taking longer than I had hoped: https://github.com/FabricMC/fabric/pull/1180

I'm guessing we will have to live with the issue until then.

kwpugh commented 1 year ago

Screen Shot 2022-07-08 at 4 27 29 PM

it seems to crash on line 102. I cannot find getCraftingRemainingItem, is that vanilla code?

TropheusJ commented 1 year ago

it's vanilla, probably mapping differences.

In vanilla code, there's hasCraftingRemainingItem() and getCraftingRemainingItem(). has returns true if get != null. your extended remainders break this promise: has returns true, but the vanilla get returns null. You could add a custom has method, that only returns true if an item has one of your extended remainders. Problem is then your item is consumed entirely by stuff that only checks vanilla. You could make it so as a backup it has itself as a vanilla remainder, but then infinite durability. There's no good solution, but I'd personally go with having itself as a vanilla remainder. The best option is an official API.

kwpugh commented 1 year ago

I see these in the vanilla Item.class

Screen Shot 2022-07-08 at 4 45 56 PM

I'm not keen on the idea of the hammer used in automation anyway. I would at least like it to not crash the game.

My primary goal is to work in vanilla crafting grid and take durability damage with each use.

I was never able to figure out how to register the item with a recipe remainder because you cannot register a remainder that refers to itself.

kwpugh commented 1 year ago

Screen Shot 2022-07-08 at 4 53 05 PM

Seems like you can only assign a recipe remainder that is a different item

It is my understanding that this was one of things the Fabric PR mentioned above would address.

TropheusJ commented 1 year ago

you could override getRecipeRemainder in CrackHammer to return ItemInit.CRACK_HAMMER

kwpugh commented 1 year ago

I could, but that would just give the player a brand new Crack Hammer and I want it to take damage with each crafting. Screen Shot 2022-07-08 at 4 57 50 PM

and infinite ore doubling

TropheusJ commented 1 year ago

yeah, there's no way to do that with the vanilla stuff. You can either have infinite durability, or 1-use. If you don't want it automated, go with 1-use and have hasRecipeRemainder return false.

To be clear, this is just a fallback to avoid these crashes. There's no way to accomplish this without your extended remainder system, you need both that as well as a vanilla fallback.

kwpugh commented 1 year ago

Well, thanks for the insights. I will put a note on the CF page that the hammers are not compatible with the Create Mixer.