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

@@ -220,13 +220,10 @@ ReaSamplerEditor::SetupMarkers ReaSamplerEditor::pickedMarkers(std::int64_t fram
     return m;
 }
 
-void ReaSamplerEditor::commitPickedMarkers(const SetupMarkers& m) {
+void ReaSamplerEditor::upsertPickedOverride(const SetupMarkers& m) {
-    // Materialize the edited markers as a per-zone loop/start override on the picked id (upsert,
+    // Find-or-append the zone for selectedId_ and write the loop/start override fields.
-    // mirror of the root-marker path): a full-keyboard zone carrying the override. This plays
+    // The bank intrinsic is NEVER written (read-only bank consumer, D-B). selectedId_ must
-    // identically to the un-zoned single capture (one chromatic zone) and round-trips through
+    // be non-empty; callers are responsible for that guard.
-    // 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;
     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;
+        upsertPickedOverride(m);
-        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);
-        }
         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
+            // Capture stolen mid-drag (modal dialog, alt-tab, etc.) — restore map_ to its
-            // state machine so stale capture-less WM_MOUSEMOVEs don't keep editing.
+            // 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();
             }

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

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<AudioSample> 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();