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

Define standard FluidConstants #1138

Open
wants to merge 1 commit into
base: 1.21.x
Choose a base branch
from

Conversation

Technici4n
Copy link
Member

Happy to discuss which constants should exist, with what definition, etc...

@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Jun 18, 2024

  • Publish PR to GitHub Packages

Last commit published: 17a008b98cae1c75573739fd13704153723b9d9b.

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 #1138' // https://github.com/neoforged/NeoForge/pull/1138
        url 'https://prmaven.neoforged.net/NeoForge/pr1138'
        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-pr1138.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1138
cd NeoForge-pr1138
curl -L https://prmaven.neoforged.net/NeoForge/pr1138/net/neoforged/neoforge/21.0.19-beta-pr-1138-fluid-constants/mdk-pr1138.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.

@RaymondBlaze
Copy link
Contributor

RaymondBlaze commented Jun 19, 2024

The BUCKET = 1000 feels a bit off due to vanilla 1 Bucket = 3 Bottles :P
If the bucket should be the multiple of 3, using the same of Block of 9 would be enough.
For the sake of things that craft into 16 like iron bars, the nugget should be 16 instead of the randomly defined 10, anything craft into more is out of the scope of vanilla and thus out of the scope of the standard.
Since there is case like honey where 4 Bottles = 1 Block, the Bottle should be 1/4 of Block of 4.
The idea of 1 fluid block (bucket) = 1 solid block should be pretty much seen as not mandatory, and can be reserved for Block of 9.

My suggestion:

  • Bucket: 1296
  • Block of 9: 1296
  • Block of 4: 1728
  • Ingot / Item of 1/9 Block: 144
  • Bottle / Item of 1/4 Block: 432
  • Nugget: 16

*
* @see #BLOCK_9 for the corresponding block
*/
public static final int GEM_9 = 90;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, gems are 100mb

@KnightMiner
Copy link
Contributor

KnightMiner commented Jun 19, 2024

The BUCKET = 1000 feels a bit off due to vanilla 1 Bucket = 3 Bottles :P If the bucket should be the multiple of 3, using the same of Block of 9 with 810 would be enough. Since there is case like honey where 4 Bottles = 1 Block of 1080, the Block of 4 could use that value.

My suggestion:

  • Bucket: 810
  • Bottle: 270
  • Block of 9: 810
  • Block of 4: 1080
  • Gem/Misc of 9: 90
  • Gem/Misc of 4: 270
  • Ingot: 90
  • Nugget: 10

This PR is not about changing the standards, its just documenting standards. and 810 as a bucket is horrible for some mods (including notably Tinkers' where only some fluids can use 810, but many such as clay or glass cannot)

@KnightMiner
Copy link
Contributor

KnightMiner commented Jun 19, 2024

I've talked about this on discord, I'll say it here: Neo is a horrible place to track these standards. Presently all we need is two modders who share a fluid to agree and we make a standard between each other. Putting these in Neo means I'm unable to make a standard without consulting a team of modders who may not even touch fluids. I've had this problem many times with forge, only 1 dev on the team even used fluids in their mod so I was forbidden from making any changes until they got off vacation. I don't have any desire to see that happen again with fluid values.

A good place to track these standards is a public wiki. Let modders discuss the standards and document them in a public place so that it can be modified by the authors of those standards, not a third party.


Also, this PR fails at handling the most important part of fluid standards: documenting them in game for the player to see. That is where fluid unit tooltips come in. Before and after this PR, these standards are just numbers hidden in code that a player never sees.

@3TUSK
Copy link

3TUSK commented Jun 19, 2024

vanilla 1 Bucket = 3 Bottles

Vanilla itself has already been inconsistent by requiring 4 honey bottles to craft a honey block, instead of 3 honey bottles.

Fortunately, vanilla does not have a "honey bucket" yet, so technically this can have a pass.

@lukebemish
Copy link
Contributor

lukebemish commented Jun 19, 2024

This is, frankly, unworkable, as it bakes in all the issues of the existing system. The fluid unit system proposed on discord is much more flexible, and actually handles the sort of not-uncommon cases where this proposal just leads to lots of pain and pointless limitations on mods. Say I've got a molten metal. And a bucket placed in-world should cool to a single metal block. And I've got 9 ingots per block. See how this proposal immediately leads to issues as soon as you want to do something vaguely out of the ordinary? We should be trying to expand what modders can do with the APIs provided, not lock them down to arbitrary standard, while still providing the right tools to communicate sensibly between mods and ensure that they interact as users would expect -- which is exactly what the fluid unit proposal in discord does! It doesn't arbitrarily limit what modders can do with fluids, but it allows modders to interact sensibly with other people's fluids, simply by working in whatever relevant units both mods understand.

@RaymondBlaze
Copy link
Contributor

This PR is not about changing the standards, its just documenting standards. and 810 as a bucket is horrible for some mods (including notably Tinkers' where only some fluids can use 810, but many such as clay or glass cannot)

Haven't been tracking the discord discussion much, but the "define" in the title can definitely include changes.
810 is horrible because the 10 for nugget is horrible, I know Tinkers' cares things like iron bars and glass panes where things break down to 16 due to crafting, and 16 for nugget would solve that for sure.
Bring in more smaller edge case units will only lead this to nowhere, and I don't believe the unit proposal would solve that case either.


Vanilla itself has already been inconsistent by requiring 4 honey bottles to craft a honey block, instead of 3 honey bottles.

Fortunately, vanilla does not have a "honey bucket" yet, so technically this can have a pass.

That's pretty much irrelevant, fluid blocks don't have to be equal with solid blocks even they do in 90% cases.


I've talked about this on discord, I'll say it here: Neo is a horrible place to track these standards. Presently all we need is two modders who share a fluid to agree and we make a standard between each other. Putting these in Neo means I'm unable to make a standard without consulting a team of modders who may not even touch fluids. I've had this problem many times with forge, only 1 dev on the team even used fluids in their mod so I was forbidden from making any changes until they got off vacation. I don't have any desire to see that happen again with fluid values.

Neo is definitely the place for these standards, and standards exists because it is meant to eliminate the need to let 2 or more random mods to agree on anything. Arguing about Neo's members not touching fluids is pointless, if one care about fluids, they should come here and discuss, show their needs, propose proper standards.


We should be trying to expand what modders can do with the APIs provided, not lock them down to arbitrary standard.

A standard does not lock anyone, the only thing one is forbidden to do is to claim they're following the standard while they actually don't, and self-baked standards will only be more arbitrary than Neo's.
Mods are free to make a random ingot the craft into block using some random amount like 5, it's fine, nothing forbids this, they just shouldn't claim their random ingot and block in c:ingots and c:storage_blocks for the sake of recipes.

@KnightMiner
Copy link
Contributor

KnightMiner commented Jun 20, 2024

810 is horrible because the 10 for nugget is horrible, I know Tinkers' cares things like iron bars and glass panes where things break down to 16 due to crafting, and 16 for nugget would solve that for sure.
Bring in more smaller edge case units will only lead this to nowhere, and I don't believe the unit proposal would solve that case either.

Its only horrible if you prioritize a non-reversible crafting recipe over buckets. The 90mb standard was made to ensure 100 nuggets is a bucket, the classic 144mb standard means the bucket volume cannot be expressed cleanly as any metal unit (62.5 nuggets per bucket is disgusting). This is the reason for the 100mb standard for gems as well: 10 gems per bucket (as gem "nuggets" are non-standard, you shouldn't be forced to have 1 of them in every bucket. Tinkers' just assumes if they divide gems divide into quarters).

Glass panes/iron bars are trivial to solve if you just assume they are like stairs (crafting table recipe is lossy): make them 250mb per pane (means a pane is 4/16 instead of 6/16 of a block when casting/recycling, a nice discount). Similarly, iron bars are just 3 nuggets (so iron bars when casting are 48/144 instead of 54/144 of an ingot, also a nice discount).

Since those are just nicer numbers for players and they don't require switching to numbers that conflict with buckets (a very important unit), they are just a better choice (I'd probably stick to them even if I could divide ingots into 16, because no one wants 1/16 of an ingot, you want 1/9 of an ingot).


Anyways, as previously stated, this issue is for documenting the existing standards in Neo to ensure consistency among mods. If you want to change the standards used, feel free to open a thread on the Neo Forge discord where we can discuss this. That said, I think my opinion (as the author of a mod using these standards) has a lot more weight in this discussion than yours (unless you make some mod I am unaware of that uses molten metals that is)

@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request 1.21 Targeted at Minecraft 1.21 labels Jul 5, 2024
@sciwhiz12
Copy link
Member

As I see it, the only standard that this PR solidifies is the already defined standard for the bucket -- one bucket is equal to 1000 millibuckets, or 1 B = 1000 mB.

The other definitions in FluidConstants are longstanding conventions I believe, which hold because of widespread copying of these definitions from mods to mods throughout the ages, rather than being thought out by a group of modders in the past and written down for future shared use.

I would say that we should wait a bit more time to see the fruits of the fluids discussion in the Discord server if they manifest, and compare the highs and lows of that new approach versus this approach of codifying existing conventions.

In the interests of ensuring this PR goes forward in a somewhat timely manner, let's set a deadline here: I propose that if the fluids discussion in the Discord server does not bear fruit (in the form of a concrete issue or PR) by the end of the 2nd week of August, then we will definitely move forward with this PR's discussion to eventual merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Targeted at Minecraft 1.21 enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants