Make sound enhancement - Githubissues

NetherEran commented 4 years ago

I made mobkit.make_sound accept tables in a mob's sound table in addition to strings.

These allow setting custom pitch, gain, fade, max_hear_distance and loop values to play the sound with. Apart from loop, either numbers or ranges can be specified. If a range is specified, a random number is picked within it each time the sound is played. Using a range for pitch is really useful to add variation to sounds.

Just like animations, it supports picking random sound tables. Minetest has a randomizing feature for sounds already but doing it this way allows linking sounds with specific gain/pitch values and so on. It could also be used to make sound groups that only fit together in certain situations.

TheTermos commented 4 years ago

I'm still new to this git thing, and got a quite hefty update to push, can we deal with it afer? The update is local only atm and I'm just not sure how to go about merging two concurrent pulls.

NetherEran commented 4 years ago

Yes, you can wait. If there is no chance of breakage, git will merge automatically. Otherwise it will tell you that it can't merge automatically. In this case I'll rebase this and resolve the merge conflicts.

TheTermos commented 4 years ago

Now the update is out I'm gonna look into this.

Handling sound in MT is unnecessarily difficult because documentation is not clear about it, so it may take time.

One thing I noticed right away, the table thing should work already as the first argument to sound_play is SimpleSoundSpec which is supposed to be either a string or a table, I just need to find out which parameters are supported.

NetherEran commented 4 years ago

One thing I noticed right away, the table thing should work already

Yeah, but that way I don't think there is a way to randomize values. Wanting to randomize pitch is the main reason I made this PR.

Edit: actually there is using the __index field in a metatable.

TheTermos commented 4 years ago

__index only fires when there's no corresponding key in the table. This can be worked around by proxy but this is where it gets too complex for my taste. Better just to pass a modified table copy instead of the original.

I'd like to stick with simplesoundspec for now because it does not take the loop parameter. Looping sounds require managing handles so let's leave it for later and go with the simplest way that allows pitch modulation.

Apart from that, I'll probably need some other things changed, how do we go about this, do I just write it here or file some PR for your PR ;)

NetherEran commented 4 years ago

do I just write it here or file some PR for your PR ;)

You can do either, but you don't have to write a PR, I sent an invite for push acces to my repo.

TheTermos commented 4 years ago

I'm not receiving any notifications nor invites for some reason, so I'll just write here.

I enclose an attachment with my take on the problem, it's untested code so forgive me if there's anything stupid, please let me know if you agree with the direction. make_sound.zip

My reasons:

went with simplesound as I said earlier, it doesnt support hearingdistance and loop, I hope we deal with those later when I figure out the best way to manage handles.
I'm not a fan of random_within_range() because the call is longer than the formula
I'm not a fan of having too many variables if I can avoid it.
I'm not a fan of keyed tables with meaningless keys, so for ranges went with arrays instead.

NetherEran commented 4 years ago

Appearently invites aren't sent automatically, it's kind of confusing. Anyways:

I'm not a fan of random_within_range() because the call is longer than the formula

The point of using functions is to avoid code duplication so you only have to debug it once. It's also more readable that way.

I'm not a fan of keyed tables with meaningless keys, so for ranges went with arrays instead.

The only reason I went with x and y for keys is for consistency with animations. Arrays are completely fine aswell though.

Two bugs I found in your code: It returns without playing the sound if the sound spec is a string. It also overwrites the range array of the spec when picking a random value. I fixed them by removing the return statement and making a copy of the spec before picking random values.

A bigger problem is that minetest doesn't actually seem to use the pitch field of a SimpleSoundSpec. I'll propably open an issue on the engine's github for this after I've done some more testing.

For this PR should I just use the sound parameter table again since it's propably going to take a while to resolve? IMO having to copy the SimpleSoundSpec does lower the difference in elegance anyways.

TheTermos commented 4 years ago

Actually it is worse, even if used in sound parameter table the pitch parameter doesn't work as it should - it affects playback speed instead, I can't think of any uses for this feature.

My ad hoc code was almost fine, one thing missing was spec=table.copy(spec) just before if #spec, too bad it's useless due to defunct soundspec.

For this PR should I just use the sound parameter table again since it's propably going to take a while to resolve? IMO having to copy the SimpleSoundSpec does lower the difference in elegance anyways.

If you still think broken pitch modulation is somehow useful, yes. Would be nice if it was as concise as possible, my internal parser seems to see it as more elegant that way.

NetherEran commented 4 years ago

Alright I've updated the PR. Is it okay like this?

TheTermos / mobkit

Make sound enhancement #7