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

[MU3] Fix #314899: Identical expression #7179

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Dec 29, 2020

Resolves: https://musescore.org/en/node/314899

Same fix needed in master, see #7180, or #7627, both closed by now.
And see #8762, which brought this up again.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Dec 29, 2020

The mtest fail (https://github.com/musescore/MuseScore/pull/7179/checks?check_run_id=1622482883#step:5:761) seems to be the result of this PR's change.

For some strange reason the corresponding PR for master builds cleanly though

@AntonioBL
Copy link
Contributor

AntonioBL commented Dec 29, 2020

The crash is caused by an assert at line 104 of importexport/midiimport/importmidi_simplify.cpp for test voiceSeparationTuple (i.e. importing voice_tuplet.mid)
QFATAL : TestImportMidi::voiceSeparationTuplet() unknown: ASSERT failure in Simplify::lengthenNote: "Too short note durations remaining", file /media/antonio/Mint0/MuseScore/importexport/midiimport/importmidi_simplify.cpp, line 104

@AntonioBL
Copy link
Contributor

And I think that in master importmidi mtests are not being tested at the moment.

@Jojo-Schmitz
Copy link
Contributor Author

It had to be one of those 4 Q_ASSERT_X, but why?

@AntonioBL
Copy link
Contributor

AntonioBL commented Dec 29, 2020

By using a break at that X_ASSERT (line 104 in 3.x and line 102 in master), the noteDurations list is:

first second
2/2 1/16 dots 0
2/2 1/128 dots 0
2/2 1/512 dots 0

which gives sum = 37/512, while the desiredLen (offset - DurationStart) is 7/96.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Dec 29, 2020

Close, but not the same, 0.072265625 vs 0.072916666

Guess a case of "never check for error conditions you can't handle"

Need to introduce some kind of an epsilon for this apparent floating point comparison?

@Jojo-Schmitz Jojo-Schmitz force-pushed the importmidi-simplify branch 2 times, most recently from 2ee11ba to 2ae2276 Compare December 30, 2020 09:41
@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Dec 30, 2020

Importing the "tuplet_3_5_7_tuplets.mid" file produces a corrupt MuseScore file (in 3.5.2 and 3.6 RC):
Measure 1, Staff 2 incomplete. Expected: 4/4; Found: 126/72
image
The reference score though looks very different:
image

It dies at an entirely different assert:

Q_ASSERT_X(!doesClefBreakTie(staff), "MidiClef::createClefs", "Clef breaks the tie");

and I don't see how that might be related to this PR here?

Not sure yet what's going wrong with the "voice_tuplet.mid" file/test, this is the one that fails the assertion at

Q_ASSERT_X(areDurationsEqual(noteDurations, offTime - durationStart),
"Simplify::lengthenNote", "Too short note durations remaining");

and as such is related to this PR

@AntonioBL
Copy link
Contributor

AntonioBL commented Dec 30, 2020

tuplet_3_5_7_tuplets.mid is imported exactly as the reference if "Split staff" is unchecked. And indeed in the mtest there is data.trackOpers.doStaffSplit.setDefaultValue(false, false);
Edit:
Actually, not exactly as the reference; in order to have it exactly as the reference, also "clef changes" must be unchecked (in the mtest data.trackOpers.changeClef.setDefaultValue(false, false); )
Edit 2:
But then, when playing with the import options and then exiting without saving, MuseScore 3.5.2 crashes

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Dec 30, 2020

For me that .mid file crashes with an assertion failure on load (in a debug build). Only when importing in a non-debug build I could save it as mscz (but without using any of those options) and got that corrupt score.
Anyway; I think this crash might be a red herring, as it crashes in an entirely different place, one that my change can't possibly have caused (famous last words?).

by removing the entire thing, it seems useless, esp. that
`return desiredLen == desiredLen;` and even harmful and
wrong anyway.
@Jojo-Schmitz Jojo-Schmitz marked this pull request as ready for review August 9, 2021 08:58
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 9, 2021
by removing the entire thing, it seems useless, esp. that
`return desiredLen == desiredLen;` and even harmful and
wrong anyway.

Duplicate of musescore#7179, but also part of the backport of musescore#8762
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 10, 2021
by removing the entire thing, it seems useless, esp. that
`return desiredLen == desiredLen;` and even harmful and
wrong anyway.

Duplicate of musescore#7179, but also part of the backport of musescore#8762
@RomanPudashkin RomanPudashkin merged commit 3ca622a into musescore:3.x Aug 12, 2021
@Jojo-Schmitz Jojo-Schmitz deleted the importmidi-simplify branch August 12, 2021 16:52
@Jojo-Schmitz
Copy link
Contributor Author

Want me to port it over to master?

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
counterpart to musescore#7179, but for master
@Jojo-Schmitz
Copy link
Contributor Author

That "Thumbs up" doesn't notify...
Anyway, see #8849

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.

3 participants