feat: round-trip octave-shift size - #365
Merged
Merged
Conversation
3 tasks
webern
force-pushed
the
claude/direction-fidelity-s6eab2
branch
2 times, most recently
from
July 19, 2026 18:20
4d6b8d8 to
e053287
Compare
2 tasks
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".
webern
force-pushed
the
claude/direction-fidelity-s6eab2
branch
from
July 19, 2026 20:19
e053287 to
97c22a6
Compare
webern
commented
Jul 19, 2026
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)
webern
added a commit
that referenced
this pull request
Jul 19, 2026
## 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.xml` to 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 existing `canonicalizeEncodingChildOrder` / `collapseEqualPageMargins`): - **Pedal defaults.** mx writes every pedal in an explicit form: a line pedal (the change/continue/discontinue/resume/sostenuto types, or start/stop with `line="yes"`) as `line="yes"`; a sign pedal (start/stop without `line="yes"`) as `line="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 grouping.** mx flattens a direction's `<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 (`sortAttributes` must run last). The genuinely-semantic gaps on ly31a -- the dropped `<offset>` and the force-written octave `size="8"` -- are fixed in the stacked library PRs, not normalized away. Pins `ly31a_Directions.xml` (the target) plus `ly33d_Spanners_OctaveShifts.xml` and `testDirections1.xml`, which the stacked octave-shift/offset fixes also flip to PASS. ## Testing - [x] api round-trip regression: 287 passed, 0 failed (of 287 pinned) -- the three new pins included - [x] api round-trip discovery: 287 PASS, 0 regressions - [x] Full unit suite unaffected (canonicalizers live only in the corpus round-trip harness) ## References - Closes #324 (the `symbol` item remains open under #294) - Third of a three-PR stack: octave-shift size (#365) → direction offset (#367) → round-trip canonicalizers + pins (this)
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
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 fromOttavaType, soemitOttavaStartforce-wrotesize="8"-- the spec default -- on every 8va/8vb line.OttavaStart::writeDefaultSize(a plain-bool principle-7 fidelity knob, in the mold ofClefData::writeStaffNumber) records whether the source spelled out that redundantsize="8"; the writer now omits it by default while always emitting the load-bearing, non-defaultsize="15". The reader sets the flag only for an 8va/8vb line that actually carried the attribute, so a file that did writesize="8"still round-trips it.The knob governs only the redundant default: a plain
bool(not the ternaryBool) because the only real degree of freedom is whether to showsize="8"on an 8-line. There is deliberately no way to suppress the load-bearingsize="15"on a 15-line -- that state would be corrupting, so it is made unrepresentable.Surfaced while hill-climbing
lysuite/ly31a_Directions.xmlto a full round-trip PASS.Testing
lysuite/ly33d_Spanners_OctaveShifts.xmlmoves toward PASS from the size fix. The pins land in the stacked round-trip PR (test: canonicalize non-semantic direction spellings and pin ly31a #366).References