test: canonicalize non-semantic direction spellings and pin ly31a - #366
Merged
Conversation
webern
force-pushed
the
claude/direction-fidelity-s6eab2
branch
from
July 19, 2026 17:41
8c83d0c to
4d6b8d8
Compare
webern
force-pushed
the
claude/ly31a-roundtrip-s6eab2
branch
from
July 19, 2026 17:41
7caa3b2 to
d1ec82e
Compare
webern
force-pushed
the
claude/direction-fidelity-s6eab2
branch
from
July 19, 2026 18:20
4d6b8d8 to
e053287
Compare
webern
force-pushed
the
claude/ly31a-roundtrip-s6eab2
branch
from
July 19, 2026 18:20
d1ec82e to
bbfc460
Compare
This was referenced Jul 19, 2026
webern
changed the base branch from
claude/direction-fidelity-s6eab2
to
claude/direction-offset-s6eab2
July 19, 2026 18:21
octave-shift's size (8 or 15) follows from OttavaType, so the writer force-wrote size="8" -- the spec default -- on every 8va/8vb line. OttavaStart::writeDefaultSize records whether the source spelled out that redundant size="8", so the writer now omits it by default while always emitting the load-bearing, non-default size="15".
A direction's <offset> was folded into tickTimePosition and then dropped on write, so an offset that landed on a note tick was indistinguishable from a plain direction there, and the explicit <offset> never round-tripped. Model the two quantities separately. tickTimePosition is now the direction's anchor -- the musical location it belongs to -- and DirectionData::offset is the source's <offset>, a drawn-position nudge, kept verbatim or left absent. The drawn position is their sum. The reader stores the anchor and the offset without folding; the writer emits a modeled offset with no sound attribute (it moves only the drawing) and still synthesizes a sound="yes" offset for a direction that merely sits off the cursor tick. Authors set tickTimePosition to where the direction belongs and ignore offset. BREAKING: tickTimePosition changes meaning for offset-bearing directions, from the drawn location to the anchor. The OutOfOrder tests now assert the effective location (tickTimePosition + offset).
…trip The api serializes pedals and grouped direction-types in a fixed explicit form that is notation-equivalent to, but textually different from, the shorthand many sources use. Canonicalize both on the in-memory documents just before comparison (the corpus files are never edited): expand bare pedals to mx's line="no" sign="yes" / line="yes" form, and merge consecutive words-only <direction-type> siblings. Pin ly31a_Directions (the hill-climb target) plus ly33d_Spanners_OctaveShifts and testDirections1, which the octave-shift/offset fidelity fixes also flip to PASS.
webern
force-pushed
the
claude/direction-offset-s6eab2
branch
from
July 19, 2026 20:19
a53cd06 to
8f962c6
Compare
webern
force-pushed
the
claude/ly31a-roundtrip-s6eab2
branch
from
July 19, 2026 20:19
bbfc460 to
32b005a
Compare
webern
added a commit
that referenced
this pull request
Jul 19, 2026
## Human Summary Apparently, emitting "8" for a single octave shift is optional. Here we add a round-trip fidelity knob that can actually be ignored when using the API. ## Summary octave-shift's `size` (8 or 15) is derived from `OttavaType`, so `emitOttavaStart` force-wrote `size="8"` -- the spec default -- on every 8va/8vb line. `OttavaStart::writeDefaultSize` (a plain-bool principle-7 fidelity knob, in the mold of `ClefData::writeStaffNumber`) records whether the source spelled out that redundant `size="8"`; the writer now omits it by default while always emitting the load-bearing, non-default `size="15"`. The reader sets the flag only for an 8va/8vb line that actually carried the attribute, so a file that did write `size="8"` still round-trips it. The knob governs only the redundant default: a plain `bool` (not the ternary `Bool`) because the only real degree of freedom is whether to show `size="8"` on an 8-line. There is deliberately no way to suppress the load-bearing `size="15"` on a 15-line -- that state would be corrupting, so it is made unrepresentable. Surfaced while hill-climbing `lysuite/ly31a_Directions.xml` to a full round-trip PASS. ## Testing - [x] Full unit suite passes (5130 assertions in 451 test cases) - [x] api round-trip discovery: no regressions; `lysuite/ly33d_Spanners_OctaveShifts.xml` moves toward PASS from the size fix. The pins land in the stacked round-trip PR (#366). ## References - Progresses #324 - First of a three-PR stack: octave-shift size (this) → direction offset (#367) → round-trip canonicalizers + pins (#366)
webern
added a commit
that referenced
this pull request
Jul 19, 2026
## Human Summary This one is sort of complicated. But if I understand it correctly, the `<offset>` element places a direction somewhere other than where it happens to actually be in the stateful parse position. I believe it is unnecessary, perhaps, in MusicXML. Or perhaps it allows you to place a direction in the measure close to a note that the application attaches the direction to, then say where it actually appears by nudging its time position. It is weird. But this optional offset allows us to represent certain corpus files correctly and, as such, probably is actually needed for correctness in some cases. ## Summary Reworks how a direction's `<offset>` is modeled so that the anchor a direction belongs to and the drawn-position nudge are two separate, first-class quantities. The goal is that an author positions a direction by the note it belongs to and never has to reach for the arcane `<offset>` element. **Before.** The reader folded `<offset>` into `tickTimePosition` and discarded the raw value. `tickTimePosition` therefore meant the *drawn* location, and on write the offset was re-synthesized as `tickTimePosition - cursor`. An offset that landed exactly on a note tick collapsed to zero, so the `<offset>` silently vanished and the direction re-anchored to the wrong note. The model also could not tell a visual-only offset apart from one meant to move playback. **After.** - `tickTimePosition` is the **anchor** -- the musical location (note/beat) the direction belongs to. - `DirectionData::offset` (new, `std::optional<int>`) is the source's `<offset>`, a drawn-position shift in divisions, kept verbatim or left absent. The drawn position is `tickTimePosition + offset`. - The reader stores the anchor and the offset without folding. The writer is field-first: a modeled offset is re-emitted with **no** `sound` attribute (an `<offset>` moves only where the direction is drawn, so playback stays at the anchor); a direction that merely sits off the cursor tick with no modeled offset still gets a synthesized `sound="yes"` offset, exactly as before. - `MeasureWriter` is untouched -- it already places directions by `tickTimePosition`, which is now the anchor. Authoring stays simple: set `tickTimePosition` to where the direction belongs and leave `offset` absent; the automatic path handles placement. `offset` is a round-trip fidelity knob you only touch to preserve a source's explicit `<offset>`. ## Breaking change `tickTimePosition` changes meaning for **offset-bearing** directions: it is now the anchor, not the drawn location. Directions without an offset are unaffected (anchor == drawn location). A consumer that reads `tickTimePosition` for an offset direction and wants the drawn location must now add `offset`. The `OutOfOrderDoesntThrow` / `OutOfOrderTorture` round-trip tests now assert the effective (drawn) location `tickTimePosition + offset`, which equals what they previously asserted against the folded tick. ## Testing - [x] Full unit suite passes (5130 assertions in 451 test cases), including the reworked `OutOfOrder*` direction round-trip tests - [x] api round-trip discovery: 287 PASS, 0 regressions; the `<offset>`-bearing corpus files (`ly31a_Directions`, `ly33d_Spanners_OctaveShifts`) round-trip their bare `<offset>` elements byte-for-byte. The pins land in the stacked round-trip PR (#366). ## References - Progresses #324 - Second of a three-PR stack: octave-shift size (#365) → direction offset (this) → round-trip canonicalizers + pins (#366)
Owner
Author
|
/coverage |
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36658 |
| Functions | 74.2% | 6352 / 8561 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 86.5% | 8261 / 9551 |
| Functions | 78.7% | 3247 / 4124 |
| Branches | 53.6% | 7252 / 13533 |
Core HTML report | API HTML report
Commit 0271454c3898c2bcb35ca19de338fb7ef21f990b.
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
1 participant
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
There are certain cases where the same thing can be represented in multiple ways in MusicXML. In this case, with pedal defaults and words groupings, we doctor the test to expect what mx will write which, I believe, is semantically equivalent to what was in the test file.
Summary
Completes the hill-climb of
lysuite/ly31a_Directions.xmlto a full api round-trip PASS. The remaining differences on that file are non-semantic spelling choices, not fidelity losses, so they are reconciled by canonicalizing the in-memory documents just before comparison -- the corpus input files are never edited.Two canonicalizers, applied to both the expected and actual DOM in
CorpusRoundtripMain.cpp(same pattern as the existingcanonicalizeEncodingChildOrder/collapseEqualPageMargins):line="yes") asline="yes"; a sign pedal (start/stop withoutline="yes") asline="no" sign="yes". Sources often use the bare shorthand (<pedal type="start"/>) that leans on those same spec defaults. Each<pedal>is rewritten to the explicit form; because the target is derived from each element's own type/line, a genuine line-vs-sign disagreement still fails.<words>into one<direction-type>, whereas a source may split consecutive<words>across separate<direction-type>siblings. With each<words>stating its own formatting the boxing is not notation-semantic (the spec's font carry-over only bites when attributes are omitted to be inherited), so consecutive words-only<direction-type>siblings are merged.Adding pedal attributes can leave them out of order, so attributes are re-sorted afterward (
sortAttributesmust run last).The genuinely-semantic gaps on ly31a -- the dropped
<offset>and the force-written octavesize="8"-- are fixed in the stacked library PRs, not normalized away.Pins
ly31a_Directions.xml(the target) plusly33d_Spanners_OctaveShifts.xmlandtestDirections1.xml, which the stacked octave-shift/offset fixes also flip to PASS.Testing
References
symbolitem remains open under Proposal: Support Interleaved Words and Symbols inmx::apiDirections #294)