daniel/reasampler
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Close Γ-W3-T2 review remediation: widen the bake-reset test's per-parameter coverage, correct four overclaiming comments, state the stage-time ceiling's disposition, and fix a missing include

Browse Source 
Dials and asserts pitch-env/filter-env/trigEnv fields the sweep previously skipped in both fixtures; no reset behavior changed.
This commit is contained in:
Daniel Harvey
2026-08-02 07:41:27 -04:00
parent e2981e83ee
commit 1490c25250
3 changed files with 64 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/test_bake_render.cpp
+7 -4
View File
@@ -232,10 +232,12 @@ int main() {
}
// --- Regression baseline: the neutral render is the source, sample for sample --------
// DERIVED rather than recorded, so it survives a compiler change: a Trigger voice at its
// own root under Varispeed, with the default flat amp and velocity shapes and no filter,
// reads at ratio exactly 1 — so every printed frame must BE its source frame. An added
// stage, a moved default, or a lost early-out anywhere in the chain moves a sample here.
// A Trigger voice at its own root under Varispeed reads at ratio exactly 1 and hits no
// filter, so every printed frame equals its source frame PROVIDED the amp curve's gain at
// the plan's velocity is exactly 1.0 too (asserted below rather than assumed) — that exact
// value is a property of flat()'s two endpoints cancelling at velocity 100, not a
// guarantee of eval() at an arbitrary velocity. An added stage, a moved default, or a lost
// early-out anywhere in the chain moves a sample here.
{
SampleData s;
s.frames.resize(4000);
@@ -249,6 +251,7 @@ int main() {
// Shorter than the play span, so the window closes before any note-end shaping.
const BakePlan plan = planOf(/*total=*/1000, /*noteOn=*/0, /*noteOff=*/1000);
CHECK(s.velocityCurve.eval(100.0) == 1.0); // names the real cause if this ever fails
const BakeAudio audio = renderBake(s, plan, kUnity);
CHECK(audio.channelCount == 1);
CHECK(audio.frameCount() == 1000);
tests/test_bake_reset.cpp
+54 -11
View File
@@ -1,12 +1,9 @@
// Standalone tests for reasampler::instrument::bake::bake_reset — no VST3, no REAPER, no
// framework. Same fast assert loop as the sibling pure tests.
//
// Covers the ratified reset scope PER PARAMETER, in both directions: every control whose
// effect the render printed comes back at its default, and every mapping fact comes back
// untouched. Asserted field by field rather than by struct equality on purpose — a
// whole-struct compare would pass while silently resetting a survivor, or vice versa. The
// sweep runs over TWO independently dialed fixtures that share only the survivors, which is
// what makes it a property of the survivors alone rather than of one input.
// The ratified reset scope itself is bake/CLAUDE.md's; this sweep checks it field by field
// (never struct equality, which would pass while silently resetting a survivor) over TWO
// independently dialed fixtures, so a pass is a property of the survivors, not of one input.
#include "../src/core/instrument/bake/bake_reset.h"
@@ -14,6 +11,7 @@
// reset loop LOOKS like on the band is the thing worth asserting, and only that module says.
#include "../src/core/instrument/ui/loop_marks.h"
#include <cstdint>
#include <cstdio>
#include <optional>
@@ -65,6 +63,9 @@ InstrumentParams dialed() {
p.play.pitchEnv.peakSemitones = -7.0;
p.play.pitchEnv.shape.attackSeconds = 0.05;
p.play.pitchEnv.shape.decaySeconds = 0.15;
p.play.pitchEnv.shape.holdFraction = 0.6;
p.play.pitchEnv.shape.attackCurve = 2.1;
p.play.pitchEnv.shape.decayCurve = 0.4;
p.play.pitchVelocityCurve = VelocityCurve::linear();
p.play.playRate = 0.5;
p.play.pitchOffsetSemitones = -7.5;
@@ -75,7 +76,18 @@ InstrumentParams dialed() {
p.play.filter.velAmount = 0.6;
p.play.filter.keyTrack = 1.5;
p.play.filter.env.attackSeconds = 0.7;
p.play.filter.env.holdSeconds = 0.15;
p.play.filter.env.decaySeconds = 0.35;
p.play.filter.env.sustainLevel = 0.25;
p.play.filter.env.releaseSeconds = 0.55;
p.play.filter.env.attackCurve = 2.3;
p.play.filter.env.decayCurve = 0.5;
p.play.filter.env.releaseCurve = 2.8;
p.play.filter.trigEnv.attackSeconds = 0.12;
p.play.filter.trigEnv.decaySeconds = 0.8;
p.play.filter.trigEnv.holdFraction = 0.44;
p.play.filter.trigEnv.attackCurve = 1.6;
p.play.filter.trigEnv.decayCurve = 0.6;
p.play.filter.velocityCurve = VelocityCurve::linear();
p.play.ampSpline.mode = EnvMode::Spline;
p.play.ampSpline.contour = VelocityCurve::linear();
@@ -92,8 +104,8 @@ InstrumentParams dialed() {
// every other field — including play mode, which is a chosen neutral rather than a survivor.
// Both run the same neutral sweep: if the reset ever inverted into "copy the dialed set, then
// clear a blacklist", a non-survivor would come through and at most one fixture could still
// land on the defaults. A parameter added later is covered here by moving it in BOTH
// fixtures, which is the same work as adding it to the sweep.
// land on the defaults. The inherent limit this does NOT cover: nothing forces a newly added
// `InstrumentParams` field to be dialled in either fixture at all, let alone asserted.
InstrumentParams dialedOther() {
InstrumentParams p;
p.rootOverride = 43; // survivor — same as dialed()
@@ -123,6 +135,9 @@ InstrumentParams dialedOther() {
p.play.pitchEnv.peakSemitones = 11.0;
p.play.pitchEnv.shape.attackSeconds = 0.25;
p.play.pitchEnv.shape.decaySeconds = 0.35;
p.play.pitchEnv.shape.holdFraction = 0.2;
p.play.pitchEnv.shape.attackCurve = 0.5;
p.play.pitchEnv.shape.decayCurve = 2.4;
p.play.pitchVelocityCurve = VelocityCurve::rampDown();
p.play.playRate = 1.75;
p.play.pitchOffsetSemitones = 3.25;
@@ -133,7 +148,18 @@ InstrumentParams dialedOther() {
p.play.filter.velAmount = -0.35;
p.play.filter.keyTrack = -0.9;
p.play.filter.env.attackSeconds = 1.7;
p.play.filter.env.holdSeconds = 0.95;
p.play.filter.env.decaySeconds = 0.75;
p.play.filter.env.sustainLevel = 0.85;
p.play.filter.env.releaseSeconds = 0.15;
p.play.filter.env.attackCurve = 0.4;
p.play.filter.env.decayCurve = 2.9;
p.play.filter.env.releaseCurve = 0.35;
p.play.filter.trigEnv.attackSeconds = 0.62;
p.play.filter.trigEnv.decaySeconds = 1.8;
p.play.filter.trigEnv.holdFraction = 0.77;
p.play.filter.trigEnv.attackCurve = 0.3;
p.play.filter.trigEnv.decayCurve = 2.5;
p.play.filter.velocityCurve = VelocityCurve::rampDown();
p.play.ampSpline.mode = EnvMode::Spline;
p.play.ampSpline.contour = VelocityCurve::flat();
@@ -204,6 +230,9 @@ void checkNeutral(const BakeReset& reset) {
CHECK(after.play.pitchEnv.peakSemitones == 0.0);
CHECK(after.play.pitchEnv.shape.attackSeconds == freshPlay.pitchEnv.shape.attackSeconds);
CHECK(after.play.pitchEnv.shape.decaySeconds == freshPlay.pitchEnv.shape.decaySeconds);
CHECK(after.play.pitchEnv.shape.holdFraction == freshPlay.pitchEnv.shape.holdFraction);
CHECK(after.play.pitchEnv.shape.attackCurve == freshPlay.pitchEnv.shape.attackCurve);
CHECK(after.play.pitchEnv.shape.decayCurve == freshPlay.pitchEnv.shape.decayCurve);
// --- RESET: Rate and the baseline Pitch offset -----------------------------------
// Both are processing the bake already printed, so the whitelist leaves them at their
@@ -225,7 +254,18 @@ void checkNeutral(const BakeReset& reset) {
CHECK(after.play.filter.settings.driveNorm == freshPlay.filter.settings.driveNorm);
CHECK(after.play.filter.settings.morphLaw == freshPlay.filter.settings.morphLaw);
CHECK(after.play.filter.env.attackSeconds == freshPlay.filter.env.attackSeconds);
CHECK(after.play.filter.env.holdSeconds == freshPlay.filter.env.holdSeconds);
CHECK(after.play.filter.env.decaySeconds == freshPlay.filter.env.decaySeconds);
CHECK(after.play.filter.env.sustainLevel == freshPlay.filter.env.sustainLevel);
CHECK(after.play.filter.env.releaseSeconds == freshPlay.filter.env.releaseSeconds);
CHECK(after.play.filter.env.attackCurve == freshPlay.filter.env.attackCurve);
CHECK(after.play.filter.env.decayCurve == freshPlay.filter.env.decayCurve);
CHECK(after.play.filter.env.releaseCurve == freshPlay.filter.env.releaseCurve);
CHECK(after.play.filter.trigEnv.attackSeconds == freshPlay.filter.trigEnv.attackSeconds);
CHECK(after.play.filter.trigEnv.decaySeconds == freshPlay.filter.trigEnv.decaySeconds);
CHECK(after.play.filter.trigEnv.holdFraction == freshPlay.filter.trigEnv.holdFraction);
CHECK(after.play.filter.trigEnv.attackCurve == freshPlay.filter.trigEnv.attackCurve);
CHECK(after.play.filter.trigEnv.decayCurve == freshPlay.filter.trigEnv.decayCurve);
// --- RESET: the three spline contours AND their mode flags ----------------------
// The flag selects which shape ran, so the shape it selected is in the audio; with
@@ -260,7 +300,9 @@ int main() {
const InstrumentParams& after = reset.params;
const InstrumentParams fresh;
// The fixtures must really move every field the sweep asserts, or the sweep is vacuous.
// A sample of the fields checkNeutral asserts, confirming the fixtures actually moved them
// off default — not the whole sweep, but enough spot checks that a fixture regressing to
// the defaults (making the sweep vacuous) would show here first.
CHECK(fresh.keyTrack != before.keyTrack);
CHECK(before.limiterEnabled != fresh.limiterEnabled);
CHECK(before.bakeHold != fresh.bakeHold);
@@ -300,8 +342,9 @@ int main() {
// --- The loop enable and its points come back together ---------------------------
// What the band shows after a bake is the reset override read against the newly banked
// entry, which carries NO loop intrinsic (the shell records that pairing where it builds
// the entry) — so the enable has nothing left to fall back to.
// entry's loop intrinsic — assumed std::nullopt below, which is what the shell lands
// today; if it ever populated one, the enable would have something to fall back to and
// this assumption, not just this test, would need revisiting.
{
constexpr std::int64_t kFrames = 1000;
const LoopMarks dialedMarks =