From be615d189d98bf273af3f806c79b6f12e2324691 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sat, 19 Sep 2026 13:36:00 +0200 Subject: [PATCH 1/2] feat: model pedal number so overlapping pedal lines keep their identity Add api::PedalLineData::number and read 's number attribute, so two pedal lines held down at once stay distinguishable on read. The writer emits the attribute through SpannerResolver, which now gives pedals their own number pool like every other spanner family; an authored identity is assigned a level from serialization order and an explicit level is written verbatim. A single pedal line is unchanged and writes no number. --- src/include/mx/api/PedalLineData.h | 16 +- src/private/mx/impl/DirectionReader.cpp | 4 + src/private/mx/impl/DirectionWriter.cpp | 11 +- src/private/mx/impl/DirectionWriter.h | 4 +- src/private/mx/impl/SpannerResolver.cpp | 23 ++- src/private/mx/impl/SpannerResolver.h | 13 +- .../api/DirectionMarksRoundTripTest.cpp | 34 ++++ .../mxtest/api/SpannerIdentityTest.cpp | 153 ++++++++++++++++++ .../mxtest/impl/DirectionReaderTest.cpp | 49 ++++++ 9 files changed, 292 insertions(+), 15 deletions(-) diff --git a/src/include/mx/api/PedalLineData.h b/src/include/mx/api/PedalLineData.h index 4c1b59ef6..aa96d70c5 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,24 @@ 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. Give every event of one line the same SpannerNumber (see + // SpannerNumber.h): an explicit level is written verbatim, or an identity label lets the + // writer assign the level. + 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 +81,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 From 72bf138dc0b9720fc7d7a8d01dbcc73557f86b82 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sat, 19 Sep 2026 16:31:33 +0200 Subject: [PATCH 2/2] Update src/include/mx/api/PedalLineData.h Signed-off-by: Matthew James Briggs --- src/include/mx/api/PedalLineData.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/include/mx/api/PedalLineData.h b/src/include/mx/api/PedalLineData.h index aa96d70c5..40cce86df 100644 --- a/src/include/mx/api/PedalLineData.h +++ b/src/include/mx/api/PedalLineData.h @@ -60,9 +60,7 @@ struct PedalLineData // 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. Give every event of one line the same SpannerNumber (see - // SpannerNumber.h): an explicit level is written verbatim, or an identity label lets the - // writer assign the level. + // the right downstroke. In non-ambiguous cases, leaving this unset is fine. SpannerNumber number; // The element's id attribute (see Id.h).