Fix autoplace of fret diagrams pushed onto next system #29132
Conversation
miiizen
force-pushed
the
28894-autoplace-fb
branch
from
ffc1e27 to
963dd12
Compare
| @@ -1051,6 +1052,9 @@ void MeasureLayout::layoutMeasure(MeasureBase* currentMB, LayoutContext& ctx) | |||
| for (EngravingItem* e : segment.annotations()) { | |||
| if (e->isSymbol() || e->isHarmony() || e->isFretDiagram()) { | |||
| TLayout::layoutItem(e, ctx); | |||
| if ((e->isHarmony() || e->isFretDiagram()) && e->autoplace() && measure->system()) { | |||
| Autoplace::autoplaceSegmentElement(e, e->mutldata(), false); | |||
There was a problem hiding this comment.
Is this call really necessary? If I'm not mistaken, when this function gets called the staff skyline doesn't even exist yet (at least on the first layout, and on the next layouts it will probably find the skyline from the previuos layout) so this call wouldn't do much. I'm also wondering if the same issue exists for all other textbased elements?
There was a problem hiding this comment.
Yes - in this situation the layout call above (which is necessary for horizontal spacing) resets the y position of the chord symbol causing a collision between the fret diagram and chord symbol. The measure containing the fret diagram gets moved to the next system, so the autoplace call in SystemLayout::layoutSystemElements is never reached and we get a collision between the fret diagram and chord symbol.
Although, maybe this means that we just need to layout the next system if a measure has been added to it. Thinking about this now, it seems like that should definitely be done?
| @@ -192,6 +192,8 @@ void HarmonyLayout::layoutModifierParentheses(const Harmony* item) | |||
| const std::vector<HarmonyRenderItem*>& itemList = item->ldata()->renderItemList(); | |||
| // Layout parentheses | |||
| std::vector<ChordSymbolParen*> openingParenStack; | |||
| double lastTsHeight = 0.0; | |||
| double lastTsTop = 0.0; | |||
There was a problem hiding this comment.
What does lastTs mean? For readability, let's try to avoid calling things with abbreviations unless they are obvious or well known
|
Sorry for delay, but retested this case now that this PR is merged, and it looks to be fixed. Approved! Previous buggy behavior with visibility: Screen.Recording.2025-08-13.at.12.19.42.PM.mov |
miiizen
force-pushed
the
28894-autoplace-fb
branch
2 times, most recently
from
bdcdc4a to
32d1912
Compare
miiizen
force-pushed
the
28894-autoplace-fb
branch
from
32d1912 to
4d8f315
Compare
Resolves: #28894
Resolves: #29127
Chords and fretboard diagrams need to be laid out to calculate horizontal space. This is done in
MeasureLayout. If a chord or fretboard diagram is pushed onto the next system, the layout call inMeasureLayouthappens, but the autoplace call inSystemLayoutis never made.