diff --git a/src/include/mx/api/PedalLineData.h b/src/include/mx/api/PedalLineData.h index 4c1b59ef6..40cce86df 100644 --- a/src/include/mx/api/PedalLineData.h +++ b/src/include/mx/api/PedalLineData.h @@ -7,6 +7,7 @@ #include "mx/api/ApiCommon.h" #include "mx/api/Id.h" #include "mx/api/PositionData.h" +#include "mx/api/SpannerNumber.h" #include #include @@ -54,14 +55,22 @@ struct PedalLineData int tickTimePosition; PositionData positionData; + // Which line this event belongs to when two pedal lines are held down at once; MusicXML calls + // it the number. It can be left unspecified, and usually should be: a lone line, and any run + // of lines that do not overlap, needs no number. A number is only needed to tell two lines + // apart when they overlap -- when a line opens before the one before it has closed, as a + // sostenuto line under a damper line does -- because otherwise a lift cannot be matched to + // the right downstroke. In non-ambiguous cases, leaving this unset is fine. + SpannerNumber number; + // The element's id attribute (see Id.h). std::optional id; - PedalLineData() : kind{PedalLineKind::unspecified}, tickTimePosition{0}, positionData{}, id{} + PedalLineData() : kind{PedalLineKind::unspecified}, tickTimePosition{0}, positionData{}, number{}, id{} { } - PedalLineData(PedalLineKind inKind) : kind{inKind}, tickTimePosition{0}, positionData{}, id{} + PedalLineData(PedalLineKind inKind) : kind{inKind}, tickTimePosition{0}, positionData{}, number{}, id{} { } }; @@ -70,6 +79,7 @@ MXAPI_EQUALS_BEGIN(PedalLineData) MXAPI_EQUALS_MEMBER(kind) MXAPI_EQUALS_MEMBER(tickTimePosition) MXAPI_EQUALS_MEMBER(positionData) +MXAPI_EQUALS_MEMBER(number) MXAPI_EQUALS_MEMBER(id) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(PedalLineData); diff --git a/src/private/mx/impl/DirectionReader.cpp b/src/private/mx/impl/DirectionReader.cpp index c79a07900..b24523837 100644 --- a/src/private/mx/impl/DirectionReader.cpp +++ b/src/private/mx/impl/DirectionReader.cpp @@ -714,6 +714,10 @@ void DirectionReader::parsePedal(const core::DirectionType &directionType) pedalData.tickTimePosition = myOutDirectionData.tickTimePosition; pedalData.positionData = getPositionData(pedal); pedalData.positionData.placement = placement; + if (pedal.number().has_value()) + { + pedalData.number = api::SpannerNumber(pedal.number()->value()); + } pedalData.id = getId(pedal); myOutDirectionData.directionTypes.emplace_back(api::DirectionChoice{std::move(pedalData)}); return; diff --git a/src/private/mx/impl/DirectionWriter.cpp b/src/private/mx/impl/DirectionWriter.cpp index ee84ddc42..3fe66fa72 100644 --- a/src/private/mx/impl/DirectionWriter.cpp +++ b/src/private/mx/impl/DirectionWriter.cpp @@ -333,7 +333,7 @@ core::PedalType corePedalType(api::PedalLineKind kind) return core::PedalType::start(); } -void DirectionWriter::emitPedal(const api::PedalLineData &item, core::Direction &direction) +void DirectionWriter::emitPedal(const api::PedalLineData &item, const void *inIdentity, core::Direction &direction) { // An unspecified kind describes no pedal event; emit nothing rather than a guessed default. if (item.kind == api::PedalLineKind::unspecified) @@ -344,6 +344,13 @@ void DirectionWriter::emitPedal(const api::PedalLineData &item, core::Direction core::Pedal pedal{}; pedal.setType(corePedalType(item.kind)); pedal.setLine(core::YesNo::yes()); + + const auto number = mySpannerResolver.emittedNumber(item.number, inIdentity); + if (number.has_value()) + { + pedal.setNumber(core::NumberLevel{*number}); + } + setAttributesFromPositionData(item.positionData, pedal); setId(item.id, pedal, myDiagnostics, cursorLocation(myCursor)); core::DirectionType dt{}; @@ -1330,7 +1337,7 @@ void DirectionWriter::emitDirectionTypes(core::Direction &direction) break; case api::DirectionChoice::Kind::pedal: - emitPedal(choice.pedal(), direction); + emitPedal(choice.pedal(), &choice, direction); break; case api::DirectionChoice::Kind::wordsRun: diff --git a/src/private/mx/impl/DirectionWriter.h b/src/private/mx/impl/DirectionWriter.h index 15b9f4422..acebde72b 100644 --- a/src/private/mx/impl/DirectionWriter.h +++ b/src/private/mx/impl/DirectionWriter.h @@ -24,7 +24,7 @@ namespace impl class DirectionWriter { public: - // inSpannerResolver supplies the resolved 'number' for start/stop pairs such as wedge, + // inSpannerResolver supplies the resolved 'number' for start/stop pairs such as wedge, pedal, // octave-shift, bracket, and dashes and stop semantics such as the size attribute of an // octave-shift (see SpannerResolver); it outlives this writer. DirectionWriter(const api::DirectionData &inDirectionData, const MeasureCursor &inCursor, @@ -42,7 +42,7 @@ class DirectionWriter // spanner: the same address the SpannerResolver saw when it walked the score, so the resolved // 'number' -- and, for an ottava stop, the size -- can be looked up for the emitted element. void emitMark(api::MarkData mark, core::Direction &direction); - void emitPedal(const api::PedalLineData &pedal, core::Direction &direction); + void emitPedal(const api::PedalLineData &pedal, const void *inIdentity, core::Direction &direction); void emitWedgeStop(const api::WedgeStop &wedgeStop, const void *inIdentity, core::Direction &direction); void emitWedgeStart(const api::WedgeStart &wedgeStart, const void *inIdentity, core::Direction &direction); void emitOttavaStop(const api::OttavaStop &ottavaStop, const void *inIdentity, core::Direction &direction); diff --git a/src/private/mx/impl/SpannerResolver.cpp b/src/private/mx/impl/SpannerResolver.cpp index 7cbfe2a4c..621f02057 100644 --- a/src/private/mx/impl/SpannerResolver.cpp +++ b/src/private/mx/impl/SpannerResolver.cpp @@ -25,7 +25,8 @@ namespace impl // elements (octave-shift, bracket, dashes) whose number attributes are // independent of each other in MusicXML, so each gets its own pool. // GlissandoType similarly distinguishes from , two distinct -// elements with independent number attributes. +// elements with independent number attributes. Pedal lines number themselves +// independently of every other family too. enum class SpannerNumberClass { slur, @@ -34,6 +35,7 @@ enum class SpannerNumberClass octaveShift, bracket, dashes, + pedal, glissando, slide, wavyLine @@ -175,8 +177,7 @@ class SpannerEventCollector // Mirrors DirectionWriter::emitDirectionTypes: one pass over the ordered direction-type // content, registering each spanner event with the address of its DirectionChoice -- the - // same identity the writer presents when it asks for the emitted number. Pedals are - // skipped: mx::api does not model 's number attribute. + // same identity the writer presents when it asks for the emitted number. void addDirection(const api::DirectionData &inDirection) { myCurrentNoteTag = nullptr; @@ -227,6 +228,20 @@ class SpannerEventCollector add(SpannerNumberClass::dashes, &choice, choice.dashesStop().number, false, true); break; + case api::DirectionChoice::Kind::pedal: { + // A pedal line opens with a downstroke, a sostenuto mark, or a resume, and ends + // with an explicit or implicit lift; change and continueLine happen while the + // line stays open. + const auto pedal = choice.pedal(); + const bool opens = pedal.kind == api::PedalLineKind::start || + pedal.kind == api::PedalLineKind::sostenuto || + pedal.kind == api::PedalLineKind::resume; + const bool closes = + pedal.kind == api::PedalLineKind::stop || pedal.kind == api::PedalLineKind::discontinue; + add(SpannerNumberClass::pedal, &choice, pedal.number, opens, closes); + break; + } + default: break; } @@ -503,6 +518,8 @@ static const char *spannerClassName(SpannerNumberClass inClass) return "bracket"; case SpannerNumberClass::dashes: return "dashes"; + case SpannerNumberClass::pedal: + return "pedal"; case SpannerNumberClass::glissando: return "glissando"; case SpannerNumberClass::slide: diff --git a/src/private/mx/impl/SpannerResolver.h b/src/private/mx/impl/SpannerResolver.h index 2255de5e9..3bd5e6ccf 100644 --- a/src/private/mx/impl/SpannerResolver.h +++ b/src/private/mx/impl/SpannerResolver.h @@ -29,9 +29,9 @@ namespace impl // concurrently-open identity spanner is never handed the same number. // // Numbers come from a pool of 1..16 per part and per spanner class (slur, -// tied, wedge, octave-shift, bracket, dashes, glissando, slide, and wavy-line -// each have their own pool; a slur numbered 1 and a wedge numbered 1 do not -// conflict). The number-level documentation scopes concurrency to the part, +// tied, wedge, octave-shift, bracket, dashes, pedal, glissando, slide, and +// wavy-line each have their own pool; a slur numbered 1 and a wedge numbered 1 +// do not conflict). The number-level documentation scopes concurrency to the part, // never the staff: two spanners conflict exactly when they overlap in the // order a streaming reader encounters them, even when they sit on different // staves of the part. So resolvePart walks the part in the exact order @@ -49,9 +49,10 @@ namespace impl // // Identity ids are scoped per part and per spanner class: events in the same // part sharing a class and id are one logical spanner, even across staves. -// Pedal starts/stops carry SpannerNumber, but mx::api does not model the -// number attribute (added in MusicXML 3.1), so pedals are ignored -// here and no number is ever emitted for one. +// The events of one pedal line -- its downstroke, any change or formatting +// continuation, and the lift that ends it -- likewise share a SpannerNumber +// identity, and a pedal line numbered with an explicit level keeps that level +// while it is open. // // If more than 16 spanners of one class are open at once in a part (which no // real score approaches), resolution refuses the write rather than emitting diff --git a/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp b/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp index 99b1d6fc6..fe9a59d4c 100644 --- a/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp +++ b/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp @@ -320,6 +320,40 @@ TEST(PedalPlacement, DirectionMarksRoundTrip) T_END; +// A pedal line's number survives a round trip, which is what lets two lines held down at once +// be told apart (#411). +TEST(PedalExplicitNumber, DirectionMarksRoundTrip) +{ + DirectionData direction; + PedalLineData pedal{PedalLineKind::start}; + pedal.number = SpannerNumber(2); + direction.directionTypes.emplace_back(DirectionChoice{pedal}); + const auto directions = roundTripDirectionData(direction); + REQUIRE(directions.size() == 1); + REQUIRE(directions.front().directionTypes.size() == 1); + REQUIRE(directions.front().directionTypes.front().isPedal()); + CHECK(directions.front().directionTypes.front().pedal().kind == PedalLineKind::start); + CHECK(SpannerNumber(2) == directions.front().directionTypes.front().pedal().number); +} + +T_END; + +// A lone pedal line needs no number, so none is written; every ordinary score stays unchanged. +TEST(PedalWithoutNumberWritesNoAttribute, DirectionMarksRoundTrip) +{ + DirectionData direction; + direction.directionTypes.emplace_back(DirectionChoice{PedalLineData{PedalLineKind::start}}); + const auto xml = xmlForDirectionData(direction); + const auto pedalStart = xml.find("", pedalStart); + REQUIRE(pedalEnd != std::string::npos); + const auto pedalElement = xml.substr(pedalStart, pedalEnd - pedalStart); + CHECK(pedalElement.find("number=") == std::string::npos); +} + +T_END; + TEST(Scordatura, DirectionMarksRoundTrip) { DirectionData direction; diff --git a/src/private/mxtest/api/SpannerIdentityTest.cpp b/src/private/mxtest/api/SpannerIdentityTest.cpp index efece7cd0..3a549d238 100644 --- a/src/private/mxtest/api/SpannerIdentityTest.cpp +++ b/src/private/mxtest/api/SpannerIdentityTest.cpp @@ -433,4 +433,157 @@ TEST(wedgeNumbersRoundTrip, SpannerIdentity) T_END +namespace spannerIdentityTest +{ +// One measure of four quarter notes on one staff, with a directions list the caller fills in. +inline ScoreData makeSingleMeasureScore() +{ + ScoreData score; + score.ticksPerQuarter = ticksPerQuarter; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + auto &voice = measure.staves.back().voices[0]; + for (int i = 0; i < 4; ++i) + { + voice.notes.push_back(makeQuarter(i * ticksPerQuarter, Step::c, 4)); + } + return score; +} + +inline void addPedalDirection(std::vector &ioDirections, int inTick, PedalLineKind inKind, + const SpannerNumber &inNumber) +{ + DirectionData direction; + direction.tickTimePosition = inTick; + PedalLineData pedal{inKind}; + pedal.number = inNumber; + direction.directionTypes.emplace_back(DirectionChoice{pedal}); + ioDirections.push_back(direction); +} +} // namespace spannerIdentityTest + +// Two pedal lines held down at once -- a damper line and a sostenuto line -- are told apart by +// their numbers. Authored with identity numbers, they overlap in document order and must receive +// different ones, and both numbers must survive the round trip (#411). +TEST(overlappingIdentityPedalLinesGetDistinctNumbers, SpannerIdentity) +{ + using namespace spannerIdentityTest; + auto score = makeSingleMeasureScore(); + auto &directions = score.parts.at(0).measures.at(0).staves.at(0).directions; + + addPedalDirection(directions, 0, PedalLineKind::start, SpannerNumber("damper")); + addPedalDirection(directions, 0, PedalLineKind::sostenuto, SpannerNumber("sostenuto")); + addPedalDirection(directions, 4 * ticksPerQuarter, PedalLineKind::stop, SpannerNumber("damper")); + addPedalDirection(directions, 4 * ticksPerQuarter, PedalLineKind::discontinue, SpannerNumber("sostenuto")); + + const auto xml = toXml(score); + REQUIRE(!xml.empty()); + const auto roundTripped = fromXml(xml); + + const auto &readDirections = roundTripped.parts.at(0).measures.at(0).staves.at(0).directions; + REQUIRE(readDirections.size() == 4); + CHECK(SpannerNumber(1) == readDirections.at(0).directionTypes.at(0).pedal().number); + CHECK(SpannerNumber(2) == readDirections.at(1).directionTypes.at(0).pedal().number); + CHECK(SpannerNumber(1) == readDirections.at(2).directionTypes.at(0).pedal().number); + CHECK(SpannerNumber(2) == readDirections.at(3).directionTypes.at(0).pedal().number); +} + +T_END + +// An explicit pedal number is emitted verbatim for both ends of the line. +TEST(explicitPedalNumberRoundTrips, SpannerIdentity) +{ + using namespace spannerIdentityTest; + auto score = makeSingleMeasureScore(); + auto &directions = score.parts.at(0).measures.at(0).staves.at(0).directions; + + addPedalDirection(directions, 0, PedalLineKind::start, SpannerNumber(4)); + addPedalDirection(directions, 2 * ticksPerQuarter, PedalLineKind::stop, SpannerNumber(4)); + + const auto xml = toXml(score); + REQUIRE(!xml.empty()); + const auto roundTripped = fromXml(xml); + + const auto &readDirections = roundTripped.parts.at(0).measures.at(0).staves.at(0).directions; + REQUIRE(readDirections.size() == 2); + CHECK(SpannerNumber(4) == readDirections.at(0).directionTypes.at(0).pedal().number); + CHECK(SpannerNumber(4) == readDirections.at(1).directionTypes.at(0).pedal().number); +} + +T_END + +// A pedal line and a wedge draw their numbers from separate pools, so two concurrent identity +// spanners of the two families both take number 1. +TEST(pedalAndWedgePoolsAreIndependent, SpannerIdentity) +{ + using namespace spannerIdentityTest; + auto score = makeSingleMeasureScore(); + auto &directions = score.parts.at(0).measures.at(0).staves.at(0).directions; + + addPedalDirection(directions, 0, PedalLineKind::start, SpannerNumber("p")); + addPedalDirection(directions, 2 * ticksPerQuarter, PedalLineKind::stop, SpannerNumber("p")); + + DirectionData wedgeStartDirection; + wedgeStartDirection.tickTimePosition = 0; + WedgeStart wedgeStart; + wedgeStart.wedgeType = WedgeType::crescendo; + wedgeStart.number = SpannerNumber("w"); + wedgeStartDirection.directionTypes.emplace_back(DirectionChoice{wedgeStart}); + directions.push_back(wedgeStartDirection); + + DirectionData wedgeStopDirection; + wedgeStopDirection.tickTimePosition = 2 * ticksPerQuarter; + WedgeStop wedgeStop; + wedgeStop.number = SpannerNumber("w"); + wedgeStopDirection.directionTypes.emplace_back(DirectionChoice{wedgeStop}); + directions.push_back(wedgeStopDirection); + + const auto xml = toXml(score); + REQUIRE(!xml.empty()); + const auto roundTripped = fromXml(xml); + + const auto &readDirections = roundTripped.parts.at(0).measures.at(0).staves.at(0).directions; + + // The reader orders directions by tick, so collect the numbers by kind rather than by index. + SpannerNumber pedalStartNumber; + SpannerNumber pedalStopNumber; + SpannerNumber wedgeStartNumber; + SpannerNumber wedgeStopNumber; + for (const auto &direction : readDirections) + { + for (const auto &choice : direction.directionTypes) + { + if (choice.isPedal()) + { + if (choice.pedal().kind == PedalLineKind::start) + { + pedalStartNumber = choice.pedal().number; + } + else + { + pedalStopNumber = choice.pedal().number; + } + } + else if (choice.isWedgeStart()) + { + wedgeStartNumber = choice.wedgeStart().number; + } + else if (choice.isWedgeStop()) + { + wedgeStopNumber = choice.wedgeStop().number; + } + } + } + + CHECK(SpannerNumber(1) == pedalStartNumber); + CHECK(SpannerNumber(1) == pedalStopNumber); + CHECK(SpannerNumber(1) == wedgeStartNumber); + CHECK(SpannerNumber(1) == wedgeStopNumber); +} + +T_END + #endif diff --git a/src/private/mxtest/impl/DirectionReaderTest.cpp b/src/private/mxtest/impl/DirectionReaderTest.cpp index a91d546f0..b129d6943 100644 --- a/src/private/mxtest/impl/DirectionReaderTest.cpp +++ b/src/private/mxtest/impl/DirectionReaderTest.cpp @@ -7,10 +7,14 @@ #include "cpul/cpulTestHarness.h" #include "mx/api/OttavaData.h" +#include "mx/api/SpannerNumber.h" #include "mx/core/generated/Direction.h" #include "mx/core/generated/DirectionType.h" #include "mx/core/generated/DirectionTypeChoice.h" #include "mx/core/generated/OctaveShift.h" +#include "mx/core/generated/Pedal.h" +#include "mx/core/generated/PedalType.h" +#include "mx/core/generated/YesNo.h" #include "mx/impl/DirectionReader.h" #include @@ -167,4 +171,49 @@ TEST(ottavaStopWithoutSize, DirectionReader) T_END +// A pedal line's number is what tells two lines held down at once apart, so the reader has to +// keep it (#411). +TEST(pedalLineNumber, DirectionReader) +{ + core::Pedal pedal{}; + pedal.setType(core::PedalType::start()); + pedal.setLine(core::YesNo::yes()); + pedal.setNumber(core::NumberLevel{2}); + core::DirectionType dirType{}; + dirType.setChoice(core::DirectionTypeChoice::pedal(pedal)); + core::Direction dir{}; + dir.setDirectionType(core::OneOrMore{dirType}); + MeasureCursor cursor{1, 100}; + cursor.tickTimePosition = 150; + DirectionReader reader{dir, cursor}; + const auto directionData = reader.getDirectionData(); + REQUIRE(directionData.directionTypes.size() == 1); + REQUIRE(directionData.directionTypes.front().isPedal()); + const auto pedalData = directionData.directionTypes.front().pedal(); + CHECK(pedalData.kind == api::PedalLineKind::start); + CHECK(api::SpannerNumber(2) == pedalData.number); +} + +T_END + +// A lone pedal line needs no number, so its absence reads as unspecified rather than as a level. +TEST(pedalLineWithoutNumber, DirectionReader) +{ + core::Pedal pedal{}; + pedal.setType(core::PedalType::stop()); + pedal.setLine(core::YesNo::yes()); + core::DirectionType dirType{}; + dirType.setChoice(core::DirectionTypeChoice::pedal(pedal)); + core::Direction dir{}; + dir.setDirectionType(core::OneOrMore{dirType}); + MeasureCursor cursor{1, 100}; + DirectionReader reader{dir, cursor}; + const auto directionData = reader.getDirectionData(); + REQUIRE(directionData.directionTypes.size() == 1); + REQUIRE(directionData.directionTypes.front().isPedal()); + CHECK(directionData.directionTypes.front().pedal().number.isUnspecified()); +} + +T_END + #endif