Add lava environment sound

[sfan5]

env_sounds_lava.1.ogg taken directly from this ZIP: https://forum.minetest.net/viewtopic.php?p=151166#p151166
attribution for the sound was found here
env_sounds_lava.2.ogg cut from the source myself

[paramat]

Having more sounds was actually my original intention, and my early experimental version of this code had lava sounds, but the sounds were reduced to water only in order to wait for an improved minetest.find_nodes_in_area() API that can search for multiple nodes in a single search.

A problem with the approach of this PR is that the somewhat intensive search has to be repeated for each sound, and it is already being repeated for every player.
Perhaps it is better we create a better 'find nodes' API first and do this a different way?

See:
#864 (comment)
Last part of #2064 (comment)
#2064 (comment)
#2064 (comment)

[sfan5]

Has this been benchmarked?
I doubt that find_nodes_in_area is (too) slow in itself and that 1 call vs. 2 API calls makes a relevant difference. This would of course matter if more sounds are to be added in the future.

wait for an improved minetest.find_nodes_in_area() API that can search for multiple nodes in a single search.

This could actually be done with a single call to find_nodes_in_area. Then you'd either lose correct sound positioning if both lava and water appear or have to call get_node to determine which nodes are actually where.

Perhaps it is better we create a better 'find nodes' API first and do this a different way?

A better API would be if instead of returning

pos = {{x=1, y=2, z=3}, {x=2, y=3, z=4}, ...}
counts = { ["default:lava_flowing"] = 123, ["default:water_source"] = 16 }

find_nodes_in_area returned

pos = {
    ["default:lava_flowing"] = {{x=1, y=2, z=3}, {x=1, y=2, z=33}, ...},
    ["default:water_source"] = {{x=2, y=3, z=4}, {x=2, y=3, z=44}, ...}
}

This isn't hard to implement at all and could even be done "on time" for this PR.

[paramat]

minetest.find_nodes_in_area() can already search for multiple nodes, but places all the position data together instead of separated out for each node, so as you wrote, sound positioning would not work.

Because the APi is already searching for multiple nodes and recording their positions, but just not separating the positions, it seems obvious we should add a new and similar API that simply separates the positions, because this would come at near-zero cost. It would be silly not to do this and repeatedly use minetest.find_nodes_in_area() instead.

A better API would be [...]

Yes that is exactly what i suggest, i would approve that.
I guess the node counts could still be fetched from the lengths of the position tables.

[ghost]

Your cyclic timer is global (and hence equivalent to register_globalstep), and not per-player in case you're interested. On Sun, May 17, 2020 at 8:07 AM James Stevenson <[email protected]> wrote:

…

No ephemeral tag? On Sat, May 16, 2020 at 10:12 PM Paramat ***@***.***> wrote: > minetest.find_nodes_in_area() can already search for multiple nodes, but > places all the position data together instead of separated out for each > node, so as you wrote, sound positioning would not work. > > Because the APi is already searching for multiple nodes and recording > their positions, but just not separating the positions, it seems obvious we > should add a new API that simply separates the positions, because this > would come at near-zero cost. It would be silly not to do this and > repeatedly use minetest.find_nodes_in_area() instead. > > A better API would be [...] > > Yes that is exactly what i suggest, i would approve that. > I guess the node counts could still be fetched from the lengths of the > position tables. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#2683 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AFS4UYQDG2Y5PXALKB5W2ADRR5BZ7ANCNFSM4NCT4LVA> > . >

[sfan5]

Quick benchmark done by moving the timer into a globalstep (since the profiler can't profile minetest.after):

 instrumentation            |    min µs |    max µs |    avg µs | min % | max % | avg %
  - globalstep[1] ........  |         1 |       840 |         7 |   0.0 |  97.1 |   6.1  (two sounds defined)
  - globalstep[1] ........  |         1 |       476 |         6 |   0.1 |  95.8 |   6.6  (one sound defined)

As expected the max time almost doubled, average time barely changed but this likely due to most calls not doing anything except incrementing the timer.

[sfan5]

With the new implementation and two sounds:

 instrumentation             |    min µs |    max µs |    avg µs | min % | max % | avg %
  - globalstep[1] .........  |         1 |       485 |         4 |   0.1 |  96.0 |   6.5

Unfortunately the code now looks like ass.

[paramat]

Nice benchmarking results.
This approach with a new engine API seems good to me.

[SmallJoker]

Works well, sounds good.