Concert Pitch switching does not preserve key spelling if the transposition crosses between sharp and flat keys

I was actually about to have a look at a bug you reported three years ago http://musescore.org/en/node/6281 that I ran into myself testing out some new code. They might be related. I'll keep a lookout as I go through the code.

Mike

This bugged me, esp when writing in sharp keys with instruments that are flat key transposing (primarily Eb/Bb Saxes & Trumpets), because the final parts becomes effectively unreadable. I'm just making a note that this bug is still present in 75b66fd which is the latest 2.1 testing nightly.

I'm fairly certain the cause is that enum Key only goes from -7 (Cb) to +7 (C#): https://github.com/musescore/MuseScore/blob/2.1/libmscore/key.h#L27-L36

(There is the function KeySig::enforceLimits(), which ensures that they key is within -7 and +7 limits: https://github.com/musescore/MuseScore/blob/2.1/libmscore/key.cpp#L34-L… but searching github codebase reveals that enforceLimits() is never called anywhere. Seems like that function should be deleted from the code.)

Without looking deeper into the code, I'm sure that somewhere else in the codebase MuseScore is enforcing this -7 to +7 limit. And that enforcement will cause a problem when toggling between concert pitch (for both flat key transposing instruments written in sharp concert keys, as well as for sharp key transposing instruments written in flat concert keys), since the change of key signature is not a reversible operation.

Now I see two possible ways to resolve:

1. Allow for key signatures that contain double flats and double sharps. These are technically known as "Theoretical Keys" https://en.wikipedia.org/wiki/Theoretical_key and they are actually notated on rare occasion: "The final pages of John Foulds' A World Requiem are written in G-sharp major (with Fdouble sharp in the key signature), and the third movement of Victor Ewald's Brass Quintet Op. 8 is written in F-flat major (with Bdouble flat in the key signature)". Seem useful when near the bottom of the circle of fifths when modulating to another nearby key on the other side of the circle. I think would be useful when writing for transposing instruments such that when user toggles concert pitch, the transposed part will remain in the flat realm if was in a flat concert key, or will remain in sharp realm if was in a sharp concert key, instead of switching realms. Some score writers may prefer to stay in thinking in the same "realm" when toggling concert pitch.
2. Keep the current behavior of keysig elements in staffs of transposing instruments flipping between sharp and flat keys when exceeding the limit of permitted key signatures, but have the keysig elements *remember* what concert pitch key signature they were originally assigned. I'll suggest two ways to implement:
   • a.) One way I would implement this is to only store the concert pitch integer value of the keysig in each KeySig element's KeySigEvent _key for each staff instead of the transposed integer value. So for instance if concert pitch is currently untoggled, and I drop an F# Major keysig onto the staff of a Bb instrument, then instead of storing the transposed value of Ab Major (enum -4), I would store as the concert pitch enum value for B Major (enum 5). Then when concert pitch is toggled ON, that _key will remain as B Major (enum 5). And then if concert pitch is toggled OFF, that _key will remain stored as enum 5. I would add a wrapper function around the KeySigEvent which would return the transposed key according to the current value of StyleIdx::concertPitch. So transposeKeys() wouldn't be called unless actually performing an Edit->Transpose (that means I would remove the line calling transposeKeys() in places like Score::cmdConcertPitchChanged() and Excerpt::createExcerpt()). Now the big disadvantage of this method is that now the file format has changed in a significant way since the key signature integer is representing the concert pitch key sig, not the transposed key sig...maybe that is too big of a change to be accepted by others. Also I don't know the best place to put such a wrapper function...if put inside of KeySig, that would make class KeySig be dependent on its parent score just for reading the state of StyleIdx::ConcertPitch, which I don't think is desired.
   • b.) Alternatively instead of storing the transposed value of the keysignature, allow enum class Key to contain ints below -7 and above +7, but interpret ints beyond the limit to mean that transposing from concert pitch has caused the key sig to change between flat/sharp key. The advantage of this is that the file format wouldn't change other than allow for this keysig int to hold values beyond the limit if the staff is a transposing instrument. (Note: if user then ends up doing a regular Edit->Transpose, then I suppose the transpose function might then force the keysig int to be within the limit and so might "forget" what realm original key sig was in.) Would have KeySigEvent::key() be modified to work as the wrapper function, such that instead of just returning _key would return the aliased key if the stored value was outside of the limit. An advantage over 2a is that can probably implement without having class KeySigEvent or class KeySig depend upon class Score for the state of StyleIdx::ConcertPitch. Another advantage is that this allows for the future option of "Theoretical Keys" whereby have a boolean option in preferences that says whether or not to make transposing instruments switch between flat & sharp realms when exceed the limits.

I think both options are valid, and I think MuseScore should allow for both with option in preferences. I think could do 2b safely with minimal changes.

There is another semi-serious concern I want to bring up, but which might be implemented in a seperate issue. Currently when toggling concert pitch, I've discovered that even though the key signature might flip between the sharp/flat realms, that unfortunately the transposion of the individual notes are not obeying that flip. For instance if write an F# Major scale in key of F# Major for a Bb & Eb instrument while in ConcertPitch:

Then after I toggle Concert Pitch OFF, I unfortuantely get this confusing mess:

Same mess for the individual part ("excerpt") child scores. So clearly if MuseScore is having key signature flip realms, then the transposition of the notes and harmonies needs to be aware of this flip.

I'm trying to implement 2b now...

The last comment about having notes and harmonies obey the flip, seems like need to handle this in Score::cmdConcertPitchChanged(), Excerpt::createExcerpt(), and Score:: Score::transpositionChanged(), which all call Score::transposeKeys()...would be nice to somehow merge the logic handling that into one function so easier to ensure consistent behavior...

I wanted to add another related corollary minor bug occuring when *changing a staff to a different transposing instrument* while ConcertPitch is OFF:

1. Create new default score (single staff of concert pitch piano).
2. Drag F# major (6 sharps) key signature to first measure.
3. Change that instrument to a Bb transposing instrument (by right clicking the first measure, selecting Staff Properties, pressing Change Instrument, and selecting a Bb inst, and pressing Ok/Apply).
4. Observe that key sig changed from F# Major to Ab Major.
5. Now change that staff's instrument back to a concert pitched instrument like piano.
6. Observe that we are now in Gb Major

Expected behavior: The key should have returned back to F# major (6 sharps).
Actual behavior: The key morphed into Gb major (6 flats).

Also note that if I originally started with notes, then upon doing these transformations they similar get messed up just as I described at the end of comment #5:

While this is honestly not as serious as the originally described bug, I think it makes sense to fix it at the same time since the code changes are very related, although these changes would involve Score::transpositionChanged().

(I guess a similar issue would also occur when adding Change Instrument text element before a keysignature and then deleting that key signature. Although the Change Instrument text change doesn't seem to be doing anything in 2.1)

BTW, if you haven't thought of this already, keep in mind #39176: Option to convert transposed instrument key signatures into enharmonic equivalent when number of accidentals exceeds limit. Ideally there should be some simple control over how the transposition affects keys in the first place.

Also, the behavior you show where changing the key from sharps to flats or vice versa doesn't affect the pitches is perfectly normal taken at face value. That is, even in music for piano, if you enter notes in the key of F# and then change the key signature to Gb, it doesn't and shouldn't automatically respell the notes. If you *manually* change keys, you need to *manually* respell notes as well. The trick is making sure any *automatic* conversions handle both. Do keep in mind we store separate TPC's for all notes in concert versus transposed pitches, specifically to allow for different enharmonic spellings according to context (which is an important and necessary feature). But we don't do anything like that for key signatures. Or chord symbols.

thanks for pointing out #39176: Option to convert transposed instrument key signatures into enharmonic equivalent when number of accidentals exceeds limit...I was unaware of that issue, and it makes sense to provide the option for switching sharp/flat even if not exceeding the limit.

R.e. your second paragraph, the pngs I was showing were not from manual key signature, but those were automatic keysig changes when toggling ConcertPitch, so I do think MuseScore needs to transpose the notes according to the same way the automatic keysig changes. But of course if user manually changes the key sig then there should be no expectation for MuseScore changing the note spellings.

Do keep in mind we store separate TPC's for all notes in concert versus transposed pitches, specifically to allow for different enharmonic spellings according to context (which is an important and necessary feature). But we don't do anything like that for key signatures. Or chord symbols.

Aha, was not aware of that, thanks. Sounds like storing seperate TPC's for key sigs and chord symbols is probably the way I should go...

(Side Note: If not in concert pitch and add a Change Instrument Text set to a transposing instrument, MuseScore does not seem to change keysigs after that change insrument text (but only changes note pitches) as of current 3.0dev 2274353)

(or I guess something equivalent to TPC's for key sigs)
Well looks like class Note stores int _tpc[2] as a simple 2-element array, so I'm going to try doing that same thing for class KeySigEvent by turning Key _key into Key _key[2] to be analogous....edit: honestly when looking at note.cpp I find the use of _tpc[0] to hold non-transposed and _tpc[1] to hold transposed to be a very confusing naming system, so for KeySigEvent I'm just going to name them instead: _keyConcertPitch and _keyWritten for the time being (where _keyWritten is how this particular keysig is currently displayed) until a better name comes along.

BTW, I mentioned the thing about manual versus automatic key changes mostly just to make sure you didn't inadvertently change the behavior for manual changes. But also, I should point out that some *would* want a command to change key signature and respell notes simultaneously. So it's worth thinking about the possibility of a keyboard modifier (like the way "Ctrl" marks a key signature local to one staff) to respell notes. Anyhow, the bottom line is just making sure the two concepts remain separable.

re

some *would* want a command to change key signature and respell notes simultaneously. So it's worth thinking about the possibility of a keyboard modifier (like the way "Ctrl" marks a key signature local to one staff) to respell notes.

I can see how that might be useful for switching from flat to sharp or from sharp to flat keys. Would that also be used for switching keys in general?

I was making the change of having KeySigEvent contain _keyConcertPitch and _keyWritten, but I'm stopping myself now before I go too far down the rabbit hole because it is seems that I am going to have to change a lot of code in a lot of files in order to keep those two values properly in sync with one another.

I'm am going to try an alternative method with is a bit simpler: I'm going to add a simple boolean to the KeySigEvent data struct, named something like _transposedKeyExceededAccidentalLimit (any better names welcomed), which is normally false but gets set true for the case when the calculation of the non-ConcertPitch key has exceeded the limits of the maximum number of accidentals. So now hopefully the only changes to code will be around the concert pitch toggling (and that boolean will be saved/read from the xml). I think this will be a much simpler patch compared to keeping track of concertpitch key (which seemed to require a complete overhall, and would likely not get accepted).

Oh and as I said in #39176: Option to convert transposed instrument key signatures into enharmonic equivalent when number of accidentals exceeds limit I'm adding two styles:

      { StyleIdx::keySigTransposedMaxSharps,     "keySigTransposedMaxSharps",    QVariant(7) },
      { StyleIdx::keySigTransposedMaxFlats,      "keySigTransposedMaxFlats",     QVariant(7) },

I currently have the boolean _transposedKeyExceededAccidentalLimit successfully get set when an instrument transposition causes the key to exceed those Styles I defined, and can see it in the debugger (which now has my boolean variable displayed as checkbox):

Just making a note that this is the way I'm going forward with this issue, because it seems much easier. I am basically having a separate transpose key function instead of extern Key transposeKey(Key oldKey, const Interval&); which I'm calling:

void KeySigEvent::setTransposedInstrumentKey(Key key, const Interval& interval, int sharpLimit, int flatLimit);

And that I'm using specifically for dealing with transposing instruments handling concertpitch setting, but not for any other transposing. It sets the flag if necessary.

Now I need to finish up for when returning to concert pitch...

Oh, I've ended up treating the transposedKyeExceededAccidentalLimit in KeySigEvent as a 3-state enum:

enum class TransposedInstrumentKeyExceededAccidentalLimit {
EXCEEDED_FLAT_LIMIT = -1,
HAS_NOT_EXCEEDED_LIMIT = 0,
EXCEEDED_SHARP_LIMIT = 1
};

and am storing those when writing or reading from xml if they are set. Turns out that makes the code easier. Anyone may feel free to suggest a shorter name for "_transposedInstrumentKeyExceededAccidentalLimit" in the code.

I have it working for basic case of if I insert a bunch of keysigs in concert pitch and then toggle concert pitch off, I can see the key sigs properly obey the transition:

And then upon re-enabling concert pitch, I can see that each key signature returns to their original spelling:

So now I need to handle the various other times when transposing occurs (I haven't handled dropping keysigs while already in concert pitch mode or excerpts...but I'll handle that next) and write some tests... One question is should I go ahead and handle the situation when user adds Instrument Change staff text before a key signature? MuseScore currently doesn't seem to transpose the later key signatures, but I would think it should... Should I go ahead and implement that?

here's the current state of my basic changes: https://github.com/musescore/MuseScore/compare/master...ericfont:22687-…

When investigating how to handle when transposition changes from one key to another (for instance when the staff was previous set to one transposing instrument but then user changes that staff to another transposing instrument) I've been careful to break the operation of transposing the key signatures into two discreet steps:

1. first undo the old transposition (this is important for proper unsetting of transposedKeyExceededAccidentalLimit flags) by going to concert pitch
2. then go from concert pitch to the new transposition (which may set transposedKeyExceededAccidentalLimit flags for a different set of key signatures)

The previous code for Score::transpositionChanged() had that as one combined step using the difference between the two transpositions as the interval. But that caused problems because for some cases the flags were set but needed to be unset.

I'm bringing this up now because the rest of Score::transpositionChanged() uses the combined difference interval for transposing notes and chord symbols. Now I don't know if for this PR I should take care of handling all the proper conversions for that to. Maybe I'll make a seperate issue request to have chord symbols also obey this new behavior of the keysignature...

Seems this came up again in https://musescore.org/en/node/225816