fix: close round-2 review findings — Critical silent-note bug plus majors/minors

Fixes the 2-point spline-EG early-free bug causing silent fade-ins, the pitch/filter enable-toggle Trigger-forcing hole, the contour-node/marker pixel shadow, missing deck-residue test coverage, and stale comments in knob_deck and spline_edit.

src/core/instrument/engine/velocity_curve.h

@@ -229,11 +229,16 @@ public:
     }
     void clear() { pts_ = nullptr; n_ = 0; }
     bool active() const { return n_ >= 2; }
-    // True once the cursor has settled on the contour's LAST segment: past this point there is
-    // no further point to rise into, so a value read here that reaches 0 is a genuine permanent
-    // terminus (Voice::tickAmplitude's early-free), unlike a 0 touched mid-contour, which a
-    // later segment may still rise out of (the spline is deliberately not globally monotone).
+    // True once the cursor has settled on the contour's LAST segment. On its own this does NOT
+    // make a 0 read here a terminus: the final segment's LEFT endpoint can also be 0 (a 2-point
+    // contour is nothing but a single "final" segment starting at frame 0), which would read 0
+    // while about to rise. Voice::tickAmplitude pairs this with terminalValue() == 0 — the
+    // segment's RIGHT endpoint, i.e. the whole contour's true end — before calling a 0 read the
+    // note's genuine permanent terminus.
     bool onFinalSegment() const { return seg_ + 2 == n_; }
+    // The current segment's right endpoint — the whole contour's terminal Y only when paired
+    // with onFinalSegment() (see there).
+    double terminalValue() const { return y1_; }
 
     // `phase` is normalized position over the contour's whole span, [0,1]; out-of-range clamps
     // to the terminal values (a note past its span holds the contour's last level).

src/core/instrument/engine/voice.h

@@ -186,12 +186,18 @@ private:
         if (ampSplineCur_.active() && playMode_ == PlayMode::Trigger) {
             // A contour covers the sample end to end, so the head leaving the span IS the end of
             // the note — the exhaustion path in advanceFrame is what frees the voice. A contour
-            // that flatlines at 0 across its FINAL segment is a permanent terminus (no later
-            // segment to rise out of), so that case frees early too, the spline analogue of a
-            // staged AHD's finished() — mid-contour dips do not, since the spline is deliberately
+            // whose TERMINAL value (the final segment's right endpoint) is 0 reaches a genuine
+            // permanent terminus early, the spline analogue of a staged AHD's finished(). Gating
+            // on the terminal value, not just the segment index, matters because a 2-point
+            // contour IS a single "final" segment from frame 0 — checking onFinalSegment() alone
+            // would call a contour that STARTS at 0 (e.g. a fade-in) over before it ever rises.
+            // Mid-contour dips through 0 still don't free early, since the spline is deliberately
             // not globally monotone.
             amp = ampSplineCur_.eval(splinePhase());
-            if (amp == 0.0 && ampSplineCur_.onFinalSegment()) amplitudeDone_ = true;
+            if (amp == 0.0 && ampSplineCur_.onFinalSegment() &&
+                ampSplineCur_.terminalValue() == 0.0) {
+                amplitudeDone_ = true;
+            }
         } else if (playMode_ == PlayMode::Gate) {
             amp = env_.tick();
             if (env_.finished()) amplitudeDone_ = true;

src/core/instrument/ui/knob_deck.cpp

@@ -180,11 +180,12 @@ DeckHit hitTestDeck(const DeckLayout& layout, int x, int y) {
                 return {DeckHitKind::RowToggle, g.rowToggle.id, 1};
         }
         for (const DeckCellLayout& c : g.cells) {
-            if (c.id >= 0 && contains(c.cell, x, y)) {
+            // Every entry here already has a real id — a reserve yields no DeckCellLayout at all.
+            if (contains(c.cell, x, y)) {
                 return {DeckHitKind::Knob, c.id, -1, contains(c.inner, x, y)};
             }
         }
-        return {};  // inside the box but on fence/padding/blank — a miss (groups never overlap)
+        return {};  // inside the box but on fence/padding — a miss (groups never overlap)
     }
     return {};
 }

src/core/instrument/ui/knob_deck.h

@@ -69,7 +69,7 @@ struct DeckGroupDesc {
     // first is absent) — why this exists rather than a rowToggle is recorded once, at this
     // module's CLAUDE.md bullet.
     DeckToggleDesc captionToggle2;
-    std::vector<int> cellIds;      // knob cells; -1 = blank reserve
+    std::vector<int> cellIds;      // knob cells; -1 reserves width only, no cell (see above)
     DeckToggleDesc rowToggle;      // in the knob row after the cells; id -1 = none
 };
 

src/core/instrument/ui/spline_edit.h

@@ -1,7 +1,8 @@
-// spline_edit.h — THE point-editing grammar, and the one place it is written down. Both spline
-// consumers route their mouse-down through it — the velocity-curve popup and the spline EG
-// overlay — so the two cannot drift into two grammars. Mirror of envelope_edit: decision logic
-// only, no host types, no drawing.
+// spline_edit.h — the point-editing grammar's CLICK resolution: add/grab/delete/toggle from a
+// single (x, y). Both spline consumers — the velocity-curve popup and the spline EG overlay —
+// route their mouse-down through it, so the two cannot drift into two click grammars. Two more
+// gesture rules complete the grammar but live in the shell (see the note near the bottom of this
+// file). Mirror of envelope_edit otherwise: decision logic only, no host types, no drawing.
 
 #pragma once
 
@@ -39,4 +40,15 @@ SplineEdit resolveSplineEdit(const VelocityCurve& curve, const VelocityCurve::Bo
 // waveform beneath it. Takes the overlay (not a lane) — see waveform_view.h's overlay contract.
 VelocityCurve::Box splineOverlayBox(const OverlayArea& area);
 
+// Two more rules complete the grammar. Both are enforced in the shell — mouse-tracking / drag
+// state has no home in a pure module — and recorded here as their one home rather than restated
+// at each call site:
+// - DRAG-OFF DELETE: releasing a grabbed node well outside its box deletes it (endpoints exempt,
+//   per deletePoint's own refusal) — editor_input.cpp's onMouseUp, shared verbatim by the popup
+//   and the overlay.
+// - THE OVERLAY'S OUTSIDE-BOX EXCEPTION: resolveSplineEdit's own outside-box grab/toggle/delete
+//   allowance (above) is meant for the popup's inset ring; the overlay narrows it back to
+//   strictly in-box, since splineOverlayBox has no inset — editor_input_waveform.cpp's
+//   splineOverlayClick.
+
 }  // namespace reasampler::instrument::ui

src/shell/instrument/editor_controls.cpp

@@ -185,7 +185,6 @@ void ReaSamplerEditor::applyControl(int id, PlaySeconds& play, double value,
                 case ParamControl::kPitchEnvMode:  play.pitchSpline.mode = m; break;
                 default:                           play.filterSpline.mode = m; break;
             }
-            if (splineActive(play)) play.playMode = PlayMode::Trigger;
             break;
         }
         case ParamControl::kPitchEngine:
@@ -271,6 +270,12 @@ void ReaSamplerEditor::applyControl(int id, PlaySeconds& play, double value,
             play.filter.trigEnv.decayCurve = util::curveFromKnobNorm(value); break;
         default: break;
     }
+    // ONE normalization point for every control that can flip splineActive — a mode toggle
+    // (above) or an enable toggle (kPitchEnvEnable/kFilterEnable), whose enabling can make an
+    // already-Spline pitch/filter envelope newly active. Applying it once here, rather than at
+    // each site that could cause the flip, is what keeps a future such control from reopening
+    // the same hole.
+    if (splineActive(play)) play.playMode = PlayMode::Trigger;
 }
 
 double ReaSamplerEditor::liveSampleRate() const {

src/shell/instrument/editor_input_waveform.cpp

@@ -31,11 +31,6 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) {
     if (frames <= 0) return false;
     const OverlayArea overlay = waveformOverlayArea(fl.bands.waveform);
 
-    // A drawn EG's contour is evaluated below, AFTER the markers: pointAtPixel's pick radius
-    // has no box check of its own, so a coincident contour node would otherwise shadow a
-    // marker's own dedicated grab rect (the loop-crossfade tab most sharply, since it is the
-    // ONLY affordance at zero crossfade) — marker reachability wins on any pixel overlap. The
-    // staged envelope-node pass just below is unaffected (its own, longer-standing ordering).
     const DeckEnableState gates = deckEnableState();
     const bool splineLive = overlayIsSpline() && overlayEnvEnabled(overlayEnv_, gates);
     const SplineGesture gesture = (GetKeyState(VK_CONTROL) & 0x8000) != 0
@@ -64,6 +59,17 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) {
             return true;  // node moves once the cursor drags
         }
     }
+    // An existing contour node's grab/toggle/delete runs BEFORE the markers — mirroring
+    // markerHandleRect's tab-vs-column split (below): a coincident pixel (the default contour
+    // endpoint sits at the same x as the default start marker) is resolved by asking the
+    // NARROWER target first. pointAtPixel's pick radius is a small box around the node's own
+    // (x, y), not a full-height column, so this claims only genuine node hits — the marker's
+    // column stays grabbable at every other y along the same x. Never add here (kAdd is only
+    // tried once the markers have also passed on the click, below).
+    if (splineLive && splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false)) {
+        return true;
+    }
+
     const SetupMarkers m = pickedMarkers(frames);
     // The crossfade handle first, and only when there IS a loop to fade: at a zero fade it
     // sits exactly on the loop start, so it can only stay reachable by owning the top strip
@@ -82,11 +88,6 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) {
         beginMarkerDrag(static_cast<WaveMarker>(hit), m, frames, x);
         return true;
     }
-    // No marker wanted the click: a drawn EG's own node grab/toggle (never add — empty space is
-    // tried last, below).
-    if (splineLive && splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false)) {
-        return true;
-    }
     // Nothing else wanted the click: now the drawn contour may take the empty space.
     if (splineLive) return splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/true);
     return false;

src/shell/instrument/editor_models.cpp

@@ -69,10 +69,14 @@ StageEnvelope ReaSamplerEditor::packEnvelope(OverlayEnv which, const PlaySeconds
     // post-start span, since it keeps running after a Trigger one-shot's amplitude has ended.
     const std::int64_t playLen =
         triggerPlayLength(play.trigger.lengthFraction, frames, startFrame);
-    const double playSpan = rate > 0.0 ? static_cast<double>(playLen) / rate : 0.0;
     const double fullSpan =
         rate > 0.0 ? static_cast<double>((std::max)(std::int64_t{0}, frames - startFrame)) / rate
                    : 0.0;
+    // Voice::start makes kTrigLength inert while any spline EG is active (trigSpan == postStart,
+    // not the %-length) — the overlay's amp/filter AHD must read the SAME span the engine plays,
+    // or its drawn shape and node drags cover only a fraction of what the note actually does.
+    const double playSpan =
+        splineActive(play) ? fullSpan : (rate > 0.0 ? static_cast<double>(playLen) / rate : 0.0);
     const bool trigger = (play.playMode == PlayMode::Trigger);
     switch (which) {
         case OverlayEnv::kPitch:

tests/test_component_state_io.cpp

@@ -788,6 +788,11 @@ static void testNonFiniteAhdSecondsLiftToZero() {
 // the record is disturbed. Corrupts only the AMP curve's tail; FILTER/PITCH follow at their
 // normal, byte-precise offsets, proving a mismatch on one curve does not cascade to its
 // neighbours.
+//
+// Companion, not a regression guard: this in-bounds-mismatch path was already non-wiping
+// before the r.ok fix below — it pins the documented promise, not the fix. The two tests that
+// follow (OUT-OF-BOUNDS count, and a mid-count truncation) are what actually guard it — both
+// tripped the old "reset the whole record to defaults" behavior.
 static void testV13HardFlagInBoundsMismatchDropsFlagsOnly() {
     ComponentState in;
     in.selectionId = "pad";

tests/test_deck_groups.cpp

@@ -565,6 +565,31 @@ static void testNoFaceLeavesSlackWhereItsDroppedControlsWere() {
     }
 }
 
+// Every shipped face's reserve divides its present-cell count evenly (see
+// testNoFaceLeavesSlackWhereItsDroppedControlsWere), so none of them exercises the
+// "residue lands in symmetric end margins" rule knob_deck.cpp documents — only that the
+// leftover is small, not where it goes. A synthetic 7-slot reserve with 5 present (336/5,
+// remainder 1) forces a real residue and pins it split across BOTH ends.
+static void testASyntheticIndivisibleReserveSplitsItsResidueAcrossBothEnds() {
+    const DeckGroupDesc g{0, 78, {}, {100, 44}, {}, {20, 21, 22, 23, 24, -1, -1}, {}};
+    const std::vector<DeckGroupDesc> gs{g};
+    const DeckLayout dl = layoutDeck(gs, 0, 0, kAvailAtMinWidth);
+    const DeckGroupLayout& lay = dl.groups[0];
+    CHECK(lay.cells.size() == 5);
+
+    const int run = 7 * kDeckCellW;
+    const int cellW = run / 5;                // the same integer division layoutGroup uses
+    const int residue = run - cellW * 5;       // 1: nonzero, unlike every shipped face's reserve
+    CHECK(residue > 0);
+
+    const int covered = lay.cells.back().cell.right() - lay.cells.front().cell.x;
+    CHECK(run - covered == residue);
+    const int leadPad = lay.cells.front().cell.x - (lay.box.x + kDeckGroupPadX);
+    const int trailPad = (lay.box.right() - kDeckGroupPadX) - lay.cells.back().cell.right();
+    CHECK(leadPad == residue / 2);
+    CHECK(trailPad == residue - leadPad);      // both ends share it, not one cell absorbing it
+}
+
 // Gate is the common face and it already packs correctly: pin its group widths and row
 // assignment at the floor so a later edit anywhere in the deck cannot reflow it silently.
 // (Measured from the shipped descriptors, not copied out of a failing run.)
@@ -666,6 +691,7 @@ int main() {
     testWrappedDeckHeightAtTheEditorFloorWidth();
     testDeckFitsInsideTheEnforcedMinimumWindow();
     testNoFaceLeavesSlackWhereItsDroppedControlsWere();
+    testASyntheticIndivisibleReserveSplitsItsResidueAcrossBothEnds();
     testGateModeWidthsAndRowAssignmentAreUnchanged();
     testGateSplineGateRoundTripsToTheSameLayout();
     testHitTestResolvesTheNewFilterControls();

tests/test_knob_deck.cpp

@@ -156,9 +156,14 @@ static void testHitTest() {
     const DeckGroupLayout& tg = tl.groups[0];
     CHECK(tg.cells.size() == 3);
     for (const DeckCellLayout& c : tg.cells) CHECK(c.id >= 0);
-    const DeckCellLayout& last = tg.cells.back();
-    for (int px = tg.cells[0].cell.x; px < last.cell.right(); ++px) {
-        const DeckHit rowHit = hitTestDeck(tl, px, last.cell.y + 5);
+    // Bound the sweep against the RESERVED run (5 slots, not the 3 present cells) rather than
+    // the cells' own extent — the cells are what's under test, so deriving the bound from them
+    // could never catch a layout that under-covers the run they were reserved out of.
+    const int runStart = tg.box.x + kDeckGroupPadX;
+    const int runEnd = runStart + static_cast<int>(trig[0].cellIds.size()) * kDeckCellW;
+    const int rowY = tg.cells.back().cell.y + 5;
+    for (int px = runStart; px < runEnd; ++px) {
+        const DeckHit rowHit = hitTestDeck(tl, px, rowY);
         CHECK(rowHit.kind == DeckHitKind::Knob && rowHit.id >= 0);
     }
 
@@ -214,6 +219,31 @@ static void testReservedCellWidthGoesToTheCellsPresent() {
           layoutDeck(b, 0, 0, 824).groups[0].rowToggle.seg0);
 }
 
+// The three faces above (240/3, 240/4, 240/1) all divide their run evenly, so none of them
+// actually exercises "residue in symmetric end margins" — a 7-slot reserve with 5 present
+// (336/5, remainder 1) does, and pins the residue split across BOTH ends rather than only
+// the leading one.
+static void testIndivisibleResidueLandsInSymmetricEndMargins() {
+    const DeckGroupDesc g{0, 78, {}, {100, 44}, {}, {20, 21, 22, 23, 24, -1, -1}, {}};
+    std::vector<DeckGroupDesc> gs{g};
+    const DeckLayout dl = layoutDeck(gs, 0, 0, 824);
+    const DeckGroupLayout& lay = dl.groups[0];
+    CHECK(lay.cells.size() == 5);
+
+    const int run = 7 * kDeckCellW;
+    const int present = 5;
+    const int cellW = run / present;               // 67: the same integer division the layout uses
+    const int expectedResidue = run - cellW * present;  // 1: the case the even-dividing faces can't reach
+    CHECK(expectedResidue > 0);
+
+    const int covered = lay.cells.back().cell.right() - lay.cells.front().cell.x;
+    CHECK(run - covered == expectedResidue);
+    const int leadPad = lay.cells.front().cell.x - (lay.box.x + kDeckGroupPadX);
+    const int trailPad = (lay.box.right() - kDeckGroupPadX) - lay.cells.back().cell.right();
+    CHECK(leadPad == expectedResidue / 2);
+    CHECK(trailPad == expectedResidue - leadPad);  // both ends share it, not one absorbing it
+}
+
 // The corner radio widens the caption row, takes the far corner, and pushes the caption
 // toggle left of itself — the three properties the overlay-select switch relies on.
 static void testCaptionRadioGeometryAndHit() {
@@ -307,6 +337,7 @@ int main() {
     testGroupInnerGeometry();
     testHitTest();
     testReservedCellWidthGoesToTheCellsPresent();
+    testIndivisibleResidueLandsInSymmetricEndMargins();
     testCaptionRadioGeometryAndHit();
     testInnerDialHit();
     testCaptionToggle2();

tests/test_spline_egs.cpp

@@ -321,6 +321,29 @@ static void testAContourReplaysProportionallyOnADifferentLengthSample() {
     CHECK(a[shortLen / 2] > a[10] + 0.2);
 }
 
+// --- Regression: a 2-point contour starting at 0 is not a terminus at frame 0 -----------
+//
+// onFinalSegment() (seg_+2==n_) is trivially true for a 2-point contour. Gating the amp
+// spline's early-free on that alone reads a contour's OWN opening value as the note's end,
+// so the simplest fade-in (left knot dragged to the box floor) went silent at frame 0. The
+// fix requires the TERMINAL value (the segment's right endpoint) to be 0, not just the
+// segment index.
+static void testTwoPointContourRisingFromZeroSoundsForItsFullSpan() {
+    const VelocityCurve contour =
+        VelocityCurve::fromPoints({{kVelMin, 0.0}, {kVelMax, 1.0}}, CurveDomain::Unipolar);
+    const std::size_t frames = 1000;
+    const SampleData s = splineAmpSample(frames, contour);
+
+    Voice v;
+    v.start(60, 100, s);
+    CHECK(v.soundingNote());              // fresh note: sounding before anything is rendered
+    const double y0 = v.renderFrame();
+    CHECK(near(y0, 0.0, 1e-9));           // the contour's own value at phase 0 IS 0 ...
+    CHECK(v.soundingNote());              // ...but the note itself must not be over yet
+    for (std::size_t i = 1; i < frames / 2; ++i) v.renderFrame();
+    CHECK(v.soundingNote());              // still sounding at the midpoint, rising toward 1
+}
+
 // --- 9. A fresh spline EG opens on the smooth y = 1 - x ----------------------
 
 static void testAFreshSplineEgDefaultsToTheSmoothDownwardSlope() {
@@ -467,6 +490,7 @@ int main() {
     testStagedAndSplineStatesBothSurviveAFlipAndASaveReload();
     testAV12PayloadLoadsWithoutLoss();
     testAContourReplaysProportionallyOnADifferentLengthSample();
+    testTwoPointContourRisingFromZeroSoundsForItsFullSpan();
     testAFreshSplineEgDefaultsToTheSmoothDownwardSlope();
     testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards();
     testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete();