diff --git a/src/private/mx/impl/SpannerNumberResolver.cpp b/src/private/mx/impl/SpannerNumberResolver.cpp index 344849e47..0592c7e10 100644 --- a/src/private/mx/impl/SpannerNumberResolver.cpp +++ b/src/private/mx/impl/SpannerNumberResolver.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include @@ -36,7 +35,6 @@ enum class SpannerNumberClass struct SpannerNumberEvent { int position; - int staffIndex; const void *object; const api::SpannerNumber *number; bool opens; // a start @@ -57,40 +55,40 @@ inline bool spannerNumberIntervalsOverlap(const SpannerNumberInterval &inLeft, c } // Collects spanner events in the exact order MeasureWriter serializes them. -// The position counter is global; only events within the same class (and -// staff pool) are ever compared, so cross-class interleaving is irrelevant, -// but within a class the order matches what a streaming reader sees. +// The position counter is global; only events within the same class are ever +// compared, so cross-class interleaving is irrelevant, but within a class the +// order matches what a streaming reader sees. class SpannerNumberEventCollector { public: - void addNote(int inStaffIndex, const api::NoteData &inNote) + void addNote(const api::NoteData &inNote) { // NotationsWriter emits curve stops, then continues, then starts, and // skips curves whose type is neither tie nor slur. const auto &attachments = inNote.noteAttachmentData; for (const auto &stop : attachments.curveStops) { - addCurve(inStaffIndex, stop.curveType, &stop, stop.number, false, true); + addCurve(stop.curveType, &stop, stop.number, false, true); } for (const auto &curveContinue : attachments.curveContinuations) { - addCurve(inStaffIndex, curveContinue.curveType, &curveContinue, curveContinue.number, false, false); + addCurve(curveContinue.curveType, &curveContinue, curveContinue.number, false, false); } for (const auto &start : attachments.curveStarts) { - addCurve(inStaffIndex, start.curveType, &start, start.number, true, false); + addCurve(start.curveType, &start, start.number, true, false); } } - void addDirection(int inStaffIndex, const api::DirectionData &inDirection) + void addDirection(const api::DirectionData &inDirection) { if (inDirection.orderedComponents.empty()) { - addDirectionFixedOrder(inStaffIndex, inDirection); + addDirectionFixedOrder(inDirection); } else { - addDirectionOrderedComponents(inStaffIndex, inDirection); + addDirectionOrderedComponents(inDirection); } } @@ -100,68 +98,66 @@ class SpannerNumberEventCollector } private: - void add(SpannerNumberClass inClass, int inStaffIndex, const void *inObject, const api::SpannerNumber &inNumber, - bool inOpens, bool inCloses) + void add(SpannerNumberClass inClass, const void *inObject, const api::SpannerNumber &inNumber, bool inOpens, + bool inCloses) { - myEvents[inClass].push_back( - SpannerNumberEvent{myPosition, inStaffIndex, inObject, &inNumber, inOpens, inCloses}); + myEvents[inClass].push_back(SpannerNumberEvent{myPosition, inObject, &inNumber, inOpens, inCloses}); ++myPosition; } - void addCurve(int inStaffIndex, api::CurveType inCurveType, const void *inObject, - const api::SpannerNumber &inNumber, bool inOpens, bool inCloses) + void addCurve(api::CurveType inCurveType, const void *inObject, const api::SpannerNumber &inNumber, bool inOpens, + bool inCloses) { if (inCurveType == api::CurveType::slur) { - add(SpannerNumberClass::slur, inStaffIndex, inObject, inNumber, inOpens, inCloses); + add(SpannerNumberClass::slur, inObject, inNumber, inOpens, inCloses); } else if (inCurveType == api::CurveType::tie) { - add(SpannerNumberClass::tie, inStaffIndex, inObject, inNumber, inOpens, inCloses); + add(SpannerNumberClass::tie, inObject, inNumber, inOpens, inCloses); } } // Mirrors DirectionWriter::emitFixedOrder. Pedals are skipped: has // no number attribute. - void addDirectionFixedOrder(int inStaffIndex, const api::DirectionData &inDirection) + void addDirectionFixedOrder(const api::DirectionData &inDirection) { for (const auto &item : inDirection.wedgeStops) { - add(SpannerNumberClass::wedge, inStaffIndex, &item, item.number, false, true); + add(SpannerNumberClass::wedge, &item, item.number, false, true); } for (const auto &item : inDirection.wedgeStarts) { - add(SpannerNumberClass::wedge, inStaffIndex, &item, item.number, true, false); + add(SpannerNumberClass::wedge, &item, item.number, true, false); } for (const auto &item : inDirection.ottavaStops) { - add(SpannerNumberClass::octaveShift, inStaffIndex, &item.spannerStop, item.spannerStop.number, false, true); + add(SpannerNumberClass::octaveShift, &item.spannerStop, item.spannerStop.number, false, true); } for (const auto &item : inDirection.ottavaStarts) { - add(SpannerNumberClass::octaveShift, inStaffIndex, &item.spannerStart, item.spannerStart.number, true, - false); + add(SpannerNumberClass::octaveShift, &item.spannerStart, item.spannerStart.number, true, false); } for (const auto &item : inDirection.bracketStarts) { - add(SpannerNumberClass::bracket, inStaffIndex, &item, item.number, true, false); + add(SpannerNumberClass::bracket, &item, item.number, true, false); } for (const auto &item : inDirection.bracketStops) { - add(SpannerNumberClass::bracket, inStaffIndex, &item, item.number, false, true); + add(SpannerNumberClass::bracket, &item, item.number, false, true); } for (const auto &item : inDirection.dashesStarts) { - add(SpannerNumberClass::dashes, inStaffIndex, &item, item.number, true, false); + add(SpannerNumberClass::dashes, &item, item.number, true, false); } for (const auto &item : inDirection.dashesStops) { - add(SpannerNumberClass::dashes, inStaffIndex, &item, item.number, false, true); + add(SpannerNumberClass::dashes, &item, item.number, false, true); } } // Mirrors DirectionWriter::emitOrderedComponents (same bounds checks). - void addDirectionOrderedComponents(int inStaffIndex, const api::DirectionData &inDirection) + void addDirectionOrderedComponents(const api::DirectionData &inDirection) { for (const auto &component : inDirection.orderedComponents) { @@ -178,7 +174,7 @@ class SpannerNumberEventCollector if (index < inDirection.wedgeStops.size()) { const auto &item = inDirection.wedgeStops.at(index); - add(SpannerNumberClass::wedge, inStaffIndex, &item, item.number, false, true); + add(SpannerNumberClass::wedge, &item, item.number, false, true); } break; @@ -186,7 +182,7 @@ class SpannerNumberEventCollector if (index < inDirection.wedgeStarts.size()) { const auto &item = inDirection.wedgeStarts.at(index); - add(SpannerNumberClass::wedge, inStaffIndex, &item, item.number, true, false); + add(SpannerNumberClass::wedge, &item, item.number, true, false); } break; @@ -194,8 +190,7 @@ class SpannerNumberEventCollector if (index < inDirection.ottavaStops.size()) { const auto &item = inDirection.ottavaStops.at(index); - add(SpannerNumberClass::octaveShift, inStaffIndex, &item.spannerStop, item.spannerStop.number, - false, true); + add(SpannerNumberClass::octaveShift, &item.spannerStop, item.spannerStop.number, false, true); } break; @@ -203,8 +198,7 @@ class SpannerNumberEventCollector if (index < inDirection.ottavaStarts.size()) { const auto &item = inDirection.ottavaStarts.at(index); - add(SpannerNumberClass::octaveShift, inStaffIndex, &item.spannerStart, item.spannerStart.number, - true, false); + add(SpannerNumberClass::octaveShift, &item.spannerStart, item.spannerStart.number, true, false); } break; @@ -212,7 +206,7 @@ class SpannerNumberEventCollector if (index < inDirection.bracketStarts.size()) { const auto &item = inDirection.bracketStarts.at(index); - add(SpannerNumberClass::bracket, inStaffIndex, &item, item.number, true, false); + add(SpannerNumberClass::bracket, &item, item.number, true, false); } break; @@ -220,7 +214,7 @@ class SpannerNumberEventCollector if (index < inDirection.bracketStops.size()) { const auto &item = inDirection.bracketStops.at(index); - add(SpannerNumberClass::bracket, inStaffIndex, &item, item.number, false, true); + add(SpannerNumberClass::bracket, &item, item.number, false, true); } break; @@ -228,7 +222,7 @@ class SpannerNumberEventCollector if (index < inDirection.dashesStarts.size()) { const auto &item = inDirection.dashesStarts.at(index); - add(SpannerNumberClass::dashes, inStaffIndex, &item, item.number, true, false); + add(SpannerNumberClass::dashes, &item, item.number, true, false); } break; @@ -236,7 +230,7 @@ class SpannerNumberEventCollector if (index < inDirection.dashesStops.size()) { const auto &item = inDirection.dashesStops.at(index); - add(SpannerNumberClass::dashes, inStaffIndex, &item, item.number, false, true); + add(SpannerNumberClass::dashes, &item, item.number, false, true); } break; @@ -253,26 +247,20 @@ class SpannerNumberEventCollector // Assigns numbers within one spanner class. Explicit levels reserve their // number for the serialized extent of their start/stop pair; identity groups // then take the lowest number whose reservations and prior assignments do not -// overlap the group's own extent, per staff pool. +// overlap the group's own extent. The pool is scoped to the part: MusicXML's +// number-level documentation judges concurrency in document order within the +// part, so spanners on different staves of the part still conflict when their +// serialized extents overlap. static void spannerNumberAssignClass(const std::vector &inEvents, std::unordered_map &ioResolved) { constexpr int kSpannerNumberMax = 16; - // occupied[staffIndex][number] -> intervals during which that number is taken - std::map>> occupied; - - const auto reserve = [&occupied](int inNumber, const SpannerNumberInterval &inInterval, int inStaffA, - int inStaffB) { - occupied[inStaffA][inNumber].push_back(inInterval); - if (inStaffB != inStaffA) - { - occupied[inStaffB][inNumber].push_back(inInterval); - } - }; + // occupied[number] -> intervals during which that number is taken + std::map> occupied; // Match explicit start/stop pairs by level in stream order and reserve - // their intervals. A pair that spans staves reserves in both pools. + // their intervals. std::map> openExplicit; for (const auto &event : inEvents) { @@ -292,15 +280,13 @@ static void spannerNumberAssignClass(const std::vector &inEv { const auto *start = stack.back(); stack.pop_back(); - reserve(level, SpannerNumberInterval{start->position, event.position}, start->staffIndex, - event.staffIndex); + occupied[level].push_back(SpannerNumberInterval{start->position, event.position}); } else { // a stop with no visible start (authoring error); reserve its // own position so at least the stop itself cannot collide - reserve(level, SpannerNumberInterval{event.position, event.position}, event.staffIndex, - event.staffIndex); + occupied[level].push_back(SpannerNumberInterval{event.position, event.position}); } } else @@ -308,7 +294,7 @@ static void spannerNumberAssignClass(const std::vector &inEv // a continue between its start and stop is already covered by the // pair's interval; this point reservation only matters when the // continue dangles - reserve(level, SpannerNumberInterval{event.position, event.position}, event.staffIndex, event.staffIndex); + occupied[level].push_back(SpannerNumberInterval{event.position, event.position}); } } for (const auto &levelAndStack : openExplicit) @@ -316,8 +302,8 @@ static void spannerNumberAssignClass(const std::vector &inEv for (const auto *start : levelAndStack.second) { // a start with no stop stays open to the end of the part - reserve(levelAndStack.first, SpannerNumberInterval{start->position, std::numeric_limits::max()}, - start->staffIndex, start->staffIndex); + occupied[levelAndStack.first].push_back( + SpannerNumberInterval{start->position, std::numeric_limits::max()}); } } @@ -327,7 +313,6 @@ static void spannerNumberAssignClass(const std::vector &inEv { SpannerNumberInterval interval; std::vector objects; - std::set staves; }; std::vector groups; @@ -341,13 +326,12 @@ static void spannerNumberAssignClass(const std::vector &inEv const auto found = groupIndexByIdentity.emplace(event.number->identity(), groups.size()); if (found.second) { - groups.push_back(SpannerNumberGroup{SpannerNumberInterval{event.position, event.position}, {}, {}}); + groups.push_back(SpannerNumberGroup{SpannerNumberInterval{event.position, event.position}, {}}); } auto &group = groups.at(found.first->second); group.interval.first = std::min(group.interval.first, event.position); group.interval.last = std::max(group.interval.last, event.position); group.objects.push_back(event.object); - group.staves.insert(event.staffIndex); } for (const auto &group : groups) @@ -356,18 +340,9 @@ static void spannerNumberAssignClass(const std::vector &inEv for (int candidate = 1; candidate <= kSpannerNumberMax && chosen == 0; ++candidate) { bool isFree = true; - for (const int staffIndex : group.staves) + const auto numberIter = occupied.find(candidate); + if (numberIter != occupied.cend()) { - const auto staffIter = occupied.find(staffIndex); - if (staffIter == occupied.cend()) - { - continue; - } - const auto numberIter = staffIter->second.find(candidate); - if (numberIter == staffIter->second.cend()) - { - continue; - } for (const auto &interval : numberIter->second) { if (spannerNumberIntervalsOverlap(interval, group.interval)) @@ -376,10 +351,6 @@ static void spannerNumberAssignClass(const std::vector &inEv break; } } - if (!isFree) - { - break; - } } if (isFree) { @@ -391,10 +362,7 @@ static void spannerNumberAssignClass(const std::vector &inEv MX_THROW("more than 16 spanners of one type are open at the same point in the serialized " "stream; MusicXML number attributes only range from 1 to 16"); } - for (const int staffIndex : group.staves) - { - occupied[staffIndex][chosen].push_back(group.interval); - } + occupied[chosen].push_back(group.interval); for (const void *object : group.objects) { ioResolved[object] = chosen; @@ -408,21 +376,19 @@ void SpannerNumberResolver::resolvePart(const api::PartData &inPart) for (const auto &measure : inPart.measures) { - int staffIndex = 0; for (const auto &staff : measure.staves) { for (const auto &voicePair : staff.voices) { for (const auto ¬e : voicePair.second.notes) { - collector.addNote(staffIndex, note); + collector.addNote(note); } } for (const auto &direction : staff.directions) { - collector.addDirection(staffIndex, direction); + collector.addDirection(direction); } - ++staffIndex; } } diff --git a/src/private/mx/impl/SpannerNumberResolver.h b/src/private/mx/impl/SpannerNumberResolver.h index 0850f87e0..b1d1b88d1 100644 --- a/src/private/mx/impl/SpannerNumberResolver.h +++ b/src/private/mx/impl/SpannerNumberResolver.h @@ -23,26 +23,27 @@ namespace impl // explicit level must be treated as reserved while it is open so a // concurrently-open identity spanner is never handed the same number. // -// Numbers come from a pool of 1..16 per staff and per spanner class (slur, +// Numbers come from a pool of 1..16 per part and per spanner class (slur, // tied, wedge, octave-shift, bracket, and dashes each have their own pool; a -// slur numbered 1 and a wedge numbered 1 do not conflict). Two spanners -// conflict only when they overlap in the order a streaming reader encounters -// them, so resolvePart walks the part in the exact order MeasureWriter -// serializes it: measures in order, staves in order, voices ascending, notes -// in vector order (curve stops, then continues, then starts per note -- -// mirroring NotationsWriter), and each staff's directions in vector order -// (mirroring DirectionWriter's per-direction emission order). An identity -// spanner takes the lowest number that is free across its whole serialized -// extent -- from its first event to its last, whichever of start/stop comes -// first in the stream -- and releases it afterward. +// 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 MeasureWriter serializes it: +// measures in order, staves in order, voices ascending, notes in vector order +// (curve stops, then continues, then starts per note -- mirroring +// NotationsWriter), and each staff's directions in vector order (mirroring +// DirectionWriter's per-direction emission order). An identity spanner takes +// the lowest number that is free across its whole serialized extent -- from +// its first event to its last, whichever of start/stop comes first in the +// stream -- and releases it afterward. // // 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. A -// cross-staff identity spanner reserves its number in every staff pool it -// touches. Pedal starts/stops carry SpannerNumber but the element has -// no number attribute, so pedals are ignored here. +// part sharing a class and id are one logical spanner, even across staves. +// Pedal starts/stops carry SpannerNumber but the element has no +// number attribute, so pedals are ignored here. // -// If more than 16 spanners of one class are open at once in a staff (which no +// If more than 16 spanners of one class are open at once in a part (which no // real score approaches), resolution fails loudly with an exception rather // than emitting an illegal number. class SpannerNumberResolver diff --git a/src/private/mxtest/api/SpannerIdentityTest.cpp b/src/private/mxtest/api/SpannerIdentityTest.cpp index 18c39f536..74fef91a8 100644 --- a/src/private/mxtest/api/SpannerIdentityTest.cpp +++ b/src/private/mxtest/api/SpannerIdentityTest.cpp @@ -164,6 +164,128 @@ TEST(identityAssignsNumbersFromSerializationOrder, SpannerIdentity) T_END +namespace spannerIdentityTest +{ +// A two-staff part (one piano-style part) with one slur per staff. When +// inSlursCrossTheBarline is true each slur starts in measure 1 and stops in +// measure 2, so the two slurs overlap in document order; otherwise both slurs +// start and stop within measure 1 of their own staff and do not. +inline ScoreData makeTwoStaffSlurScore(bool inSlursCrossTheBarline) +{ + ScoreData score; + score.ticksPerQuarter = ticksPerQuarter; + score.parts.emplace_back(); + auto &part = score.parts.back(); + + for (int measureIndex = 0; measureIndex < 2; ++measureIndex) + { + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.timeSignature = TimeChoice(TimeSignatureData{"4", "4"}); + measure.timeSignature.isImplicit = measureIndex > 0; + measure.staves.emplace_back(); + measure.staves.emplace_back(); + for (int staffIndex = 0; staffIndex < 2; ++staffIndex) + { + auto &voice = measure.staves.at(static_cast(staffIndex)).voices[0]; + const Step step = staffIndex == 0 ? Step::g : Step::c; + const int octave = staffIndex == 0 ? 5 : 3; + for (int i = 0; i < 4; ++i) + { + voice.notes.push_back(makeQuarter(i * ticksPerQuarter, step, octave)); + } + } + } + + const auto attachSlur = [&part, inSlursCrossTheBarline](std::size_t staffIndex, const char *identity) { + CurveStart start{CurveType::slur}; + start.number = SpannerNumber{identity}; + part.measures.at(0).staves.at(staffIndex).voices[0].notes.at(0).noteAttachmentData.curveStarts.push_back(start); + + CurveStop stop{CurveType::slur}; + stop.number = SpannerNumber{identity}; + auto &stopMeasure = inSlursCrossTheBarline ? part.measures.at(1) : part.measures.at(0); + const std::size_t stopNoteIndex = inSlursCrossTheBarline ? 0 : 3; + stopMeasure.staves.at(staffIndex) + .voices[0] + .notes.at(stopNoteIndex) + .noteAttachmentData.curveStops.push_back(stop); + }; + attachSlur(0, "top"); + attachSlur(1, "bottom"); + + return score; +} +} // namespace spannerIdentityTest + +// The number-level pool is scoped to the part, not the staff. Two slurs on +// different staves of the same part that overlap in document order (both span +// the barline, so the top staff's slur is still open when measure 1's bottom +// staff serializes) must take different numbers. +TEST(overlappingSlursOnDifferentStavesGetDistinctNumbers, SpannerIdentity) +{ + using namespace spannerIdentityTest; + const auto score = makeTwoStaffSlurScore(true); + + const auto xml = toXml(score); + REQUIRE(!xml.empty()); + const auto roundTripped = fromXml(xml); + + REQUIRE(roundTripped.parts.size() == 1); + const auto &measure1 = roundTripped.parts.at(0).measures.at(0); + const auto &measure2 = roundTripped.parts.at(0).measures.at(1); + + const auto &topStart = measure1.staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.curveStarts; + const auto &bottomStart = measure1.staves.at(1).voices.at(0).notes.at(0).noteAttachmentData.curveStarts; + const auto &topStop = measure2.staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.curveStops; + const auto &bottomStop = measure2.staves.at(1).voices.at(0).notes.at(0).noteAttachmentData.curveStops; + + REQUIRE(topStart.size() == 1); + REQUIRE(bottomStart.size() == 1); + REQUIRE(topStop.size() == 1); + REQUIRE(bottomStop.size() == 1); + + CHECK(SpannerNumber(1) == topStart.at(0).number); + CHECK(SpannerNumber(1) == topStop.at(0).number); + CHECK(SpannerNumber(2) == bottomStart.at(0).number); + CHECK(SpannerNumber(2) == bottomStop.at(0).number); +} + +T_END + +// The spec-blessed reuse case: slurs on different staves that stay within one +// measure never overlap in document order (each staff's notes serialize as a +// contiguous run), so both may take number 1. +TEST(sameMeasureSlursOnDifferentStavesReuseTheNumber, SpannerIdentity) +{ + using namespace spannerIdentityTest; + const auto score = makeTwoStaffSlurScore(false); + + const auto xml = toXml(score); + REQUIRE(!xml.empty()); + const auto roundTripped = fromXml(xml); + + REQUIRE(roundTripped.parts.size() == 1); + const auto &measure1 = roundTripped.parts.at(0).measures.at(0); + + const auto &topStart = measure1.staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.curveStarts; + const auto &bottomStart = measure1.staves.at(1).voices.at(0).notes.at(0).noteAttachmentData.curveStarts; + const auto &topStop = measure1.staves.at(0).voices.at(0).notes.at(3).noteAttachmentData.curveStops; + const auto &bottomStop = measure1.staves.at(1).voices.at(0).notes.at(3).noteAttachmentData.curveStops; + + REQUIRE(topStart.size() == 1); + REQUIRE(bottomStart.size() == 1); + REQUIRE(topStop.size() == 1); + REQUIRE(bottomStop.size() == 1); + + CHECK(SpannerNumber(1) == topStart.at(0).number); + CHECK(SpannerNumber(1) == topStop.at(0).number); + CHECK(SpannerNumber(1) == bottomStart.at(0).number); + CHECK(SpannerNumber(1) == bottomStop.at(0).number); +} + +T_END + // An explicit spanner reserves its number while it is open in the stream: an // identity spanner whose extent covers the explicit one must skip that number. TEST(explicitAndIdentityShareThePool, SpannerIdentity)