-
Notifications
You must be signed in to change notification settings - Fork 120
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
Conversation
Bump. |
There was a problem hiding this 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.)
Oops, I haven't seen the other review until I submitted. Interesting how we found the same issues. x) |
I fixed most comments now.
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 |
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. |
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. |
Remove this comment: That aside, LGTM now. |
Done. |
Nice! Here's a follow-up PR, adding many new noteblock sounds: #635 |
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:
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).