-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[MU3] Add support for triple sharp and triple flat #6744
[MU3] Add support for triple sharp and triple flat #6744
Conversation
f81da9f
to
1ad39a3
Compare
I'm puzzled about those mtest failures in this PR here, they didn't happen in #6002? Edit: A couple rather stupid copy&paste errors were the culprit... |
f9a1d48
to
3e9f8db
Compare
9e9ac99
to
d59e771
Compare
@@ -198,15 +198,15 @@ AccidentalVal AccidentalState::accidentalVal(int line, bool &error) const | |||
|
|||
void AccidentalState::init(Key key) |
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.
There are very tricky calculations here
@Jojo-Schmitz @mattmcclinch Looks like you're studied this method, is it possible for you to add a comment that would explain what is going on here?
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.
Hmm, the changes of this PR don't seem complicated at all, they are just adjusted to the change number of accidentals (that affect playback in semitones), replacing 'magic numbers' by something more sensible and readable. The underling formula indeed looks quite complex, but I didn't even try to understand it ;-)
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.
See mattmcclinch@345042b. I think it adds a bit more clarity. The variable octave
is perhaps a bit misleading. octaveStart
might be better.
d59e771
to
851896a
Compare
SMuFL provides glyphs for triple sharp and triple flat, so they might as well be available to use in MuseScore. This involves adding 14 new TPCs and expanding the range of the AccidentalVal enum class.
This was requested on the forum topic in Triple sharps and triple flats (sort of, ignore the digressions into politics and key signatures).
See https://seanyeh.com/pages/alkans_triple_sharp/ for an example of a triple sharp being used in a piece of music from the Romantic period.
Rebased version of #6002 (@mattmcclinch, please review).
"If the mountain won't come to Mohammed, Mohammed must go to the mountain." ;-)