diff --git a/src/private/mx/impl/DirectionReader.cpp b/src/private/mx/impl/DirectionReader.cpp index 38a4781dd..113a60994 100644 --- a/src/private/mx/impl/DirectionReader.cpp +++ b/src/private/mx/impl/DirectionReader.cpp @@ -755,8 +755,10 @@ void DirectionReader::parseOctaveShift(const core::DirectionType &directionType) bool isUp = octaveShift.type().tag() == core::UpDownStopContinue::Tag::up; // MusicXML does not properly constrain the ottava size to valid music notation values. We do - // so here by interpolating the value into an enum that maps to valid music notation. - if (!isUp && amount == 22) + // so here by interpolating the value into an enum that maps to valid music notation. A size + // larger than 22 takes the widest line we have rather than falling back to the 15th, so the + // line we draw is the closest one to what the source asked for. + if (!isUp && amount >= 22) { ottavaType = api::OttavaType::o22ma; } @@ -768,7 +770,7 @@ void DirectionReader::parseOctaveShift(const core::DirectionType &directionType) { ottavaType = api::OttavaType::o8va; } - else if (isUp && amount == 22) + else if (isUp && amount >= 22) { ottavaType = api::OttavaType::o22mb; } diff --git a/src/private/mxtest/api/CorpusRoundtripMain.cpp b/src/private/mxtest/api/CorpusRoundtripMain.cpp index 2f6e37fa4..21b40ba46 100644 --- a/src/private/mxtest/api/CorpusRoundtripMain.cpp +++ b/src/private/mxtest/api/CorpusRoundtripMain.cpp @@ -266,6 +266,37 @@ void canonicalizeDirectionSpellings(pugi::xml_document &doc) mxtest::sortAttributes(doc); } +// MusicXML lets octave-shift/@size be any positive integer, so a corpus file can carry a number +// that does not name a line a performer could read: 27, 11, 1. mx::api narrows each of those to +// the closest ottava it can draw -- 22 and up become 22, anything else above 8 becomes 15, and the +// rest become 8 -- because the api models the six ottava lines music notation has rather than the +// whole integer range. That narrowing is what the api is for, so the expected document is brought to the +// same value instead of the api growing a field to carry the original number back out. +// +// Only the expected side is narrowed. mx writes 8, 15, or 22, so a write that produced some other +// size still fails. The size of a stop is taken here from the stop's own attribute, while mx takes +// it from the start the stop closes, so a source whose stop contradicts its start still fails too. +void narrowOctaveShiftSizes(pugi::xml_node el) +{ + if (std::string_view{el.name()} == "octave-shift") + { + pugi::xml_attribute size = el.attribute("size"); + const int stated = size ? size.as_int(0) : 0; + if (stated > 0) + { + size.set_value(stated >= 22 ? 22 : (stated > 8 ? 15 : 8)); + } + } + + for (pugi::xml_node c = el.first_child(); c; c = c.next_sibling()) + { + if (c.type() == pugi::node_element) + { + narrowOctaveShiftSizes(c); + } + } +} + bool hasSuffix(const std::string &name, std::string_view suffix) { return name.size() >= suffix.size() && name.compare(name.size() - suffix.size(), suffix.size(), suffix) == 0; @@ -445,6 +476,10 @@ RoundtripResult runRoundtrip(const std::string &absolutePath) canonicalizeDirectionSpellings(expectedDoc); canonicalizeDirectionSpellings(actualDoc); + // mx::api narrows an octave-shift size to a size music notation has, so the expected document + // states the narrowed size rather than the one the source wrote. + narrowOctaveShiftSizes(expectedDoc.document_element()); + // Compare const auto fail = mxtest::corert::compareElements(expectedDoc.document_element(), actualDoc.document_element()); if (fail.isFailure) @@ -530,6 +565,7 @@ void dumpDocuments(const std::string &absolutePath, const std::string &relPath, canonicalizeEncodingChildOrder(expectedDoc.document_element()); collapseEqualPageMargins(expectedDoc.document_element()); canonicalizeDirectionSpellings(expectedDoc); + narrowOctaveShiftSizes(expectedDoc.document_element()); if (!expectedDoc.save_file(expectedPath.c_str())) std::cerr << "dump: failed to write " << expectedPath << "\n"; diff --git a/src/private/mxtest/api/OttavaSizeApiTest.cpp b/src/private/mxtest/api/OttavaSizeApiTest.cpp index 0d57cd6f8..8bbc364bc 100644 --- a/src/private/mxtest/api/OttavaSizeApiTest.cpp +++ b/src/private/mxtest/api/OttavaSizeApiTest.cpp @@ -343,4 +343,101 @@ TEST(OttavaStopSizeContradictionIsNormalizedToTheStart, OttavaSize) T_END; +// MusicXML's schema allows any positive integer in octave-shift/@size, so a file can ask for a +// line music notation has no name for. The api narrows it to the closest line it can draw, which +// is what lets everything downstream assume an ottava is one of the six real ones. A size between +// the 8th and the 15th takes the 15th. +TEST(OttavaSizeBetweenTheLinesIsNarrowedToTheFifteenth, OttavaSize) +{ + const std::string sourceXml = + R"()" + R"()" + R"(P)" + R"(1)" + R"()" + R"(C41quarter)" + R"()" + R"()"; + + const auto score = mxtest::fromXml(sourceXml); + REQUIRE(score.parts.size() == 1); + const auto &directions = score.parts.front().measures.front().staves.front().directions; + REQUIRE(directions.size() == 2); + const auto &startTypes = directions.front().directionTypes; + REQUIRE(startTypes.size() == 1); + REQUIRE(startTypes.front().isOttavaStart()); + CHECK(startTypes.front().ottavaStart().ottavaType == OttavaType::o15ma); + + const auto xml = mxtest::toXml(score); + REQUIRE(!xml.empty()); + const auto shifts = ottavaSizeWrittenShifts(xml); + REQUIRE(shifts.size() == 2); + CHECK_EQUAL(std::string{"15"}, shifts.front().size); + CHECK_EQUAL(std::string{"15"}, shifts.back().size); +} + +T_END; + +// A size past the 22nd asks for more than the widest line we can draw, so it takes that widest +// line. Falling back to the 15th would move the reader further from what the source asked for. +TEST(OttavaSizePastTheTwentySecondIsNarrowedToTheTwentySecond, OttavaSize) +{ + const std::string sourceXml = + R"()" + R"()" + R"(P)" + R"(1)" + R"()" + R"(C41quarter)" + R"()" + R"()"; + + const auto score = mxtest::fromXml(sourceXml); + REQUIRE(score.parts.size() == 1); + const auto &directions = score.parts.front().measures.front().staves.front().directions; + REQUIRE(directions.size() == 2); + const auto &startTypes = directions.front().directionTypes; + REQUIRE(startTypes.size() == 1); + REQUIRE(startTypes.front().isOttavaStart()); + CHECK(startTypes.front().ottavaStart().ottavaType == OttavaType::o22ma); + + const auto xml = mxtest::toXml(score); + REQUIRE(!xml.empty()); + const auto shifts = ottavaSizeWrittenShifts(xml); + REQUIRE(shifts.size() == 2); + CHECK_EQUAL(std::string{"22"}, shifts.front().size); + CHECK_EQUAL(std::string{"22"}, shifts.back().size); +} + +T_END; + +// A size below 8 asks for less than an octave, which an ottava cannot draw either. It becomes a +// plain octave line, and the size the source spelled out stays spelled out. +TEST(OttavaSizeBelowEightIsNarrowedToAnOctave, OttavaSize) +{ + const std::string sourceXml = + R"()" + R"()" + R"(P)" + R"(1)" + R"()" + R"(C41quarter)" + R"()"; + + const auto score = mxtest::fromXml(sourceXml); + REQUIRE(score.parts.size() == 1); + const auto &startTypes = score.parts.front().measures.front().staves.front().directions.front().directionTypes; + REQUIRE(startTypes.size() == 1); + REQUIRE(startTypes.front().isOttavaStart()); + CHECK(startTypes.front().ottavaStart().ottavaType == OttavaType::o8vb); + + const auto xml = mxtest::toXml(score); + REQUIRE(!xml.empty()); + const auto shifts = ottavaSizeWrittenShifts(xml); + REQUIRE(shifts.size() == 1); + CHECK_EQUAL(std::string{"8"}, shifts.front().size); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index eb3347da1..dd8cc51eb 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -716,3 +716,12 @@ lysuite/ly33b_Spanners_Tie.xml # was written under the wrong name. musuite/testChordNoVoice.xml musuite/testStringVoiceName.xml + +# Unblocked in the harness rather than in mx::api. Each of these sources states an +# octave-shift size that no ottava line can mean -- 27, 11, 1 -- which MusicXML's +# schema allows and music notation does not. mx::api narrows the size to a line it +# can draw, which is the behavior we want, so the harness narrows the expected +# document the same way instead of the api learning to echo the original number. +lysuite/ly33e_Spanners_OctaveShifts_InvalidSize.xml +synthetic/octave-shift.3.0.xml +synthetic/octave-shift.3.1.xml