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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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); }
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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 <cstdint>
|
||||
#include <type_traits>
|
||||
|
||||
#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, EndOffset>,
|
||||
"StartOffset and EndOffset must not be interchangeable at compile time");
|
||||
static_assert(!std::is_convertible_v<OffsetAmount, StartOffset>,
|
||||
"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; }
|
||||
};
|
||||
|
||||
@@ -11,6 +11,10 @@ constexpr double kSecondsPerMinute = 60.0;
|
||||
|
||||
std::optional<Tempo> 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);
|
||||
}
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@
|
||||
#pragma once
|
||||
|
||||
#include <optional>
|
||||
#include <type_traits>
|
||||
|
||||
namespace reasampler::instrument::note {
|
||||
|
||||
@@ -34,4 +35,7 @@ private:
|
||||
double bpm_;
|
||||
};
|
||||
|
||||
static_assert(!std::is_default_constructible_v<Tempo>,
|
||||
"Tempo must not be constructible without a validated BPM");
|
||||
|
||||
} // namespace reasampler::instrument::note
|
||||
|
||||
Reference in New Issue
Block a user