-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[MU4] Fix GH#8893: allow for 128th as the denominator for measures, prevent crash on shorter ones #8894
Conversation
52f7724
to
388f0c3
Compare
<property name="text"> | ||
<string>128</string> | ||
</property> | ||
</item> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this going to be replaced with a qml version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, it isn't yet though.
3c9a9b8
to
a620ec9
Compare
and report an errori rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
Backport of musescore#8894, part 2
and report an error rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
Backport of musescore#8894, part 2
if (ticks1.denominator() > 128 || ticks2.denominator() > 128) { | ||
MScore::setError(MsError::CANNOT_SPLIT_MEASURE_TOO_SHORT); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difficult to test, because of #8895. It doesn't crash, it doesn't split the measure, but also doesn't give any error message
The use of 64th as the denominator limit was also present in the MusicXML importer. |
Well, creating such a short measure (1/128) was possible before, via the split measure tool (and even shorter, leading to a crash, fixed with this PR). Just not the 'official' way, not via the measure poperties UI. But let's see what the mtests say about such a change. And/or what checking the artifacts does say. Capella import does allow for it too. But it seems BB and GTP import may not. Not sure whether this is because of MuseScore or because of BB and GTP though? Shouldn't matter much though, as we don't export to those. Indeed an mtest is failing: https://github.com/musescore/MuseScore/pull/8894/checks?check_run_id=3399699714#step:8:8215 Actually it seems that mtest is loosing exactly that unprinted/invisible 128th rest, which it should not have shown in the first place, and only does because before that change it can't have a measure with just a single 128th duration in it, or rather one with a 128th in the denominator. So simply that mtest needs to get adjusted. A musicxml file with a 1/128th measure does import cleanly into this PR with that 3rd commit, importing it without has a issue though, it imports as an 1/64th, but with 2 128th rests, one of which made invisible. That much seems unavoidable, the code not being prepared for such a short MusicXML measure due to that 64th rounding going on. And it did that before this PR too, see above. |
86c1c00
to
6319e8d
Compare
Backport of musescore#8894, part 3
plus fixing a typo found while searching for the old 64th limit Backport of musescore#8894, part 4
f824853
to
1c553bc
Compare
and report an error rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
64be75f
to
5727a54
Compare
and report an error rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
Backport of musescore#8894, part 2
Backport of musescore#8894, part 3
plus fixing a typo found while searching for the old 64th limit Backport of musescore#8894, part 4
and report an error rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
Backport of musescore#8894, part 2
Backport of musescore#8894, part 3
plus fixing a typo found while searching for the old 64th limit Backport of musescore#8894, part 4
and report an error rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
Backport of musescore#8894, part 2
Backport of musescore#8894, part 3
plus fixing a typo found while searching for the old 64th limit Backport of musescore#8894, part 4
plus fixing some typos found while searching the old 64th limit
5727a54
to
ca296eb
Compare
and report an error rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
Backport of musescore#8894, part 2
Backport of musescore#8894, part 3
plus fixing a typo found while searching for the old 64th limit Backport of musescore#8894, part 4
and report an error rather than crash if the score has something shorter See musescore#8890 and musescore#8893, backport of musescore#8894, part 1
Backport of musescore#8894, part 2
Backport of musescore#8894, part 3
plus fixing a typo found while searching for the old 64th limit Backport of musescore#8894, part 4
Partly resolves #8893
The same fixes apply to 3.x too BTW.