NeoForge icon indicating copy to clipboard operation
NeoForge copied to clipboard

[1.20.5] Add a Fluid Ingredient system as an analogue to vanilla's Ingredient

Open MaxNeedsSnacks opened this issue 3 months ago • 10 comments

Motivation

Vanilla's Ingredients along with Neo's custom ingredient types provide a robust and well-established system for matching predicates over items (mostly) within the context of recipes. This PR aims to build a similar system for another common component within recipes that Vanilla unfortunately does not already offer us a baseline for: Fluids.

As it stands right now, any mods with fluid inputs that can match more than a single fluid stack need to provide their own implementation of a "fluid ingredient", which has led to some interesting and sometimes peculiar design choices, some of which I will address here as well. Additionally, mods like KubeJS or CraftTweaker don't currently have a unified way to represent a "fluid input" in recipes due to that disparity (my primary motivation for this actually initially came from the frustration of having to deal with fluid ingredients in some mods), and as such, adding a system like this would greatly benefit us as well as we could do away with our common abstractions over these various systems.

Implementation

FluidIngredient / FluidIngredientType

The current implementation of FluidIngredient and FluidIngredientType follows the current item ingredient system pretty closely. This has been decided on after some preliminary discussions about this PR were done a couple months ago (albeit in Forgecraft), as nobody involved saw a reason to depart from Vanilla's system all too far.

There are however some slight changes; for instance, with Neo's custom ingredients having been reworked in #793, a value class is no longer necessary, so I opted to redesign the system to be based on a single abstract class, FluidIngredient, with all types of fluid ingredients (including single, tag and empty) being subclasses of that.

Size-sensitive matching

As previously mentioned, this PR is based on the Vanilla Ingredient system, and as such, the main FluidIngredient does not do size-sensitive matching and instantiates all display stacks with a size of 1000mB (this has been a minor point of contention in the past because there technically is no "default" fluid amount, but this felt the most fitting out of the available options).

If you want size-sensitive matching and sized display stacks, you may use the SIzedFluidIngredient class, which is a wrapper around (ingredient, count) and serves as a standard / "reference" implementation for fluid ingredient stacks.

The Value class

~~I don't have particularly strong thoughts about whether or not Value is a worthwhile class to "copy" for our fluid ingredients, but I remember having been advised to keep it around in this implementation, though I can't remember the exact reason...~~

see above, Value is 🦀🦀🦀

Tests

So far, this is the biggest question mark I have for this entire system. Obviously, I would like to implement some proper game tests before getting this anywhere close to being merged, though this time, unlike with Ingredients, there is nothing comparable to a "crafter" that we can rely on functioning properly, so the test harness would have to be... rather expansive if we actually wanted to go with proper "in-world" / observable game tests. I would love to hear feedback on how you think I should be going about creating tests for this system, perhaps even simple unit tests would suffice though.


For the time being, this should hopefully sum up part of the "why" and "how" of this pull request, I'd be interested in further discussion since I have no doubt missed something important in my outline so far that I am currently just tunnel visioning, and I'm going to be involved in discussions in the #neoforge-github channel on Discord regarding this PR (as well as the brainstorming channel, whose existence i JUST learned about today lol), so feel free to just ping me there with any thoughts you may have on the matter, as well.

And yeah, I know there are some changes / cleanup that IDEA was... a little too eager to apply, so I'll have to undo those manually, that shouldn't take too long though.

MaxNeedsSnacks avatar Apr 08 '24 11:04 MaxNeedsSnacks

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 08 '24 11:04 CLAassistant

  • [ ] Publish PR to GitHub Packages

I'd make the argument that if this ingredient does not handle size, its entirely missing the point of fluid ingredients and thus will not get common usage. Its pretty trivial to toss a simple size parameter inside the ingredient and make sure it serializes/deserializes. Either do the general "argument must be greater than or equal to size to match" or make the logic for validating the size up to the caller.

Not saying there is no usecase for sizeless ingredients, but its not hard to have two matches methods: one that matches fluid alone, and one that matches fluid and size (make it final and delegate to the other)

KnightMiner avatar Apr 09 '24 01:04 KnightMiner

I understand that fluid ingredients in practice will mostly be used as part of ingredient stacks, much as item ingredients already are, however, I don't think just mixing matching and amounts is a good idea, especially since it isn't just as simple as "just addding a size field". For example, these questions all still remain unanswered:

  1. When using a fluid stack as input, do you just take its amount as the required size for the ingredient, or do you still require the amount to be specified explicitly?
  2. What happens when people are using an array of fluid stacks, which amount do you pick then?
  3. Related to the above, is mixing and matching different amounts possible? (e.g. Mekanism does this for their ingredient stacks, allowing either 3 apples or 4 carrots for instance)
  4. When using more complex ingredients (for example difference ingredients etc.), if the size is declared on the ingredient, do we have to just use some weird dummy size on the component ingredients? That seems a bit redundant to me honestly...

I would much rather just provide a MapCodec for both fluid and item ingredients s.t. developers (including possibly Neo) can sensibly combine it with a count field at the top level without having to nest the ingredient

MaxNeedsSnacks avatar Apr 09 '24 13:04 MaxNeedsSnacks

As for the "questionable design decisions" inherited from vanilla, which I ultimately interpret as the existence of the Value class (and the subclasses added in custom ingredients), I would agree that that is a blocker on this PR, iff there are plans to move away from Value for (custom) item ingredients in Neo itself. I myself don't really... care about whether that class survives or not in the end, it should be easy enough to patch out if need be, but I would appreciate some clarification perhaps on why Value classes are even used at all for custom item ingredients, mainly since it would help me understand a little better if that system is worth expanding to fluids or not ^^;

MaxNeedsSnacks avatar Apr 09 '24 13:04 MaxNeedsSnacks

  1. When using a fluid stack as input, do you just take its amount as the required size for the ingredient, or do you still require the amount to be specified explicitly?

In Tinkers' we always did a >= match, that is the ingredient matches if the input is larger than or equal to the ingredient. This is consistent with how most sized ingredients work so is probably the ideal. However, if someone has another approach you could leave it generic, pass in something like

public interface SizeValidator {
  boolean matches(int ingredientSize, int inputSize);
}

I cannot think of any usecases of other sizes as practically no one is making milibucket specific recipes, but who knows.

  1. What happens when people are using an array of fluid stacks, which amount do you pick then?

You pick the amount that matches the fluid. There are two ways you might use the ingredient to accomplish this:

a. You are matching the ingredient on the stack itself, just iterate the list and return true if any matches. b. You are matching the ingredient on the fluid, then wish to report the size needed for that fluid later. For this you just need a method on your fluid ingredient FluidIngredient#getAmount(FluidStack)

Note that I don't believe that each fluid matching a different size is strictly required for this PR, but if you follow this approach its not hard to support.

  1. Related to the above, is mixing and matching different amounts possible? (e.g. Mekanism does this for their ingredient stacks, allowing either 3 apples or 4 carrots for instance)

Its possible. Whether its useful for fluids is a different question. Key thing is FluidIngredient#test does not really care what the size is, as long as you provide a FluidIngredient#getAmount(FluidStack) so people know how much to shrink the fluid by.

  1. When using more complex ingredients (for example difference ingredients etc.), if the size is declared on the ingredient, do we have to just use some weird dummy size on the component ingredients? That seems a bit redundant to me honestly...

I would much rather just provide a MapCodec for both fluid and item ingredients s.t. developers (including possibly Neo) can sensibly combine it with a count field at the top level without having to nest the ingredient

Difference ingredients is a fair point, while it would be trivial to handle (just "subtract" if the size matches), it would be a lot more intuitive if you subtract size free fluids.

You could do a simple nesting approach if you think that would be easier, e.g. record SizedFluidIngredient(FluidIngredient ingredient, int amount), important thing is that needs to be functionality provided in this PR both so we have a standard for how its serialized and so this PR can actually replace the functionality used by modders (as I doubt many mods are doing size insensitive fluid ingredients). This approach would also deal with the "weird size matchers" case as if they want to match on amount differently, they just use a different object than the forge one.

Should be trivial to implement overall, just make sure amount is required in the JSON, and add a size specific List<FluidStack> getDisplayFluids() in place of the size generic one FluidIngredient provides in this case. The only downside to this is you would lose fluid specific sizes, though not sure that is essential functionality.

KnightMiner avatar Apr 09 '24 18:04 KnightMiner

Putting a potential rework of this on hold for a bit, probably until #793 is merged since I'd love to base this API on that

MaxNeedsSnacks avatar Apr 19 '24 22:04 MaxNeedsSnacks

There we go; I hate to force push things, but unfortunately there were just... a lot of conflicts that prevented me from merging into my old PR cleanly, especially since I essentially just redid the whole thing from the ground up now?

This should be closer to something mergeable now, though I still have some open questions I would like to address, namely:

  1. How should isEmpty() be used? Should it be like we have with Vanilla ingredients, where only explicitly empty ingredients return isEmpty == true, or should it be a check for accidentally empty ingredients as well?
  2. In case of the latter, some ingredients may have to resolve in order to return a definitive answer as to whether they are empty. I think this is likely to be undesirable behaviour, I have kept it in for now, but I think it should be removed or at least changed.
  3. Should SingleFluidIngredient throw an error upon being constructed with an empty stack?
  4. Should we force FluidIngredient instances to implement equals and hashCode? It would definitely be useful to have for mods such as AE2, though I'm wondering whether there are some types of fluid ingredients where forcing this upon them would be a bad idea...
  5. How would we go about designing tests for FluidIngredients? I can come up with serialisation tests and simple functionality checks no problem, but for an actual GameTest, we would have to introduce a lot of mechanics to the framework, since FluidIngredient does not find any use in vanilla anywhere...

MaxNeedsSnacks avatar Apr 28 '24 12:04 MaxNeedsSnacks

Decided to get rid of isEmpty in its current form after all, I will likely be adding a hasNoFluids method after all as that makes it more clear that the ingredient may be resolved during the check.

MaxNeedsSnacks avatar Apr 28 '24 16:04 MaxNeedsSnacks

Got to a point where I am content with the basic structure, documentation and game tests, so this should be ready for review now!

MaxNeedsSnacks avatar Apr 29 '24 00:04 MaxNeedsSnacks