Musicxml duration 3.x #8282

lvinken · 2021-06-08T19:43:35Z

Resolves:

321751:
Currently, the importer uses MusicXML note duration instead of note length as calculated from the MusicXML note
type, dots, time-modification etcetera to determine how long a note takes and where to place the next note. When
the MusicXML note duration does not match the calculated value, various types of corruption occur.

When using the calculated duration, none of these problems occur. This change does not affect correctly encoded
MusicXML files.

321753:
See discussion in https://musescore.org/en/node/313339. This change enables correct import of Overture files containing
changed note durations (while leaving the note type unchanged) by using the duration to change the note's off time.
Strictly speaking Overture does not adhere to the MusicMXL spec, but this change does not seem to introduce any regression. If necessary it can be made optional in future (either as a user setting or by making it Overture specific).

Providing a 3.x PR to benefit from the better auto test coverage and in case another 3.x release will be made.
Will also syncing to master shortly.

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

…n to determine note length Currently, the importer uses MusicXML note duration instead of note length as calculated from the MusicXML note type, dots, time-modification etcetera to determine how long a note takes and where to place the next note. When the MusicXML note duration does not match the calculated value, various types of corruption occur. When using the calculated duration, none of these problems occur. This change does not affect correctly encoded MusicXML files.

…on to set duration as shown in the piano roll editor See discussion in https://musescore.org/en/node/313339. This change enables correct import of Overture files containing changed note durations (while leaving the note type unchanged) by using the duration to change the note's off time. Strictly speaking Overture does not adhere to the MusicMXL spec, but this change does not seem to introduce any regression. If necessary it can be made optional in future (either as a user setting or by making it Overture specific).

…n to determine note length Currently, the importer uses MusicXML note duration instead of note length as calculated from the MusicXML note type, dots, time-modification etcetera to determine how long a note takes and where to place the next note. When the MusicXML note duration does not match the calculated value, various types of corruption occur. When using the calculated duration, none of these problems occur. This change does not affect correctly encoded MusicXML files. Duplicate of musescore#8282, resp. backport of musescore#8429, part 1

…on to set duration as shown in the piano roll editor See discussion in https://musescore.org/en/node/313339. This change enables correct import of Overture files containing changed note durations (while leaving the note type unchanged) by using the duration to change the note's off time. Strictly speaking Overture does not adhere to the MusicMXL spec, but this change does not seem to introduce any regression. If necessary it can be made optional in future (either as a user setting or by making it Overture specific). Duplicate of musescore#8282, resp. backport of musescore#8429, part 2

…n to determine note length Currently, the importer uses MusicXML note duration instead of note length as calculated from the MusicXML note type, dots, time-modification etcetera to determine how long a note takes and where to place the next note. When the MusicXML note duration does not match the calculated value, various types of corruption occur. When using the calculated duration, none of these problems occur. This change does not affect correctly encoded MusicXML files. Duplicate of musescore#8282, resp. backport of musescore#8429, part 1

…on to set duration as shown in the piano roll editor See discussion in https://musescore.org/en/node/313339. This change enables correct import of Overture files containing changed note durations (while leaving the note type unchanged) by using the duration to change the note's off time. Strictly speaking Overture does not adhere to the MusicMXL spec, but this change does not seem to introduce any regression. If necessary it can be made optional in future (either as a user setting or by making it Overture specific). Duplicate of musescore#8282, resp. backport of musescore#8429, part 2

Jojo-Schmitz · 2021-08-03T14:27:04Z

importexport/musicxml/importmxmlpass2.cpp

+                  if (c)
+                        c->setPlayEventType(PlayEventType::User);
+                  }
+


This change here somwhat collide with changes from #8581, results in mtest failures (in a branch having both these chenges):

62/62 Test #51: tst_mxml_io ......................***Failed 77.27 sec --- /home/runner/work/MuseScore/MuseScore/mtest/musicxml/io/testCueGraceNotes_ref.mscx 2021-08-03 11:40:32.492648198 +0000 +++ testCueGraceNotes.mscx 2021-08-03 12:05:19.714046284 +0000 @@ -153,6 +153,11 @@ <durationType>eighth</durationType> <acciaccatura/> Errors while running CTest <Note> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>65</pitch> <tpc>13</tpc> <play>0</play> @@ -233,6 +238,11 @@ <durationType>eighth</durationType> <acciaccatura/> <Note> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>69</pitch> <tpc>17</tpc> <play>0</play> @@ -300,6 +310,11 @@ <durationType>eighth</durationType> <acciaccatura/> <Note> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>69</pitch> <tpc>17</tpc> <play>0</play> @@ -386,6 +401,11 @@ <durationType>eighth</durationType> <acciaccatura/> <Note> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>67</pitch> <tpc>15</tpc> </Note> @@ -449,6 +469,11 @@ <durationType>eighth</durationType> <appoggiatura/> <Note> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>60</pitch> <tpc>14</tpc> </Note> @@ -493,6 +518,11 @@ <Accidental> <subtype>accidentalNatural</subtype> </Accidental> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>74</pitch> <tpc>16</tpc> </Note> @@ -645,6 +675,11 @@ <Accidental> <subtype>accidentalFlat</subtype> </Accidental> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>70</pitch> <tpc>12</tpc> <play>0</play> @@ -743,6 +778,11 @@ <Accidental> <subtype>accidentalSharp</subtype> </Accidental> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>66</pitch> <tpc>20</tpc> <play>0</play> @@ -807,6 +847,11 @@ <durationType>eighth</durationType> <acciaccatura/> <Note> + <Events> + <Event> + <len>0</len> + </Event> + </Events> <pitch>65</pitch> <tpc>13</tpc> <play>0</play> <diff -u /home/runner/work/MuseScore/MuseScore/mtest/musicxml/io/testCueGraceNotes_ref.mscx testCueGraceNotes.mscx failed---

Hmm, well, maybe not, hhttps://github.com//pull/8581/commits/3acbeaa56f9c525502b0451e9b6393e2649bdfad#diff-645ebec3e64371f077f82ba2d49a76539692a9788981503718fa72d6fb4d475bR5036-L5029 moves the check for grace notes up a bit, and sets cue notes to not play a bit earlier, so those <len>0</len> might be exactly what we want here?

Maybe @iveshenry18 can chip in here?

This would affect #8429 too, once #8581, gets ported to master?

The conflict with #8581 seems like it would be relatively simple to reconcile when porting to master; however, I also suspect that there would be a more significant conflict with an updated tuplet-rounding system I wrote in #8766

So we have to align on how to proceed. I would expect we will finally want to have master contain the sum of all mentioned PRs. I.e. we'l simply have to fix the conflicts.

And as the tuplet handling code (and especially the error handling) is not very good, implementing the updated tuplet-rounding system should help. I would like to review that later, as I cannot quickly judge the impact.

Sounds good!

MusicXML exporters tend to have a fixed maximum <divisions> value, meaning more complex tuplets (5's, 7's) have incorrect duration values. These were already handled for tuplets, but sometimes not handled for <backup> elements within or after a tuplet. This commit adds a more thorough system for rounding durations off to more sensible values. In doing so, it unifies the conversion process from <duration> to Fraction (which previously existed almost identically in three separate places) to pass1.calcTicks().

MusicXML exporters tend to have a fixed maximum <divisions> value, meaning more complex tuplets (5's, 7's) have incorrect duration values. These were already handled for tuplets, but sometimes not handled for <backup> elements within or after a tuplet. This commit adds a more thorough system for rounding durations off to more sensible values. In doing so, it unifies the conversion process from <duration> to Fraction (which previously existed almost identically in three separate places) to pass1.calcTicks(). Duplicate of musescore#8766, part 2, plus resolving a merge conflict with musescore#8282 (resp. 8429, for master) and fixing 2 compiler warnings.

and its collision with musescore#8282