From 38238c66ace2812b1bc6b3c6f99e32f6cb4c3ff7 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 21:56:06 -0400 Subject: [PATCH] fix S11 review: extract upsertPickedOverride, restore map_ on drag-cancel, clamp start to frames-1, pin startFrame>loopEnd wrap test --- src/vst/reasampler_editor.cpp | 57 +++++++++++++++-------------------- src/vst/reasampler_editor.h | 6 ++++ tests/test_sampler_core.cpp | 38 +++++++++++++++++++++++ 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/src/vst/reasampler_editor.cpp b/src/vst/reasampler_editor.cpp index 18f311f..18a0f30 100644 --- a/src/vst/reasampler_editor.cpp +++ b/src/vst/reasampler_editor.cpp @@ -220,13 +220,10 @@ ReaSamplerEditor::SetupMarkers ReaSamplerEditor::pickedMarkers(std::int64_t fram return m; } -void ReaSamplerEditor::commitPickedMarkers(const SetupMarkers& m) { - // Materialize the edited markers as a per-zone loop/start override on the picked id (upsert, - // mirror of the root-marker path): a full-keyboard zone carrying the override. This plays - // identically to the un-zoned single capture (one chromatic zone) and round-trips through - // the component state; the zone becomes visible if the user opens the Zones panel. The bank - // intrinsic is NEVER written (read-only bank consumer, D-B). - if (selectedId_.empty()) return; +void ReaSamplerEditor::upsertPickedOverride(const SetupMarkers& m) { + // Find-or-append the zone for selectedId_ and write the loop/start override fields. + // The bank intrinsic is NEVER written (read-only bank consumer, D-B). selectedId_ must + // be non-empty; callers are responsible for that guard. SampleLoop loop; loop.hasLoop = m.hasLoop; loop.start = m.loopStart; @@ -249,6 +246,16 @@ void ReaSamplerEditor::commitPickedMarkers(const SetupMarkers& m) { z.startPoint = m.start; map_.zones.push_back(z); } +} + +void ReaSamplerEditor::commitPickedMarkers(const SetupMarkers& m) { + // Materialize the edited markers as a per-zone loop/start override on the picked id (upsert, + // mirror of the root-marker path): a full-keyboard zone carrying the override. This plays + // identically to the un-zoned single capture (one chromatic zone) and round-trips through + // the component state; the zone becomes visible if the user opens the Zones panel. The bank + // intrinsic is NEVER written (read-only bank consumer, D-B). + if (selectedId_.empty()) return; + upsertPickedOverride(m); commitAndReload(); } @@ -758,6 +765,7 @@ void ReaSamplerEditor::onMouseDown(int x, int y) { dragStartX_ = x; dragStartMarkers_ = m; dragSampleFrames_ = frames; + dragStartMap_ = map_; return; // no immediate set — the marker only moves once the cursor drags } } @@ -770,6 +778,7 @@ void ReaSamplerEditor::onMouseDown(int x, int y) { drag_ = DragKind::kRootMarker; dragStartX_ = x; dragStartRoot_ = note; + dragStartMap_ = map_; // A click sets the root immediately (drag then refines); the override lives on // a one-zone map entry for the picked capture (D-B, never written to the bank). onMouseMove(x, y); // apply the click position as the first delta==0 set @@ -836,6 +845,7 @@ void ReaSamplerEditor::onMouseDown(int x, int y) { dragStartX_ = x; dragStartLow_ = z.lowNote; dragStartHigh_ = z.highNote; + dragStartMap_ = map_; switch (hit.grab) { case ZoneGrab::kLowEdge: drag_ = DragKind::kZoneLow; break; case ZoneGrab::kHighEdge: drag_ = DragKind::kZoneHigh; break; @@ -918,7 +928,7 @@ void ReaSamplerEditor::onMouseMove(int x, int y) { } // Build the edited marker set from the snapshot, moving only the grabbed marker, then - // clamp: loopStart <= loopEnd, start in [0, frames]. Dragging a loop marker MAKES a loop. + // clamp: loopStart <= loopEnd, start in [0, frames-1]. Dragging a loop marker MAKES a loop. SetupMarkers m = dragStartMarkers_; if (waveMarker_ == WaveMarker::kStart) { m.start = newFrame; @@ -930,32 +940,11 @@ void ReaSamplerEditor::onMouseMove(int x, int y) { m.hasLoop = true; } if (m.start < 0) m.start = 0; - if (m.start > frames) m.start = frames; + if (m.start > frames - 1) m.start = frames - 1; // Upsert the override on the picked id (mirror of the root-marker path); commit lands on // release, this is live feedback. - SampleLoop loop; - loop.hasLoop = m.hasLoop; - loop.start = m.loopStart; - loop.end = m.loopEnd; - bool found = false; - for (PerformanceZone& z : map_.zones) { - if (z.sampleId == selectedId_) { - z.loopOverride = loop; - z.startPoint = m.start; - found = true; - break; - } - } - if (!found) { - PerformanceZone z; - z.sampleId = selectedId_; - z.lowNote = 0; - z.highNote = 127; - z.loopOverride = loop; - z.startPoint = m.start; - map_.zones.push_back(z); - } + upsertPickedOverride(m); invalidate(); return; } @@ -1017,10 +1006,12 @@ LRESULT CALLBACK ReaSamplerEditor::wndProc(HWND hwnd, UINT msg, WPARAM wParam, } return 0; case WM_CAPTURECHANGED: - // Capture stolen mid-drag (modal dialog, alt-tab, etc.) — reset the drag - // state machine so stale capture-less WM_MOUSEMOVEs don't keep editing. + // Capture stolen mid-drag (modal dialog, alt-tab, etc.) — restore map_ to its + // pre-grab snapshot so the in-flight live-drag mutation is rolled back, then reset + // the drag state machine so stale capture-less WM_MOUSEMOVEs don't keep editing. // Mirror of bank_panel.cpp's WM_CAPTURECHANGED handler. if (self && self->drag_ != DragKind::kNone) { + self->map_ = self->dragStartMap_; self->drag_ = DragKind::kNone; self->invalidate(); } diff --git a/src/vst/reasampler_editor.h b/src/vst/reasampler_editor.h index a34f704..1aada1f 100644 --- a/src/vst/reasampler_editor.h +++ b/src/vst/reasampler_editor.h @@ -135,6 +135,11 @@ private: // (upsert on the picked id — mirror of the root-marker path), then reload off-thread. void commitPickedMarkers(const SetupMarkers& m); + // Write `m` as a loop/start override upsert into map_ for selectedId_ (find-or-append). + // Does NOT call commitAndReload — callers decide whether this is a live-drag update or a + // final commit. selectedId_ must be non-empty before calling. + void upsertPickedOverride(const SetupMarkers& m); + ReaSamplerProcessor* processor_ = nullptr; // --- Snapshot of the live bank (drawn each paint; refreshed off the audio thread) --- @@ -155,6 +160,7 @@ private: int dragStartLow_ = 0; // the grabbed field's note at grab time int dragStartHigh_ = 0; int dragStartRoot_ = 60; + PerformanceMap dragStartMap_; // map_ snapshotted at grab; restored on capture-loss // S11 waveform-marker drag: which marker + the marker set snapshotted at grab time (so the // pixel-delta resolver shifts the grabbed frame from its grab-time value, and inter-marker diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index 700e9a0..4c1e949 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -592,6 +592,43 @@ static void testStartFrameWithLoop() { for (std::size_t i = 60; i < out.size(); ++i) CHECK(approx(out[i], 0.5, 1e-4)); } +static void testStartAfterLoopEndWrapsIntoLoop() { + // Regression (S11 reviewer finding): if startFrame > loop.end (but still < frameCount), + // the voice's initial read head is past the loop end. The wrap-while in renderFrame must + // pull it back into [loopStart, loopEnd) on the very first frame, so the note sounds from + // somewhere inside the loop rather than running off the sample end silently. + // + // Setup: 100-frame sample; loop is [20, 40); startFrame = 60 (past loop.end = 40). + // loop body is a constant 0.5 so every frame inside it reads 0.5. + // After wrap: readPos starts inside [20, 40), first output frame == 0.5. + // Voice must stay active (loop sustains it) and emit the loop value, NOT go silent. + SampleData s; + s.frames.resize(100, 0.0f); + for (int i = 20; i < 40; ++i) s.frames[i] = 0.5f; // loop body + s.rootNote = 60; + s.startFrame = 60; // > loop.end (40), < frameCount (100) + s.loop.hasLoop = true; + s.loop.start = 20; + s.loop.end = 40; + + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, flatAdsr()); + eng.noteOn(60, 127); // unity ratio, full velocity + + std::vector out; + eng.render(out, 50); + + // Voice must still be active — the usable loop keeps it alive indefinitely. + CHECK(eng.activeVoiceCount() == 1); + // Every frame after the first wrap must read 0.5 (the loop body). We skip the very + // first frame because the fractional-position wrap lands somewhere in [20,40) and the + // exact offset depends on how many loop lengths fit into 60; what matters is that the + // voice is alive and emitting the loop value, not 0.0 (pre-loop region). + for (std::size_t i = 5; i < out.size(); ++i) { + CHECK(approx(out[i], 0.5, 1e-4)); + } +} + // --------------------------------------------------------------------------- // velocity -> volume. // --------------------------------------------------------------------------- @@ -656,6 +693,7 @@ int main() { testStartFrameZeroIsUnchanged(); testStartFrameOutOfRangeClampsToZero(); testStartFrameWithLoop(); + testStartAfterLoopEndWrapsIntoLoop(); testVelocityToVolume(); testPolyphonyMixesAdditively();