Fix large turbine recipe selection

[josiah-roberts]

What

Large turbines have a disagreement between recipe selection and recipe execution.

• For selection: any recipe where fuel inputs are available
• For execution: must have fuel inputs * parallel available

This leads to cases where turbines get stuck thinking they can run a recipe (findRecipe, checkPreviousRecipe), but fail to actually run the recipe (prepareRecipe). Because there is a disagreement about what it means to have a valid recipe, the turbine won't switch to other fuels even if they are available.

Implementation Details

This PR updates LargeTurbineWorkableHandler to consider the parallelism of the turbine when implementing findRecipe and checkPreviousRecipe. This ensures that recipes with enough fuel for base but not enough for parallel are not selected / retained.

Note that this has the drawback of requiring a handshake between findRecipe + checkPreviousRecipe and prepareRecipe. There is no hard guarantee that they will agree (this PR fixes such a disagreement).

Alternatives considered (discussed in Discord):

• Filter to parallel-runnable recipes, and also yield recipe * parallel instead of recipe
  • Pro: enforces agreement between different parts of the code, single-source-of-truth for the recipe, parallel, etc.
  • Con: all the existing code around this assumes that the recipe passed around is a base recipe. Would require a lot of re-work
• Simply run for a shorter time if we can't run the full parallel * normal runtime
  • Pro: very simple
  • Con: violates same-runtime principle

Outcome

• Large turbines can freely switch fuels when they have base < fuel < base * parallel fuel
• A number of new overloads for findRecipe taking optional recipe predicates Removed and simply copied underlying function implementation based on @ghzdude's review

Additional Information

• I need to ensure that LargeTurbineWorkableHandler.excessVoltage isn't misbehaving when used prior to prepareRecipe, and figure out the reason it's a stateful class-level field to begin with.
  For efficiency adjustment in fuel consumption; impl should be right.
• I am also unsure if there's a cleaner way to manage docs for overloads.
  There doesen't appear to be, beyond see, which I'm not a big fan of here.
• There's a bit more factoring I want to do in LargeTurbineWorkableHandler.
  Done

Potential Compatibility Issues

Some turbines that jammed previously should run now. I don't think that should be a problem.

[ghzdude]

I don't think the added overloaded methods to AbstractRecipeLogic and RecipeMap are necessary, as you can call find() from the RecipeMap like so:

    // In LargeTurbineWorkableHandler...
    
    // override findRecipe() from ARL
    @Override
    protected @Nullable Recipe findRecipe(long maxVoltage, IItemHandlerModifiable inputs,
                                          IMultipleTankHandler fluidInputs) {
        return findRecipe(maxVoltage, inputs, fluidInputs, this::canDoRecipeConsideringParallel);
    }

    // local method for finding the recipe along with a new predicate
    protected @Nullable Recipe findRecipe(long maxVoltage, IItemHandlerModifiable inputs,
                                          IMultipleTankHandler fluidInputs, Predicate<Recipe> parallelCheck) {
        RecipeMap<?> map = getRecipeMap();
        if (map == null || !isRecipeMapValid(map)) {
            return null;
        }

        final List<ItemStack> items = GTUtility.itemHandlerToList(inputs)
                .stream().filter(s -> !s.isEmpty()).collect(Collectors.toList());
        final List<FluidStack> fluids = GTUtility.fluidHandlerToList(fluidInputs)
                .stream().filter(f -> f != null && f.amount != 0).collect(Collectors.toList());

        return map.find(items, fluids, recipe -> {
            if (recipe.getEUt() > maxVoltage) return false;
            return recipe.matches(false, inputs, fluidInputs) &&
                    (parallelCheck == null || parallelCheck.test(recipe));
        });
    }

This essentially does what findRecipe() in ARL does.

I also think canDoRecipeConsideringParallel() could be shortened, maybe.

getInputFluidStack() would also need to be updated to use the local findRecipe() method (with my version)

    // In LargeTurbineWorkableHandler...

    public FluidStack getInputFluidStack() {
        // Previous Recipe is always null on first world load, so try to acquire a new recipe
        if (previousRecipe == null) {
            Recipe recipe = findRecipe(Integer.MAX_VALUE, getInputInventory(), getInputTank(), null);

            return recipe == null ? null : getInputTank().drain(
                    new FluidStack(recipe.getFluidInputs().get(0).getInputFluidStack().getFluid(), Integer.MAX_VALUE),
                    false);
        }
        FluidStack fuelStack = previousRecipe.getFluidInputs().get(0).getInputFluidStack();
        return getInputTank().drain(new FluidStack(fuelStack.getFluid(), Integer.MAX_VALUE), false);
    }

Otherwise, I've tested the changes (both mine and this PR) in-game and the issue is fixed.

[josiah-roberts]

I don't think the added overloaded methods to AbstractRecipeLogic and RecipeMap are necessary, as you can call find() from the RecipeMap like so:

    // In LargeTurbineWorkableHandler...
    
    // override findRecipe() from ARL
    @Override
    protected @Nullable Recipe findRecipe(long maxVoltage, IItemHandlerModifiable inputs,
                                          IMultipleTankHandler fluidInputs) {
        return findRecipe(maxVoltage, inputs, fluidInputs, this::canDoRecipeConsideringParallel);
    }

    // local method for finding the recipe along with a new predicate
    protected @Nullable Recipe findRecipe(long maxVoltage, IItemHandlerModifiable inputs,
                                          IMultipleTankHandler fluidInputs, Predicate<Recipe> parallelCheck) {
        RecipeMap<?> map = getRecipeMap();
        if (map == null || !isRecipeMapValid(map)) {
            return null;
        }

        final List<ItemStack> items = GTUtility.itemHandlerToList(inputs)
                .stream().filter(s -> !s.isEmpty()).collect(Collectors.toList());
        final List<FluidStack> fluids = GTUtility.fluidHandlerToList(fluidInputs)
                .stream().filter(f -> f != null && f.amount != 0).collect(Collectors.toList());

        return map.find(items, fluids, recipe -> {
            if (recipe.getEUt() > maxVoltage) return false;
            return recipe.matches(false, inputs, fluidInputs) &&
                    (parallelCheck == null || parallelCheck.test(recipe));
        });
    }

This essentially does what findRecipe() in ARL does.

Ah, yeah - I was trying to avoid copy- 🍝 the internals of getting the recipie map and constructing the lists. Figured the overloads were a reasonable tradeoff for that, but I can flip it around.

I also think canDoRecipeConsideringParallel() could be shortened, maybe.

Lol it used to be canDoRecipe but I lengthened it to be more specific. Fine to chop it down.

getInputFluidStack() would also need to be updated to use the local findRecipe() method

👍

[josiah-roberts]

@ghzdude looking at your change to getInputFluidStack - wouldn't we always want to use the predicate (not passing null)? Otherwise we could get the fluid input stack of a different recipe that matches without the predicate, sorted higher than a recipe we can actually afford.

[ghzdude]

Lol it used to be canDoRecipe but I lengthened it to be more specific. Fine to chop it down.

maybe canDoParallelRecipe() or canDoRecipeParallel()?

Ah, yeah - I was trying to avoid copy- 🍝 the internals of getting the recipie map and constructing the lists. Figured the overloads were a reasonable tradeoff for that, but I can flip it around.

I mean, it's possible that the method could be moved to ARL for addons to use, but idk what tech or sereni thinks about that.

@ghzdude looking at your change to getInputFluidStack - wouldn't we always want to use the predicate (not passing null)? Otherwise we could get the fluid input stack of a different recipe that matches without the predicate, sorted higher than a recipe we can actually afford.

@josiah-roberts i've found that if you use the predicate, then it won't show any fuel once the turbine is formed, because there wouldn't be enough of anything to get a recipe

[josiah-roberts]

i've found that if you use the predicate, then it won't show any fuel once the turbine is formed, because there wouldn't be enough of anything to get a recipe

Sorry, I'm not following. It won't show any fuel where?

[ghzdude]

Sorry, I'm not following. It won't show any fuel where?

when you open the GUI of the large turbine, the bottom left bar shows the fuel and how full it is

here, to be precise:

[josiah-roberts]

Sorry, I'm not following. It won't show any fuel where?

when you open the GUI of the large turbine, the bottom left bar shows the fuel and how full it is

Yeah just discovered that.

I don't think that calling without a predicate is a viable solution though; imagine a case where enumerate recipes:

1. LPG comes first - we have some, but not enough for parallel
2. Nitrobenzene comes second - we have enough

If we run the fluid stack without the predicate logic, we'll pull the wrong fluid stack on prepare.

---

It sounds like we want the behavior of, "if there's not enough to actually run something, but we have some, then show that", then we need to do the equivelent of findRecipe(...) ?? super.findRecipe(...) - that is, search with the filters for actually being able to use a recipe and then, if we can't run any recipe, show the fluid of the first un-runnable candidate.

This is kinda gross though; we're potentially showing a fluid input stack for a recipe we don't think is valid. But it's the most-correct approach that comes to mind right now.

[ghzdude]

Yeah just discovered that.

I don't think that calling without a predicate is a viable solution though; imagine a case where enumerate recipes:

1. LPG comes first - we have some, but not enough for parallel

2. Nitrobenzene comes second - we have enough

If we run the fluid stack without the predicate logic, we'll pull the wrong fluid stack on prepare.

It sounds like we want the behavior of, "if there's not enough to actually run something, but we have some, then show that", then we need to do the equivelent of findRecipe(...) ?? super.findRecipe(...) - that is, search with the filters for actually being able to use a recipe and then, if we can't run any recipe, show the fluid of the first un-runnable candidate.

This is kinda gross though; we're potentially showing a fluid input stack for a recipe we don't think is valid. But it's the most-correct approach that comes to mind right now.

it would only be incorrect if previousRecipe was null, but once a valid recipe is found and run, it displays that instead

[josiah-roberts]

How about this? b19a2a9

[josiah-roberts]

And removed the overloads here: 1faffbc

[ghzdude]

mkay, this looks good to me now. I'm gonna wait and let the other devs look at this and see what they think.

[josiah-roberts]

mkay, this looks good to me now. I'm gonna wait and let the other devs look at this and see what they think.

Great, thanks for walking me through my first PR. 🙌🏻