Push 3 audit follow-up: modify anchor validation, spaced slur_shape, stale docs
Three source-audit findings: High — ModifyCrossCutting could still introduce dangling spanner anchors. create_cross_cutting was fixed (P13-D3) to validate anchor_object_refs(), but modify_cross_cutting still validated only endpoints() — empty for a spanner's region/measure anchors — so a live event-anchored spanner could be MODIFIED onto a missing RegionId/MeasureId and written into the graph past the core invariant that checks spanner anchors at all three kinds. Fixed by mirroring create's anchor_object_refs() liveness precondition in modify (endpoints() still feeds the event-only referent index). Regression extends create_cross_cutting_spanner_preconditions_region_measure_anchors with a modify case (verified to fail without the fix: the dangling modify reached the graph). Medium — slur_shape_penalty measured the constrained (pre-remap) curves, not the drawn shape. The Engraver remaps curves before casting, so an ideal-in-source slur could read ideal even after horizontal re-spacing visibly flattened or steepened it; the catalog units are "drawn slurs." Now measured over the SPACED whole curves (post-remap, pre-split) — threaded into quality::measure — so re-spacing distortion is honestly captured while a break-spanning slur is still measured whole (not as flatter fragments). Low — stale comments: CastLayout.curves and curve_fate said break-spanning curves draw whole with de Casteljau deferred (they now split); SlurContent.line said non-solid slurs surface a diagnostic (they now render dashed/dotted); the system_derived_rewrite doc called the never-minted system-pitch introduction an unfixed Pass-13 residue (P13-K1 now rejects it). 940 tests, clippy 0, docs -D warnings, conformance 8/8, zero golden churn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
430214d619
commit
ca687d8e7e
|
|
@ -496,14 +496,18 @@ distance outside the shallow-arc band `[0.08, 0.25]`
|
|||
(`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.
|
||||
**The unit is the WHOLE SPACED slur curve** — post-horizontal-remap (the drawn
|
||||
shape) and pre-cast-split (one unit per slur) — **not the cast output's
|
||||
per-system fragments** (review fixes). 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 unit. An
|
||||
earlier take measured the *constrained* `input.curves` (pre-remap); a second
|
||||
audit noted the catalog's units are "drawn slurs," so measurement moved to the
|
||||
**spaced** curves (`spaced_curves`, threaded into `measure`) — horizontal
|
||||
re-spacing that flattens or steepens a drawn slur is now honestly captured
|
||||
rather than hidden behind the intended (pre-remap) shape.
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -180,8 +180,9 @@ pub(crate) struct CastLayout {
|
|||
/// the synthesized continuation segments.
|
||||
pub strokes: Vec<Stroke>,
|
||||
/// Final curves, in input order, each translated with its system. A curve
|
||||
/// spanning a system break is drawn whole in its start system (Minimal
|
||||
/// boundary: an honest cubic split needs de Casteljau, deferred).
|
||||
/// spanning a system break is split into per-system sub-curves by de
|
||||
/// Casteljau subdivision (the first keeps the source's provenance, the rest
|
||||
/// are synthesized continuations, like system-spanning strokes).
|
||||
pub curves: Vec<Curve>,
|
||||
/// The populated page tree (empty when the input declares no regions).
|
||||
pub pages: Vec<ResolvedPage>,
|
||||
|
|
@ -1315,12 +1316,15 @@ fn stroke_fate(
|
|||
}
|
||||
}
|
||||
|
||||
/// The system a curve rides whole: the nearest region's system whose clip
|
||||
/// interval contains the curve's **start** control point (its drawing origin),
|
||||
/// else that region's nearest system, else `None` (no region claimed it —
|
||||
/// left in the spaced frame, on no page). A curve is never split — an honest
|
||||
/// cubic split across a system break needs de Casteljau subdivision, deferred
|
||||
/// to a later tier; here a break-spanning slur draws whole in its start system.
|
||||
/// A curve's casting fate. A curve overlapping ONE system rides it whole
|
||||
/// (`Rigid(Some(s))`) — the nearest region's system whose clip interval
|
||||
/// contains the curve's **start** control point, else that region's nearest
|
||||
/// system; a curve no region claims is `Rigid(None)` (left in the spaced frame,
|
||||
/// on no page). A curve spanning MULTIPLE systems is `Split` into per-system
|
||||
/// sub-curves by de Casteljau subdivision at the parameters where its
|
||||
/// x-monotonic path crosses each system's content-clip edges (a non-monotonic
|
||||
/// curve — not produced by the engraver — cannot be honestly split and rides
|
||||
/// its start system whole).
|
||||
fn curve_fate(
|
||||
curve: &Curve,
|
||||
region_spans: &[Option<(f32, f32)>],
|
||||
|
|
|
|||
|
|
@ -291,7 +291,7 @@ impl Engraver {
|
|||
// fixed above.
|
||||
let metric_vector = match (&cast, catalog_valid) {
|
||||
(Some(cast), true) => {
|
||||
let vector = quality::measure(input, cast, &self.geometry);
|
||||
let vector = quality::measure(input, cast, &spaced_curves, &self.geometry);
|
||||
warnings.extend(quality::floor_warnings(
|
||||
&vector,
|
||||
profile_thresholds(config.profile),
|
||||
|
|
|
|||
|
|
@ -26,13 +26,15 @@
|
|||
//! reference). The clef/key/time lead and barlines bear no notehead or rest,
|
||||
//! so they contribute no column and a note-to-note advance spans them
|
||||
//! (catalog §`spacing_distortion` — measuring rhythmic spacing, not furniture).
|
||||
//! * **`slur_shape_penalty`** — **measured** (Push 3): each drawn slur curve's
|
||||
//! arc ratio `ρ = apex height / chord length` is penalized by its distance
|
||||
//! outside the shallow-arc band `[0.08, 0.25]` (catalog §`slur_shape`). The
|
||||
//! Minimal tier's mid-span slurs sit at `ρ ≈ 0.16` (in band, 0), but its
|
||||
//! fixed height clamps push short slurs above the band (too bulgy) and very
|
||||
//! long ones below it (too flat) — a real non-zero value. A curve-free layout
|
||||
//! measures 0 by the vacuous-geometry rule. **`beam_slope_penalty`** stays
|
||||
//! * **`slur_shape_penalty`** — **measured** (Push 3): each drawn slur's arc
|
||||
//! ratio `ρ = apex height / chord length` is penalized by its distance
|
||||
//! outside the shallow-arc band `[0.08, 0.25]` (catalog §`slur_shape`),
|
||||
//! measured over the *spaced* whole curves (the drawn shape, one unit per
|
||||
//! slur — not the cast's per-system fragments). The Minimal tier's mid-span
|
||||
//! slurs sit at `ρ ≈ 0.16` (in band, 0), but its fixed height clamps push
|
||||
//! short slurs above the band (too bulgy) and very long ones below it (too
|
||||
//! flat) — a real non-zero value. A curve-free layout measures 0 by the
|
||||
//! vacuous-geometry rule. **`beam_slope_penalty`** stays
|
||||
//! **vacuous 0.0**: no beam geometry is drawn yet (beams exist logically, not
|
||||
//! as segments), so its contributing-unit set is empty.
|
||||
//! * **`vertical_density_penalty`** — realized gaps against the band model's
|
||||
|
|
@ -62,8 +64,9 @@ use epiphany_layout_ir::quality::{
|
|||
anchors, normalize, MetricThresholds, QUALITY_FLOOR_FRACTION, QUALITY_METRIC_KINDS,
|
||||
};
|
||||
use epiphany_layout_ir::{
|
||||
inter_staff_gap_id, ConstrainedLayoutIR, GlyphObject, GlyphObjectId, QualityMetricVector,
|
||||
SolverWarning, SolverWarningKind, SpringSlotId, VerticalBand, VerticalBandId, VerticalBandKind,
|
||||
inter_staff_gap_id, ConstrainedLayoutIR, Curve, GlyphObject, GlyphObjectId,
|
||||
QualityMetricVector, SolverWarning, SolverWarningKind, SpringSlotId, VerticalBand,
|
||||
VerticalBandId, VerticalBandKind,
|
||||
};
|
||||
|
||||
use crate::casting::{CastLayout, PageGeometry};
|
||||
|
|
@ -107,18 +110,18 @@ const SLUR_APEX_SAMPLES: usize = 32;
|
|||
/// mean is `0` (the vacuous-geometry rule) — not by construction but by
|
||||
/// measurement.
|
||||
///
|
||||
/// Measured over the **whole** slur curves of the constrained input (one unit
|
||||
/// per drawn slur — the engraver's arc-proportion decision), *not* the cast
|
||||
/// Measured over the **whole spaced** slur curves — post-horizontal-remap (the
|
||||
/// drawn shape) and pre-cast-split (one unit per slur) — *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
|
||||
/// for every slur measures 0" holds only when the whole arc is the unit.
|
||||
/// Measuring the *spaced* (not constrained) curves means horizontal re-spacing
|
||||
/// that flattens or steepens a drawn slur is honestly captured here rather than
|
||||
/// hidden — the units are "drawn slurs" (catalog §`slur_shape`).
|
||||
fn slur_shape_raw(spaced_curves: &[Curve]) -> f64 {
|
||||
let per_curve: Vec<f64> = spaced_curves
|
||||
.iter()
|
||||
.filter_map(|curve| {
|
||||
let cp = curve.control_points();
|
||||
|
|
@ -528,6 +531,7 @@ fn symbol_density_raw(input: &ConstrainedLayoutIR, census: &SystemCensus) -> f64
|
|||
pub(crate) fn measure(
|
||||
input: &ConstrainedLayoutIR,
|
||||
cast: &CastLayout,
|
||||
spaced_curves: &[Curve],
|
||||
geometry: &PageGeometry,
|
||||
) -> QualityMetricVector {
|
||||
let census = census(input, cast);
|
||||
|
|
@ -540,14 +544,15 @@ pub(crate) fn measure(
|
|||
),
|
||||
spacing_distortion: normalize(spacing_raw(&census), anchors::SPACING_R_WORST),
|
||||
// Slurs draw (E2), so their shape is now MEASURED (Push 3), not pinned:
|
||||
// each drawn curve's arc ratio ρ = apex height / chord length is
|
||||
// each drawn slur's arc ratio ρ = apex height / chord length is
|
||||
// penalized by its distance outside the shallow-arc band [0.08, 0.25].
|
||||
// The Minimal tier's mid-span slurs sit at ρ ≈ 0.16 (in band, 0
|
||||
// penalty), but its fixed height clamps push short slurs above the band
|
||||
// (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(input), anchors::SLUR_SHAPE_R_WORST),
|
||||
// Measured on the SPACED whole curves — post-horizontal-remap (the
|
||||
// shape the reader sees), pre-cast-split (the whole slur, one unit) —
|
||||
// so re-spacing that visibly flattens or steepens a slur is caught, yet
|
||||
// a well-shaped slur that merely crosses a system break is not
|
||||
// penalized by its fragments' diagonal chords. A curve-free layout
|
||||
// measures 0 by the vacuous-geometry rule.
|
||||
slur_shape_penalty: normalize(slur_shape_raw(spaced_curves), 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(
|
||||
|
|
|
|||
|
|
@ -225,9 +225,9 @@ pub struct SlurContent {
|
|||
/// preserved for a kind-aware higher tier (a phrase mark's longer curve, an
|
||||
/// editorial slur's distinct line).
|
||||
pub kind: SlurKind,
|
||||
/// The authored line style (`style.line`). The Minimal tier draws every
|
||||
/// slur solid; a non-`Solid` style is surfaced as a diagnostic rather than
|
||||
/// silently rendered solid (its dash pattern is a higher-tier refinement).
|
||||
/// The authored line style (`style.line`). Rendered faithfully: the style
|
||||
/// rides the emitted `Curve`, so a non-`Solid` slur draws dashed or dotted
|
||||
/// (`stroke-dasharray`) rather than silently solid.
|
||||
pub line: LineStyle,
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -3345,8 +3345,16 @@ impl<'a> Reducer<'a> {
|
|||
};
|
||||
}
|
||||
}
|
||||
// Every anchored object of the NEW value must be live — events for the
|
||||
// event-anchored kinds, and a spanner's region/measure anchor targets
|
||||
// too (P13-D3, modify sibling): otherwise a live event-anchored spanner
|
||||
// could be modified onto a missing region/measure — whose empty
|
||||
// `endpoints()` slipped the check — and written into the graph past the
|
||||
// core invariant that validates spanner anchors at all three kinds.
|
||||
// Mirrors `create_cross_cutting`; `endpoints()` still feeds the
|
||||
// event-only referent index below.
|
||||
let endpoints = op.structure.endpoints();
|
||||
for e in &endpoints {
|
||||
for e in &op.structure.anchor_object_refs() {
|
||||
if !matches!(self.objects.get(e), Some(ObjectState::Live)) {
|
||||
return OperationEffect::NoOp {
|
||||
reason: NoOpReason::PreconditionFailedUnderReduction {
|
||||
|
|
@ -5227,15 +5235,13 @@ impl<'a> Reducer<'a> {
|
|||
/// base seeded) is unverifiable and passes, like the other
|
||||
/// graph-aware-only preconditions.
|
||||
///
|
||||
/// Known residue (filed as a Pass-13 candidate; see DECISIONS.md): a
|
||||
/// system pitch *introduced into the graph by a ModifyEvent replacement*
|
||||
/// (never minted — the pre-walk deliberately excludes ModifyEvent) gets
|
||||
/// this verdict only after a snapshot re-seeds the registry from the
|
||||
/// base graph; in-session it reads `TargetMissing` instead. That
|
||||
/// checkpoint-cut asymmetry for ModifyEvent-introduced content predates
|
||||
/// this precondition (pre-K3 the same split read `TargetMissing` vs a
|
||||
/// silent `Applied` rewrite) and is a ModifyEvent-introduction question,
|
||||
/// not a K3 one.
|
||||
/// This is the *rewrite* precondition (a live system pitch's content
|
||||
/// changed in place). The sibling *introduction* case — a system pitch
|
||||
/// never minted, introduced by a `ModifyEvent` replacement — is refused
|
||||
/// upfront in [`Self::modify_event`] (P13-K1, resolved: reject the
|
||||
/// introduction), which closes the checkpoint-cut asymmetry that once let
|
||||
/// an introduced system pitch slip through in-session yet read
|
||||
/// `SystemDerivedContentImmutable` after a snapshot re-seeded it here.
|
||||
fn system_derived_rewrite(&self, id: PitchId, value: &Pitch) -> bool {
|
||||
if id.replica() != ReplicaId::SYSTEM_DERIVED {
|
||||
return false;
|
||||
|
|
@ -8406,6 +8412,68 @@ mod tests {
|
|||
"live region/measure anchors mint the spanner"
|
||||
);
|
||||
assert!(epiphany_core::check_invariants(&result.score).is_empty());
|
||||
|
||||
// MODIFY sibling: a live event-anchored spanner MODIFIED onto a missing
|
||||
// region is refused too — `endpoints()` is empty for the new region
|
||||
// anchors, so the pre-fix modify check slipped a dangling value into the
|
||||
// graph past the core invariant.
|
||||
let events = base
|
||||
.voices()
|
||||
.map(|(_, _, v)| v.events.clone())
|
||||
.next()
|
||||
.expect("the fixture has a voice");
|
||||
let (e0, e1) = (events[0], events[1]);
|
||||
let mod_id = SpannerId::new(ReplicaId(9), 812);
|
||||
let mod_obj = TypedObjectId::Spanner(mod_id);
|
||||
let mut set = OperationSet::new();
|
||||
set.accept_all(vec![
|
||||
prim_env(
|
||||
9,
|
||||
0,
|
||||
10,
|
||||
CausalContext::new(),
|
||||
spanner(
|
||||
mod_id,
|
||||
crate::valuegen::event_anchor(e0),
|
||||
crate::valuegen::event_anchor(e1),
|
||||
),
|
||||
),
|
||||
prim_env(
|
||||
9,
|
||||
1,
|
||||
11,
|
||||
CausalContext::new().with_seen(ReplicaId(9), 0),
|
||||
OperationKind::ModifyCrossCutting(crate::payload::ModifyCrossCuttingOp {
|
||||
structure: CrossCuttingValue::Spanner(epiphany_core::Spanner {
|
||||
id: mod_id,
|
||||
start: TimeAnchor::Region {
|
||||
id: RegionId::new(ReplicaId(9), 7_778),
|
||||
edge: RegionEdge::Start,
|
||||
offset: AnchorOffset::Zero,
|
||||
},
|
||||
end: region_anchor(region_id),
|
||||
staves: Vec::new(),
|
||||
kind: Default::default(),
|
||||
style: Default::default(),
|
||||
}),
|
||||
}),
|
||||
),
|
||||
]);
|
||||
let result = set.reduce_onto(&base);
|
||||
// The spanner stays live at its original event anchors (modify refused)…
|
||||
assert!(
|
||||
matches!(result.state.objects.get(&mod_obj), Some(ObjectState::Live)),
|
||||
"the spanner survives the refused modify"
|
||||
);
|
||||
// …and the dangling region anchor never reached the graph.
|
||||
assert!(
|
||||
!result.score.cross_cutting.spanners.iter().any(|s| {
|
||||
s.id == mod_id
|
||||
&& matches!(s.start, TimeAnchor::Region { id, .. } if id == RegionId::new(ReplicaId(9), 7_778))
|
||||
}),
|
||||
"the dangling modify never reached the graph"
|
||||
);
|
||||
assert!(epiphany_core::check_invariants(&result.score).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
Loading…
Reference in New Issue