Fix filter DSP click-train flush and finish namespace nesting + test coverage

src/core/instrument/engine/filter/CLAUDE.md

@@ -3,7 +3,11 @@
 ## Scope
 
 The pure 2-pole resonant low/high-pass a sounding voice runs. No REAPER, no VST3, no
-allocation, no I/O. Four files, one responsibility each:
+allocation, no I/O. Everything here lives in `reasampler::instrument::engine::filter`,
+nested per the directory-mirrors-namespace convention — this keeps `FilterMode` and
+friends out of `reasampler::instrument::engine` proper, where `zone_params.h` lives, since
+this module has no call site yet to force the collision into the open at compile time.
+Four files, one responsibility each:
 
 - `filter_params` — the control domain: `FilterMode`, normalized [0,1] knob position →
   cutoff Hz and Q, and the exact inverses.
@@ -43,9 +47,10 @@ is 200 Hz and 2/3 is 2 kHz) and takes no sample rate. The persisted value is the
 normalized knob position, so a rate-derived endpoint would make one preset sound different
 at 44.1k and 96k. The Nyquist clamp (`kFilterNyquistFraction`, 0.48) is a property of the
 bilinear transform — `tan(pi*fc/sr)` diverges at Nyquist — so it lives in `biquadCoeffs`
-where the rate is already a parameter. 20 kHz is under 0.48·sr at every supported rate, so
+where the rate is already a parameter. 20 kHz is under 0.48·sr at 44.1k and above, so the
-the clamp never eats live knob travel; the source's hardcoded 23 kHz endpoint did exactly
+clamp never eats live knob travel there; the source's hardcoded 23 kHz endpoint did exactly
-that at 44.1k.
+that at 44.1k. Below 44.1k (e.g. 32k, 22.05k) the clamp still handles the math correctly —
+it just legitimately eats the top of the knob travel at those rates.
 
 `biquadCoeffs` with a non-positive sample rate returns pass-through coefficients. It does
 **not** fall back to 44100 — that would breach the standing no-hardcoded-sample-rates

src/core/instrument/engine/filter/filter_coeffs.cpp

@@ -2,7 +2,7 @@
 
 #include <cmath>
 
-namespace reasampler::instrument::engine {
+namespace reasampler::instrument::engine::filter {
 namespace {
 
 // M_PI is not standard C++ and is absent on MSVC without _USE_MATH_DEFINES.
@@ -40,4 +40,4 @@ BiquadCoeffs biquadCoeffs(FilterMode mode, float cutoffHz, float q, double sampl
     return c;
 }
 
-}  // namespace reasampler::instrument::engine
+}  // namespace reasampler::instrument::engine::filter

src/core/instrument/engine/filter/filter_coeffs.h

@@ -7,7 +7,7 @@
 
 #include "core/instrument/engine/filter/filter_params.h"
 
-namespace reasampler::instrument::engine {
+namespace reasampler::instrument::engine::filter {
 
 // Already normalized by a0. The denominator is 1 + a1*z^-1 + a2*z^-2, so the difference
 // equation SUBTRACTS the a terms: y = b0*x + b1*x1 + b2*x2 - a1*y1 - a2*y2.
@@ -28,4 +28,4 @@ inline constexpr double kFilterNyquistFraction = 0.48;
 // no-hardcoded-sample-rates ruling means we refuse to invent a rate rather than assume 44.1k.
 BiquadCoeffs biquadCoeffs(FilterMode mode, float cutoffHz, float q, double sampleRate);
 
-}  // namespace reasampler::instrument::engine
+}  // namespace reasampler::instrument::engine::filter

src/core/instrument/engine/filter/filter_params.cpp

@@ -2,7 +2,7 @@
 
 #include <cmath>
 
-namespace reasampler::instrument::engine {
+namespace reasampler::instrument::engine::filter {
 namespace {
 
 double clamp01(double v) { return v < 0.0 ? 0.0 : (v > 1.0 ? 1.0 : v); }
@@ -56,4 +56,4 @@ float filterNormFromQ(float q) {
     return static_cast<float>(clamp01((-k.b + std::sqrt(d)) / (2.0 * k.c)));
 }
 
-}  // namespace reasampler::instrument::engine
+}  // namespace reasampler::instrument::engine::filter

src/core/instrument/engine/filter/filter_params.h

@@ -5,7 +5,7 @@
 
 #pragma once
 
-namespace reasampler::instrument::engine {
+namespace reasampler::instrument::engine::filter {
 
 enum class FilterMode { LowPass, HighPass };
 
@@ -13,7 +13,8 @@ enum class FilterMode { LowPass, HighPass };
 // norm 2/3 is 2 kHz. NOT derived from the sample rate — a rate-dependent endpoint would make
 // one saved preset sound different at 44.1k and 96k, and at 44.1k the top of the travel would
 // be dead against the Nyquist clamp (the ported firmware's 23 kHz endpoint had exactly that
-// defect). 20 kHz sits under 0.48*sr at every rate we support, so the whole knob stays live.
+// defect). 20 kHz sits under 0.48*sr at 44.1 kHz and above; below that (e.g. 32 kHz, 22.05 kHz)
+// the clamp still handles it correctly, it just eats the top of the knob travel at those rates.
 inline constexpr float kFilterCutoffMinHz = 20.0f;
 inline constexpr float kFilterCutoffMaxHz = 20000.0f;
 
@@ -36,4 +37,4 @@ float filterQFromNorm(float norm);
 // Exact inverse of filterQFromNorm; out-of-range Q clamps to 0 or 1.
 float filterNormFromQ(float q);
 
-}  // namespace reasampler::instrument::engine
+}  // namespace reasampler::instrument::engine::filter

src/core/instrument/engine/filter/filter_saturate.h

@@ -4,7 +4,7 @@
 
 #pragma once
 
-namespace reasampler::instrument::engine {
+namespace reasampler::instrument::engine::filter {
 
 // Rational tanh approximation inside +/-threshold, continued past it with a gentle 0.1 slope
 // anchored at the threshold value so the curve stays continuous rather than hard-clipping.
@@ -24,4 +24,4 @@ inline float tanhSaturate(float x, float threshold, float a, float b) {
 // path produces, not for audio-level signals — do not reuse it as a general waveshaper.
 inline float feedbackSaturate(float x) { return tanhSaturate(x, 2.0f, 27.0f, 9.0f); }
 
-}  // namespace reasampler::instrument::engine
+}  // namespace reasampler::instrument::engine::filter

src/core/instrument/engine/filter/voice_filter.cpp

@@ -1,6 +1,6 @@
 #include "core/instrument/engine/filter/voice_filter.h"
 
-namespace reasampler::instrument::engine {
+namespace reasampler::instrument::engine::filter {
 
 void VoiceFilter::prepare(const FilterSettings& settings, double sampleRate) {
     mode_ = settings.mode;
@@ -26,4 +26,4 @@ bool VoiceFilter::isSilent() const {
     return true;
 }
 
-}  // namespace reasampler::instrument::engine
+}  // namespace reasampler::instrument::engine::filter

src/core/instrument/engine/filter/voice_filter.h

@@ -11,7 +11,7 @@
 #include "core/instrument/engine/filter/filter_params.h"
 #include "core/instrument/engine/filter/filter_saturate.h"
 
-namespace reasampler::instrument::engine {
+namespace reasampler::instrument::engine::filter {
 
 // Normalized control positions, as the editor moves them and the persisted state carries them.
 struct FilterSettings {
@@ -23,6 +23,13 @@ struct FilterSettings {
 // Share of the last output fed back into the high-pass input at full resonance. Driven by the
 // raw control position rather than by Q: Q reaches 10, and scaling the feedback by it would
 // push the loop gain past unity at the top of the range.
+//
+// The HP/LP resonance asymmetry this produces is a known ear call reserved for Daniel, not a
+// bug: measured peak/passband at res=1.0, fc=1kHz/sr=48k is LP 9.98 (flat at every input level)
+// vs HP 7.44 (input 0.001-0.1), 7.59 (0.3), 8.52 (1.0) — HP resonance is level-dependent because
+// feedbackSaturate's threshold (+/-2.0) is an absolute level, not a fraction of the signal.
+// Retuning this constant alone cannot make the two modes track, since it does not touch that
+// level-dependence.
 inline constexpr float kHighPassFeedbackShare = 0.24f;
 
 // Below this the recursion has decayed past -600 dB. Flushing keeps the history out of the
@@ -56,7 +63,8 @@ public:
 
         // The high-pass numerator collapses toward zero as cutoff falls, taking the resonance
         // with it; feeding a saturated share of the last output back into the input restores
-        // the character the coefficients alone stop producing down there.
+        // the character the coefficients alone stop producing down there. The 0.9f pre-scale is
+        // carried from the source firmware, uncalibrated here — no derivation is known for it.
         const float in = (mode_ == FilterMode::HighPass)
                              ? x - fbAmount_ * feedbackSaturate(s.fb * 0.9f)
                              : x;
@@ -69,15 +77,25 @@ public:
         s.y2 = s.y1;
         s.y1 = y;
 
-        // Snap the WHOLE state once the recursion as a whole has decayed past -600 dB.
+        // Snap the RECURSIVE half of the state once it has decayed past -600 dB. Only y1/y2
-        // Zeroing individual samples instead does not work: a resonator swings through zero
+        // are flushed (and only they are tested) — x1/x2 is an FIR tail that shifts out within
-        // twice a cycle, so a per-sample flush injects a step in phase with the resonance,
+        // two samples on its own, and a high-pass has an exact DC null (b1 == -2*b0 bit-exactly),
-        // which the resonance then amplifies — the filter limit-cycles at the floor forever
+        // so under a constant/DC-biased input y decays to zero while x1/x2 sit at the input
-        // rather than going quiet. Testing y1 AND y2 tests the envelope, not one sample.
+        // level; clearing x1/x2 too would discard that history and the next sample would
+        // recompute a full-amplitude step from b0*in alone, re-ringing forever (a click train).
+        // Zeroing individual samples instead of the pair does not work either: a resonator
+        // swings through zero twice a cycle, so a per-sample flush injects a step in phase with
+        // the resonance, which the resonance then amplifies — the filter limit-cycles at the
+        // floor forever rather than going quiet. Testing y1 AND y2 tests the envelope, not one
+        // sample.
         if (s.y1 > -kFilterDenormalFloor && s.y1 < kFilterDenormalFloor &&
             s.y2 > -kFilterDenormalFloor && s.y2 < kFilterDenormalFloor) {
-            s = State{};
+            s.y1 = 0.0f;
+            s.y2 = 0.0f;
         }
+        // Stored unconditionally even in LP mode, where nothing reads it: the mode branch above
+        // already exists, but gating this one store on it buys nothing a dead-store-eliminating
+        // compiler doesn't already do for free, at the cost of a second branch on the mode.
         s.fb = s.y1;
         return y;
     }
@@ -110,4 +128,4 @@ private:
 static_assert(!std::is_polymorphic_v<VoiceFilter>, "no vtable on the per-sample path");
 static_assert(std::is_trivially_copyable_v<VoiceFilter>, "state is plain values, never owned");
 
-}  // namespace reasampler::instrument::engine
+}  // namespace reasampler::instrument::engine::filter

tests/test_filter.cpp

@@ -13,7 +13,7 @@
 #include <cstdio>
 #include <initializer_list>
 
-using namespace reasampler::instrument::engine;
+using namespace reasampler::instrument::engine::filter;
 
 static int g_fail = 0;
 #define CHECK(cond) do { if(!(cond)) { \
@@ -41,9 +41,6 @@ static void testCutoffMapsThreeDecadesLogarithmically() {
     CHECK_NEAR(filterCutoffHzFromNorm(1.0f / 6.0f), 20.0 * std::sqrt(10.0), 1e-3);
     CHECK_NEAR(filterCutoffHzFromNorm(0.5f), 20.0 * std::sqrt(1000.0), 1e-2);
 
-    // A linear sweep would put the midpoint at 10010 Hz; the log sweep is nowhere near it.
-    CHECK(filterCutoffHzFromNorm(0.5f) < 1000.0f);
-
     CHECK_NEAR(filterCutoffHzFromNorm(-1.0f), 20.0, 1e-3);
     CHECK_NEAR(filterCutoffHzFromNorm(2.0f), 20000.0, 1e-2);
 }
@@ -351,6 +348,33 @@ static void testStateFlushesToZeroWithoutStallingInDenormals() {
     }
 }
 
+// A high-pass has an exact DC null (b1 == -2*b0 bit-exactly), so under sustained DC the
+// recursive y decays to zero while x1/x2 sit pinned at the DC level -- the case the zero-input
+// test above cannot see, since there x1/x2 are zero anyway. A flush that clears x1/x2 along
+// with y1/y2 discards that pinned history; the next sample then recomputes a full-amplitude
+// step from b0*in alone, which re-rings and repeats forever (a click train). This must fail
+// against a flush that also clears x1/x2.
+static void testHighPassSustainedDCDoesNotReRing() {
+    const double sr = 48000.0;
+    VoiceFilter f;
+    f.prepare({FilterMode::HighPass, filterNormFromCutoffHz(1000.0f), 0.0f}, sr);
+    f.reset();
+
+    const int settle = 1000;
+    float worstAfterSettle = 0.0f;
+    for (int i = 0; i < 20000; ++i) {
+        const float y = f.process(0, 1.0f);
+        if (i >= settle) {
+            const float a = std::fabs(y);
+            if (a > worstAfterSettle) worstAfterSettle = a;
+        }
+    }
+    // A correct flush leaves the settled output pinned near zero. The click train this
+    // regresses against recurs every ~4760 samples at a magnitude around 0.6 -- nowhere near
+    // this tolerance.
+    CHECK(worstAfterSettle < 1e-3f);
+}
+
 // ---------------------------------------------------------------------------
 // Impulse / step sanity and saturation
 // ---------------------------------------------------------------------------
@@ -380,11 +404,23 @@ static void testLowpassStepSettlesToUnity() {
     for (int i = 0; i < 48000; ++i) y = f.process(0, 1.0f);
     CHECK_NEAR(y, 1.0, 1e-3);  // DC passes a lowpass at unity
 
+    // A DC step through a highpass should settle to (and STAY AT) zero. Sampling only the
+    // final value is not enough to prove that: a resonator swings through zero twice a cycle,
+    // so a single late sample can land near zero while the envelope is still ringing well
+    // above it elsewhere in the same run -- track the worst case over the settled region.
     VoiceFilter hp;
     hp.prepare({FilterMode::HighPass, filterNormFromCutoffHz(1000.0f), 0.0f}, sr);
     hp.reset();
-    for (int i = 0; i < 48000; ++i) y = hp.process(0, 1.0f);
+    const int settle = 200;
-    CHECK_NEAR(y, 0.0, 1e-3);  // and is fully rejected by a highpass
+    float worstAfterSettle = 0.0f;
+    for (int i = 0; i < 48000; ++i) {
+        y = hp.process(0, 1.0f);
+        if (i >= settle) {
+            const float a = std::fabs(y);
+            if (a > worstAfterSettle) worstAfterSettle = a;
+        }
+    }
+    CHECK(worstAfterSettle < 1e-3f);  // fully rejected by a highpass, not just at one instant
 }
 
 static void testResetClearsHistoryButPrepareKeepsIt() {
@@ -415,13 +451,14 @@ static void testChannelStateIsIndependent() {
     CHECK(frame[0] != frame[1]);
 }
 
-static void testFeedbackSaturationIsContinuousAndBounded() {
+static void testFeedbackSaturationIsContinuousWithGentleLinearTail() {
     CHECK_NEAR(feedbackSaturate(0.0f), 0.0, 1e-9);
     // Odd symmetry.
     CHECK_NEAR(feedbackSaturate(1.5f), -feedbackSaturate(-1.5f), 1e-6);
     // Continuous across the threshold at +/-2.
     CHECK_NEAR(feedbackSaturate(2.0f - 1e-4f), feedbackSaturate(2.0f + 1e-4f), 1e-4);
-    // Compresses hard: a 100x input does not give a 100x output.
+    // Past the threshold the curve continues on a 0.1 slope rather than hard-clipping -- it is
+    // NOT bounded, so this pins the linear continuation's shallow slope, not a ceiling.
     CHECK(std::fabs(feedbackSaturate(100.0f)) < 12.0f);
     CHECK(feedbackSaturate(100.0f) > feedbackSaturate(50.0f));
 }
@@ -438,11 +475,12 @@ int main() {
     testMeasuredResponsePeaksAtCutoffInBothModes();
     testFullRangeCutoffSweepAtAudioRateStaysBounded();
     testStateFlushesToZeroWithoutStallingInDenormals();
+    testHighPassSustainedDCDoesNotReRing();
     testImpulseResponseMatchesDifferenceEquation();
     testLowpassStepSettlesToUnity();
     testResetClearsHistoryButPrepareKeepsIt();
     testChannelStateIsIndependent();
-    testFeedbackSaturationIsContinuousAndBounded();
+    testFeedbackSaturationIsContinuousWithGentleLinearTail();
 
     if (g_fail == 0) std::printf("filter_tests: all passed\n");
     else std::printf("filter_tests: %d FAILED\n", g_fail);