Fix liquid falling if in "float" group, fixes #13782

[kromka-chleba]

Goal of the PR

(Hopefully) fixes #13782

How does the PR work?

It makes liquid source nodes that are in falling_node = 1 and float = 1 fall correctly if liquidtype = "flowing" is below. It also makes liquid source nodes replace flowing nodes when it falls.

Does it resolve any reported issue?

Yes #13782

Does this relate to a goal in the roadmap?

No.

If not a bug fix, why is this PR needed? What usecases does it solve?

I didn't fill a proper bug report because I initially though it was a mistake on my side, but after doing some test I believe this is a bug.
A liquid node that is in falling_node = 1 and float = 1 can't fall because it is often surrounded with flowing liquid nodes. Sources almost always are surrounded by flowing nodes, and rarely with air. This means they can only fall if a solid node under them is removed or directly after placing. Existing water fountains can't really fall and I believe this is a bug.

Nothing to do

This PR is Ready for Review.

How to test

Create a liquid in falling_node = 1 and float = 1. The flowing variant should not be in falling_node = 1.
Place the liquid on the ground and wait for it to spawn flowing nodes. Drop a source node on the flowing nodes and see as it replaces the flowing node on the ground.

Build a tower, place a source node on top. Place a source node over the column of flowing water nodes. The source node should fall to the bottom of the column and touch the ground instead of hitting the topmost flowing node like it was doing before this PR.

Try dropping a source node on another source node - the source node should float on top of the other source node.

Make an ordinary solid node in falling_node = 1 and float = 1 groups, drop it on flowing and source liquid nodes. The node should float on both like it did before this PR.

[kromka-chleba]

Not sure what that check is, someone smarter than me could give me a hint here.

[Zughy]

To be honest, I'm not sure if this is a feature or a bugfix. I've never experimented with liquids, I can't really make up my mind. I'll leave it to some core dev. In the meanwhile I've adopted the same label put in the issue. Unsubscribing

[kromka-chleba]

I think the unsuccessful check is a failing unit test?
I'll adjust the test but would be good to know if there's any interest in merging this PR in the first place.

The current code allows for either: 1. falling source nodes that are not floating (oceans will collapse easily) 2. source nodes that can only fall directly after placing or if a solid node is removed under them

[SmallJoker]

👍 I support this feature. It does make sense to me that falling liquid nodes should be capable of replacing flowing nodes. Whereas I cannot think of practical use-cases in mods, I am certain that this behaviour will open up some more possibilities.

[kromka-chleba]

Whereas I cannot think of practical use-cases in mods, I am certain that this behaviour will open up some more possibilities.

Well, we already use this in Exile to implement "realistic" moisture spread.
I added some overrides here and there for builtin stuff. Making liquids falling prevents the formation of lakes hanging in air and helps liquids to move down.

River beds in Exile are dry so everything you can see in the screenshot below is rain + moisture spread in soil + leaching out + falling water sources. Screens:

https://codeberg.org/Mantar/Exile/issues/502#issuecomment-1100624

---

As for formal stuff, how can I check what the unsuccessful GitHub check is? When I click details it gives me no clues. I couldn't find any unit tests for this in the devtest game, it only tests "float".
So what should I do to get this merged?

[rubenwardy]

After clicking details, I see this:

Checking builtin/game/falling.lua 1 warning

builtin/game/falling.lua:544:1: inconsistent indentation (SPACE followed by TAB)

https://github.com/minetest/minetest/actions/runs/6136949544/job/16652302237?pr=13789#step:7:41

[kromka-chleba]

Yeah so I fixed the problem with tabs vs spaces.
Should I squash the two commits into one?
I also could give a longer description in the commit.

I will retest the commit again today to check if everything's fine and works as it should.

[kromka-chleba]

Okay, tested the PR again and everything appears be working as expected.

[kromka-chleba]

Yeah, I improved commit descriptions and I think the PR is ready to go.

[SmallJoker]

What would you think about the following styling? I hope it does make the purpose of the conditions clearer by also documenting them:

local do_float = core.get_item_group(n.name, "float") > 0
-- Otherwise only if the bottom node is considered "fall through"
if not same and
		(not d_bottom.walkable or d_bottom.buildable_to)
		and -- Take "float" group into consideration:
		(
			-- Fall through non-liquids
			not do_float or d_bottom.liquidtype == "none" or
			-- Only let sources fall through flowing liquids
			(do_float and d_falling.liquidtype == "source" and d_bottom.liquidtype ~= "source")
		) then

	local success, _ = convert_to_falling_node(p, n)
	return success
end

[kromka-chleba]

I'm fine with anything as long as it works. I simply used whatever Emacs does when I press tab.
So feel free to add any changes necessary.

[kromka-chleba]

It's my first time using this GitHub review feature to be honest.
Do you want me to apply your changes manually and add them as a separate commit?

[SmallJoker]

I took the liberty to change the code accordingly. Perhaps I should've used the code suggestion feature to simplify the integration with a single click.

Different topic: Generally I would recommend you to keep a clean master branch that acts as a base for PR-specific branches. That way you will not have to restore the master branch (force push) after the PR is merged (or dismissed).

[SmallJoker]

Works on my system.

[kromka-chleba]

Is there anything stopping this from getting merged? (other than developer time to hit the merge button?)
Would be cool if this could avoid bitrot.

[appgurueu]

Thanks for the PR!

Tested, works. This still isn't perfect (for example a water source will fall through flowing lava), but I think it is better than the previous behavior. I've taken the liberty to add a line to the docs and to modify devtest liquid nodes so that this is easily testable.

Is there anything stopping this from getting merged? (other than developer time to hit the merge button?) Would be cool if this could avoid bitrot.

PRs need two approvals by core devs. (If a "ready for review" PR is opened by a core dev, it has an implicit approval by the author, though.)

[kromka-chleba]

Tested, works. This still isn't perfect (for example a water source will fall through flowing lava), but I think it is better than the previous behavior.

Water vs whatever other liquid behavior is game-specific and shouldn't really be handled by builtin unless some generalized groups are added. Falling water could do many different things when hitting flowing lava, e.g. turn into cobble stone, evaporate or explode. Builtin shouldn't make any MTG- or MCL2-specific assumptions.
Perfect falling code would probably allow modders to register on_collision instead of hardcoding behavior into builtin. Some abstract examples would be cold water falling into boiling oil, dense glycerol solution falling into water, liquid mercury falling into water, etc.

I've taken the liberty to add a line to the docs and to modify devtest liquid nodes so that this is easily testable.

Good, thank you.

PRs need two approvals by core devs. (If a "ready for review" PR is opened by a core dev, it has an implicit approval by the author, though.)

I see.

[appgurueu]

Water vs whatever other liquid behavior is game-specific and shouldn't really be handled by builtin unless some generalized groups are added. Falling water could do many different things when hitting flowing lava, e.g. turn into cobble stone, evaporate or explode. Builtin shouldn't make any MTG- or MCL2-specific assumptions.
Perfect falling code would probably allow modders to register on_collision instead of hardcoding behavior into builtin. Some abstract examples would be cold water falling into boiling oil, dense glycerol solution falling into water, liquid mercury falling into water, etc.

I agree, we should have more hookable callbacks here. See also #14291 (comment).