From be9c28e5222ecaa374266a013854e4b98eb46a31 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sat, 19 Sep 2026 15:02:28 +0200 Subject: [PATCH] fix: write back tuplet position data, midi-unpitched and accidental marks --- src/include/mx/api/MarkData.h | 1 + src/private/mx/api/MarkData.cpp | 29 ++++ src/private/mx/impl/NotationsWriter.cpp | 25 +++ src/private/mx/impl/NotationsWriter.h | 1 + src/private/mx/impl/PartWriter.cpp | 9 + .../mxtest/api/MidiNameRoundTripTest.cpp | 55 ++++-- src/private/mxtest/api/NoteDataTest.cpp | 158 ++++++++++++++++++ 7 files changed, 261 insertions(+), 17 deletions(-) diff --git a/src/include/mx/api/MarkData.h b/src/include/mx/api/MarkData.h index 938b68700..2fe21d5a1 100644 --- a/src/include/mx/api/MarkData.h +++ b/src/include/mx/api/MarkData.h @@ -222,6 +222,7 @@ bool isMarkFermata(MarkType); bool isMarkArpeggiate(MarkType); bool isMarkNonArpeggiate(MarkType); bool isMarkOtherNotation(MarkType); +bool isMarkAccidentalMark(MarkType); bool isMarkCustom(MarkType); std::string getCustomMarkName(MarkType); diff --git a/src/private/mx/api/MarkData.cpp b/src/private/mx/api/MarkData.cpp index cd4a7a4d0..6ba96a854 100644 --- a/src/private/mx/api/MarkData.cpp +++ b/src/private/mx/api/MarkData.cpp @@ -75,6 +75,35 @@ bool isMarkOtherNotation(MarkType markType) return markType == MarkType::otherNotation; } +// Every accidental-mark value. accidentalUnknown is not one of them: it is the error state, and +// the converter would turn it into a natural, inventing a glyph the caller did not ask for. +bool isMarkAccidentalMark(MarkType markType) +{ + return (markType == MarkType::accidentalMarkSharp) || (markType == MarkType::accidentalMarkNatural) || + (markType == MarkType::accidentalMarkFlat) || (markType == MarkType::accidentalMarkDoubleSharp) || + (markType == MarkType::accidentalMarkSharpSharp) || (markType == MarkType::accidentalMarkFlatFlat) || + (markType == MarkType::accidentalMarkNaturalSharp) || (markType == MarkType::accidentalMarkNaturalFlat) || + (markType == MarkType::accidentalMarkQuarterFlat) || (markType == MarkType::accidentalMarkQuarterSharp) || + (markType == MarkType::accidentalMarkThreeQuartersFlat) || + (markType == MarkType::accidentalMarkThreeQuartersSharp) || + (markType == MarkType::accidentalMarkSharpDown) || (markType == MarkType::accidentalMarkSharpUp) || + (markType == MarkType::accidentalMarkNaturalDown) || (markType == MarkType::accidentalMarkNaturalUp) || + (markType == MarkType::accidentalMarkFlatDown) || (markType == MarkType::accidentalMarkFlatUp) || + (markType == MarkType::accidentalMarkDoubleSharpDown) || + (markType == MarkType::accidentalMarkDoubleSharpUp) || (markType == MarkType::accidentalMarkFlatFlatDown) || + (markType == MarkType::accidentalMarkFlatFlatUp) || (markType == MarkType::accidentalMarkArrowDown) || + (markType == MarkType::accidentalMarkArrowUp) || (markType == MarkType::accidentalMarkTripleSharp) || + (markType == MarkType::accidentalMarkTripleFlat) || + (markType == MarkType::accidentalMarkSlashQuarterSharp) || + (markType == MarkType::accidentalMarkSlashSharp) || (markType == MarkType::accidentalMarkSlashFlat) || + (markType == MarkType::accidentalMarkDoubleSlashFlat) || (markType == MarkType::accidentalMarkSharp1) || + (markType == MarkType::accidentalMarkSharp2) || (markType == MarkType::accidentalMarkSharp3) || + (markType == MarkType::accidentalMarkSharp5) || (markType == MarkType::accidentalMarkFlat1) || + (markType == MarkType::accidentalMarkFlat2) || (markType == MarkType::accidentalMarkFlat3) || + (markType == MarkType::accidentalMarkFlat4) || (markType == MarkType::accidentalMarkSori) || + (markType == MarkType::accidentalMarkKoron); +} + bool isMarkPedal(MarkType markType) { return (markType == MarkType::pedal) || (markType == MarkType::damp); diff --git a/src/private/mx/impl/NotationsWriter.cpp b/src/private/mx/impl/NotationsWriter.cpp index 5cd53a701..aac726dac 100644 --- a/src/private/mx/impl/NotationsWriter.cpp +++ b/src/private/mx/impl/NotationsWriter.cpp @@ -4,6 +4,7 @@ #include "mx/impl/NotationsWriter.h" #include "mx/core/Token.h" +#include "mx/core/generated/AccidentalMark.h" #include "mx/core/generated/Arpeggiate.h" #include "mx/core/generated/ArrowChoice.h" #include "mx/core/generated/ArrowChoiceGroup.h" @@ -103,6 +104,7 @@ core::NotationsChoice notationsWriterMakeTupletStop(const api::TupletStop &inTup { core::Tuplet tuplet; tuplet.setType(core::StartStop::stop()); + setAttributesFromPositionData(inTupletStop.positionData, tuplet); setId(inTupletStop.id, tuplet, diagnostics, location); if (inTupletStop.numberLevel > 0) @@ -236,6 +238,7 @@ core::Notations NotationsWriter::getNotations() const { core::Tuplet tuplet; tuplet.setType(core::StartStop::start()); + setAttributesFromPositionData(tupletStart.positionData, tuplet); setId(tupletStart.id, tuplet, myScoreWriter.getDiagnostics(), cursorLocation(myCursor)); core::TupletPortion actual; @@ -336,6 +339,10 @@ core::Notations NotationsWriter::getNotations() const { this->addOrnament(mark, ornaments); } + if (isMarkAccidentalMark(mark.markType)) + { + this->addAccidentalMark(mark, outNotations); + } if (isMarkTechnical(mark.markType)) { this->addTechnical(mark, technicals); @@ -982,6 +989,24 @@ void NotationsWriter::addOrnament(const api::MarkData &mark, core::Ornaments &ou outOrnaments.addGroup(group); } +// An accidental mark is written at the notations level. MusicXML also allows one inside +// , next to the ornament it decorates, but the api does not record which of the two +// places a mark came from -- both arrive here as the same MarkType -- and an group is +// required to carry an ornament, so putting one there would mean inventing an ornament. Marks +// from either place are written here instead. +void NotationsWriter::addAccidentalMark(const api::MarkData &mark, core::Notations &outNotations) const +{ + if (!isMarkAccidentalMark(mark.markType)) + { + return; + } + + core::AccidentalMark accidentalMark; + accidentalMark.setValue(myConverter.convertAccidentalMark(mark.markType)); + setAttributesFromPositionData(mark.positionData, accidentalMark); + outNotations.addChoice(core::NotationsChoice::accidentalMark(accidentalMark)); +} + void NotationsWriter::addTechnical(const api::MarkData &mark, core::Technical &outTechnical) const { if (!isMarkTechnical(mark.markType)) diff --git a/src/private/mx/impl/NotationsWriter.h b/src/private/mx/impl/NotationsWriter.h index 3b852bbd5..e18d44e31 100644 --- a/src/private/mx/impl/NotationsWriter.h +++ b/src/private/mx/impl/NotationsWriter.h @@ -36,6 +36,7 @@ class NotationsWriter core::NotationsChoice makeTechnicalNotationsChoice() const; void addArticulation(const api::MarkData &markData, core::Articulations &outArticulations) const; void addOrnament(const api::MarkData &markData, core::Ornaments &outOrnaments) const; + void addAccidentalMark(const api::MarkData &markData, core::Notations &outNotations) const; void addTechnical(const api::MarkData &markData, core::Technical &outTechnical) const; // Kept as their own (non-inlined) methods rather than loops inlined in getNotations(), the diff --git a/src/private/mx/impl/PartWriter.cpp b/src/private/mx/impl/PartWriter.cpp index 7d8e0486c..279b7ce9e 100644 --- a/src/private/mx/impl/PartWriter.cpp +++ b/src/private/mx/impl/PartWriter.cpp @@ -270,6 +270,15 @@ core::ScorePart PartWriter::getScorePart() const midiInstrument.setMIDIProgram(program); } + if (myPartData.instrumentData.midiData.unpitched != api::VALUE_UNSPECIFIED) + { + addMidiElement = true; + const core::MIDI128 unpitched{myPartData.instrumentData.midiData.unpitched}; + reportAdjusted(diagnostics(), partLocation(myPartIndex), "midi-unpitched", + myPartData.instrumentData.midiData.unpitched, unpitched.value()); + midiInstrument.setMIDIUnpitched(unpitched); + } + if (myPartData.instrumentData.midiData.isElevationSpecified) { addMidiElement = true; diff --git a/src/private/mxtest/api/MidiNameRoundTripTest.cpp b/src/private/mxtest/api/MidiNameRoundTripTest.cpp index b2bcd3f98..0f80c6385 100644 --- a/src/private/mxtest/api/MidiNameRoundTripTest.cpp +++ b/src/private/mxtest/api/MidiNameRoundTripTest.cpp @@ -13,9 +13,7 @@ using namespace std; using namespace mx::api; -namespace -{ -ScoreData makeScoreWithMidiName(const std::string &midiName) +static ScoreData midiRoundTripMakeScore(const std::string &midiName) { VoiceData voiceData; NoteData n; @@ -45,15 +43,15 @@ ScoreData makeScoreWithMidiName(const std::string &midiName) } // The same score, but with the instrument id left to the library. -ScoreData makeScoreWithMidiNameAndNoInstrumentId(const std::string &midiName) +static ScoreData midiRoundTripMakeScoreWithNoId(const std::string &midiName) { - auto scoreData = makeScoreWithMidiName(midiName); + auto scoreData = midiRoundTripMakeScore(midiName); scoreData.parts.at(0).instrumentData.uniqueId.clear(); return scoreData; } // The value of the first id attribute after the given tag name. -std::string idAfterTag(const std::string &xml, const std::string &tag) +static std::string midiRoundTripIdAfterTag(const std::string &xml, const std::string &tag) { const auto tagPosition = xml.find(tag); if (tagPosition == std::string::npos) @@ -73,16 +71,15 @@ std::string idAfterTag(const std::string &xml, const std::string &tag) } return xml.substr(valueStart, valueEnd - valueStart); } -} // namespace TEST(midiInstrumentId, isTheSameOnEveryWrite) { - const auto scoreData = makeScoreWithMidiNameAndNoInstrumentId("Flute Player One"); + const auto scoreData = midiRoundTripMakeScoreWithNoId("Flute Player One"); const auto first = mxtest::toXml(scoreData); // Writing an unrelated score in between used to shift the id the library gives the // instrument, because the counter behind it outlived the call. - static_cast(mxtest::toXml(makeScoreWithMidiName("Other Player"))); + static_cast(mxtest::toXml(midiRoundTripMakeScore("Other Player"))); const auto second = mxtest::toXml(scoreData); CHECK(!first.empty()); @@ -91,17 +88,17 @@ TEST(midiInstrumentId, isTheSameOnEveryWrite) TEST(midiInstrumentId, isReferencedByTheMidiInstrument) { - const auto xml = mxtest::toXml(makeScoreWithMidiNameAndNoInstrumentId("Flute Player One")); - const auto scoreInstrumentId = idAfterTag(xml, ". writeDeviceId defaults to unspecified, so no id is // emitted here; only the port is exercised. - auto in = makeScoreWithMidiName("Flute Player One"); + auto in = midiRoundTripMakeScore("Flute Player One"); in.parts.at(0).instrumentData.midiData.devicePort = 1; const auto out = mxtest::roundTrip(in); @@ -137,7 +134,7 @@ TEST(midiDeviceRoundTrip, writeDeviceIdSurvivesWriteAndRead) // When a source spells out the device-to-instrument link, mx re-emits it as // , taking the id from the part's instrument (uniqueId "P1-I1" // here), so the flag survives the round-trip. - auto in = makeScoreWithMidiName("Flute Player One"); + auto in = midiRoundTripMakeScore("Flute Player One"); in.parts.at(0).instrumentData.midiData.devicePort = 1; in.parts.at(0).instrumentData.midiData.writeDeviceId = Bool::yes; @@ -146,4 +143,28 @@ TEST(midiDeviceRoundTrip, writeDeviceIdSurvivesWriteAndRead) CHECK(out.parts.at(0).instrumentData.midiData.writeDeviceId == Bool::yes); } +// issue #443: midi-unpitched names the MIDI note a percussion instrument sounds. The reader read +// it and the writer never wrote it. +TEST(midiUnpitchedRoundTrip, survivesWriteAndRead) +{ + auto in = midiRoundTripMakeScore("Flute Player One"); + in.parts.at(0).instrumentData.midiData.unpitched = 43; + + const auto written = mxtest::toXml(in); + CHECK(written.find("43") != std::string::npos); + + const auto out = mxtest::roundTrip(in); + REQUIRE(out.parts.size() == 1); + CHECK_EQUAL(43, out.parts.at(0).instrumentData.midiData.unpitched); +} + +// VALUE_UNSPECIFIED means the part has no unpitched note, and an ordinary pitched instrument +// always does: nothing is written for it. +TEST(midiUnpitchedAbsentByDefault, writesNoElement) +{ + const auto in = midiRoundTripMakeScore("Flute Player One"); + const auto written = mxtest::toXml(in); + CHECK(written.find("midi-unpitched") == std::string::npos); +} + #endif diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index c90af6a62..b49d19b6c 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -1867,6 +1867,164 @@ TEST(guessedTupletNormalKeepsTheNoteDots, NoteData) CHECK_EQUAL(1, rereread.at(0).normalDots); } +// issue #443: the position attributes of a were read into TupletStart and TupletStop and +// never written, so the placement of a tuplet bracket was lost on write. +TEST(tupletPositionDataSurvivesWriteAndRead, 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 &staff = measure.staves.back(); + auto &voice = staff.voices[0]; + voice.notes.emplace_back(); + auto ¬e = voice.notes.back(); + + TupletStart start; + start.positionData.isDefaultXSpecified = true; + start.positionData.defaultX = 12.5; + start.positionData.isDefaultYSpecified = true; + start.positionData.defaultY = -3.5; + start.positionData.isRelativeXSpecified = true; + start.positionData.relativeX = 1.5; + start.positionData.isRelativeYSpecified = true; + start.positionData.relativeY = 2.5; + start.positionData.placement = Placement::above; + note.noteAttachmentData.tupletStarts.push_back(start); + + TupletStop stop; + stop.positionData.isDefaultXSpecified = true; + stop.positionData.defaultX = 44.5; + note.noteAttachmentData.tupletStops.push_back(stop); + + const auto written = toXml(score); + CHECK(written.find("default-x=\"12.5\"") != std::string::npos); + CHECK(written.find("default-y=\"-3.5\"") != std::string::npos); + CHECK(written.find("relative-x=\"1.5\"") != std::string::npos); + CHECK(written.find("relative-y=\"2.5\"") != std::string::npos); + CHECK(written.find("placement=\"above\"") != std::string::npos); + CHECK(written.find("default-x=\"44.5\"") != std::string::npos); + + const auto out = roundTrip(score); + const auto &outStarts = + out.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.tupletStarts; + REQUIRE(1 == outStarts.size()); + CHECK(outStarts.at(0).positionData.isDefaultXSpecified); + CHECK(outStarts.at(0).positionData.isDefaultYSpecified); + CHECK(outStarts.at(0).positionData.isRelativeXSpecified); + CHECK(outStarts.at(0).positionData.isRelativeYSpecified); + CHECK_DOUBLES_EQUAL(12.5, outStarts.at(0).positionData.defaultX, 0.00001); + CHECK_DOUBLES_EQUAL(-3.5, outStarts.at(0).positionData.defaultY, 0.00001); + CHECK_DOUBLES_EQUAL(1.5, outStarts.at(0).positionData.relativeX, 0.00001); + CHECK_DOUBLES_EQUAL(2.5, outStarts.at(0).positionData.relativeY, 0.00001); + CHECK(Placement::above == outStarts.at(0).positionData.placement); + + const auto &outStops = + out.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.tupletStops; + REQUIRE(1 == outStops.size()); + CHECK(outStops.at(0).positionData.isDefaultXSpecified); + CHECK_DOUBLES_EQUAL(44.5, outStops.at(0).positionData.defaultX, 0.00001); +} + +T_END; + +// issue #443: accidental marks were read into MarkData and never written back. +TEST(accidentalMarkSurvivesWriteAndRead, 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 &staff = measure.staves.back(); + auto &voice = staff.voices[0]; + voice.notes.emplace_back(); + auto ¬e = voice.notes.back(); + + MarkData mark{MarkType::accidentalMarkSharp}; + mark.positionData.isDefaultXSpecified = true; + mark.positionData.defaultX = 7.5; + mark.positionData.placement = Placement::above; + note.noteAttachmentData.marks.push_back(mark); + + const auto written = toXml(score); + CHECK(written.find("sharp<") != std::string::npos); + CHECK(written.find("default-x=\"7.5\"") != std::string::npos); + + const auto out = roundTrip(score); + const auto &marks = out.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.marks; + REQUIRE(1 == marks.size()); + CHECK(MarkType::accidentalMarkSharp == marks.at(0).markType); + CHECK(marks.at(0).positionData.isDefaultXSpecified); + CHECK_DOUBLES_EQUAL(7.5, marks.at(0).positionData.defaultX, 0.00001); + CHECK(Placement::above == marks.at(0).positionData.placement); +} + +T_END; + +// issue #443: a source may hang an accidental mark on the ornament it decorates. The mark is +// kept and written at the notations level, which is the other place MusicXML allows, because the +// api does not record which of the two places a mark came from. +TEST(accidentalMarkOnAnOrnamentIsWrittenAtTheNotationsLevel, NoteData) +{ + const std::string xml = R"( + + + MusicXML Part + + + + + + + C + 4 + + 1 + 1 + quarter + + + + sharp + + + + + + +)"; + + const auto score = fromXml(xml); + const auto &marks = score.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.marks; + REQUIRE(2 == marks.size()); + CHECK(MarkType::trillMark == marks.at(0).markType); + CHECK(MarkType::accidentalMarkSharp == marks.at(1).markType); + + const auto written = toXml(score); + CHECK(written.find("