feat: unified spanner numbering with writer-side assignment#320
Open
webern wants to merge 2 commits into
Open
Conversation
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.
webern
force-pushed
the
claude/friendly-cori-pg1yfm
branch
from
41c2446 to
e6bc65d
Compare
Coverage reportAPI coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| (api coverage not produced) |
Core HTML report | API HTML report
Commit 3a8a3b659389c145aa9f8a9eede95b634486ef65.
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.
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
|
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
Take @rpatters1's suggested design from #297 and attempts to implement it with a spanner identifying object and a sort of spanner resource pool.
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 inApiCommon.hwith 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, and misuse (out-of-range level, empty id, wrong-kind accessor) throwsstd::logic_error.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, factory preconditions, wrong-kind accessors, 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-tripcustom/slurs_overbars.musicxml(from the file attached to Add API-level spanner identity for writer-sidenumberassignment #297) pinned inroundtrip-baseline.txt; corert count 833 -> 834,make auditregeneratedmake test(4777 assertions in 388 test cases),make test-api-roundtrip(159/159 pinned),make test-core-dev(834 files),make check-core-dev,make validate-cpp,make test-cpp-unitCMAKE_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)