Skip to content
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 #272403: Corruption on pasting a range with a truncated note that requires a tie #3688

Merged

Conversation

mattmcclinch
Copy link
Contributor

See 272403: Corruption on pasting a range with a truncated note that requires a tie.

Score::pasteStaff() will shorten a ChordRest if necessary to make it fit into the gap created for it. This may result in a duration which is not achievable without ties (or multiple rests). The ChordRest is then assigned a duration type which may or may not match its actual duration. Score::pasteChordRest() can then check for this mismatch, and it is already able to do what needs to be done.

The example given in the bug report uses a compound meter. toRhythmicDurationList() does a better job of choosing appropriate TDurations for the individual ChordRests than toDurationList() in the case of a compound meter, and it handles simple meters just as well. The second argument to toRhythmicDurationList() has nothing to do with the second argument to toDurationList(). They just both happen to be of type bool.

@@ -482,7 +483,7 @@ void Score::pasteChordRest(ChordRest* cr, int tick, const Interval& srcTranspose
// we have already disallowed a tuplet from crossing the barline, so there is no problem here
// but due to rounding, it might appear from actualTicks() that the last note is too long by a couple of ticks

if (!isGrace && !cr->tuplet() && (tick + cr->actualTicks() > measureEnd || convertMeasureRest)) {
if (!isGrace && !cr->tuplet() && (tick + cr->actualTicks() > measureEnd || cr->durationTypeTicks() != cr->actualTicks() || convertMeasureRest)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good to have additional parenthesizes not to remember the operations sequence. If something like || vs && vs == is clear, but + in this sequence makes things worse. Could you please add the parenthesizes?

@mattmcclinch
Copy link
Contributor Author

As a result of the change to toRhythmicDurationList(), a couple tests are now failing due to a different sequence of durations being chosen for rests that have to be split up. I think toRhythmicDurationList() builds the more correct duration list in each case, given the beat (or subbeat) that the rest falls on.

First failed test: mtest/libmscore/copypaste/copypaste17.mscx
Expected result:
copypaste17-ref
New result:
copypaste17

Second failed test: mtest/libmscore/copypaste/copypaste20.mscx
Expected result:
copypaste20-ref
New result:
copypaste20

So to be clear, the "Expected result" is the result expected by mtest. The "New result" is the one that I believe to be more correct.

@mattmcclinch mattmcclinch force-pushed the 272403-truncate-chordrest branch 2 times, most recently from fcf88b6 to 6946a74 Compare May 23, 2018 14:40
@mattmcclinch
Copy link
Contributor Author

I modified copypaste17-ref.mscx and copypaste20-ref.mscx so mtest now expects the new results and the tests pass.

@anatoly-os
Copy link
Contributor

@mattmcclinch could you please rebase the commits onto master? This PR looks good.

@anatoly-os
Copy link
Contributor

@mattmcclinch could you please rebase the PR so it could be merged to master?

@mattmcclinch
Copy link
Contributor Author

I must have missed you asking me earlier. Or I might have been trying to focus on something else at the time. Either way, I have rebased the PR now.

@anatoly-os anatoly-os merged commit 4c6f877 into musescore:master Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants