Fluid Unit system #1264

Random832 · 2024-07-09T00:09:53Z

This implements the system described at https://gist.github.com/KnightMiner/ec4f21d56d466e32ac5d14a1b1124923, with a handful of extra features [a default display unit can be specified for contexts where displaying a quantity as a single number would be useful, and when a fractional quantity of the smallest unit is present it is displayed with a configurable number of decimal places]

As is, it is completely untested (except that it compiles and datagen runs), I was told posting it as a PR would be a useful step to give other people the opportunity to test with it and otherwise evaluate it.

neoforged-pr-publishing · 2024-07-09T00:10:06Z

Publish PR to GitHub Packages

Last commit published: 6e4763ad1ca8d7d989ec7325cd3c715ab9dc6f9d.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1264' // https://github.com/neoforged/NeoForge/pull/1264
        url 'https://prmaven.neoforged.net/NeoForge/pr1264'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1264.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1264
cd NeoForge-pr1264
curl -L https://prmaven.neoforged.net/NeoForge/pr1264/net/neoforged/neoforge/21.0.97-beta-pr-1264-fluid-units/mdk-pr1264.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

HenryLoenwind · 2024-07-09T04:05:46Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnitSpec.java

+            if (i < listForTooltip.size() - 1 || amount == 0) {
+                if (amountUnit != 0)
+                    results.add(Component.translatable("fluid_unit.number_and_unit_format", // "%s %s"
+                            amountUnit, unit.getDescriptionId(amountUnit != 1)));


There are plenty of languages for which this way of pluralising is wrong.

com.ibm.icu.text.PluralFormat is available.

If/when #1269 gets merged, you can simplify this.

"fluid_unit.c.block": "%n(one{# Block} other{# Blocks})",

and then simply results.add(Component.translatable(unit.getDescriptionId(), amountUnit)).

BTW, consider using getTranslationKey(), as that's what's used in (at least some) other places.

getDescriptionId is the MojMap name for that method in vanilla classes. IIRC getTranslationKey was the MCP name before we moved to mojmaps, and not every area got switched over for vanilla consistency.

src/main/java/net/neoforged/neoforge/fluids/FluidUnit.java

IchHabeHunger54 · 2024-07-09T14:10:04Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnit.java

+import net.minecraft.network.codec.StreamCodec;
+import net.minecraft.resources.ResourceLocation;
+
+public class FluidUnit {


Please document this class and all public methods in it.

IchHabeHunger54 · 2024-07-09T14:10:18Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnitSpec.java

+import net.minecraft.util.Mth;
+import org.jetbrains.annotations.Nullable;
+
+public class FluidUnitSpec {


Please document this class and all public methods in it.

IchHabeHunger54 · 2024-07-09T14:10:46Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnitSpec.java

+    private final FluidUnit defaultUnit;
+    private final List<FluidUnit> listForTooltip;
+
+    public static final FluidUnitSpec DEFAULT = new FluidUnitSpec(


What does this default value represent? It's not immediately clear from looking at it.

Looks to me like its the fallback if units are not defined for a fluid, which just displays in buckets with 3 decimal points below buckets. So you get values like 1.345 buckets.

IchHabeHunger54 · 2024-07-09T14:11:54Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnits.java

+    public static final FluidUnit NUGGET = c("nugget");
+    public static final FluidUnit GEM = c("gem");
+
+    public static final FluidUnit SLIMEBALL = c("slimeball");


Would use "slime_ball" instead of "slimeball" here, to match the item id of the vanilla slime ball.

KnightMiner · 2024-07-09T15:21:17Z

src/main/java/net/neoforged/neoforge/common/data/internal/NeoForgeLanguageProvider.java

@@ -445,6 +464,11 @@ private <T> void addColored(TagKey<T> baseTagKey, String pattern) {
        }
    }

+    private void add(FluidUnit unit, String singular, String plural) {


You could save a little effort here with an overload that just appends s for plural.

"mB" for millibucket isn't pluralized though, which means [unless i want to switch to writing it out as "millibucket"] i need two different methods and this method can't simply be called 'add' but needs to be something like 'addWithPluralS'. I don't think it's worth it.

I said overload, you can keep around this signature too for the odd cases. Its a private method so the name is not too important, the two parameter add is overriding the plural

KnightMiner · 2024-07-09T15:23:15Z

src/main/java/net/neoforged/neoforge/fluids/FluidStack.java

+    /**
+     * Returns the amount of this stack as a Component.
+     */
+    public Component getHoverAmount() {


Its probably worth having a method on FluidStack to get the full tooltip including display name to standardize the order and color between that and the amount string. Plus would call appendHoverText

raised for discussion on discord due to the possibility that such a method should also take certain data components into account.

src/main/java/net/neoforged/neoforge/fluids/FluidUnit.java

KnightMiner · 2024-07-09T15:28:25Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnitSpec.java

+    private final FluidUnit defaultUnit;
+    private final List<FluidUnit> listForTooltip;
+
+    public static final FluidUnitSpec DEFAULT = new FluidUnitSpec(


Looks to me like its the fallback if units are not defined for a fluid, which just displays in buckets with 3 decimal points below buckets. So you get values like 1.345 buckets.

KnightMiner · 2024-07-09T15:29:38Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnitSpec.java

+    /**
+     * Gets the quantity in millibuckets associated with a given unit.
+     */
+    public @Nullable Integer getValue(FluidUnit unit) {


Rather than nullable, can we use some value that will never happen such as 0 or -1?

I don't want the scenario of someone trying to get a unit that they're certain is there that isn't, and then blithely using the 0 or -1 returned.

Another nitpick, this feels like it should be OptionalInt instead of a nullable Integer keeps the boxing and is much more clear that there either is some amount or something at all imo

I have replaced it with Optional<Integer> in the latest version

src/main/java/net/neoforged/neoforge/fluids/FluidUnits.java

src/main/java/net/neoforged/neoforge/fluids/crafting/UnitAmountFluidIngredient.java

KnightMiner · 2024-07-09T15:48:43Z

src/main/java/net/neoforged/neoforge/fluids/crafting/UnitAmountFluidIngredient.java

+            for (Map.Entry<FluidUnit, Integer> entry : amount.entrySet()) {
+                Integer value = fluidUnits.getValue(entry.getKey());
+                if (value == null)
+                    valid = false;


why no break here? we want fluid ingredients to match or fail to match as fast as possible (if its not valid no point looking at the remaining units or continuing to math).

If we wanted to be fancy, we could write this whole loop using labels and cut out some of the extra variables/if statements, e.g.

public Optional<Integer> amount(FluidStack stack) { FluidUnitSpec fluidUnits = stack.getFluidType().getUnits(); unitOptions: for (Map<FluidUnit, Integer> amount : amounts) { int accumulator = 0; for (Map.Entry<FluidUnit, Integer> entry : amount.entrySet()) { Integer value = fluidUnits.getValue(entry.getKey()); if (value == null) continue unitOptions; accumulator += value * entry.getValue(); } return Optional.of(accumulator); } return Optional.empty(); }

KnightMiner · 2024-07-09T15:50:59Z

src/main/java/net/neoforged/neoforge/fluids/crafting/UnitAmountFluidIngredient.java

+ *
+ * @see net.neoforged.neoforge.common.crafting.SizedIngredient
+ */
+public final class UnitAmountFluidIngredient {


Making this non-final may be useful to support other matching methods (e.g. exact match) without reimplementing the logic.

Alternatively, make some static helpers for anything complex so someone can make their own unit amount fluid ingredient.

Moved the logic to a static function

KnightMiner · 2024-07-09T15:52:33Z

src/main/java/net/neoforged/neoforge/fluids/crafting/UnitAmountFluidIngredient.java

+                    .map(input -> amount(input).map(input::copyWithAmount))
+                    .flatMap(Optional::stream)


Nitpick, but these could easily be 1 map call and save a bit of effort, .flatMap(input -> amount(input).map(input::copyWithAmount).stream())

KnightMiner · 2024-07-09T15:56:53Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnitSpec.java

+import net.minecraft.util.Mth;
+import org.jetbrains.annotations.Nullable;
+
+public class FluidUnitSpec {


For example sake, and to prevent issues with conflicting definitions, Neo should define fluid units for water, lava, and milk.

Lava and milk would probably be just buckets, which would mostly prevent anyone else from doing so (making it conflict in dev instead of waiting to conflict at runtime when someone else tries to do it). Water I feel is worth showing bottles (at the 250mb standard) in the tooltip, but if people would rather leave it to just buckets that would be a reasonable option.

Probably should also do a test mod which defines more units to a fluid, perhaps do an example with the metal standards.

RaymondBlaze

The CauldronFluidContent need to be updated to adapt the system, it should use a FluidUnit for the fluid amount it contains per level.
For vanilla water cauldron, the fluid unit should be FluidUnits.BOTTLE, for lava cauldron, the fluid unit should be FluidUnits.BUCKET, for empty cauldron, the fluid unit should be left as null.
The cauldron itself, however, doesn't need a FluidUnits.CAULDRON, as mods should query its total amount through CauldronFluidContent with the level FluidUnit and total levels.

KnightMiner · 2024-07-10T15:37:19Z

Water cauldrons will be difficult to adapt to this system without dupe bugs until we change the bucket volume (which the original proposal recommends doing after a fluid unit system is adopted). If it is implemented it should in some way be config controlled for the sake of finite water modpacks (remember that vanilla has a gamerule now for finite water, could potentially query that for the cauldron capability levels)

RaymondBlaze · 2024-07-14T11:35:17Z

Water cauldrons will be difficult to adapt to this system without dupe bugs until we change the bucket volume (which the original proposal recommends doing after a fluid unit system is adopted).

The water dupe bug: This only happens if mods provides a conversion of FluidUnits.BUCKET = 4FluidUnits.BOTTLE, vanilla itself does not provide such conversion.
Finite water modpack: In vanilla, cauldron + water bottle + bucket + placeable water block is already a infinite water source due to the fact that water bottles can be filled infinitely from water blocks. The modpack must change vanilla mechanics to make water finite, and again as stated before, it's up to the mods to provide a conversion with dupe bugs.

I do believe the bucket volume needs to change, however it's not a mandatory dependency for adapting cauldrons to the Fluid Unit system.

Random832 · 2024-07-15T07:17:32Z

I do believe the bucket volume needs to change, however it's not a mandatory dependency for adapting cauldrons to the Fluid Unit system.

Regardless of this system, it is a fact that water cauldron levels are, as it stands, 333, 333, and 334 mb. Unless you're suggesting a separate FluidUnit for "the third third of a cauldron", this isn't really viable.

It sounds like your suggestion might have been that we define some value (333? 250?) for water bottles [which I am hesitant to do in the first place] and change the full cauldron to be three times this amount instead of a bucket. I don't really like that either, but I'm open to being convinced. I suspect it will be controversial.

HenryLoenwind · 2024-07-15T09:57:21Z

If the cauldron is the big problem, there's always the possibility of fixing the root issue. Add a config flag to enable a neo patch that changes it to have 4 liquid levels instead of three.

By default, it would be off, as water is infinite, and it literally doesn't matter that a cauldron can dupe some water.

Packs that disable infinite water can then turn the setting on; the cauldron stops being a duping mechanism at the cost of working slightly differently than in vanilla. (Neo has abandoned the notion that mods shall not be allowed to change how vanilla stuff works, only add new stuff, hasn't it?)

And BTW, a Minecraft block is defined as exactly 1 m³, which means it holds exactly 1000 litres of water. This is why mB was chosen to be 1000 and not, for example, 16x16x16 to match the cube pixels in a block or 240 like the old British Pound because it has so many factors (1, 2, 3, 4, 5, 6, 8, 10, 12, 15, 16, 20, 24, 30, 40, 48, 60, 80, 120 and 240). It literally is a real-life value that makes intuitive sense to anyone but people from 3 mostly unimportant countries.

There simply isn't a good solution for the fluid to block/item conversion unless fluid amounts get stored as e.g. (x + y/2520) mB. (Note: y/2520 can store the non-integer part of dividing by any number between 1 and 9 (i.e. the number of slots in a crafting grid) as a whole number. y/36 would be enough to only support splitting into 2/3/4/6/9 pieces.)

KingLemming · 2024-07-16T03:24:37Z

I think the bike shed should be painted blue.

Seriously though, this is kind of a silly problem to keep chasing. Henry is correct though that if ultimately the source of all dupes and the major hangup people have is the cauldron, then it should be addressed directly.

When I wrote Capable Cauldrons (before it was just...lifted for NeoForge), I had the restriction that automation only worked in full bucket amounts, which avoided the 333/334 silliness that has been added now. Offhand I think the best solution is just to call it 250 and go from there - there's precedence in vanilla for it (honey) and it hardly breaks anything since the only change here is to...Water.

ApexModder · 2024-07-16T03:31:04Z

src/main/java/net/neoforged/neoforge/fluids/FluidUnitSpec.java

+    /**
+     * Gets the quantity in millibuckets associated with a given unit.
+     */
+    public @Nullable Integer getValue(FluidUnit unit) {


Another nitpick, this feels like it should be OptionalInt instead of a nullable Integer keeps the boxing and is much more clear that there either is some amount or something at all imo

ApexModder · 2024-07-16T03:31:14Z

src/main/java/net/neoforged/neoforge/fluids/crafting/UnitAmountFluidIngredient.java

+        return amounts;
+    }
+
+    public Optional<Integer> amount(FluidStack stack) {


Small nitpick but why Optional<Integer> when Optionalnt exists?

OptionalInt does not support the map/orElse etc APIs.

marchermans

Leaving this on RC because I am not sure this is a great API design if fluids can just ignore FluidUnits from others.

src/main/java/net/neoforged/neoforge/fluids/crafting/UnitAmountFluidIngredient.java

KnightMiner · 2024-07-16T15:11:25Z

Seriously though, this is kind of a silly problem to keep chasing. Henry is correct though that if ultimately the source of all dupes and the major hangup people have is the cauldron, then it should be addressed directly.

Ah yes, vanilla is incompatible with a NeoForge API, so vanilla is the problem. NeoForge should dictate the design of the vanilla game. Lets just call up Mojang and tell them they were wrong for not considering the Forge API when they added cauldrons.

Cauldrons are a problem here, but they just serve to highlight how inadequate fluid capacities are. We are all forced to use buckets, but then Neo forces a stupid bucket division that is incompatible with the fundamental numbers that show up in Minecraft. Nothing in Minecraft is metric system apart from some statistics calling blocks "meters". Rather, numbers that show up are 3, 4, 8, 9, or 64. Why must we limit ourselves just to be compatible with a standard you wrote a decade ago? The standard is flawed because it does not work with cases the vanilla game covers. Changing the vnailla game just to make the API work just proves the API is horribly flawed.

Misinterpretted the API

KingLemming · 2024-07-16T16:12:21Z

Ah yes, vanilla is incompatible with a NeoForge API, so vanilla is the problem. NeoForge should dictate the design of the vanilla game. Lets just call up Mojang and tell them they were wrong for not considering the Forge API when they added cauldrons.

You know, there are times where Forge has made subtle adjustments to the vanilla game for consistency's sake.

Also @Matyrobbrt, don't edit people's posts. NeoForge should be better than that which you sought to replace.

HenryLoenwind · 2024-07-16T16:24:06Z

Could you please keep your personal quarrels to Discord? We all have over a decade of baggage; if we put that on the table all the time, we'd never achieve anything.

marchermans

This is an RC not focused on the Code directly:

This PR introduces a relatively complex API (combining several subsystems and adding new ones), however its implementation is not well enough documented, the PR description is not well enough documented, it has no direct use-case examples etc. Which makes it very hard to determine what is supposed to be used and how.

Could you please expand the documentation on this PR massively, the gist is simply not sufficient, and might be deleted.

Greets,

Marc

Matyrobbrt · 2024-07-16T20:15:36Z

Also @Matyrobbrt, don't edit people's posts. NeoForge should be better than that which you sought to replace.

@KingLemming I did it on purpose, those interested in this PR's technical merits and comments do not need to read beef between developers, or snarky comments. Consider this an official warning, I am politely suggesting that you refrain from this behaviour in the future or your comments will get completely deleted (as you might have noticed, I kept the actual argument in your comment) according to our Code of Conduct.

Any replies to this comment will be marked as off topic, if you wish to discuss my actions you may do so in another thread or on Discord.

Edit: initially mentioned the wrong person, but point still stands for both of you.

neoforged-automation · 2024-08-08T17:04:25Z

@Random832, this pull request has conflicts, please resolve them for this PR to move forward.

sciwhiz12 added enhancement New (or improvement to existing) feature or request 1.21 Targeted at Minecraft 1.21 labels Jul 9, 2024

HenryLoenwind reviewed Jul 9, 2024

View reviewed changes

Random832 force-pushed the fluid-units branch 2 times, most recently from d770d52 to d01fb38 Compare July 9, 2024 04:32

IchHabeHunger54 reviewed Jul 9, 2024

View reviewed changes

KnightMiner reviewed Jul 9, 2024

View reviewed changes

RaymondBlaze suggested changes Jul 10, 2024

View reviewed changes

ApexModder reviewed Jul 16, 2024

View reviewed changes

Random832 force-pushed the fluid-units branch from d01fb38 to 4c82c9c Compare July 16, 2024 04:12

Fluid Unit system

6e4763a

Random832 force-pushed the fluid-units branch from 4c82c9c to 6e4763a Compare July 16, 2024 04:48

marchermans previously requested changes Jul 16, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/fluids/crafting/UnitAmountFluidIngredient.java Show resolved Hide resolved

marchermans requested changes Jul 16, 2024

View reviewed changes

neoforged-automation bot added the needs rebase label Aug 8, 2024

Speiger mentioned this pull request Sep 6, 2024

Custom language hooks #1519

Closed

		.map(input -> amount(input).map(input::copyWithAmount))
		.flatMap(Optional::stream)

Fluid Unit system #1264

Are you sure you want to change the base?

Fluid Unit system #1264

Conversation

Random832 commented Jul 9, 2024 • edited Loading

neoforged-pr-publishing bot commented Jul 9, 2024 • edited Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random832 Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaymondBlaze left a comment

Choose a reason for hiding this comment

KnightMiner commented Jul 10, 2024 • edited Loading

RaymondBlaze commented Jul 14, 2024

Random832 commented Jul 15, 2024 • edited Loading

HenryLoenwind commented Jul 15, 2024

KingLemming commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchermans left a comment

Choose a reason for hiding this comment

KnightMiner commented Jul 16, 2024

KingLemming commented Jul 16, 2024 • edited Loading

HenryLoenwind commented Jul 16, 2024

marchermans left a comment

Choose a reason for hiding this comment

Matyrobbrt commented Jul 16, 2024 • edited Loading

neoforged-automation bot commented Aug 8, 2024

Random832 commented Jul 9, 2024 •

edited

Loading

neoforged-pr-publishing bot commented Jul 9, 2024 •

edited

Loading

Random832 Jul 17, 2024 •

edited

Loading

KnightMiner commented Jul 10, 2024 •

edited

Loading

Random832 commented Jul 15, 2024 •

edited

Loading

KingLemming commented Jul 16, 2024 •

edited

Loading

Matyrobbrt commented Jul 16, 2024 •

edited

Loading