fix: guard filter tail NaNs, skip static-filter envelope work, pin stereo filter path, correct stale comments

Codec fallback for non-finite filter fields, a modAmount==0 early-out in tickFilterCutoff,
new stereo render tests for the dual-mono mirror, and comment/test accuracy fixes flagged
in review (stale deck-width claims, restated invariants, drifted CMake link comments).
This commit is contained in:
2026-07-30 17:35:45 -04:00
parent 67215509cb
commit 39389c1183
14 changed files with 137 additions and 41 deletions
+6 -5
View File
@@ -96,10 +96,8 @@ struct PitchEnvParams {
// normalized control positions verbatim rather than a parallel set, so no control range is // normalized control positions verbatim rather than a parallel set, so no control range is
// re-derived here; `filter_params.h` owns every law that maps them to Hz/Q/depth. // re-derived here; `filter_params.h` owns every law that maps them to Hz/Q/depth.
// //
// The three modulation depths all land in that same normalized cutoff domain and sum before a // The three modulation depths below land in that same normalized cutoff domain and sum
// single clamp: `modAmount` scales the per-frame filter envelope, `velAmount` scales the // before a single clamp; all three are zero/neutral by default.
// note-on velocity through `velocityCurve`, and `keyTrack` moves cutoff by octaves per octave
// above the root. All three are zero/neutral by default.
struct FilterParams { struct FilterParams {
bool enabled = false; bool enabled = false;
instrument::engine::filter::FilterSettings settings; instrument::engine::filter::FilterSettings settings;
@@ -109,7 +107,10 @@ struct FilterParams {
AdsrParams env; // the same staged AHDSR the amp runs; frames AdsrParams env; // the same staged AHDSR the amp runs; frames
// Shapes velocity before velAmount scales it. Linear rather than the amp's flat() default // Shapes velocity before velAmount scales it. Linear rather than the amp's flat() default
// because a flat curve under a depth control would make every velocity the same offset; // because a flat curve under a depth control would make every velocity the same offset;
// the no-op at rest is velAmount == 0, not the curve. // the no-op at rest is velAmount == 0, not the curve. NOTE: this default only governs a
// FRESH FilterParams — the shared codec's corrupt/truncated-point-list repair
// (VelocityCurve::fromPoints, used for both this curve and the amp's) still degrades to
// flat() regardless, since that repair has no curve-specific fallback.
VelocityCurve velocityCurve = VelocityCurve::linear(); VelocityCurve velocityCurve = VelocityCurve::linear();
}; };
+2 -2
View File
@@ -92,8 +92,8 @@ void Voice::start(int note, int velocity, const SampleData& sample, bool declick
pitchEnv_.configure(p.pitchEnv); pitchEnv_.configure(p.pitchEnv);
pitchEnv_.noteOn(); pitchEnv_.noteOn();
// Filter: fresh integrators per note (prepare() preserves state on purpose, so a note-on // Filter: reset() clears integrator state for the new note (prepare() preserves it —
// is the one place that must clear it). Velocity maps through the curve once here, off the // voice_filter.h / filter/CLAUDE.md). Velocity maps through the curve once here, off the
// per-frame path, exactly as the amp's velocityGain_ does. // per-frame path, exactly as the amp's velocityGain_ does.
filterOn_ = p.filter.enabled; filterOn_ = p.filter.enabled;
if (filterOn_) { if (filterOn_) {
+12 -4
View File
@@ -54,8 +54,9 @@ inline double filterNormPerOctave() {
// A modulated cutoff re-solves the SVF coefficients, which costs a tan() plus the morph's // A modulated cutoff re-solves the SVF coefficients, which costs a tan() plus the morph's
// cos/sin — so the solve is gated on the modulated position crossing one step of this // cos/sin — so the solve is gated on the modulated position crossing one step of this
// quantization of the sweep. 2048 steps over three decades is ~0.06 semitone, far under the // quantization of the sweep. 2048 steps over three decades is ~0.06 semitone, far under the
// ear's resolution for a filter corner, and it collapses the solve to nothing across a static // ear's resolution for a filter corner. tickFilterCutoff's own modAmount==0 early-out (see
// envelope stage: an unmodulated voice pays one integer compare per frame. // there) skips the envelope/clamp/quantize math too, so a static, already-solved voice pays
// two cheap compares per frame there, never the tan/cos/sin.
inline constexpr int kFilterModSteps = 2048; inline constexpr int kFilterModSteps = 2048;
// Takeover declick: a restart of a sounding voice (mono retrigger takeover/fallback or a // Takeover declick: a restart of a sounding voice (mono retrigger takeover/fallback or a
@@ -178,9 +179,16 @@ private:
} }
// Advances the filter envelope and re-solves the filter's coefficients when the modulated // Advances the filter envelope and re-solves the filter's coefficients when the modulated
// cutoff has moved a whole quantization step (see kFilterModSteps). prepare() deliberately // cutoff has moved a whole quantization step (see kFilterModSteps); state preservation
// preserves integrator state, so a moving cutoff glides rather than clicking. // across that solve is voice_filter's own contract (voice_filter.h / filter/CLAUDE.md).
//
// filterModAmount_ is fixed for the note's lifetime (set once in start()), so once the
// first solve has run (filterModStep_ != -1) a zero depth can only ever re-derive the same
// cutoff — skip the envelope tick, clamp, and quantization entirely rather than pay them
// to land on the answer already solved. filterModStep_ == -1 (forced by start()/retune()
// via updateFilterCutoffBase) still falls through here so the base cutoff's own solve runs.
void tickFilterCutoff() { void tickFilterCutoff() {
if (filterModAmount_ == 0.0 && filterModStep_ != -1) return;
double cut = static_cast<double>(filterBaseCutoff_) + double cut = static_cast<double>(filterBaseCutoff_) +
filterModAmount_ * filterEnv_.tick(); filterModAmount_ * filterEnv_.tick();
if (cut < 0.0) cut = 0.0; if (cut < 0.0) cut = 0.0;
+2
View File
@@ -15,6 +15,8 @@ reasampler_test(bank_sync LINK bank_sync)
# The state codec is shared with the extension's preset-blob path, so it must link WITHOUT # The state codec is shared with the extension's preset-blob path, so it must link WITHOUT
# the voice engine: velocity_curve (the curve field) and master_gain (the wire gain cap) only. # the voice engine: velocity_curve (the curve field) and master_gain (the wire gain cap) only.
# play_params.h also pulls in filter/'s headers (FilterSettings, MorphLaw) for the v9 filter
# tail -- plain value types, so no filter symbol is linked and this stays true.
reasampler_pure_library(component_state_io reasampler_pure_library(component_state_io
SOURCES component_state_io.cpp SOURCES component_state_io.cpp
LINK PUBLIC velocity_curve master_gain) LINK PUBLIC velocity_curve master_gain)
@@ -162,9 +162,14 @@ void readFilterTail(ByteReader& r, InstrumentParams& p) {
f.settings.driveNorm = static_cast<float>(bitsToDouble(r.u64())); f.settings.driveNorm = static_cast<float>(bitsToDouble(r.u64()));
f.settings.morphLaw = (r.u8() != 0) ? engine::filter::MorphLaw::HighNotchLow f.settings.morphLaw = (r.u8() != 0) ? engine::filter::MorphLaw::HighNotchLow
: engine::filter::MorphLaw::HighBandLow; : engine::filter::MorphLaw::HighBandLow;
f.modAmount = bitsToDouble(r.u64()); // Same non-finite-falls-back-to-neutral guard as the v8 master gain above: these three
f.velAmount = bitsToDouble(r.u64()); // reach Voice::tickFilterCutoff's clamp compares and a static_cast<int>, both UB on NaN.
f.keyTrack = bitsToDouble(r.u64()); double modAmount = bitsToDouble(r.u64());
double velAmount = bitsToDouble(r.u64());
double keyTrack = bitsToDouble(r.u64());
f.modAmount = std::isfinite(modAmount) ? modAmount : 0.0;
f.velAmount = std::isfinite(velAmount) ? velAmount : 0.0;
f.keyTrack = std::isfinite(keyTrack) ? keyTrack : 0.0;
f.env.attackSeconds = bitsToDouble(r.u64()); f.env.attackSeconds = bitsToDouble(r.u64());
f.env.holdSeconds = bitsToDouble(r.u64()); f.env.holdSeconds = bitsToDouble(r.u64());
f.env.decaySeconds = bitsToDouble(r.u64()); f.env.decaySeconds = bitsToDouble(r.u64());
+5 -2
View File
@@ -44,8 +44,11 @@ reasampler_test(envelope_edit LINK envelope_edit)
reasampler_pure_library(knob_deck SOURCES knob_deck.cpp LINK PUBLIC editor_geometry) reasampler_pure_library(knob_deck SOURCES knob_deck.cpp LINK PUBLIC editor_geometry)
reasampler_test(knob_deck LINK knob_deck) reasampler_test(knob_deck LINK knob_deck)
# The deck's group COMPOSITION, split from its layout: knob_deck stays engine-free, while this # The deck's group COMPOSITION, split from its layout: knob_deck stays engine-free (see
# names the controls and so reads PlayMode (velocity_curve comes along with play_params.h). # core/instrument/CLAUDE.md's deck_groups entry for why this module, not knob_deck, reads
# PlayMode). velocity_curve is the filter's own curve field; peaks is play_params.h's
# AudioSample dependency. play_params.h also drags in filter/'s headers (FilterSettings,
# MorphLaw) for the v9 filter tail -- plain value types, no filter symbol linked.
reasampler_pure_library(deck_groups reasampler_pure_library(deck_groups
SOURCES deck_groups.cpp SOURCES deck_groups.cpp
LINK PUBLIC knob_deck velocity_curve peaks) LINK PUBLIC knob_deck velocity_curve peaks)
+1 -1
View File
@@ -72,7 +72,7 @@ std::vector<DeckGroupDesc> sampleDeckGroups(PlayMode playMode) {
id(DeckParam::kRelease)}; id(DeckParam::kRelease)};
} else { } else {
// Trigger, time-ordered left-to-right (Fade In / Length % / Fade Out — matches // Trigger, time-ordered left-to-right (Fade In / Length % / Fade Out — matches
// the drawn envelope), plus the two reserved blanks that hold the Gate width. // the drawn envelope), plus two blanks (see knob_deck.h's blank-cell contract).
amp.cellIds = {id(DeckParam::kTrigFadeIn), id(DeckParam::kTrigLength), amp.cellIds = {id(DeckParam::kTrigFadeIn), id(DeckParam::kTrigLength),
id(DeckParam::kTrigFadeOut), -1, -1}; id(DeckParam::kTrigFadeOut), -1, -1};
} }
+2 -3
View File
@@ -68,9 +68,8 @@ enum DeckGroupId {
}; };
// The deck's groups, left to right, in SIGNAL-FLOW order: pitch -> filter -> amp, then the // The deck's groups, left to right, in SIGNAL-FLOW order: pitch -> filter -> amp, then the
// two instance-wide groups. `playMode` picks the AMP group's face; its width is // two instance-wide groups. `playMode` picks the AMP group's face, via knob_deck's blank-cell
// mode-independent (Trigger leaves two blank cells) so a mode flip never reflows the // reservation (knob_deck.h) so a mode flip never reflows the neighbouring groups.
// neighbouring groups.
std::vector<DeckGroupDesc> sampleDeckGroups(PlayMode playMode); std::vector<DeckGroupDesc> sampleDeckGroups(PlayMode playMode);
// The deck's BIPOLAR knob law: 0.5 of the knob's travel is zero depth, the ends are -1 and // The deck's BIPOLAR knob law: 0.5 of the knob's travel is zero depth, the ends are -1 and
+3 -3
View File
@@ -1,8 +1,8 @@
// editor_controls.cpp — the ReaSamplerEditor's parameter plumbing: the band-stack layout // editor_controls.cpp — the ReaSamplerEditor's parameter plumbing: the band-stack layout
// resolve every paint/hit-test path shares, the control-value domain maps (controlValue / // resolve every paint/hit-test path shares, the control-value domain maps (controlValue /
// applyControl — seconds/fraction/frames <-> normalized 0..1), the knob-deck group // applyControl — seconds/fraction/frames <-> normalized 0..1), the control-id<->value binding
// descriptors + control-id<->value binding, and the envelope pack/unpack (the trigger-seam // against the pure `deck_groups` module's descriptors, and the envelope pack/unpack (the
// converter). Value logic only — no painting, no window plumbing. // trigger-seam converter). Value logic only — no painting, no window plumbing.
#include "shell/instrument/reasampler_editor.h" #include "shell/instrument/reasampler_editor.h"
+3 -2
View File
@@ -38,8 +38,9 @@ using util::readFileBytes;
ReaSamplerEditor::ReaSamplerEditor(ReaSamplerProcessor* processor) ReaSamplerEditor::ReaSamplerEditor(ReaSamplerProcessor* processor)
: CPluginView(nullptr), processor_(processor) { : CPluginView(nullptr), processor_(processor) {
// Default view size, tuned to the three band heights: chrome + two-lane waveform + // Default view size, tuned to the band heights: chrome + two-lane waveform + the deck
// deck row. 840x620 clears the full face without scroll on 1080p. // (now two rows since the filter group). 840x620 clears the full face without scroll on
// 1080p.
ViewRect r(0, 0, 840, 620); ViewRect r(0, 0, 840, 620);
setRect(r); setRect(r);
} }
+26
View File
@@ -11,6 +11,7 @@
#include <cstdio> #include <cstdio>
#include <cstring> #include <cstring>
#include <limits>
#include <string> #include <string>
#include <vector> #include <vector>
@@ -608,6 +609,30 @@ static void testFilterTailRoundTripsLosslessly() {
reasampler::instrument::engine::VelocityCurve::flat())); reasampler::instrument::engine::VelocityCurve::flat()));
} }
// A non-finite modAmount/velAmount/keyTrack (a corrupt blob, or any writer that skipped the
// same guard the v8 master gain already applies) must lift to the neutral default rather than
// reach Voice::tickFilterCutoff, where both clamp compares are false against NaN and the
// static_cast<int> is UB. Mirrors testCorruptFieldsFallBackToDefaults' per-field precedent.
static void testNonFiniteFilterFieldsLiftToTheNeutralDefault() {
ComponentState in;
in.selectionId = "pad";
FilterSeconds& f = in.params.play.filter;
f.enabled = true;
f.modAmount = std::numeric_limits<double>::quiet_NaN();
f.velAmount = std::numeric_limits<double>::infinity();
f.keyTrack = -std::numeric_limits<double>::infinity();
const ComponentState out =
deserializeComponentState(serializeComponentState(in), 48000.0);
const FilterSeconds& g = out.params.play.filter;
const FilterSeconds def;
CHECK(g.modAmount == def.modAmount);
CHECK(g.velAmount == def.velAmount);
CHECK(g.keyTrack == def.keyTrack);
// The fallback is per-field, not per-record: the untouched fields still round-trip.
CHECK(g.enabled);
}
// The WRITER emits the CURRENT payload version, and the marker + version sit at the head of // The WRITER emits the CURRENT payload version, and the marker + version sit at the head of
// the payload — the self-describing property every legacy branch depends on. Asserted // the payload — the self-describing property every legacy branch depends on. Asserted
// against the semantic constants, not literals. // against the semantic constants, not literals.
@@ -1080,6 +1105,7 @@ int main() {
testTruncationDegradesCleanly(); testTruncationDegradesCleanly();
testV8RecordLiftsToTheOffNeutralFilter(); testV8RecordLiftsToTheOffNeutralFilter();
testFilterTailRoundTripsLosslessly(); testFilterTailRoundTripsLosslessly();
testNonFiniteFilterFieldsLiftToTheNeutralDefault();
if (failures == 0) { if (failures == 0) {
std::printf("component_state_io_tests: all tests passed\n"); std::printf("component_state_io_tests: all tests passed\n");
return 0; return 0;
+2 -1
View File
@@ -96,7 +96,8 @@ static void testWrappedDeckHeightAtThePinnedEditorWidths() {
// the remaining four fit the second. // the remaining four fit the second.
CHECK(deckRowCount(g, kAvailAtDefaultWidth) == 2); CHECK(deckRowCount(g, kAvailAtDefaultWidth) == 2);
CHECK(deckHeight(g, kAvailAtDefaultWidth) == 2 * kDeckGroupH + kDeckRowGap); CHECK(deckHeight(g, kAvailAtDefaultWidth) == 2 * kDeckGroupH + kDeckRowGap);
// At the 560 floor it takes four; FILTER is wider than the row on its own. // At the 560 floor it takes four: FILTER (440px) fits the row alone, but not alongside
// PITCH + PITCH ENV (318 + 12 + 440 = 770 > 544), so it wraps to its own row.
CHECK(deckRowCount(g, kAvailAtMinWidth) == 4); CHECK(deckRowCount(g, kAvailAtMinWidth) == 4);
CHECK(deckHeight(g, kAvailAtMinWidth) == 4 * kDeckGroupH + 3 * kDeckRowGap); CHECK(deckHeight(g, kAvailAtMinWidth) == 4 * kDeckGroupH + 3 * kDeckRowGap);
-14
View File
@@ -51,19 +51,6 @@ static void testGroupWidth() {
CHECK(deckGroupWidth(master) == kDeckCellW + 2 * kDeckGroupPadX); CHECK(deckGroupWidth(master) == kDeckCellW + 2 * kDeckGroupPadX);
} }
static void testShellDeckFitsOneRowAtDefaultWidth() {
// The r11 default window is 840 with kPad=8 margins -> 824 available. The five shell
// groups must fit ONE deck row there (the elastic hero keeps ~430px — the layout spec's
// premise). Locks the constants against accidental growth.
const auto deck = shellLikeDeck();
int total = 0;
for (const auto& g : deck) total += deckGroupWidth(g);
total += (static_cast<int>(deck.size()) - 1) * kDeckGroupGap;
CHECK(total <= 824);
CHECK(deckRowCount(deck, 824) == 1);
CHECK(deckHeight(deck, 824) == kDeckGroupH);
}
static void testWrapAtNarrowWidthIsDeterministic() { static void testWrapAtNarrowWidthIsDeterministic() {
// At the 560x460 checkSizeConstraint floor (544 available) the deck wraps to TWO rows, // At the 560x460 checkSizeConstraint floor (544 available) the deck wraps to TWO rows,
// whole trailing groups only. // whole trailing groups only.
@@ -185,7 +172,6 @@ static void testEmptyDeck() {
int main() { int main() {
testGroupWidth(); testGroupWidth();
testShellDeckFitsOneRowAtDefaultWidth();
testWrapAtNarrowWidthIsDeterministic(); testWrapAtNarrowWidthIsDeterministic();
testFirstGroupAlwaysPlaces(); testFirstGroupAlwaysPlaces();
testGroupInnerGeometry(); testGroupInnerGeometry();
+65 -1
View File
@@ -9,6 +9,7 @@
#include <cmath> #include <cmath>
#include <cstdio> #include <cstdio>
#include <type_traits> #include <type_traits>
#include <utility>
#include <vector> #include <vector>
using namespace reasampler; using namespace reasampler;
@@ -67,6 +68,22 @@ static std::vector<double> render(SampleData& sample, int note, int velocity, in
return out; return out;
} }
// Renders via renderFrameStereo — exercises voice.h's stereo filter block (both the
// filter_.process(1, ...) call for a genuinely stereo source and the dual-mono mirror for a
// mono one), never touched by the mono `render` helper above.
static std::vector<std::pair<double, double>> renderStereo(SampleData& sample, int note,
int velocity, int frames) {
Voice v;
v.start(note, velocity, sample);
std::vector<std::pair<double, double>> out(static_cast<std::size_t>(frames));
for (int i = 0; i < frames; ++i) {
AudioSample l = 0.0f, r = 0.0f;
v.renderFrameStereo(l, r);
out[static_cast<std::size_t>(i)] = {static_cast<double>(l), static_cast<double>(r)};
}
return out;
}
static double rms(const std::vector<double>& x, std::size_t from, std::size_t to) { static double rms(const std::vector<double>& x, std::size_t from, std::size_t to) {
double acc = 0.0; double acc = 0.0;
for (std::size_t i = from; i < to; ++i) acc += x[i] * x[i]; for (std::size_t i = from; i < to; ++i) acc += x[i] * x[i];
@@ -218,7 +235,8 @@ static void testModAmountPolarityDrivesCutoffFromOppositeEnds() {
const std::vector<double> falling = sweep(1.0f, -1.0); const std::vector<double> falling = sweep(1.0f, -1.0);
CHECK(rms(falling, 10000, 12000) < 0.05 * rms(falling, 0, 2000)); CHECK(rms(falling, 10000, 12000) < 0.05 * rms(falling, 0, 2000));
// Zero depth: the envelope is still running, but it must not reach cutoff at all. // Zero depth: tickFilterCutoff's early-out skips the envelope entirely, so cutoff must
// never move regardless of how long the (unread) envelope stage runs.
const std::vector<double> steady = sweep(0.5f, 0.0); const std::vector<double> steady = sweep(0.5f, 0.0);
const double early = rms(steady, 2000, 4000); const double early = rms(steady, 2000, 4000);
const double late = rms(steady, 10000, 12000); const double late = rms(steady, 10000, 12000);
@@ -282,6 +300,50 @@ static void testNoteOnResetsTheFilterSoAPreviousNoteCannotLeak() {
for (std::size_t i = 0; i < fresh.size(); ++i) CHECK(restarted[i] == fresh[i]); for (std::size_t i = 0; i < fresh.size(); ++i) CHECK(restarted[i] == fresh[i]);
} }
// ---------------------------------------------------------------------------
// Stereo: the two filter paths renderFrame() (mono) never reaches.
// ---------------------------------------------------------------------------
static void testStereoRenderOfAMonoSampleMirrorsTheMonoResultExactly() {
// A mono SampleData in a stereo render never calls filter_.process(1, ...) — voice.h's
// dual-mono block copies channel 0's already-filtered result to channel 1 instead. Pin
// both channels bit-identical to the plain mono render.
SampleData s = periodicSine(4000, 64);
s.play.adsr = flatAdsr();
s.play.filter = engagedFilter(0.3f, 0.85f, 1.0f);
s.play.filter.settings.driveNorm = 0.6f;
SampleData monoSrc = s;
const std::vector<double> mono = render(monoSrc, 60, 100, 2000);
SampleData stereoSrc = s;
const auto stereo = renderStereo(stereoSrc, 60, 100, 2000);
for (std::size_t i = 0; i < stereo.size(); ++i) {
CHECK(stereo[i].first == mono[i]);
CHECK(stereo[i].second == mono[i]);
}
}
static void testStereoFilterChannel1MatchesChannel0ForIdenticalLRInput() {
// A genuinely stereo sample whose channels are identical drives filter_.process(1, ...)
// for real (haveR true). Identical input plus identical reset() state must give identical
// output on both channels — the mirror's correctness argument, pinned against the actual
// per-channel path instead of only asserted in a comment.
SampleData s = periodicSine(4000, 64);
s.framesR = s.frames; // genuinely stereo, L == R
s.play.adsr = flatAdsr();
s.play.filter = engagedFilter(0.3f, 0.85f, 1.0f);
s.play.filter.settings.driveNorm = 0.6f;
const auto stereo = renderStereo(s, 60, 100, 2000);
bool sawSignal = false;
for (const auto& lr : stereo) {
CHECK(lr.first == lr.second);
if (std::fabs(lr.first) > 1e-6) sawSignal = true;
}
CHECK(sawSignal);
}
int main() { int main() {
testDisengagedFilterIsBitInertEvenWithExtremeSettingsStored(); testDisengagedFilterIsBitInertEvenWithExtremeSettingsStored();
testFilterSeesThePreAmpSignalSoAmpGainScalesTheResultLinearly(); testFilterSeesThePreAmpSignalSoAmpGainScalesTheResultLinearly();
@@ -289,6 +351,8 @@ int main() {
testModAmountPolarityDrivesCutoffFromOppositeEnds(); testModAmountPolarityDrivesCutoffFromOppositeEnds();
testVelocityAndKeyTrackingReachCutoffAndAreNoOpsAtTheirDefaults(); testVelocityAndKeyTrackingReachCutoffAndAreNoOpsAtTheirDefaults();
testNoteOnResetsTheFilterSoAPreviousNoteCannotLeak(); testNoteOnResetsTheFilterSoAPreviousNoteCannotLeak();
testStereoRenderOfAMonoSampleMirrorsTheMonoResultExactly();
testStereoFilterChannel1MatchesChannel0ForIdenticalLRInput();
if (g_fail == 0) std::printf("sampler_filter: all tests passed\n"); if (g_fail == 0) std::printf("sampler_filter: all tests passed\n");
return g_fail == 0 ? 0 : 1; return g_fail == 0 ? 0 : 1;
} }