feat: unified spanner numbering with writer-side assignment - #320
Merged
Conversation
webern
force-pushed
the
claude/friendly-cori-pg1yfm
branch
from
July 7, 2026 09:38
41c2446 to
e6bc65d
Compare
Coverage reportAPI coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| (api coverage not produced) |
Core HTML report | API HTML report
Commit 3a8a3b659389c145aa9f8a9eede95b634486ef65.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36640 |
| Functions | 74.3% | 6352 / 8551 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.2% | 6632 / 8167 |
| Functions | 68.0% | 2238 / 3293 |
| Branches | 49.7% | 5719 / 11501 |
Core HTML report | API HTML report
Commit 53dd840359525db5d5eb01585b60fcb714928397.
gen-quality
|
Replace the raw int numberLevel on the six spanner start/continue/stop structs (CurveStart/Continue/Stop, WedgeStart/Stop, SpannerStart/Stop) with a single SpannerNumber value type in ApiCommon.h. Exactly one of three states -- unspecified, explicit level 1..16 (emitted verbatim), or an author-only identity label (never serialized) -- so a contradictory object is unrepresentable by construction (design principle #3). The writer now owns number assignment for identity spanners: a new SpannerNumberResolver walks each part in true serialization order (measures -> staves -> voices ascending -> notes, directions in vector order, mirroring MeasureWriter/NotationsWriter/DirectionWriter) and assigns the lowest free number from a 1..16 pool per staff and per spanner class (slur, tied, wedge, octave-shift, bracket, dashes). Explicit levels reserve their number for their serialized extent so explicit and identity spanners never collide. Pool exhaustion throws rather than emitting an illegal number. Wedge and octave-shift writers now emit the resolved number (they previously dropped numbers entirely). Readers produce only unspecified or explicit; identity is authoring-only. Adds the slurs_overbars fixture from #297 (two slurs disjoint in musical time that overlap in the serialized stream once voice 1 is written ahead of voice 2), pinned in roundtrip-baseline.txt; corert count 833 -> 834. This is a deliberate breaking change to mx::api.
The rebase onto main picked up the new api::OttavaStop type (a SpannerStop plus an optional size, from the octave-shift stop-size change) everywhere except SpannerNumberResolver.cpp, which still read item.number on ottava stops and did not compile. Register ottava stops by their nested spannerStop (matching the address DirectionWriter uses for lookup) and re-run clang-format over the conflict-resolved files.
webern
force-pushed
the
claude/friendly-cori-pg1yfm
branch
from
July 11, 2026 14:07
bbd0f06 to
e36ed5e
Compare
Coverage reportAPI coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| (api coverage not produced) |
Core HTML report | API HTML report
Commit 7fda4e0f0d0b557e63686f6927c7546426fab4e9.
gen-quality
|
The rebase onto main also picked up #321's TimeChoice split of MeasureData::timeSignature into a variant of TimeSignatureData/ ComplexTimeSignature, but SpannerIdentityTest.cpp (added by this PR) still set the old direct beats/beatType members, which no longer exist and failed to compile everywhere. Separately, main had already grown its own corert-pinned file count to 834 (a new rpatters1 fixture from #321) before this PR's own slurs_overbars fixture was rebased on top, so the true total is 835, not the 834 the previous repair commit left in place. Ran `make audit` to regenerate the missing feature sidecar and corpus.xml, and bumped the pinned count accordingly. Verified locally: make test, test-core-dev, test-api-roundtrip, test-cpp-unit, validate-cpp, probe-cpp, check, check-core-dev, and a unity build all pass.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.4% | 6765 / 8315 |
| Functions | 68.2% | 2266 / 3323 |
| Branches | 49.8% | 5841 / 11722 |
Core HTML report | API HTML report
Commit 118e3d5fbf7ac9aabb8000ecba632f2a61135367.
gen-quality
|
webern
commented
Jul 12, 2026
Move SpannerNumber out of ApiCommon.h into its own SpannerNumber.h/.cpp pair (declaration + implementation, matching the TimeChoice.h/.cpp split). Replace the makeLevel/makeIdentity factory functions with constructors. Drop exception throwing entirely, following the TimeChoice pattern: an out-of-range level or empty id collapses to unspecified at construction, and calling level()/identity() for the wrong kind returns a harmless default (NUMBER_LEVEL_UNSPECIFIED, or an empty string) instead of throwing. Move the slurs_overbars fixture from data/custom into data/rpatters1 now that its provenance is confirmed: rpatters1 attached the original file to issue #297, so it belongs alongside his other contributed fixture rather than in data/custom (reserved for LLM-authored files). Regenerated data/corpus.xml and updated roundtrip-baseline.txt for the new path.
…repair-w6uwh2 # Conflicts: # src/private/mxtest/api/roundtrip-baseline.txt
Owner
Author
|
Pushed a commit addressing the
On the two Also moved Generated by Claude Code |
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36644 |
| Functions | 74.3% | 6352 / 8554 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.5% | 6880 / 8437 |
| Functions | 68.6% | 2296 / 3346 |
| Branches | 50.1% | 5939 / 11857 |
Core HTML report | API HTML report
Commit 557706ef98a73f68b9d3801ea9d6ad27aa7b04af.
gen-quality
|
3 tasks
webern
added a commit
that referenced
this pull request
Jul 19, 2026
## Human Summary Changes the way pedals are modeled into two different things. Pedal marks, which are the old fashioned Ped and * marks, and a separate type for more modern lines. ## Summary `DirectionReader::parsePedal` handled only `PedalType` start/stop and silently dropped `sostenuto`, `change`, `continue`, `discontinue`, and `resume`, even though `core::PedalType` carries all seven tags. This was the last unmodeled item in #324. The shape decision (called for in #324) was to store the pedal type as one fact in one field rather than scatter it across start/stop/other vectors. This replaces the `pedalStarts` / `pedalStops` spanner vectors on `DirectionData` with a single `std::vector<PedalData> pedals`, where each `PedalData` carries a `PedalKind` enum covering every pedal-line type. Each pedal event stands on its own and is placed at its own tick, so the model has no neighbor-dependent meaning. - New `PedalData.h`: the `PedalKind` enum (`start`, `stop`, `sostenuto`, `change`, `continueLine`, `discontinue`, `resume`) and the `PedalData` struct. - Reader: line pedals (`line="yes"`, plus the inherently line-formatting types `sostenuto`/`change`/`continue`/`discontinue`/`resume`) go to `pedals`; sign start/stop still route through `MarkData` as `MarkType::pedal` / `damp`, unchanged. - Writer: a single `emitPedal` maps `PedalKind` back to `core::PedalType`, emits `line="yes"`, and emits nothing for an unspecified kind. Only two corpus files use these types (`lysuite/ly31a_Directions.xml`, `lysuite/ly33a_Spanners.xml`) and both fail first on unrelated features, so this does not flip any file to PASS. It is validated by a focused unit test that round-trips each pedal type through the api. This is a breaking change: it reshapes `DirectionData`'s pedal fields. ## Testing - [x] New `PedalAllKinds` round-trips all seven pedal types through the api; `PedalPlacement` covers placement (`*Pedal*`: 44 assertions in 5 test cases) - [x] Full unit suite passes (5130 assertions in 451 test cases) - [x] api round-trip regression unchanged (284 passed, 0 failed of 284 pinned) ## References - Closes #324 (the `symbol` item from that worklist is tracked separately by #294 and is not included here) - Follows the merged direction-type stack #358–#363 - Part of #208 - Spanner-numbering context: #320, #351
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
Take @rpatters1's suggested design from #297 and attempts to implement it with a spanner identifying object and a sort of spanner resource pool.
AI Summary
MusicXML pairs a spanner's start with its stop using a
numberattribute, and two spanners of the same type that are open at the same point in the serialized stream must carry different numbers. Which spanners are open at the same point depends on the order the writer emits notes (voice interleaving viabackup), so a caller cannot reliably choose the number. This PR moves number assignment into the writer.The design differs from the shape proposed in #297 (a second
spannerIdfield besidenumberLevel): two mutually exclusive fields side by side is the recurring defect that design principle #3 and #249 exist to eliminate. Instead the two are unified into one value type.SpannerNumbervalue type (SpannerNumber.h/SpannerNumber.cpp, following theTimeChoice.h/.cppdeclaration/implementation split) with exactly three states: unspecified (default, no number emitted for a lone spanner), explicit level 1..16 (emitted verbatim; what the reader produces), and identity (an author-only, never-serialized label; start/continue/stop sharing an id are the same logical spanner and the writer assigns the number). A contradictory state is unrepresentable by construction. Constructed directly (SpannerNumber{3},SpannerNumber{"my-id"}), not via factory functions. Follows theTimeChoice"choice" pattern: nothing throws. An out-of-range level or an empty id collapses to unspecified at construction, and callinglevel()/identity()for the wrong kind returns a harmless default (NUMBER_LEVEL_UNSPECIFIED, or an empty string) instead of an exception.int numberLevelis replaced bySpannerNumber numberinCurveStart,CurveContinue,CurveStop,WedgeStart,WedgeStop,SpannerStart, andSpannerStop. Tuplets are untouched and keep the legacyint+NUMBER_LEVEL_UNSPECIFIED.SpannerNumberResolverruns once per part before writing. It walks the part in true serialization order (measures, staves, voices ascending, notes in vector order with curve stops/continues/starts per note, directions in vector order) and assigns each identity spanner the lowest free number from a 1..16 pool per staff and per spanner class (slur, tied, wedge, octave-shift, bracket, and dashes pools are independent). While an explicit spanner is open its level is reserved, so explicit and identity spanners sharing a pool never collide. Pool exhaustion (more than 16 concurrently open) throws rather than emitting an illegal number.Testing
SpannerNumberTest: three-state semantics, out-of-range/empty-input construction collapsing to unspecified, wrong-kind accessors returning harmless defaults (never throwing), equality across all kind combinationsSpannerIdentityTest: the Add API-level spanner identity for writer-sidenumberassignment #297 scenario — slurs disjoint in musical time that overlap in the stream once voice 1 serializes ahead of voice 2 get distinct numbers (1, 1-reused, 2); explicit + identity pool sharing; slur/tied pool independence; wedge number round-tripdata/rpatters1/slurs_overbars.musicxml: the file rpatters1 attached to Add API-level spanner identity for writer-sidenumberassignment #297 (a genuine Finale export, confirmed provenance in review), pinned inroundtrip-baseline.txtalongside his other contributed fixtures indata/rpatters1/make test,make test-api-roundtrip(166/166 pinned),make test-core-dev(835 corert files),make check-core-dev,make validate-cpp,make test-cpp-unit,make probe-cppCMAKE_UNITY_BUILD=ON, batch size 0) compilesReferences
numberassignment #297isSpecifiedbool pairs with an explicit optional value type #249 (one fact, one field: motivates unifying level and identity into a single type instead of adding a parallel field)