From fea446a182aec5a5dc208604e5f159cf412accf0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 8 Jul 2026 19:54:18 -0400 Subject: [PATCH] Justification review fix: keep stems attached to their noteheads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review of the justification commit found a SEVERE bug: stems detach from their noteheads (~0.75 ss, up to ~1.5) in every justified system. Root cause: the code used is_rigid_width_stroke to select slot-anchored strokes on the false premise it covered stems. It is LEDGER-ONLY. A stem is an Event-sourced stroke drawn at notehead_x + 1.15 with no same-source glyph (noteheads are Pitch-sourced) and no baseline in its x-span, so it fell to the affine branch and its intra-slot offset was scaled by the justification factor a, floating it off its head into the gap. The spacing pass shared the same classification (a smaller latent drift). Fix: component_glyph classifies a stroke — a Staff (staff line) or RepeatStructure (volta bracket, whose ending-number glyphs share its source) source SPANS (affine); else owning_glyph (a ledger over its notehead, same Pitch source); else the glyph with the greatest baseline <= the stroke's x — a stem's own in-column notehead (stem_offset 1.15 < column step 1.6, so exactly its slot). Applied in BOTH the spacing remap and casting, so stems ride their heads through the whole pipeline. Ledgers are unchanged (owning_glyph path). Regression: stem_offsets_from_the_notehead_survive_justification (verified to fail without the fix). Goldens regenerated (stems now on their heads). The minor slur-inset drift (same root cause, ~0.3 ss on a soft connector) is deferred with a note. Folded into ENGRAVER_VERSION 8 (unreleased). 941 tests, clippy 0, docs -D warnings, conformance 8/8. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/epiphany-engrave/DECISIONS.md | 35 ++++- crates/epiphany-engrave/src/casting.rs | 22 ++- crates/epiphany-engrave/src/lib.rs | 137 +++++++++++++++--- .../ten_measure_single_staff.engrave.svg | 80 +++++----- .../ten_measure_with_repeats.engrave.svg | 80 +++++----- .../golden/ten_measure_with_slurs.engrave.svg | 80 +++++----- .../tests/golden/valid_score_rich.engrave.svg | 14 +- 7 files changed, 286 insertions(+), 162 deletions(-) diff --git a/crates/epiphany-engrave/DECISIONS.md b/crates/epiphany-engrave/DECISIONS.md index 10cef7d..521b7a4 100644 --- a/crates/epiphany-engrave/DECISIONS.md +++ b/crates/epiphany-engrave/DECISIONS.md @@ -546,11 +546,36 @@ per-slot deltas at the first/last slots, and the justified ink spans exactly SLOT's source (`Placement::slot_dx`), constant per slot, so intra-slot offsets (a time signature after its barline, an accidental left of its notehead) survive verbatim — never scaled by `a`. A spanning stroke (staff line, volta bracket) -maps each endpoint through the affine (it stretches). A rigid-width stroke (a -stem or ledger, via `owning_glyph`) translates by its owning slot's delta, so it -stays attached without stretching. A slur's control points map straight through -the affine — its endpoints follow their anchor notes and the arc stretches with -the span. +maps each endpoint through the affine (it stretches). A per-event **component +stroke** (a stem or ledger) translates by its owning slot's delta, so it stays +attached without stretching its offset. + +**Component-stroke classification (`component_glyph`), a review fix.** The first +justification cut used `is_rigid_width_stroke` (LEDGER-only) to pick slot-anchored +strokes, on the false premise that it also covered stems. It does not: a stem is +an `Event`-sourced stroke drawn at `notehead_x + 1.15` — no same-source glyph +(noteheads are `Pitch`-sourced) and no baseline in its x-span — so it fell to the +affine branch and its offset was scaled by `a`, detaching it ~0.75 ss (up to +~1.5) from its head in every justified system (and, latently, a smaller drift in +the spacing pass, which had the same classification). `component_glyph` now +classifies a stroke: a `Staff` (staff line) or `RepeatStructure` (volta bracket, +whose ending-number glyphs share its source) source SPANS → affine; else the +`owning_glyph` (a ledger over its notehead, same `Pitch` source, overlapping in +x); else the glyph with the greatest baseline ≤ the stroke's x — a stem's own +in-column notehead/dot, all in the one slot (`stem_offset 1.15 < column step +1.6`, so this is exactly the stem's slot). Applied in BOTH the spacing remap and +casting, so stems stay on their heads through the whole pipeline. Locked by +`stem_offsets_from_the_notehead_survive_justification`. + +**A slur's control points map straight through the affine** — its endpoints +follow their anchor notes and the arc stretches with the span. **Known minor gap +(deferred):** the endpoints carry an authored `SLUR_INSET` (0.6 ss) tuck from +their notes; the affine scales that offset by `a`, so a slur in a stretched +system tucks ~`0.6·(a−1)` further from its heads (≈0.3 ss at `a≈1.5`) — still +reading as "near" the note, same root cause as the stem drift but on a soft +connector. A proper fix would slot-anchor `p0`/`p3` to their event slots while +stretching the interior; deferred until slur fidelity warrants it (the curve +carries no per-endpoint slot today). **Which systems justify.** Not the last system of a region (ragged-right, as engraving convention wants); not a system with no finite width target, a diff --git a/crates/epiphany-engrave/src/casting.rs b/crates/epiphany-engrave/src/casting.rs index 99f9830..fc00bfe 100644 --- a/crates/epiphany-engrave/src/casting.rs +++ b/crates/epiphany-engrave/src/casting.rs @@ -1604,11 +1604,11 @@ fn justify_system( } } -/// Places a whole stroke under a system's justification. A rigid-width stroke -/// (a stem or ledger) tracks its notehead: both endpoints translate by the -/// owning slot's delta, so it stays attached without stretching. A spanning -/// stroke (a staff line, a volta bracket) stretches with the system: each -/// endpoint maps through the affine. +/// Places a whole stroke under a system's justification. A per-event component +/// stroke (a stem or ledger) tracks its notehead: both endpoints translate by +/// the owning slot's delta, so it stays attached without stretching its offset. +/// A spanning stroke (a staff line, a volta bracket) stretches with the system: +/// each endpoint maps through the affine. fn place_stroke( source: &Stroke, spaced: &Stroke, @@ -1616,13 +1616,11 @@ fn place_stroke( slot_source_x: &BTreeMap, glyphs: &[GlyphObject], ) -> Stroke { - if is_rigid_width_stroke(source) { - if let Some(dx) = owning_glyph(source, glyphs) - .and_then(|g| slot_source_x.get(&g.horizontal_slot)) - .map(|&sx| p.slot_dx(sx)) - { - return translated(spaced, dx, p.dy); - } + if let Some(dx) = crate::component_glyph(source, glyphs) + .and_then(|g| slot_source_x.get(&g.horizontal_slot)) + .map(|&sx| p.slot_dx(sx)) + { + return translated(spaced, dx, p.dy); } Stroke { provenance: spaced.provenance.clone(), diff --git a/crates/epiphany-engrave/src/lib.rs b/crates/epiphany-engrave/src/lib.rs index a92b467..4a6a646 100644 --- a/crates/epiphany-engrave/src/lib.rs +++ b/crates/epiphany-engrave/src/lib.rs @@ -78,6 +78,7 @@ mod spacing; use std::collections::{BTreeMap, BTreeSet}; +use epiphany_core::TypedObjectId; use epiphany_layout_ir::{ all_available, profile_thresholds, Axis, BravuraCatalog, ConstrainedLayoutIR, ConstraintId, ConstraintSolver, ConstraintStrength, Curve, GlyphCatalog, GlyphObject, GlyphObjectId, @@ -106,6 +107,50 @@ pub(crate) fn owning_glyph<'a>( }) } +/// The glyph a per-event COMPONENT stroke belongs to — the notehead that shares +/// its `Event` source — found by source ALONE, so it also catches a **stem**, +/// which sits offset from its column (at `notehead_x + stem_offset`) and whose +/// x therefore contains no glyph baseline (`owning_glyph`'s x-span test misses +/// it). Such a stroke tracks its notehead's slot rigidly, so re-spacing and +/// justification move it *with* its head instead of stretching its offset. A +/// stroke whose source is not an `Event` — a staff line (`Staff`), a volta +/// bracket (a `RepeatStructure`, a source its ending-number glyphs also carry) — +/// has no same-slot owner here and stretches with its system instead. (Every +/// event-sourced stroke today is a single-slot component: stem, ledger, or a +/// zero-extent anchor. A future event-spanning stroke — a beam — would need a +/// span-aware guard added here.) +pub(crate) fn component_glyph<'a>( + stroke: &Stroke, + glyphs: &'a [GlyphObject], +) -> Option<&'a GlyphObject> { + // Spanning strokes stretch with their system, so they own no single slot: a + // staff line (`Staff` source), a volta bracket (`RepeatStructure` source — + // which its ending-number glyphs also carry, so a plain source match would + // wrongly anchor the whole bracket to one number's slot). + if matches!( + stroke.provenance.source, + TypedObjectId::Staff(_) | TypedObjectId::RepeatStructure(_) + ) { + return None; + } + // A ledger shares its notehead's `Pitch` source and overlaps it in x, so + // `owning_glyph` finds it directly. + if let Some(g) = owning_glyph(stroke, glyphs) { + return Some(g); + } + // A stem is `Event`-sourced with NO same-source glyph (noteheads are + // `Pitch`-sourced) and sits offset from its column (`stem_x = notehead_x + + // 1.15`, inside the 1.6 column step), so its x contains no glyph baseline. + // It belongs to the slot just to its LEFT — the glyph with the greatest + // baseline ≤ its x, which is a notehead or dot in the stem's OWN column (all + // of that column's glyphs share the one slot, so the pick's slot is exact). + let x = stroke.from.x.0.max(stroke.to.x.0); + glyphs + .iter() + .filter(|g| g.baseline.x.0 <= x + f32::EPSILON) + .max_by(|a, b| a.baseline.x.0.total_cmp(&b.baseline.x.0)) +} + /// The Epiphany engraving solver (Chapter 9). A `Minimal`-tier solver: it spaces /// glyphs horizontally, casts the result off into systems and pages against its /// [`PageGeometry`], and satisfies the IR's declared hard constraints — break @@ -140,9 +185,13 @@ pub struct Engraver { /// one system is unchanged, and to `8` when **per-system justification** landed /// (every non-final system of a multi-system region stretches its horizontal /// slack so its ink fills the content width, instead of sitting at its natural -/// left-aligned width; the baked geometry of any wrapping score differs, while a -/// single-system score — whose only system is ragged-right by convention — is -/// unchanged). +/// left-aligned width) **together with correct component-stroke slot-anchoring** +/// (a stem — an `Event`-sourced stroke offset from its column, which the old +/// ledger-only rigid-width test missed — now rides its notehead's slot in both +/// the spacing pass and casting, instead of being stretched by the interpolation +/// / justification map and drifting off its head; any wrapping score's baked +/// geometry differs, and any score with drawn stems shifts them onto their +/// heads). pub const ENGRAVER_VERSION: SolverVersion = SolverVersion(8); impl Engraver { @@ -460,21 +509,19 @@ impl HorizontalRemap { .strokes .iter() .map(|s| { - let (from_x, to_x) = if epiphany_layout_ir::is_rigid_width_stroke(s) { - // Translate rigidly by the *owning glyph's* slot delta — found by - // source, not the stroke's midpoint, which for a wide head could - // pick a neighbouring column and reintroduce drift. The slot delta - // is the exact column translation (the same one the glyph itself - // moves by), keeping the stroke's offset from its glyph and its - // length. - let delta = owning_glyph(s, &input.glyphs) - .map(|g| { - self.slot_delta - .get(&g.horizontal_slot) - .copied() - .unwrap_or_else(|| self.map(g.baseline.x.0) - g.baseline.x.0) - }) - .unwrap_or(0.0); + let (from_x, to_x) = if let Some(g) = component_glyph(s, &input.glyphs) { + // A per-event component stroke (a stem, a ledger) translates + // rigidly by its *owning glyph's* slot delta — found by + // source, not the stroke's own x (a stem sits offset from its + // column, so its midpoint could pick a neighbouring slot). The + // slot delta is the exact column translation the glyph itself + // moves by, so the stroke keeps its offset from its head and + // its length. + let delta = self + .slot_delta + .get(&g.horizontal_slot) + .copied() + .unwrap_or_else(|| self.map(g.baseline.x.0) - g.baseline.x.0); (s.from.x.0 + delta, s.to.x.0 + delta) } else { (self.map(s.from.x.0), self.map(s.to.x.0)) @@ -1329,6 +1376,60 @@ mod tests { assert!(checked > 0, "no ledger/notehead pairs exercised"); } + #[test] + fn stem_offsets_from_the_notehead_survive_justification() { + // A stem is `Event`-sourced with no same-source glyph, so it tracks its + // column via `component_glyph`'s nearest-left notehead. Its offset from + // that column must survive the FULL solve — spacing AND per-system + // justification — so a stem in a stretched non-final system stays + // attached to its head rather than being dragged into the gap (the + // review's severe finding). Checked across seeds that wrap (justify). + let mut checked = 0; + for seed in 0..16 { + let input = to_constrained(&to_logical(&valid_score_rich(seed))); + let report = Engraver::default().solve(&input, &SolverConfig::default()); + for s_in in &input.strokes { + // Vertical, non-zero-length strokes are drawn stems. + if (s_in.from.x.0 - s_in.to.x.0).abs() > 1e-4 + || (s_in.from.y.0 - s_in.to.y.0).abs() < 1e-3 + { + continue; + } + let Some(owner_in) = component_glyph(s_in, &input.glyphs) else { + continue; + }; + // A stem's owner is found by nearest-left, NOT source match + // (that path is the ledger case); skip anything same-source. + if owner_in.provenance.source == s_in.provenance.source { + continue; + } + let (Some(s_out), Some(g_out)) = ( + report + .layout + .strokes + .iter() + .find(|s| s.provenance.stable_id == s_in.provenance.stable_id), + report + .layout + .glyphs + .iter() + .find(|g| g.provenance.stable_id == owner_in.provenance.stable_id), + ) else { + continue; + }; + let offset_in = s_in.from.x.0 - owner_in.baseline.x.0; + let offset_out = s_out.from.x.0 - g_out.position.x.0; + assert!( + (offset_out - offset_in).abs() < 1e-3, + "seed {seed}: stem offset drifted {offset_in} -> {offset_out} \ + (justification scaled it off its notehead)" + ); + checked += 1; + } + } + assert!(checked > 0, "no stem/notehead pairs exercised"); + } + #[test] fn adjacent_ledger_lines_are_spaced_not_overlapping() { use std::collections::HashMap; diff --git a/crates/epiphany-render-svg/tests/golden/ten_measure_single_staff.engrave.svg b/crates/epiphany-render-svg/tests/golden/ten_measure_single_staff.engrave.svg index e5d4f22..d850c62 100644 --- a/crates/epiphany-render-svg/tests/golden/ten_measure_single_staff.engrave.svg +++ b/crates/epiphany-render-svg/tests/golden/ten_measure_single_staff.engrave.svg @@ -10,85 +10,85 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + diff --git a/crates/epiphany-render-svg/tests/golden/ten_measure_with_repeats.engrave.svg b/crates/epiphany-render-svg/tests/golden/ten_measure_with_repeats.engrave.svg index e12a30b..66864d8 100644 --- a/crates/epiphany-render-svg/tests/golden/ten_measure_with_repeats.engrave.svg +++ b/crates/epiphany-render-svg/tests/golden/ten_measure_with_repeats.engrave.svg @@ -10,85 +10,85 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + diff --git a/crates/epiphany-render-svg/tests/golden/ten_measure_with_slurs.engrave.svg b/crates/epiphany-render-svg/tests/golden/ten_measure_with_slurs.engrave.svg index 12ab45d..418a821 100644 --- a/crates/epiphany-render-svg/tests/golden/ten_measure_with_slurs.engrave.svg +++ b/crates/epiphany-render-svg/tests/golden/ten_measure_with_slurs.engrave.svg @@ -10,85 +10,85 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + diff --git a/crates/epiphany-render-svg/tests/golden/valid_score_rich.engrave.svg b/crates/epiphany-render-svg/tests/golden/valid_score_rich.engrave.svg index 3af7c31..c8d654c 100644 --- a/crates/epiphany-render-svg/tests/golden/valid_score_rich.engrave.svg +++ b/crates/epiphany-render-svg/tests/golden/valid_score_rich.engrave.svg @@ -10,11 +10,11 @@ - + - + - + @@ -28,9 +28,9 @@ - + - + @@ -38,9 +38,9 @@ - + - +