-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
[1.20.5] Add a Fluid Ingredient system as an analogue to vanilla's Ingredient #789
Conversation
|
There was a problem hiding this 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.
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) |
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:
I would much rather just provide a |
As for the "questionable design decisions" inherited from vanilla, which I ultimately interpret as the existence of the |
In Tinkers' we always did a 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.
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. 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.
Its possible. Whether its useful for fluids is a different question. Key thing is
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. Should be trivial to implement overall, just make sure amount is required in the JSON, and add a size specific |
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 |
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:
|
Decided to get rid of |
…tack (idk why I copied Vanilla's bad design there)
Got to a point where I am content with the basic structure, documentation and game tests, so this should be ready for review now! |
src/main/java/net/neoforged/neoforge/fluids/crafting/SingleFluidIngredient.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/fluids/crafting/DifferenceFluidIngredient.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/fluids/crafting/IntersectionFluidIngredient.java
Outdated
Show resolved
Hide resolved
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
src/main/java/net/neoforged/neoforge/fluids/crafting/SizedFluidIngredient.java
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/fluids/crafting/SizedFluidIngredient.java
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/fluids/crafting/FluidIngredient.java
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/fluids/crafting/FluidIngredient.java
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/fluids/crafting/IntersectionFluidIngredient.java
Outdated
Show resolved
Hide resolved
commit 8ac9f9c Author: Minecraftschurli <[email protected]> Date: Tue May 21 15:25:59 2024 +0200 [Testframework] Fix wrapping GameTestAssertException (neoforged#988) commit 66197fe Author: Max <[email protected]> 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 <[email protected]> Date: Sun May 19 00:43:32 2024 +0800 New Crowdin updates (neoforged#972) commit a7edbae Author: Sara Freimer <[email protected]> Date: Fri May 17 01:32:34 2024 -0500 Implement equals and hashCode for SizedIngredients (neoforged#965) commit e9e8af0 Author: Dennis C <[email protected]> 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 <[email protected]> Date: Fri May 17 02:28:12 2024 -0400 Patch in missing LivingJumpEvent for Slime and Camel (neoforged#966) commit 2bf3073 Author: TelepathicGrunt <[email protected]> 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 <[email protected]> Date: Fri May 17 02:25:24 2024 -0400 Fix ComputeCameraAngles event applying roll in global space (neoforged#968) commit bb3537b Author: TelepathicGrunt <[email protected]> Date: Wed May 15 23:02:02 2024 -0400 [no ci] Remove PitcherCrop patch TODO (neoforged#963) commit d80f154 Author: TelepathicGrunt <[email protected]> Date: Wed May 15 23:01:36 2024 -0400 [no ci] Fixed Bubble Column gametest template size (neoforged#964) commit c888a68 Author: Luke Bemish <[email protected]> Date: Wed May 15 01:29:03 2024 -0500 [1.20.6] Add KnownPacks for mods (neoforged#901) commit a28986f Author: shartte <[email protected]> Date: Mon May 13 09:13:19 2024 +0200 Update FML to 3.0.29 (neoforged#957) commit 2aadb61 Author: TelepathicGrunt <[email protected]> Date: Sat May 11 14:11:06 2024 -0400 Make StructureTemplate's Palette caches thread safe (neoforged#954) commit 2bced72 Author: TelepathicGrunt <[email protected]> Date: Sat May 11 13:49:18 2024 -0400 Address POI memory leak and Generate Command issues (neoforged#937) Co-authored-by: embeddedt <[email protected]> commit abc54b7 Author: Sirttas <[email protected]> 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 <[email protected]> Date: Sat May 11 01:28:34 2024 -0400 Delete ItemModelShaper patch (neoforged#952) commit 018e104 Author: embeddedt <[email protected]> Date: Fri May 10 19:58:05 2024 -0400 Fix more particle culling issues (neoforged#951) commit 03e346b Author: TelepathicGrunt <[email protected]> Date: Fri May 10 12:14:37 2024 -0400 Fix MC-268617 to allow valid filepaths in Minecraft (neoforged#767) commit fe57823 Author: TelepathicGrunt <[email protected]> Date: Fri May 10 12:02:54 2024 -0400 Fire Villager/Wandering Trader trade events on /reload (neoforged#922) commit d0ed10a Author: TelepathicGrunt <[email protected]> Date: Fri May 10 12:02:04 2024 -0400 Remove Unnecessary Stitcher patch (neoforged#948) commit a598489 Author: Matyrobbrt <[email protected]> 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 <[email protected]> Date: Fri May 10 10:29:47 2024 +0800 Fix dedicated server GUI never showing up (neoforged#946) commit 238a273 Author: embeddedt <[email protected]> Date: Thu May 9 17:07:08 2024 -0400 Add more accurate culling for single quad particles (neoforged#885) commit 27c5d7a Author: Brennan Ward <[email protected]> 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 <[email protected]> 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)
commit 8ac9f9c Author: Minecraftschurli <[email protected]> Date: Tue May 21 15:25:59 2024 +0200 [Testframework] Fix wrapping GameTestAssertException (neoforged#988) commit 66197fe Author: Max <[email protected]> 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 <[email protected]> Date: Sun May 19 00:43:32 2024 +0800 New Crowdin updates (neoforged#972) commit a7edbae Author: Sara Freimer <[email protected]> Date: Fri May 17 01:32:34 2024 -0500 Implement equals and hashCode for SizedIngredients (neoforged#965) commit e9e8af0 Author: Dennis C <[email protected]> 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 <[email protected]> Date: Fri May 17 02:28:12 2024 -0400 Patch in missing LivingJumpEvent for Slime and Camel (neoforged#966) commit 2bf3073 Author: TelepathicGrunt <[email protected]> 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 <[email protected]> Date: Fri May 17 02:25:24 2024 -0400 Fix ComputeCameraAngles event applying roll in global space (neoforged#968) commit bb3537b Author: TelepathicGrunt <[email protected]> Date: Wed May 15 23:02:02 2024 -0400 [no ci] Remove PitcherCrop patch TODO (neoforged#963) commit d80f154 Author: TelepathicGrunt <[email protected]> Date: Wed May 15 23:01:36 2024 -0400 [no ci] Fixed Bubble Column gametest template size (neoforged#964) commit c888a68 Author: Luke Bemish <[email protected]> Date: Wed May 15 01:29:03 2024 -0500 [1.20.6] Add KnownPacks for mods (neoforged#901) commit a28986f Author: shartte <[email protected]> Date: Mon May 13 09:13:19 2024 +0200 Update FML to 3.0.29 (neoforged#957) commit 2aadb61 Author: TelepathicGrunt <[email protected]> Date: Sat May 11 14:11:06 2024 -0400 Make StructureTemplate's Palette caches thread safe (neoforged#954) commit 2bced72 Author: TelepathicGrunt <[email protected]> Date: Sat May 11 13:49:18 2024 -0400 Address POI memory leak and Generate Command issues (neoforged#937) Co-authored-by: embeddedt <[email protected]> commit abc54b7 Author: Sirttas <[email protected]> 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 <[email protected]> Date: Sat May 11 01:28:34 2024 -0400 Delete ItemModelShaper patch (neoforged#952) commit 018e104 Author: embeddedt <[email protected]> Date: Fri May 10 19:58:05 2024 -0400 Fix more particle culling issues (neoforged#951) commit 03e346b Author: TelepathicGrunt <[email protected]> Date: Fri May 10 12:14:37 2024 -0400 Fix MC-268617 to allow valid filepaths in Minecraft (neoforged#767) commit fe57823 Author: TelepathicGrunt <[email protected]> Date: Fri May 10 12:02:54 2024 -0400 Fire Villager/Wandering Trader trade events on /reload (neoforged#922) commit d0ed10a Author: TelepathicGrunt <[email protected]> Date: Fri May 10 12:02:04 2024 -0400 Remove Unnecessary Stitcher patch (neoforged#948) commit a598489 Author: Matyrobbrt <[email protected]> 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 <[email protected]> Date: Fri May 10 10:29:47 2024 +0800 Fix dedicated server GUI never showing up (neoforged#946) commit 238a273 Author: embeddedt <[email protected]> Date: Thu May 9 17:07:08 2024 -0400 Add more accurate culling for single quad particles (neoforged#885) commit 27c5d7a Author: Brennan Ward <[email protected]> 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 <[email protected]> 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)
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
andFluidIngredientType
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 mainFluidIngredient
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 notValue
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
Ingredient
s, 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.