Don't block fluid (water, lava) flow by flowers, mushrooms, grass and saplings

[bartoszkosiorek]

In previous implementation, little flower or mushroom was barieer
by which water was not able to pass.

With this implementation water is able to pass through flowers
by destroying it and also is allowing to pick up these flowers.

[appgurueu]

Why are you removing the flammable group? Setting flowers on fire should be left unchanged.

[bartoszkosiorek]

I didn't removed flammable group, I just make it default to 1.

[appgurueu]

I didn't removed flammable group, I just make it default to 1.

Not quite a "default" as it overrides whatever is in the def but yes, I overlooked that. Sorry.

[appgurueu]

Looks fine. PR title is misleading as the flowers are dropped, not destroyed.

[Desour]

• For consistency, this should also be done for grass and other shrubs, or not at all, imo.
• You might want to drop the drops and not the nodes themselves.
• It would be nice if the drop pos was randomized a bit.

[bartoszkosiorek]

• For consistency, this should also be done for grass and other shrubs, or not at all, imo.

Done

• You might want to drop the drops and not the nodes themselves.

All flooded things are producing drops.

• It would be nice if the drop pos was randomized a bit.

I would like to keep alghoritm simple and fast. Maybe it is not worth to implement that. Do you have example how it could be done?

[appgurueu]

I would like to keep alghoritm simple and fast. Maybe it is not worth to implement that. Do you have example how it could be done?

Performance impact is almost negligible. It might looks as follows:

local cube_size = 0.6
local offset = (1 - cube_size) / 2
local function randomize_drop_pos(pos)
    return vector.add(vector.add(vector.multiply(vector.new(math.random(), math.random(), math.random()), cube_size), offset), pos)
end

or using signed random (perhaps more readable?):

local cube_size = 0.6
local function sigrand()
    return 2*math.random() - 1
end
local function randomize_drop_pos(pos)
    return vector.add(vector.multiply(vector.new(sigrand(), sigrand(), sigrand()), cube_size), pos)
end

[appgurueu]

All flooded things are producing drops.

What @Desour was saying is that when you generalize this to all MTG plant nodes, you should be determining the dropped item over the node def "drop" field using minetest.get_node_drops(oldnode).

Which leaves me with an API suggestion: What about a default.def.floodable_drop function which extends & returns the node def by setting floodable = true and the appropriate on_flood func:

default.def = {}
local function drop_on_flood(pos, oldnode, newnode)
    for _, item in ipairs(minetest.get_node_drops(oldnode)) do
        minetest.add_item(randomize_drop_pos(pos), item)
    end
end
function default.def.floodable_drop(def)
    def.floodable = true
    def.on_flood = drop_on_flood
end

which you can then simply use in register_flower etc. if you want all flowers to be dropped on flooding.

[bartoszkosiorek]

Unfortunately after changing implementation to:

local function on_flood_drop_item(pos, oldnode, newnode)
	minetest.get_node_drops(oldnode)
end

or

local function on_flood_drop_item(pos, oldnode, newnode)
	minetest.get_node_drops(oldnode.name, "")
end

there is no more drops.

It seems that it is not initialized here.

[appgurueu]

Unfortunately after changing implementation to:

local function on_flood_drop_item(pos, oldnode, newnode)
	minetest.get_node_drops(oldnode)
end

or

local function on_flood_drop_item(pos, oldnode, newnode)
	minetest.get_node_drops(oldnode.name, "")
end

there is no more drops.

It seems that it is not initialized here.

You need to add the drops using add_item as I have shown in my second snippet.

[bartoszkosiorek]

Could you please test it with Lava and Water?
It seems that after ading randomization, sometimes the drops disapear under ground.

[appgurueu]

Could you please test it with Lava and Water?

Lava is interesting as it should both flood the crops and set them on fire; this might lead to a race condition if the ABM runs before on_flood I suppose. Perhaps destroy (and don't drop) the plant if the new node has the igniter group set? Shouldn't matter as the drops will be destroyed by the lava though.

It seems that after ading randomization, sometimes the drops disapear under ground.

It is possible that 0.4 extent (0.8³) is too large a size for the cube (plane). Try smaller values (say, 0.25 (0.4³ cube)).

[bartoszkosiorek]

Lava is interesting as it should both flood the crops and set them on fire; this might lead to a race condition if the ABM runs before on_flood I suppose. Perhaps destroy (and don't drop) the plant if the new node has the igniter group set? Shouldn't matter as the drops will be destroyed by the lava though.

From my testing it seems that crops are destroyed/dropped instantly, and there is no time for burn.
It also looks very realistic, as after few seconds, the drops are burned.

[appgurueu]

What about griefers? Should there be a minetest.is_protected check in the on_flood callback?

[bartoszkosiorek]

What about griefers? Should there be a minetest.is_protected check in the on_flood callback?

Unfortunately I am completely unexperienced with griefers. Could you please explain a little more how we should improve it?

[appgurueu]

Unfortunately I am completely unexperienced with griefers. Could you please explain a little more how we should improve it?

Say you have a protected area including a flower bed / mushroom farm / grass / tree farm (saplings). Griefers might now use liquids to destroy (flood) your beautiful farm as the liquids don't care about protection.

A simple fix might be checking for protection, but the problem here is that liquids carry no owner/source information, which would make crops entirely unfloodable in protected areas...

[bartoszkosiorek]

Ok, I get it.
The floodable items are waterlilly, butterflies, torch, fire, fireflies and snow. There is no any protection for it.
Is that mean that currently someone could easily destroy above by using water?
Maybe water should not flow in protected areas?

How I could set and test protected areas?

[appgurueu]

Ok, I get it. The floodable items are waterlilly, butterflies, torch, fire, fireflies and snow. There is no any protection for it. Is that mean that currently someone could easily destroy above by using water?

Yes.

Maybe water should not flow in protected areas?

Agreed, but rather hard to implement with the current state of the engine.

How I could set and test protected areas?

You need a protection mod such as simple_protection for that

[bartoszkosiorek]

I have disabled randomization, as it looks weird. Instead I would like to implement moving dropping by water flow. I will do that in next Pull Request.

[bartoszkosiorek]

I would not be worried about destroying flowers and grass with water by griefers. It could be easily avoided by putting some fence or wall etc.
Do you know if protected areas could be burnt?

[appgurueu]

Instead I would like to implement moving dropping by water flow. I will do that in next Pull Request.

Please think it through first! A clean implementation definitely requires an engine change (to obtain liquid flow direction), a hacky one requires a significant amount of code checking all flowingliquid neighbors for each "flowing" item every step, which I imagine might eventually be performance critical. If you're determined to implement this, have a look at the following func to determine liquid flowing dir.

Furthermore, this will require modifying the builtin item entity, which may be considered hacky as well (and should perhaps better be left to mods).

[bartoszkosiorek]

Instead I would like to implement moving dropping by water flow. I will do that in next Pull Request.

Please think it through first! A clean implementation definitely requires an engine change (to obtain liquid flow direction), a hacky one requires a significant amount of code checking all flowingliquid neighbors for each "flowing" item every step, which I imagine might eventually be performance critical. If you're determined to implement this, have a look at the following func to determine liquid flowing dir.

Thanks for advice. You have right that flowing could be too heavy for MTG, and it is betted to implement it in mods.

[Desour]

Protection checks seem unnecessary (and also weird).
Griefers can also let lava and water flow into your area to fill it with stone (if the server allows placing lava), or they can build up into the sky and let sand drop or build platforms to make shadow. If you want to protect your flowers, can build a trench or similar at the border of your protected area.

[bartoszkosiorek]

Do you think it is ready to merge this PR?

I have reached my personal goals with this implementation:

• simple implementation (easy to understand)
• easy to extend (it is an example for mods, how it could be implemented for other nodes)
• minor impact on performance

[appgurueu]

Please deduplicate the local on_flood_drop func by storing it in an appropriate node namespace (say, default.on_flood_drop in functions.lua). You may still localize it as on_flood_drop = default.on_flood_drop in the mods depending on it.

Not a fan of the on_flood and floodable code duplication in the defs (I would've preferred just passing the defs to a func which sets both, or setting both in the register_* funcs for these groups of nodes), but by MTG standards, the code duplication is acceptable, so my approval still stands.

[bartoszkosiorek]

Fixed duplication of on_flood_drop function.

How many approvals need to be get to be able to merge PR?

[appgurueu]

How many approvals need to be get to be able to merge PR?

You need two approvals.

[sfan5]

It's unfortunate to have to ask this question now that the PR is finished but: Since Minetest Game is not accepting new features, shouldn't this be rejected? (#2710 for more detail)
Worth noting that I think this would be a good addition, but no features means no features.

[bartoszkosiorek]

@sfan5 I think it is not a feature, as the flooding is working for example for torch and fire. It is just align the same behaviour with the rest of minetest_game.

[bartoszkosiorek]

I renamed the title to show what is the real issue with water/lava flow.
Could you please change the PR to Bug Fix?
Should I create a bug report?

[sfan5]

This was discussed in a coredev meeting we just had and the we weren't too sure either but the conclusion was that it is too close to being feature.
Consider making a mod, sorry.