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

Rework OverlayedFluidHandler to fix Fluid Parallel Limiting #2423

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

krossgg
Copy link
Contributor

@krossgg krossgg commented Nov 23, 2024

What

This PR reworks the OverlayedFluidHandler, which was naively copied from 1.12, to work with Modern's Fluid Storage systems. The reason is that Modern does not wrap all of a machine's fluid tanks into a single object used for recipe handling, unlike 1.12's IMultipleTankHandler/FluidTankList. Instead, we have separate instances of NotifiableFluidTanks (henceforth Noti Tank) attached to a machine that are stored in the capabilities proxy.

Previously, by wrapping all the Noti Tanks in a FluidHandlerList the allowSameFluidFill property of the Noti Tank was ignored, leading to instances where FluidRecipeCapability#limitParallel would not properly limit the parallel amount and return an incorrect parallel amount, usually skewed towards the higher end. Consequently, the recipe logic would be provided an incorrectly modified recipe and would not run, even if the same recipe as a lower parallel amount would work.

This issue has come about due to the reworking of Noti Tanks to actually respect allowSameFluidFill during recipe handling in #1975.

Here is an example of this issue:
I have a Large Electrolysis Chamber with its parallel hatch set to 64. It is electrolyzing Ammonium Formate (64B), which decomposes into 5B x H, 1B x N, and 2B x O.
My output space is a Quadruple EV Output Hatch, which has 4 tanks at 32B each. (There is plenty of output space for the item output)
image
image image

Theoretically, the multiblock should be able to handle 6 recipes, as then the output slot for Hydrogen would cap out at 30/32B.
In actuality, the FluidRecipeCapability will calculate it to handle 12 recipes, as it thinks that the 4th slot can also be filled by Hydrogen.
image

This then gets passed back to the modifier, which will multiply the recipe by 12. However, when that modified recipe is matched against the multiblock, it fails, and no recipe is run.

Implementation Details

The new class has been called the OverlayedTankHandler, and the addFluidsToFluidHandler method (which was never used anywhere) has been deleted since it relied on the old OverlayedFluidHandler.
The nested class OverlayedTank now represents a single Noti Tank and will keep track of what fluids have been filled into it. It also remembers certain properties of the Noti Tank.

Additional Details

Note that NotifiableFluidTanks (or subclasses) are the only IRecipeHandler type that we use for fluid output in multiblocks. Assuming it stays that way (which it should), this solution should be robust enough for future features.

@krossgg krossgg added the type: refactor Suggestion to refactor a section of code label Nov 23, 2024
@krossgg krossgg requested a review from a team as a code owner November 23, 2024 07:18
@Synthitic
Copy link

LGTM 👍

@screret
Copy link
Member

screret commented Nov 23, 2024

Note that NotifiableFluidTanks (or subclasses) are the only IRecipeHandler type that we use for fluid output in multiblocks. Assuming it stays that way (which it should), this solution should be robust enough for future features.

someone might do something different,,,

@krossgg
Copy link
Contributor Author

krossgg commented Nov 23, 2024

Ok well, we can let this be a bandaid until recipe handling is rewritten and I can separate the actual tank part from the recipehandler part, if we want. Right now they are completely coupled and will take significant effort to decouple.

@Spicierspace153
Copy link
Contributor

lgtm!

@YoungOnionMC YoungOnionMC merged commit 098bedb into 1.20.1 Nov 25, 2024
4 checks passed
@YoungOnionMC YoungOnionMC deleted the kr/overlayed-tank branch November 25, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactor Suggestion to refactor a section of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants