Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pitch variations for most noteblock sounds #535

Merged
merged 4 commits into from Mar 18, 2023

Conversation

Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Sep 18, 2020

This PR adds pitch variations for almost all noteblock sounds with all semitones from C to B (like the piano).
Pitch is calculated with this formula:

pitch = 2^((val-6)/12)

Where val is a value from 0 (=C) to 11 (=B).

Piano is still using the custom piano sounds, but the other sounds use the Minetest pitch feature.

The fire sound (Lava Source) and explosion sound (Coal Block) still ignore pitch because these sound don't work well when pitched.
I changed the litecrash sound because it was the same as the crash sound but at a higher pitch. Since pitch is now

Finally, I made sure the noteblock is initialized at the C note instead C# when placed (using place_param2, so this change will be backwards-compatible).

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Mar 14, 2023

Bump.

mesecons_noteblock/init.lua Show resolved Hide resolved
mesecons_noteblock/init.lua Show resolved Hide resolved
mesecons_noteblock/init.lua Show resolved Hide resolved
mesecons_noteblock/init.lua Show resolved Hide resolved
Copy link
Contributor

@Desour Desour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pitch calculation is correct. (According to OpenAL docs, a factor of 2 means +12 semitones.)

You can use mesecons_noteblock_fsharp and mesecons_noteblock_fsharp2, and vary those by pitch, and drop all the other piano sounds (tested, it sounds exactly the same).
(Idk if other mods depend on the piano sounds, but I don't know of any. You can also just keep the files in the media folders (and not use them) for backwards compatibility.)

mesecons_noteblock/init.lua Show resolved Hide resolved
mesecons_noteblock/init.lua Show resolved Hide resolved
mesecons_noteblock/init.lua Outdated Show resolved Hide resolved
mesecons_noteblock/init.lua Show resolved Hide resolved
@Desour
Copy link
Contributor

Desour commented Mar 15, 2023

Oops, I haven't seen the other review until I submitted. Interesting how we found the same issues. x)

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Mar 15, 2023

I fixed most comments now.

I doubt it’s from C to B. That would mean the original sound is F#, but is it?

I'm not a music expert. How about testing it? The note order is not changed, it is the same as for the already existing piano sounds.

I've tested replacing the piano sounds with a direct pitch change but I think this sounds a bit worse. I think keeping the original piano sounds is better IMHO. It's not like these files take away that much disk space anyway. :P

@numberZero
Copy link
Contributor

I doubt it’s from C to B. That would mean the original sound is F#, but is it?

I'm not a music expert. How about testing it? The note order is not changed, it is the same as for the already existing piano sounds.

Thing is, while octaves and semitones are relative things (octave = 12 semitones = 2× frequency) and are applicable to all sounds, notes aren’t. They are absolute¹. So what the code does is not pitching the sound to C, C#, D, ..., B (which wouldn’t even make sense for some sounds), it is pitching it by 6 semitones down, 5 semitones down, 4 semitones down, ..., 5 semitones up.

¹ Relative to A1 technically, but it is almost always 440 Hz nowadays.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Mar 16, 2023

Thankfully, none of this is exposed to the player and is completely hidden behind the code.

So what do I have to do now to get this accepted? I don't know music theory and don't know what I must do next.

@numberZero
Copy link
Contributor

Remove this comment:
-- All semitones from C to B (analog to piano mode)
It’s likely possible to write a good one instead but that’s optional.

That aside, LGTM now.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Mar 18, 2023

Done.

@numberZero numberZero merged commit 54de66b into minetest-mods:master Mar 18, 2023
2 checks passed
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Mar 18, 2023

Nice! Here's a follow-up PR, adding many new noteblock sounds: #635

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants