Fix regroup rhythms: split 8th note at 16th-note subbeat crossing a quarter beat#33637
Closed
csimmons0 wants to merge 1 commit into
Closed
Fix regroup rhythms: split 8th note at 16th-note subbeat crossing a quarter beat#33637csimmons0 wants to merge 1 commit into
csimmons0 wants to merge 1 commit into
Conversation
…uarter beat Previously, an eighth note starting at a sixteenth-note subbeat position and crossing a quarter-beat boundary was incorrectly preserved as a single note, instead of being split into two tied sixteenth notes. Root cause: populateRhythmicList() had a 'level-2 syncopation' check that suppressed splitting for notes where startLevel == endLevel and the strongest beat crossed was 2 levels stronger (startLevel - 2). The condition ticksToNext != ticksPastPrev was intended to reject non-centered notes, but this is always false for any note at a level-L subbeat position (such notes are always exactly equidistant from adjacent level-(L-1) subbeat positions). So the check was unconditionally suppressing the split. Fix: Add a length check: the note must span at least one full unit of the crossed beat level to qualify as a valid syncopation. Specifically: l.ticks() >= nominal.subbeatTicks(strongestLevelCrossed) In 4/4 time, subbeatTicks(0) = 480 ticks (a quarter note). An eighth note is 240 ticks < 480, so it fails the check and is correctly split into two tied sixteenth notes. A quarter note at a 16th-note position (480 >= 480) still qualifies as a valid syncopation and is preserved. An eighth note at a 32nd-note position crossing an eighth-beat boundary also qualifies (240 >= subbeatTicks(1) = 240). Also fix a compile error in notationviewinputcontroller_tests.cpp: remove a dangling conditional block that was placed after a statement-terminating semicolon, causing syntax errors on Qt < 6.9. Note: The rhythmic grouping reference test files (groupSubbeats-ref.mscx, groupConflicts-ref.mscx, groupShortenNotes-ref.mscx) need to be regenerated by running the engraving tests with a Qt 6.8+ build environment. The EID system assigns sequential IDs at write time, so splitting notes adds new elements that cascade-shift all subsequent EIDs in the file. Fixes: musescore#17322 https://claude.ai/code/session_01BLLP1CS39qygdYGC5oePKZ
Contributor
|
Please read, understand, and complete the contributor checklist before opening a PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, an eighth note starting at a sixteenth-note subbeat position
and crossing a quarter-beat boundary was incorrectly preserved as a single
note, instead of being split into two tied sixteenth notes.
Root cause: populateRhythmicList() had a 'level-2 syncopation' check that
suppressed splitting for notes where startLevel == endLevel and the strongest
beat crossed was 2 levels stronger (startLevel - 2). The condition
ticksToNext != ticksPastPrev
was intended to reject non-centered notes, but this is always false for any
note at a level-L subbeat position (such notes are always exactly equidistant
from adjacent level-(L-1) subbeat positions). So the check was unconditionally
suppressing the split.
Fix: Add a length check: the note must span at least one full unit of the
crossed beat level to qualify as a valid syncopation. Specifically:
l.ticks() >= nominal.subbeatTicks(strongestLevelCrossed)
In 4/4 time, subbeatTicks(0) = 480 ticks (a quarter note). An eighth note
is 240 ticks < 480, so it fails the check and is correctly split into two
tied sixteenth notes. A quarter note at a 16th-note position (480 >= 480)
still qualifies as a valid syncopation and is preserved. An eighth note at
a 32nd-note position crossing an eighth-beat boundary also qualifies
(240 >= subbeatTicks(1) = 240).
Also fix a compile error in notationviewinputcontroller_tests.cpp: remove a
dangling conditional block that was placed after a statement-terminating
semicolon, causing syntax errors on Qt < 6.9.
Note: The rhythmic grouping reference test files (groupSubbeats-ref.mscx,
groupConflicts-ref.mscx, groupShortenNotes-ref.mscx) need to be regenerated
by running the engraving tests with a Qt 6.8+ build environment. The EID
system assigns sequential IDs at write time, so splitting notes adds new
elements that cascade-shift all subsequent EIDs in the file.
Fixes: #17322
https://claude.ai/code/session_01BLLP1CS39qygdYGC5oePKZ
Resolves: #NNNNN