Add on_flood() callback. #5619

sofar · 2017-04-20T06:16:23Z

This callback is called if a liquid definitely floods a non-air
node on the map. The callback arguments are (pos, oldnode, newnode)
and can return a bool value indicating whether flooding the
node should be cancelled (return true will prevent the node
from flooding).

Documentation is added, the callback function was tested with a
modified minetest_game.

Note that return true will likely cause the node's on_flood()
callback to be called every second until the node gets removed,
so care must be taken to prevent many callbacks from using this
return value. The current default liquid update interval is 1.0
seconds, which isn't unmanageable.

The larger aim of this patch is to remove the lava cooling ABM,
which is a significant cost to idle servers that have lava on their
map. This callback will be much more efficient.

sofar · 2017-04-20T06:17:01Z

Parts of this PR came from #4130

raymoo · 2017-04-20T06:18:26Z

Will lava still be able to be cooled by water that is below it with this?

sofar · 2017-04-20T06:26:34Z

lava flowing on top of water floods the water node below the lava. The water doesn't magically jump up one node.

raymoo · 2017-04-20T06:28:50Z

Oh, I see, I was only thinking of water doing the flooding.

nerzhul · 2017-04-20T08:28:25Z

src/map.cpp

@@ -637,7 +638,8 @@ s32 Map::transforming_liquid_size() {
        return m_transforming_liquid.size();
 }

-void Map::transformLiquids(std::map<v3s16, MapBlock*> &modified_blocks)
+void Map::transformLiquids(std::map<v3s16, MapBlock*> &modified_blocks,
+		ServerEnvironment& env)


ServerEnv should be passed as pointer like everywhere

nerzhul

Just one change, else perfect shipit

Desour · 2017-04-20T09:17:05Z

A ~~point~~flooded_thing could be useful to know from where the liquid comes.

raymoo · 2017-04-20T13:49:54Z

@DS-Minetest That wouldn't make sense since there isn't any pointing involved, but I guess the callback could receive a flooder position argument too.

paramat · 2017-04-20T14:19:43Z

doc/lua_api.txt

@@ -3974,6 +3974,12 @@ Definition tables
        ^ Node destructor; called after removing node
        ^ Not called for bulk node placement (i.e. schematics and VoxelManip)
        ^ default: nil ]]
+        on_flood = func(pos, oldnode, newnode), --[[
+        ^ Called when a liquid (newnode) is about to flood oldnode.


Should make clear it is called only for non-air nodes.

Perhaps mention 'floodable = true' here, "called for non-air nodes with 'floodable = true'".

paramat · 2017-04-20T14:22:52Z

Good to see this being done at last.
Could you explain the usage of 'return true' stopping the node being flooded? I can only see this causing unending repeated callbacks, which should be avoided, how would we avoid this if the node is not flooded?

paramat · 2017-04-20T14:43:12Z

No need for flooder pos, this should be as simple as possible.

Ekdohibs · 2017-04-20T14:53:00Z

@paramat it causes repeated callbacks, but only 1/s and this is really not that much, except if you really have lots of nodes preventing flooding that way. If we had some nodetimer-based system or something like that based on node updates, it would avoid those repeated callbacks as well as allowing to make the water flow faster as was requested in another issue, for instance.

Ekdohibs

Otherwise, looks good!

Ekdohibs · 2017-04-20T14:58:30Z

src/map.cpp

+
+		// on_flood() the node
+		if (floodable_node != CONTENT_AIR) {
+			if (env.getScriptIface()->node_on_flood(p0, n0, new_node_content))


The callback is called after n0.param2 is modified. That means that the param2 of n0 will already be the new param2, which can cause problems with coloured nodes, for instance (like the node dropping with a different colour than it should).

paramat · 2017-04-20T15:13:07Z

it causes repeated callbacks, but only 1/s and this is really not that much,

I agree, but it seems to cause a never-ending series of calls, which is a bad idea even if only 1 per second. So i'm wondering if this should not be encouraged / allowed. However there may be a good reason to allow this.

sofar · 2017-04-20T21:58:03Z

One reason to allow cancellation is that it could permit a security mod to effectively and retroactively mop up a hostile liquid spill.

This mopping up requires us to stop the flooding. If we don't stop flooding the node, then the callback here will require hacks like minetest.after(0, ...set_node, oldnode) or similar tricks depending on timing workarounds to avoid the flooding. That will turn out to be very fragile and problematic in itself.

With the return value honored, we can allow protection mods to delete vertical columns of lava or water relatively safely and not cause performance spikes.

You could also use this feature to safely change the liquid into a "non-flooding" liquid that just stops right there as well.

So even though it won't be used much, or shouldn't be used much, it serves a useful purpose.

sofar · 2017-04-20T22:01:37Z

I'm gonna try and see if I can replace n0 with e.g. n00 in the call and get the proper param2 in place here, but it'll have to wait until tonight when I can test (->WIP)

raymoo · 2017-04-20T22:10:25Z

@sofar For that particular case you could make the on_flood callback be called after the flooding, and then it would be a good time to put the security node back in, so no need for minetest.after. It might cause problems if a node has metadata that should be preserved, though.

paramat · 2017-04-20T22:23:32Z

Ok i'm neutral on the return true behaviour.
Very keen on removing the lava ABM.

sofar · 2017-04-20T22:35:36Z

@raymoo right, that's why I don't want to wait until the metadata is destroyed.

This callback is called if a liquid definitely floods a non-air node on the map. The callback arguments are (pos, oldnode, newnode) and can return a `bool` value indicating whether flooding the node should be cancelled (`return true` will prevent the node from flooding). Documentation is added, the callback function was tested with a modified minetest_game. Note that `return true` will likely cause the node's `on_flood()` callback to be called every second until the node gets removed, so care must be taken to prevent many callbacks from using this return value. The current default liquid update interval is 1.0 seconds, which isn't unmanageable. The larger aim of this patch is to remove the lava cooling ABM, which is a significant cost to idle servers that have lava on their map. This callback will be much more efficient.

sofar · 2017-04-21T05:15:54Z

Pushed what I feel should be the final update.

@Ekdohibs what seems to happen is that the param2 of every node that gets newly flooded is by definition 0, until later the engine raises the liquid level to a higher value in subsequent liquidtransform calls and the liquid becomes stable.

So, per definition the param2 of newnode will always be zero.

Because of the ambiguity of the liquid direction, I think it's false to try and include a liquid direction. If one needs to see if the node above equals the liquid in newnode, one could just do a quick get_node and decide based on that, which would be relatively quick.

Ekdohibs · 2017-04-21T06:08:56Z

@sofar n0.param2 = (flowing_down ? LIQUID_FLOW_DOWN_MASK : 0x00) | (new_node_level & LIQUID_LEVEL_MASK); for me it looks like the param2 is not always zero... however, any behaviour is fine with me as long as it is properly documented.

sofar · 2017-04-21T06:12:04Z

@Ekdohibs I think that case only happens if the liquid will be flowing down through the node entirely, like a waterfall. I have not been able to confirm that, and in all my testing of flooding e.g. flowers and other stuff, I was only ever getting 0.

Ekdohibs · 2017-04-21T06:34:42Z

@sofar well, as long as the param2 given is the one that is used to flood, I think it is ok, regardless of what it is.

sofar · 2017-04-21T16:47:02Z

@Ekdohibs right, that's why I'm convinced that this patch is correct as-is.

env pointer seems done.

paramat · 2017-04-21T19:21:43Z

@Ekdohibs seems to be +1?

paramat · 2017-04-21T19:23:07Z

Docs are ok for me now.

paramat · 2017-04-22T00:30:36Z

cca58fe

sofar added @ Script API @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature labels Apr 20, 2017

sofar added this to the 0.4.16 milestone Apr 20, 2017

nerzhul reviewed Apr 20, 2017

View reviewed changes

nerzhul previously requested changes Apr 20, 2017

View reviewed changes

paramat reviewed Apr 20, 2017

View reviewed changes

Ekdohibs requested changes Apr 20, 2017

View reviewed changes

paramat closed this Apr 22, 2017

paramat mentioned this pull request Apr 22, 2017

Water Physics #47

Closed

sofar deleted the on_flood branch April 24, 2017 18:00

nerzhul added this to Done in Minetest 0.4.16 Apr 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add on_flood() callback. #5619

Add on_flood() callback. #5619

sofar commented Apr 20, 2017 •

edited

sofar commented Apr 20, 2017

raymoo commented Apr 20, 2017

sofar commented Apr 20, 2017

raymoo commented Apr 20, 2017

nerzhul Apr 20, 2017

sofar Apr 20, 2017

nerzhul left a comment

Desour commented Apr 20, 2017 •

edited

raymoo commented Apr 20, 2017

paramat Apr 20, 2017

paramat Apr 20, 2017 •

edited

sofar Apr 20, 2017

paramat commented Apr 20, 2017

paramat commented Apr 20, 2017

Ekdohibs commented Apr 20, 2017

Ekdohibs left a comment

Ekdohibs Apr 20, 2017

paramat commented Apr 20, 2017 •

edited

sofar commented Apr 20, 2017

sofar commented Apr 20, 2017

raymoo commented Apr 20, 2017

paramat commented Apr 20, 2017 •

edited

sofar commented Apr 20, 2017

sofar commented Apr 21, 2017

Ekdohibs commented Apr 21, 2017

sofar commented Apr 21, 2017

Ekdohibs commented Apr 21, 2017

sofar commented Apr 21, 2017

paramat commented Apr 21, 2017

paramat commented Apr 21, 2017

paramat commented Apr 22, 2017

Add on_flood() callback. #5619

Add on_flood() callback. #5619

Conversation

sofar commented Apr 20, 2017 • edited

sofar commented Apr 20, 2017

raymoo commented Apr 20, 2017

sofar commented Apr 20, 2017

raymoo commented Apr 20, 2017

nerzhul Apr 20, 2017

Choose a reason for hiding this comment

sofar Apr 20, 2017

Choose a reason for hiding this comment

nerzhul left a comment

Choose a reason for hiding this comment

Desour commented Apr 20, 2017 • edited

raymoo commented Apr 20, 2017

paramat Apr 20, 2017

Choose a reason for hiding this comment

paramat Apr 20, 2017 • edited

Choose a reason for hiding this comment

sofar Apr 20, 2017

Choose a reason for hiding this comment

paramat commented Apr 20, 2017

paramat commented Apr 20, 2017

Ekdohibs commented Apr 20, 2017

Ekdohibs left a comment

Choose a reason for hiding this comment

Ekdohibs Apr 20, 2017

Choose a reason for hiding this comment

paramat commented Apr 20, 2017 • edited

sofar commented Apr 20, 2017

sofar commented Apr 20, 2017

raymoo commented Apr 20, 2017

paramat commented Apr 20, 2017 • edited

sofar commented Apr 20, 2017

sofar commented Apr 21, 2017

Ekdohibs commented Apr 21, 2017

sofar commented Apr 21, 2017

Ekdohibs commented Apr 21, 2017

sofar commented Apr 21, 2017

paramat commented Apr 21, 2017

paramat commented Apr 21, 2017

paramat commented Apr 22, 2017

sofar commented Apr 20, 2017 •

edited

Desour commented Apr 20, 2017 •

edited

paramat Apr 20, 2017 •

edited

paramat commented Apr 20, 2017 •

edited

paramat commented Apr 20, 2017 •

edited