From 2671b8e2ae6931e58f87e2354f71d324e2f47643 Mon Sep 17 00:00:00 2001 From: Robert Patterson Date: Mon, 3 Aug 2026 09:55:07 -0500 Subject: [PATCH] feat: artificial and natural harmonic detail in mx::api MusicXML's `` states how a harmonic is produced (`` or ``) and which of its three pitches the written notehead represents (``, ``, ``). `mx::core::Harmonic` modeled both as `choice` and `choice2`, but `mx::api` dropped them in both directions: `NotationsWriter::addTechnical` built a bare `core::Harmonic` and never called `setChoice`/`setChoice2`, and `TechnicalFunctions::parseTechicalMark` read only the attributes. Only an undecorated `` could be written, so an artificial harmonic was indistinguishable from a natural one. Following the `TremoloMarkData` / `ArpeggiateMarkData` precedent, this adds a fourth `MarkDataChoice` alternative: - `HarmonicMarkData::kind` (`HarmonicKind::unspecified/natural/artificial`) - `HarmonicMarkData::pitch` (`HarmonicPitch::unspecified/basePitch/ touchingPitch/soundingPitch`) Both default to `unspecified`, so authoring without the payload writes the plain `` exactly as before, and the reader leaves `choice` as `none` for a bare element rather than inventing a natural/artificial value. The two notes of an artificial harmonic each carry their own mark -- `artificial` + `basePitch` on the stopped note, `artificial` + `touchingPitch` on the diamond-notehead note -- so neither note depends on its neighbor for meaning. Also fixes a write-side gap found while adding it: the harmonic writer used `setAttributesFromPositionData`, so the element's font, color, and `print-object` were all lost on write. It now uses `setAttributesFromMarkData` and sets `print-object` explicitly, since `setAttributesFromPrintData` writes color and font but never touches `printObject`. That omission still affects every other mark going through `setAttributesFromMarkData` (fermata, dynamics, arpeggiate, non-arpeggiate) and is left for a separate change. `data/synthetic/harmonic.3.0.xml` already existed but had never passed; it is now pinned in `roundtrip-baseline.txt` (296 -> 297). `lysuite/ly32a_Notations.xml` and `ksuite/k004a_Technical.xml` get past their harmonic content but still fail on unrelated `` and `` ordering, so neither is pinned. - [x] Three new cases in MarkRoundTripTest: the artificial pair (base pitch and touching pitch), all eleven kind/pitch combinations, and a bare `` that must read back with no payload - [x] Harmonic tests pass (`*Harmonic*`: 43 assertions in 3 test cases) - [x] Full api suite passes (5512 assertions in 499 test cases) - [x] `make api-roundtrip` passes (297 of 297 pinned) - [x] `make core-roundtrip-test` passes (837 test cases) --- src/include/mx/api/MarkDataChoice.h | 71 +++++++++++++++++- src/private/mx/api/MarkDataChoice.cpp | 22 ++++++ src/private/mx/impl/NotationsWriter.cpp | 33 ++++++++- src/private/mx/impl/TechnicalFunctions.cpp | 48 +++++++++++- src/private/mxtest/api/MarkRoundTripTest.cpp | 73 +++++++++++++++++++ src/private/mxtest/api/roundtrip-baseline.txt | 6 ++ 6 files changed, 249 insertions(+), 4 deletions(-) diff --git a/src/include/mx/api/MarkDataChoice.h b/src/include/mx/api/MarkDataChoice.h index 800c6de77..4eff58120 100644 --- a/src/include/mx/api/MarkDataChoice.h +++ b/src/include/mx/api/MarkDataChoice.h @@ -137,6 +137,63 @@ MXAPI_EQUALS_MEMBER(id) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(OtherNotationMarkData); +// How a harmonic is produced on a string instrument. +enum class HarmonicKind +{ + // Leave the production method unstated, drawing only the circular harmonic symbol. + unspecified, + + // The string sounds open and is touched lightly at a node, so the harmonic's pitch is fixed by + // where the node falls on the open string. + natural, + + // The string is stopped at one pitch and touched lightly a fixed interval above it -- usually a + // fourth, sometimes a major third or a fifth. The stopped note is written as an ordinary + // notehead and the touched note as a diamond (see NoteData::notehead). + artificial, +}; + +// Which of a harmonic's three pitches the note's written notehead states. +enum class HarmonicPitch +{ + // Leave it unstated. Notation that writes only one note for the harmonic, or that relies on + // notehead shape alone to say which pitch is meant, does not need this. + unspecified, + + // The stopped pitch: where the finger presses the string. The lower note of an artificial + // harmonic pair. + basePitch, + + // The lightly touched pitch. The upper, diamond-notehead note of an artificial harmonic pair, + // and the node touched on an open string for a natural harmonic. + touchingPitch, + + // The pitch that actually sounds. Written when the notation states the sounding result + // explicitly, often as a small or parenthesized note above the pair. + soundingPitch, +}; + +// Payload for MarkType::harmonic: the symbol placed on a string-instrument note. +// +// A note in an artificial harmonic pair carries HarmonicKind::artificial along with the +// HarmonicPitch its notehead states -- basePitch on the stopped note, touchingPitch on the diamond +// note -- so each note says what it is on its own. Leaving both fields unspecified draws the plain +// circular harmonic symbol, which is what a natural harmonic most often needs. +struct HarmonicMarkData +{ + // Whether the harmonic is natural or artificial. + HarmonicKind kind = HarmonicKind::unspecified; + + // Which pitch this note's notehead states. + HarmonicPitch pitch = HarmonicPitch::unspecified; +}; + +MXAPI_EQUALS_BEGIN(HarmonicMarkData) +MXAPI_EQUALS_MEMBER(kind) +MXAPI_EQUALS_MEMBER(pitch) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(HarmonicMarkData); + // A variant class that carries data for MarkType values whose payload does not fit MarkData's // common fields. // @@ -163,7 +220,8 @@ class MarkDataChoice otherMark, dynamic, compoundDynamics, - otherNotation + otherNotation, + harmonic }; MarkDataChoice(); @@ -185,6 +243,8 @@ class MarkDataChoice MarkDataChoice(OtherNotationMarkData value); + MarkDataChoice(HarmonicMarkData value); + Kind kind() const; bool isNone() const; bool isTremolo() const; @@ -194,6 +254,7 @@ class MarkDataChoice bool isDynamic() const; bool isCompoundDynamics() const; bool isOtherNotation() const; + bool isHarmonic() const; // Returns a copy of the internally held TremoloMarkData. // @@ -225,11 +286,17 @@ class MarkDataChoice // Returns a copy of the internally held OtherNotationMarkData, or a default value for another kind. const OtherNotationMarkData otherNotation() const; + // Returns a copy of the internally held HarmonicMarkData. + // + // Check isHarmonic() first. If this is not a harmonic payload, a default constructed + // HarmonicMarkData is returned. + const HarmonicMarkData harmonic() const; + bool operator==(const MarkDataChoice &other) const; private: std::variant + StandardDynamic, CompoundDynamicsData, OtherNotationMarkData, HarmonicMarkData> myValue; }; diff --git a/src/private/mx/api/MarkDataChoice.cpp b/src/private/mx/api/MarkDataChoice.cpp index 3303459a8..cadb16412 100644 --- a/src/private/mx/api/MarkDataChoice.cpp +++ b/src/private/mx/api/MarkDataChoice.cpp @@ -54,6 +54,10 @@ MarkDataChoice::MarkDataChoice(OtherNotationMarkData value) : myValue{std::move( { } +MarkDataChoice::MarkDataChoice(HarmonicMarkData value) : myValue{std::move(value)} +{ +} + MarkDataChoice::Kind MarkDataChoice::kind() const { if (std::holds_alternative(myValue)) @@ -84,6 +88,10 @@ MarkDataChoice::Kind MarkDataChoice::kind() const { return Kind::otherNotation; } + if (std::holds_alternative(myValue)) + { + return Kind::harmonic; + } return Kind::none; } @@ -190,6 +198,20 @@ const OtherNotationMarkData MarkDataChoice::otherNotation() const return OtherNotationMarkData{}; } +bool MarkDataChoice::isHarmonic() const +{ + return std::holds_alternative(myValue); +} + +const HarmonicMarkData MarkDataChoice::harmonic() const +{ + if (const auto *value = std::get_if(&myValue)) + { + return *value; + } + return HarmonicMarkData{}; +} + bool MarkDataChoice::operator==(const MarkDataChoice &other) const { return myValue == other.myValue; diff --git a/src/private/mx/impl/NotationsWriter.cpp b/src/private/mx/impl/NotationsWriter.cpp index 9e80f3ff3..6973c3b25 100644 --- a/src/private/mx/impl/NotationsWriter.cpp +++ b/src/private/mx/impl/NotationsWriter.cpp @@ -24,6 +24,8 @@ #include "mx/core/generated/HandbellValue.h" #include "mx/core/generated/HarmonMute.h" #include "mx/core/generated/Harmonic.h" +#include "mx/core/generated/HarmonicChoice.h" +#include "mx/core/generated/HarmonicChoice2.h" #include "mx/core/generated/HeelToe.h" #include "mx/core/generated/Hole.h" #include "mx/core/generated/HoleClosed.h" @@ -802,7 +804,36 @@ void NotationsWriter::addTechnical(const api::MarkData &mark, core::Technical &o } case core::TechnicalChoice::Kind::harmonic: { core::Harmonic h; - setAttributesFromPositionData(mark.positionData, h); + impl::setAttributesFromMarkData(mark, h); + const auto harmonicData = mark.choice.isHarmonic() ? mark.choice.harmonic() : api::HarmonicMarkData{}; + + switch (harmonicData.kind) + { + case api::HarmonicKind::natural: + h.setChoice(core::HarmonicChoice::natural(core::Empty{})); + break; + case api::HarmonicKind::artificial: + h.setChoice(core::HarmonicChoice::artificial(core::Empty{})); + break; + case api::HarmonicKind::unspecified: + break; + } + + switch (harmonicData.pitch) + { + case api::HarmonicPitch::basePitch: + h.setChoice2(core::HarmonicChoice2::basePitch(core::Empty{})); + break; + case api::HarmonicPitch::touchingPitch: + h.setChoice2(core::HarmonicChoice2::touchingPitch(core::Empty{})); + break; + case api::HarmonicPitch::soundingPitch: + h.setChoice2(core::HarmonicChoice2::soundingPitch(core::Empty{})); + break; + case api::HarmonicPitch::unspecified: + break; + } + outTechnical.addChoice(core::TechnicalChoice::harmonic(h)); break; } diff --git a/src/private/mx/impl/TechnicalFunctions.cpp b/src/private/mx/impl/TechnicalFunctions.cpp index fa6abfc8d..b557fe524 100644 --- a/src/private/mx/impl/TechnicalFunctions.cpp +++ b/src/private/mx/impl/TechnicalFunctions.cpp @@ -10,6 +10,9 @@ #include "mx/core/generated/Fingering.h" #include "mx/core/generated/Handbell.h" #include "mx/core/generated/HandbellValue.h" +#include "mx/core/generated/Harmonic.h" +#include "mx/core/generated/HarmonicChoice.h" +#include "mx/core/generated/HarmonicChoice2.h" #include "mx/core/generated/Hole.h" #include "mx/core/generated/HoleClosed.h" #include "mx/core/generated/HoleClosedValue.h" @@ -103,6 +106,42 @@ std::string technicalFunctionsHandbellToSmuflName(const mx::core::HandbellValue } } +api::HarmonicMarkData harmonicMarkDataFromCore(const core::Harmonic &inHarmonic) +{ + api::HarmonicMarkData outData{}; + + if (inHarmonic.choice().has_value()) + { + switch (inHarmonic.choice()->kind()) + { + case core::HarmonicChoice::Kind::natural: + outData.kind = api::HarmonicKind::natural; + break; + case core::HarmonicChoice::Kind::artificial: + outData.kind = api::HarmonicKind::artificial; + break; + } + } + + if (inHarmonic.choice2().has_value()) + { + switch (inHarmonic.choice2()->kind()) + { + case core::HarmonicChoice2::Kind::basePitch: + outData.pitch = api::HarmonicPitch::basePitch; + break; + case core::HarmonicChoice2::Kind::touchingPitch: + outData.pitch = api::HarmonicPitch::touchingPitch; + break; + case core::HarmonicChoice2::Kind::soundingPitch: + outData.pitch = api::HarmonicPitch::soundingPitch; + break; + } + } + + return outData; +} + TechnicalFunctions::TechnicalFunctions(std::span inTechincalChoiceSet, Cursor inCursor) : myTechincalChoiceSet{inTechincalChoiceSet}, myCursor{inCursor} { @@ -148,8 +187,15 @@ bool TechnicalFunctions::parseTechicalMark(const core::TechnicalChoice &techical return true; } case core::TechnicalChoice::Kind::harmonic: { - parseMarkDataAttributes(techicalChoice.asHarmonic(), outMarkData); + const auto &harmonic = techicalChoice.asHarmonic(); + parseMarkDataAttributes(harmonic, outMarkData); outMarkData.name = "harmonic"; + const auto harmonicData = harmonicMarkDataFromCore(harmonic); + if (harmonicData.kind != api::HarmonicKind::unspecified || + harmonicData.pitch != api::HarmonicPitch::unspecified) + { + outMarkData.choice = api::MarkDataChoice{harmonicData}; + } return true; } case core::TechnicalChoice::Kind::openString: { diff --git a/src/private/mxtest/api/MarkRoundTripTest.cpp b/src/private/mxtest/api/MarkRoundTripTest.cpp index 8f43f465e..361a83358 100644 --- a/src/private/mxtest/api/MarkRoundTripTest.cpp +++ b/src/private/mxtest/api/MarkRoundTripTest.cpp @@ -285,6 +285,79 @@ TEST(NonArpeggiateAttributes, MarkRoundTrip) T_END; +TEST(HarmonicArtificialPair, MarkRoundTrip) +{ + // The two notes of an artificial harmonic each state what they are: the stopped note carries + // the base pitch, the diamond-notehead note the touching pitch. + MarkData stopped{Placement::unspecified, MarkType::harmonic}; + HarmonicMarkData stoppedPayload{}; + stoppedPayload.kind = HarmonicKind::artificial; + stoppedPayload.pitch = HarmonicPitch::basePitch; + stopped.choice = stoppedPayload; + + const auto stoppedMarks = roundTripMarkData(stopped); + REQUIRE(stoppedMarks.size() == 1); + CHECK(stoppedMarks.front().markType == MarkType::harmonic); + REQUIRE(stoppedMarks.front().choice.isHarmonic()); + CHECK(stoppedPayload == stoppedMarks.front().choice.harmonic()); + + MarkData touched{Placement::unspecified, MarkType::harmonic}; + HarmonicMarkData touchedPayload{}; + touchedPayload.kind = HarmonicKind::artificial; + touchedPayload.pitch = HarmonicPitch::touchingPitch; + touched.choice = touchedPayload; + + const auto touchedMarks = roundTripMarkData(touched); + REQUIRE(touchedMarks.size() == 1); + REQUIRE(touchedMarks.front().choice.isHarmonic()); + CHECK(touchedPayload == touchedMarks.front().choice.harmonic()); +} + +T_END; + +TEST(HarmonicKindAndPitchCombinations, MarkRoundTrip) +{ + const HarmonicKind kinds[] = {HarmonicKind::unspecified, HarmonicKind::natural, HarmonicKind::artificial}; + const HarmonicPitch pitches[] = {HarmonicPitch::unspecified, HarmonicPitch::basePitch, HarmonicPitch::touchingPitch, + HarmonicPitch::soundingPitch}; + + for (const auto kind : kinds) + { + for (const auto pitch : pitches) + { + if (kind == HarmonicKind::unspecified && pitch == HarmonicPitch::unspecified) + { + continue; // covered by HarmonicDefaultsToNoPayload below + } + + MarkData mark{Placement::unspecified, MarkType::harmonic}; + HarmonicMarkData payload{}; + payload.kind = kind; + payload.pitch = pitch; + mark.choice = payload; + + const auto marks = roundTripMarkData(mark); + REQUIRE(marks.size() == 1); + REQUIRE(marks.front().choice.isHarmonic()); + CHECK(payload == marks.front().choice.harmonic()); + } + } +} + +T_END; + +TEST(HarmonicDefaultsToNoPayload, MarkRoundTrip) +{ + // An undecorated must not acquire a natural/artificial or pitch value, and must + // still round-trip for callers written before the payload existed. + const auto marks = roundTripMark(MarkType::harmonic); + REQUIRE(marks.size() == 1); + CHECK(marks.front().markType == MarkType::harmonic); + CHECK(marks.front().choice.isNone()); +} + +T_END; + TEST(CaesuraEmpty, MarkRoundTrip) { // The common empty form must not acquire a "normal" text value on write. diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index f4268889c..7f811545b 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -627,3 +627,9 @@ lysuite/ly33i_Ties_NotEnded.xml lysuite/ly61d_Lyrics_Melisma.xml lysuite/ly61f_Lyrics_GracedNotes.xml synthetic/tied.cue.4.0.xml + +# Artificial/natural harmonic detail: HarmonicMarkData in MarkDataChoice carries the +# element's (natural|artificial) and (base-pitch|touching-pitch|sounding-pitch) +# children, which both directions previously dropped. The writer also now emits the +# harmonic's print-object attribute. +synthetic/harmonic.3.0.xml