From d923b352aed16ad8c81349d2815760d2089931c7 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 30 Jul 2026 20:11:06 -0400 Subject: [PATCH] fix: harden note-program model against corrupt tempo/division/denomination records Reject subnormal BPM that overflows to NaN, normalize Division equality, pin a single out-of-range-denomination interpretation across all readers, flag collapsed capture windows, add offset off-view editors and structural static_asserts, collapse duplicated CLAUDE.md facts. --- src/core/instrument/note/CLAUDE.md | 21 ++--- src/core/instrument/note/CMakeLists.txt | 8 +- src/core/instrument/note/musical_division.cpp | 6 +- src/core/instrument/note/note_program.cpp | 43 +++++++--- src/core/instrument/note/note_program.h | 20 ++++- src/core/instrument/note/tempo.cpp | 4 + src/core/instrument/note/tempo.h | 4 + tests/test_musical_division.cpp | 24 ++++-- tests/test_note_program.cpp | 78 +++++++++++++++++-- tests/test_tempo.cpp | 3 + 10 files changed, 175 insertions(+), 36 deletions(-) diff --git a/src/core/instrument/note/CLAUDE.md b/src/core/instrument/note/CLAUDE.md index 4f3c7c2..9a00025 100644 --- a/src/core/instrument/note/CLAUDE.md +++ b/src/core/instrument/note/CLAUDE.md @@ -29,16 +29,18 @@ diverge: the capture-signal popup that edits it and the bake that renders it. - **A division persists as its `{quarterExponent, modifier}` pair, never as its picker index.** The index is presentation order and would silently re-map every saved record if the ladder ever gained a rung or a modifier. -- **An offset stores the denomination it was entered in.** The other view is derived on - demand. Storing resolved seconds instead would make a beats-denominated offset stop - following the tempo, which is the only reason to express one in beats. +- **An offset stores the denomination it was entered in** — see `OffsetAmount` in + `note_program.h` for why. +- **Does not carry a MIDI note number.** `NoteProgram` describes timing and velocity only; + render pitch is deferred to a later additive field (Ξ-W2) rather than assumed to live + here. ## Modules - `musical_division` — the note-length ladder: 1/64 through 64/1 (a rung is the base-2 exponent of its length in quarter notes, -4..8), each straight, dotted (x3/2), or triplet - (x2/3); the 39-entry picker order; and the `"1/8."` / `"1/4t"` label notation. Lengths in - beats only, so it links no tempo. + (x2/3); the 39-entry picker order; and the `"1/8."` / `"1/4t"` label notation. Beats only + — see `musical_division.h` for why it links no tempo. - `tempo` — a validated project tempo plus every beats <-> seconds <-> ms conversion. Construction (`Tempo::fromBpm`) is the only place a bad BPM is rejected, which is what lets each conversion be total and every downstream resolver be failure-free. @@ -48,10 +50,11 @@ diverge: the capture-signal popup that edits it and the bake that renders it. ## Gotchas -- **A beat is a quarter note.** REAPER states project tempo in quarter notes per minute - regardless of time signature, so divisions resolve with no time signature in sight. A - beats *readout* that should track a compound meter's dotted-quarter pulse would need the - time signature threaded in — it is not, deliberately. +- **A beat is a quarter note** — see `tempo.h` for why. A beats *readout* that should track + a compound meter's dotted-quarter pulse would need the time signature threaded in — it is + not, deliberately. `src/core/ui/card_meta.cpp` is the sibling module that *does* fold + `timeSigDenom` into its own seconds-per-beat — a different, also-correct convention for a + different job; don't read the divergence as a bug in either. - **`StartOffset` and `EndOffset` are distinct types on purpose.** They hold the same payload and differ only in what they anchor to (note-on and note-off respectively); collapsing them into one type with an anchor field makes the swap a runtime bug instead diff --git a/src/core/instrument/note/CMakeLists.txt b/src/core/instrument/note/CMakeLists.txt index 25dee5e..dbffa83 100644 --- a/src/core/instrument/note/CMakeLists.txt +++ b/src/core/instrument/note/CMakeLists.txt @@ -1,13 +1,13 @@ reasampler_pure_library(musical_division SOURCES musical_division.cpp) -# Links only musical_division: the ladder is beats-only, so it must prove out with no tempo -# in the link line at all. +# Links only musical_division — the beats-only contract (see musical_division.h) needs no +# tempo in the link line. reasampler_test(musical_division LINK musical_division) reasampler_pure_library(tempo SOURCES tempo.cpp) reasampler_test(tempo LINK tempo) -# The record composes the ladder and the tempo and nothing else — the programmed signal is -# plain data, provable without the engine, the bank, or a host. +# note_program links exactly these two: it composes the ladder and the tempo and nothing +# else (see note_program.h). reasampler_pure_library(note_program SOURCES note_program.cpp LINK PUBLIC musical_division tempo) diff --git a/src/core/instrument/note/musical_division.cpp b/src/core/instrument/note/musical_division.cpp index 22de6e7..443d0ac 100644 --- a/src/core/instrument/note/musical_division.cpp +++ b/src/core/instrument/note/musical_division.cpp @@ -24,7 +24,11 @@ int clampExponent(int quarterExponent) { } // namespace bool operator==(Division a, Division b) { - return a.quarterExponent == b.quarterExponent && a.modifier == b.modifier; + // Normalize both sides through makeDivision first: a persisted off-ladder exponent must + // compare equal to its clamped form, the same as every other reader in this file. + const Division la = makeDivision(a.quarterExponent, a.modifier); + const Division lb = makeDivision(b.quarterExponent, b.modifier); + return la.quarterExponent == lb.quarterExponent && la.modifier == lb.modifier; } bool operator!=(Division a, Division b) { return !(a == b); } diff --git a/src/core/instrument/note/note_program.cpp b/src/core/instrument/note/note_program.cpp index f5a4be5..683a356 100644 --- a/src/core/instrument/note/note_program.cpp +++ b/src/core/instrument/note/note_program.cpp @@ -24,20 +24,31 @@ OffsetAmount offsetFromMs(double ms) { return {ms, Denomination::Milliseconds}; OffsetAmount offsetFromBeats(double beats) { return {beats, Denomination::Beats}; } +// All three readers switch on Denomination with the same default (Milliseconds, the +// struct's own default value) so a corrupt persisted record reads identically everywhere — +// a popup and a bake must never disagree on an out-of-range denomination byte. double offsetMs(OffsetAmount amount, Tempo tempo) { - return amount.denomination == Denomination::Milliseconds ? amount.magnitude - : tempo.beatsToMs(amount.magnitude); + switch (amount.denomination) { + case Denomination::Beats: return tempo.beatsToMs(amount.magnitude); + case Denomination::Milliseconds: + default: return amount.magnitude; + } } double offsetBeats(OffsetAmount amount, Tempo tempo) { - return amount.denomination == Denomination::Beats ? amount.magnitude - : tempo.msToBeats(amount.magnitude); + switch (amount.denomination) { + case Denomination::Beats: return amount.magnitude; + case Denomination::Milliseconds: + default: return tempo.msToBeats(amount.magnitude); + } } double offsetSeconds(OffsetAmount amount, Tempo tempo) { - return amount.denomination == Denomination::Beats - ? tempo.beatsToSeconds(amount.magnitude) - : msToSeconds(amount.magnitude); + switch (amount.denomination) { + case Denomination::Beats: return tempo.beatsToSeconds(amount.magnitude); + case Denomination::Milliseconds: + default: return msToSeconds(amount.magnitude); + } } OffsetAmount redenominate(OffsetAmount amount, Denomination to, Tempo tempo) { @@ -46,6 +57,18 @@ OffsetAmount redenominate(OffsetAmount amount, Denomination to, Tempo tempo) { : offsetFromMs(offsetMs(amount, tempo)); } +OffsetAmount withMsView(OffsetAmount amount, double ms, Tempo tempo) { + return amount.denomination == Denomination::Milliseconds + ? offsetFromMs(ms) + : offsetFromBeats(tempo.msToBeats(ms)); +} + +OffsetAmount withBeatsView(OffsetAmount amount, double beats, Tempo tempo) { + return amount.denomination == Denomination::Beats + ? offsetFromBeats(beats) + : offsetFromMs(tempo.beatsToMs(beats)); +} + bool operator==(const NoteProgram& a, const NoteProgram& b) { return a.length == b.length && a.start.amount() == b.start.amount() && a.end.amount() == b.end.amount() && a.velocity == b.velocity; @@ -57,10 +80,12 @@ ResolvedNote resolveNote(const NoteProgram& program, Tempo tempo) { ResolvedNote out; out.noteOffSeconds = tempo.beatsToSeconds(divisionBeats(program.length)); out.captureStartSeconds = offsetSeconds(program.start.amount(), tempo); - out.captureEndSeconds = out.noteOffSeconds + offsetSeconds(program.end.amount(), tempo); + const double rawEndSeconds = out.noteOffSeconds + offsetSeconds(program.end.amount(), tempo); // An inverted window has no meaning to a renderer, so a far-negative end offset yields a // zero-length capture the caller can reject rather than a negative one it cannot. - out.captureEndSeconds = (std::max)(out.captureEndSeconds, out.captureStartSeconds); + // windowCollapsed distinguishes that from a genuinely zero-length program. + out.windowCollapsed = rawEndSeconds < out.captureStartSeconds; + out.captureEndSeconds = (std::max)(rawEndSeconds, out.captureStartSeconds); out.velocity = program.velocity.value(); return out; } diff --git a/src/core/instrument/note/note_program.h b/src/core/instrument/note/note_program.h index 02aacad..d0973fa 100644 --- a/src/core/instrument/note/note_program.h +++ b/src/core/instrument/note/note_program.h @@ -1,12 +1,12 @@ // note_program — the programmed capture signal: one note length, one velocity, two anchored // offsets, and the one resolver a preview and a bake must share. // -// Resolved times are rate-free SECONDS relative to note-on; the caller converts against the -// live sample rate. +// Resolved times are rate-free seconds (this directory's CLAUDE.md: the standing ruling). #pragma once #include +#include #include "core/instrument/note/musical_division.h" #include "core/instrument/note/tempo.h" @@ -52,6 +52,12 @@ double offsetSeconds(OffsetAmount amount, Tempo tempo); // The unit toggle: the same instant restated in the other denomination. OffsetAmount redenominate(OffsetAmount amount, Denomination to, Tempo tempo); +// Edit the magnitude via its non-stored view without changing which denomination is stored +// — a popup's ms and beats fields both stay live no matter which one the offset was entered +// in; only `redenominate` changes the stored denomination itself. +OffsetAmount withMsView(OffsetAmount amount, double ms, Tempo tempo); +OffsetAmount withBeatsView(OffsetAmount amount, double beats, Tempo tempo); + // Two types rather than one carrying an anchor field: the anchor is then unswappable at // compile time. Sign is uniform — positive is later in time — so a capture that opens before // the note is a negative start offset, and a negative end offset truncates before release. @@ -75,6 +81,11 @@ private: OffsetAmount amount_{}; }; +static_assert(!std::is_constructible_v, + "StartOffset and EndOffset must not be interchangeable at compile time"); +static_assert(!std::is_convertible_v, + "the anchor constructor must stay explicit"); + struct NoteProgram { Division length{}; StartOffset start{}; @@ -89,7 +100,10 @@ struct ResolvedNote { double noteOffSeconds = 0.0; // == the note's sounding length, note-on being 0 double captureStartSeconds = 0.0; // negative when the capture opens before the note double captureEndSeconds = 0.0; - std::uint8_t velocity = 1; + std::uint8_t velocity = 100; // resolveNote always overwrites this; matches Velocity's own default + // True when the programmed end offset inverted the window and resolveNote collapsed it + // to zero length instead — lets a popup explain an empty window rather than just show one. + bool windowCollapsed = false; double captureLengthSeconds() const { return captureEndSeconds - captureStartSeconds; } }; diff --git a/src/core/instrument/note/tempo.cpp b/src/core/instrument/note/tempo.cpp index 7f75d7d..bb7e5e4 100644 --- a/src/core/instrument/note/tempo.cpp +++ b/src/core/instrument/note/tempo.cpp @@ -11,6 +11,10 @@ constexpr double kSecondsPerMinute = 60.0; std::optional Tempo::fromBpm(double beatsPerMinute) { if (!std::isfinite(beatsPerMinute) || beatsPerMinute <= 0.0) return std::nullopt; + // A subnormal BPM is finite and positive but overflows 60/bpm to +inf, which then turns + // any beatsToSeconds(0) into NaN downstream — reject it here so every conversion below + // stays total. + if (!std::isfinite(kSecondsPerMinute / beatsPerMinute)) return std::nullopt; return Tempo(beatsPerMinute); } diff --git a/src/core/instrument/note/tempo.h b/src/core/instrument/note/tempo.h index 706a18e..7c2f8a6 100644 --- a/src/core/instrument/note/tempo.h +++ b/src/core/instrument/note/tempo.h @@ -7,6 +7,7 @@ #pragma once #include +#include namespace reasampler::instrument::note { @@ -34,4 +35,7 @@ private: double bpm_; }; +static_assert(!std::is_default_constructible_v, + "Tempo must not be constructible without a validated BPM"); + } // namespace reasampler::instrument::note diff --git a/tests/test_musical_division.cpp b/tests/test_musical_division.cpp index 449bb53..f9cc6b9 100644 --- a/tests/test_musical_division.cpp +++ b/tests/test_musical_division.cpp @@ -111,16 +111,19 @@ static void testNamedExamples() { // --- Picker order ------------------------------------------------------------- static void testPickerOrderIsShortestFirst() { - // Straight lengths ascend across rungs; within a rung the order is straight, dotted, - // triplet (so the index is not itself sorted by duration — only the rungs are). + // Straight lengths ascend across rungs; within EVERY rung (not just rung 0) the order is + // straight, dotted, triplet — so the index is not itself sorted by duration. for (int rung = 1; rung < kRungCount; ++rung) { const double prev = divisionBeats(divisionAt((rung - 1) * kModifierCount)); const double here = divisionBeats(divisionAt(rung * kModifierCount)); CHECK(here > prev); } - CHECK(divisionAt(0) == makeDivision(kMinQuarterExponent, DivisionModifier::Straight)); - CHECK(divisionAt(1) == makeDivision(kMinQuarterExponent, DivisionModifier::Dotted)); - CHECK(divisionAt(2) == makeDivision(kMinQuarterExponent, DivisionModifier::Triplet)); + for (int rung = 0; rung < kRungCount; ++rung) { + const int e = kMinQuarterExponent + rung; + CHECK(divisionAt(rung * kModifierCount + 0) == makeDivision(e, DivisionModifier::Straight)); + CHECK(divisionAt(rung * kModifierCount + 1) == makeDivision(e, DivisionModifier::Dotted)); + CHECK(divisionAt(rung * kModifierCount + 2) == makeDivision(e, DivisionModifier::Triplet)); + } } static void testIndexRoundTripsOverTheWholeSet() { @@ -151,6 +154,16 @@ static void testOffLadderExponentClampsToTheNearestRung() { CHECK(almostEqual(divisionBeats(corrupt), 256.0)); } +static void testEqualityNormalizesOffLadderExponentsLikeEveryOtherReader() { + // divisionBeats/divisionIndex/divisionLabel all re-clamp through makeDivision; equality + // must too, or a corrupt persisted value reads as a spurious diff on every reload. + Division corrupt; + corrupt.quarterExponent = 120; + corrupt.modifier = DivisionModifier::Straight; + CHECK(corrupt == makeDivision(kMaxQuarterExponent, DivisionModifier::Straight)); + CHECK(corrupt != makeDivision(kMaxQuarterExponent, DivisionModifier::Dotted)); +} + static void testOutOfRangeIndexClampsIntoTheSet() { CHECK(divisionAt(-1) == divisionAt(0)); CHECK(divisionAt(kDivisionCount) == divisionAt(kDivisionCount - 1)); @@ -169,6 +182,7 @@ int main() { testEverySetMemberIsDistinct(); testOffLadderExponentClampsToTheNearestRung(); + testEqualityNormalizesOffLadderExponentsLikeEveryOtherReader(); testOutOfRangeIndexClampsIntoTheSet(); if (g_fail == 0) std::printf("musical_division: all tests passed\n"); diff --git a/tests/test_note_program.cpp b/tests/test_note_program.cpp index 03a5afa..92ca6c8 100644 --- a/tests/test_note_program.cpp +++ b/tests/test_note_program.cpp @@ -1,10 +1,11 @@ // Standalone tests for reasampler::instrument::note::note_program — no VST3, no REAPER, no // framework. Same fast assert loop as the sibling pure tests. // -// Covers: velocity clamping; the ms/beats denomination seam and its round-trip; anchoring -// (start to note-on, end to note-off); the resolved window against hand-computed values; -// every division resolving to its duration in seconds; proportionality across two tempos; -// record equality and copy round-trip. +// Covers: velocity clamping; the ms/beats denomination seam and its round-trip, including a +// corrupt denomination byte; anchoring (start to note-on, end to note-off); the resolved +// window against hand-computed values and its windowCollapsed flag; every division resolving +// to its duration in seconds; proportionality across two tempos; record equality and copy +// round-trip; editing an offset via its non-stored view (withMsView/withBeatsView). #include "../src/core/instrument/note/note_program.h" @@ -323,7 +324,67 @@ static void testDefaultRecordIsAQuarterNoteWithNoOffsets() { CHECK(almostEqual(r.noteOffSeconds, 0.5)); CHECK(almostEqual(r.captureStartSeconds, 0.0)); CHECK(almostEqual(r.captureEndSeconds, 0.5)); - CHECK(r.velocity >= Velocity::kMin && r.velocity <= Velocity::kMax); + CHECK(r.velocity == 100); // NoteProgram{}'s default Velocity, documented in note_program.h +} + +// --- Corrupt denomination byte -------------------------------------------------- + +static void testOutOfRangeDenominationReadsAsMillisecondsEverywhere() { + // A denomination byte outside {Milliseconds, Beats} is well-defined but unnamed; all + // three readers must default it to the same interpretation or a popup and a bake can + // report different instants for one record. + OffsetAmount corrupt; + corrupt.magnitude = 250.0; + corrupt.denomination = static_cast(7); + const Tempo t = at(120.0); + CHECK(almostEqual(offsetMs(corrupt, t), 250.0)); + CHECK(almostEqual(offsetSeconds(corrupt, t), 0.25)); + CHECK(almostEqual(offsetBeats(corrupt, t), t.msToBeats(250.0))); +} + +// --- windowCollapsed ------------------------------------------------------------- + +static void testWindowCollapsedFlagsAnInvertedWindow() { + const Tempo t = at(120.0); + const ResolvedNote inverted = resolveNote( + program(makeDivision(0, DivisionModifier::Straight), offsetFromMs(0.0), + offsetFromMs(-5000.0), 100), + t); + CHECK(inverted.windowCollapsed); + + const ResolvedNote normal = resolveNote( + program(makeDivision(0, DivisionModifier::Straight), offsetFromMs(-20.0), + offsetFromMs(500.0), 100), + t); + CHECK(!normal.windowCollapsed); +} + +// --- Editing via the non-stored view --------------------------------------------- + +static void testWithMsViewPreservesTheStoredDenomination() { + const Tempo t = at(120.0); // one beat is 500 ms + const OffsetAmount msOffset = offsetFromMs(10.0); + const OffsetAmount editedMs = withMsView(msOffset, 40.0, t); + CHECK(editedMs.denomination == Denomination::Milliseconds); + CHECK(almostEqual(editedMs.magnitude, 40.0)); + + const OffsetAmount beatsOffset = offsetFromBeats(1.0); + const OffsetAmount editedBeats = withMsView(beatsOffset, 250.0, t); + CHECK(editedBeats.denomination == Denomination::Beats); // stays beats-denominated + CHECK(almostEqual(offsetMs(editedBeats, t), 250.0, 1e-6)); // but reads back as 250 ms +} + +static void testWithBeatsViewPreservesTheStoredDenomination() { + const Tempo t = at(120.0); // one beat is 500 ms + const OffsetAmount beatsOffset = offsetFromBeats(0.5); + const OffsetAmount editedBeats = withBeatsView(beatsOffset, 2.0, t); + CHECK(editedBeats.denomination == Denomination::Beats); + CHECK(almostEqual(editedBeats.magnitude, 2.0)); + + const OffsetAmount msOffset = offsetFromMs(100.0); + const OffsetAmount editedMs = withBeatsView(msOffset, 1.0, t); + CHECK(editedMs.denomination == Denomination::Milliseconds); // stays ms-denominated + CHECK(almostEqual(offsetBeats(editedMs, t), 1.0)); // but reads back as 1 beat } int main() { @@ -353,6 +414,13 @@ int main() { testRedenominatedRecordDescribesTheSameWindow(); testDefaultRecordIsAQuarterNoteWithNoOffsets(); + testOutOfRangeDenominationReadsAsMillisecondsEverywhere(); + + testWindowCollapsedFlagsAnInvertedWindow(); + + testWithMsViewPreservesTheStoredDenomination(); + testWithBeatsViewPreservesTheStoredDenomination(); + if (g_fail == 0) std::printf("note_program: all tests passed\n"); else std::printf("note_program: %d FAILED\n", g_fail); return g_fail == 0 ? 0 : 1; diff --git a/tests/test_tempo.cpp b/tests/test_tempo.cpp index dcfa06c..28c2a0a 100644 --- a/tests/test_tempo.cpp +++ b/tests/test_tempo.cpp @@ -41,6 +41,9 @@ static void testUnusableBpmIsRejected() { CHECK(!Tempo::fromBpm(-120.0).has_value()); CHECK(!Tempo::fromBpm(std::numeric_limits::quiet_NaN()).has_value()); CHECK(!Tempo::fromBpm(std::numeric_limits::infinity()).has_value()); + // Finite, positive, subnormal — but 60/bpm overflows to +inf, which turns + // beatsToSeconds(0) into NaN downstream if let through. + CHECK(!Tempo::fromBpm(1e-310).has_value()); } // --- Conversions ---------------------------------------------------------------