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().

Duplicate of musescore#8766, part 2, plus resolving a merge conflict with musescore#8282
(resp. 8429, for master) and fixing 2 compiler warnings.
  • Loading branch information
@iveshenry18 @Jojo-Schmitz
iveshenry18 authored and Jojo-Schmitz committed Aug 5, 2021
1 parent e0cec3c commit 10784e3
Show file tree
Hide file tree
Showing 11 changed files with 4,675 additions and 54 deletions.
9 changes: 7 additions & 2 deletions importexport/musicxml/importmxmlnoteduration.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
// 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) {
if (qAbs(_calcDura.ticks() - _dura.ticks()) <= _pass1->maxDiff()) {
errorStr += " -> assuming rounding error";
_pass1->insertAdjustedDuration(_dura, _calcDura);
_dura = _calcDura;
_specDura = _calcDura; // prevent changing off time
}
Expand Down Expand Up @@ -174,6 +174,7 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
_dura = Fraction(4, 4);
}

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

Expand All @@ -192,6 +193,9 @@ void mxmlNoteDuration::duration(QXmlStreamReader& e)

_specDura.set(0, 0); // invalid unless set correctly
int intDura = e.readElementText().toInt();
#if 1 // PR #8282 vs. #8766
_dura = _pass1->calcTicks(intDura, &e); // Duration reading (and rounding) code consolidated to pass1
#else
if (intDura > 0) {
if (_divs > 0) {
_specDura.set(intDura, 4 * _divs);
Expand All @@ -203,6 +207,7 @@ void mxmlNoteDuration::duration(QXmlStreamReader& e)
else
_logger->logError("illegal duration", &e);
//qDebug("specified duration %s valid %d", qPrintable(_specDura.print()), _specDura.isValid());
#endif
}

//---------------------------------------------------------
Expand Down
5 changes: 4 additions & 1 deletion importexport/musicxml/importmxmlnoteduration.h
View file
Original file line number Diff line number Diff line change
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 duration() const { return _dura; } // duration to use
Fraction calculatedDuration() const { return _calcDura; } // value calculated from note type etcetera
Expand All @@ -52,6 +54,7 @@ class mxmlNoteDuration
TDuration _normalType;
Fraction _timeMod { 1, 1 }; // default to no time modification
MxmlLogger* _logger; ///< Error logger
MusicXMLParserPass1* _pass1;
};

} // namespace Ms
Expand Down
65 changes: 49 additions & 16 deletions importexport/musicxml/importmxmlpass1.cpp
View file
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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);
constexpr 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;
static constexpr 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
Original file line number Diff line number Diff line change
Expand Up @@ -966,14 +966,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 @@ -2391,7 +2399,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 @@ -2771,25 +2779,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 @@ -2875,7 +2864,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 @@ -2904,7 +2892,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 @@ -5389,7 +5377,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 @@ -5833,7 +5821,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 @@ -6262,7 +6250,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
Original file line number Diff line number Diff line change
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
Loading

0 comments on commit 10784e3

Please sign in to comment.