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.

Sign up for GitHub

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

Musicxml duration 3.x #8282

Closed
wants to merge 2 commits into from
Closed

Musicxml duration 3.x #8282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e9eea7
Fix #321751 - [MusicXML import] use note type instead of note duratio…
lvinken May 21, 2021
a8731c2
Fix #321753 - [MusicXML import] enable import of MusicXML note durati…
lvinken May 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 38 additions & 21 deletions importexport/musicxml/importmxmlnoteduration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,30 +102,44 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
QString errorStr;

// normalize duration
if (_dura.isValid())
_dura.reduce();
if (_specDura.isValid())
_specDura.reduce();

const auto calcDura = calculateFraction(type, _dots, _timeMod);
if (_dura.isValid() && calcDura.isValid()) {
if (_dura != calcDura) {
// default: use calculated duration
_calcDura = calculateFraction(type, _dots, _timeMod);
if (_calcDura.isValid()) {
_dura = _calcDura;
}

if (_specDura.isValid() && _calcDura.isValid()) {
if (_specDura != _calcDura) {
errorStr = QString("calculated duration (%1) not equal to specified duration (%2)")
.arg(calcDura.print(), _dura.print());
.arg(_calcDura.print(), _specDura.print());
//qDebug("rest %d type '%s' timemod %s", rest, qPrintable(type), qPrintable(_timeMod.print()));

if (rest && type == "whole" && _dura.isValid()) {
if (rest && type == "whole" && _specDura.isValid()) {
// Sibelius whole measure rest (not an error)
errorStr = "";
_dura = _specDura;
}
else if (grace && _dura == Fraction(0, 1)) {
else if (grace && _specDura == Fraction(0, 1)) {
// grace note (not an error)
errorStr = "";
_dura = _specDura;
}
else {
const int maxDiff = 3; // maximum difference considered a rounding error
if (qAbs(calcDura.ticks() - _dura.ticks()) <= maxDiff) {
// note:
// rounding error detection may conflict with changed duration detection (Overture).
// exporters that intentionally change note duration typically produce a large difference,
// most likely in practice this will not be an issue
const int maxDiff = 2; // maximum difference considered a rounding error
if (qAbs(_calcDura.ticks() - _specDura.ticks()) <= maxDiff) {
errorStr += " -> assuming rounding error";
_dura = calcDura;
_dura = _calcDura;
_specDura = _calcDura; // prevent changing off time
}
else
errorStr += " -> using calculated duration";
}

// Special case:
Expand All @@ -134,22 +148,25 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
// based on note type. If actual is 2/3 of expected, the rest is part
// of a tuplet.
if (rest) {
if (2 * calcDura.ticks() == 3 * _dura.ticks()) {
if (2 * _calcDura.ticks() == 3 * _specDura.ticks()) {
_timeMod = Fraction(2, 3);
errorStr += " -> assuming triplet";
errorStr += errorStr.isEmpty() ? " ->" : ",";
errorStr += " assuming triplet";
_dura = _specDura;
}
}
}
}
else if (_dura.isValid()) {
else if (_specDura.isValid() && !_calcDura.isValid()) {
// do not report an error for typeless (whole measure) rests
if (!(rest && type == ""))
if (!(rest && type == "")) {
errorStr = "calculated duration invalid, using specified duration";
}
_dura = _specDura;
}
else if (calcDura.isValid()) {
else if (!_specDura.isValid() && _calcDura.isValid()) {
if (!grace) {
errorStr = "specified duration invalid, using calculated duration";
_dura = calcDura; // overrule dura
}
}
else {
Expand All @@ -173,19 +190,19 @@ void mxmlNoteDuration::duration(QXmlStreamReader& e)
Q_ASSERT(e.isStartElement() && e.name() == "duration");
_logger->logDebugTrace("MusicXMLParserPass1::duration", &e);

_dura.set(0, 0); // invalid unless set correctly
_specDura.set(0, 0); // invalid unless set correctly
int intDura = e.readElementText().toInt();
if (intDura > 0) {
if (_divs > 0) {
_dura.set(intDura, 4 * _divs);
_dura.reduce(); // prevent overflow in later Fraction operations
_specDura.set(intDura, 4 * _divs);
_specDura.reduce(); // prevent overflow in later Fraction operations
}
else
_logger->logError("illegal or uninitialized divisions", &e);
}
else
_logger->logError("illegal duration", &e);
//qDebug("duration %s valid %d", qPrintable(dura.print()), dura.isValid());
//qDebug("specified duration %s valid %d", qPrintable(_specDura.print()), _specDura.isValid());
}

//---------------------------------------------------------
Expand Down
6 changes: 5 additions & 1 deletion importexport/musicxml/importmxmlnoteduration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ class mxmlNoteDuration
public:
mxmlNoteDuration(int divs, MxmlLogger* logger) : _divs(divs), _logger(logger) { /* nothing so far */ }
QString checkTiming(const QString& type, const bool rest, const bool grace);
Fraction dura() const { return _dura; }
Fraction duration() const { return _dura; } // duration to use
Fraction calculatedDuration() const { return _calcDura; } // value calculated from note type etcetera
Fraction specifiedDuration() const { return _specDura; } // value read from the duration element
int dots() const { return _dots; }
TDuration normalType() const { return _normalType; }
bool readProperties(QXmlStreamReader& e);
Expand All @@ -44,6 +46,8 @@ class mxmlNoteDuration
void timeModification(QXmlStreamReader& e);
const int _divs; // the current divisions value
int _dots = 0;
Fraction _calcDura;
Fraction _specDura;
Fraction _dura;
TDuration _normalType;
Fraction _timeMod { 1, 1 }; // default to no time modification
Expand Down
4 changes: 2 additions & 2 deletions importexport/musicxml/importmxmlpass1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3236,7 +3236,7 @@ void MusicXMLParserPass1::note(const QString& partId,
// check for timing error(s) and set dura
// keep in this order as checkTiming() might change dura
auto errorStr = mnd.checkTiming(type, bRest, grace);
dura = mnd.dura();
dura = mnd.duration();
if (errorStr != "")
_logger->logError(errorStr, &_e);

Expand All @@ -3251,7 +3251,7 @@ void MusicXMLParserPass1::note(const QString& partId,
// do tuplet
auto timeMod = mnd.timeMod();
auto& tupletState = tupletStates[voice];
tupletState.determineTupletAction(mnd.dura(), timeMod, tupletStartStop, mnd.normalType(), missingPrev, missingCurr);
tupletState.determineTupletAction(mnd.duration(), timeMod, tupletStartStop, mnd.normalType(), missingPrev, missingCurr);
}

// store result
Expand Down
23 changes: 21 additions & 2 deletions importexport/musicxml/importmxmlpass2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4424,7 +4424,7 @@ Note* MusicXMLParserPass2::note(const QString& partId,
// check for timing error(s) and set dura
// keep in this order as checkTiming() might change dura
auto errorStr = mnd.checkTiming(type, rest, grace);
dura = mnd.dura();
dura = mnd.duration();
if (errorStr != "")
_logger->logError(errorStr, &_e);

Expand Down Expand Up @@ -4455,7 +4455,7 @@ Note* MusicXMLParserPass2::note(const QString& partId,
if (!chord && !grace) {
auto& tuplet = tuplets[voice];
auto& tupletState = tupletStates[voice];
tupletAction = tupletState.determineTupletAction(mnd.dura(), timeMod, notations.tupletDesc().type, mnd.normalType(), missingPrev, missingCurr);
tupletAction = tupletState.determineTupletAction(mnd.duration(), timeMod, notations.tupletDesc().type, mnd.normalType(), missingPrev, missingCurr);
if (tupletAction & MxmlTupletFlag::STOP_PREVIOUS) {
// tuplet start while already in tuplet
if (missingPrev.isValid() && missingPrev > Fraction(0, 1)) {
Expand Down Expand Up @@ -4575,6 +4575,25 @@ Note* MusicXMLParserPass2::note(const QString& partId,
setNoteHead(note, noteheadColor, noteheadParentheses, noteheadFilled);
note->setVisible(printObject); // TODO also set the stem to invisible

if (mnd.calculatedDuration().isValid()
&& mnd.specifiedDuration().isValid()
&& mnd.calculatedDuration().isNotZero()
&& mnd.calculatedDuration() != mnd.specifiedDuration()) {

// convert duration into note length
Fraction durationMult { (mnd.specifiedDuration() / mnd.calculatedDuration()).reduced() };
durationMult = (1000 * durationMult).reduced();
const int noteLen = durationMult.numerator() / durationMult.denominator();

NoteEventList nel;
NoteEvent ne;
ne.setLen(noteLen);
nel.append(ne);
note->setPlayEvents(nel);
if (c)
c->setPlayEventType(PlayEventType::User);
}

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

if (velocity > 0) {
note->setVeloType(Note::ValueType::USER_VAL);
note->setVeloOffset(velocity);
Expand Down
Loading