From 6651ae5fce4f226e946653e4b4ec08fe4e2a6211 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 7 Jul 2026 22:16:54 -0400 Subject: [PATCH] E1 follow-up: slot-relative glyph remap (same-slot spacing preserved) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spacing pass reserves a slot's full content extent in its advance, but HorizontalRemap::glyphs moved every glyph independently by piecewise interpolation — a same-slot companion whose absolute x crossed the next slot's source was dragged by the wrong interval. E1 made it reachable: a time signature after a morphed repeatLeft sits TIME_SIG_X + the sign's right extension (~1.8sp) right of its barline, past the 1.6sp constrained column step, collapsing the digits into the following note through the real Engraver. spacing::space_slots now returns each glyph-bearing slot's (source, target) beside the interpolation control points; glyphs translate by their own slot's rigid delta (intra-slot offsets survive verbatim), spanning strokes keep endpoint interpolation, and rigid ledger strokes use the owning glyph's slot delta exactly. Folded into ENGRAVER_VERSION 4 (unreleased this push); scores without same-slot companions are byte-identical — only the repeat fixture's engrave golden moved (volta digits). Regression: time_signature_digits_ride_their_barline_slot_past_a_repeat_sign (unbounded page so x-disjointness compares one line; verified to fail against the interpolated remap). Co-Authored-By: Claude Fable 5 --- crates/epiphany-engrave/DECISIONS.md | 17 ++ crates/epiphany-engrave/src/lib.rs | 184 ++++++++++++++++-- crates/epiphany-engrave/src/spacing.rs | 77 +++++--- .../ten_measure_with_repeats.engrave.svg | 8 +- 4 files changed, 241 insertions(+), 45 deletions(-) diff --git a/crates/epiphany-engrave/DECISIONS.md b/crates/epiphany-engrave/DECISIONS.md index 43faf85..bbe7327 100644 --- a/crates/epiphany-engrave/DECISIONS.md +++ b/crates/epiphany-engrave/DECISIONS.md @@ -428,3 +428,20 @@ Repeat-free scores are byte-identical (the existing `ten_measure` / - Volta bracket strokes raise their system's extent, so vertical stacking and page overflow account for them with no engraver change (system height is computed from every member box and stroke). +- **Same-slot spacing preservation (review follow-up, folded into version + 4 before release).** The spacing pass reserves a slot's full content extent + (its companions included) in the slot's advance, but the remap moved every + glyph independently by piecewise interpolation — so a same-slot companion + whose absolute x crossed the next slot's *source* (E1 made this reachable: + a time signature after a morphed `repeatLeft` sits `TIME_SIG_X` + the + sign's right extension ≈ 1.8 staff spaces right of its barline, past the + 1.6-space constrained column step) was dragged by the wrong interval and + collapsed into the following note. Glyphs now translate by **their own + slot's rigid delta** (`spacing::space_slots` returns the per-slot + `(source, target)` pairs beside the interpolation control points), which + is what honors the reservation; intra-slot offsets survive verbatim. + Spanning strokes keep endpoint interpolation; rigid (ledger) strokes now + translate by the owning glyph's slot delta exactly. Regression: + `time_signature_digits_ride_their_barline_slot_past_a_repeat_sign` + (unbounded page so x-disjointness compares one line; verified to fail + against the interpolated remap). diff --git a/crates/epiphany-engrave/src/lib.rs b/crates/epiphany-engrave/src/lib.rs index f644975..5520fa6 100644 --- a/crates/epiphany-engrave/src/lib.rs +++ b/crates/epiphany-engrave/src/lib.rs @@ -122,8 +122,12 @@ pub struct Engraver { /// when casting-off gained its widow-rebalance phase (a wrapping score's system /// breaks — and so its baked geometry — differ again from version `2`'s pure /// greedy first-fit), and to `4` when repeat barlines and volta brackets landed -/// (a repeat-bearing score's baked geometry differs from version `3`'s -/// invisible traced anchors; repeat-free scores are unchanged). +/// and the horizontal remap became **slot-relative** (a repeat-bearing score's +/// baked geometry differs from version `3`'s invisible traced anchors, and a +/// same-slot companion glyph — a time-signature digit, key-signature +/// accidental, or spelling accidental — now rides its slot's rigid delta +/// instead of drifting by interpolation; scores without such companions are +/// unchanged). pub const ENGRAVER_VERSION: SolverVersion = SolverVersion(4); impl Engraver { @@ -339,6 +343,14 @@ impl Engraver { struct HorizontalRemap { /// `(source_x, target_x)` control points, sorted by source, sources distinct. points: Vec<(f32, f32)>, + /// Each glyph-bearing slot's rigid translation (`target − source`). A + /// glyph moves by **its own slot's** delta — never by interpolation, which + /// would drag a same-slot companion (a time-signature digit after its + /// barline, a key-signature accidental after the clef) by a neighbouring + /// interval whenever its absolute x crosses the next slot's source. The + /// spacing pass already reserved the companion's extent in the slot's + /// advance; the rigid delta is what honors that reservation. + slot_delta: BTreeMap, } impl HorizontalRemap { @@ -346,8 +358,14 @@ impl HorizontalRemap { // The control points are computed collision-aware (per-slot bearings) by // the spacing pass; sources are globally monotonic because regions tile // left-to-right. + let spaced = spacing::space_slots(input); HorizontalRemap { - points: spacing::control_points(input), + points: spaced.points, + slot_delta: spaced + .by_slot + .into_iter() + .map(|(id, (source, target))| (id, target - source)) + .collect(), } } @@ -373,20 +391,29 @@ impl HorizontalRemap { } } - /// Re-places each glyph at its mapped x, baseline `y` preserved; provenance, - /// glyph identity, bounds, style, and layer carried through. + /// Re-places each glyph by **its slot's rigid delta** (intra-slot offsets + /// preserved verbatim — see [`HorizontalRemap::slot_delta`]), baseline `y` + /// preserved; provenance, glyph identity, bounds, style, and layer carried + /// through. A glyph whose slot the spacing pass never placed + /// (out-of-pipeline input) falls back to the interpolated map. fn glyphs(&self, input: &ConstrainedLayoutIR) -> Vec { input .glyphs .iter() - .map(|g| ResolvedGlyph { - provenance: g.provenance.clone(), - glyph: g.glyph.clone(), - position: Point::new(self.map(g.baseline.x.0), g.baseline.y.0), - transform: None, - bounding_box: g.bounding_box, - style: g.style, - layer: g.layer, + .map(|g| { + let x = match self.slot_delta.get(&g.horizontal_slot) { + Some(delta) => g.baseline.x.0 + delta, + None => self.map(g.baseline.x.0), + }; + ResolvedGlyph { + provenance: g.provenance.clone(), + glyph: g.glyph.clone(), + position: Point::new(x, g.baseline.y.0), + transform: None, + bounding_box: g.bounding_box, + style: g.style, + layer: g.layer, + } }) .collect() } @@ -403,14 +430,19 @@ impl HorizontalRemap { .iter() .map(|s| { let (from_x, to_x) = if epiphany_layout_ir::is_rigid_width_stroke(s) { - // Translate rigidly by the *owning glyph's* column delta — found by + // 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 glyph - // baseline is a control point, so `map(baseline) − baseline` is its - // exact column delta; applying it keeps the stroke's offset from the - // glyph and its length. + // 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.map(g.baseline.x.0) - g.baseline.x.0) + .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); (s.from.x.0 + delta, s.to.x.0 + delta) } else { @@ -1712,6 +1744,120 @@ mod tests { ); } + /// A repeat-morphed barline whose measure introduces a time signature: the + /// digits sit in the barline's **slot** but right of the *following note + /// column's source* (`TIME_SIG_X` plus the sign's right extension exceeds + /// the constrained column step), so a per-glyph interpolated remap would + /// drag them by the wrong interval and collapse them into the following + /// note. The slot-relative remap keeps them with their barline: resolved, + /// every digit clears both the repeat sign and every notehead, and every + /// same-slot pair keeps its exact constrained offset. + #[test] + fn time_signature_digits_ride_their_barline_slot_past_a_repeat_sign() { + use epiphany_core::{ + BeatGroup, MusicalDuration, PowerOfTwo, RationalTime, TimeSignature, + TimeSignatureDisplay, TimeSignatureId, TypedObjectId, + }; + use epiphany_layout_ir::{Margins, Size2D}; + + let mut score = epiphany_testkit::fixtures::ten_measure_with_repeats(0x000A_11CE); + let ts_id: TimeSignatureId = score.identity.mint(); + let beat = || BeatGroup { + duration: MusicalDuration(RationalTime::new(1, 4).expect("nonzero")), + subdivision: None, + accent: 1, + }; + score.time_signatures.push( + TimeSignature::new( + ts_id, + TimeSignatureDisplay::Standard { + numerator: 4, + denominator: PowerOfTwo::new(4).expect("4 is a power of two"), + }, + MusicalDuration(RationalTime::new(1, 1).expect("nonzero")), + vec![beat(), beat(), beat(), beat()], + ) + .expect("4/4 beat groups sum to a whole note"), + ); + // Measure 2 is the fixture's repeatLeft morph (the first repeat's start). + score.canvas.regions[0] + .content + .staff_instances_mut() + .expect("the fixture is staff-based")[0] + .measures[1] + .time_signature = Some(ts_id); + + let constrained = to_constrained(&to_logical(&score)); + // An unbounded page keeps everything on one endless system, so + // x-disjointness below compares glyphs of the same line (casting + // restarts x per system, making cross-system x-overlap legitimate). + let layout = Engraver::with_geometry(PageGeometry { + size: Size2D::default(), + margins: Margins::default(), + }) + .solve(&constrained, &SolverConfig::default()) + .layout; + let interval = |position: f32, bounding_box: &epiphany_layout_ir::BoundingBox| { + ( + position + bounding_box.left.0, + position + bounding_box.right.0, + ) + }; + + let digits: Vec<_> = layout + .glyphs + .iter() + .filter(|g| { + g.glyph.as_str().starts_with("timeSig") + && matches!(g.provenance.source, TypedObjectId::Measure(_)) + }) + .collect(); + assert!(!digits.is_empty(), "the 4/4 draws digit glyphs"); + let sign = layout + .glyphs + .iter() + .find(|g| { + g.glyph.as_str() == "repeatLeft" + && matches!(g.provenance.source, TypedObjectId::Measure(_)) + }) + .expect("the morphed start sign is engraved"); + let (_, sign_right) = interval(sign.position.x.0, &sign.bounding_box); + for digit in &digits { + let (digit_left, digit_right) = interval(digit.position.x.0, &digit.bounding_box); + assert!( + digit_left >= sign_right, + "a digit must clear the repeat sign's ink" + ); + for head in layout + .glyphs + .iter() + .filter(|g| g.glyph.as_str().starts_with("notehead")) + { + let (head_left, head_right) = interval(head.position.x.0, &head.bounding_box); + assert!( + digit_right <= head_left || head_right <= digit_left, + "a digit must not cross a notehead's ink" + ); + } + } + + // The invariant behind the fix: same-slot companions keep their exact + // constrained offsets through the re-spacing. + let resolved: Vec<_> = constrained.glyphs.iter().zip(&layout.glyphs).collect(); + for (a, ra) in &resolved { + for (b, rb) in &resolved { + if a.horizontal_slot == b.horizontal_slot { + let before = b.baseline.x.0 - a.baseline.x.0; + let after = rb.position.x.0 - ra.position.x.0; + assert!( + (before - after).abs() < 1e-4, + "intra-slot offsets must survive the re-spacing" + ); + } + } + } + } + /// The editing-loop vertical slice (testkit's `run_edit_loop_with`) driven /// through the **real Engraver**: click a notehead, sharpen its pitch, re-space /// with the Engraver, and confirm the selection survives. The selection is the diff --git a/crates/epiphany-engrave/src/spacing.rs b/crates/epiphany-engrave/src/spacing.rs index 6b818ed..129f97e 100644 --- a/crates/epiphany-engrave/src/spacing.rs +++ b/crates/epiphany-engrave/src/spacing.rs @@ -30,14 +30,27 @@ use crate::owning_glyph; /// the next slot's left content. const SLOT_GAP: f32 = 0.3; -/// Horizontal coordinate-map control points `(source_x, target_x)`, one per -/// glyph-bearing slot, sorted left to right. The source is the slot's column -/// reference (its first member glyph's baseline); the target accumulates -/// collision-aware advances so neighbouring slots' content — including -/// left-overhanging accidentals — never overlaps, and a wide lead (clef + key -/// signature) reserves real space. Deterministic: a pure function of the glyphs -/// and their bounding boxes. -pub(crate) fn control_points(input: &ConstrainedLayoutIR) -> Vec<(f32, f32)> { +/// The spacing pass's output: the interpolation control points for spanning +/// strokes, and each glyph-bearing slot's exact `(source, target)` pair — the +/// rigid delta every member glyph translates by, so intra-slot offsets (a +/// time signature after its barline, key-signature accidentals after the +/// clef, an accidental left of its notehead) survive the re-spacing verbatim. +pub(crate) struct SpacedSlots { + /// `(source_x, target_x)` control points, sorted by source, sources + /// distinct — the piecewise-linear map for content that genuinely *spans* + /// columns (staff lines, brackets). + pub points: Vec<(f32, f32)>, + /// Each glyph-bearing slot's own `(source_x, target_x)`. + pub by_slot: BTreeMap, +} + +/// Spaces the glyph-bearing slots left to right. Each slot's source is its +/// column reference (its first member glyph's baseline); the target +/// accumulates collision-aware advances so neighbouring slots' content — +/// including left-overhanging accidentals — never overlaps, and a wide lead +/// (clef + key signature) reserves real space. Deterministic: a pure function +/// of the glyphs and their bounding boxes. +pub(crate) fn space_slots(input: &ConstrainedLayoutIR) -> SpacedSlots { /// One slot's horizontal extent, from its member glyphs. struct Extent { /// Column reference x (the first member's baseline). @@ -94,28 +107,34 @@ pub(crate) fn control_points(input: &ConstrainedLayoutIR) -> Vec<(f32, f32)> { } } - let mut slots: Vec = by_slot.into_values().collect(); + let mut slots: Vec<(SpringSlotId, Extent)> = by_slot.into_iter().collect(); slots.sort_by(|a, b| { - a.source - .partial_cmp(&b.source) + a.1.source + .partial_cmp(&b.1.source) .unwrap_or(std::cmp::Ordering::Equal) }); let mut points = Vec::with_capacity(slots.len()); + let mut placed: BTreeMap = BTreeMap::new(); let mut target = 0.0_f32; for i in 0..slots.len() { - points.push((slots[i].source, target)); - let right_bearing = slots[i].max_right - slots[i].source; + let (id, extent) = &slots[i]; + points.push((extent.source, target)); + placed.insert(*id, (extent.source, target)); + let right_bearing = extent.max_right - extent.source; // The next slot's left overhang must be cleared by *this* slot's advance. let next_left = slots .get(i + 1) - .map(|next| next.source - next.min_left) + .map(|(_, next)| next.source - next.min_left) .unwrap_or(0.0); - let advance = slots[i].preferred.max(right_bearing + SLOT_GAP + next_left); + let advance = extent.preferred.max(right_bearing + SLOT_GAP + next_left); target += advance; } points.dedup_by(|a, b| a.0 == b.0); - points + SpacedSlots { + points, + by_slot: placed, + } } #[cfg(test)] @@ -127,12 +146,24 @@ mod tests { #[test] fn control_points_are_monotonic_in_source_and_target() { let c = to_constrained(&to_logical(&valid_score_rich(7))); - let points = control_points(&c); - assert!(!points.is_empty()); - for w in points.windows(2) { + let spaced = space_slots(&c); + assert!(!spaced.points.is_empty()); + for w in spaced.points.windows(2) { assert!(w[1].0 > w[0].0, "sources strictly increase"); assert!(w[1].1 > w[0].1, "targets strictly increase"); } + // The two views describe one spacing: every placed slot's pair is one + // of the control points (this fixture's slot sources are all distinct, + // so the equal-source dedup removes nothing). + for (source, target) in spaced.by_slot.values() { + assert!( + spaced + .points + .iter() + .any(|(s, t)| s == source && t == target), + "slot pair ({source}, {target}) must be a control point" + ); + } } #[test] @@ -140,9 +171,9 @@ mod tests { // A wide lead (clef) advances by more than a uniform note slot, so the // engraved targets are not a copy of the source columns. let c = to_constrained(&to_logical(&valid_score_rich(7))); - let points = control_points(&c); + let spaced = space_slots(&c); assert!( - points.iter().any(|(s, t)| (s - t).abs() > 1e-3), + spaced.points.iter().any(|(s, t)| (s - t).abs() > 1e-3), "targets must differ from sources (re-spacing happened)" ); } @@ -150,6 +181,8 @@ mod tests { #[test] fn spacing_is_deterministic() { let c = to_constrained(&to_logical(&valid_score_rich(3))); - assert_eq!(control_points(&c), control_points(&c)); + let (a, b) = (space_slots(&c), space_slots(&c)); + assert_eq!(a.points, b.points); + assert_eq!(a.by_slot, b.by_slot); } } 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 721ed7e..092b59b 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 @@ -159,11 +159,11 @@ - - - + + + - +