Play tool breaking sounds.

[sofar]

If a tool wears out and is destroyed, it's itemstack count
goes to 0, and we can optionally play a breaking sound.

This patch implements playing a breaking sound when this
occurs. Sounds need to be added to the tool itemdef
registration as the sound name string in the .sound_break
member.

[PilzAdam]

This should be in sound.break.

[est31]

Like the idea.

[sofar]

Changed to tooldef.sound.break

[sofar]

edit: also updated lua_api.txt to reflect the submember location.

[sofar]

Note I have added 3 sound effects as well in minetest_game.

[sofar]

yeah This doesn't even run for me... break is a lua reserved word?

[sofar]

wdef.sound.breaks then, I suppose

[sofar]

For consistency, adjusted the sound names and names of sound files as well in the minetest_game PR.

[sofar]

@est31 also, this properly tests for wdef.sound

[est31]

👍

[PilzAdam]

The sound table is already present 10 lines below. Please move breaks into it.

[sofar]

ahh yes, I didn't even know there was a place sound as well :)

[BlockMen]

Can be done in subgames or by mods, no need to add this to core. 👎

[rubenwardy]

👍 for concept, however, isn't the tool worn elsewhere? It should be consistent. Maybe it should be in the place that sets count to zero after the tool is worn.

[sofar]

@BlockMen To do this efficiently in mods would require to override the on_wear() of every single tool that you'd want to attach this sound effect to. That's dumb, and not something mods should ever attempt to do.

[sofar]

@rubenwardy This is the place in lua where the count gets set to zero, and the tool is removed from the inventory. (C++ does the taking, hence the code here detects that that has occurred).

[rubenwardy]

What about weapons, get worn on punch. And other wear additions.

[sofar]

@rubenwardy The only occurrence of :add_wear() is in this section. The only place in C++ where addWear() is called, is the function that gets called if the lua method :add_wear() get called.

So hitting a player with a sword will not wear the sword, currently. I can't confirm this at the moment, as hitting myself in the office with a wooden sword would look kind of odd. (This would also be a separate bug, obviously).

[PilzAdam]

I think @BlockMen raises a valid point here. Why should this be done in core?

Also, @sofar, there is no on_wear callback for tools yet. We could add on_wear(old_wear, new_wear) to itemdef, and minetest.register_on_wear(tool, old_wear, new_wear). It would make it rather trivial for games to add their own sounds then.

[sofar]

@PilzAdam I'm up for solutions, but without a callback there is no way to determine right now in the game that an entity has been hit with a sword, except by coding all the entities and adding damage handlers that also wear out the sword. It seems to me that an implementation that forces the mod writers to call tool:add_wear() inside the entities:on_punch is anything but a structurally good approach.

(If it wants to add more wear, or refund some wear, sure, that would be a special circumstance, and the entitiy code already can do that)

So something needs to be added in core, this can't be done in minetest_game. At least I don't see how.

Even if we'd add an on_wear callback, we still have the problem that currently only digging causes it to add_wear, so yet again something in core needs to be changed to allow swords to wear down when hitting a player, or an entity.

[nerzhul]

👍

[sofar]

I count two 👍 from core devs.

[est31]

And one 👎 . So it needs one additional 👍 , no?

[0-afflatus]

I want to see an on_wear callback as well as something like a on_punchentity.
The sounds and in-game stuff should then be softcodeable.

[paramat]

Hm it almost has 2 disapprovals, so perhaps discussion should continue a little longer.

[kilbith]

I think @est31 were joking, as he has already approved earlier. I don't see a reason for his disapproval either.

[est31]

I wasn't joking nor disapproving. I just counted the 👍 and 👎 's and found the following dev votes:

• 👍 by me
• 👎 by @BlockMen
• 👍 by @nerzhul

The 👎 I talked about was blockmen's.

[sofar]

There already is a after_use() callback if mods want to intercept wear, item breakage and the playback sound:

    if wdef and wdef.after_use then
        wielded = wdef.after_use(wielded, digger, node, dp) or wielded
    else
        -- wear handling code be here

So, while @BlockMen is complaining that it can be done in subgames, it completely fails to acknowledge that if that indeed is the major concern, it forces us to to add after_use() callbacks to every minetest_game tool to add tool breaking sounds in minetest_game.

This patch does not remove the ability of ~~~subgames~~~mods to override the sound - it makes that trivially easy, one can even just override the tooldef sounds, or even attach after_use() methods. That's 2 simple ways to override breakage sounds.

So, politely, @BlockMen s disaproval is short-sighted, and this patch adds functionality that makes it significantly easier for mod writers to add tool breaking sounds, and especially, it allows mintest_game to trivially add tool breaking sounds as well.

[0-afflatus]

Since the disapproval was BlockMen's, core devs need to make an executive decision, which looks to me like PilzAdam's reservations need to be addressed.

[C1ffisme]

Didn't BlockMen leave Minetest?

It seems we have our two approvals, and an expired disapproval. Although, I would prefer an on_wear or on_tool_break callback, though IMHO.

[0-afflatus]

This isn't a numbers game @C1ffisme - it's a question of ascertaining whether the issues raised are still valid.

[paramat]

Well i think this can probably go ahead unless there are objections.

[paramat]

Minor edit needed, then it can be merged.

[sofar]

Updated, thanks!

[est31]

it's a question of ascertaining whether the issues raised are still valid.

I agree with @0-afflatus on this.

[paramat]

@PilzAdam any objections to merge?

[sofar]

I'm obviously somewhat biased on the discussion, but here's how I felt the arguments played out technically:

The main argument was: this patch isn't generic enough.

The alternative solution proposes to make a function that essentially has a single use (play a sound when a tool breaks).

While there may be some use for it, there are arguments against that as well: (a) there's already a far more generic event that can be used to do exactly that, (b) essentially the only use for it in minetest_game will always and forever likely be to play the breaking sound.

Now, from a sound perspective, the objection has been that clients are playing the sounds, not the server through lua.

My problem with that is that (1) that's almost irrelevant from a mod perspective, and (2) it's against what I think is the right place and way to encode sound properties of a craftitem, which is obviously a "property", and not an event handler.

Now, we can't play tool breaking sounds in the client since the client has no way to identify whether a stack went to 0(zero) count because something moved the thing in there, or because the server broke it, so then it would be appropriate for the server code to tell the client to play a breaking sound, right? But that's exactly what this patch does, except it's in lua, and not C++.

Then there's an argument for taking this patch, which enables without obstructing further enhancement to convert the current code into what was alternatively proposed.

The argument that was brought up that the after_use() event is too complex is a bit nearsighted, as the code needed to wear out is 1 line in C++, and could easily be done in lua IMHO.

To me, at the end, I still favor this simple approach. There are a lot of mods that will want to add sound effects to their versions of tools, and I think this is the easiest method.

[PilzAdam]

I think there should be a default after_use for itemdefs that wears out the tool and, if needed, plays the tool breaking sound. This default function should be overrideable by mods, so they can easily disable tool breaking sounds.

[sofar]

If you prefer that all tools have the same breaking sounds, then we should implement an after_use callback that is by default attached to every tool, and thus you convert every tool to have breaking sounds.

However, I think that doing so isn't appropriate, and this PR doesn't attach a tool breaking sound to any node by default.

There are no default node sounds for most nodes, so, similarly, I don't think there should be default tool breaking sounds for tools.

[PilzAdam]

@sofar I'm not saying that all tools should have the same sound. There still should be sound.breaks in the tooldef.

[infmagic2047]

Will this only work when digging nodes? If that is the case, an on_wear callback is probably better because it is also called when tools are used elsewhere.

[sofar]

@PilzAdam, the problem I have is requiring everyone to include 2 lines instead of one, for every tooldef, to enable sounds.

First you require on_some_event = default.play_tool_break_sound,, which is completely new, and not needed for other existing sounds, plus it's odd to have a function in default for playing one type of sound.

Then you still require sound = { break = "default_tool_break" },.

All I'm saying is, let's do the second part and omit the awkward function callback for now. If it really is such a problem and people revolt, not much is lost by adding it later.

[PilzAdam]

First you require on_some_event = default.play_tool_break_sound,, which is completely new, and not needed for other existing sounds, plus it's odd to have a function in default for playing one type of sound.

This would be set by builtin. Same concept as e.g. for on_drop; builtin defines a default function, and mods can override it if they want extra functionality.

[kwolekr]

👍

[paramat]

Has 2 approvals but sofar may update this, so wait until after release? @sofar

[sofar]

Let's revisit this after the release.

[paramat]

@sofar

[Ferk]

@PilzAdam

I think there should be a default after_use for itemdefs that wears out the tool and, if needed, plays the tool breaking sound. This default function should be overrideable by mods, so they can easily disable tool breaking sounds.

Seeing the code it actually looks like that's basically what's already doing. Just that instead of using a default function, it checks if the function is defined and if not it performs the default action directly there, saving a function call, which might be more efficient.

Well, having the default action inside a function has an advantage if you want to expose it for mods to use directly. But I'm not sure if it's useful enough to matter much.

[rubenwardy]

👍
I think that sounds.break is the most API friendly way to do this, and is consistent with other sounds

[paramat]

Well this has 5 approvals now, and blockmen and @PilzAdam are no longer active, any final objections to a merge?
@sofar is this good for merge?

[paramat]

4a0a672