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

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

Merged
merged 28 commits into from
May 18, 2024

Conversation

MaxNeedsSnacks
Copy link
Contributor

@MaxNeedsSnacks MaxNeedsSnacks commented Apr 8, 2024

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.

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2024

CLA assistant check
All committers have signed the CLA.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now this PR inherits some questionable decisions from Ingredient. We should first revisit our Ingredient patches before we merge this PR.

@KnightMiner
Copy link

KnightMiner commented Apr 9, 2024

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)

@MaxNeedsSnacks
Copy link
Contributor Author

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
Copy link
Contributor Author

MaxNeedsSnacks commented Apr 9, 2024

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 ^^;

@KnightMiner
Copy link

KnightMiner commented Apr 9, 2024

  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.

@Shadows-of-Fire Shadows-of-Fire added enhancement New (or improvement to existing) feature or request 1.20.5 Targeted at Minecraft 1.20.5 labels Apr 15, 2024
@MaxNeedsSnacks
Copy link
Contributor Author

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 MaxNeedsSnacks changed the base branch from port/24w14a to 1.20.x April 28, 2024 11:56
@MaxNeedsSnacks
Copy link
Contributor Author

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
Copy link
Contributor Author

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 MaxNeedsSnacks marked this pull request as ready for review April 29, 2024 00:21
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice. Just a few comments.

This reverts commit 589fec3

Reason: Since FluidStacks similar to their item counterparts may contain default components later down the line, simply checking if the component patch matches the patch on the stack is not enough for strict component ingredients
Technici4n
Technici4n previously approved these changes May 15, 2024
@Technici4n Technici4n added the last call Planned to be resolved by the end of the week, awaiting any last-minute comments label May 15, 2024
pupnewfster
pupnewfster previously approved these changes May 16, 2024
pupnewfster
pupnewfster previously approved these changes May 16, 2024
@pupnewfster pupnewfster merged commit 66197fe into neoforged:1.20.x May 18, 2024
4 checks passed
CodexAdrian added a commit to CodexAdrian/NeoForge that referenced this pull request May 21, 2024
commit 8ac9f9c
Author: Minecraftschurli <minecraftschurli@gmail.com>
Date:   Tue May 21 15:25:59 2024 +0200

    [Testframework] Fix wrapping GameTestAssertException (neoforged#988)

commit 66197fe
Author: Max <max@someone.ky>
Date:   Sat May 18 20:11:00 2024 +0200

    Add a Fluid Ingredient system as an analogue to vanilla's Ingredient (neoforged#789)

commit b1c3fe0
Author: NeoForged Localizations <machine-l10n@neoforged.net>
Date:   Sun May 19 00:43:32 2024 +0800

    New Crowdin updates (neoforged#972)

commit a7edbae
Author: Sara Freimer <sara@freimer.com>
Date:   Fri May 17 01:32:34 2024 -0500

    Implement equals and hashCode for SizedIngredients (neoforged#965)

commit e9e8af0
Author: Dennis C <xfacthd@gmx.de>
Date:   Fri May 17 08:31:55 2024 +0200

    [1.20.6] Use extensible enum codec and streamcodec for rarity enum (neoforged#958)

commit 986e0e3
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Fri May 17 02:28:12 2024 -0400

    Patch in missing LivingJumpEvent for Slime and Camel (neoforged#966)

commit 2bf3073
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Fri May 17 02:26:49 2024 -0400

    Pass growing plant blockstate into onCropsGrowPre for various vine blocks (neoforged#967)

commit 0033435
Author: embeddedt <42941056+embeddedt@users.noreply.github.com>
Date:   Fri May 17 02:25:24 2024 -0400

    Fix ComputeCameraAngles event applying roll in global space (neoforged#968)

commit bb3537b
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Wed May 15 23:02:02 2024 -0400

    [no ci] Remove PitcherCrop patch TODO (neoforged#963)

commit d80f154
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Wed May 15 23:01:36 2024 -0400

    [no ci] Fixed Bubble Column gametest template size (neoforged#964)

commit c888a68
Author: Luke Bemish <lukebemish@lukebemish.dev>
Date:   Wed May 15 01:29:03 2024 -0500

    [1.20.6] Add KnownPacks for mods (neoforged#901)

commit a28986f
Author: shartte <shartte@users.noreply.github.com>
Date:   Mon May 13 09:13:19 2024 +0200

    Update FML to 3.0.29 (neoforged#957)

commit 2aadb61
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Sat May 11 14:11:06 2024 -0400

    Make StructureTemplate's Palette caches thread safe (neoforged#954)

commit 2bced72
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Sat May 11 13:49:18 2024 -0400

    Address POI memory leak and Generate Command issues (neoforged#937)

    Co-authored-by: embeddedt <42941056+embeddedt@users.noreply.github.com>

commit abc54b7
Author: Sirttas <Sirttas@users.noreply.github.com>
Date:   Sat May 11 19:48:09 2024 +0200

    [Test Framework] Add a check in `StructureTemplateBuilder#set` to prevent placing block outside template bondaries (neoforged#950)

commit 2d93ce5
Author: embeddedt <42941056+embeddedt@users.noreply.github.com>
Date:   Sat May 11 01:28:34 2024 -0400

    Delete ItemModelShaper patch (neoforged#952)

commit 018e104
Author: embeddedt <42941056+embeddedt@users.noreply.github.com>
Date:   Fri May 10 19:58:05 2024 -0400

    Fix more particle culling issues (neoforged#951)

commit 03e346b
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Fri May 10 12:14:37 2024 -0400

    Fix MC-268617 to allow valid filepaths in Minecraft (neoforged#767)

commit fe57823
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Fri May 10 12:02:54 2024 -0400

    Fire Villager/Wandering Trader trade events on /reload (neoforged#922)

commit d0ed10a
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Fri May 10 12:02:04 2024 -0400

    Remove Unnecessary Stitcher patch (neoforged#948)

commit a598489
Author: Matyrobbrt <65940752+Matyrobbrt@users.noreply.github.com>
Date:   Fri May 10 14:44:48 2024 +0300

    Bump Mixin to 0.13.4 to fix local variable issues (neoforged#945)

    Fixes neoforged#768

commit 212f760
Author: sciwhiz12 <arnoldnunag12@gmail.com>
Date:   Fri May 10 10:29:47 2024 +0800

    Fix dedicated server GUI never showing up (neoforged#946)

commit 238a273
Author: embeddedt <42941056+embeddedt@users.noreply.github.com>
Date:   Thu May 9 17:07:08 2024 -0400

    Add more accurate culling for single quad particles (neoforged#885)

commit 27c5d7a
Author: Brennan Ward <Bward7864@gmail.com>
Date:   Thu May 9 10:50:55 2024 -0700

    Fix FinalizeSpawnEvent for trial spawners and fix SpawnGroupData propagation (neoforged#880)

    Closes neoforged#783
    Closes neoforged#939

commit 989ed3b
Author: TelepathicGrunt <40846040+TelepathicGrunt@users.noreply.github.com>
Date:   Thu May 9 13:40:34 2024 -0400

    [1.20.6] Fix several NeoForge command crashes and allow chat OpenFile click action on Integrated Server  (neoforged#915)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20.5 Targeted at Minecraft 1.20.5 enhancement New (or improvement to existing) feature or request last call Planned to be resolved by the end of the week, awaiting any last-minute comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants