feat: model pedal number so overlapping pedal lines keep their identity

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 <optional>
 #include <string>
@@ -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 <pedal> element's id attribute (see Id.h).
     std::optional<Id> 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);

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;

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:

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);

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 <glissando> from <slide>, 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 <pedal>'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:

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
-// <pedal> 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

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("<pedal");
+    REQUIRE(pedalStart != std::string::npos);
+    const auto pedalEnd = 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;

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<DirectionData> &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