Fix MS #314800 Add Ledger Lines to Key Signatures #10488

sammik · 2022-02-08T09:20:47Z

Added ledger lines to key signatures. If accidental was above, or belove staff, ledger lines were missing and it was difficualt to recognize its position. Now it is fixed.

~~Possible conflicts with #10609, will need to rebase on top of it.~~
Rebased on top of current master.

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

sammik · 2022-02-08T09:21:59Z

cbjeukendrup · 2022-02-08T09:50:20Z

src/engraving/libmscore/keysig.cpp

+            drawSymbol(SymId::legerLine, painter, PointF((ks.pos.x() - xshift), (_line * _spatium)));
+            ++_line;
+        }
+        while (_line >= 5) {


Here you assume that a staff has 5 lines, but that might not always be the case. Better use staff() ? staff()->staffTypeForElement(this)->lines() : 5 instead of 5. As an optimization, you could take that out of the for-loop and store it in a variable.

Seems like you'll also need to account for staff line distance. Look at this failing VTest for example:

It's just a 5-line staff, but with a staff line spacing of 2.50 sp.

Wouldn't staff() ? staff()->staffTypeForElement(this)->lineDistance().val() : 1.0; help here?

Scaling bug for ledger lines seems to be fxed now:

Found another bug, but it is not caused by this PR (already there in 3.6.2) - scaling misplaces accidentals in custom key signatures #10497

Personally I would expect the distance between the ledger lines to be the same as the distance between the staff lines. That of course relates to the other bug you just discovered. If you would like to solve that bug yourself (while on it), I could assign it to you.

Also, that VTest still seems to fail the same way as before... you will indeed need to use staff() ? staff()->staffTypeForElement(this)->lineDistance().val() : 1.0;, as Jojo suggests.

Yes, distance should be same, as distance between lines, but as you can see, ledger lines corresponds with accidentals. I will chek something. Thank You.
One question staff()->staffTypeForElement(this)->lineDistance().val() or staff()->lineDistance(tick()) would be OK?

The latter looks simpler, but for symmetry alone I'd rather use the former?

The former contains a small optimization compared to the latter, because it avoids a call to tick() in case there is only one staff type on that staff (which is the case in most scores, I'd think). For some elements, tick() is a very trivial call, but for others, it involves a bit more.

backport of musescore#10488

Jojo-Schmitz · 2022-02-08T17:10:42Z

src/engraving/libmscore/keysig.cpp

+        qreal _spatium = spatium();
+        qreal xshift = (ks.sym == SymId::accidentalSharp) ? (_spatium * .15) : (_spatium * .25);
+        int y = int(ks.spos.y());
+        int staffLines = staff() ? staff()->staffTypeForElement(this)->lines() : 5;


Maybe better move this line to before the for loop?

backport of musescore#10488

Jojo-Schmitz · 2022-02-08T19:00:03Z

src/engraving/libmscore/keysig.cpp

@@ -315,6 +315,19 @@ void KeySig::draw(mu::draw::Painter* painter) const
    painter->setPen(curColor());
    for (const KeySym& ks: _sig.keySymbols()) {
        drawSymbol(ks.sym, painter, PointF(ks.pos.x(), ks.pos.y()));
+        // draw ledgerLines
+        qreal _spatium = spatium();
+        qreal xshift = (ks.sym == SymId::accidentalSharp) ? (_spatium * .15) : (_spatium * .25);


Maybe make this qreal x = ks.pos.x() - (ks.sym == SymId::accidentalSharp) ? (_spatium * .15) : (_spatium * .25); and use that in the 2 drawSymbol() calls?

In fact, they are never called both, every time just one of them, so what is the reason for this change? Pretyfng code?

Ah, I missed that. but it won't harm anyway

backport of musescore#10488

sammik · 2022-02-08T22:07:29Z

I hope, finaly its fixed.
Thank You @Jojo-Schmitz and @cbjeukendrup for Your patience and help!

(It looks may be weird a bit, because of that other bug.)

Jojo-Schmitz · 2022-02-09T07:14:50Z

Hmm, my backport to 3.x doesn't work well:

using the sample score from #10497, maybe that's why.

Have a sample score to test this? Preferably for MuseScore 3?

backport of musescore#10488

sammik · 2022-02-09T08:29:05Z

Hmm, my backport to 3.x doesn't work well

Somehow one line became wrong in Your backport.
Jojo-Schmitz@4ba24e6#r66315029

Jojo-Schmitz · 2022-02-09T08:41:52Z

src/engraving/libmscore/keysig.cpp

    for (const KeySym& ks: _sig.keySymbols()) {
        drawSymbol(ks.sym, painter, PointF(ks.pos.x(), ks.pos.y()));
+        // draw ledgerLines
+        qreal x = ks.pos.x() - ((ks.sym == SymId::accidentalSharp) ? (_spatium * .15) : (_spatium * .25));
+        int i = int(ks.spos.y() / lineDist);


This kind of casts are frowned upon, in master at least, better use the more verbose static_cast<int>(...)

backport of musescore#10488

Backport of musescore#10488

cbjeukendrup · 2022-02-18T12:21:59Z

@sammik I think it would be good if we would merge #10609 first, and then you rebase this PR on top of it. So could you please fix the review comments on that PR?

sammik · 2022-02-18T20:02:47Z

@sammik I think it would be good if we would merge #10609 first, and then you rebase this PR on top of it. So could you please fix the review comments on that PR?

Sorry, I was playing.
It should be fixed, so once it will be merged, Ill rebase this one.

Backport of musescore#10488

sammik · 2022-05-01T06:47:09Z

vtests failure is good

before

after

sammik · 2022-05-10T15:19:39Z

@cbjeukendrup may I kindly ask for vtest label?

cbjeukendrup · 2022-05-10T16:04:57Z

Added!

Backport of musescore#10488

cbjeukendrup · 2022-07-02T23:17:00Z

I had another look at this today, and I thought, maybe we should not use the SymId::legerLine etc. symbols, but instead, just draw lines, using the Painter object. This enables us to make the line length dynamic, so that it will be good for every accidental regardless of how wide it is. It also enables us to follow the ledger line thickness specified in the style settings. This is how it is done for chords too. Probably a good example of this is in ShadowNote::draw. The relevant Sids are Sid::ledgerLineLength (the amount to be added at each side of the accidental) and Sid::ledgerLineWidth (the line thickness).

sammik · 2022-07-03T22:42:16Z

I had another look at this today, and I thought, maybe we should not use the SymId::legerLine etc. symbols, but instead, just draw lines, using the Painter object. This enables us to make the line length dynamic, so that it will be good for every accidental regardless of how wide it is. It also enables us to follow the ledger line thickness specified in the style settings. This is how it is done for chords too. Probably a good example of this is in ShadowNote::draw. The relevant Sids are Sid::ledgerLineLength (the amount to be added at each side of the accidental) and Sid::ledgerLineWidth (the line thickness).

Done!

Backport of musescore#10488

cbjeukendrup reviewed Feb 8, 2022

View reviewed changes

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 8, 2022

Fix #314800: Add Ledger Lines to Key Signatures

e56fa11

backport of musescore#10488

Jojo-Schmitz reviewed Feb 8, 2022

View reviewed changes

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 8, 2022

Fix #314800: Add Ledger Lines to Key Signatures

d8a0652

backport of musescore#10488

sammik force-pushed the key-sig-ledger-lines branch from 0717dfc to d748d71 Compare February 8, 2022 18:48

Jojo-Schmitz reviewed Feb 8, 2022

View reviewed changes

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 8, 2022

Fix #314800: Add Ledger Lines to Key Signatures

fb02e9e

backport of musescore#10488

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 9, 2022

Fix #314800: Add Ledger Lines to Key Signatures

4ba24e6

backport of musescore#10488

Jojo-Schmitz reviewed Feb 9, 2022

View reviewed changes

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 9, 2022

Fix #314800: Add Ledger Lines to Key Signatures

d140bb3

backport of musescore#10488

Jojo-Schmitz mentioned this pull request Feb 9, 2022

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 9, 2022

Fix #314800: Add Ledger Lines to Key Signatures

c85f97c

backport of musescore#10488

sammik force-pushed the key-sig-ledger-lines branch from ee5bc8d to 87f8a5f Compare February 9, 2022 14:47

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 9, 2022

Fix #314800: Add Ledger Lines to Key Signatures

8fb90d2

backport of musescore#10488

sammik requested a review from cbjeukendrup February 9, 2022 21:30

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 10, 2022

Fix #314800: Add Ledger Lines to Key Signatures

47cb499

Backport of musescore#10488

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 14, 2022

Fix #314800: Add Ledger Lines to Key Signatures

6c6d2d0

Backport of musescore#10488

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 24, 2022

Fix #314800: Add Ledger Lines to Key Signatures

eb343fd

Backport of musescore#10488

sammik force-pushed the key-sig-ledger-lines branch from 87f8a5f to 4ce902c Compare May 1, 2022 06:28

sammik force-pushed the key-sig-ledger-lines branch 2 times, most recently from d653437 to 52a72ad Compare May 10, 2022 14:22

cbjeukendrup added the vtests This PR produces approved changes to vtest results label May 10, 2022

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 12, 2022

Fix #314800: Add Ledger Lines to Key Signatures

6bb93ba

Backport of musescore#10488

sammik force-pushed the key-sig-ledger-lines branch from 52a72ad to 677c8d2 Compare July 1, 2022 21:33

Fix MS #314800 Add Ledger Lines to Key Signatures

225a04f

sammik force-pushed the key-sig-ledger-lines branch from 677c8d2 to 225a04f Compare July 3, 2022 22:33

cbjeukendrup approved these changes Jul 4, 2022

View reviewed changes

cbjeukendrup merged commit 5a006a4 into musescore:master Jul 4, 2022

sammik deleted the key-sig-ledger-lines branch July 4, 2022 11:57

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023

Fix #314800: Add Ledger Lines to Key Signatures

d288a7d

Backport of musescore#10488

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MS #314800 Add Ledger Lines to Key Signatures #10488

Fix MS #314800 Add Ledger Lines to Key Signatures #10488

sammik commented Feb 8, 2022 •

edited

Loading

sammik commented Feb 8, 2022

cbjeukendrup Feb 8, 2022

cbjeukendrup Feb 8, 2022

Jojo-Schmitz Feb 8, 2022

sammik Feb 8, 2022 •

edited

Loading

cbjeukendrup Feb 8, 2022

cbjeukendrup Feb 8, 2022

sammik Feb 8, 2022

Jojo-Schmitz Feb 8, 2022

cbjeukendrup Feb 8, 2022

Jojo-Schmitz Feb 8, 2022 •

edited

Loading

Jojo-Schmitz Feb 8, 2022 •

edited

Loading

sammik Feb 8, 2022

Jojo-Schmitz Feb 8, 2022 •

edited

Loading

sammik commented Feb 8, 2022

Jojo-Schmitz commented Feb 9, 2022 •

edited

Loading

sammik commented Feb 9, 2022 •

edited

Loading

Jojo-Schmitz Feb 9, 2022

cbjeukendrup commented Feb 18, 2022 •

edited

Loading

sammik commented Feb 18, 2022

sammik commented May 1, 2022

sammik commented May 10, 2022

cbjeukendrup commented May 10, 2022

cbjeukendrup commented Jul 2, 2022

sammik commented Jul 3, 2022

Fix MS #314800 Add Ledger Lines to Key Signatures #10488

Fix MS #314800 Add Ledger Lines to Key Signatures #10488

Conversation

sammik commented Feb 8, 2022 • edited Loading

sammik commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammik Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

sammik commented Feb 8, 2022

Jojo-Schmitz commented Feb 9, 2022 • edited Loading

sammik commented Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

cbjeukendrup commented Feb 18, 2022 • edited Loading

sammik commented Feb 18, 2022

sammik commented May 1, 2022

sammik commented May 10, 2022

cbjeukendrup commented May 10, 2022

cbjeukendrup commented Jul 2, 2022

sammik commented Jul 3, 2022

sammik commented Feb 8, 2022 •

edited

Loading

sammik Feb 8, 2022 •

edited

Loading

Jojo-Schmitz Feb 8, 2022 •

edited

Loading

Jojo-Schmitz Feb 8, 2022 •

edited

Loading

Jojo-Schmitz Feb 8, 2022 •

edited

Loading

Jojo-Schmitz commented Feb 9, 2022 •

edited

Loading

sammik commented Feb 9, 2022 •

edited

Loading

cbjeukendrup commented Feb 18, 2022 •

edited

Loading