feat: support SMuFL other marks in mx::api - #382
Merged
Conversation
5 tasks
webern
reviewed
Aug 4, 2026
Comment on lines
49
to
74
| @@ -73,6 +73,7 @@ enum class MarkType | |||
| pf, | |||
| sfzp, | |||
Owner
There was a problem hiding this comment.
Should all of this be replaced by standardDynamics so that we don't have duplication of the standard dynamic marks? Not sure, but the duplication of these into DynamicsData.h feels strange.
## Human Summary Preserve MusicXML other-notation and other mark SMuFL data through mx::api. Compound dynamics such as ffz retain their ordered dynamic components instead of losing everything after the first child. ## Summary Add typed API payloads for standard and other dynamic components, including ordered compound dynamics, and carry the smufl attribute for other-articulation, other-dynamics, other-ornament, and other-technical marks. Add a symmetric other-notation payload for its text, type, number, smufl, id, position, and print data. Keep single standard dynamics on the existing MarkData representation for source compatibility. Multiple children of one MusicXML dynamics element are owned by one compound mark, avoiding neighbor inference and preserving their order on write. Text and smufl remain independent for other dynamics because MusicXML permits a fallback text value alongside the glyph name. Also correct generic print-object writing so an explicit no value is emitted. Retire anonymous namespaces in the touched implementation files for strict unity-build compatibility. Nine synthetic other-mark fixtures now pass the API round-trip gate and are pinned in roundtrip-baseline.txt. ## Testing - make api-test MX_RUNNING_IN_DOCKER=1 (500 test cases, 5510 assertions) - make api-roundtrip MX_RUNNING_IN_DOCKER=1 (305 pinned files, 0 failed) - make fmt-check MX_RUNNING_IN_DOCKER=1 - strict unity build of target mx - git diff --check
## Human Summary
There were two enums for the same 26 dynamic markings: the dynamics block of `MarkType`, and
`StandardDynamic`, which the previous commit added for compound dynamics. They are now one.
`MarkType` keeps a single `dynamics` value and the symbol itself moves into `MarkDataChoice`, so
`<dynamics>` has one shape in the api instead of three. Writing a dynamic is unchanged in length --
`MarkData{StandardDynamic::ff}` -- and `ffz` now spells itself in `MarkData::name`, which it could
not do before.
## Summary
`StandardDynamic` won and the 29 dynamics values in `MarkType` were removed (`p` through `sfzp`,
`otherDynamics`, `compoundDynamics`, `unknownDynamics`), replaced by one `dynamics` value.
`StandardDynamic` was chosen despite being the newer type because it had not shipped -- it exists
only on this branch -- while removing `MarkType::ff` and friends would break every downstream
caller. The `Enclosure` unification in webern#378 is the precedent for accepting a breaking api change
where the model improves.
The symbol lives in `MarkDataChoice`, which gains `Kind::dynamic` holding a `StandardDynamic`
alongside the existing `Kind::compoundDynamics`. `MarkDataChoice(CompoundDynamicsData)` collapses
a lone standard component to `Kind::dynamic`, following `TimeChoice(ComplexTimeSignature)`. The
collapse is what makes two alternatives safe rather than confusing: `ff` has exactly one
representation however it was built, so code reading a plain dynamic never has to look inside a
compound for it. A lone other-dynamics does not collapse -- it has no dedicated MusicXML element,
so it stays a compound of one, which keeps `Kind::dynamic` meaning exactly "a standard
abbreviation" and lets `dynamic()` return a bare `StandardDynamic`.
Before this change one `<dynamics>` element had three api shapes: `markType == ff`; or
`markType == otherDynamics` with the text in `MarkData::name` and the glyph in
`choice.otherMark().smufl`; or `markType == compoundDynamics` with both facts restated inside
`DynamicsComponent`. All three collapse to one component list. `DynamicsReader` loses its
single-child special case and `DynamicsWriter` loses its entire non-compound branch.
`MarkData` gains `MarkData(StandardDynamic)` and `MarkData(CompoundDynamicsData)`, which set
`markType`, `choice`, and `name` together. Dynamics is the one mark family where a payload
determines its mark type uniquely -- `Kind::tremolo` covers `tremoloStart`/`tremoloStop`,
`Kind::arpeggiate` covers three mark types -- so it is the one family where a constructor can set
both without guessing. Every dynamics mark in the tree, including the reader, now goes through
these constructors; there are no direct `choice = CompoundDynamicsData{...}` assignments left.
`MarkData::name` keeps working. It spells the whole mark -- "ff", "z", or "ffz" -- derived by one
rule with no special cases, the way articulations and fermatas already name themselves. The writer
ignores it for dynamics, so it cannot contradict the output. A single standard component yields
what the old reader yielded, and a lone other-dynamics yields its text, so compatibility falls out
of the general rule rather than being special-cased. `name` is documented for the first time,
including that a self-naming mark derives it at construction and that replacing `choice` afterwards
means re-deriving it.
Mapping tables went from two to one, and the survivor is public. `markDataDynamicsKindToString`
and `dynamicsKindToName` were near-identical private Kind-to-string switches differing only in
their default arm; both are deleted, along with `Converter::dynamicsMap` and both `convertDynamic`
overloads. `standardDynamicsMap` remains as the only dynamics map. In their place
`DynamicsData.h` exports `toString(StandardDynamic)`, `toString(const DynamicsComponent &)` and
`toString(const CompoundDynamicsData &)`, so clients can spell a mark they built rather than
reaching for `name`. The core cannot supply these strings: it hard-codes the element literals
inline in the generated parse and serialize routines with no accessor, and `mx::impl` never sees
the XML -- by the time `DynamicsReader` runs, `<ff/>` is already `DynamicsChoice::Kind::ff`.
`isMarkDynamic` is now a single comparison and no longer has its duplicated `MarkType::p` test.
Dispatch written as `if (isMarkDynamic(...))` keeps working verbatim, so notations and directions
are untouched, as are `NoteAttachmentData::marks`, `DirectionChoice`, and every other mark family.
MusicXML output is unchanged; this is an api-shape change only.
## Breaking changes
- The 29 dynamics values of `MarkType` are gone. Use `MarkType::dynamics` with
`MarkData(StandardDynamic)` or `MarkData(CompoundDynamicsData)`. Every site is a compile error.
- `MarkData::name` is no longer the other-dynamics text on the write path; that text is
`OtherDynamicsData::text`. The reader still fills `name` in, so read-only clients are unaffected.
- Building a `CompoundDynamicsData` with a single standard component yields `Kind::dynamic`, not
`Kind::compoundDynamics`.
## Testing
- make test-all MX_RUNNING_IN_DOCKER=1 (837 + 41 + 502 test cases, 5541 assertions; 305 pinned
round-trip files, 0 failed)
- strict unity build of target mx (CMAKE_UNITY_BUILD_BATCH_SIZE=0)
- git diff --check
- make fmt MX_RUNNING_IN_DOCKER=1. Formatting was verified against the host clang-format only;
Docker was unavailable, so CI's clang-format has not seen these files. ApiEquality.h is
deliberately left as-is (host/Docker version drift, see 3a99c92).
rpatters1
force-pushed
the
smufl-for-other
branch
from
August 4, 2026 22:35
f94d3a7 to
52cec9b
Compare
Contributor
Author
|
Okay, I refactored so that dynamics types are removed from |
webern
approved these changes
Aug 5, 2026
webern
left a comment
webern
left a comment
Owner
There was a problem hiding this comment.
Nice. I think it's better with the deduplication even if it's a bigger breaking change. 👍
webern
pushed a commit
that referenced
this pull request
Aug 5, 2026
## Human Summary mx::api could only ever write an undecorated `<harmonic/>`. The natural/artificial distinction and the base/touching/sounding-pitch detail were dropped in both directions, so an artificial harmonic was indistinguishable from a natural one. This adds them as a MarkDataChoice payload. Also fixes the harmonic writer dropping print-object, font, and color. The wider print-object gap it uncovered is called out in the Summary and left for a separate change. ## Summary MusicXML's `<technical><harmonic>` states how a harmonic is produced (`<natural/>` or `<artificial/>`) and which of its three pitches the written notehead represents (`<base-pitch/>`, `<touching-pitch/>`, `<sounding-pitch/>`). `mx::core::Harmonic` models 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. Following the `TremoloMarkData` / `ArpeggiateMarkData` precedent, this adds a fourth `MarkDataChoice` alternative: - `HarmonicMarkData::kind` is a `HarmonicKind` of unspecified, natural, or artificial. - `HarmonicMarkData::pitch` is a `HarmonicPitch` of unspecified, basePitch, touchingPitch, or soundingPitch. Both default to unspecified, so authoring without the payload writes the plain `<harmonic/>` 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 with basePitch on the stopped note and artificial with 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, because `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 the `impl::setPrintObject` template in `PrintFunctions.h` that looks like it was meant to cover it is unused and has its condition inverted. Left for a separate change. `data/synthetic/harmonic.3.0.xml` already existed but had never passed; it is now pinned (296 -> 297). `lysuite/ly32a_Notations.xml` and `ksuite/k004a_Technical.xml` get past their harmonic content but still fail on unrelated `<lyric>` and `<alter>` ordering, so neither is pinned. ## Testing - [x] Three new cases in MarkRoundTripTest: the artificial pair (base pitch and touching pitch), all eleven kind/pitch combinations, and a bare `<harmonic/>` 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) ## References - Related to #382 and #383, which also add `MarkDataChoice` alternatives. Whichever of the three lands first, the others need a union resolution in `MarkDataChoice.h` and `MarkDataChoice.cpp`; the three payloads are independent.
rpatters1
added a commit
to rpatters1/denigma
that referenced
this pull request
Aug 6, 2026
mx replaced BarlineData::endingType and the single int endingNumber with one optional EndingData carrying the whole pass list and the printed label, so both halves of the Finale ending record can now travel. RepeatPassList is copied whole instead of contributing only its first value, and the Info message apologizing for the discarded passes is gone. An ending played on passes 1, 2, and 3 exports as number="1, 2, 3" rather than number="1". The label comes from RepeatEndingStart::createEndingText(), which returns custom RepeatEndingText verbatim when the author set one and otherwise builds the pass list, appending a period when the Add Period repeat option is on. It is assigned only when it differs from the label MusicXML would render from the number attribute on its own, which is the pass list joined the way core::EndingNumber::toString joins it. A document with Add Period on therefore keeps the periods Finale draws, and one without it writes no text node at all. The closing barline repeats the numbers so both barlines identify the same bracket, but never the text, because MusicXML draws a label at every <ending> that carries one. The gap entry for ending display text and multiple numbers is closed. What remains unmappable is the ending's appearance, so the entry is replaced by one covering print-object, end-length, text-x, text-y, system, and the print-style group, each named with the Finale field that would feed it. Hidden endings are the notable loss, since they are structural in Finale rather than decorative. Pins mx to f52c88e, the head of rpatters1/mx@ending-numbers-and-text and open upstream as webern/mx#388. It supersedes 2671b8e, whose dynamics and harmonic work it carries in merged form as webern/mx#382 and #384. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rpatters1
added a commit
to rpatters1/denigma
that referenced
this pull request
Aug 6, 2026
mx replaced BarlineData::endingType and the single int endingNumber with one optional EndingData carrying the whole pass list and the printed label, so both halves of the Finale ending record can now travel. RepeatPassList is copied whole instead of contributing only its first value, and the Info message apologizing for the discarded passes is gone. An ending played on passes 1, 2, and 3 exports as number="1, 2, 3" rather than number="1". The label comes from RepeatEndingStart::createEndingText(), which returns custom RepeatEndingText verbatim when the author set one and otherwise builds the pass list, appending a period when the Add Period repeat option is on. It is assigned only when it differs from the label MusicXML would render from the number attribute on its own, which is the pass list joined the way core::EndingNumber::toString joins it. A document with Add Period on therefore keeps the periods Finale draws, and one without it writes no text node at all. The closing barline repeats the numbers so both barlines identify the same bracket, but never the text, because MusicXML draws a label at every <ending> that carries one. The gap entry for ending display text and multiple numbers is closed. What remains unmappable is the ending's appearance, so the entry is replaced by one covering print-object, end-length, text-x, text-y, system, and the print-style group, each named with the Finale field that would feed it. Hidden endings are the notable loss, since they are structural in Finale rather than decorative. Pins mx to f52c88e, the head of rpatters1/mx@ending-numbers-and-text and open upstream as webern/mx#388. It supersedes 2671b8e, whose dynamics and harmonic work it carries in merged form as webern/mx#382 and #384. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
Preserve SMuFL glyph names for MusicXML
other-*marks and expose generalother-notationthroughmx::api. Compound dynamics such asffznow retain all ordered components instead of losing everything after the first child.Summary
Add mark-specific API payloads for
other-articulation,other-dynamics,other-ornament, andother-technical, keeping fallback text and the optionalsmuflglyph name independent.Represent one MusicXML
<dynamics>element as an ordered compound mark when it contains multiple children. Single standard dynamics retain the existingMarkDatarepresentation for source compatibility.Add symmetric
other-notationsupport for text, type, number,smufl, id, position, and print data. The change also fixes explicitprint-object="no"emission and retires anonymous namespaces in touched files for unity-build compatibility.Nine existing synthetic
other-*fixtures now pass and are pinned in the API round-trip baseline.Testing
make api-test MX_RUNNING_IN_DOCKER=1(500 test cases, 5510 assertions)make api-roundtrip MX_RUNNING_IN_DOCKER=1(305 pinned files, 0 failed)make fmt-check MX_RUNNING_IN_DOCKER=1mxgit diff --checkReferences