Refactored the liquid system, and added an optional feature for liquids to flow to the next slope.

[cosin15]

I refactored the liquid system, and added a feature that enables MC like liquids.

The mod-creator can now use the property liquid_slope_range in the liquid definition.

• If the value is 0, the liquid should behave like before.
• If the value is at it's maximum 8. It will search 8 blocks for the nearest slope.

To do

This PR is a Work in Progress. I plan to spend some time.

• Viscosity refactored
• Liquid range refactored
• Falling liquid refactored (May be there implicit)
• Renewable liquid refactored
• Rollback refactored
• Liquid of different kinds do not interact.
• Lua access refactored
• Time limit refactored
• Ensure that it fits the need of the community.
• Ensure that the default behavior is the same as the old behavior
• Ensure that it does not break MCL (cobwebs are liquid)

How to test

Add the property liquid_slope_range to the liquid definition of your favorite game. Don't forget to make a backup of your map before trying this PR.

[cosin15]

I think this would close: #5387 and #11201

[NathanSalapat]

A single picture doesn't tell much, but the current system for has always seemed kinda jank to me.

[cosin15]

After multiple failed attempts to decipher the system I've decided to rewrite it. The hard part now is to ensure that the behavior is compatible. It was crazy to see how cobwebs spread all over the place.

[dax-er]

Once you have finished the PR, maybe post a video showing which parts have changed from before. Then it would be easier to see how things work differently.

[cosin15]

If the liquid enables flow_down it will behave like the water explained in this video: https://www.youtube.com/watch?v=9uIRD9sMBU0

[cosin15]

Does somebody know why my liquids are rendered flat despite the level in param2?

[kromka-chleba]

Falling liquid refactored (May be there implicit)

Hi, is it possible to an option for #13782 in nodedef of liquids when you're at it?
That is as long as this feature is a low hanging fruit and could be implemented painlessly.
Liquids currently have trouble falling while they're surrounded by flowing nodes - sometimes they fall, sometimes they don't. I only get them to fall by forcefully running VoxelManip:update_liquids() and then minetest.check_for_falling() - this removes flowing nodes around sources and a race condition allows for falling.

Edit: I fixed this by modifying 2 lines in builtin/game/falling.lua, see #13782
If my changes are not hacky, please consider including them in the PR.

[cosin15]

Have you tried it with my PR? I think when you make water falling like sand. It should do what you expect.

[kromka-chleba]

I'll try your PR tomorrow then and see if it fixes my problem.

[kromka-chleba]

Okay, checked your PR.
It doesn't solve my problem - liquid source nodes still won't fall through flowing liquid nodes.
Also now liquid sources with liquid_range = 0 stop spawning flowing nodes, is this intentional?

[kromka-chleba]

Okay, I have fix for my issue ready:
https://github.com/kromka-chleba/minetest
It is on the master branch.

I tried to make a PR myself but couldn't press the "make a PR" button for some reason...

[cosin15]

It was my interpretation of liquid_range. If my PR get approved, I will change that for compatibility.

[kromka-chleba]

Ok, managed to publish the PR #13789

It was my interpretation of liquid_range. If my PR get approved, I will change that for compatibility.

If it's intentional then there's no need to do so, I will simply adjust the code of our game if needed.
Your interpretation is correct.

[cosin15]

Okay, checked your PR. It doesn't solve my problem - liquid source nodes still won't fall through flowing liquid nodes. Also now liquid sources with liquid_range = 0 stop spawning flowing nodes, is this intentional?

I cannot reproduce this behavior. In that regard my PR does exactly the same as 5.7.0

[cosin15]

@sfan5 If I get roadmap approval I will create a comprehensive unit test for the previous liquid system, complete the refactor and ensure that the new liquid system stays compatible.

[kromka-chleba]

I cannot reproduce this behavior. In that regard my PR does exactly the same as 5.7.0

This is from Exile v4.
https://codeberg.org/Mantar/Exile
It's on the v4 branch.

Source def:
https://codeberg.org/Mantar/Exile/src/branch/master/mods/nodes_nature/liquids.lua#L32
Flowing def:
https://codeberg.org/Mantar/Exile/src/branch/master/mods/nodes_nature/liquids.lua#L78

[sfan5]

@sfan5 If I get roadmap approval I will create a comprehensive unit test for the previous liquid system, complete the refactor and ensure that the new liquid system stays compatible.

I don't have an opinion on this yet but yes, unit tests and backwards compatibility (that is: opt-in behaviour) are important.

[cosin15]

[SmallJoker]

Discussed a bit in the dev meeting: https://irc.minetest.net/minetest-dev/2023-09-17#i_6114953

In addition to the compatibility of existing worlds, how well does it scale in performance? Currently it does require less nodes to be updated, but optimally it should perform the same or better as the current (master) water propagation. Hence, benchmarks would be somewhat welcome to not undo years of slow optimizations in this code.

---

EDIT: Based on additional meeting comments: (where I agree to)

i think it's fair to require any refactor+feature to be split into first a refactor, then a feature ~celeron55

The PR should be split into refactor first, feature after for proper performance and compatibility checks. By the looks of it, the feature PR will then be rather small (mainly network and Lua API additions). However, this PR is currently in a suboptimal state for review.

[cosin15]

Discussed a bit in the dev meeting: https://irc.minetest.net/minetest-dev/2023-09-17#i_6114953

In addition to the compatibility of existing worlds, how well does it scale in performance? Currently it does require less nodes to be updated, but optimally it should perform the same or better as the current (master) water propagation. Hence, benchmarks would be somewhat welcome to not undo years of slow optimizations in this code.

I have not done a benchmark jet.
I think the performance stays similar when the feature is turned on. More nodes are red to find the nearest slope, but on the other hand, the liquid does not spread as often, or even rarely.
If the feature is disabled the performance should stay the same. In my refactored version: If the viscosity is maximal every node that becomes a flowing liquid gets updated exactly once. I think I can guarantee that.

EDIT: Based on additional meeting comments: (where I agree to)

i think it's fair to require any refactor+feature to be split into first a refactor, then a feature ~celeron55

The PR should be split into refactor first, feature after for proper performance and compatibility checks. By the looks of it, the feature PR will then be rather small (mainly network and Lua API additions). However, this PR is currently in a suboptimal state for review.

That's fine.
As I've mentioned before: If I get roadmap approval for this feature I will:

• Create a test suit for the current liquid system.
• Refactor the liquid system.
• Implement the feature (another pull request).

Another option would be: Keep the existing liquid system as is. And create the alternative liquid system with the feature alongside (Tests included).
Then it would be up to the game-creator/or user to choose. This could be done via an interface.

[SmallJoker]

This liquid system could open up for some new puzzles or world modelling that is currently not possible with the current spread mechanism.

Discussed again in a dev meeting: https://irc.minetest.net/minetest-dev/2023-10-01#i_6119109

If the performance of the current liquid flow is the same (or better) and bug-to-bug compatible, then I see no reason against this PR.

[cosin15]

@SmallJoker Do you mind if I isolate the current Liquid system as it is into separate Module, for the purpose of testing?

[SmallJoker]

@cosin15 If it simplifies the code structure or helps you during the development I have no objections. It will have to be tested for compat anyway.

[Zughy]

@cosin15 any updates on this?

[Zughy]

It looks like OP has disappeared (according to their GH history). Closing

[cosin15]

I'm back.
After doing some tests with the current liquid system I've come to realize that my new implementation will never be bug to bug compatible. I saw some really strange behavior.

I would suggest keeping the current liquid system as it is, and adding an alternative, well tested liquid system that can be enabled.