From b1bfe04d6ccad7eb24f2a772228c76be51762749 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 10:05:14 -0400 Subject: [PATCH] Inter-staff review fix: attribute a curve by its arc, not its nearest glyph The stem tear-off repaired in 4132a7a had a twin one layer deeper, live in the same fixture and baked into the same golden: the bottom staff's slur was attributed to the TOP staff, kept shift 0, and tore off its own notes. A distance metric cannot fix this one. A slur's start endpoint is deliberately lifted off its notes -- staff_top + gap above, staff_bottom - gap below -- into the inter-staff zone, where the nearest glyph is routinely a note on the adjacent staff (here, a top-staff ledger note). So attribute a curve the way it is drawn: the arc's direction picks the side. An upward arc (p1.y >= p0.y) hangs below a staff -- take the greatest staff-line band bottom at or above p0.y; a downward arc sits above one -- take the smallest band top at or below p0.y. Fall back to the nearest band mid-line for a curve inside a staff. Locked by a_slur_travels_with_its_own_staff, verified to fail without the fix (d_bottom=8.695 > d_top=7.235 -- the slur riding the wrong staff). Also assert the two metrics the solve moves rather than leaving them untested: collision_penalty is 0 (the staves separate cleanly) and vertical_density_penalty saturates at 1.0 -- the solve targets content extents while the metric scores the realized gap against the band model's preferred height. That is the same metric-vs-solver tension as casting_off under justification; DECISIONS.md records it, plus three further reviewed gaps: staff-less content takes shift 0, the preferred gap is read from the constructor rather than the region's declared band, and the 3+-staff cumulative cascade is correct but unexercised. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/epiphany-engrave/DECISIONS.md | 54 +++++++++++++++-- crates/epiphany-engrave/src/casting.rs | 59 ++++++++++++++---- crates/epiphany-engrave/src/lib.rs | 60 +++++++++++++++++++ .../two_staff_close_content.engrave.svg | 2 +- 4 files changed, 158 insertions(+), 17 deletions(-) diff --git a/crates/epiphany-engrave/DECISIONS.md b/crates/epiphany-engrave/DECISIONS.md index 4a503bf..35758a4 100644 --- a/crates/epiphany-engrave/DECISIONS.md +++ b/crates/epiphany-engrave/DECISIONS.md @@ -712,11 +712,12 @@ attributed to its owning staff: a glyph via its `vertical_band` (`VerticalBandKind::Staff` → `StaffId`); a ledger via its notehead (`owning_glyph`, a shared `Pitch` source); a **stem** — which has no same-source glyph — via the glyph nearest its BASE point *in two dimensions*; a staff line -via its `Staff` provenance source; a slur via the notehead nearest its start -endpoint. Spacing is horizontal-only, so a primitive's y is unchanged from the -source frame the attribution reads, and the resolved and source indices line up. -(The geometric `round(-y / pitch)` alternative was rejected: an extreme ledgered -note on the top or bottom staff rounds to a non-existent neighbour.) +via its `Staff` provenance source; a **curve** via its arc direction against the +staff-line bands (below). Spacing is horizontal-only, so a primitive's y is +unchanged from the source frame the attribution reads, and the resolved and +source indices line up. (The geometric `round(-y / pitch)` alternative was +rejected: an extreme ledgered note on the top or bottom staff rounds to a +non-existent neighbour.) **Why staff attribution must be y-aware (review fix).** The first cut reused `component_glyph`, whose fallback picks the nearest glyph **by x alone**. That is @@ -733,6 +734,49 @@ attributed primitives contribute their y ONLY through the shifted path (`Extent::add_x` for x, `add_y` for the shifted staff extent), so a lower staff's unshifted content can no longer inflate a system's `max_y`. +**Why a curve cannot use ANY nearest-glyph rule (second review fix).** A slur is +the same bug class one layer deeper, and no distance metric can fix it. A slur's +start endpoint is deliberately *lifted off* its notes — `staff_top + gap` above, +`staff_bottom - gap` below — into the inter-staff zone, where the nearest glyph +is frequently a note on the ADJACENT staff (in `two_staff_close_content`, a top- +staff ledger note). The bottom staff's slur was therefore attributed to the top +staff, kept shift 0, and tore off its own notes — baked into the golden, +byte-identical to the pre-solve slur path. The rule is now the one that reads the +geometry as drawn: **the arc's direction picks the side.** `p1.y >= p0.y` means +the curve arcs upward, so it hangs BELOW a staff → take the staff whose staff- +line band bottom is greatest among those at or above `p0.y`; otherwise it arcs +downward, sitting ABOVE a staff → take the staff whose band top is smallest among +those at or below `p0.y`. Falls back to the nearest band mid-line when neither +side matches (a curve inside a staff). Locked by `a_slur_travels_with_its_own_staff` +(the slur's endpoint band-gap to the bottom staff is smaller than to the top, and +under a staff space of margin). + +**Known gaps (reviewed, not bugs today).** + +- `vertical_density_penalty` **saturates at 1.0** on the two-staff fixture. The + solve targets *content* extents; the metric scores the realized gap against the + band model's *preferred* height (2 staff spaces). The separation the colliding + ledgers and slur require dwarfs that, so the axis pins and its floor diagnostic + fires. This is the same metric-vs-solver tension as `casting_off_quality` under + justification and inter-system density under vertical justification: a catalog + refinement scoring only the EXCESS beyond the content-required minimum is the + deferred follow-up. Asserted, not silently tolerated, in + `inter_staff_solve_separates_colliding_staves`. +- **Staff-less content does not move.** Margin-band glyphs, and spanning strokes + whose source is `RepeatStructure` (volta brackets), attribute to no staff, take + shift 0, and stay put while the staves below them descend. Symmetrically, a + volta stroke that happens to sit near a notehead is attributed to *that* staff + by the 2-D nearest fallback — possibly the wrong one. No fixture exercises + either; both want the band model to carry the attribution outright. +- The preferred gap is read from `VerticalBand::inter_staff_gap(VerticalBandId(0))` + rather than from the region's *declared* inter-staff band. Harmless while both + come from the same constructor; it would silently diverge from the metric the + day per-region gaps become customizable. +- The cumulative shift **cascades** correctly for 3+ staves (staff *i* carries the + sum of every gap correction above it), but is unexercised: `valid_score_rich`'s + three staves are three separate single-staff regions, so each lands in its own + system. + **The solve.** Per system, per staff, the real content y-extent is collected (glyphs, strokes, curves — ledgers and slurs included, not just noteheads). The staves are ordered top-to-bottom by their staff-line reference y and that order diff --git a/crates/epiphany-engrave/src/casting.rs b/crates/epiphany-engrave/src/casting.rs index 4f3a616..7037b31 100644 --- a/crates/epiphany-engrave/src/casting.rs +++ b/crates/epiphany-engrave/src/casting.rs @@ -676,22 +676,59 @@ pub(crate) fn cast_off( .and_then(glyph_band_staff), }) .collect(); + // Each staff's staff-line band, in the source frame (a staff's lines sit at + // the same y in every system, since the constrained stage stacks them once). + let mut staff_lines: BTreeMap = BTreeMap::new(); + for s in &input.strokes { + if let TypedObjectId::Staff(st) = s.provenance.source { + let (lo, hi) = (s.from.y.0.min(s.to.y.0), s.from.y.0.max(s.to.y.0)); + staff_lines + .entry(st) + .and_modify(|e| { + e.0 = e.0.min(lo); + e.1 = e.1.max(hi); + }) + .or_insert((lo, hi)); + } + } + // A slur's staff. Its endpoints are LIFTED clear of their own staff — an + // above-slur sits `STAFF_HEIGHT + gap` over the top line, a below-slur a gap + // under the bottom one — so they land in the inter-staff zone and the + // notehead nearest `p0` can belong to the ADJACENT staff (the same trap the + // stroke attribution fell into, and a WORSE one: the lift is by design, so + // no distance metric can recover the staff). Use the arc's direction, which + // the control point gives (`p1.y > p0.y` ⇔ above), against the staff-line + // bands: an ABOVE slur belongs to the nearest staff whose top line is at or + // below its endpoints; a BELOW slur to the nearest staff whose bottom line is + // at or above them. That is exact for any lift, with no constants shared. + let eps = 1e-3_f32; let curve_staff_of: Vec> = input .curves .iter() .map(|c| { - let (px, py) = (c.p0.x.0, c.p0.y.0); - input - .glyphs - .iter() - .filter(|g| g.glyph.as_str().starts_with("notehead")) - .min_by(|a, b| { - let d = |g: &GlyphObject| { - (g.baseline.x.0 - px).powi(2) + (g.baseline.y.0 - py).powi(2) - }; - d(a).total_cmp(&d(b)) + let p0y = c.p0.y.0; + let above = c.p1.y.0 >= p0y; + let picked = if above { + staff_lines + .iter() + .filter(|(_, (_, hi))| *hi <= p0y + eps) + .max_by(|a, b| a.1 .1.total_cmp(&b.1 .1)) + } else { + staff_lines + .iter() + .filter(|(_, (lo, _))| *lo >= p0y - eps) + .min_by(|a, b| a.1 .0.total_cmp(&b.1 .0)) + }; + // A slur clear of every staff on its arc side (no staff below an + // above-slur) falls back to the staff whose band is nearest. + picked + .or_else(|| { + staff_lines.iter().min_by(|a, b| { + let d = |e: &(f32, f32)| (((e.0 + e.1) * 0.5) - p0y).abs(); + d(a.1).total_cmp(&d(b.1)) + }) }) - .and_then(glyph_band_staff) + .map(|(st, _)| *st) }) .collect(); diff --git a/crates/epiphany-engrave/src/lib.rs b/crates/epiphany-engrave/src/lib.rs index 97535a9..1802b81 100644 --- a/crates/epiphany-engrave/src/lib.rs +++ b/crates/epiphany-engrave/src/lib.rs @@ -2233,6 +2233,52 @@ mod tests { } } + #[test] + fn a_slur_travels_with_its_own_staff() { + use epiphany_layout_ir::{to_constrained, to_logical}; + // Review-found bug — the TWIN of the stem tear-off, and a worse one: a + // slur's endpoints are LIFTED clear of their own staff by construction + // (an above-slur sits a staff-height plus a gap over the top line), so + // they land in the inter-staff zone where the notehead nearest `p0` + // belongs to the ADJACENT staff. The slur was attributed to the top + // staff, kept its shift of 0, and tore off its own notes — its baked + // path was byte-identical to the pre-solve golden. No distance metric + // can recover the staff (the lift is by design), so attribution now + // keys on the arc's direction against the staff-line bands. + let report = Engraver::default().solve( + &to_constrained(&to_logical( + &epiphany_testkit::fixtures::two_staff_close_content(1), + )), + &SolverConfig::default(), + ); + let sys = &report.layout.pages[0].systems[0]; + assert_eq!(report.layout.curves.len(), 1, "the fixture draws one slur"); + let p0y = report.layout.curves[0].p0.y.0; + let band_gap = |b: &epiphany_layout_ir::Rect| { + let (lo, hi) = (b.origin.y.0, b.origin.y.0 + b.size.height.0); + if p0y < lo { + lo - p0y + } else if p0y > hi { + p0y - hi + } else { + 0.0 + } + }; + // Staff records are top-first; the slur belongs to the BOTTOM staff. + let (d_top, d_bottom) = ( + band_gap(&sys.staves[0].bounding_box), + band_gap(&sys.staves[1].bounding_box), + ); + assert!( + d_bottom < d_top, + "the slur rides its OWN (bottom) staff: d_bottom={d_bottom} d_top={d_top}" + ); + assert!( + d_bottom < 2.0, + "and sits just outside that staff's lines: {d_bottom}" + ); + } + #[test] fn multi_staff_stems_stay_on_their_own_staff() { use epiphany_layout_ir::{to_constrained, to_logical}; @@ -2308,6 +2354,20 @@ mod tests { gap > 12.0, "the colliding staves separate: staff-line gap {gap}" ); + assert_eq!( + report.metric_vector.collision_penalty.0, 0.0, + "the separated staves collide nowhere" + ); + // The solve targets CONTENT extents, while `vertical_density_penalty` + // measures the realized gap against the band model's *preferred* height + // (2 staff spaces). The separation this fixture requires is far larger, + // so the deviation-from-preferred axis saturates at 1.0 and its floor + // diagnostic fires. Scoring only the EXCESS beyond the content-required + // minimum is the deferred catalog refinement (see DECISIONS.md). + assert_eq!( + report.metric_vector.vertical_density_penalty.0, 1.0, + "the required separation saturates the deviation-from-preferred axis" + ); // A no-pressure single-staff score has no inter-staff pair, so it is // untouched — its goldens stay byte-stable (see the render-svg suite). let single = diff --git a/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.svg b/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.svg index 6a6502e..bdf19b8 100644 --- a/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.svg +++ b/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.svg @@ -44,7 +44,7 @@ - +