Skip to content

Commit

Permalink
ENG-85: Improve tuplet rounding error correction
Browse files Browse the repository at this point in the history 
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().
  • Loading branch information
@iveshenry18
iveshenry18 committed Jul 30, 2021
1 parent 86e9930 commit aae940d
Show file tree
Hide file tree
Showing 11 changed files with 4,673 additions and 66 deletions.
19 changes: 5 additions & 14 deletions importexport/musicxml/importmxmlnoteduration.cpp
View file
Expand Up @@ -121,9 +121,9 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
errorStr = "";
}
else {
const int maxDiff = 3; // maximum difference considered a rounding error
if (qAbs(calcDura.ticks() - _dura.ticks()) <= maxDiff) {
if (qAbs(calcDura.ticks() - _dura.ticks()) <= _pass1->maxDiff()) {
errorStr += " -> assuming rounding error";
_pass1->insertAdjustedDuration(_dura, calcDura);
_dura = calcDura;
}
}
Expand Down Expand Up @@ -157,6 +157,7 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
_dura = Fraction(4, 4);
}

_pass1->insertSeenDenominator(_dura.reduced().denominator());
return errorStr;
}

Expand All @@ -173,19 +174,9 @@ 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
_dura.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
}
else
_logger->logError("illegal or uninitialized divisions", &e);
}
else
_logger->logError("illegal duration", &e);
//qDebug("duration %s valid %d", qPrintable(dura.print()), dura.isValid());
_dura = _pass1->calcTicks(intDura, &e); // Duration reading (and rounding) code consolidated to pass1
}

//---------------------------------------------------------
Expand Down
5 changes: 4 additions & 1 deletion importexport/musicxml/importmxmlnoteduration.h
View file
Expand Up @@ -15,6 +15,7 @@

#include "libmscore/durationtype.h"
#include "libmscore/fraction.h"
#include "importmxmlpass1.h"

namespace Ms {

Expand All @@ -31,7 +32,8 @@ class MxmlLogger;
class mxmlNoteDuration
{
public:
mxmlNoteDuration(int divs, MxmlLogger* logger) : _divs(divs), _logger(logger) { /* nothing so far */ }
mxmlNoteDuration(int divs, MxmlLogger* logger, MusicXMLParserPass1* pass1) :
_divs(divs), _logger(logger), _pass1(pass1) { /* nothing so far */ }
QString checkTiming(const QString& type, const bool rest, const bool grace);
Fraction dura() const { return _dura; }
int dots() const { return _dots; }
Expand All @@ -47,6 +49,7 @@ class mxmlNoteDuration
Fraction _dura;
TDuration _normalType;
Fraction _timeMod { 1, 1 }; // default to no time modification
MusicXMLParserPass1* _pass1;
MxmlLogger* _logger; ///< Error logger
};

Expand Down
65 changes: 49 additions & 16 deletions importexport/musicxml/importmxmlpass1.cpp
View file
Expand Up @@ -3254,7 +3254,7 @@ void MusicXMLParserPass1::note(const QString& partId,
QString instrId;
MxmlStartStop tupletStartStop { MxmlStartStop::NONE };

mxmlNoteDuration mnd(_divs, _logger);
mxmlNoteDuration mnd(_divs, _logger, this);

while (_e.readNextStartElement()) {
if (mnd.readProperties(_e)) {
Expand Down Expand Up @@ -3419,6 +3419,49 @@ void MusicXMLParserPass1::notePrintSpacingNo(Fraction& dura)
Q_ASSERT(_e.isEndElement() && _e.name() == "note");
}

//---------------------------------------------------------
// calcTicks
//---------------------------------------------------------

Fraction MusicXMLParserPass1::calcTicks(const int& intTicks, const QXmlStreamReader* const xmlReader)
{
Fraction dura(0, 1); // invalid unless set correctly

if (_divs > 0) {
dura.set(intTicks, 4 * _divs);
dura.reduce(); // prevent overflow in later Fraction operations

// Correct for previously adjusted durations
// This is necessary when certain tuplets are
// followed by a <backup> element.
// There are two strategies:
// 1. Use a lookup table of previous adjustments
// 2. Check if within maxDiff of a seenDenominator
if (_adjustedDurations.contains(dura)) {
dura = _adjustedDurations.value(dura);
}
else if (dura.reduced().denominator() > 64) {
for (auto seenDenominator : _seenDenominators) {
int seenDenominatorTicks = Fraction(1, seenDenominator).ticks();
if (qAbs(dura.ticks() % seenDenominatorTicks) <= _maxDiff) {
Fraction roundedDura = Fraction(std::round(dura.ticks() / double(seenDenominatorTicks)), seenDenominator);
roundedDura.reduce();
_logger->logError(QString("calculated duration (%1) assumed to be a rounding error by proximity to (%2)")
.arg(dura.print(), roundedDura.print()));
insertAdjustedDuration(dura, roundedDura);
dura = roundedDura;
break;
}
}
}
}
else
_logger->logError("illegal or uninitialized divisions", xmlReader);
//qDebug("duration %s valid %d", qPrintable(dura.print()), dura.isValid());

return dura;
}

//---------------------------------------------------------
// duration
//---------------------------------------------------------
Expand All @@ -3427,24 +3470,14 @@ void MusicXMLParserPass1::notePrintSpacingNo(Fraction& dura)
Parse the /score-partwise/part/measure/note/duration node.
*/

void MusicXMLParserPass1::duration(Fraction& dura)
void MusicXMLParserPass1::duration(Fraction& dura, QXmlStreamReader& e)
{
Q_ASSERT(_e.isStartElement() && _e.name() == "duration");
//_logger->logDebugTrace("MusicXMLParserPass1::duration", &_e);
Q_ASSERT(e.isStartElement() && e.name() == "duration");
_logger->logDebugTrace("MusicXMLParserPass1::duration", &e);

dura.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
}
else
_logger->logError("illegal or uninitialized divisions", &_e);
}
else
_logger->logError("illegal duration", &_e);
//qDebug("duration %s valid %d", qPrintable(dura.print()), dura.isValid());
int intDura = e.readElementText().toInt();
dura = calcTicks(intDura);
}

//---------------------------------------------------------
Expand Down
12 changes: 11 additions & 1 deletion importexport/musicxml/importmxmlpass1.h
View file
Expand Up @@ -145,7 +145,10 @@ class MusicXMLParserPass1 {
void notations(MxmlStartStop& tupletStartStop);
void note(const QString& partId, const Fraction cTime, Fraction& missingPrev, Fraction& dura, Fraction& missingCurr, VoiceOverlapDetector& vod, MxmlTupletStates& tupletStates);
void notePrintSpacingNo(Fraction& dura);
void duration(Fraction& dura);
Fraction calcTicks(const int& intTicks, const QXmlStreamReader* const xmlReader);
Fraction calcTicks(const int& intTicks) { return calcTicks(intTicks, &_e); }
void duration(Fraction& dura, QXmlStreamReader& e);
void duration(Fraction& dura) { duration(dura, _e); }
void forward(Fraction& dura);
void backup(Fraction& dura);
void timeModification(Fraction& timeMod);
Expand Down Expand Up @@ -177,6 +180,10 @@ class MusicXMLParserPass1 {
const std::set<int>& pageStartMeasureNrs,
const CreditWordsList& crWords,
const QSize pageSize);
const int maxDiff() { return _maxDiff; }
void insertAdjustedDuration(Fraction key, Fraction value) { _adjustedDurations.insert(key, value); }
QMap<Fraction, Fraction>& adjustedDurations() { return _adjustedDurations; }
void insertSeenDenominator(int val) { _seenDenominators.emplace(val); }
QString supportsTranspose() const { return _supportsTranspose; }
void addInferredTranspose(const QString& partId);
void setHasInferredHeaderText(bool b) { _hasInferredHeaderText = b; }
Expand All @@ -202,6 +209,9 @@ class MusicXMLParserPass1 {
bool _hasBeamingInfo; ///< Whether the score supports or contains beaming info
QString _supportsTranspose; ///< Whether the score supports transposition info
bool _hasInferredHeaderText;
const int _maxDiff = 5; ///< Duration rounding tick threshold;
QMap<Fraction, Fraction> _adjustedDurations; ///< Rounded durations
std::set<int> _seenDenominators; ///< Denominators seen. Used for rounding errors.

// part specific data (TODO: move to part-specific class)
Fraction _timeSigDura; ///< Measure duration according to last timesig read
Expand Down
44 changes: 16 additions & 28 deletions importexport/musicxml/importmxmlpass2.cpp
View file
Expand Up @@ -959,14 +959,22 @@ static void handleTupletStop(Tuplet*& tuplet, const int normalNotes)
tuplet->setTicks(f);
// TODO determine usefulness of following check
int totalDuration = 0;
foreach (DurationElement* de, tuplet->elements()) {
int ticksPerNote = f.ticks() / tuplet->ratio().numerator();
bool ticksCorrect = true;
for (DurationElement* de : tuplet->elements()) {
if (de->type() == ElementType::CHORD || de->type() == ElementType::REST) {
totalDuration+=de->globalTicks().ticks();
int globalTicks = de->globalTicks().ticks();
if (globalTicks != ticksPerNote)
ticksCorrect = false;
totalDuration += globalTicks;
}
}
if (!(totalDuration && normalNotes)) {
if (totalDuration != f.ticks()) {
qDebug("MusicXML::import: tuplet stop but bad duration"); // TODO
}
if (!ticksCorrect) {
qDebug("MusicXML::import: tuplet stop but uneven note ticks"); // TODO
}
tuplet = 0;
}

Expand Down Expand Up @@ -2382,7 +2390,7 @@ void MusicXMLParserPass2::measure(const QString& partId,
attributes(partId, measure, time + mTime);
else if (_e.name() == "direction") {
MusicXMLParserDirection dir(_e, _score, _pass1, *this, _logger);
dir.direction(partId, measure, time + mTime, _divs, _spanners, delayedDirections, inferredFingerings);
dir.direction(partId, measure, time + mTime, _spanners, delayedDirections, inferredFingerings);
}
else if (_e.name() == "figured-bass") {
FiguredBass* fb = figuredBass();
Expand Down Expand Up @@ -2762,25 +2770,6 @@ void MusicXMLParserPass2::measureStyle(Measure* measure)
}
}

//---------------------------------------------------------
// calcTicks
//---------------------------------------------------------

static Fraction calcTicks(const QString& text, int divs, MxmlLogger* logger, const QXmlStreamReader* const xmlreader)
{
Fraction dura(0, 0); // invalid unless set correctly

int intDura = text.toInt();
if (divs > 0) {
dura.set(intDura, 4 * divs);
dura.reduce();
}
else
logger->logError(QString("illegal or uninitialized divisions (%1)").arg(divs), xmlreader);

return dura;
}

//---------------------------------------------------------
// preventNegativeTick
//---------------------------------------------------------
Expand Down Expand Up @@ -2866,7 +2855,6 @@ void DelayedDirectionsList::combineTempoText()
void MusicXMLParserDirection::direction(const QString& partId,
Measure* measure,
const Fraction& tick,
const int divisions,
MusicXmlSpannerMap& spanners,
DelayedDirectionsList& delayedDirections,
InferredFingeringsList& inferredFingerings)
Expand Down Expand Up @@ -2895,7 +2883,7 @@ void MusicXMLParserDirection::direction(const QString& partId,
if (_e.name() == "direction-type")
directionType(starts, stops);
else if (_e.name() == "offset") {
_offset = calcTicks(_e.readElementText(), divisions, _logger, &_e);
_offset = _pass1.calcTicks(_e.readElementText().toInt(), &_e);
preventNegativeTick(tick, _offset, _logger);
}
else if (_e.name() == "sound")
Expand Down Expand Up @@ -5354,7 +5342,7 @@ Note* MusicXMLParserPass2::note(const QString& partId,
MusicXMLParserLyric lyric { _pass1.getMusicXmlPart(partId).lyricNumberHandler(), _e, _score, _logger };
MusicXMLParserNotations notations { _e, _score, _logger };

mxmlNoteDuration mnd { _divs, _logger };
mxmlNoteDuration mnd { _divs, _logger, &_pass1 };
mxmlNotePitch mnp { _logger };

while (_e.readNextStartElement()) {
Expand Down Expand Up @@ -5779,7 +5767,7 @@ void MusicXMLParserPass2::duration(Fraction& dura)
dura.set(0, 0); // invalid unless set correctly
const auto elementText = _e.readElementText();
if (elementText.toInt() > 0)
dura = calcTicks(elementText, _divs, _logger, &_e);
dura = _pass1.calcTicks(elementText.toInt(), &_e);
else
_logger->logError(QString("illegal duration %1").arg(dura.print()), &_e);
//qDebug("duration %s valid %d", qPrintable(dura.print()), dura.isValid());
Expand Down Expand Up @@ -6208,7 +6196,7 @@ void MusicXMLParserPass2::harmony(const QString& partId, Measure* measure, const
else if (_e.name() == "level")
skipLogCurrElem();
else if (_e.name() == "offset") {
offset = calcTicks(_e.readElementText(), _divs, _logger, &_e);
offset = _pass1.calcTicks(_e.readElementText().toInt(), &_e);
preventNegativeTick(sTime, offset, _logger);
}
else if (_e.name() == "staff") {
Expand Down
4 changes: 2 additions & 2 deletions importexport/musicxml/importmxmlpass2.h
View file
Expand Up @@ -359,15 +359,15 @@ class MusicXMLParserPass2 {
class MusicXMLParserDirection {
public:
MusicXMLParserDirection(QXmlStreamReader& e, Score* score, MusicXMLParserPass1& pass1, MusicXMLParserPass2& pass2, MxmlLogger* logger);
void direction(const QString& partId, Measure* measure, const Fraction& tick, const int divisions,
void direction(const QString& partId, Measure* measure, const Fraction& tick,
MusicXmlSpannerMap& spanners, DelayedDirectionsList& delayedDirections, InferredFingeringsList& inferredFingerings);
qreal totalY() const { return _defaultY + _relativeY; }
QString placement() const;

private:
QXmlStreamReader& _e;
Score* const _score; // the score
MusicXMLParserPass1& _pass1; // the pass1 results
MusicXMLParserPass1& _pass1; // the pass1 results
MusicXMLParserPass2& _pass2; // the pass2 results
MxmlLogger* _logger; ///< Error logger

Expand Down

0 comments on commit aae940d

Please sign in to comment.