Skip to content

feat: model direction offset as an anchor plus a fidelity knob - #367

Merged
webern merged 3 commits into
mainfrom
claude/direction-offset-s6eab2
Jul 19, 2026
Merged

feat: model direction offset as an anchor plus a fidelity knob#367
webern merged 3 commits into
mainfrom
claude/direction-offset-s6eab2

Conversation

@webern

@webern webern commented Jul 19, 2026

Copy link
Copy Markdown
Owner

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

  • Full unit suite passes (5130 assertions in 451 test cases), including the reworked OutOfOrder* direction round-trip tests
  • 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 (test: canonicalize non-semantic direction spellings and pin ly31a #366).

References

@webern webern added feature new feature request breaking fixes or implementation that require breaking changes api Affects the mx::api layer impl Affects the mx::impl layer ai Issues opened by, or through, a coding agent. labels Jul 19, 2026 — with Claude
webern added 2 commits July 19, 2026 20:10
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).
@webern
webern force-pushed the claude/direction-fidelity-s6eab2 branch from e053287 to 97c22a6 Compare July 19, 2026 20:19
@webern
webern force-pushed the claude/direction-offset-s6eab2 branch from a53cd06 to 8f962c6 Compare July 19, 2026 20:19
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) &rarr; direction
offset (#367) &rarr; round-trip canonicalizers + pins (#366)
Base automatically changed from claude/direction-fidelity-s6eab2 to main July 19, 2026 20:48
@webern
webern merged commit ab7946b into main Jul 19, 2026
7 checks passed
@webern
webern deleted the claude/direction-offset-s6eab2 branch July 19, 2026 20:55
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) &rarr; direction
offset (#367) &rarr; round-trip canonicalizers + pins (this)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai Issues opened by, or through, a coding agent. api Affects the mx::api layer breaking fixes or implementation that require breaking changes feature new feature request impl Affects the mx::impl layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant