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
Ledger lines clear stems when voices overlap #19402
Conversation
// Above stave | ||
Note* topUpNote = upStemNotes.back(); | ||
Note* bottomDownNote = downStemNotes.front(); | ||
if (!bottomUpNote->chord()->noStem() && bottomUpNote->chord()->stem()) { |
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.
Is there a particular reason to check noStem()
? As far as I know, whenever noStem
is true, the stem is (supposed to be) removed, based on the value of shouldHaveStem()
.
int sep = topUpNote->line() - bottomDownNote->line(); | ||
if (topDownNote->line() < -1 && topDownNote->line() < bottomUpNote->line() && sep > 1 | ||
&& bottomUpNote->line() - bottomUpStemLen <= firstLedgerAbove) { | ||
upOffset += 0.15 * sp; |
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.
I see a hard-coded offset here, but the length of ledger lines can be varied using Sid::ledgerLineLength
. Shouldn't that be taken into account?
int firstLedgerBelow = staff->lines(bottomUpNote->tick()) * 2; | ||
// Don't adjust unisons or 2nds as these move anyway | ||
int sep = topUpNote->line() - bottomDownNote->line(); | ||
if (bottomUpNote->line() > staff->lines(bottomUpNote->tick()) * 2 - 1 && topDownNote->line() < bottomUpNote->line() |
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.
What about using firstLedgerBelow
here too?
if (bottomUpNote->line() > staff->lines(bottomUpNote->tick()) * 2 - 1 && topDownNote->line() < bottomUpNote->line() | |
if (bottomUpNote->line() > firstLedgerBelow - 1 && topDownNote->line() < bottomUpNote->line() |
That review unearthed quite a few issues, apologies. After a bit of a refactor:
|
83f75e1
to
9ee6fb2
Compare
3995f0c
to
22d1f96
Compare
@oktophonie This may need re-testing since there were quite some changes since your approval. But perhaps you already did, I'm not sure. |
22d1f96
to
b7bbe17
Compare
b7bbe17
to
18db73d
Compare
Resolves: #18724