-
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
Fix arpeggio cross stave collisions #19734
Conversation
ad3cec0
to
f437994
Compare
6873adc
to
e603857
Compare
@@ -1771,7 +1771,8 @@ void TRead::read(Arpeggio* a, XmlReader& e, ReadContext& ctx) | |||
} else if (tag == "userLen2") { | |||
a->setUserLen2(e.readDouble() * a->spatium()); | |||
} else if (tag == "span") { | |||
a->setSpan(e.readInt()); | |||
// Span now refers to number of voices spanned, not staves | |||
a->setSpan((e.readInt() - 1) * VOICES + 1); |
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.
It seems that this way, the arpeggio will only span the first voice of the bottom staff. Would it make sense to span all voices by default? Or is spanning only the first voice the closest to the old behaviour?
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.
Arpeggios used to span to the same voice on the bottom stave as the arpeggio was added to on the upper stave. This preserves this when opening old scores, so an arpeggio added to voice 1 will span to voice 1 on the bottom staff, an arpeggio on voice 2 will span to voice 2 etc.
bool aboveStart = chord->vStaffIdx() <= item->vStaffIdx() && chord->line() < item->chord()->line(); | ||
bool belowEnd = chord->vStaffIdx() >= endChord->vStaffIdx() && chord->line() > endChord->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.
Don't you want to use upLine
and downLine
explicitly, instead of just line
which decides based on the stem direction (which seems irrelevant to me here)?
Also, shouldn't the logic be like this:
bool aboveStart = chord->vStaffIdx() <= item->vStaffIdx() && chord->line() < item->chord()->line(); | |
bool belowEnd = chord->vStaffIdx() >= endChord->vStaffIdx() && chord->line() > endChord->line(); | |
bool aboveStart = chord->vStaffIdx() < item->vStaffIdx() ||(chord->vStaffIdx() == item->vStaffIdx() && chord->line() < item->chord()->line()); | |
bool belowEnd = chord->vStaffIdx() > endChord->vStaffIdx() || (chord->vStaffIdx() == endChord->vStaffIdx() && chord->line() > endChord->line()); |
which might be written more concisely as
bool aboveStart = chord->vStaffIdx() <= item->vStaffIdx() && chord->line() < item->chord()->line(); | |
bool belowEnd = chord->vStaffIdx() >= endChord->vStaffIdx() && chord->line() > endChord->line(); | |
bool aboveStart = std::make_pair(chord->vStaffIdx(), chord->line()) < std::make_pair(item->vStaffIdx(), item->chord()->line()); | |
bool belowEnd = std::make_pair(chord->vStaffIdx(), chord->line()) > std::make_pair(endChord->vStaffIdx(), endChord->line()); |
bool aboveStart = item->vStaffIdx() <= spanArp->vStaffIdx() && item->line() < spanArp->chord()->line(); | ||
bool belowEnd = item->vStaffIdx() >= endChord->vStaffIdx() && item->line() > endChord->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.
Same as above about upLine
/downLine
vs line
e8b67cc
to
38338ca
Compare
ebb6dfb
to
155527b
Compare
src/engraving/dom/chord.cpp
Outdated
Chord* chord = toChord(linkedObject); | ||
Score* score = chord->score(); | ||
if (score) { | ||
score->undo(new ChangeSpanArpeggio(chord, a)); |
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.
Instead of a
itself, shouldn't we use the Arpeggio that is linked to a
in the same score as chord
?
// insetDistance | ||
//--------------------------------------------------------- | ||
|
||
double ArpeggioLayout::insetDistance(Arpeggio* item, LayoutContext& ctx, double mag_, Chord* chord, std::vector<Accidental*> accidentals) |
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.
const std::vector<Accidental*>&
@@ -96,7 +96,7 @@ void ScoreRenderer::layoutText1(TextBase* item, bool base) | |||
void ScoreRenderer::layoutOnEdit(Arpeggio* item) |
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.
Looks like this method doesn't do much anymore, can we get rid of it?
4b16c5e
to
2934183
Compare
if (downnote->line() > firstLedgerBelow || upnote->line() < firstLedgerAbove) { | ||
gapSize = arpeggioLedgerDistance + ctx.conf().styleS(Sid::ledgerLineLength).val() * item->spatium(); | ||
} | ||
} else if (leftNote->line() > firstLedgerBelow || leftNote->line() < firstLedgerAbove) { |
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.
Don't we need a null check?
} else if (leftNote->line() > firstLedgerBelow || leftNote->line() < firstLedgerAbove) { | |
} else if (leftNote && (leftNote->line() > firstLedgerBelow || leftNote->line() < firstLedgerAbove)) { |
@@ -198,7 +202,11 @@ void ChordLayout::layoutPitched(Chord* item, LayoutContext& ctx) | |||
|
|||
double gapSize = arpeggioNoteDistance; | |||
|
|||
if (downnote->line() > firstLedgerBelow || upnote->line() < firstLedgerAbove) { | |||
if (leftNote && leftNote->x() == 0) { |
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.
Perhaps RealIsNull(leftNote->x())
is more reliable (from realfn.h)
2934183
to
a1b3373
Compare
src/engraving/dom/arpeggio.cpp
Outdated
} | ||
} | ||
|
||
void Arpeggio::rebaseStartAnchor(int direction) |
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.
It would be better to use enum instead of int (rebaseStartAnchor/rebaseEndAnchor)
src/engraving/dom/arpeggio.h
Outdated
@@ -40,6 +40,7 @@ class Arpeggio final : public EngravingItem | |||
|
|||
public: | |||
|
|||
~Arpeggio(); |
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.
override
double chordX = -chordShape.left(); | ||
double diff = chordX - arpX; | ||
|
||
if (diff != 0.0) { |
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.
Please use RealIsNull
// Arpeggio doesn't start on this staff so move to last note and extend upwards | ||
curArpShape.translate(PointF(0.0, endChord->downNote()->pagePos().y() - item->pagePos().y())); | ||
curArpShape.addBBox(RectF(curArpShape.bbox().topLeft().x(), curArpShape.bbox().topLeft().y() - 10000, | ||
curArpShape.bbox().width(), 10000)); |
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.
Please use a named constant instead of this magic number (same for other places where you use it)
} | ||
} | ||
|
||
if (_accidentals.size() == 0) { |
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.
if (_accidentals.empty())
// insetDistance | ||
//--------------------------------------------------------- | ||
|
||
double ArpeggioLayout::insetDistance(Arpeggio* item, LayoutContext& ctx, double mag_, Chord* chord, |
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.
item, ctx and chord should be const
} | ||
|
||
//--------------------------------------------------------- | ||
// insetDistance |
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.
Please remove this comment (and other similar comments)
|| type == ArpeggioType::BRACKET; | ||
|
||
Accidental* furthestAccidental = nullptr; | ||
for (auto accidental : accidentals) { |
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.
Please use const Accidental* instead of auto
@@ -91,6 +91,7 @@ void ChordLayout::layout(Chord* item, LayoutContext& ctx) | |||
|
|||
void ChordLayout::layoutPitched(Chord* item, LayoutContext& ctx) | |||
{ | |||
PaddingTable paddingTable = item->score()->paddingTable(); |
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.
- Please declare this closer to the place where it's used
- It should be const PaddingTable & paddingTable
if (item->arpeggio()) { | ||
item->arpeggio()->findAndAttachToChords(); | ||
item->arpeggio()->mutldata()->maxChordPad = 0.0; | ||
item->arpeggio()->mutldata()->minChordX = 10000; |
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.
magic number
|
||
const Segment* seg = item->chord()->segment(); | ||
Chord* endChord = item->chord(); | ||
const PaddingTable paddingTable = item->score()->paddingTable(); |
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.
Don't forget &
return 0.0; | ||
} | ||
|
||
if (accidentals.size() == 0) { |
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.
if (accidentals.empty())
|| type == ArpeggioType::BRACKET; | ||
|
||
Accidental* furthestAccidental = nullptr; | ||
for (Accidental* accidental : accidentals) { |
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.
const Accidental*
lll += extraX; | ||
} | ||
// _arpeggio->layout() called in layoutArpeggio2() | ||
if (chordAccidentals.size()) { |
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.
if (!chordAccidentals.empty())
6e87e9f
to
58781a8
Compare
Resolves: #19668
Resolves: #14359