fix #50461: corruption on paste of septuplet #1879
Conversation
|
I'll pass on that one for now. The proposed fix crashes when copy pasting the measure from the example score at http://musescore.org/en/node/50461 |
|
Good catch! So, yes, that's fair enough. It's going to be a relatively hard case to hit, and it's easily seen and easily solved. So I won't lose sleep over this. Looking at the crash, I have a sense we do need to consider rounding down the tick counts of, and/or chaning this section of code to at least preserve the duration type. But it will be better to figure that out without the time pressure of an imminent release! |
|
I fixed the crash, which was caused by trying to tie the 3-ticks-too-long note over the barline. I simply added an exemption for tuplets to the code in pasteChordRest that splits a chord or rest over the barline. My sense is, this does fix the problem, and the tuplets thus created are identical in appearance and in internal structure to the original. So if the original tuplet (which is apparently 3 ticks too long) is considered OK, so should the copy. Of course, we'd want to put this fix through more testing before we can consider merging it. So that's why I'm reopening the PR - so people can review it, try it out, etc. |
MarcSabatella
force-pushed
the
50461-paste-septuplet
branch
from
92d28be
to
69b9454
Compare
|
FYI, I'm adding commits to this branch rather than creating separate branches to make it easier to test these changes all together. My plan is to run this branch all week (rebasing as necessary with other changes to the master) and look for regressions or other issues periodically. |
| @@ -443,7 +447,7 @@ void Score::pasteChordRest(ChordRest* cr, int tick, const Interval& srcTranspose | |||
|
|
|||
| int measureEnd = measure->endTick(); | |||
| bool isGrace = (cr->type() == Element::Type::CHORD) && (((Chord*)cr)->noteType() != NoteType::NORMAL); | |||
| if (!isGrace && (tick + cr->actualTicks() > measureEnd || convertMeasureRest)) { | |||
| if (!isGrace && !cr->tuplet() && (tick + cr->actualTicks() > measureEnd || convertMeasureRest)) { | |||
There was a problem hiding this comment.
I'm correct assuming we shouldn't be there if cr is a tuplet right? So this is a precaution in case of rounding. If this is really the case, could you add a comment?
There was a problem hiding this comment.
Well, it's not that we "shouldn't be there" if cr is a tuplet. If the last note of the tuplet extend to the barline, that's not a problem. The problem is that actualTicks() might be a little too long, so without this check, it might appear that the note won't fit in the measure in though it really does. Since we have already disallowed tuplets that cross barlines, it seems safe to simply skip this code for tuplets. And yes, a comment is a good idea, am adding it.
fix #50461: corruption on paste of septuplet
A fix I made just before beta 1 may have not gone far enough to avoid corrupting the end of a tuplet on paste. There is code to shorten the last note when pasting if necessary to fit the gap, to cover cases where a multi-voice selection includes a note that actual is longer than the selection itself. This code was shortening tuplets incorrectly.
My previous fix was here:
5752215
It took care of the basic case where there was no roundoff error or if the tick lengths in the tuplet were rounded down, so that the tick counts in the tuplet add up slightly short of the full value. But it still results in the tuplet being truncated if the tick counts add up to slightly more than the full value, and that's the case here. We could consider rounding tick counts down always as there are other bad effects from the overlap that results (it's hard to select just the tuplet), but that seems a riskier change, and in any case, my change here doesn't rule out make that other change as well.