Fix unnecessary notehead separation for unison notes of different durations#33206
Fix unnecessary notehead separation for unison notes of different durations#33206julianz- wants to merge 1 commit intomusescore:masterfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
julianz-
force-pushed
the
notehead-unison-fix
branch
from
d82fe2b to
f6f48d2
Compare
|
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:
📝 WalkthroughWalkthroughUnison notehead-sharing logic was relaxed: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/chordlayout.cpp`:
- Around line 1995-2017: The hunk's brace/blank-line formatting violates the
project's uncrustify style and is causing the codestyle job to fail; run the
repository's uncrustify/codestyle formatter to normalize spacing or manually fix
the blank-line and brace placement in this block so it matches the project's
style. Focus on the conditional block around variables and symbols like
unisonMatch, nHeadType, pHeadType, nchord->dots(), pchord->dots(), shareHeads,
and the subsequent NoteHeadType::HEAD_QUARTER / setHidden branches so braces and
blank lines follow the project's formatter output.
- Around line 1996-2017: The unison-sharing logic currently hides a quarter
notehead based only on nHeadType != pHeadType and unisonMatch, which
accidentally overrides explicit notehead choices when one side is auto-derived;
update the branch that handles unison head-type conflicts (the else if with
condition "nHeadType != pHeadType && unisonMatch") to skip hiding when exactly
one side is auto (i.e., preserve explicit overrides). Concretely, only perform
the existing setHidden(true) logic on n or p when both notes have headType() ==
NoteHeadType::HEAD_AUTO or when both are explicitly set (both headType() !=
HEAD_AUTO); if one side is auto and the other explicit, do not hide either head
— this change touches the unisonMatch check, the nHeadType/pHeadType comparison,
and the calls to n->headType(), p->headType(), n->setHidden(), p->setHidden().
In `@src/engraving/rendering/score/tlayout.cpp`:
- Line 4150: The else-if condition using NoteHeadType::HEAD_QUARTER contains a
spacing typo around the item->hidden() call; update the condition in tlayout.cpp
(the else if that checks NoteHeadType::HEAD_QUARTER && item->hidden()) to remove
the extra space before the closing parenthesis so it reads with correct
parenthesis spacing (e.g., item->hidden()) and matches surrounding codestyle for
conditional spacing.
🪄 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: CHILL
Plan: Pro
Run ID: 7b0f2504-9bac-45d9-904c-c3c1a4394450
📒 Files selected for processing (2)
src/engraving/rendering/score/chordlayout.cppsrc/engraving/rendering/score/tlayout.cpp
|
|
||
| // check if the notes are unison | ||
| bool unisonMatch = (n->pitch() == p->pitch() && n->tpc() == p->tpc()); | ||
|
|
||
| // for unisons we allow half noteheads to merge even with different durations, | ||
| // as the stem, flag, or beam makes the duration clear. | ||
| // dotted notes can also merge with non-dotted notes at unison. | ||
| if (((!unisonMatch && nchord->dots() != pchord->dots()) || !nchord->stem() || !pchord->stem() | ||
| || (nHeadType != pHeadType && !unisonMatch) | ||
| || n->isSmall() || p->isSmall()) | ||
| && ((n->headType() == NoteHeadType::HEAD_AUTO && p->headType() == NoteHeadType::HEAD_AUTO) | ||
| || nHeadType != pHeadType) | ||
| && (n->visible() == p->visible())) { | ||
| shareHeads = false; | ||
| } else if (nHeadType != pHeadType && unisonMatch) { | ||
| // ensure if a unison includes an open notehead this is shown in | ||
| // preference to a filled notehead | ||
| if (nHeadType == NoteHeadType::HEAD_QUARTER) { | ||
| n->setHidden(true); | ||
| } | ||
| else if (pHeadType == NoteHeadType::HEAD_QUARTER) { | ||
| p->setHidden(true); | ||
| } |
There was a problem hiding this comment.
Run uncrustify on this hunk before merge.
The codestyle job is currently failing on this block, so the PR will stay red until the blank-line/brace formatting here is normalized. Running tools/codestyle/uncrustify_run_file.sh on this file should fix it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engraving/rendering/score/chordlayout.cpp` around lines 1995 - 2017, The
hunk's brace/blank-line formatting violates the project's uncrustify style and
is causing the codestyle job to fail; run the repository's uncrustify/codestyle
formatter to normalize spacing or manually fix the blank-line and brace
placement in this block so it matches the project's style. Focus on the
conditional block around variables and symbols like unisonMatch, nHeadType,
pHeadType, nchord->dots(), pchord->dots(), shareHeads, and the subsequent
NoteHeadType::HEAD_QUARTER / setHidden branches so braces and blank lines follow
the project's formatter output.
f9f1cd5 to
2dd9251
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/chordlayout.cpp`:
- Around line 2009-2016: The current logic in the chord-layout branch (when
nHeadType != pHeadType && unisonMatch) uses n->setHidden(true) /
p->setHidden(true) which hides the whole Note (Note::setHidden) and suppresses
rendering/MIDI/voice behavior; change this to suppress only the notehead
glyph/symbol instead of the entire Note: replace the n->setHidden(true) and
p->setHidden(true) calls with a notehead-only suppression API (e.g. call
Note::setHeadHidden(true) or Note::setNoteheadVisible(false) if such exists), or
if no API exists add a lightweight Note::setHeadHidden(bool) (and corresponding
head-only render check) so the quarter head is not drawn but the Note remains
active for rendering paths and MIDI overlap handling; keep the same conditional
logic around nHeadType/pHeadType/unisonMatch and update render code to respect
the new head-only flag.
🪄 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: CHILL
Plan: Pro
Run ID: c9f0607c-3433-4a76-aeb4-e0e5fd069877
📒 Files selected for processing (2)
src/engraving/rendering/score/chordlayout.cppsrc/engraving/rendering/score/tlayout.cpp
…ations When two notes share the same pitch and spelling (unison), MuseScore was forcing them apart even when their noteheads could be shared. This change allows unison notes to share a notehead regardless of duration difference, hiding the shorter note's filled notehead in favour of the open one. Dot differences are also no longer treated as a reason to separate unison notes, since the dot can survive independently while the noteheads are combined. Notes with explicitly set notehead types are unaffected, providing a natural override for cases where separation is intentional.
julianz-
force-pushed
the
notehead-unison-fix
branch
from
2dd9251 to
35780ed
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/engraving/rendering/score/chordlayout.cpp (1)
2002-2016:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExplicit notehead overrides can still be auto-merged in unisons.
At Line 2009, the quarter-head hiding branch still runs for unison mismatches even when one side is explicit and the other is auto-derived. That can override intentional separation. Restrict this path to auto/auto pairs (and same-visibility pairs) before applying
setHideNotehead(true).Suggested fix
+ const bool bothAutoHeadTypes + = n->headType() == NoteHeadType::HEAD_AUTO + && p->headType() == NoteHeadType::HEAD_AUTO; + const bool sameVisibility = (n->visible() == p->visible()); + if (((!unisonMatch && nchord->dots() != pchord->dots()) || !nchord->stem() || !pchord->stem() - || (nHeadType != pHeadType && !unisonMatch) + || (nHeadType != pHeadType && (!unisonMatch || !bothAutoHeadTypes)) || n->isSmall() || p->isSmall()) - && ((n->headType() == NoteHeadType::HEAD_AUTO && p->headType() == NoteHeadType::HEAD_AUTO) + && (bothAutoHeadTypes || nHeadType != pHeadType) - && (n->visible() == p->visible())) { + && sameVisibility) { shareHeads = false; - } else if (nHeadType != pHeadType && unisonMatch) { + } else if (bothAutoHeadTypes && sameVisibility && nHeadType != pHeadType && unisonMatch) { if (nHeadType == NoteHeadType::HEAD_QUARTER) { n->setHideNotehead(true); } else if (pHeadType == NoteHeadType::HEAD_QUARTER) { p->setHideNotehead(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/rendering/score/chordlayout.cpp` around lines 2002 - 2016, The unison branch that hides quarter noteheads can override explicit notehead choices; update the conditional before calling n->setHideNotehead(true) / p->setHideNotehead(true) in the chord-layout logic so it only runs when both notes are auto-derived and have matching visibility — i.e., add checks that n->headType() == NoteHeadType::HEAD_AUTO && p->headType() == NoteHeadType::HEAD_AUTO (or equivalent stored booleans) and n->visible() == p->visible() in the else-if (unisonMatch && nHeadType != pHeadType) path so explicit/auto pairs are not auto-merged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/engraving/rendering/score/chordlayout.cpp`:
- Around line 2002-2016: The unison branch that hides quarter noteheads can
override explicit notehead choices; update the conditional before calling
n->setHideNotehead(true) / p->setHideNotehead(true) in the chord-layout logic so
it only runs when both notes are auto-derived and have matching visibility —
i.e., add checks that n->headType() == NoteHeadType::HEAD_AUTO && p->headType()
== NoteHeadType::HEAD_AUTO (or equivalent stored booleans) and n->visible() ==
p->visible() in the else-if (unisonMatch && nHeadType != pHeadType) path so
explicit/auto pairs are not auto-merged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46f26657-3513-4c0a-a780-5d1b861e7125
📒 Files selected for processing (3)
src/engraving/dom/note.hsrc/engraving/rendering/score/chordlayout.cppsrc/engraving/rendering/score/tlayout.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engraving/rendering/score/tlayout.cpp
|
Hmm, I have some major reservations with this. First, it's already completely possible to force the non-matching noteheads to overlap - just make the shorter notehead invisible, or set its notehead explicitly to match the longer. And that's exactly what people have been doing for years when they want to achieve this result. So what we're really talking about here is changing the default, not enabling something that was never possible before. And changing it such that the method that formerly forced noteheads to merge now forces the opposite. So, second, if we're going to change the default - and the behavior of the existing override - we need to handle this in a way that preserves compatibility for all the existing scores that rely on the existing behavior (both the existing default and the existing override). So, yes, it definitely needs to be an option, and there needs to be handling to make sure older scores are not adversely affected. Third, I am not at all convinced it makes sense to change the default in the first place. Yes, obviously there do exist cases where sharing the notehead makes sense. But separate noteheads for filled and open notes - and for dotted and undotted notes - are absolutely required in many cases, such as vocal music with separate parts reading off the same staff, or piano or guitar music that is inherently contrapuntal and needs to show the independent lines. It's really only a few pretty special cases - e.g., piano/guitar/harp music with repeated patterns of arpeggios over sustained bass notes - where it's appropriate to merge them. If we're going to change the default to make those cases work more automatically, we could limit it to piano/guitar/harp, or least exclude vocal staves from the equation. Or making it a staff and not a style setting. We could also consider limiting it to cases where the filled notes are eighth notes or shorter - it would be less common to want to share quarters and halves. Without some additional refinement of the algorithm to not always merge non-matching durations, I suspect this will actually create more work for users overall. But mostly, this strikes me as an unnecessary change even if all these precautionary steps were taken. |
|
@MarcSabatella Thanks for insightful comments. I was not aware of the trick for rendering unisons by making one note invisible. I would say this is not a very intuitive solution though as I would assume by making one note invisible all traces of it would disappear whereas what I would like is for the two notes to merge in some controllable way. But there's another potentially bigger problem and that's with representing the result in musicxml. My idea for this fix came from loading musicxml from this dataset: https://github.com/fosfrancesco/asap-dataset/. If you look at the copy of Chopin's Ballade No 1 in G minor, measure 8 was something of a disaster with multiple notation errors: 
Some of this came from various performance hacks the dataset owner had applied which I was not interested in. I tried to fix the xml so that the notation would look more like how this part is typically printed and found that I could not achieve a good result. My fix enables me to load from musicxml this version (which is close to how this phrase is printed in the Polish Instytut Fryderyka Chopina edition): 
I just tried using your solution with the invisible notes and was able to achieve the same thing but when saving to musicxml and then reloading from musicxml, the empty notehead for the eighth note C is filled in. 
Looking at the musicxml, I see invisibility is not encoded so unless I am missing something your solution does not work for roundtripping to musicxml and back? Nevertheless, I do take your point about backward compatibility. Perhaps this should not be a default but rather a note property or style option that can be explicitly set. So unless there is another way to fix the musicxml representation issue, I'd be happy to think about a revision that avoided making this the default. Let me know your thoughts. |
|
The override to merge the notes is to make the note head invisible. If you make the entire note invisible, that does indeed hide all trace of it. Or, as I said, explicitly override the visual duration - the exact same method you were proposing to use. It's true that no matter which way you make the default, it's not obvious how to override it, so adding a simple checkbox to the Properties panel would be another possible solution. Or just be sure the app[roriate Handbook section is clearly written. MusicXML is not a perfect representation of the score, which is why we don't say you can "save" to it - only "export". but visibility is definitely encoded. if it weren't, you wouldn't see the notes merged in your example. I think you are just being confused by the fact that be defualt, invisible elements do still display on screen as greyed out so they can be easily selected and edited. But you can turn off via the View menu. And if you use the other method I suggested - overriding the visual duration of the shorter note to match the longer - that also gets exported to the MusicXML and then reimported correctly. In any case, I think you should think about these considerations and then if you have a proposal for how you think things can be improved, open a new feature request issue with your proposal. And then see what kind of feedback it gets from others, including the design team, before worrying about attempting to implement it. |
|
For the record, this is how the invisible notehead method gets exported as MusicXML: And here is how the explicit visual duration overrriude is encoded: In both cases, it's the "notehead" tag that is used. |
|
Ok if use If I use 
In the end I had to fix by manually editing the XML. It may be that I am not using the UI correctly but on the example above the UI is grayed out for changing the notehead for both notes in the unison. So my feeling is there is a bug in the UI too. Either that or the UI is causing a lot of confusion. It shouldn't be that hard to fix this kind of thing. Update: in the end I could fix the above example in the UI by playing with the visibility of the notes. Changing visibility on the noteheads did not appear to work properly though. |
|
As I said, invisible objects are shown as gray on screen by defaulty, so yes, it's correct that if your MusicXML calls for an object to be rendered as invisible, you'll see it gray onscreen. And yes, it's correct that the option to hide the notehead in the UI does the same thing. As I explained, that is to make it possible to select and edit the invisible object later. The grayed-out object t won't export or print that way, though - it really is treated as invisible. And again, if you wish to make the invisible object completely invisible even in the on-screen preview, just turn off the option to show invisible elements in the View menu. All of this works exactly as intended. If you expect the noteheads to be merged, then the MusicXML file you have is incorrect and you should indeed consider fixing it there. Or fix it in the UI if you prefer. Either way, the program is working as intended, and even to the extent it might be possible to improve the defaults or the discoverability of the existing overrides, I don't think this PR is an appropriate way to achieve either goal. |
5 participants
When two notes share the same pitch and spelling (unison), MuseScore was forcing them apart even when their noteheads could be shared. This change allows unison notes to share a notehead regardless of duration difference, hiding the shorter note's filled notehead in favour of the open one.
Dot differences are also no longer treated as a reason to separate unison notes, since the dot can survive independently while the noteheads are combined.
Notes with explicitly set notehead types are unaffected, providing a natural override for cases where separation is intentional.