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) <noreply@anthropic.com>
This commit is contained in:
parent
4132a7ac05
commit
b1bfe04d6c
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<StaffId, (f32, f32)> = 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<Option<StaffId>> = 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();
|
||||
|
||||
|
|
|
|||
|
|
@ -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 =
|
||||
|
|
|
|||
|
|
@ -44,7 +44,7 @@
|
|||
<line x1="15.8058" y1="-27.9526" x2="17.5864" y2="-27.9526" stroke="#000000" stroke-width="0.13" data-prov="1caed515110c4415278d31cc474bf065" data-source-kind="1" data-kind="stroke"/>
|
||||
<line x1="15.8058" y1="-26.9526" x2="17.5864" y2="-26.9526" stroke="#000000" stroke-width="0.13" data-prov="1189a8cf5e7e1526a44f3cf89e0b160e" data-source-kind="1" data-kind="stroke"/>
|
||||
<line x1="19.3364" y1="-30.4526" x2="19.3364" y2="-26.9526" stroke="#000000" stroke-width="0.12" data-prov="f230acdf26cd79ad3ba5cd02002fa1f9" data-source-kind="0" data-kind="stroke"/>
|
||||
<path d="M 14.8053 -20.1926 C 15.6723 -19.1259 16.5392 -19.1259 17.4062 -20.1926" fill="none" stroke="#000000" stroke-width="0.12" data-prov="0165993515166bea8f7cada46e27c86f" data-source-kind="11" data-kind="curve"/>
|
||||
<path d="M 14.8053 -28.2526 C 15.6723 -27.1859 16.5392 -27.1859 17.4062 -28.2526" fill="none" stroke="#000000" stroke-width="0.12" data-prov="0165993515166bea8f7cada46e27c86f" data-source-kind="11" data-kind="curve"/>
|
||||
<path d="M1.504 1.66C1.496 1.708 1.504 1.712 1.528 1.736C1.96 2.14 2.288 2.648 2.288 3.26C2.288 3.608 2.192 3.952 2.028 4.192C1.968 4.28 1.864 4.392 1.82 4.392C1.764 4.392 1.64 4.288 1.56 4.2C1.264 3.872 1.168 3.372 1.168 2.956C1.168 2.724 1.196 2.464 1.224 2.3C1.232 2.252 1.236 2.244 1.188 2.204C0.612 1.728 0 1.156 0 0.348C0 -0.348 0.476 -1.008 1.456 -1.008C1.548 -1.008 1.652 -1 1.732 -0.984C1.776 -0.976 1.784 -0.972 1.792 -1.02C1.84 -1.288 1.9 -1.636 1.9 -1.824C1.9 -2.416 1.5 -2.488 1.264 -2.488C1.048 -2.488 0.944 -2.424 0.944 -2.372C0.944 -2.344 0.98 -2.332 1.072 -2.304C1.196 -2.268 1.34 -2.16 1.34 -1.928C1.34 -1.708 1.2 -1.52 0.956 -1.52C0.688 -1.52 0.528 -1.732 0.528 -1.98C0.528 -2.24 0.684 -2.632 1.288 -2.632C1.556 -2.632 2.076 -2.512 2.076 -1.832C2.076 -1.604 2.004 -1.224 1.96 -0.976C1.952 -0.928 1.956 -0.932 2.012 -0.908C2.416 -0.748 2.684 -0.408 2.684 0.044C2.684 0.556 2.308 1.008 1.72 1.008C1.616 1.008 1.616 1.008 1.604 1.08ZM1.88 3.772C2.012 3.772 2.12 3.664 2.12 3.444C2.12 3 1.74 2.64 1.424 2.364C1.396 2.34 1.38 2.344 1.372 2.396C1.356 2.5 1.348 2.636 1.348 2.764C1.348 3.388 1.636 3.772 1.88 3.772ZM1.444 1.048C1.456 0.972 1.456 0.976 1.384 0.952C1.032 0.832 0.804 0.516 0.804 0.176C0.804 -0.184 0.992 -0.44 1.264 -0.532C1.296 -0.544 1.344 -0.556 1.372 -0.556C1.404 -0.556 1.42 -0.536 1.42 -0.512C1.42 -0.484 1.388 -0.472 1.36 -0.46C1.192 -0.388 1.072 -0.216 1.072 -0.032C1.072 0.196 1.228 0.368 1.472 0.436C1.536 0.452 1.544 0.448 1.552 0.404L1.752 -0.788C1.76 -0.832 1.756 -0.832 1.696 -0.844C1.632 -0.856 1.552 -0.864 1.472 -0.864C0.772 -0.864 0.32 -0.476 0.32 0.08C0.32 0.316 0.36 0.632 0.692 1.008C0.932 1.276 1.116 1.424 1.304 1.576C1.344 1.608 1.352 1.604 1.36 1.56ZM1.72 0.412C1.712 0.46 1.716 0.472 1.764 0.468C2.088 0.44 2.356 0.168 2.356 -0.184C2.356 -0.436 2.204 -0.64 1.98 -0.752C1.932 -0.776 1.924 -0.776 1.916 -0.728Z" transform="translate(8.6599 -11.8926)" fill="#000000" data-prov="e81b0f9cc6ccb1d0209d51b1d9797db5" data-source-kind="4" data-glyph="gClef" data-class="clef"/>
|
||||
<path d="M0.388 -0.5C0.744 -0.5 1.18 -0.172 1.18 0.168C1.18 0.372 1.02 0.5 0.792 0.5C0.352 0.5 0 0.176 0 -0.168C0 -0.376 0.172 -0.5 0.388 -0.5Z" transform="translate(11.9444 -13.8926)" fill="#000000" data-prov="9b3ac956eb2deb3e538d9b1942c6ae97" data-source-kind="1" data-glyph="noteheadBlack" data-class="notehead"/>
|
||||
<path d="M0.388 -0.5C0.744 -0.5 1.18 -0.172 1.18 0.168C1.18 0.372 1.02 0.5 0.792 0.5C0.352 0.5 0 0.176 0 -0.168C0 -0.376 0.172 -0.5 0.388 -0.5Z" transform="translate(14.0251 -17.3926)" fill="#000000" data-prov="751a5a693a87d6c1a15a8dfc3eda8102" data-source-kind="1" data-glyph="noteheadBlack" data-class="notehead"/>
|
||||
|
|
|
|||
|
Before Width: | Height: | Size: 15 KiB After Width: | Height: | Size: 15 KiB |
Loading…
Reference in New Issue