diff --git a/crates/epiphany-core/src/pitch.rs b/crates/epiphany-core/src/pitch.rs index fc922ba..1923d27 100644 --- a/crates/epiphany-core/src/pitch.rs +++ b/crates/epiphany-core/src/pitch.rs @@ -760,6 +760,50 @@ pub struct PitchSpelling { } impl PitchSpelling { + /// Moves this spelling by `interval`, keeping its enharmonic choice. + /// + /// The authored **nominal** is what carries that choice — an author who + /// wrote B♯3 rather than C4 chose the letter B — so the diatonic component + /// moves the nominal and octave, and the accidental stack is then whatever + /// the transposed pitch requires on that new staff line. + /// `sounding_semitone` is the transposed pitch's + /// [`Pitch::twelve_tet_semitone`]. + /// + /// So B♯3 (sounding C4) transposed by a perfect fifth `(4, 7)` becomes + /// F×3 (sounding G4): the letter moved four diatonic steps, and the + /// double-sharp is what F needs to sound a G. The chromatic component never + /// touches the spelling directly; it reaches it only through the pitch. + /// + /// `None` when the spelling is not CMN (no nominal to move), or when the + /// resulting octave or alteration is not representable. + pub fn transposed( + &self, + interval: TranspositionInterval, + sounding_semitone: i32, + ) -> Option { + let SpellingNominal::Cmn(nominal) = self.nominal else { + return None; + }; + // `i64` throughout, for the same reason `Pitch::transposed` does. + let step = i64::from(nominal as u8) + i64::from(interval.diatonic_steps); + let new_nominal = CmnNominal::from_index(step.rem_euclid(7) as i32); + let new_octave = i64::from(self.octave) + step.div_euclid(7); + let alteration = + i64::from(sounding_semitone) - (i64::from(new_nominal.chromatic()) + 12 * new_octave); + + let octave = i8::try_from(new_octave).ok()?; + let alteration = i32::try_from(alteration).ok()?; + // Guard the accidental stack: `accidental_ids` allocates |alteration|/2 + // glyphs, so an unbounded alteration is an unbounded allocation. + i8::try_from(alteration).ok()?; + Some(PitchSpelling { + nominal: SpellingNominal::Cmn(new_nominal), + accidentals: crate::prepass::accidental_ids(alteration), + octave, + render_hints: self.render_hints, + }) + } + /// A bare CMN spelling with no accidental glyph at the given octave. pub fn cmn(nominal: CmnNominal, octave: i8) -> Self { PitchSpelling { @@ -1132,6 +1176,68 @@ mod tests { assert_eq!(iv(4, 7).inverse(), Some(iv(-4, -7))); } + #[test] + fn a_spelling_moves_by_its_nominal_and_keeps_the_authors_enharmonic_choice() { + let semitone = |p: &Pitch| p.twelve_tet_semitone().unwrap(); + + // The author wrote C-sharp, not D-flat. Up a perfect fifth that must be + // G-sharp, not A-flat: the pre-pass, left to itself, may prefer either. + let cs4 = cmn(CmnNominal::C, 1, 4); + let spelled = PitchSpelling { + nominal: SpellingNominal::Cmn(CmnNominal::C), + accidentals: crate::prepass::accidental_ids(1), + octave: 4, + render_hints: SpellingRenderHints::default(), + }; + let up = cs4.transposed(iv(4, 7)).unwrap(); + let moved = spelled.transposed(iv(4, 7), semitone(&up)).unwrap(); + assert_eq!(moved.nominal, SpellingNominal::Cmn(CmnNominal::G)); + assert_eq!(moved.octave, 4); + assert_eq!(moved.accidentals, crate::prepass::accidental_ids(1)); + + // B-sharp 3 sounds as C4. Up a fifth it must stay a *letter F*, spelled + // F-double-sharp 4, sounding G4 — the nominal carries the choice. + let bs3 = PitchSpelling { + nominal: SpellingNominal::Cmn(CmnNominal::B), + accidentals: crate::prepass::accidental_ids(1), + octave: 3, + render_hints: SpellingRenderHints::default(), + }; + let c4 = cmn(CmnNominal::C, 0, 4); + let g4 = c4.transposed(iv(4, 7)).unwrap(); + let moved = bs3.transposed(iv(4, 7), semitone(&g4)).unwrap(); + assert_eq!(moved.nominal, SpellingNominal::Cmn(CmnNominal::F)); + assert_eq!(moved.octave, 4); + assert_eq!(moved.accidentals, crate::prepass::accidental_ids(2)); + + // A plain sharpen: the staff line never moves. + let c = PitchSpelling::cmn(CmnNominal::C, 4); + let sharp = c4.transposed(iv(0, 1)).unwrap(); + let moved = c.transposed(iv(0, 1), semitone(&sharp)).unwrap(); + assert_eq!(moved.nominal, SpellingNominal::Cmn(CmnNominal::C)); + assert_eq!(moved.octave, 4); + assert_eq!(moved.accidentals, crate::prepass::accidental_ids(1)); + } + + #[test] + fn a_spelling_refuses_to_move_when_it_cannot_be_written() { + let c = PitchSpelling::cmn(CmnNominal::C, 4); + // Octave out of range, and an extreme interval that must not panic. + assert_eq!(c.transposed(iv(7 * 200, 12 * 200), 0), None); + assert_eq!(c.transposed(iv(i32::MAX, 0), 0), None); + assert_eq!(c.transposed(iv(i32::MIN, 0), 0), None); + // An alteration that will not fit an `i8`. + assert_eq!(c.transposed(iv(0, 0), 4000), None); + // A non-CMN nominal has no letter to move. + let integer = PitchSpelling { + nominal: SpellingNominal::Integer(7), + accidentals: Vec::new(), + octave: 4, + render_hints: SpellingRenderHints::default(), + }; + assert_eq!(integer.transposed(iv(4, 7), 55), None); + } + #[test] fn a_transposition_refuses_a_non_cmn_position() { let mut p = cmn(CmnNominal::C, 0, 4); diff --git a/crates/epiphany-core/src/prepass.rs b/crates/epiphany-core/src/prepass.rs index 5156ec7..ff06830 100644 --- a/crates/epiphany-core/src/prepass.rs +++ b/crates/epiphany-core/src/prepass.rs @@ -595,7 +595,7 @@ fn lof_candidates(pitch_class: i32) -> Vec { /// The accidental glyph stack for a chromatic alteration. An empty stack means /// *no glyph is drawn* (a natural with no key signature to cancel), distinct /// from a natural sign (Chapter 2 §"Absent Accidentals"). -fn accidental_ids(alteration: i32) -> Vec { +pub(crate) fn accidental_ids(alteration: i32) -> Vec { // The inferred candidate band ([`LOF_MIN`, `LOF_MAX`]) only ever yields single // accidentals, but the *authored* path preserves any `alteration` an author // wrote (`alteration` is an `i8`), so a triple-sharp can reach here. Emit a diff --git a/crates/epiphany-ops/src/reduce.rs b/crates/epiphany-ops/src/reduce.rs index 4248383..da95568 100644 --- a/crates/epiphany-ops/src/reduce.rs +++ b/crates/epiphany-ops/src/reduce.rs @@ -39,7 +39,8 @@ use epiphany_core::{ RegionTimeModel, ReplicaId, Score, ScoreMetadata, SpellingAttachment, SpellingDirective, SpellingScope, SpellingSource, Staff, StaffId, StaffInstance, StaffInstanceId, StaffLineConfiguration, TempoMap, TempoSegment, TempoShape, TimeAnchor, TimeSignature, - TimeSignatureId, TransactionId, TransposeRefusal, TypedObjectId, Voice, VoiceId, VoiceOrigin, + TimeSignatureId, TransactionId, TransposeRefusal, TranspositionInterval, TypedObjectId, Voice, + VoiceId, VoiceOrigin, }; use epiphany_determinism::CanonicalEncode; @@ -747,6 +748,15 @@ type StaffLayoutValue = (Option, Option, b /// (spelling, breaks) keep the [`Predecessor`] distinction: a base predecessor /// returns the ledger map to key-absence while the graph restores the base /// value. +/// One target's fully-resolved transposition, computed before anything is +/// written so the operation can refuse atomically. +struct ResolvedTranspose { + pitch: PitchId, + value: Pitch, + /// `(index into score.spelling_attachments, moved spelling)`. + authored: Vec<(usize, PitchSpelling)>, +} + enum ValueRestoration { Event { event: EventId, @@ -5604,17 +5614,17 @@ impl<'a> Reducer<'a> { }; } - // Atomicity: resolve every target's new value BEFORE writing any of - // them. `targets` is a set, so `mutable` is in canonical order, and the - // reported failure is the canonically-first offender — - // `req:opcat:transpose-interval-atomic`. - let mut resolved: Vec<(PitchId, Pitch)> = Vec::with_capacity(mutable.len()); + // Atomicity: resolve every target's new value AND every spelling + // rewrite BEFORE writing any of them. `targets` is a set, so `mutable` + // is in canonical order, and the reported failure is the + // canonically-first offender — `req:opcat:transpose-interval-atomic`. + let mut resolved: Vec = Vec::with_capacity(mutable.len()); for pitch in &mutable { let Some(current) = self.graph_pitch_value(*pitch) else { continue; // base-free: no value to check, and none to write }; - match current.transposed(op.interval) { - Ok(next) => resolved.push((*pitch, next)), + let next = match current.transposed(op.interval) { + Ok(next) => next, Err(refusal) => { let reason = match refusal { TransposeRefusal::NonCmnPosition => { @@ -5631,16 +5641,90 @@ impl<'a> Reducer<'a> { reason: NoOpReason::PreconditionFailedUnderReduction { reason }, }; } + }; + match self.resolve_transposed_spellings(*pitch, &next, op.interval) { + Some(authored) => resolved.push(ResolvedTranspose { + pitch: *pitch, + value: next, + authored, + }), + // An authored spelling that cannot be written at the transposed + // staff position refuses the whole operation, like any other + // untransposable target. Silently leaving it stale is what this + // operation exists to stop. + None => { + return OperationEffect::NoOp { + reason: NoOpReason::PreconditionFailedUnderReduction { + reason: PreconditionFailureReason::TranspositionOutOfRange, + }, + } + } } } - for (pitch, value) in resolved { - self.graph_modify_pitch(pitch, &value); - self.graph_propagate_spelling(pitch, &value); + // Two passes. The authored rewrites address `spelling_attachments` by + // index, and the propagated upsert may PUSH — which would shift every + // later target's indices. So every rewrite lands before any append. + for r in &resolved { + self.graph_modify_pitch(r.pitch, &r.value); + self.graph_rewrite_authored_spellings(&r.authored); + } + for r in &resolved { + self.graph_propagate_spelling(r.pitch, &r.value); } OperationEffect::Applied } + /// The engraved-layer authored spellings on `pitch`, each moved to where the + /// transposed pitch writes it. `None` if any of them cannot be written + /// there. + /// + /// `Propagated` attachments are skipped: they are this operation's own + /// output, regenerated from the transposed value. Everything else — + /// `UserChosen`, `Imported` — is an assertion that *this pitch is written + /// like so*, and once the pitch moves that assertion has to move with it or + /// it is simply false. (Import fidelity is a property of the file on disk, + /// which a transposition does not touch.) + fn resolve_transposed_spellings( + &self, + pitch: PitchId, + transposed: &Pitch, + interval: TranspositionInterval, + ) -> Option> { + let Some(score) = self.graph.as_ref() else { + return Some(Vec::new()); + }; + let semitone = transposed.twelve_tet_semitone()?; + let mut out = Vec::new(); + for (index, att) in score.spelling_attachments.iter().enumerate() { + if att.layer.is_some() || matches!(att.source, SpellingSource::Propagated { .. }) { + continue; + } + if !matches!(&att.scope, SpellingScope::Pitch(p) if *p == pitch) { + continue; + } + let SpellingDirective::Explicit(spelling) = &att.directive else { + continue; + }; + out.push((index, spelling.transposed(interval, semitone)?)); + } + Some(out) + } + + /// Applies the rewrites `resolve_transposed_spellings` computed, preserving + /// each attachment's `source`, `priority`, and `layer`. A transposed + /// `UserChosen` spelling is still the user's choice. + fn graph_rewrite_authored_spellings(&mut self, rewrites: &[(usize, PitchSpelling)]) { + let Some(score) = self.graph.as_mut() else { + return; + }; + for (index, spelling) in rewrites { + if let Some(att) = score.spelling_attachments.get_mut(*index) { + att.directive = SpellingDirective::Explicit(spelling.clone()); + } + } + } + /// The current value of a live embedded pitch, or `None` under base-free /// reduction (no graph) or if the pitch is not embedded in any event. fn graph_pitch_value(&self, pitch: PitchId) -> Option { @@ -5661,14 +5745,20 @@ impl<'a> Reducer<'a> { /// "editing operations that move or transpose a pitch MUST produce /// attachments with source `Propagated`"). /// - /// Without this the spelling pre-pass re-infers a spelling for the moved - /// pitch, and any authored `UserChosen` attachment stays pinned to the - /// notehead it was written against — a deliberately-spelled C# transposed - /// up a fifth, re-inferred as Ab against the author's wish. The interval's - /// diatonic component already chose the nominal; `simplest_spelling` on a - /// `Cmn` position returns that authored letter verbatim (it infers only for - /// non-`Cmn` positions, which this operation refuses), so the attachment - /// carries exactly the interval's determination. + /// This is the record for a pitch with **no authored spelling**: without it + /// the pre-pass re-infers, and may pick the enharmonic the interval did not + /// choose. `simplest_spelling` on a `Cmn` position returns that position's + /// letter verbatim (it infers only for non-`Cmn` positions, which this + /// operation refuses), so the attachment carries exactly the interval's + /// determination. + /// + /// It does **not** by itself fix a *stale authored* spelling: the default + /// precedence ranks `UserChosen` and `Imported` above `Propagated` + /// (Chapter 2 §"Configurable Precedence"), so an authored attachment wins + /// and would engrave the pre-transpose notehead — a C#4 drawn as a C + /// natural, accidental and all. Authored spellings are therefore *moved*, + /// by `resolve_transposed_spellings`; this attachment is the fallback for + /// pitches that have none. fn graph_propagate_spelling(&mut self, pitch: PitchId, value: &Pitch) { let Some(spelling) = simplest_spelling(value) else { return; @@ -9203,6 +9293,134 @@ mod tests { assert_eq!(spelling.accidentals.len(), 1); } + /// The effective engraved spelling for `pitch`, through the same resolver + /// the pre-pass uses — which is the only thing that says what gets drawn. + fn engraved_spelling( + score: &Score, + pitch: PitchId, + ) -> (epiphany_core::SpellingProvenance, PitchSpelling) { + let value = pitch_of(score, pitch); + let inferred = simplest_spelling(&value).expect("cmn"); + let resolved = epiphany_core::resolve_spelling( + score, + pitch, + inferred, + &epiphany_core::SpellingPrecedence::default(), + ); + (resolved.provenance, resolved.spelling) + } + + fn author_spelling(score: &mut Score, pitch: PitchId, spelling: PitchSpelling) { + score.spelling_attachments.push(SpellingAttachment { + scope: SpellingScope::Pitch(pitch), + directive: SpellingDirective::Explicit(spelling), + source: SpellingSource::UserChosen, + priority: 0, + layer: None, + }); + } + + #[test] + fn transpose_interval_moves_an_authored_spelling_instead_of_leaving_it_stale() { + // The gap the original test could not see, because it started with no + // attachment: default precedence ranks UserChosen ABOVE Propagated, so + // recording a Propagated attachment does not, by itself, stop the + // engraved layer from drawing the pre-transpose notehead. Before this + // fix, a C4 sharpened to C#4 still RESOLVED to C natural — the + // accidental vanished. + let (mut base, pid) = base_with_pitch(cmn_pitch(CmnNominal::C, 0, 4)); + author_spelling(&mut base, pid, PitchSpelling::cmn(CmnNominal::C, 4)); + + let (effect, score) = run_transpose(&base, &[pid], interval(0, 1)); + assert_eq!(effect, OperationEffect::Applied); + assert_eq!(cmn_of(&pitch_of(&score, pid)), (CmnNominal::C, 1, 4)); + + let (provenance, spelling) = engraved_spelling(&score, pid); + // Still the author's choice — a transposed UserChosen spelling is still + // user-chosen — but it now names the pitch that is actually there. + assert_eq!( + provenance, + epiphany_core::SpellingProvenance::Authored( + epiphany_core::SpellingSourceKind::UserChosen + ) + ); + assert_eq!( + spelling.nominal, + epiphany_core::SpellingNominal::Cmn(CmnNominal::C) + ); + assert_eq!(spelling.octave, 4); + assert_eq!(spelling.accidentals.len(), 1, "the sharp must be drawn"); + } + + #[test] + fn a_transposed_authored_spelling_keeps_the_authors_enharmonic_choice() { + // The discriminating case: the authored spelling's NOMINAL must disagree + // with the pitch's own Cmn nominal, or re-inferring from the pitch would + // give the same answer and the test would prove nothing. (It did. The + // mutation that replaced `spelling.transposed(..)` with + // `simplest_spelling(transposed)` passed the first version of this test, + // which spelled a C#4 pitch as "C#".) + // + // So: a pitch sounding C4, which the author deliberately wrote as B#3. + // Up a perfect fifth it sounds G4 — and must be written F##4, keeping + // the letter the author's B implies. Re-inference would say "G". + let (mut base, pid) = base_with_pitch(cmn_pitch(CmnNominal::C, 0, 4)); + author_spelling( + &mut base, + pid, + PitchSpelling { + nominal: epiphany_core::SpellingNominal::Cmn(CmnNominal::B), + accidentals: vec![epiphany_core::AccidentalId::new("sharp")], + octave: 3, + render_hints: Default::default(), + }, + ); + + let (effect, score) = run_transpose(&base, &[pid], interval(4, 7)); + assert_eq!(effect, OperationEffect::Applied); + // The pitch itself is plain G4. + assert_eq!(cmn_of(&pitch_of(&score, pid)), (CmnNominal::G, 0, 4)); + + // The engraved spelling is F-double-sharp 4, not G. + let (provenance, spelling) = engraved_spelling(&score, pid); + assert_eq!( + provenance, + epiphany_core::SpellingProvenance::Authored( + epiphany_core::SpellingSourceKind::UserChosen + ) + ); + assert_eq!( + spelling.nominal, + epiphany_core::SpellingNominal::Cmn(CmnNominal::F), + "the author's letter moved; re-inference would have said G" + ); + assert_eq!(spelling.octave, 4); + assert_eq!( + spelling.accidentals, + vec![epiphany_core::AccidentalId::new("double-sharp")] + ); + } + + #[test] + fn an_untransposable_authored_spelling_refuses_the_whole_operation() { + // The pitch itself transposes fine; its authored spelling cannot be + // written at the new staff position. Leaving it stale is exactly the + // bug, so the operation refuses and nothing moves. + let (mut base, pid) = base_with_pitch(cmn_pitch(CmnNominal::C, 0, 4)); + author_spelling(&mut base, pid, PitchSpelling::cmn(CmnNominal::C, 127)); + + let (effect, score) = run_transpose(&base, &[pid], interval(7, 12)); + assert_eq!( + effect, + OperationEffect::NoOp { + reason: NoOpReason::PreconditionFailedUnderReduction { + reason: PreconditionFailureReason::TranspositionOutOfRange, + }, + } + ); + assert_eq!(cmn_of(&pitch_of(&score, pid)), (CmnNominal::C, 0, 4)); + } + #[test] fn transpose_interval_skips_a_tombstoned_target_but_refuses_a_missing_one() { // The skip/refuse distinction: a deleted pitch is not an untransposable diff --git a/spec/operation_catalog.pdf b/spec/operation_catalog.pdf index 60ab587..79abe7c 100644 Binary files a/spec/operation_catalog.pdf and b/spec/operation_catalog.pdf differ diff --git a/spec/operation_catalog.tex b/spec/operation_catalog.tex index f04c0dd..ad605ef 100644 --- a/spec/operation_catalog.tex +++ b/spec/operation_catalog.tex @@ -737,21 +737,49 @@ is transposed by \texttt{interval}. \begin{requirement} \label{req:opcat:transpose-interval-spelling} - A \texttt{TransposeInterval} that changes a pitch's spelling \MUST{} record - that spelling as a \texttt{SpellingAttachment} with source + A \texttt{TransposeInterval} \MUST{} record the spelling its interval + determined as a \texttt{SpellingAttachment} with source \texttt{SpellingSource::Propagated}, per the core specification's Chapter~2, \sectionsc{Spelling Sources}. The attachment's \texttt{from} is the transposed pitch's own identifier, which the operation preserves. + + That attachment alone is \emph{not sufficient}, because the default + precedence ranks \texttt{UserChosen} and \texttt{Imported} above + \texttt{Propagated} (Chapter~2, \sectionsc{Configurable Precedence}). A + \texttt{TransposeInterval} \MUST{} therefore also \textbf{move} every + engraved-layer, pitch-scoped, explicit spelling attachment on each target + that is not itself \texttt{Propagated}, preserving each attachment's + \texttt{source}, \texttt{priority}, and \texttt{layer}. A spelling is moved + by transposing its \emph{nominal} diatonically and taking whatever accidental + the transposed pitch then requires at that staff position. If any such + attachment cannot be written at the transposed position, the operation + \MUST{} refuse, atomically, with \texttt{TranspositionOutOfRange}. \end{requirement} \begin{rationale} - Without the propagated attachment the spelling pre-pass re-infers a spelling - for the moved pitch, and an authored \texttt{UserChosen} spelling from before - the transposition stays pinned to a notehead that has moved --- a C-sharp the - author spelled deliberately, transposed up a fifth, re-inferred as A-flat - against their wish. The interval's diatonic component already determines the - correct nominal; the attachment is how that determination survives into the - engraved layer. + Two different failures hide here, and the propagated attachment addresses only + the first. + + For a pitch with \emph{no} authored spelling, the pre-pass would re-infer, and + may pick the enharmonic the interval did not choose. The propagated attachment + pins the interval's determination. + + For a pitch \emph{with} an authored spelling, the propagated attachment is + outranked and changes nothing: a C4 the author spelled ``C'', sharpened to + C-sharp~4, would still resolve to \texttt{Authored(UserChosen)} and engrave a + C natural --- the accidental simply disappears, and the notehead names a pitch + that is not there. So the authored spelling is moved instead of being left or + discarded. It is moved by its \textbf{nominal}, because the nominal is what + carries the author's enharmonic decision: an author who wrote B-sharp~3 rather + than C4 chose the letter B, and a perfect fifth up must give F-double-sharp~4, + not G. Discarding the attachment would silently lose that decision; re-inferring + it from the transposed pitch would give G. + + A transposed \texttt{UserChosen} spelling is still the user's choice, so its + source is preserved. \texttt{Imported} likewise: an imported attachment asserts + ``this pitch is written thus'', and once the pitch moves that assertion must + move with it or become false. Import fidelity is a property of the source file, + which a transposition does not touch. \end{rationale} \textbf{Conflict cases.} None --- composition is deterministic in canonical order.