Splitting irregular measures produce extra rests

• Mar 23, 2017 - 23:40
Reported version
2.1
Type
Functional
Severity
S2 - Critical
Status
closed
Project

See https://musescore.org/en/node/183801

1. create new score
2. select a measure and change actual duration to large number (like 15/4)
3. Enter various length notes to fill the measure
4. select a note and split measure before current note

result 1: second half of measure has rests randomly inserted into the measure
result 2: measure is reported as corrupted (with shorter duration) when score is saved and reopened.

See attached score for another example

Attachment Size
irregular_measures.mscz 7.23 KB

Comments

I have verified this bug 2.0.3, 2.1-dev 9ca7d0b, and 3.0-dev 90f9d78.

I am noting that the resulting split measure have the correct duration according to their measure properties, but the problem is that unnecessary rests are added (as the title says).

I'm noting that in 2.0.3 and 2.1 that if created a time signature of 15/4 before adding the notes and apply the split, then the result is correct in 2.0.3 and 2.1-dev. Although in 3.0-dev I ran into this problem when split a breve and dotted whole:

split-breve-dotted-whole.png

Other steps (with 2.0.3)

1) "My First Score"
2) Right-click first measure -> Measure properties -> Change "Actual" duration to 6/4 -> Ok
3) Enter two quarter notes

The test file at this step: split irregular.mscz

4) Select the second quarter note D -> Edit -> Measure -> Split measure before selected note

Result: in the second measure: corruption (extra rest)

After save/close/ reload: 1 split.mscz

- Note I observe the same behavior as much as I go back in time
Eg, image below produced in May 2014 with 56177c3
may.jpg

So sounds like this behavior was never correctly implemented to being with. Should I take a stab at this?
It seems like musescore is only looking at the time signature, but needs to look at the measure's actual duration instead. Because if I did place a 6/4 time sig between steps 3 & 4 in your above comment, then I get valid result:

add-time-sign-first-before-splitting.png

where that first measure's actual duration correctly becomes 1/4 and the second measures actual duration correctly becomes 5/4.

cmdSplitMeasure seems to be going along well

adjustToLen called for 1st measure with fraction 1/4 ...Good
adjustToLen called for 2nd measure with fraction 5/4 ...Good

until the very end "range.write(this, m1->tick());" which seems to be where to problem will be...

I'm also noting that happens with splitting an irregular measure containing only rests. For instance if have this irregular 6/4 measure:

6-4-before-split-after-one-quarter-rest.png

and select the 2nd quarter note and split it, the result is:

6-4-split-after-one-quarter-rest.png

Where what looks might look like a whole rest in the 2nd measure is actually a "measure rest" internally represented with a duration 5/4.

hmm...acutally I noticed in cmdSplitMeasure that m1 and m2 don't get their _irregular flag set true after
m1->adjustToLen(Fraction::fromTicks(ticks1));
m2->adjustToLen(Fraction::fromTicks(ticks2));
even though they end up with _len that doesn't match their _timesig
. Never mind, it appears the original measure is not _irregular to begin with...and i think that _irregular means something different than what is in this issue's title...

oh, this is interested. In cmdSplitMeasures, after the operations:

m1->adjustToLen(Fraction::fromTicks(ticks1));
m2->adjustToLen(Fraction::fromTicks(ticks2));

the result is m1 contains 0 segments, while m2 (which had 0 segments before) gets 1 segment of ChordRest. Also intestering is that m2 has both _segments _first and _last defined as different non-null pointers, even though the size of _segments is 1 (so I'm confused why _last would be something other than NULL). Also interesting is that the first segment is a ChordRest of a quarter rest. Probably this quarter rest is the superfluous rest that is causing the bug...

That additional quarter note at tick 2000 at the end of of the new 2nd measure in this if block near the end of Measure::adjustToLen():

                  if ((n > 0) && (rFlag || voice == 0)) {
                        // add rest to measure
                        int rtick = tick() + nl - n;
                        int track = staffIdx * VOICES + voice;
                        s->setRest(rtick, track, Fraction::fromTicks(n), false, 0, false);
                        }

If I comment that block out, then my mtest passes. (I'm not saying I'll comment out that block...what I am saying is that we need to make sure that block doesn't execute when splitting an irregular measure)

I've changed the function definiton of Measure::adjustToLen to have a default boolean parameter "appendRestsIfNecessary" which is set to true by default, but which I'm calling with false when when calling this function from cmdSplitMeasures. That way cmdSplitMeasures doesn't add rests so my test passes, but any other functionality for everyone else who calls it remains the same.

Everything seems to be working. I am noting the following behavior, which results from behavior that was already in the code. If I start with

split183846-irregular-wr-wr-wr-hf-qr.png

and split before the 3rd rest, then all the rests in each measure become merged into a single measure rest, of a duration equal to the duration of their irregular-length measure:

split183846-irregular-wr-wr-wr-hf-qr-ref.png

now to me, I'm not sure this is desired behavior...user may have organized the rests in that manner and would want to keep rests in same manner, but if this is indeed desired behavior, then I can't argue. Anyway, I'm just making a note for posterity...as it is I'm including this as one of my test cases, so if it gets merged then that will be solidified behavior...if that is not desired, then we should probably figure out a way to not have rests merged.

Ok, thanks for making a note to that feature request...I added a comment that I agreed with it.

So that means I won't include that test case, since I don't want to be stuck with the behavior or rests being merged into measure rests.

Status (old) patch (code needs review) fixed

Fixed in branch 2.1, commit aa3261eb96

fix #183846 split irreg-len meas shouldn't add rest

This fixes an bug which produced corrupted measures when splitting irregular length measures. In Score::cmdSplitMeasure() after the newly split measures are first created, if their new actual length was greater than their nominal length, then Measure::adjustToLen() would append additional padding rests at the end of the measure. However, the subsequent range.write() assumed that the measures were entirely empty before copying contents from the original measure into the new measures. The extra padding rests were unnecessary, and caused the resulting measures to contain too many notes than their actual length, hence the corruption.

The fix here is to add a default boolean parameter to adjustToLen() called appendRestsIfNecessary which is true by default so as to not change behavior when it is called without specifiying the parameter. However, cmdSplitMeasure() will call adjustToLen with that boolean explicitly false, so that the new measures don't get unnecessary rests.

Fixed in branch master, commit c08b67ff90

fix #183846 split irreg-len meas shouldn't add rest
This fixes an bug which produced corrupted measures when splitting irregular length measures. In Score::cmdSplitMeasure() after the newly split measures are first created, if their new actual length was greater than their nominal length, then Measure::adjustToLen() would append additional padding rests at the end of the measure. However, the subsequent range.write() assumed that the measures were entirely empty before copying contents from the original measure into the new measures. The extra padding rests were unnecessary, and caused the resulting measures to contain too many notes than their actual length, hence the corruption.

The fix here is to add a default boolean parameter to adjustToLen() called appendRestsIfNecessary which is true by default so as to not change behavior when it is called without specifiying the parameter. However, cmdSplitMeasure() will call adjustToLen with that boolean explicitly false, so that the new measures don't get unnecessary rests.