P13-I2: Staff::default_clef is the fallback, not decoration
to_constrained took the active clef from the staff instance's clef_sequence and
fell back to Clef::default() -- treble. A staff that declared its clef only on the
Staff, with no ClefChange, therefore drew a treble clef and placed every note
against it. The field was decorative in the projection.
It is the fallback. StaffContent now carries default_clef -- the clef belongs to
the Staff, the sequence to the StaffInstance, and resolving "the clef at time t"
needs both -- and active_clef_or(clefs, at, default) resolves against it.
active_clef remains as that with the treble default, for callers with no staff to
hand, so the public API is intact.
The part worth pausing on: epiphany-editor-core reads the same function for
hit-test pitch resolution. Fixing only the projection would have left a click on a
bass staff resolving its pitch as treble -- the engraved clef and the editor
disagreeing about what note is where. Both now go through active_clef_or.
Removal was rejected: the field is named for its purpose, is encoded on the wire,
and dropping it would be schema-major.
Zero golden churn: every fixture and generator declares treble, which is also
Clef::default(), so nothing that exists today moves. Locked by
a_staff_declaring_only_a_default_clef_engraves_in_it and mutation-verified by
restoring the Clef::default() fallback. Scoping that test by provenance was
necessary -- valid_score_rich has three staves and only one was re-clefed, so the
first assertion I wrote ("no gClef anywhere") failed against a correct fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
b7dab0fd44
commit
afb487c24d
|
|
@ -63,9 +63,10 @@ use epiphany_core::{
|
|||
VoiceId, WallClockTime,
|
||||
};
|
||||
use epiphany_layout_ir::{
|
||||
active_clef, manifestation_layout_id, staff_step_pitch, to_constrained, to_logical, to_render,
|
||||
ConstraintSolver, ExtensionRef, HitTestMap, LayoutContent, LayoutObjectId, LogicalLayoutIR,
|
||||
Point, Rect, RenderIR, ResolvedLayoutIR, ResolvedSystem, SolverConfig, TimePoint,
|
||||
active_clef_or, manifestation_layout_id, staff_step_pitch, to_constrained, to_logical,
|
||||
to_render, ConstraintSolver, ExtensionRef, HitTestMap, LayoutContent, LayoutObjectId,
|
||||
LogicalLayoutIR, Point, Rect, RenderIR, ResolvedLayoutIR, ResolvedSystem, SolverConfig,
|
||||
TimePoint,
|
||||
};
|
||||
use epiphany_ops::{
|
||||
advisory_violations, AcceptOutcome, AuthorId, CausalContext, DeleteEventOp,
|
||||
|
|
@ -2606,7 +2607,13 @@ fn staff_start_clefs(logical: &LogicalLayoutIR) -> StartClefs {
|
|||
for object in ®ion.objects {
|
||||
if let (Some(staff), LayoutContent::Staff(content)) = (object.staff(), object.content())
|
||||
{
|
||||
clefs.insert((region_id, staff), active_clef(&content.clefs, &start));
|
||||
// The staff's own default backs an empty (or later-starting)
|
||||
// clef sequence, exactly as the projection resolves it — else a
|
||||
// click on a bass staff would resolve its pitch as treble.
|
||||
clefs.insert(
|
||||
(region_id, staff),
|
||||
active_clef_or(&content.clefs, &start, content.default_clef),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -856,6 +856,7 @@ mod tests {
|
|||
manifested(
|
||||
TypedObjectId::StaffInstance(StaffInstanceId::from_raw(1)),
|
||||
LayoutContent::Staff(StaffContent {
|
||||
default_clef: epiphany_core::Clef::default(),
|
||||
clefs: vec![],
|
||||
keys: vec![],
|
||||
}),
|
||||
|
|
@ -1277,6 +1278,7 @@ mod tests {
|
|||
let mut objects = vec![manifested(
|
||||
TypedObjectId::StaffInstance(StaffInstanceId::from_raw(1)),
|
||||
LayoutContent::Staff(StaffContent {
|
||||
default_clef: epiphany_core::Clef::default(),
|
||||
clefs: vec![],
|
||||
keys: vec![],
|
||||
}),
|
||||
|
|
@ -1780,6 +1782,7 @@ mod tests {
|
|||
manifested(
|
||||
TypedObjectId::StaffInstance(StaffInstanceId::from_raw(1)),
|
||||
LayoutContent::Staff(StaffContent {
|
||||
default_clef: epiphany_core::Clef::default(),
|
||||
clefs: vec![],
|
||||
keys: vec![PlacedKeySignature {
|
||||
time: TimePoint::Musical(MusicalPosition::origin()),
|
||||
|
|
|
|||
|
|
@ -710,7 +710,7 @@ find what the author wrote); guessing produces a score that looks engraved and i
|
|||
wrong, with nothing in the IR to say so. Ratified as implemented; locked by
|
||||
`an_unengravable_object_is_recorded_and_still_placed`.
|
||||
|
||||
## Parked: `Staff::default_clef` is never consulted (2026-07-09)
|
||||
## RESOLVED (P13-I2): `Staff::default_clef` is now the fallback (2026-07-09)
|
||||
|
||||
`to_constrained` takes a staff instance's active clef from its `clef_sequence`
|
||||
(via `staff_content`'s `PlacedClef` list) and, when that sequence is empty, falls
|
||||
|
|
@ -720,11 +720,25 @@ the field is decorative in the projection. No consumer in this crate reads it
|
|||
(verified: `default_clef` appears only in core's codec/generators and the
|
||||
fixtures).
|
||||
|
||||
Found while building `percussion_placeholder_staff`, which therefore has to
|
||||
declare its percussion clef as a `ClefChange` rather than on the staff. Whether
|
||||
the staff's default should seed the sequence, or the field should be removed, is
|
||||
a small design question — not a silent-corruption bug (nothing is lost, only
|
||||
ignored).
|
||||
Found while building `percussion_placeholder_staff`, which therefore had to
|
||||
declare its percussion clef as a `ClefChange` rather than on the staff.
|
||||
|
||||
**Resolved as P13-I2: it is the fallback.** `StaffContent` now carries
|
||||
`default_clef` (the clef belongs to the `Staff`, the sequence to the
|
||||
`StaffInstance`, and resolving "the clef at time t" needs both), and
|
||||
`active_clef_or(clefs, at, default)` resolves against it. `active_clef` remains as
|
||||
that with the treble default, for callers with no staff to hand.
|
||||
|
||||
The subtle part is that `epiphany-editor-core` reads the same function for
|
||||
hit-test pitch resolution: had only the projection been fixed, a click on a bass
|
||||
staff would have resolved its pitch as treble, and the engraved clef and the
|
||||
editor would have disagreed. Both now go through `active_clef_or`.
|
||||
|
||||
Removal was rejected: the field is named for its purpose, is encoded on the wire,
|
||||
and dropping it would be schema-major. Zero golden churn — every fixture and
|
||||
generator declares treble, which is also `Clef::default()`. Locked by
|
||||
`a_staff_declaring_only_a_default_clef_engraves_in_it`, mutation-verified against
|
||||
the old `Clef::default()` fallback.
|
||||
|
||||
**Both of the above are parked, not open.** The Pass-13 batch is closed and the
|
||||
house rule opens a pass at ≥3 candidates; these are two. They join a future batch
|
||||
|
|
|
|||
|
|
@ -802,14 +802,16 @@ pub fn try_to_constrained(
|
|||
// The clef *sequence* in force on each staff (a staff instance carries it).
|
||||
// The active clef at a given position is the latest change at or before it,
|
||||
// so a mid-staff clef change moves later pitches without affecting earlier
|
||||
// ones. An empty sequence defaults to treble.
|
||||
// ones. An empty sequence falls back to the staff's own `default_clef`.
|
||||
let mut clef_seq_of: BTreeMap<StaffId, Vec<PlacedClef>> = BTreeMap::new();
|
||||
let mut clef_default_of: BTreeMap<StaffId, Clef> = BTreeMap::new();
|
||||
for object in ®ion.objects {
|
||||
if let (Some(staff), LayoutContent::Staff(content)) = (object.staff(), object.content())
|
||||
{
|
||||
clef_seq_of
|
||||
.entry(staff)
|
||||
.or_insert_with(|| content.clefs.clone());
|
||||
clef_default_of.entry(staff).or_insert(content.default_clef);
|
||||
}
|
||||
}
|
||||
let clef_seq = |staff: Option<StaffId>| -> &[PlacedClef] {
|
||||
|
|
@ -818,6 +820,13 @@ pub fn try_to_constrained(
|
|||
.map(Vec::as_slice)
|
||||
.unwrap_or(&[])
|
||||
};
|
||||
// A staff's own default clef, in force before its first `ClefChange`.
|
||||
let clef_default = |staff: Option<StaffId>| -> Clef {
|
||||
staff
|
||||
.and_then(|s| clef_default_of.get(&s))
|
||||
.copied()
|
||||
.unwrap_or_default()
|
||||
};
|
||||
|
||||
// Pass 1 — compute every glyph's notation, keyed for emission in pass 2,
|
||||
// and collect the distinct columns it occupies. A note/rest notated as a
|
||||
|
|
@ -862,7 +871,7 @@ pub fn try_to_constrained(
|
|||
let time = shift_time(¬e.position, &offset);
|
||||
let key = ColumnKey::Timed(time.clone(), ColumnRole::Note);
|
||||
keys.insert(key.clone());
|
||||
let clef = active_clef(clef_seq(staff), &time);
|
||||
let clef = active_clef_or(clef_seq(staff), &time, clef_default(staff));
|
||||
let name = notehead_glyph(value);
|
||||
let mut ys = Vec::new();
|
||||
for pitch in ¬e.pitches {
|
||||
|
|
@ -1022,7 +1031,7 @@ pub fn try_to_constrained(
|
|||
// The staff instance's clef glyph occupies the lead column. The
|
||||
// *displayed* clef is the one in force at the staff start, by
|
||||
// time — consistent with how notes resolve their active clef.
|
||||
let clef = active_clef(&content.clefs, &origin());
|
||||
let clef = active_clef_or(&content.clefs, &origin(), content.default_clef);
|
||||
if clef_glyph(clef.shape).is_some() {
|
||||
keys.insert(ColumnKey::Lead);
|
||||
// The key signature shares the lead column; reserve its width.
|
||||
|
|
@ -1223,7 +1232,9 @@ pub fn try_to_constrained(
|
|||
// The displayed clef is the one in force at the staff start, by
|
||||
// time — the same query the notes use, so they always agree.
|
||||
let clef = match content {
|
||||
Some(LayoutContent::Staff(c)) => active_clef(&c.clefs, &origin()),
|
||||
Some(LayoutContent::Staff(c)) => {
|
||||
active_clef_or(&c.clefs, &origin(), c.default_clef)
|
||||
}
|
||||
_ => Clef::default(),
|
||||
};
|
||||
match clef_glyph(clef.shape) {
|
||||
|
|
@ -1433,7 +1444,7 @@ pub fn try_to_constrained(
|
|||
// An unmatched pitch (no event content reached it): a black
|
||||
// notehead on the clef reference line, with the gap surfaced.
|
||||
emit.diag(provenance.source, LayoutDiagnosticKind::MissingSpelling);
|
||||
let clef = active_clef(clef_seq(staff), &origin());
|
||||
let clef = active_clef_or(clef_seq(staff), &origin(), clef_default(staff));
|
||||
emit.stroke(anchor(
|
||||
provenance,
|
||||
Point::new(default_x, step_to_y(yo, reference_step(&clef))),
|
||||
|
|
@ -2507,7 +2518,10 @@ fn active_key(keys: &[PlacedKeySignature]) -> Option<KeySignature> {
|
|||
/// the key is C major, or the clef has no diatonic positions (percussion).
|
||||
fn key_accidentals_for(content: &StaffContent) -> Vec<KeyAccidental> {
|
||||
match active_key(&content.keys) {
|
||||
Some(key) => key_signature(key, &active_clef(&content.clefs, &origin())),
|
||||
Some(key) => key_signature(
|
||||
key,
|
||||
&active_clef_or(&content.clefs, &origin(), content.default_clef),
|
||||
),
|
||||
None => Vec::new(),
|
||||
}
|
||||
}
|
||||
|
|
@ -2581,6 +2595,15 @@ fn shift_time(base: &TimePoint, offset: &MusicalDuration) -> TimePoint {
|
|||
/// agrees with the notes. The sequence is not assumed sorted: `[bass@1, treble@0]`
|
||||
/// resolves a note after time 1 to bass and one before to treble.
|
||||
pub fn active_clef(clefs: &[PlacedClef], at: &TimePoint) -> Clef {
|
||||
active_clef_or(clefs, at, Clef::default())
|
||||
}
|
||||
|
||||
/// The clef in force at `at`, falling back to `default` — the staff's own
|
||||
/// `Staff::default_clef` — when the sequence names none. A staff that declares
|
||||
/// its clef only on the `Staff` (no `ClefChange` at all) engraves in that clef;
|
||||
/// `active_clef` is this with the treble default, kept for callers that have no
|
||||
/// staff to hand.
|
||||
pub fn active_clef_or(clefs: &[PlacedClef], at: &TimePoint, default: Clef) -> Clef {
|
||||
clefs
|
||||
.iter()
|
||||
.filter(|placed| {
|
||||
|
|
@ -2592,7 +2615,7 @@ pub fn active_clef(clefs: &[PlacedClef], at: &TimePoint) -> Clef {
|
|||
.max_by(|a, b| time_total(&a.time, &b.time))
|
||||
.or_else(|| clefs.iter().min_by(|a, b| time_total(&a.time, &b.time)))
|
||||
.map(|p| p.clef)
|
||||
.unwrap_or_default()
|
||||
.unwrap_or(default)
|
||||
}
|
||||
|
||||
/// The musical origin (the active-clef query for an unanchored pitch).
|
||||
|
|
@ -3406,6 +3429,7 @@ mod tests {
|
|||
let instance = StaffInstanceId::from_raw(1);
|
||||
// D major (two sharps), default treble clef.
|
||||
let content = LayoutContent::Staff(StaffContent {
|
||||
default_clef: Clef::default(),
|
||||
clefs: vec![],
|
||||
keys: vec![PlacedKeySignature {
|
||||
time: TimePoint::Musical(MusicalPosition::origin()),
|
||||
|
|
@ -3718,6 +3742,7 @@ mod tests {
|
|||
let staff = StaffId::from_raw(10);
|
||||
let at = |n, d| TimePoint::Musical(MusicalPosition(RationalTime::new(n, d).unwrap()));
|
||||
let content = LayoutContent::Staff(StaffContent {
|
||||
default_clef: Clef::default(),
|
||||
// Authored out of order: bass at time 1, treble at time 0.
|
||||
clefs: vec![
|
||||
PlacedClef {
|
||||
|
|
@ -4050,6 +4075,7 @@ mod tests {
|
|||
TypedObjectId::StaffInstance(StaffInstanceId::from_raw(si)),
|
||||
staff,
|
||||
LayoutContent::Staff(StaffContent {
|
||||
default_clef: Clef::default(),
|
||||
clefs: vec![],
|
||||
keys: vec![],
|
||||
}),
|
||||
|
|
@ -4808,6 +4834,53 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
/// A staff that declares its clef ONLY on the `Staff` — no `ClefChange` at
|
||||
/// all — engraves in that clef. `Staff::default_clef` used to be decorative:
|
||||
/// the projection took the active clef from the staff instance's sequence and
|
||||
/// fell back to `Clef::default()`, so a bass staff drew a treble clef and put
|
||||
/// every note two steps wrong (P13-I2).
|
||||
#[test]
|
||||
fn a_staff_declaring_only_a_default_clef_engraves_in_it() {
|
||||
let (mut score, _) = repeat_ready_score(48);
|
||||
let staff_id = score.canvas.regions[0].staff_instances()[0].staff;
|
||||
for staff in &mut score.staves {
|
||||
if staff.id == staff_id {
|
||||
staff.default_clef = Clef::bass();
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
score.canvas.regions[0].staff_instances()[0]
|
||||
.clef_sequence
|
||||
.is_empty(),
|
||||
"the staff declares no ClefChange — only its default"
|
||||
);
|
||||
let instance_id = score.canvas.regions[0].staff_instances()[0].id;
|
||||
let c = to_constrained(&to_logical(&score));
|
||||
|
||||
// THIS staff's clef glyph is the bass clef, not the treble default. The
|
||||
// score's other staves keep their own defaults, so scope by provenance.
|
||||
let drawn: Vec<&str> = c
|
||||
.glyphs
|
||||
.iter()
|
||||
.filter(|g| g.provenance.source == TypedObjectId::StaffInstance(instance_id))
|
||||
.map(|g| g.glyph.as_str())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
drawn,
|
||||
vec!["fClef"],
|
||||
"a staff whose only clef is its default draws it"
|
||||
);
|
||||
// And its notes are placed against that clef: `active_clef_or` is what the
|
||||
// staff-position computation reads, so the two can never disagree.
|
||||
let at = TimePoint::Musical(epiphany_core::MusicalPosition::origin());
|
||||
assert_eq!(active_clef_or(&[], &at, Clef::bass()), Clef::bass());
|
||||
assert_eq!(
|
||||
active_clef(&[], &at),
|
||||
Clef::default(),
|
||||
"the old API is intact"
|
||||
);
|
||||
}
|
||||
|
||||
/// `req:layoutir:coverage-diagnostics`: an object the projection cannot
|
||||
/// engrave faithfully is **recorded and still placed** — never guessed at,
|
||||
/// never dropped. A percussion clef has no bundled glyph, so the staff
|
||||
|
|
|
|||
|
|
@ -93,8 +93,8 @@ pub use cache::{
|
|||
ResolvedSystemCache, SystemId,
|
||||
};
|
||||
pub use constrained::{
|
||||
active_clef, is_rigid_width_stroke, to_constrained, try_to_constrained, Axis, BreakClass,
|
||||
BreakKind, BreakOrigin, ConstrainedLayoutIR, ConstrainedLayoutRegion,
|
||||
active_clef, active_clef_or, is_rigid_width_stroke, to_constrained, try_to_constrained, Axis,
|
||||
BreakClass, BreakKind, BreakOrigin, ConstrainedLayoutIR, ConstrainedLayoutRegion,
|
||||
ConstrainedValidationError, ConstraintParameters, ConstraintRegistryId, Curve, GlyphObject,
|
||||
GlyphObjectId, GlyphStyle, LayoutConstraint, LayoutTransformError, SpringSlot, Stroke,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -75,6 +75,12 @@ pub enum LayoutContent {
|
|||
pub struct StaffContent {
|
||||
pub clefs: Vec<PlacedClef>,
|
||||
pub keys: Vec<PlacedKeySignature>,
|
||||
/// The staff's own `default_clef` — the clef in force before the first
|
||||
/// `ClefChange`, and throughout a staff that declares none. Carried here
|
||||
/// because the clef belongs to the `Staff` while the sequence belongs to the
|
||||
/// `StaffInstance`, and every consumer resolving "the clef at time t" needs
|
||||
/// both (`active_clef_or`).
|
||||
pub default_clef: Clef,
|
||||
}
|
||||
|
||||
/// A clef change with its score anchor resolved into the layout time axis.
|
||||
|
|
@ -827,7 +833,14 @@ fn derive_score_version(score: &Score) -> ScoreVersion {
|
|||
/// are carried as-is — the constrained pass defaults the *active* clef/key to
|
||||
/// treble / C major.
|
||||
fn staff_content(score: &Score, si: &epiphany_core::StaffInstance) -> LayoutContent {
|
||||
let default_clef = score
|
||||
.staves
|
||||
.iter()
|
||||
.find(|staff| staff.id == si.staff)
|
||||
.map(|staff| staff.default_clef)
|
||||
.unwrap_or_default();
|
||||
LayoutContent::Staff(StaffContent {
|
||||
default_clef,
|
||||
clefs: si
|
||||
.clef_sequence
|
||||
.iter()
|
||||
|
|
|
|||
|
|
@ -218,6 +218,7 @@ pub fn gen_layout_content(rng: &mut Rng) -> LayoutContent {
|
|||
match rng.below(6) {
|
||||
0 => LayoutContent::Structural,
|
||||
1 => LayoutContent::Staff(StaffContent {
|
||||
default_clef: epiphany_core::Clef::default(),
|
||||
clefs: clefs(rng),
|
||||
keys: keys(rng),
|
||||
}),
|
||||
|
|
|
|||
|
|
@ -31,5 +31,5 @@ what is true.
|
|||
| Id | One-line statement | Filed in | Status |
|
||||
|---|---|---|---|
|
||||
| P13-I1 | Chapter 7's `ConstrainedLayoutIR` listing elides **three** fields the code carries: `break_origins: Vec<BreakOrigin>` (named by `req:layoutir:break-origin-attribution`, its own shape unlisted), `catalog: GlyphCatalogIdentity` (its type specified, the field unlisted), and `diagnostics: Vec<LayoutDiagnostic>` — which appears **nowhere** in core_spec, though it is how the projection's honesty rule manifests: an unspellable pitch or an unbundled glyph is placed as a fallback *and recorded*, never silently guessed | `crates/epiphany-layout-ir/DECISIONS.md` ("the ConstrainedLayoutIR listing is still abridged") | **resolved** (Pass 13: listing gains all three fields; `BreakOrigin` and `LayoutDiagnostic` shapes added; new `req:layoutir:coverage-diagnostics` ratifies as-implemented that an unengravable object is recorded AND still placed — never guessed, never dropped) |
|
||||
| P13-I2 | `Staff::default_clef` is never consulted: `to_constrained` takes the active clef from the staff instance's `clef_sequence` and falls back to `Clef::default()` (treble), so a bass-clef staff that declares its clef only on the `Staff` engraves as treble. The field is decorative in the projection — is it the fallback, or should it not exist? | `crates/epiphany-layout-ir/DECISIONS.md` ("`Staff::default_clef` is never consulted") | **open** |
|
||||
| P13-I2 | `Staff::default_clef` is never consulted: `to_constrained` takes the active clef from the staff instance's `clef_sequence` and falls back to `Clef::default()` (treble), so a bass-clef staff that declares its clef only on the `Staff` engraves as treble. The field is decorative in the projection — is it the fallback, or should it not exist? | `crates/epiphany-layout-ir/DECISIONS.md` ("`Staff::default_clef` is never consulted") | **resolved** (Pass 13: it IS the fallback. `StaffContent` carries it, `active_clef_or` resolves against it, and `editor-core`'s hit-test reads the same function — else a click on a bass staff would resolve its pitch as treble. Removal was rejected: the field is named for its purpose, is encoded on the wire, and dropping it is schema-major) |
|
||||
| P13-I3 | `BRAVURA_METRICS`' `NOTEHEAD_ANCHORS` are hand-written, unconsumed, and doubly suspect: they name `stemUpNW`/`stemDownSE` — the corners a normal notehead's stems do *not* attach to, and a pair Bravura's `noteheadBlack` does not define — and their x of `1180` reads like 1.18 staff spaces written in thousandths rather than the table's `1/1024` units (1.18 sp = 1208). They enter only `metrics_hash`, so any correction moves the `GlyphCatalogIdentity` every conformance claim declares. The font is not vendored, so the values cannot be verified in-tree | `crates/epiphany-layout-ir/DECISIONS.md` ("the notehead stem anchors are unusable as written") | **open** |
|
||||
|
|
|
|||
Loading…
Reference in New Issue