From afb487c24d5cd816260cc2f873fbac3e6b4f55a9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 14:47:42 -0400 Subject: [PATCH] P13-I2: Staff::default_clef is the fallback, not decoration to_constrained took the active clef from the staff instance's clef_sequence and fell back to Clef::default() -- treble. A staff that declared its clef only on the Staff, with no ClefChange, therefore drew a treble clef and placed every note against it. The field was decorative in the projection. It is the fallback. StaffContent now carries default_clef -- the clef belongs to the Staff, the sequence to the StaffInstance, and resolving "the clef at time t" needs both -- and active_clef_or(clefs, at, default) resolves against it. active_clef remains as that with the treble default, for callers with no staff to hand, so the public API is intact. The part worth pausing on: epiphany-editor-core reads the same function for hit-test pitch resolution. Fixing only the projection would have left a click on a bass staff resolving its pitch as treble -- the engraved clef and the editor disagreeing about what note is where. Both now go through active_clef_or. Removal was rejected: the field is named for its purpose, is encoded on the wire, and dropping it would be schema-major. Zero golden churn: every fixture and generator declares treble, which is also Clef::default(), so nothing that exists today moves. Locked by a_staff_declaring_only_a_default_clef_engraves_in_it and mutation-verified by restoring the Clef::default() fallback. Scoping that test by provenance was necessary -- valid_score_rich has three staves and only one was re-clefed, so the first assertion I wrote ("no gClef anywhere") failed against a correct fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/epiphany-editor-core/src/lib.rs | 15 +++- crates/epiphany-engrave/src/lib.rs | 3 + crates/epiphany-layout-ir/DECISIONS.md | 26 ++++-- crates/epiphany-layout-ir/src/constrained.rs | 87 ++++++++++++++++++-- crates/epiphany-layout-ir/src/lib.rs | 4 +- crates/epiphany-layout-ir/src/logical.rs | 13 +++ crates/epiphany-testkit/src/layout_stub.rs | 1 + spec/PASS13_CANDIDATES.md | 2 +- 8 files changed, 131 insertions(+), 20 deletions(-) diff --git a/crates/epiphany-editor-core/src/lib.rs b/crates/epiphany-editor-core/src/lib.rs index 377a6c1..de44ac2 100644 --- a/crates/epiphany-editor-core/src/lib.rs +++ b/crates/epiphany-editor-core/src/lib.rs @@ -63,9 +63,10 @@ use epiphany_core::{ VoiceId, WallClockTime, }; use epiphany_layout_ir::{ - active_clef, manifestation_layout_id, staff_step_pitch, to_constrained, to_logical, to_render, - ConstraintSolver, ExtensionRef, HitTestMap, LayoutContent, LayoutObjectId, LogicalLayoutIR, - Point, Rect, RenderIR, ResolvedLayoutIR, ResolvedSystem, SolverConfig, TimePoint, + active_clef_or, manifestation_layout_id, staff_step_pitch, to_constrained, to_logical, + to_render, ConstraintSolver, ExtensionRef, HitTestMap, LayoutContent, LayoutObjectId, + LogicalLayoutIR, Point, Rect, RenderIR, ResolvedLayoutIR, ResolvedSystem, SolverConfig, + TimePoint, }; use epiphany_ops::{ advisory_violations, AcceptOutcome, AuthorId, CausalContext, DeleteEventOp, @@ -2606,7 +2607,13 @@ fn staff_start_clefs(logical: &LogicalLayoutIR) -> StartClefs { for object in ®ion.objects { if let (Some(staff), LayoutContent::Staff(content)) = (object.staff(), object.content()) { - clefs.insert((region_id, staff), active_clef(&content.clefs, &start)); + // The staff's own default backs an empty (or later-starting) + // clef sequence, exactly as the projection resolves it — else a + // click on a bass staff would resolve its pitch as treble. + clefs.insert( + (region_id, staff), + active_clef_or(&content.clefs, &start, content.default_clef), + ); } } } diff --git a/crates/epiphany-engrave/src/lib.rs b/crates/epiphany-engrave/src/lib.rs index 11f0de3..71fff8e 100644 --- a/crates/epiphany-engrave/src/lib.rs +++ b/crates/epiphany-engrave/src/lib.rs @@ -856,6 +856,7 @@ mod tests { manifested( TypedObjectId::StaffInstance(StaffInstanceId::from_raw(1)), LayoutContent::Staff(StaffContent { + default_clef: epiphany_core::Clef::default(), clefs: vec![], keys: vec![], }), @@ -1277,6 +1278,7 @@ mod tests { let mut objects = vec![manifested( TypedObjectId::StaffInstance(StaffInstanceId::from_raw(1)), LayoutContent::Staff(StaffContent { + default_clef: epiphany_core::Clef::default(), clefs: vec![], keys: vec![], }), @@ -1780,6 +1782,7 @@ mod tests { manifested( TypedObjectId::StaffInstance(StaffInstanceId::from_raw(1)), LayoutContent::Staff(StaffContent { + default_clef: epiphany_core::Clef::default(), clefs: vec![], keys: vec![PlacedKeySignature { time: TimePoint::Musical(MusicalPosition::origin()), diff --git a/crates/epiphany-layout-ir/DECISIONS.md b/crates/epiphany-layout-ir/DECISIONS.md index 2fe4168..78c8dcd 100644 --- a/crates/epiphany-layout-ir/DECISIONS.md +++ b/crates/epiphany-layout-ir/DECISIONS.md @@ -710,7 +710,7 @@ find what the author wrote); guessing produces a score that looks engraved and i wrong, with nothing in the IR to say so. Ratified as implemented; locked by `an_unengravable_object_is_recorded_and_still_placed`. -## Parked: `Staff::default_clef` is never consulted (2026-07-09) +## RESOLVED (P13-I2): `Staff::default_clef` is now the fallback (2026-07-09) `to_constrained` takes a staff instance's active clef from its `clef_sequence` (via `staff_content`'s `PlacedClef` list) and, when that sequence is empty, falls @@ -720,11 +720,25 @@ the field is decorative in the projection. No consumer in this crate reads it (verified: `default_clef` appears only in core's codec/generators and the fixtures). -Found while building `percussion_placeholder_staff`, which therefore has to -declare its percussion clef as a `ClefChange` rather than on the staff. Whether -the staff's default should seed the sequence, or the field should be removed, is -a small design question — not a silent-corruption bug (nothing is lost, only -ignored). +Found while building `percussion_placeholder_staff`, which therefore had to +declare its percussion clef as a `ClefChange` rather than on the staff. + +**Resolved as P13-I2: it is the fallback.** `StaffContent` now carries +`default_clef` (the clef belongs to the `Staff`, the sequence to the +`StaffInstance`, and resolving "the clef at time t" needs both), and +`active_clef_or(clefs, at, default)` resolves against it. `active_clef` remains as +that with the treble default, for callers with no staff to hand. + +The subtle part is that `epiphany-editor-core` reads the same function for +hit-test pitch resolution: had only the projection been fixed, a click on a bass +staff would have resolved its pitch as treble, and the engraved clef and the +editor would have disagreed. Both now go through `active_clef_or`. + +Removal was rejected: the field is named for its purpose, is encoded on the wire, +and dropping it would be schema-major. Zero golden churn — every fixture and +generator declares treble, which is also `Clef::default()`. Locked by +`a_staff_declaring_only_a_default_clef_engraves_in_it`, mutation-verified against +the old `Clef::default()` fallback. **Both of the above are parked, not open.** The Pass-13 batch is closed and the house rule opens a pass at ≥3 candidates; these are two. They join a future batch diff --git a/crates/epiphany-layout-ir/src/constrained.rs b/crates/epiphany-layout-ir/src/constrained.rs index 3cb3c3b..fe5c9bb 100644 --- a/crates/epiphany-layout-ir/src/constrained.rs +++ b/crates/epiphany-layout-ir/src/constrained.rs @@ -802,14 +802,16 @@ pub fn try_to_constrained( // The clef *sequence* in force on each staff (a staff instance carries it). // The active clef at a given position is the latest change at or before it, // so a mid-staff clef change moves later pitches without affecting earlier - // ones. An empty sequence defaults to treble. + // ones. An empty sequence falls back to the staff's own `default_clef`. let mut clef_seq_of: BTreeMap> = BTreeMap::new(); + let mut clef_default_of: BTreeMap = BTreeMap::new(); for object in ®ion.objects { if let (Some(staff), LayoutContent::Staff(content)) = (object.staff(), object.content()) { clef_seq_of .entry(staff) .or_insert_with(|| content.clefs.clone()); + clef_default_of.entry(staff).or_insert(content.default_clef); } } let clef_seq = |staff: Option| -> &[PlacedClef] { @@ -818,6 +820,13 @@ pub fn try_to_constrained( .map(Vec::as_slice) .unwrap_or(&[]) }; + // A staff's own default clef, in force before its first `ClefChange`. + let clef_default = |staff: Option| -> Clef { + staff + .and_then(|s| clef_default_of.get(&s)) + .copied() + .unwrap_or_default() + }; // Pass 1 — compute every glyph's notation, keyed for emission in pass 2, // and collect the distinct columns it occupies. A note/rest notated as a @@ -862,7 +871,7 @@ pub fn try_to_constrained( let time = shift_time(¬e.position, &offset); let key = ColumnKey::Timed(time.clone(), ColumnRole::Note); keys.insert(key.clone()); - let clef = active_clef(clef_seq(staff), &time); + let clef = active_clef_or(clef_seq(staff), &time, clef_default(staff)); let name = notehead_glyph(value); let mut ys = Vec::new(); for pitch in ¬e.pitches { @@ -1022,7 +1031,7 @@ pub fn try_to_constrained( // The staff instance's clef glyph occupies the lead column. The // *displayed* clef is the one in force at the staff start, by // time — consistent with how notes resolve their active clef. - let clef = active_clef(&content.clefs, &origin()); + let clef = active_clef_or(&content.clefs, &origin(), content.default_clef); if clef_glyph(clef.shape).is_some() { keys.insert(ColumnKey::Lead); // The key signature shares the lead column; reserve its width. @@ -1223,7 +1232,9 @@ pub fn try_to_constrained( // The displayed clef is the one in force at the staff start, by // time — the same query the notes use, so they always agree. let clef = match content { - Some(LayoutContent::Staff(c)) => active_clef(&c.clefs, &origin()), + Some(LayoutContent::Staff(c)) => { + active_clef_or(&c.clefs, &origin(), c.default_clef) + } _ => Clef::default(), }; match clef_glyph(clef.shape) { @@ -1433,7 +1444,7 @@ pub fn try_to_constrained( // An unmatched pitch (no event content reached it): a black // notehead on the clef reference line, with the gap surfaced. emit.diag(provenance.source, LayoutDiagnosticKind::MissingSpelling); - let clef = active_clef(clef_seq(staff), &origin()); + let clef = active_clef_or(clef_seq(staff), &origin(), clef_default(staff)); emit.stroke(anchor( provenance, Point::new(default_x, step_to_y(yo, reference_step(&clef))), @@ -2507,7 +2518,10 @@ fn active_key(keys: &[PlacedKeySignature]) -> Option { /// the key is C major, or the clef has no diatonic positions (percussion). fn key_accidentals_for(content: &StaffContent) -> Vec { match active_key(&content.keys) { - Some(key) => key_signature(key, &active_clef(&content.clefs, &origin())), + Some(key) => key_signature( + key, + &active_clef_or(&content.clefs, &origin(), content.default_clef), + ), None => Vec::new(), } } @@ -2581,6 +2595,15 @@ fn shift_time(base: &TimePoint, offset: &MusicalDuration) -> TimePoint { /// agrees with the notes. The sequence is not assumed sorted: `[bass@1, treble@0]` /// resolves a note after time 1 to bass and one before to treble. pub fn active_clef(clefs: &[PlacedClef], at: &TimePoint) -> Clef { + active_clef_or(clefs, at, Clef::default()) +} + +/// The clef in force at `at`, falling back to `default` — the staff's own +/// `Staff::default_clef` — when the sequence names none. A staff that declares +/// its clef only on the `Staff` (no `ClefChange` at all) engraves in that clef; +/// `active_clef` is this with the treble default, kept for callers that have no +/// staff to hand. +pub fn active_clef_or(clefs: &[PlacedClef], at: &TimePoint, default: Clef) -> Clef { clefs .iter() .filter(|placed| { @@ -2592,7 +2615,7 @@ pub fn active_clef(clefs: &[PlacedClef], at: &TimePoint) -> Clef { .max_by(|a, b| time_total(&a.time, &b.time)) .or_else(|| clefs.iter().min_by(|a, b| time_total(&a.time, &b.time))) .map(|p| p.clef) - .unwrap_or_default() + .unwrap_or(default) } /// The musical origin (the active-clef query for an unanchored pitch). @@ -3406,6 +3429,7 @@ mod tests { let instance = StaffInstanceId::from_raw(1); // D major (two sharps), default treble clef. let content = LayoutContent::Staff(StaffContent { + default_clef: Clef::default(), clefs: vec![], keys: vec![PlacedKeySignature { time: TimePoint::Musical(MusicalPosition::origin()), @@ -3718,6 +3742,7 @@ mod tests { let staff = StaffId::from_raw(10); let at = |n, d| TimePoint::Musical(MusicalPosition(RationalTime::new(n, d).unwrap())); let content = LayoutContent::Staff(StaffContent { + default_clef: Clef::default(), // Authored out of order: bass at time 1, treble at time 0. clefs: vec![ PlacedClef { @@ -4050,6 +4075,7 @@ mod tests { TypedObjectId::StaffInstance(StaffInstanceId::from_raw(si)), staff, LayoutContent::Staff(StaffContent { + default_clef: Clef::default(), clefs: vec![], keys: vec![], }), @@ -4808,6 +4834,53 @@ mod tests { ); } + /// A staff that declares its clef ONLY on the `Staff` — no `ClefChange` at + /// all — engraves in that clef. `Staff::default_clef` used to be decorative: + /// the projection took the active clef from the staff instance's sequence and + /// fell back to `Clef::default()`, so a bass staff drew a treble clef and put + /// every note two steps wrong (P13-I2). + #[test] + fn a_staff_declaring_only_a_default_clef_engraves_in_it() { + let (mut score, _) = repeat_ready_score(48); + let staff_id = score.canvas.regions[0].staff_instances()[0].staff; + for staff in &mut score.staves { + if staff.id == staff_id { + staff.default_clef = Clef::bass(); + } + } + assert!( + score.canvas.regions[0].staff_instances()[0] + .clef_sequence + .is_empty(), + "the staff declares no ClefChange — only its default" + ); + let instance_id = score.canvas.regions[0].staff_instances()[0].id; + let c = to_constrained(&to_logical(&score)); + + // THIS staff's clef glyph is the bass clef, not the treble default. The + // score's other staves keep their own defaults, so scope by provenance. + let drawn: Vec<&str> = c + .glyphs + .iter() + .filter(|g| g.provenance.source == TypedObjectId::StaffInstance(instance_id)) + .map(|g| g.glyph.as_str()) + .collect(); + assert_eq!( + drawn, + vec!["fClef"], + "a staff whose only clef is its default draws it" + ); + // And its notes are placed against that clef: `active_clef_or` is what the + // staff-position computation reads, so the two can never disagree. + let at = TimePoint::Musical(epiphany_core::MusicalPosition::origin()); + assert_eq!(active_clef_or(&[], &at, Clef::bass()), Clef::bass()); + assert_eq!( + active_clef(&[], &at), + Clef::default(), + "the old API is intact" + ); + } + /// `req:layoutir:coverage-diagnostics`: an object the projection cannot /// engrave faithfully is **recorded and still placed** — never guessed at, /// never dropped. A percussion clef has no bundled glyph, so the staff diff --git a/crates/epiphany-layout-ir/src/lib.rs b/crates/epiphany-layout-ir/src/lib.rs index d5a1e7e..2fa7438 100644 --- a/crates/epiphany-layout-ir/src/lib.rs +++ b/crates/epiphany-layout-ir/src/lib.rs @@ -93,8 +93,8 @@ pub use cache::{ ResolvedSystemCache, SystemId, }; pub use constrained::{ - active_clef, is_rigid_width_stroke, to_constrained, try_to_constrained, Axis, BreakClass, - BreakKind, BreakOrigin, ConstrainedLayoutIR, ConstrainedLayoutRegion, + active_clef, active_clef_or, is_rigid_width_stroke, to_constrained, try_to_constrained, Axis, + BreakClass, BreakKind, BreakOrigin, ConstrainedLayoutIR, ConstrainedLayoutRegion, ConstrainedValidationError, ConstraintParameters, ConstraintRegistryId, Curve, GlyphObject, GlyphObjectId, GlyphStyle, LayoutConstraint, LayoutTransformError, SpringSlot, Stroke, }; diff --git a/crates/epiphany-layout-ir/src/logical.rs b/crates/epiphany-layout-ir/src/logical.rs index aebaedf..4c1ce1e 100644 --- a/crates/epiphany-layout-ir/src/logical.rs +++ b/crates/epiphany-layout-ir/src/logical.rs @@ -75,6 +75,12 @@ pub enum LayoutContent { pub struct StaffContent { pub clefs: Vec, pub keys: Vec, + /// The staff's own `default_clef` — the clef in force before the first + /// `ClefChange`, and throughout a staff that declares none. Carried here + /// because the clef belongs to the `Staff` while the sequence belongs to the + /// `StaffInstance`, and every consumer resolving "the clef at time t" needs + /// both (`active_clef_or`). + pub default_clef: Clef, } /// A clef change with its score anchor resolved into the layout time axis. @@ -827,7 +833,14 @@ fn derive_score_version(score: &Score) -> ScoreVersion { /// are carried as-is — the constrained pass defaults the *active* clef/key to /// treble / C major. fn staff_content(score: &Score, si: &epiphany_core::StaffInstance) -> LayoutContent { + let default_clef = score + .staves + .iter() + .find(|staff| staff.id == si.staff) + .map(|staff| staff.default_clef) + .unwrap_or_default(); LayoutContent::Staff(StaffContent { + default_clef, clefs: si .clef_sequence .iter() diff --git a/crates/epiphany-testkit/src/layout_stub.rs b/crates/epiphany-testkit/src/layout_stub.rs index 639829c..591095b 100644 --- a/crates/epiphany-testkit/src/layout_stub.rs +++ b/crates/epiphany-testkit/src/layout_stub.rs @@ -218,6 +218,7 @@ pub fn gen_layout_content(rng: &mut Rng) -> LayoutContent { match rng.below(6) { 0 => LayoutContent::Structural, 1 => LayoutContent::Staff(StaffContent { + default_clef: epiphany_core::Clef::default(), clefs: clefs(rng), keys: keys(rng), }), diff --git a/spec/PASS13_CANDIDATES.md b/spec/PASS13_CANDIDATES.md index 62ed4bd..a2a1bd3 100644 --- a/spec/PASS13_CANDIDATES.md +++ b/spec/PASS13_CANDIDATES.md @@ -31,5 +31,5 @@ what is true. | Id | One-line statement | Filed in | Status | |---|---|---|---| | P13-I1 | Chapter 7's `ConstrainedLayoutIR` listing elides **three** fields the code carries: `break_origins: Vec` (named by `req:layoutir:break-origin-attribution`, its own shape unlisted), `catalog: GlyphCatalogIdentity` (its type specified, the field unlisted), and `diagnostics: Vec` — which appears **nowhere** in core_spec, though it is how the projection's honesty rule manifests: an unspellable pitch or an unbundled glyph is placed as a fallback *and recorded*, never silently guessed | `crates/epiphany-layout-ir/DECISIONS.md` ("the ConstrainedLayoutIR listing is still abridged") | **resolved** (Pass 13: listing gains all three fields; `BreakOrigin` and `LayoutDiagnostic` shapes added; new `req:layoutir:coverage-diagnostics` ratifies as-implemented that an unengravable object is recorded AND still placed — never guessed, never dropped) | -| P13-I2 | `Staff::default_clef` is never consulted: `to_constrained` takes the active clef from the staff instance's `clef_sequence` and falls back to `Clef::default()` (treble), so a bass-clef staff that declares its clef only on the `Staff` engraves as treble. The field is decorative in the projection — is it the fallback, or should it not exist? | `crates/epiphany-layout-ir/DECISIONS.md` ("`Staff::default_clef` is never consulted") | **open** | +| P13-I2 | `Staff::default_clef` is never consulted: `to_constrained` takes the active clef from the staff instance's `clef_sequence` and falls back to `Clef::default()` (treble), so a bass-clef staff that declares its clef only on the `Staff` engraves as treble. The field is decorative in the projection — is it the fallback, or should it not exist? | `crates/epiphany-layout-ir/DECISIONS.md` ("`Staff::default_clef` is never consulted") | **resolved** (Pass 13: it IS the fallback. `StaffContent` carries it, `active_clef_or` resolves against it, and `editor-core`'s hit-test reads the same function — else a click on a bass staff would resolve its pitch as treble. Removal was rejected: the field is named for its purpose, is encoded on the wire, and dropping it is schema-major) | | P13-I3 | `BRAVURA_METRICS`' `NOTEHEAD_ANCHORS` are hand-written, unconsumed, and doubly suspect: they name `stemUpNW`/`stemDownSE` — the corners a normal notehead's stems do *not* attach to, and a pair Bravura's `noteheadBlack` does not define — and their x of `1180` reads like 1.18 staff spaces written in thousandths rather than the table's `1/1024` units (1.18 sp = 1208). They enter only `metrics_hash`, so any correction moves the `GlyphCatalogIdentity` every conformance claim declares. The font is not vendored, so the values cannot be verified in-tree | `crates/epiphany-layout-ir/DECISIONS.md` ("the notehead stem anchors are unusable as written") | **open** |