neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.18k stars 173 forks source link

Fluid should decide if FluidStacks (Components) can merge #1577

Open Speiger opened 1 week ago

Speiger commented 1 week ago

Fluids have components that allow additional data. The main issue with that is usually that a Tank has 1 fluidtype limit. Yes there are alternatives but even these usually are with restrictions or self implementations.

The question is: Why do these components exist in the first place? (not a serious question) One thing I realized is: If i want to implement fluids with custom components then Compat is going to suck with other mods because I have to expect that tanks support my implementation.

So the thought came: What if Fluids have a function that is named: public boolean canFluidsMerge(FluidStack self, FluidStack other) and public FluidStack mergeFluids(FluidStack self, FluidStack other)? This assumes the Fluid (type) itself are the same. If the fluids without components don't match then the thing gets thrown out of the window.

This would increase compat with fluids drastically and also allow mods to actually make use of such a functionality. I only ever remember one 1 mod ever using this feature. And I am pretty sure it was the mod with red obsidian that used it... Don't know the name, but it was on ForgeCraft server.

Anyways It is a thought that came to mind.

Technici4n commented 1 week ago

The FluidStacks are modeled after ItemStacks, which do not have such a feature.

Speiger commented 1 week ago

@Technici4n lazy argument since everything about storage is 100% different. On top of that fluid mixing makes a lot more sense than Item Mixing.

(Edit: This answer assumes that your answer means: "This is why it shouldn't be done", if it is a answer for: "Why it hasn't been done" then I am sorry.)

Speiger commented 1 week ago

Anyways, I would suggest that this is at least seriously considered, because it would be sad if this feature gets unused (the components). Tank Type Capacity is usually 1 and making compat working is usually include bad practices.

HenryLoenwind commented 1 week ago

The FluidStacks are modeled after ItemStacks, which do not have such a feature.

They do. ItemStack#isSameItemSameComponents() does exactly that; it checks if two stacks can be merged taking both their type and their components into account. Check any insertItem() impl and you'll find a if (!ItemStack.isSameItemSameComponents(stack, existing)) return ... there. (Edit: Or do you mean, there's no way for items/components to influence that check? That's true and in itself questionable.)

Speiger commented 6 days ago

@HenryLoenwind what i am suggesting is that the Fluid itself, not the components, can validate if 2 FluidStacks (based on its Components) can merge. So that dynamic fluids could also exist outside of 1 mod and properly work too. By default this behavior would be the exact same and would not be changed.

But if you wanted to as a fluid owner you could override it. For example: If you had a Fluid Dye so the dye color would mix based on the 2 fluids being merged. Or if you had liquid metals and alloys could be created then fluids could do that automatically if they were 1 Liquid Metal Fluid (and not 2 separate fluids)

The whole point is, Forge has a Component System for fluids like ItemStacks, but all Containers out there really only support 1 Fluid. Rarely you have multi fluid tanks out there, but Fluids should be exactly treated as Items, which have containers with tens to hundreds of slots.

And i don't remember neo/forge providing a multitank implementation last i remembered.

Since there is an obvious difference between fluids/items, i am asking to consider giving the fluids a bit more creative freedom. Especially since items can nowdays also decide how merging is working when you put them into a slot or getting things put into your slot!

KnightMiner commented 6 days ago

I'm not personally convinced by the dye fluid usecase, that sounds to me like a case that should be handled by a mixer block that combines two fluids rather than just expected for every storage tank. This is partly because fluid tanks are typically not being filled by whole stacks at once, they are being filled based on the transfer rate, meaning such a system would have to guarantee a + 5b is the same as a + b + b + b + b + b or now your results are inconsistent based on fluid pipe rates.

I think its reasonable to be able to have a fluid ignore some data components when merging at a conceptual level, though I cannot think of any example usecases of that. I could see an argument for a system that allows fluids to merge with other fluid types entirely, such as for merging "equivalent" fluids from a tag, though I am not sure that is what you had in mind with this system. I find the idea of two fluids merging and leaving behind something that matches neither a bit problematic though; thats just forcing crafting recipes on storage blocks.

Speiger commented 6 days ago

This is partly because fluid tanks are typically not being filled by whole stacks at once, they are being filled based on the transfer rate, meaning such a system would have to guarantee a + 5b is the same as a + b + b + b + b + b or now your results are inconsistent based on fluid pipe rates.

This is for the Owner of said fluid to decide how that is handled. The idea is that the dev gets full control on how things are merged with their own fluid. So saying: "This edge case may appear and cause issues" would only result in the mod that uses the feature having to deal with it. Since the API isn't at fault. (unless the API made a giant fuckup)

I think its reasonable to be able to have a fluid ignore some data components when merging at a conceptual level, If we add ignoring then removing the Components might be wiser, because it is simply to bound to be useful.

though I cannot think of any example usecases of that

The main reason nobody really does it is, because its extremely incompatible with mods and causes a lot of "Why isn't this tank supporting it" issues on everyones side. There was 1 mod that did it really far back, like 8 years ago. That's the only attempt I personally can remember.

Deep resonance

It had a system where you would get like 3 different metadata values into the fluid and they would mix together/get consumed individually based on what you did with it.

This functionality was sadly limited to its tanks and was 100% incompatible with any other mod. And providing the API would make compatibility to 95%-99%.

So that mods that went this creative actually are usable outside of their own ecosystem.

CodexAdrian commented 5 days ago

The idea of fluids merging to create something different than they had originally being the default behavior of fluids in storage seems inherently unintuitive and a bit destructive to me. The idea is interesting, but its something that should be implemented as part of a recipe system like Tinkers alloy mixing, not as part of the default behavior of storage.

In general I think that a player should reasonably be able to take something and store it without it being influenced by something. Thats consistent with the principal that things in chests are in a kind of stasis. They dont age or tick if they're inside a chest. Of course, you can change this with modded, but this is usually expressed to the user in some way, and the player chooses to do it or is aware this behavior has changed. This would inherently change the behavior of all storage to be mixing containers. They're no longer storage, but a mechanism for transforming liquids, in the same vein that a furnace is a mechanism for transforming items. Which shouldnt be the expected behavior for all storage.

I could see a situation where perhaps there are components that are ignored when 2 things are merged, but yeah just having a straightup canMerge and merge is too recipe-like, shouldnt be something built into storage.

KnightMiner commented 5 days ago

The main reason nobody really does it is, because its extremely incompatible with mods and causes a lot of "Why isn't this tank supporting it" issues on everyones side.

No, I have no desire to do that with my fluids. If I want to merge two fluids, I would use a recipe block to merge two fluids. If I have two fluids with different NBT, I assume they are different.

I agree that a mod has wanted this feature before, but I don't see a usecase for this outside of that mod, and I also find that mods fluids merging questionable in my mods; it would be very incompatible with the concept of multi-tanks as a whole.

Please, lets not merge two fluids into a fluid that is neither; I'd honestly rather not merge fluids at all as part of the transfer API. That is just going to make my life as a person who does multi-tanks way more difficult so a single mod can be more compatible with breaking tank assumptions.

Speiger commented 5 days ago

it would be very incompatible with the concept of multi-tanks as a whole Not really. I mean its not like neo/forge provides a multi tank implementation to begin with. Outside of that, if you implement a multitank you already control the behavior of how fluids are merged in your implementation.

But if you use the default implementation it should support merging, because the default is a single type tank.

No, I have no desire to do that with my fluids

And? The default behavior would be what you want. It would be an Opt In system. Why ban an opt in system if it doesn't harm you.

The whole point is: If a mod would want to do that they have the ability to. Not that all fluids should follow the rule.

Oh i just realized, with this suggestion max fluid sizes would also be possible...

CodexAdrian commented 5 days ago

Oh i just realized, with this suggestion max fluid sizes would also be possible...

This would be an awful way to implement max fluid sizes. You have no context about how large the container its being held in is for the fluid to determine the max size, and the container itself isnt given enough information to know how to scale how much of that fluid can be stored in that slot.

The whole point is: If a mod would want to do that they have the ability to. Not that all fluids should follow the rule.

This shouldn't be how mods implement this feature. This should be done via a data driven recipe system, not as part of the standard fluid api

KnightMiner commented 5 days ago

And?

You said people "want to do this but can't". I was refuting that by saying, no, I don't want to do this. Its "extremely incompatible with mods and causes a lot of "Why isn't this tank supporting it" issues on everyones side", as you said, and even with this change its still an extra burden to support, all for a feature I see no usecase for.

Just make an alloyer block like Tinkers has, served us well since 1.6.4.