diff --git a/crates/epiphany-engrave/DECISIONS.md b/crates/epiphany-engrave/DECISIONS.md index d21cb61..4a503bf 100644 --- a/crates/epiphany-engrave/DECISIONS.md +++ b/crates/epiphany-engrave/DECISIONS.md @@ -709,13 +709,29 @@ single-staff score, with no inter-staff pair, is byte-identical). **Attribution (`vertical_band` + owning-glyph).** Every resolved primitive is attributed to its owning staff: a glyph via its `vertical_band` -(`VerticalBandKind::Staff` → `StaffId`); a stem or ledger via its notehead -(`component_glyph` → that glyph's band); 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.) +(`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.) + +**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 +right for a *slot* — both staves of a system share their x columns, hence their +spring slots, so the horizontal delta is the same either way — but it is wrong +for a *staff*: it handed a lower-staff stem to the UPPER staff's notehead, so the +stem kept the wrong vertical shift and tore off its own head by several staff +spaces (and polluted the upper staff's content extent, inflating the computed +gap). The staff attribution now uses a 2-D nearest for that fallback. +`component_glyph` is unchanged and still serves the horizontal path. Locked by +`multi_staff_stems_stay_on_their_own_staff` (a stem's base sits at its notehead +on both the single- and multi-staff fixtures). A related correction: staff- +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`. **The solve.** Per system, per staff, the real content y-extent is collected (glyphs, strokes, curves — ledgers and slurs included, not just noteheads). The diff --git a/crates/epiphany-engrave/src/casting.rs b/crates/epiphany-engrave/src/casting.rs index d4d5fe6..4f3a616 100644 --- a/crates/epiphany-engrave/src/casting.rs +++ b/crates/epiphany-engrave/src/casting.rs @@ -81,7 +81,7 @@ use epiphany_layout_ir::{ SynthesisKind, SynthesisRegistryId, VerticalBand, VerticalBandId, VerticalBandKind, }; -use crate::{component_glyph, owning_glyph}; +use crate::owning_glyph; /// The registry id for the engraver's **system-continuation synthesis**: the /// segment of a region-spanning stroke (a staff line) that casting-off places @@ -353,22 +353,23 @@ impl Extent { } } - fn add(&mut self, x0: f32, y0: f32, x1: f32, y1: f32) { - if [x0, y0, x1, y1].iter().all(|v| v.is_finite()) { - self.min_x = self.min_x.min(x0.min(x1)); - self.max_x = self.max_x.max(x0.max(x1)); - self.min_y = self.min_y.min(y0.min(y1)); - self.max_y = self.max_y.max(y0.max(y1)); - self.any = true; - } - } - /// Extend only the vertical extent (the inter-staff solve grows a system's /// height by shifting staves apart, without touching its x-span). fn add_y(&mut self, y0: f32, y1: f32) { if y0.is_finite() && y1.is_finite() { self.min_y = self.min_y.min(y0.min(y1)); self.max_y = self.max_y.max(y0.max(y1)); + self.any = true; + } + } + + /// Extend only the horizontal extent. Staff-attributed content contributes + /// its y through the inter-staff solve (SHIFTED), never here. + fn add_x(&mut self, x0: f32, x1: f32) { + if x0.is_finite() && x1.is_finite() { + self.min_x = self.min_x.min(x0.min(x1)); + self.max_x = self.max_x.max(x0.max(x1)); + self.any = true; } } @@ -651,12 +652,28 @@ pub(crate) fn cast_off( .collect(); let glyph_band_staff = |g: &GlyphObject| band_to_staff.get(&g.vertical_band).copied(); let glyph_staff_of: Vec> = input.glyphs.iter().map(glyph_band_staff).collect(); + // The glyph nearest a point in TWO dimensions. Attribution to a *staff* must + // be y-aware: `component_glyph`'s horizontal fallback picks the nearest glyph + // by x alone, which is right for a slot (both staves of a system share their + // x columns, hence their slots) but would hand a stem to the ADJACENT staff. + let nearest_glyph = |x: f32, y: f32| -> Option<&GlyphObject> { + input.glyphs.iter().min_by(|a, b| { + let d = |g: &GlyphObject| (g.baseline.x.0 - x).powi(2) + (g.baseline.y.0 - y).powi(2); + d(a).total_cmp(&d(b)) + }) + }; let stroke_staff_of: Vec> = input .strokes .iter() .map(|s| match s.provenance.source { + // A staff line names its staff outright. TypedObjectId::Staff(st) => Some(st), - _ => component_glyph(s, &input.glyphs).and_then(glyph_band_staff), + // A ledger shares its notehead's `Pitch` source (`owning_glyph`); + // a stem has no same-source glyph, so it takes the staff of the + // glyph nearest its BASE (`from`, which sits at the notehead). + _ => owning_glyph(s, &input.glyphs) + .or_else(|| nearest_glyph(s.from.x.0, s.from.y.0)) + .and_then(glyph_band_staff), }) .collect(); let curve_staff_of: Vec> = input @@ -706,13 +723,14 @@ pub(crate) fn cast_off( y + glyph.bounding_box.bottom.0, y + glyph.bounding_box.top.0, ); - extents[s].add( + extents[s].add_x( x + glyph.bounding_box.left.0, - lo_y, x + glyph.bounding_box.right.0, - hi_y, ); - into_staff(&mut staff_ext, s, glyph_staff_of[g], lo_y, hi_y); + match glyph_staff_of[g] { + Some(_) => into_staff(&mut staff_ext, s, glyph_staff_of[g], lo_y, hi_y), + None => extents[s].add_y(lo_y, hi_y), + } } } } @@ -727,8 +745,11 @@ pub(crate) fn cast_off( }; for (s, from, to) in segs { let (lo_y, hi_y) = (from.y.0.min(to.y.0) - half, from.y.0.max(to.y.0) + half); - extents[s].add(from.x.0 - half, lo_y, to.x.0 + half, hi_y); - into_staff(&mut staff_ext, s, staff, lo_y, hi_y); + extents[s].add_x(from.x.0 - half, to.x.0 + half); + match staff { + Some(_) => into_staff(&mut staff_ext, s, staff, lo_y, hi_y), + None => extents[s].add_y(lo_y, hi_y), + } if is_staff_line { if let Some(st) = staff { staff_ref @@ -749,8 +770,11 @@ pub(crate) fn cast_off( }; for (s, cp) in segs { for p in cp { - extents[s].add(p.x.0 - half, p.y.0 - half, p.x.0 + half, p.y.0 + half); - into_staff(&mut staff_ext, s, staff, p.y.0 - half, p.y.0 + half); + extents[s].add_x(p.x.0 - half, p.x.0 + half); + match staff { + Some(_) => into_staff(&mut staff_ext, s, staff, p.y.0 - half, p.y.0 + half), + None => extents[s].add_y(p.y.0 - half, p.y.0 + half), + } } } } diff --git a/crates/epiphany-engrave/src/lib.rs b/crates/epiphany-engrave/src/lib.rs index 38649fb..97535a9 100644 --- a/crates/epiphany-engrave/src/lib.rs +++ b/crates/epiphany-engrave/src/lib.rs @@ -2233,6 +2233,56 @@ mod tests { } } + #[test] + fn multi_staff_stems_stay_on_their_own_staff() { + use epiphany_layout_ir::{to_constrained, to_logical}; + // Review-found bug: the inter-staff attribution reused `component_glyph`, + // whose fallback picks the nearest glyph by X ALONE — correct for a slot + // (both staves of a system share their x columns, hence their slots) but + // it handed a stem to the ADJACENT staff, so the stem kept that staff's + // vertical shift and tore off its own notehead. Attribution is now + // y-aware. Every stem's base must still sit at its notehead, offset only + // by the stem's x inset — on the single- AND multi-staff fixtures. + let worst = |score: epiphany_core::Score| -> f32 { + let r = Engraver::default().solve( + &to_constrained(&to_logical(&score)), + &SolverConfig::default(), + ); + let heads: Vec<(f32, f32)> = r + .layout + .glyphs + .iter() + .filter(|g| g.glyph.as_str().starts_with("notehead")) + .map(|g| (g.position.x.0, g.position.y.0)) + .collect(); + r.layout + .strokes + .iter() + .filter(|st| { + (st.from.x.0 - st.to.x.0).abs() < 1e-4 && (st.from.y.0 - st.to.y.0).abs() > 1e-3 + }) + .map(|st| { + heads + .iter() + .map(|(hx, hy)| { + ((hx - st.from.x.0).powi(2) + (hy - st.from.y.0).powi(2)).sqrt() + }) + .fold(f32::INFINITY, f32::min) + }) + .fold(0.0_f32, f32::max) + }; + let single = worst(epiphany_testkit::fixtures::ten_measure_single_staff(1)); + let multi = worst(epiphany_testkit::fixtures::two_staff_close_content(1)); + assert!( + single < 1.5, + "single-staff stems stay on their heads: {single}" + ); + assert!( + multi < 1.5, + "multi-staff stems stay on their OWN staff's heads: {multi}" + ); + } + #[test] fn inter_staff_solve_separates_colliding_staves() { use epiphany_layout_ir::{to_constrained, to_logical}; diff --git a/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.snapshot.txt b/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.snapshot.txt index b5154c8..9fa0611 100644 --- a/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.snapshot.txt +++ b/crates/epiphany-render-svg/tests/golden/two_staff_close_content.engrave.snapshot.txt @@ -8,7 +8,7 @@ provenance_count=54 layer_count=1 hard_constraint_count=18 xml_well_formed=true -view_box=[5.5 -41.584576 17.379084 36.084] +view_box=[5.5 -36.58458 17.379084 31.084002] class_counts: barline=2 clef=2 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 ee7267c..6a6502e 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 @@ -1,28 +1,28 @@ - + - + - - - - - - - + + + + + + + - + - + @@ -31,32 +31,32 @@ - + - - - - - - - - + + + + + + + + - - - - - + + + + + - +