FuelRecipeLogic Rework

[galyfray]

What:

reworked the FuelRecipeLogic class adding methods and changing other methods to allow better flexibility of the class.

How solved:

methods added :

• calculateRecipeDuration // equivalent to calculateRecipeDuration(lastRecipe)
• calculateFuelAmount // equivalant to calculateFuelAmount(this.previousRecipe)
• canProduceEnergy // to allow energy production when there is no recipe runing or without fuel consuption
• calculateFuelConsumptionMultiplier // applied in calculateFuelAmount(FuelRecipe).
• calculateStaticEnergyEfficiency // applied once on recipe start
• calculateDynamicEnergyEfficiency // applied every tick before producing energy
• calculateRecipeOutputVoltage // by default behave just like getRecipeOutputVoltage() but will include calculateDynamicEnergyEfficiency and return effective power output.
• calculateRecipeDurationMultiplier() // applied in calculateRecipeDuration(lastRecipe)
• hasRecipeEnded // return this.recipeDurationLeft <= 0 but can be overrided to include different element.
• canConsumeFuel // used in the update method

Methods changed :

• calculateFuelAmount // included calculateFuelConsumptionMultiplier
• calculateRecipeDuration // included calculateRecipeDurationMultiplier
• startRecipe // included calculateStaticEnergyEfficiency

the update method has changed in form but not in default behavior. the test energyContainer.get().getEnergyCanBeInserted()>= calculateRecipeOutputVoltage() || shouldVoidExcessiveEnergy() has been moved to canProduceEnergy by default canConsumeFuel return canProduceEnergy leading to the same behavior concerning power production and fuel consumption as before. New recipe is searched when the current has ended as before.

Outcome:

Make FuelRecipeLogic more flexible and easier to implement custom behavior.

Additional info:

see #1604 for additional information on why and for what those changes are made

Possible compatibility issue:

Possible compatibility issues with every multiblock using the FuelRecipeLogic as workable handler or base for their workable handler. I will write reports on compatibility for each multiblock and provide as much proof has possible about the existence or not of compatibility issue. Help would be appreciated especially from add-on devs as I don't know each of them and they will know their code better than I could.

[galyfray]

concerning the Large Turbines :

startRecipe return 0 but as getRecipeOutputVoltage has been overrided power duplication is expected.

QuickFix :

• replace getRecipeOutputVoltage by this.recipeOutputVoltage in calculateRecipeOutputVoltage consistent with the expected behavior of getRecipeOutputVoltage while allowing better compatibility with all implementation like this. Fixed

Only parts concerning the output voltage have been overrided in the LargeTurbineWorkableHandler no other issues has been found.

[galyfray]

concerning the Diesel Engines:

As no changes has been done concerning the usage of overrided methods in DieselEngineWorkableHandler no issues have been found and none are expected.

[galyfray]

Concerning Large Boilers :

only use unchanged static methods, no issues have been found and none are expected.

[galyfray]

concerning the FueledMultiblockController :
getRecipeOutputVoltage() is used and should get replaced by calculateRecipeOutputVoltage soon or late to avoid showing wrong data. No changes will be done has getRecipeOutputVoltage() can be overrided. Fixed

A compatibility solution should be found to allow smooth transition.

[galyfray]

concerning the SimpleGeneratorMetaTileEntity

No issues have been found and none are expected.

[warjort]

I don't understand why you have changed everything to calculateXXX().

I suggested changing policy methods to be called calculateXXX() to differentiate them from accessors that are used to expose metrics on displays, getXXX().

[warjort]

I also don't like the hasRecipeEnded(), canProduceEnergy() and canConsumeFuel()

I know you are just trying to reproduce the old logic in a way that can be overridden, but this abstraction doesn't look very clean.

e.g. if I want to override canProduceEnergy() I also need to reproduce the energy insertion check in my subclass.
Previously this logic was dictated by the shouldVoidExcessEnergy() policy without me having to worry about the mechanics of how it was done.

You should make these policy methods "do one thing well" and not mix things together.

[galyfray]

I don't understand why you have changed everything to calculateXXX().
I suggested changing policy methods to be called calculateXXX() to differentiate them from accessors that are used to expose metrics on displays, getXXX().

I didn't correctly understand what you was meaning when saying that. For me a getXXX method is a getter which return a stored value while calculateXXX means you have calculation behind it.

can you give me a list or something more detailed on what do think should be a getXXX or a calculateXXX ?

[galyfray]

e.g. if I want to override canProduceEnergy() I also need to reproduce the energy insertion check in my subclass.
Previously this logic was dictated by the shouldVoidExcessEnergy() policy without me having to worry about the mechanics of how it was done.
You should make these policy methods "do one thing well" and not mix things together.

The fact of looking if there is room for energy is a part of the "can you produce power" for me. still I understand your point and changing this isn't a big deal for energy production but for fuel consumption it will stay not as clean as you may want.

[warjort]

The fact of looking if there is room for energy is a part of the "can you produce power" for me

But as I said, there is already a policy for that, I shouldn't have to override it again, or even worse copy/paste the implementation details to override some other policy.

If you are mixing things together or forcing people to reproduce implementation details in their override it is a sign that the abstraction is wrong and that you need some other policy override option or maybe multiple policy overrides?

[galyfray]

But as I said, there is already a policy for that, I shouldn't have to override it again, or even worse copy/paste the implementation details to override some other policy.

that's why I said I understand your point I'm working on fixing this at least for the energy part and I'm thinking of a way to do so for fuel

[galyfray]

The abstraction has been cleaned up canProduce and canConsume are bonded by shouldConsumeWhenNotProducing allowing to override this behavior easily. the voiding energy policy has been moved again in the update function making the canProduceEnergy much clearer as well as the canConsumeFuel

[warjort]

The canConsumeFuel() is confusing to me.

I think of consuming fuel as the using of fuel in the input hatch that is consumed when starting the recipe.

I think this flag would be better named as something like canProgressRecipe() since that is what it is actually controlling.

[galyfray]

The canConsumeFuel() is confusing to me.
I think of consuming fuel as the using of fuel in the input hatch that is consumed when starting the recipe.
I think this flag would be better named as something like canProgressRecipe() since that is what it is actually controlling.

I was looking for a new name for this one since a long time ...

[warjort]

The same is true for the shouldConsumeWhenNotProducing().

Is this shouldProgressRecipeWhenNotProducingEnergy()

Phew, that's a long name :-)

[galyfray]

Is this shouldProgressRecipeWhenNotProducingEnergy()

ups forget this one and I think RecipeProgress instead of ProgressRecipe makes more sense

[warjort]

You should change the canConsume local variable as well.

[Archengius]

Literally all abstractions reproduced by this PR fall into either of 3 categories:

• Duplicated functionality (mostly from other abstractions added in this PR)
• Unnecessary and bloated functionality (like dynamic + static efficiency multipliers)
• Functionality that is still not flexible enough and has hardcoded conditions (like with canConsumeFuel or canProduceEnergy)

I heavily recommend reworking it from the ground while trying to keep things as simple as possible and avoid introducing unnecessary functionality

[warjort]

You have changed the logic for wasActiveAndNeedsUpdate.

Previously this was only set to true when the recipeDurationLeft was changed to zero.
Now it is always being set to true if the recipeDurationLeft has the value zero

I am unsure what this would change but you should try to keep the old logic to avoid introducing subtle bugs.

[galyfray]

As pointed by @Archengius getter should reworked as they can send value different from what was used in the computation.
quick fix : store those values in protected field when computed. there is probably no need to store those new value in the NBT.

[serenibyss]

I will say that I have not fully reviewed this PR, nor the original one focused on Turbines specifically. However, to me it seems very overcomplicated, and as someone who has not read all of the discussion as it is quite long, and is simply looking at the list of API method changes, I find this less productive than before, simply because of how complicated it is. Personally, I think that Archengius’s suggestion here makes the most sense to me as the path forward.

[warjort]

Personally, I think that Archengius’s suggestion here makes the most sense to me as the path forward.

I am reserving judgement until I see how the implementation of the throttle now works.

Without the multiplier injection exposed explicitly, I can see it being harder to implement a generic throttle that can be shared across generator implementations including those that have their own duration, consumption and energy modifications like the diesel generator?

This was kind of the first step so the throttle can reason about (and affect) all the generators in a generic way.

I think the main reason for adding the other methods was to try to integrate the turbine behaviour back into FuelRecipeLogic so it didn't have to "corrupt" the contract of quite a few important methods, again allowing the throttle to reason consistently.

It seems to me that with these more broad brush overrides we will again see kludges but now it will be in the new methods? :-)

[warjort]

The way I envisioned this feature working was that you;

• have a "widget" that you can add to your multiblock display or single block generator that implements a generic throttle
• You can initialise the throttle with tunable parameters/functions to dictate the efficiency curve(s)
• You wire up your workable handler to the throttle, e.g. the multiplier overrides are just delegated to the throttle

That's it. The generators don't need invasive changes into their energy production logic.
The throttle works with the FuelRecipeLogic as a kind of back channel.

I did suggest that for the multiblocks the throttle could be a multiblock part with its own multiblock ability.
That would make the integration pretty much automatic for all MB generators just by changing the pattern to allow it.
But that has issues with how to inject the GUI unless it is actually in the multiblock part and not the controller.

You could do the same for single blocks if the throttle was a cover.

[galyfray]

You wire up your workable handler to the throttle, e.g. the multiplier overrides are just delegated to the throttle
you want the throttle widget to override the multiplier by itself ?

[warjort]

you want the throttle widget to override the multiplier by itself ?

Is there a reason why it can't?

It's not really by itself if you allow the calculation to be configurable on the throttle.
Though I would expect the throttle to have a default implementation.

And if you really needed to modify the behaviour in a gross way you can always do:

protected double calculateMultiplier() {
    double throttleResult = throttle.calculateMultiplier();
    return doWhateverYouWant(throttleResult);
}

But @Archengius said he doesn't like this multiplier policy being seperated so these things are more difficult to achieve.

[galyfray]

@Archengius @warjort I applied some changes I'm not sure about the utility of some getters like the hasRecipeProgressed one
can you give me feedback ?

[galyfray]

I changed the canProduceEnergy in a way that allow full customization and doesn't require to redo implementation detail. If this way satisfy anyone I will change the canRecipeProgress to behave this way too.

[warjort]

I changed the canProduceEnergy in a way that allow full customization and doesn't require to redo implementation detail.

I don't think your compromise will satisfy either @Archengius or myself. :-)

@Archengius doesn't want a proliferation of methods in the interface that are just there on the slight chance that they might be useful, something I agree with.

While I want a proper "separation of concerns" - the "do one thing well" principle.

Let me give a concrete example loosly based on the code you are modifying.

public boolean FuelRecipeLogic.canProduceEnergy() {
    return energyContainerCheck || voidable();
}
@Override
public boolean RotorLogic.canProduceEnergy() {
   return super.canProduceEnergy() && rotorLogic
}

Scenario 1: I want to override the rotor logic in the turbine so I have to reproduce the implementation detail:

@Override
public boolean TurbineLogic.canProduceEnergy() {
    // Notice I can't use super
    return (energyContainerCheck || voidable()) && changedRotorLogic;
}

Scenario 2: Somebody now wants a new check to FuelRecipeLogic

public boolean FuelRecipeLogic.canProduceEnergy() {
    return energyContainerCheck || voidable() || newLogic;
}

I now have to know the turbine logic needs changing as well (assuming it is not in an addon project)

My argument is that the stuff I wrote in FuelRecipeLogic.canProduceEnergy() above doesn't even need to overridable. There is already the well defined voidable() api to change the behaviour.
The override should be tailored to the specific behaviour.

boolean canProduce = canProduceEnergy() && (energyContainerCheck || voidable())
protected boolean FuelRecipeLogic.canProduceEnergy() {
    return true;
}
@Override
protected boolean RotorLogic.canProduceEnergy() {
   return rotorLogic();
}
protected boolean RotorLogic.rotorLogic() {
   // whatever
}
@Override
protected boolean TurbineLogic.rotorLogic() {
   // Override what you want to change rather than mixing different concerns
}

Notice in my version I don't need to call super or copy/paste code. And it is easier to reason about what is happening.
And I can add newLogic with no problems without having to worry that somebody might not be invoking super.

boolean canProduce = canProduceEnergy() && (energyContainerCheck || voidable() || newLogic)

[galyfray]

my actual version giving (energyContainerCheck || voidable()) as a parameter for canProduceEnergy is resilient to the addition of new behavior like the newLogic thing, if the main problem is the addition of a new method I can remove it and simply put the expression directly into the call as a parameter reducing useless methods while keeping a proper "separation of concerns" as there will be just a float to take care does this way feels good for you ? or should we call a third person to decide which way will be used ?

[warjort]

or should we call a third person to decide which way will be used

@Archengius has review authority, I don't

[warjort]

if the main problem is the addition of a new method I can remove it and simply put the expression directly into the call as a parameter reducing useless methods while keeping a proper "separation of concerns"

I don't think it is a real "separation of concerns" because in that newLogic example you are going to have to || it with
hasRoomForEnergy. But that newLogic might not have anything to do with the hasRoomForEnergy state.

public boolean canProduceEnergy(boolean hasRoomForEnergy) {
       return !hasRecipeEnded() && hasRoomForEnergy;
}

[galyfray]

@Archengius is this okay now ?

[galyfray]

I won't work on this anymore, I don't even remember what I was doing in this PR, so I close it