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

Fix key sig bug #15479

Merged
merged 3 commits into from Feb 21, 2023
Merged

Fix key sig bug #15479

merged 3 commits into from Feb 21, 2023

Conversation

mike-spa
Copy link
Contributor

Resolves: #15314

@sammik
Copy link
Contributor

sammik commented Dec 29, 2022

@mike-spa I dont know, why it was imortant, not to create C (empty) key signature, but I guess, there was some reason.
Are we sure, it is not relevant enymore?
If so, I saw many places in code, where similar code ("not create emptykey sig") is present. Isnt it related with this PR?

} else if (e && e->generated() && key == keyIdx.key() && keyIdx.key() == Key::C) {
// If a key sig segment is disabled, it may be re-enabled if there is
// a transposing instrument using a different key sig.
// To prevent this from making the wrong key sig display, remove any key
// sigs on staves where the key in this measure is C.
kSegment->remove(e);

// do not create empty keysig unless custom or atonal
if (tick.isNotZero() || nkey.key() != Key::C || nkey.custom() || nkey.isAtonal()) {

// remove initial C major key signatures
if (nKey == Key::C && s->tick().isZero() && !ks->isCustom()) {
undo(new RemoveElement(ks));
if (s->empty()) {
undo(new RemoveElement(s));
}
} else {
ke.setKey(nKey);
undo(new ChangeKeySig(ks, ke, ks->showCourtesy()));
}
}

if (!ks->isCustom() && !ks->isAtonal() && ks->key() == Key::C && curTick.isZero()) {
// ignore empty key signature
LOGD("remove keysig c at tick 0");
} else {
// if key sig not at beginning of measure => courtesy key sig
bool courtesySig = (curTick == measure->endTick());

And possibly many others.

@MarcSabatella
Copy link
Contributor

I can provide a little historical background that might be relevant. There was a conscious design decision many years ago to represent C major by the absence of a key signature based on a general feeling that it was bad to have elements in the score that showed no visible trace of their existence. I personally never shared that feeling, but did what I could to make things work despite any number of complications that design caused. So some of those comments are likely mine, as is some of the code, but I'd be very happy to see it simplified.

There is nothing wrong in my book with C major being represented by an explicit element. I don't even think the code actively requires C major to be handle with no key signature, as there are still ways you can end up with a C major key signatures element in the score. But they do tend to get cleaned up on save/reload, so you'd want to make sure that isn't still happening.

@sammik
Copy link
Contributor

sammik commented Dec 31, 2022

Same fix is part of PR #12560 which fixes issue #12506

@mike-spa
Copy link
Contributor Author

mike-spa commented Jan 9, 2023

Thanks both for providing context! To me, eliminating the keySig event from a staff just because it isn't graphically shown is a profound mistake. The fact that the key isn't shown is purely incidental, and managing the showing/not showing just requires some very simple logic. Eliminating it altogether, on the contrary, effectively deletes a fundamental piece of information from our score representation that can't be reconstructed, and it keeps causing bugs and problems everywhere.
The clearest example is the problems I fixed in #14849. There is a fundamental difference between global key signatures (i.e. the ones that apply to all staves) and local key signatures (i.e. the ones that apply only to individual staves). The first need to be repeated in all extracted parts, regardless of transposition, the latter doesn't. Problem is: if your score has instruments with different transposition and some of them happen to be in C, you have staves with- and staves without key, and now you've lost the information to know whether the key was global or not, so you can't decide where to clone it or not in the parts.
So, in essence, I believe this "delete/do not create keySig if it is C" logic should be gradually phased out.

@mike-spa mike-spa closed this Jan 9, 2023
@mike-spa mike-spa reopened this Jan 9, 2023
@RomanPudashkin RomanPudashkin merged commit a68f143 into musescore:master Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MU4 Issue] Wrong key signature when i add a transposition instrument in this score
5 participants