From 855325af40a870585a007a0289002b90ca644f81 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Tue, 7 Jul 2026 06:18:19 +0000 Subject: [PATCH 1/3] fix: preserve explicit on single-staff parts NoteWriter::setStaffAndVoice only wrote when the part had more than one staff, since staff number is otherwise redundant with containment (measure -> staves -> voices -> notes). But is legal MusicXML on a single-staff part too, and many real-world exporters write it unconditionally -- so a source's explicit was silently dropped on round-trip on every single-staff part, the single largest drop signature in the api round-trip corpus. Added NoteData::isStaffValueSpecified (mirrors the existing DirectionData::isStaffValueSpecified), set by NoteReader whenever the source note carried an explicit . NoteWriter now emits whenever the part is multi-staff (unchanged, structurally required) or the source had it explicitly, matching the same source-had-it-explicitly convention already used for (NoteData::userRequestedVoiceNumber). Closes #275 --- src/include/mx/api/NoteData.h | 7 +++ src/private/mx/api/NoteData.cpp | 5 +- src/private/mx/impl/NoteFunctions.cpp | 1 + src/private/mx/impl/NoteReader.cpp | 7 ++- src/private/mx/impl/NoteReader.h | 6 ++ src/private/mx/impl/NoteWriter.cpp | 2 +- src/private/mxtest/api/NoteDataTest.cpp | 60 +++++++++++++++++++ src/private/mxtest/api/roundtrip-baseline.txt | 5 ++ 8 files changed, 87 insertions(+), 6 deletions(-) diff --git a/src/include/mx/api/NoteData.h b/src/include/mx/api/NoteData.h index 19fc1001e..c953e7e71 100644 --- a/src/include/mx/api/NoteData.h +++ b/src/include/mx/api/NoteData.h @@ -135,6 +135,12 @@ class NoteData PitchData pitchData; // step, alter, octave, accidental, etc int userRequestedVoiceNumber; + // Whether the source note carried an explicit element. mx::api normally only + // writes when the part has more than one staff (where it is structurally + // required); this preserves a source's redundant-but-legal on a single-staff + // part instead of silently dropping it. Mirrors DirectionData::isStaffValueSpecified. + bool isStaffValueSpecified; + // The zero-based index of the staff on which this note is displayed, for the rare case // that it differs from the staff that contains the note (cross-staff notation, e.g. a // beamed piano run or a chord that dips onto the other staff). The note stays in its @@ -189,6 +195,7 @@ MXAPI_EQUALS_MEMBER(isCue) MXAPI_EQUALS_MEMBER(graceSlash) MXAPI_EQUALS_MEMBER(pitchData) MXAPI_EQUALS_MEMBER(userRequestedVoiceNumber) +MXAPI_EQUALS_MEMBER(isStaffValueSpecified) MXAPI_EQUALS_MEMBER(crossStaffIndex) MXAPI_EQUALS_MEMBER(stem) MXAPI_EQUALS_MEMBER(stemPositionData) diff --git a/src/private/mx/api/NoteData.cpp b/src/private/mx/api/NoteData.cpp index 20d51b4a4..7f7c9985d 100644 --- a/src/private/mx/api/NoteData.cpp +++ b/src/private/mx/api/NoteData.cpp @@ -11,8 +11,9 @@ namespace api NoteData::NoteData() : isRest{false}, isMeasureRest{false}, isUnpitched{false}, isDisplayStepOctaveSpecified{false}, isChord{false}, isTieStart{false}, isTieStop{false}, tieLetRing{}, isGrace{false}, graceSlash{Bool::unspecified}, isCue{false}, - notehead{Notehead::normal}, pitchData{}, userRequestedVoiceNumber{VALUE_UNSPECIFIED}, stem{Stem::unspecified}, - tickTimePosition{0}, durationData{}, beams{}, positionData{}, printData{}, noteAttachmentData{}, lyrics{} + notehead{Notehead::normal}, pitchData{}, userRequestedVoiceNumber{VALUE_UNSPECIFIED}, + isStaffValueSpecified{false}, stem{Stem::unspecified}, tickTimePosition{0}, durationData{}, beams{}, + positionData{}, printData{}, noteAttachmentData{}, lyrics{} { } } // namespace api diff --git a/src/private/mx/impl/NoteFunctions.cpp b/src/private/mx/impl/NoteFunctions.cpp index d8a52bbe1..625d288bb 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -77,6 +77,7 @@ api::NoteData NoteFunctions::parseNote() const myOutNoteData.pitchData.octave = reader.getOctave(); myOutNoteData.userRequestedVoiceNumber = reader.getVoiceNumber(); + myOutNoteData.isStaffValueSpecified = reader.getIsStaffSpecified(); myOutNoteData.notehead = converter.convert(reader.getNoteheadValue()); diff --git a/src/private/mx/impl/NoteReader.cpp b/src/private/mx/impl/NoteReader.cpp index 4686e8c80..6516861a2 100644 --- a/src/private/mx/impl/NoteReader.cpp +++ b/src/private/mx/impl/NoteReader.cpp @@ -144,8 +144,8 @@ NoteReader::NoteReader(const core::Note &mxNote) : myNote(mxNote), myNoteChoice(myNote.choice()), myFullNoteGroup(findFullNoteGroup(myNoteChoice)), myIsNormal(false), myIsGrace(false), myIsCue(false), myIsRest(false), myIsChord(false), myIsMeasureRest(false), myIsUnpitched(false), myIsPitch(false), myIsDisplayStepOctaveSpecified(false), myDurationValue(0.0), - myStep(core::Step::c()), myAlter(0), myCents(0.0), myOctave(4), myStaffNumber(0), myVoiceNumber(0), - myNoteheadValue(core::NoteheadValue::normal()), myDurationType(core::NoteTypeValue::maxima()), + myStep(core::Step::c()), myAlter(0), myCents(0.0), myOctave(4), myStaffNumber(0), myIsStaffSpecified(false), + myVoiceNumber(0), myNoteheadValue(core::NoteheadValue::normal()), myDurationType(core::NoteTypeValue::maxima()), myIsDurationTypeSpecified(false), myNumDots(0), myBeams(), myTimeModificationActualNotes(-1), myTimeModificationNormalNotes(-1), myTimeModificationNormalType(core::NoteTypeValue::maxima()), myTimeModificationNormalTypeDots(0), myHasAccidental(false), myAccidental(core::AccidentalValue::natural()), @@ -304,7 +304,8 @@ void NoteReader::setChord() void NoteReader::setStaffNumber() { - if (myNote.staff().has_value()) + myIsStaffSpecified = myNote.staff().has_value(); + if (myIsStaffSpecified) { myStaffNumber = *myNote.staff(); } diff --git a/src/private/mx/impl/NoteReader.h b/src/private/mx/impl/NoteReader.h index 8ecc3f1a9..a5f51f501 100644 --- a/src/private/mx/impl/NoteReader.h +++ b/src/private/mx/impl/NoteReader.h @@ -117,6 +117,11 @@ class NoteReader return myStaffNumber; } + inline bool getIsStaffSpecified() const + { + return myIsStaffSpecified; + } + inline int getVoiceNumber() const { return myVoiceNumber; @@ -251,6 +256,7 @@ class NoteReader double myCents; int myOctave; int myStaffNumber; + bool myIsStaffSpecified; int myVoiceNumber; core::NoteheadValue myNoteheadValue; core::NoteTypeValue myDurationType; diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index 06b19d2f6..703e7c91c 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -427,7 +427,7 @@ void NoteWriter::setStaffAndVoice() const // position, but the displayed staff comes from the override (NoteData.h) myOutNote.setStaff(*myNoteData.crossStaffIndex + 1); } - else if (myCursor.getNumStaves() > 1 && myCursor.staffIndex >= 0) + else if (myCursor.staffIndex >= 0 && (myCursor.getNumStaves() > 1 || myNoteData.isStaffValueSpecified)) { myOutNote.setStaff(myCursor.staffIndex + 1); } diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index 805c2c569..7180459f7 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -12,6 +12,7 @@ #include "mx/core/generated/Document.h" #include "mx/core/generated/MusicDataChoice.h" #include "mxtest/api/RoundTrip.h" +#include "mxtest/api/TestHelpers.h" #include "mxtest/file/Path.h" using namespace std; @@ -1480,4 +1481,63 @@ TEST(strongAccentDirection, NoteData) T_END; +TEST(explicitStaffOnSingleStaffPartRoundTrips, NoteData) +{ + const std::string xml = R"( + + + + x + + + + + + 1 + + + C4 + 1 + 1 + quarter + 1 + + + + +)"; + + const auto score = fromXml(xml); + const auto ¬es = score.parts.back().measures.back().staves.back().voices.at(0).notes; + REQUIRE(notes.size() == 1); + CHECK(notes.at(0).isStaffValueSpecified); + + const auto outXml = toXml(score); + CHECK(outXml.find("1") != std::string::npos); +} + +T_END; + +TEST(implicitStaffOnSingleStaffPartOmitsElement, NoteData) +{ + ScoreData score; + 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]; + + NoteData note; + note.tickTimePosition = 0; + note.durationData.durationTimeTicks = score.ticksPerQuarter; + note.durationData.durationName = DurationName::quarter; + voice.notes.push_back(note); + + const auto xml = toXml(score); + CHECK(xml.find("") == std::string::npos); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 02d9bb0e0..8775099b9 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -320,3 +320,8 @@ lysuite/ly11h_TimeSignatures_SenzaMisura.xml # trims just those unrelated gaps so the composite time-signature round-trip # itself is pinned. rpatters1/timesigs_composite-ref-modified.musicxml + +# Unblocked by NoteData::isStaffValueSpecified: NoteWriter only wrote +# for multi-staff parts, dropping a source's explicit (but structurally +# redundant) on a single-staff part. +lysuite/ly24c_GraceNote_MeasureEnd.xml From 1ff2c1045a5dda4d8607317cd1eac3926a1d60fe Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 13:42:24 +0000 Subject: [PATCH 2/3] refactor: NoteData staff knob as ternary writeStaffNumber Replace the two-state bool isStaffValueSpecified with Bool writeStaffNumber, the same principle-7 fidelity-knob shape as ClefData::writeStaffNumber: unspecified (the default) applies the automatic rule (omit on a single-staff part, include it otherwise), yes/no force the element on or off, and the reader records an override only when the source diverges from the automatic rule. The bool could not express omission: a multi-staff note whose source legally omitted (implied 1) had 1 injected on round-trip. It also made api -> xml -> api non-idempotent on multi-staff parts (authored false read back as true on every note), and exporters that write unconditionally stamped the flag true corpus-wide, recording noise instead of divergence. The writer ignores no for a note off the first staff, where omission would move the note to staff 1. --- src/include/mx/api/NoteData.h | 15 ++-- src/private/mx/api/NoteData.cpp | 2 +- src/private/mx/impl/NoteFunctions.cpp | 14 +++- src/private/mx/impl/NoteWriter.cpp | 20 ++++- src/private/mxtest/api/NoteDataTest.cpp | 76 ++++++++++++++++++- src/private/mxtest/api/roundtrip-baseline.txt | 2 +- 6 files changed, 117 insertions(+), 12 deletions(-) diff --git a/src/include/mx/api/NoteData.h b/src/include/mx/api/NoteData.h index c953e7e71..f5a8692a1 100644 --- a/src/include/mx/api/NoteData.h +++ b/src/include/mx/api/NoteData.h @@ -135,11 +135,14 @@ class NoteData PitchData pitchData; // step, alter, octave, accidental, etc int userRequestedVoiceNumber; - // Whether the source note carried an explicit element. mx::api normally only - // writes when the part has more than one staff (where it is structurally - // required); this preserves a source's redundant-but-legal on a single-staff - // part instead of silently dropping it. Mirrors DirectionData::isStaffValueSpecified. - bool isStaffValueSpecified; + // Most users can ignore this; leave it unspecified. It only controls whether the note's + // optional element is written. unspecified (the default) applies the right rule + // automatically: omit on a single-staff part (where 1 is implied) and include it + // otherwise. yes/no force the element on or off, except that no is ignored for a note that + // is not on the first staff (omitting there would move the note to staff 1). It + // exists for round-trip fidelity - reading a file sets yes/no only when the source diverged + // from the automatic rule. Same convention as ClefData::writeStaffNumber. + Bool writeStaffNumber; // The zero-based index of the staff on which this note is displayed, for the rare case // that it differs from the staff that contains the note (cross-staff notation, e.g. a @@ -195,7 +198,7 @@ MXAPI_EQUALS_MEMBER(isCue) MXAPI_EQUALS_MEMBER(graceSlash) MXAPI_EQUALS_MEMBER(pitchData) MXAPI_EQUALS_MEMBER(userRequestedVoiceNumber) -MXAPI_EQUALS_MEMBER(isStaffValueSpecified) +MXAPI_EQUALS_MEMBER(writeStaffNumber) MXAPI_EQUALS_MEMBER(crossStaffIndex) MXAPI_EQUALS_MEMBER(stem) MXAPI_EQUALS_MEMBER(stemPositionData) diff --git a/src/private/mx/api/NoteData.cpp b/src/private/mx/api/NoteData.cpp index 7f7c9985d..619b23984 100644 --- a/src/private/mx/api/NoteData.cpp +++ b/src/private/mx/api/NoteData.cpp @@ -12,7 +12,7 @@ NoteData::NoteData() : isRest{false}, isMeasureRest{false}, isUnpitched{false}, isDisplayStepOctaveSpecified{false}, isChord{false}, isTieStart{false}, isTieStop{false}, tieLetRing{}, isGrace{false}, graceSlash{Bool::unspecified}, isCue{false}, notehead{Notehead::normal}, pitchData{}, userRequestedVoiceNumber{VALUE_UNSPECIFIED}, - isStaffValueSpecified{false}, stem{Stem::unspecified}, tickTimePosition{0}, durationData{}, beams{}, + writeStaffNumber{Bool::unspecified}, stem{Stem::unspecified}, tickTimePosition{0}, durationData{}, beams{}, positionData{}, printData{}, noteAttachmentData{}, lyrics{} { } diff --git a/src/private/mx/impl/NoteFunctions.cpp b/src/private/mx/impl/NoteFunctions.cpp index 625d288bb..d93924242 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -77,7 +77,19 @@ api::NoteData NoteFunctions::parseNote() const myOutNoteData.pitchData.octave = reader.getOctave(); myOutNoteData.userRequestedVoiceNumber = reader.getVoiceNumber(); - myOutNoteData.isStaffValueSpecified = reader.getIsStaffSpecified(); + + // Auto rule (see NoteData::writeStaffNumber): is included on a multi-staff part and + // omitted on a single-staff part. Record an override only when the source diverges from that + // rule, so the common case stays unspecified. + const bool staffAutoIncludes = myCursor.getNumStaves() > 1; + if (reader.getIsStaffSpecified() && !staffAutoIncludes) + { + myOutNoteData.writeStaffNumber = api::Bool::yes; + } + else if (!reader.getIsStaffSpecified() && staffAutoIncludes) + { + myOutNoteData.writeStaffNumber = api::Bool::no; + } myOutNoteData.notehead = converter.convert(reader.getNoteheadValue()); diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index 703e7c91c..4f8efd99a 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -427,9 +427,25 @@ void NoteWriter::setStaffAndVoice() const // position, but the displayed staff comes from the override (NoteData.h) myOutNote.setStaff(*myNoteData.crossStaffIndex + 1); } - else if (myCursor.staffIndex >= 0 && (myCursor.getNumStaves() > 1 || myNoteData.isStaffValueSpecified)) + else if (myCursor.staffIndex >= 0) { - myOutNote.setStaff(myCursor.staffIndex + 1); + // Auto rule: is structurally required on a multi-staff part and implied (1) on a + // single-staff part. writeStaffNumber forces the decision either way (round-trip + // fidelity), except that no cannot suppress off the first staff - MusicXML would + // read the omission as staff 1. + bool includeStaff = myCursor.getNumStaves() > 1; + if (myNoteData.writeStaffNumber == api::Bool::yes) + { + includeStaff = true; + } + else if (myNoteData.writeStaffNumber == api::Bool::no && myCursor.staffIndex == 0) + { + includeStaff = false; + } + if (includeStaff) + { + myOutNote.setStaff(myCursor.staffIndex + 1); + } } const bool sourceHadVoice = myNoteData.userRequestedVoiceNumber != api::VALUE_UNSPECIFIED; diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index 0ac7e1ec9..4bf2ce037 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -1587,7 +1587,7 @@ TEST(explicitStaffOnSingleStaffPartRoundTrips, NoteData) const auto score = fromXml(xml); const auto ¬es = score.parts.back().measures.back().staves.back().voices.at(0).notes; REQUIRE(notes.size() == 1); - CHECK(notes.at(0).isStaffValueSpecified); + CHECK(notes.at(0).writeStaffNumber == Bool::yes); const auto outXml = toXml(score); CHECK(outXml.find("1") != std::string::npos); @@ -1595,6 +1595,80 @@ TEST(explicitStaffOnSingleStaffPartRoundTrips, NoteData) T_END; +TEST(omittedStaffOnMultiStaffPartRoundTrips, NoteData) +{ + const std::string xml = R"( + + + + x + + + + + + 1 + 2 + + + C5 + 1 + 1 + quarter + + 1 + + C3 + 1 + 1 + quarter + 2 + + + + +)"; + + const auto score = fromXml(xml); + const auto &staves = score.parts.back().measures.back().staves; + REQUIRE(staves.size() == 2); + const auto &firstStaffNotes = staves.at(0).voices.at(0).notes; + const auto &secondStaffNotes = staves.at(1).voices.at(0).notes; + REQUIRE(firstStaffNotes.size() == 1); + REQUIRE(secondStaffNotes.size() == 1); + CHECK(firstStaffNotes.at(0).writeStaffNumber == Bool::no); + CHECK(secondStaffNotes.at(0).writeStaffNumber == Bool::unspecified); + + const auto outXml = toXml(score); + CHECK(outXml.find("1") == std::string::npos); + CHECK(outXml.find("2") != std::string::npos); +} + +T_END; + +TEST(writeStaffNumberNoIsIgnoredOffFirstStaff, NoteData) +{ + ScoreData score; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + measure.staves.emplace_back(); + + NoteData note; + note.tickTimePosition = 0; + note.durationData.durationTimeTicks = score.ticksPerQuarter; + note.durationData.durationName = DurationName::quarter; + note.writeStaffNumber = Bool::no; + measure.staves.at(1).voices[0].notes.push_back(note); + + const auto xml = toXml(score); + CHECK(xml.find("2") != std::string::npos); +} + +T_END; + TEST(implicitStaffOnSingleStaffPartOmitsElement, NoteData) { ScoreData score; diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index e1d586582..22400f90b 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -318,7 +318,7 @@ lysuite/ly11h_TimeSignatures_SenzaMisura.xml # composite time-signature round-trip in isolation from those. rpatters1/timesigs_composite-ref-modified.musicxml -# Unblocked by NoteData::isStaffValueSpecified: NoteWriter only wrote +# Unblocked by NoteData::writeStaffNumber: NoteWriter only wrote # for multi-staff parts, dropping a source's explicit (but structurally # redundant) on a single-staff part. lysuite/ly24c_GraceNote_MeasureEnd.xml From 9abdedfa83fc5701ff3930b6778eecc9d12139cb Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 13:51:49 +0000 Subject: [PATCH 3/3] test: pin four more round-trip fixtures lysuite/ly02e_Rests_NoType.xml is unlocked by writeStaffNumber's no direction: a two-staff part whose last note legally omits no longer has 1 injected on round-trip. custom/systems-and-pages.xml, ksuite/k008a_Beaming.xml, and ksuite/k014a_Fermatas.xml were found already passing when discovery was re-run; they were unlocked by earlier merged changes and never pinned. --- src/private/mxtest/api/roundtrip-baseline.txt | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 22400f90b..8bb4082ac 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -323,6 +323,19 @@ rpatters1/timesigs_composite-ref-modified.musicxml # redundant) on a single-staff part. lysuite/ly24c_GraceNote_MeasureEnd.xml +# Also unblocked by NoteData::writeStaffNumber, in the other direction: a +# two-staff part whose last note legally omits (implied 1). The +# writer used to inject 1 there; the reader now records +# writeStaffNumber = no and the omission round-trips. +lysuite/ly02e_Rests_NoType.xml + +# Found already passing when discovery was re-run for the writeStaffNumber +# work; unlocked by earlier merged changes and never pinned. Pinned here to +# defend them. +custom/systems-and-pages.xml +ksuite/k008a_Beaming.xml +ksuite/k014a_Fermatas.xml + # The original of the file above, now unblocked by PartData::groups # (/), MeasureData's measureNumbering multiple-rest-*/ # system-relation attributes, and SoundData::swing (/). Its