Multimeasure rest refactor#32900
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMade 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/rendering/score/measurelayout.cpp (1)
755-773:⚠️ Potential issue | 🟡 MinorOnly break for previous-measure fermatas that can actually be shown.
This branch bypasses the existing staff/system visibility gate. A fermata on a hidden staff will still split the multimeasure rest here even though nothing is rendered, which creates an extra visible break with no visible marking.
🛠️ Proposed fix
if (breakForAnnotation(e)) { return true; } // break for fermatas on the end barline in previous measure - if (e->isFermata()) { + const bool fermataOnVisibleContext = e->systemFlag() || (e->staff() && e->staff()->show()); + if (e->isFermata() && fermataOnVisibleContext) { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/rendering/score/measurelayout.cpp` around lines 755 - 773, Only break for fermatas in the previous measure if the fermata is actually rendered: inside the loop over prevMeas segments and annotations (the code iterating Segment* s = prevMeas->first() and EngravingItem* e), after confirming e->visible() and mid-measure, add the same staff/system visibility guard used elsewhere in this file (the one used by breakForAnnotation or earlier visibility checks) before treating e->isFermata() as a split condition; i.e., only return true for e->isFermata() when the fermata’s staff/system is visible/rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/engraving/rendering/score/measurelayout.cpp`:
- Around line 755-773: Only break for fermatas in the previous measure if the
fermata is actually rendered: inside the loop over prevMeas segments and
annotations (the code iterating Segment* s = prevMeas->first() and
EngravingItem* e), after confirming e->visible() and mid-measure, add the same
staff/system visibility guard used elsewhere in this file (the one used by
breakForAnnotation or earlier visibility checks) before treating e->isFermata()
as a split condition; i.e., only return true for e->isFermata() when the
fermata’s staff/system is visible/rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9dc94cb4-2ece-4078-b5df-5510008a62d0
📒 Files selected for processing (3)
src/engraving/dom/measure.cppsrc/engraving/rendering/score/measurelayout.cppvtest/scores/mmrest-breath.mscz
| if (!muse::contains(BREAK_TYPES, e->type())) { | ||
| const bool breakType = muse::contains(BREAK_TYPES, e->type()); | ||
| const bool fermata = e->isFermata(); | ||
| if (!breakType && !fermata) { |
There was a problem hiding this comment.
Isn't this the same as just inserting ElementType::FERMATA in BREAK_TYPES?
There was a problem hiding this comment.
Ah yes actually, it can be treated in the same way as play count text
| // break for fermatas on the end barline in previous measure | ||
| if (e->isFermata()) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Same here, if ElementType::FERMATA is in the BREAK_TYPES then breakForAnnotation should already do the job?
There was a problem hiding this comment.
No, the CONDITIONAL_BREAK_TYPES and ALWAYS_BREAK_TYPES sets are used to check if the current measure contains an element which should cause a break and start an MMRest. We don't want the measure containing the fermata to start an MMRest, we want it to end an MMRest. We should only check if the previous measure contains a barline with a fermata, like we do for end repeat barlines. Otherwise, we get results like this:
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/rendering/score/measurelayout.cpp (1)
753-768:⚠️ Potential issue | 🟠 MajorMirror the visible-staff guard in the previous-measure fermata break.
This branch now breaks on any visible fermata in
prevMeas, including fermatas attached to hidden staves.validMMRestMeasure()explicitly ignores hidden staves on Lines 595-596, so visible multimeasure rests can still be fragmented by symbols that are not rendered. If barline fermatas are the only intended case here, scoping this to the end-barline segment would also match the copy path on Lines 245-274.🔧 Proposed fix
- // break for fermatas on the end barline in previous measure - if (e->isFermata()) { + // break for fermatas on a visible staff in the previous measure + if (e->isFermata() + && e->staffIdx() < ctx.dom().nstaves() + && e->staff() + && e->staff()->show()) { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/rendering/score/measurelayout.cpp` around lines 753 - 768, The fermata check currently breaks on any visible fermata in prevMeas (including those on hidden staves); change the fermata branch so it only triggers for fermatas attached to the end-barline segment: inside the loop over Segment* s (the same s used for s->annotations()), restrict the e->isFermata() return to when the segment is the measure's end-barline segment (e.g. compare s to the prevMeas end-barline segment accessor or use a segment predicate like s->isEndBarline()/s->isBarline()), keeping the existing visible()/rtick() guards and existing call to breakForAnnotation(e). This will mirror the copy-path behavior and avoid breaking on fermatas on non-end-barline/hidden-staff annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/engraving/rendering/score/measurelayout.cpp`:
- Around line 753-768: The fermata check currently breaks on any visible fermata
in prevMeas (including those on hidden staves); change the fermata branch so it
only triggers for fermatas attached to the end-barline segment: inside the loop
over Segment* s (the same s used for s->annotations()), restrict the
e->isFermata() return to when the segment is the measure's end-barline segment
(e.g. compare s to the prevMeas end-barline segment accessor or use a segment
predicate like s->isEndBarline()/s->isBarline()), keeping the existing
visible()/rtick() guards and existing call to breakForAnnotation(e). This will
mirror the copy-path behavior and avoid breaking on fermatas on
non-end-barline/hidden-staff annotations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b65173f3-0d26-4fa0-91ac-a372f177476c
📒 Files selected for processing (1)
src/engraving/rendering/score/measurelayout.cpp
| // 1) This is the first measure of the system OR | ||
| // 2) The previous measure in the system is the first, and is excluded from numbering. | ||
| return isFirstInSystem() | ||
| return (system() && isFirstInSystem()) |
There was a problem hiding this comment.
I do wonder if it makes more sense to remove that assertion. If I am querying isFirstInSystem, it's a bit weird to be asked to check first if it does have a system. Feels like the case of not having a system is part of the query itself, i.e. just a legitimate expected return false.
miiizen
force-pushed
the
31586-breathMMR
branch
from
972cdc1 to
ed1768e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/engraving/dom/measure.cpp`:
- Around line 587-588: The second branch calls prevMeasure->isFirstInSystem()
without ensuring prevMeasure->system() is non-null; update the conditional to
also guard that prevMeasure->system() is valid. Change the expression to require
prevMeasure && prevMeasure->system() && prevMeasure->excludeFromNumbering() &&
prevMeasure->isFirstInSystem(), so calls to prevMeasure->isFirstInSystem() only
occur when prevMeasure->system() is present (referencing prevMeasure, system(),
excludeFromNumbering(), and isFirstInSystem()).
In `@src/engraving/rendering/score/measurelayout.cpp`:
- Around line 789-792: Update the inline comment above the fermata check to
accurately describe the condition: it breaks for any fermata with rtick > 0
(which covers mid-measure fermatas as well as fermatas on the end barline of the
previous measure). Locate the block where e->isFermata() is used (in the measure
layout logic handling rtick > 0) and change the comment to something like "break
for fermatas mid-measure or on the end barline in previous measure" so it
matches the implemented behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ea8a3de-a535-4061-9a3a-0132af1e84c2
📒 Files selected for processing (2)
src/engraving/dom/measure.cppsrc/engraving/rendering/score/measurelayout.cpp
miiizen
force-pushed
the
31586-breathMMR
branch
from
ed1768e to
9ca8816
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/engraving/rendering/score/measurelayout.cpp`:
- Around line 442-470: When underlyingBreathSeg exists but for a given track
there is no underlyingBreath, the code currently leaves an existing mmrBreath in
place; update the loop that iterates staffIdx/track (in the block that
references underlyingBreathSeg, mmrBreathSeg, Breath, toBreath, and
ctx.mutDom()) so that when toBreath(underlyingBreathSeg->element(track)) returns
null and toBreath(mmrBreathSeg->element(track)) returns non-null you remove the
stale mmrBreath by calling ctx.mutDom().doUndoRemoveElement(mmrBreath) (and
avoid further layout/clone logic for that track); this ensures per-track mmr
breaths are deleted when the underlying track no longer has a breath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f5e813e-3921-47e4-9cc3-7df98c2dd1c7
📒 Files selected for processing (3)
src/engraving/dom/measure.cppsrc/engraving/rendering/score/measurelayout.cppvtest/scores/mmrest-breath.mscz
| // | ||
| // check for end of measure breaths | ||
| // | ||
| Segment* underlyingBreathSeg = lastMeasure->findSegmentR(SegmentType::Breath, lastMeasure->ticks()); | ||
| Segment* mmrBreathSeg = mmrMeasure->findSegmentR(SegmentType::Breath, mmrMeasure->ticks()); | ||
| if (underlyingBreathSeg) { | ||
| if (mmrBreathSeg == 0) { | ||
| mmrBreathSeg = mmrMeasure->undoGetSegmentR(SegmentType::Breath, mmrMeasure->ticks()); | ||
| } | ||
| mmrBreathSeg->setEnabled(underlyingBreathSeg->enabled()); | ||
| for (staff_idx_t staffIdx = 0; staffIdx < ctx.dom().nstaves(); ++staffIdx) { | ||
| track_idx_t track = staffIdx * VOICES; | ||
| Breath* underlyingBreath = toBreath(underlyingBreathSeg->element(track)); | ||
| if (underlyingBreath) { | ||
| Breath* mmrBreath = toBreath(mmrBreathSeg->element(track)); | ||
| if (!mmrBreath) { | ||
| mmrBreath = underlyingBreath->generated() ? underlyingBreath->clone() : toBreath( | ||
| underlyingBreath->linkedClone()); | ||
| mmrBreath->setParent(mmrBreathSeg); | ||
| ctx.mutDom().doUndoAddElement(mmrBreath); | ||
| } else { | ||
| TLayout::layoutBreath(mmrBreath, mmrBreath->mutldata(), ctx.conf()); | ||
| } | ||
| } | ||
| } | ||
| } else if (mmrBreathSeg) { | ||
| // TODO: remove elements from mmrSeg? | ||
| ctx.mutDom().doUndoRemoveElement(mmrBreathSeg); | ||
| } |
There was a problem hiding this comment.
Remove stale per-track mmrest breaths when only some tracks lose breaths.
In Line 452-Line 466, if underlyingBreathSeg exists but a specific track has no underlyingBreath, the corresponding existing mmrBreath is never removed. That leaves stale breath marks in the mmrest after edits on individual staves.
Proposed fix
for (staff_idx_t staffIdx = 0; staffIdx < ctx.dom().nstaves(); ++staffIdx) {
track_idx_t track = staffIdx * VOICES;
Breath* underlyingBreath = toBreath(underlyingBreathSeg->element(track));
if (underlyingBreath) {
Breath* mmrBreath = toBreath(mmrBreathSeg->element(track));
if (!mmrBreath) {
mmrBreath = underlyingBreath->generated() ? underlyingBreath->clone() : toBreath(
underlyingBreath->linkedClone());
mmrBreath->setParent(mmrBreathSeg);
ctx.mutDom().doUndoAddElement(mmrBreath);
} else {
TLayout::layoutBreath(mmrBreath, mmrBreath->mutldata(), ctx.conf());
}
+ } else {
+ if (Breath* mmrBreath = toBreath(mmrBreathSeg->element(track))) {
+ ctx.mutDom().doUndoRemoveElement(mmrBreath);
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engraving/rendering/score/measurelayout.cpp` around lines 442 - 470, When
underlyingBreathSeg exists but for a given track there is no underlyingBreath,
the code currently leaves an existing mmrBreath in place; update the loop that
iterates staffIdx/track (in the block that references underlyingBreathSeg,
mmrBreathSeg, Breath, toBreath, and ctx.mutDom()) so that when
toBreath(underlyingBreathSeg->element(track)) returns null and
toBreath(mmrBreathSeg->element(track)) returns non-null you remove the stale
mmrBreath by calling ctx.mutDom().doUndoRemoveElement(mmrBreath) (and avoid
further layout/clone logic for that track); this ensures per-track mmr breaths
are deleted when the underlying track no longer has a breath.
davidstephengrant
added
the
vtests
|
@miiizen When breath marks are splitting an mmrest, edits to mmrest appearance properties or turning mmrests on/off Screencast.from.2026-04-13.11-44-56.webm |
miiizen
force-pushed
the
31586-breathMMR
branch
from
9ca8816 to
3a71911
Compare
|
@davidstephengrant Fixed! |
|
@miiizen Replacing breath marks on mmrests is behaving erratically. You can reach an odd state where different breath marks are drawn depending on if mmrests are enabled or disabled. Screencast.from.2026-04-14.09-34-01.webm |
miiizen
force-pushed
the
31586-breathMMR
branch
from
3a71911 to
eec43ee
Compare
|
In a new score with mmrests off:
This persists saving and reloading the score - but - toggling mmrests off, saving, reloading then toggling mmrests back on causes both the breath marks to draw correctly. Screencast.from.2026-04-15.11-22-41.webm |
|
In a new score with mmrests off:
Screencast.from.2026-04-15.11-43-22.webm |
miiizen
force-pushed
the
31586-breathMMR
branch
from
eec43ee to
d3962a8
Compare
miiizen
changed the title
|
@miiizen Please rebase |
|
@miiizen For now I'm adding some findings here. However, as MMRests on the whole are much improved, it might be worth merging this PR as-is and logging outstanding issues seperately. |
|
Layout objects are not added/removed correctly at the end of multimeasure rests Adding a layout object to a bar where an identical object already exists will usually remove the existing object (effectively working like a toggle). At the end of multimeasure rests multiple layout object icons are instead added. Saving and reloading the score causes a single icon to be drawn, but the toggling behaviour is nonetheless broken. Screencast.from.2026-05-25.10-12-17.webm |
mike-spa
left a comment
mike-spa
left a comment
There was a problem hiding this comment.
Absolutely outstanding work, I am SO HAPPY that we've finally cleaned this up
| <right-margin>0</right-margin> | ||
| </system-margins> | ||
| <system-distance>87.26</system-distance> | ||
| <system-distance>-40</system-distance> |
There was a problem hiding this comment.
This is bad. There must never be negative values. (This is what this test was for originally.)
There was a problem hiding this comment.
Thanks for pointing this out - fixed in #33837
Backport of musescore#32900, commit 1 Co-Authored-By: James Mizen <26510874+miiizen@users.noreply.github.com>
Backport of musescore#32900, commit 4 Co-Authored-By: James Mizen <26510874+miiizen@users.noreply.github.com>
Backport of musescore#32900, commit 1 Co-Authored-By: James Mizen <26510874+miiizen@users.noreply.github.com>
Backport of musescore#32900, commit 4 Co-Authored-By: James Mizen <26510874+miiizen@users.noreply.github.com>
Backport of musescore#32900, commit 1 Co-Authored-By: James Mizen <26510874+miiizen@users.noreply.github.com>
Backport of musescore#32900, commit 4 Co-Authored-By: James Mizen <26510874+miiizen@users.noreply.github.com>
5 participants
Resolves: #31586
Resolves: #32845
Resolves: #27880
Resolves: #30890
Resolves: #32663