Push 3 slur review fix: measure slur_shape on whole slurs, not split fragments
An adversarial review of the slur-quality tranche found a real interaction bug between curve splitting (commit5869691) and slur_shape measurement (7d61271): slur_shape_raw iterated cast.curves — the per-system SUB-CUBICS of a break-spanning slur — and measured each fragment as a unit. A slur that is ideally shaped as a whole (ρ ≈ 0.16, in-band) splits into sub-arcs whose diagonal chords each read flatter (ρ below 0.08), so the whole slur earned a spurious "too flat" penalty (confirmed ~0.088) and was double-counted — contradicting the catalog's "a tier that draws the ideal shallow arc measures 0" property. Fix: measure the WHOLE slur curves of the constrained input.curves (one unit per drawn slur — the engraver's arc-proportion decision), not the cast fragments. Casting's horizontal re-spacing and system-splitting are spacing/rendering concerns, not shape ones. The Quality Metric Catalog contributing-units definition is clarified: the unit is the whole slur, measured once even when split across a break. Regression: a break-spanning in-band slur splits (≥2 segments) yet measures 0. Still measurement-only (no ENGRAVER_VERSION bump, no golden churn, RS suite unaffected). 936 tests, conformance 8/8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
449dd2ef8e
commit
e6308371d5
|
|
@ -493,9 +493,17 @@ slur `Curve` with chord `c > 0` (the segment between its endpoints) and apex
|
|||
height `h` (max perpendicular distance from the curve to that chord, sampled at
|
||||
`SLUR_APEX_SAMPLES = 32` points), the arc ratio `ρ = h/c` is penalized by its
|
||||
distance outside the shallow-arc band `[0.08, 0.25]`
|
||||
(`max(0, 0.08 − ρ, ρ − 0.25)`), meaned over curves and normalized by
|
||||
`R_worst = 0.25`. The measurement is translation-invariant (chord and apex
|
||||
shift together under casting), so measuring the post-cast curves is correct.
|
||||
(`max(0, 0.08 − ρ, ρ − 0.25)`), meaned over units and normalized by
|
||||
`R_worst = 0.25`.
|
||||
|
||||
**The unit is the WHOLE slur, measured on the constrained `input.curves` (one
|
||||
per drawn slur), not the cast output's per-system fragments** (review fix). A
|
||||
break-spanning slur casts into per-system sub-cubics whose *diagonal* chords
|
||||
each read flatter than the whole arc, so measuring fragments would spuriously
|
||||
penalize (and double-count) a slur that is ideally shaped as a whole —
|
||||
violating the catalog's "a tier that draws the ideal shallow arc measures 0"
|
||||
property. The whole arc is the shape-decision unit; casting's horizontal
|
||||
re-spacing is a spacing concern (`spacing_distortion`), not a shape one.
|
||||
|
||||
Honest outcome: the Minimal tier's mid-span slurs sit at `ρ = height/span =
|
||||
0.16` (the `SLUR_HEIGHT_FACTOR`; in band → 0), but the fixed
|
||||
|
|
|
|||
|
|
@ -106,8 +106,18 @@ const SLUR_APEX_SAMPLES: usize = 32;
|
|||
/// is the arithmetic mean over units. A curve-free layout has no units, so the
|
||||
/// mean is `0` (the vacuous-geometry rule) — not by construction but by
|
||||
/// measurement.
|
||||
fn slur_shape_raw(cast: &CastLayout) -> f64 {
|
||||
let per_curve: Vec<f64> = cast
|
||||
///
|
||||
/// Measured over the **whole** slur curves of the constrained input (one unit
|
||||
/// per drawn slur — the engraver's arc-proportion decision), *not* the cast
|
||||
/// output's per-system fragments: casting splits a break-spanning slur into
|
||||
/// sub-cubics whose diagonal chords each read flatter than the whole arc, which
|
||||
/// would spuriously penalize (and double-count) a slur that is ideally shaped
|
||||
/// as a whole. The catalog's property "a tier that draws the ideal shallow arc
|
||||
/// for every slur measures 0" holds only when the whole arc is the unit. The
|
||||
/// horizontal re-spacing that casting applies is a spacing concern
|
||||
/// (`spacing_distortion`), not a shape one.
|
||||
fn slur_shape_raw(input: &ConstrainedLayoutIR) -> f64 {
|
||||
let per_curve: Vec<f64> = input
|
||||
.curves
|
||||
.iter()
|
||||
.filter_map(|curve| {
|
||||
|
|
@ -537,7 +547,7 @@ pub(crate) fn measure(
|
|||
// (too bulgy) and very long ones below it (too flat) — a real, honest
|
||||
// non-zero measurement. A curve-free layout measures 0 by the
|
||||
// vacuous-geometry rule.
|
||||
slur_shape_penalty: normalize(slur_shape_raw(cast), anchors::SLUR_SHAPE_R_WORST),
|
||||
slur_shape_penalty: normalize(slur_shape_raw(input), anchors::SLUR_SHAPE_R_WORST),
|
||||
// Same vacuous rule: no drawn beam segments exist in this pipeline.
|
||||
beam_slope_penalty: normalize(0.0, anchors::BEAM_SLOPE_R_WORST),
|
||||
vertical_density_penalty: normalize(
|
||||
|
|
@ -727,6 +737,50 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_break_spanning_in_band_slur_measures_zero_despite_splitting() {
|
||||
// A well-shaped (ρ ≈ 0.16, in-band) slur whose span crosses a system
|
||||
// break splits into per-system sub-curves. The shape axis measures the
|
||||
// WHOLE slur (the constrained curve), not the fragments — whose diagonal
|
||||
// chords would each read too flat — so the well-shaped slur still scores
|
||||
// 0, as the catalog's "ideal arc ⇒ 0" property requires.
|
||||
use epiphany_core::{Slur, SlurId, SlurKind, SpanStyle, TypedObjectId};
|
||||
use epiphany_layout_ir::to_constrained;
|
||||
|
||||
let mut score = epiphany_testkit::fixtures::ten_measure_single_staff(0x000A_11CE);
|
||||
let ev: Vec<_> = score.canvas.regions[0].staff_instances()[0].voices[0]
|
||||
.events
|
||||
.clone();
|
||||
let id: SlurId = score.identity.mint();
|
||||
// Events 22→26 straddle the fixture's two-system break (a ~one-measure
|
||||
// span, wide enough that the height clamp does not bind → ρ ≈ 0.16).
|
||||
score.cross_cutting.slurs.push(Slur {
|
||||
id,
|
||||
start_event: ev[22],
|
||||
end_event: ev[26],
|
||||
kind: SlurKind::Legato,
|
||||
curvature_override: None,
|
||||
style: SpanStyle::default(),
|
||||
});
|
||||
let report = Engraver::default().solve(
|
||||
&to_constrained(&to_logical(&score)),
|
||||
&SolverConfig::default(),
|
||||
);
|
||||
// The slur really did split (the fragment path would have fired)…
|
||||
let segments = report
|
||||
.layout
|
||||
.curves
|
||||
.iter()
|
||||
.filter(|c| c.provenance.source == TypedObjectId::Slur(id))
|
||||
.count();
|
||||
assert!(segments >= 2, "the slur splits, got {segments} segment(s)");
|
||||
// …yet the shape axis is 0: the whole arc is in-band.
|
||||
assert_eq!(
|
||||
report.metric_vector.slur_shape_penalty.0, 0.0,
|
||||
"a split but well-shaped slur is not spuriously penalized"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn floor_warnings_reference_the_profiles_threshold_column() {
|
||||
// The ten-measure fixture's casting-off distortion (~0.45: the
|
||||
|
|
|
|||
Binary file not shown.
|
|
@ -790,10 +790,14 @@ practice.
|
|||
|
||||
\begin{requirement}
|
||||
\label{req:qmc:slur}
|
||||
\textbf{Contributing units:} drawn slur curves in $L$ with chord length
|
||||
$c > 0$, where the \emph{chord} is the segment between the curve's
|
||||
endpoints and the \emph{apex height} $h \ge 0$ is the maximum
|
||||
perpendicular distance from the curve to its chord.
|
||||
\textbf{Contributing units:} drawn slurs in $L$ with chord length
|
||||
$c > 0$, where the \emph{chord} is the segment between the \emph{whole}
|
||||
slur's endpoints and the \emph{apex height} $h \ge 0$ is the maximum
|
||||
perpendicular distance from the curve to its chord. The unit is the whole
|
||||
slur, not a per-system fragment: a slur that a casting-off pass splits
|
||||
across a system break is measured once, as the arc it was shaped to be, so
|
||||
a well-shaped slur that happens to break is not spuriously penalized (its
|
||||
fragments' diagonal chords each read flatter than the whole).
|
||||
|
||||
\textbf{Raw measurement:} per unit, with arc ratio $\rho = h / c$,
|
||||
\[
|
||||
|
|
|
|||
Loading…
Reference in New Issue