diff --git a/crates/epiphany-engrave/DECISIONS.md b/crates/epiphany-engrave/DECISIONS.md index fe3f9ba..644640f 100644 --- a/crates/epiphany-engrave/DECISIONS.md +++ b/crates/epiphany-engrave/DECISIONS.md @@ -899,3 +899,44 @@ re-filed as a bug. while the lower staff descends. No primitive can name an `InterStaffGap` band today (`band_of` yields a staff band or the region's margin band), so this is unreachable rather than latent — building the machinery now would be speculative. + +### Review follow-up: two more glyph-members assumptions (2026-07-09) + +An adversarial review found the content-extent correction incomplete in two +places. Both were right; the second falsified a comment written in the same +commit that introduced it. + +1. **Region staff bands were still identified by glyph `members`.** `vertical_raw` + measured content over all primitives but decided *which* staff bands belong to + a region by glyph membership — the very assumption the change exists to shed. + A staff band is allowed to own no glyphs: `to_constrained` emits one per staff + of the region regardless, and a percussion-clef staff (no bundled glyph, so it + engraves to a traced anchor stroke) with no notes owns only its staff-line + strokes. Region membership now comes from **content presence** in one of the + region's systems, which identifies the band exactly, because a staff band is + per-`(staff, region)` manifestation and its content can land nowhere else. + Locked by `percussion_placeholder_staff` + `a_staff_band_owning_no_glyphs_ + still_contributes_its_gap`. Mutation-verified: the members filter scores + `4.8e-7` where the fix reports real deviation. + +2. **Only the first realizing system was measured.** The code took one system per + gap band, justified by a comment claiming rigid system translation makes every + realization agree. The inter-staff solve had *just* falsified that: it sizes + each system's gaps from that system's own content. `req:qmc:vertical` now + counts **one unit per realization** (QMC 0.2.0 → 0.3.0), matching how realized + inter-system gaps were already counted. Locked by `two_staff_wrapping_pressure` + (staff-line gap 15.93 in the pressured system, 7.87 in the slack one) + + `inter_staff_gaps_are_measured_in_every_system_that_realizes_them`. + Mutation-verified: first-system-only scores `1.3e-7`. + +**What this exposes, and it is not comfortable.** `two_staff_wrapping_pressure` +scores `vertical_density_penalty` **0.739**. Its pressured system solves to the +declared gap exactly; its slack system sits at ~5 staff spaces of content gap +against a preferred 2.0. The axis is symmetric — a gap wider than preferred is +sprawl exactly as a narrower one is crowding — and this solve **only expands, +never compresses**. The deferral recorded above ("compressing an OVER-wide fixed +gap toward preferred… the fixed pitch is generous by default, so this is rarely +wanted") is therefore promoted from *rarely wanted* to **measurably wrong**: any +un-pressured multi-staff system now reports honest sprawl until the solve can pull +staves together. Named here rather than fixed in the same breath — compression is +a layout change (golden churn, `ENGRAVER_VERSION` move), not a measurement one. diff --git a/crates/epiphany-engrave/src/lib.rs b/crates/epiphany-engrave/src/lib.rs index 30b947f..14da586 100644 --- a/crates/epiphany-engrave/src/lib.rs +++ b/crates/epiphany-engrave/src/lib.rs @@ -2333,6 +2333,41 @@ mod tests { ); } + /// A staff band that owns no GLYPHS is still a staff of its region. The + /// percussion placeholder's clef has no bundled glyph (it engraves to a + /// traced anchor stroke) and it carries no notes, so its band's `members` + /// list — which holds glyphs only — is empty, while the band does own its + /// five staff-line strokes. Identifying a region's staff bands by their + /// glyph members would drop it from the axis entirely, reintroducing the + /// very glyph-members assumption the content-extent measurement sheds. + #[test] + fn a_staff_band_owning_no_glyphs_still_contributes_its_gap() { + use epiphany_layout_ir::{to_constrained, to_logical}; + let input = to_constrained(&to_logical( + &epiphany_testkit::fixtures::percussion_placeholder_staff(1), + )); + let report = Engraver::default().solve(&input, &SolverConfig::default()); + assert_eq!(report.status, SolveStatus::Solved); + for page in &report.layout.pages { + for sys in &page.systems { + assert_eq!( + sys.staves.len(), + 2, + "the glyph-less placeholder is still laid out as a staff" + ); + } + } + // Its gap to the melody staff is slack (the solve expands, never + // compresses), so the band contributes a large deviation. Dropping the + // band would leave the region with one staff, no inter-staff unit, and + // an axis of exactly 0. + let density = report.metric_vector.vertical_density_penalty.0; + assert!( + density > 0.5, + "the glyph-less band's gap reaches the axis: {density}" + ); + } + #[test] fn inter_staff_solve_separates_colliding_staves() { use epiphany_layout_ir::{to_constrained, to_logical}; @@ -2484,6 +2519,54 @@ mod tests { assert!(gaps[2] < 2.0, "and sits close against it: {}", gaps[2]); } + /// A gap band realized in several systems contributes a unit PER SYSTEM. + /// The inter-staff solve sizes each system's gaps from that system's own + /// content, so one band's realized height genuinely differs across a + /// region's systems — here 15.93 staff spaces in the system carrying the + /// colliding first measure, 7.87 in the slack one after it. Measuring only + /// the first system realizing the band (as this metric once did, on the + /// since-falsified premise that rigid system translation makes every + /// realization agree) would report the pressured system's near-perfect gap + /// and discard the slack one entirely. + #[test] + fn inter_staff_gaps_are_measured_in_every_system_that_realizes_them() { + use epiphany_layout_ir::{to_constrained, to_logical}; + let report = Engraver::default().solve( + &to_constrained(&to_logical( + &epiphany_testkit::fixtures::two_staff_wrapping_pressure(1), + )), + &SolverConfig::default(), + ); + let systems: Vec<_> = report + .layout + .pages + .iter() + .flat_map(|page| &page.systems) + .collect(); + assert_eq!(systems.len(), 2, "the region wraps into two systems"); + let gap = |sys: &epiphany_layout_ir::ResolvedSystem| { + assert_eq!(sys.staves.len(), 2, "both staves ride every system"); + let (top, bottom) = (&sys.staves[0].bounding_box, &sys.staves[1].bounding_box); + top.origin.y.0 - (bottom.origin.y.0 + bottom.size.height.0) + }; + let (first, second) = (gap(systems[0]), gap(systems[1])); + assert!( + first > second + 4.0, + "the pressured system opens much further than the slack one: {first} vs {second}" + ); + + // The slack system's gap sits well past the band's preferred height (the + // solve expands but never compresses), so it contributes a large + // deviation. The pressured system was solved to preferred and contributes + // ~0. A first-system-only measurement would therefore report ~0 overall; + // counting both realizations reports the sprawl honestly. + let density = report.metric_vector.vertical_density_penalty.0; + assert!( + density > 0.5, + "the slack system's sprawl reaches the axis: {density}" + ); + } + #[test] fn vertical_justification_fills_non_final_pages() { use epiphany_layout_ir::{Margins, Size2D, StaffSpace}; diff --git a/crates/epiphany-engrave/src/quality.rs b/crates/epiphany-engrave/src/quality.rs index 289a965..5a0d5ce 100644 --- a/crates/epiphany-engrave/src/quality.rs +++ b/crates/epiphany-engrave/src/quality.rs @@ -64,8 +64,8 @@ use epiphany_layout_ir::quality::{ anchors, normalize, MetricThresholds, QUALITY_FLOOR_FRACTION, QUALITY_METRIC_KINDS, }; use epiphany_layout_ir::{ - inter_staff_gap_id, ConstrainedLayoutIR, Curve, GlyphObjectId, QualityMetricVector, - SolverWarning, SolverWarningKind, SpringSlotId, VerticalBand, VerticalBandId, VerticalBandKind, + inter_staff_gap_id, ConstrainedLayoutIR, Curve, QualityMetricVector, SolverWarning, + SolverWarningKind, SpringSlotId, VerticalBand, VerticalBandId, VerticalBandKind, }; use crate::casting::{CastLayout, PageGeometry}; @@ -385,26 +385,40 @@ fn vertical_raw(input: &ConstrainedLayoutIR, cast: &CastLayout, census: &SystemC } for (region_index, region) in input.regions.iter().enumerate() { - // The region's laid-out staff bands, top staff first, ordered within - // the region's first system (systems translate rigidly, so within- - // system y order is the region's staff order). - let first_system = census.region.iter().position(|&r| r == region_index); - let Some(first_system) = first_system else { + // The region's systems, in page order. A staff band is `to_constrained`'s + // per-*manifestation* band — one per (staff, region) — so its content can + // only land in its own region's systems, and "has content in one of this + // region's systems" identifies the region's staff bands exactly. + // + // Membership is NOT read from `VerticalBand::members`. That list holds + // glyphs, and a staff band is allowed to have none: a staff whose clef is + // unbundled engraves to an anchor *stroke*, and its staff lines are + // strokes regardless. Filtering by glyph members would silently drop such + // a staff from the axis — the same glyph-members assumption this metric + // exists to shed. + let region_systems: Vec = census + .region + .iter() + .enumerate() + .filter(|&(_, &r)| r == region_index) + .map(|(system, _)| system) + .collect(); + let Some(&first_system) = region_systems.first() else { continue; }; - let region_glyphs: BTreeSet = region.glyphs.iter().copied().collect(); + // Top staff first, ordered within the region's first system: a system + // translates rigidly as a whole, so the staff ORDER is the same in each + // (only the gaps between them are renegotiated per system). let mut staves: Vec<(f64, VerticalBandId)> = input .vertical_bands .iter() .filter(|band| matches!(band.kind, VerticalBandKind::Staff(_))) - .filter(|band| band.members.iter().any(|id| region_glyphs.contains(id))) .filter_map(|band| { content .get(&(first_system, band.id)) .map(|&(_, top)| (top, band.id)) }) .collect(); - // Top staff first. staves.sort_by(|a, b| b.0.total_cmp(&a.0)); // The region's declared inter-staff gap bands, by their derived ids @@ -420,19 +434,21 @@ fn vertical_raw(input: &ConstrainedLayoutIR, cast: &CastLayout, census: &SystemC continue; } let (upper, lower) = (staves[gap - 1].1, staves[gap].1); - // Realized iff the adjacent content shares a system; measure the - // separation there (rigid system translation makes every common - // system agree). - let common = (0..census.region.len()).find(|&system| { - content.contains_key(&(system, upper)) && content.contains_key(&(system, lower)) - }); - let Some(system) = common else { - continue; - }; - let upper_bottom = content[&(system, upper)].0; - let lower_top = content[&(system, lower)].1; - let realized = (upper_bottom - lower_top).max(0.0); - per_unit.push((realized - preferred).abs() / preferred); + // EVERY system realizing the pair contributes a unit, not just the + // first. The inter-staff solve sizes each system's gaps from that + // system's own content, so one band's realized height genuinely + // differs across the systems of a region — a later system may carry + // different pressure, or a bake bug touching one primitive class. + // Measuring only the first would average away both. + for &system in ®ion_systems { + let (Some(&(upper_bottom, _)), Some(&(_, lower_top))) = + (content.get(&(system, upper)), content.get(&(system, lower))) + else { + continue; + }; + let realized = (upper_bottom - lower_top).max(0.0); + per_unit.push((realized - preferred).abs() / preferred); + } } } diff --git a/crates/epiphany-layout-ir/DECISIONS.md b/crates/epiphany-layout-ir/DECISIONS.md index 9ff3824..751c976 100644 --- a/crates/epiphany-layout-ir/DECISIONS.md +++ b/crates/epiphany-layout-ir/DECISIONS.md @@ -695,6 +695,24 @@ them). Two fields the code carries are still absent from the listing: Neither blocks an implementation the way a missing `strokes`/`curves` did: both are governed by requirement text elsewhere, so a conformant implementer is not -left guessing. **Parked, not open.** The Pass-13 batch is closed and the house -rule opens a pass at ≥3 candidates; this is one. It joins a future batch rather -than reopening one on its own. +left guessing. + +## Parked: `Staff::default_clef` is never consulted (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 +back to `Clef::default()` — treble. It never reads `Staff::default_clef`. So a +bass-clef staff that declares its clef *only* on the `Staff` engraves as treble; +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). + +**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 +rather than reopening one. diff --git a/crates/epiphany-layout-ir/src/quality.rs b/crates/epiphany-layout-ir/src/quality.rs index e32b9fe..ab392f2 100644 --- a/crates/epiphany-layout-ir/src/quality.rs +++ b/crates/epiphany-layout-ir/src/quality.rs @@ -1,7 +1,7 @@ //! The Quality Metric Catalog's normative constants (companion specification -//! *Epiphany — Quality Metric Catalog*, v0.2.0): the per-axis normalization +//! *Epiphany — Quality Metric Catalog*, v0.3.0): the per-axis normalization //! anchors, the per-tier metric threshold table, the profile→threshold-column -//! mapping, and the `QualityFloorApproached` warning fraction. (The v0.2.0 +//! mapping, and the `QualityFloorApproached` warning fraction. (The v0.3.0 //! `spacing_distortion` refinement scoped that axis's *measurement domain* to //! rhythmic columns; it touched no anchor or threshold, so this transcription //! of the numeric constants is unchanged.) diff --git a/crates/epiphany-testkit/src/fixtures.rs b/crates/epiphany-testkit/src/fixtures.rs index 89feb36..d285b00 100644 --- a/crates/epiphany-testkit/src/fixtures.rs +++ b/crates/epiphany-testkit/src/fixtures.rs @@ -333,6 +333,242 @@ pub fn three_staff_close_content(seed: u64) -> Score { score } +/// A TWELVE-measure, TWO-staff metric score that wraps into more than one +/// system and carries its inter-staff pressure in the FIRST measure only: the +/// top staff dips to C2 and the bottom staff climbs to C6 there, while every +/// later measure is plain C4s on both staves. +/// +/// So the two systems of the one region want genuinely different staff gaps — +/// the first must open to clear the colliding ledgers, the second is already +/// slack. The inter-staff solve sizes each system's gaps from that system's own +/// content, so a quality metric that measured only the first system realizing a +/// gap band would average the second one away. Invariant-clean. +pub fn two_staff_wrapping_pressure(seed: u64) -> Score { + let mut rng = SplitMix64::new(seed ^ 0x0004_57AF_F123); + let replica = + ReplicaId::from_entropy(rng.next_u64().to_le_bytes()).unwrap_or(ReplicaId(0x4ADF)); + let mut idc = IdentityContext::new(replica); + + let top_staff: StaffId = idc.mint(); + let bottom_staff: StaffId = idc.mint(); + let instrument: InstrumentId = idc.mint(); + let region_id: RegionId = idc.mint(); + + const MEASURES: i64 = 12; + const BEATS: i64 = 4; + + // Measure 0 collides (top dives, bottom climbs); the rest sit inside the + // staff, where the fixed staff pitch already leaves the gap slack. + let octave = |staff_index: usize, index: i64| -> i8 { + if index >= BEATS { + return 4; + } + if staff_index == 0 { + [4i8, 3, 2, 3][index as usize] + } else { + [4i8, 5, 6, 5][index as usize] + } + }; + + let mut arena = EventArena::new(); + let mut instances: Vec = Vec::new(); + for (staff_index, staff_id) in [top_staff, bottom_staff].into_iter().enumerate() { + let instance_id: StaffInstanceId = idc.mint(); + let voice_id: VoiceId = idc.mint(); + let mut voice = Voice::user(voice_id); + for index in 0..(MEASURES * BEATS) { + let (eid, pid): (EventId, PitchId) = (idc.mint(), idc.mint()); + arena + .insert(c_at(eid, voice_id, pid, index, octave(staff_index, index))) + .unwrap(); + voice.events.push(eid); + } + let mut instance = StaffInstance::new(instance_id, staff_id); + instance.voices.push(voice); + for m in 0..MEASURES { + instance.measures.push(Measure { + id: idc.mint(), + start: TimeAnchor::Region { + id: region_id, + edge: RegionEdge::Start, + offset: AnchorOffset::Musical(MusicalDuration( + RationalTime::new(m, 1).unwrap(), + )), + }, + time_signature: None, + explicit_number: Some((m + 1) as u32), + number_visibility: Default::default(), + }); + } + instances.push(instance); + } + + let region = epiphany_core::Region { + id: region_id, + time_model: RegionTimeModel::Metric(MetricTimeModel::default()), + content: RegionContent::StaffBased(StaffBasedContent { + staff_instances: instances, + ..Default::default() + }), + time_extent: TimeExtent { + start: TimeAnchor::WallClock { + time: WallClockTime(0), + }, + end: TimeAnchor::WallClock { + time: WallClockTime(120_000_000), + }, + }, + staff_extent: StaffExtent { + staves: vec![top_staff, bottom_staff], + }, + local_tempo_map: None, + permits_spanning_slurs: false, + }; + + let staff = |id: StaffId, name: &str| Staff { + id, + name: String::from(name), + abbreviation: None, + instrument, + default_staff_lines: StaffLineConfiguration::default(), + group: None, + default_clef: epiphany_core::Clef::treble(), + }; + let mut score = Score::empty(idc.clone()); + score.identity = idc; + score.instruments = vec![epiphany_core::Instrument::new( + instrument, + String::from("Keyboard"), + )]; + score.staves = vec![staff(top_staff, "Right"), staff(bottom_staff, "Left")]; + score.events = arena; + score.canvas = Canvas { + regions: vec![region], + ..Default::default() + }; + score +} + +/// A two-staff score whose LOWER staff engraves **no glyphs at all**: it is a +/// percussion-clef placeholder — a staff instance with a `ClefChange` to +/// `ClefShape::Percussion`, which has no bundled SMuFL glyph (it engraves to a +/// traced anchor stroke), and no voices or measures. Its vertical band therefore +/// owns five staff-line strokes plus that anchor, and **zero** members. +/// +/// The upper staff carries twelve plain measures of C4, so it wraps and its gap +/// to the placeholder is slack everywhere. Together: a valid score on which any +/// consumer that identifies a region's staff bands by their glyph `members` +/// silently loses the lower staff. Invariant-clean. +pub fn percussion_placeholder_staff(seed: u64) -> Score { + let mut rng = SplitMix64::new(seed ^ 0x0005_57AF_F123); + let replica = + ReplicaId::from_entropy(rng.next_u64().to_le_bytes()).unwrap_or(ReplicaId(0x5ADF)); + let mut idc = IdentityContext::new(replica); + + let top_staff: StaffId = idc.mint(); + let drum_staff: StaffId = idc.mint(); + let instrument: InstrumentId = idc.mint(); + let region_id: RegionId = idc.mint(); + + const MEASURES: i64 = 12; + const BEATS: i64 = 4; + + let mut arena = EventArena::new(); + let top_instance: StaffInstanceId = idc.mint(); + let top_voice: VoiceId = idc.mint(); + let mut voice = Voice::user(top_voice); + for index in 0..(MEASURES * BEATS) { + let (eid, pid): (EventId, PitchId) = (idc.mint(), idc.mint()); + arena.insert(quarter(eid, top_voice, pid, index)).unwrap(); + voice.events.push(eid); + } + let mut top = StaffInstance::new(top_instance, top_staff); + top.voices.push(voice); + for m in 0..MEASURES { + top.measures.push(Measure { + id: idc.mint(), + start: TimeAnchor::Region { + id: region_id, + edge: RegionEdge::Start, + offset: AnchorOffset::Musical(MusicalDuration(RationalTime::new(m, 1).unwrap())), + }, + time_signature: None, + explicit_number: Some((m + 1) as u32), + number_visibility: Default::default(), + }); + } + + // The placeholder: a percussion clef, no voices, no measures. + let drum_instance: StaffInstanceId = idc.mint(); + let mut drums = StaffInstance::new(drum_instance, drum_staff); + drums.clef_sequence.push(epiphany_core::ClefChange { + anchor: TimeAnchor::WallClock { + time: WallClockTime(0), + }, + clef: epiphany_core::Clef { + shape: epiphany_core::ClefShape::Percussion, + line: 3, + octave_shift: 0, + }, + }); + + let region = epiphany_core::Region { + id: region_id, + time_model: RegionTimeModel::Metric(MetricTimeModel::default()), + content: RegionContent::StaffBased(StaffBasedContent { + staff_instances: vec![top, drums], + ..Default::default() + }), + time_extent: TimeExtent { + start: TimeAnchor::WallClock { + time: WallClockTime(0), + }, + end: TimeAnchor::WallClock { + time: WallClockTime(120_000_000), + }, + }, + staff_extent: StaffExtent { + staves: vec![top_staff, drum_staff], + }, + local_tempo_map: None, + permits_spanning_slurs: false, + }; + + let staff = |id: StaffId, name: &str, clef: epiphany_core::Clef| Staff { + id, + name: String::from(name), + abbreviation: None, + instrument, + default_staff_lines: StaffLineConfiguration::default(), + group: None, + default_clef: clef, + }; + let mut score = Score::empty(idc.clone()); + score.identity = idc; + score.instruments = vec![epiphany_core::Instrument::new( + instrument, + String::from("Ensemble"), + )]; + score.staves = vec![ + staff(top_staff, "Melody", epiphany_core::Clef::treble()), + staff( + drum_staff, + "Drums", + epiphany_core::Clef { + shape: epiphany_core::ClefShape::Percussion, + line: 3, + octave_shift: 0, + }, + ), + ]; + score.events = arena; + score.canvas = Canvas { + regions: vec![region], + ..Default::default() + }; + score +} + /// A 10-measure, single-staff, single-voice metric score with 40 quarter notes /// (four per measure), plus a tie, a spanner, a marker, and a chord symbol. The /// QUICKSTART layout hand-off case. Invariant-clean (the returned graph passes @@ -613,6 +849,55 @@ mod tests { assert_eq!(s.cross_cutting.slurs.len(), 1, "a slur over the high notes"); } + #[test] + fn percussion_placeholder_staff_is_invariant_clean_and_glyphless_below() { + use epiphany_layout_ir::{to_constrained, to_logical, VerticalBandKind}; + let s = percussion_placeholder_staff(1); + let v = check_invariants(&s); + assert!(v.is_empty(), "percussion fixture has violations: {v:?}"); + let c = to_constrained(&to_logical(&s)); + let glyphless: Vec<_> = c + .vertical_bands + .iter() + .filter(|b| matches!(b.kind, VerticalBandKind::Staff(_))) + .filter(|b| !c.glyphs.iter().any(|g| g.vertical_band == b.id)) + .collect(); + assert_eq!( + glyphless.len(), + 1, + "exactly one staff band owns no glyph (the percussion placeholder)" + ); + assert!( + glyphless[0].members.is_empty(), + "and therefore no band members either" + ); + assert!( + c.strokes + .iter() + .filter(|st| st.vertical_band == glyphless[0].id) + .count() + >= 5, + "but it does own its staff lines" + ); + } + + #[test] + fn two_staff_wrapping_pressure_is_invariant_clean_and_front_loaded() { + let s = two_staff_wrapping_pressure(1); + let v = check_invariants(&s); + assert!( + v.is_empty(), + "wrapping two-staff fixture has violations: {v:?}" + ); + assert_eq!(s.staves.len(), 2, "two staves"); + assert_eq!(s.events.len(), 96, "12 measures x 4 beats x 2 staves"); + assert_eq!( + s.canvas.regions[0].staff_instances().len(), + 2, + "two staff instances in ONE region -- so both staves share every system" + ); + } + #[test] fn three_staff_close_content_is_invariant_clean_and_asymmetric() { let s = three_staff_close_content(1); diff --git a/spec/PASS12_RATIFICATION_LOG.md b/spec/PASS12_RATIFICATION_LOG.md index f7f4c19..be1e9b6 100644 --- a/spec/PASS12_RATIFICATION_LOG.md +++ b/spec/PASS12_RATIFICATION_LOG.md @@ -306,13 +306,13 @@ that was an implementation defect, not a definition defect. | "Content extent" was ambiguous | **clarify (editorial)** — `req:qmc:vertical` now spells out that content extent means every primitive the band owns, each attributed by its declared `vertical_band`, and not the band's glyph `members`. Before primitive band ownership this reading was arguably unimplementable, which is why the defect survived. The stale rationale (claiming the vertical spring solve is deferred) is refreshed, and the inter-system half of the axis is recorded as a genuine trade-off against `page_fill_efficiency`, not a defect | quality_metric_catalog §`vertical_density_penalty` rationale | — | | Solve read the gap from a constructor | **fix** — the inter-staff solve now targets the `preferred_height` of the `InterStaffGap` band `to_constrained` emitted for that staff pair, not `VerticalBand::inter_staff_gap`'s default. The band is now a height model: a region declaring a wider gap gets one, and the solve and the metric agree by construction rather than by both calling the same constructor | — (behavioural, within `req:layoutir:vertical-bands`) | `epiphany-engrave` (`casting.rs`) | -**Version movements.** None. Quality Metric Catalog stays **0.2.0** (formula, -units, anchor, normalization unchanged — a clarification and a rationale refresh -are not a definition change; contrast P12-I12, which redefined -`spacing_distortion`'s unit and did move the version). Core spec unchanged. -Operation Catalog 0.7.0, Binary Format 0.6.0 unchanged. **No layout change and no -render-golden churn** — this is measurement-only, as the ENGRAVER_VERSION staying -at 11 records. +**Version movements.** Quality Metric Catalog **0.2.0 → 0.3.0** — see the review +follow-up below. (The conformance fix above needed none on its own: formula, +anchor, and normalization were unchanged, and a clarification is not a definition +change. The follow-up's per-realization unit count *is* one, so the version moves +with it.) Core spec unchanged. Operation Catalog 0.7.0, Binary Format 0.6.0 +unchanged. **No layout change and no render-golden churn** — measurement-only, as +`ENGRAVER_VERSION` staying at 11 records. **Deferred (documented, not open candidates).** Staff-less content placed *between* two staves would hold still while the lower staff descends away from @@ -320,3 +320,39 @@ it. No primitive can name an `InterStaffGap` band today (`band_of` yields a staf band or the region's margin band), so this is unreachable rather than latent; building the machinery now would be speculative. Named in `epiphany-engrave/DECISIONS.md`. + +### Review follow-up (2026-07-09) — two more glyph-members assumptions + +An adversarial review of the fix above found the correction incomplete. Both +findings were right, and the second falsified a comment I had written in the same +commit. + +| Item | Disposition | Spec locus | Consumer | +|---|---|---|---| +| Region staff bands identified by glyph `members` | **fix (conformance)** — `vertical_raw` measured content over all primitives but still decided *which* staff bands belong to a region by glyph membership, reintroducing the assumption the change exists to shed. A staff band is allowed to own no glyphs: `to_constrained` emits one per staff of the region regardless, and a percussion-clef staff (no bundled glyph → traced anchor stroke) with no notes owns only its staff-line strokes. Region membership now comes from content presence in one of the region's systems — a staff band is per-(staff, region), so this identifies it exactly. Locked by the new `percussion_placeholder_staff` fixture and `a_staff_band_owning_no_glyphs_still_contributes_its_gap` | — (conformance to `req:qmc:vertical`) | `epiphany-engrave` (`quality.rs::vertical_raw`) | +| Only the first realizing system measured | **fix + catalog definition move (0.3.0)** — the code measured one system per gap band, justified by a comment claiming rigid system translation makes every realization agree. The inter-staff solve had *just* falsified that: it sizes each system's gaps from that system's own content. `req:qmc:vertical` now counts **one unit per realization**, matching how realized inter-system gaps were already counted. Locked by the new `two_staff_wrapping_pressure` fixture (staff-line gap 15.93 in the pressured system, 7.87 in the slack one) and `inter_staff_gaps_are_measured_in_every_system_that_realizes_them` | quality_metric_catalog §`vertical_density_penalty` (`req:qmc:vertical`), **0.2.0 → 0.3.0** | `epiphany-engrave` | + +Both fixes are mutation-verified: reverting to glyph-member region identification +scores `4.8e-7` on the percussion fixture, and reverting to first-system-only +scores `1.3e-7` on the wrapping fixture — each ~0 where the corrected axis reports +real deviation. + +**What the per-realization count exposes.** `two_staff_wrapping_pressure` now +scores `vertical_density_penalty` **0.739**: its pressured system solves to the +declared gap exactly (≈0 deviation), while its slack system sits at ≈5 staff +spaces of content gap against a preferred 2.0. The axis is symmetric — a gap +wider than preferred is sprawl exactly as a narrower one is crowding — and the +inter-staff solve **only expands, never compresses**. That deferral +(`epiphany-engrave/DECISIONS.md`, "compressing an OVER-wide fixed gap toward +preferred… rarely wanted") is hereby promoted from *rarely wanted* to +*measurably wrong*: any un-pressured multi-staff system reports honest sprawl +until the solve can pull staves together. Named, not fixed — compression is a +layout change (golden churn, `ENGRAVER_VERSION` move), not a measurement one. + +**Adjacent finding, not fixed here.** `Staff::default_clef` is never consulted by +the projection: `to_constrained` takes the active clef from the staff instance's +`clef_sequence` and falls back to `Clef::default()` (treble). A bass-clef staff +that declares its clef only via `Staff::default_clef` engraves as treble. Found +while building the percussion fixture (which therefore had to declare its clef as +a `ClefChange`). Filed in `epiphany-layout-ir/DECISIONS.md`; parked with the +`ConstrainedLayoutIR` listing gap pending a ≥3-candidate Pass-13 batch. diff --git a/spec/quality_metric_catalog.pdf b/spec/quality_metric_catalog.pdf index 512b593..d1965b2 100644 Binary files a/spec/quality_metric_catalog.pdf and b/spec/quality_metric_catalog.pdf differ diff --git a/spec/quality_metric_catalog.tex b/spec/quality_metric_catalog.tex index 28fb899..048316b 100644 --- a/spec/quality_metric_catalog.tex +++ b/spec/quality_metric_catalog.tex @@ -226,7 +226,7 @@ {\Large\scshape\color{epiphanyslate}Quality Metric Catalog}\\[6pt] {\large\itshape\color{epiphanyslate}A companion to the Core Specification}\\[14pt] {\color{epiphanygold}\rule{3in}{0.8pt}}\\[24pt] - {\normalsize\color{epiphanyink}Version 0.2.0 --- Phase 3 (the normative metric set: formal definitions, normalization, weights, tier thresholds, profile registry; \texttt{spacing\_distortion} scoped to rhythmic columns)}\\[4pt] + {\normalsize\color{epiphanyink}Version 0.3.0 --- Phase 3 (the normative metric set: formal definitions, normalization, weights, tier thresholds, profile registry; \texttt{spacing\_distortion} scoped to rhythmic columns; \texttt{vertical\_density\_penalty} counted per realization)}\\[4pt] {\small\color{epiphanyslate}Normative for the metrics and thresholds it defines} \vfill \end{titlepage} @@ -246,7 +246,7 @@ deliver ``per-metric normalization functions mapping raw measurements to metric thresholds, and the formal definition of each quality metric in the normative metric set.'' -This companion (v0.2.0) delivers all four chartered items, plus two small +This companion (v0.3.0) delivers all four chartered items, plus two small registries the core specification names but defers here: \begin{itemize} @@ -868,10 +868,15 @@ inter-system gaps realized far from the spacing the band model asked for. \begin{requirement} \label{req:qmc:vertical} -\textbf{Contributing units:} vertical bands of $C$ of kind -\texttt{InterStaffGap} or \texttt{InterSystemGap} with preferred height -$p > 0$ that are realized in $L$ (the adjacent content they separate was -laid out). +\textbf{Contributing units:} each \emph{realization} in $L$ of a vertical +band of $C$ of kind \texttt{InterStaffGap} or \texttt{InterSystemGap} with +preferred height $p > 0$ (a band is realized wherever the adjacent content +it separates was laid out). A band realized in several systems contributes +\textbf{one unit per system}, not one per band: a vertical solve sizes each +system's gaps from that system's own content, so one band's realized height +genuinely differs from system to system. This matches how the realized +inter-system gaps are already counted --- one unit per adjacent system pair +on a page. \textbf{Raw measurement:} per unit, with $r \ge 0$ the realized vertical separation between the adjacent content extents the band separates @@ -918,6 +923,18 @@ The inter-system half of the axis is a genuine trade-off, not a defect: the vertical justification pass deliberately stretches inter-system gaps past preferred to fill a non-final page, trading this axis against \texttt{page\_fill\_efficiency}. Both axes are reported; neither is wrong. + +\textbf{Per-realization units} (0.3.0) followed from the same landing. While +the reference pipeline preserved constrained $y$ verbatim, a band had exactly +one realized height and the distinction was moot. Once the inter-staff solve +began sizing each system's gaps from that system's own content, a band's +realized height became per-system, and counting one unit per band would let a +well-solved system average away a badly spaced one. The axis is deliberately +symmetric: a gap wider than preferred is sprawl exactly as a narrower one is +crowding. A solver that only ever \emph{expands} a fixed stacking --- never +compressing an over-wide gap back toward preferred --- will therefore report +honest sprawl on its slack systems. That is the axis working, not +mis-measuring. \end{rationale} \section{\texttt{system\_break\_penalty}} @@ -1366,6 +1383,22 @@ than inventing a parallel one. irregularity. Normalization anchor, orientation, range, thresholds, and the eight other axes are unchanged; the optical-spacing open question (duration-proportional spacing) stays open. Batch item P12-I12. \\ + \midrule + \today & \hyperref[sec:metrics:vertical]{\texttt{vertical\_density\_penalty}} + & 0.3.0 --- Count \textbf{one contributing unit per realization} of an + \texttt{InterStaffGap} band rather than one per band, matching how realized + inter-system gaps were already counted. The inter-staff vertical solve sizes + each system's gaps from that system's own content, so a band's realized + height is per-system; one unit per band let a well-solved system average away + a badly spaced one. Also \emph{clarifies} (no semantic change) that the + ``content extents'' the raw measurement compares are every primitive the + adjacent band owns --- glyphs, strokes, and curves, each attributed by its + declared \texttt{vertical\_band} --- and not the band's glyph + \texttt{members}, a reading that was arguably unimplementable before + primitive band ownership was ratified. Raw formula, normalization anchor, + orientation, range, thresholds, and the eight other axes are unchanged. The + reference implementation's non-conforming glyph-only measurement is fixed + alongside. \\ \bottomrule \end{longtable}