diff --git a/src/core/instrument/engine/velocity_curve.h b/src/core/instrument/engine/velocity_curve.h index 24642db..a8fb324 100644 --- a/src/core/instrument/engine/velocity_curve.h +++ b/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). diff --git a/src/core/instrument/engine/voice.h b/src/core/instrument/engine/voice.h index fd094e0..5330b41 100644 --- a/src/core/instrument/engine/voice.h +++ b/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; diff --git a/src/core/instrument/ui/knob_deck.cpp b/src/core/instrument/ui/knob_deck.cpp index 273473d..3327627 100644 --- a/src/core/instrument/ui/knob_deck.cpp +++ b/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 {}; } diff --git a/src/core/instrument/ui/knob_deck.h b/src/core/instrument/ui/knob_deck.h index 8dd4f36..af371c7 100644 --- a/src/core/instrument/ui/knob_deck.h +++ b/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 cellIds; // knob cells; -1 = blank reserve + std::vector cellIds; // knob cells; -1 reserves width only, no cell (see above) DeckToggleDesc rowToggle; // in the knob row after the cells; id -1 = none }; diff --git a/src/core/instrument/ui/spline_edit.h b/src/core/instrument/ui/spline_edit.h index 31bea11..e139d04 100644 --- a/src/core/instrument/ui/spline_edit.h +++ b/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 diff --git a/src/shell/instrument/editor_controls.cpp b/src/shell/instrument/editor_controls.cpp index 57327c6..f03bdbf 100644 --- a/src/shell/instrument/editor_controls.cpp +++ b/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 { diff --git a/src/shell/instrument/editor_input_waveform.cpp b/src/shell/instrument/editor_input_waveform.cpp index 3093c38..99a101d 100644 --- a/src/shell/instrument/editor_input_waveform.cpp +++ b/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(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; diff --git a/src/shell/instrument/editor_models.cpp b/src/shell/instrument/editor_models.cpp index 4c51136..e2efb70 100644 --- a/src/shell/instrument/editor_models.cpp +++ b/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(playLen) / rate : 0.0; const double fullSpan = rate > 0.0 ? static_cast((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(playLen) / rate : 0.0); const bool trigger = (play.playMode == PlayMode::Trigger); switch (which) { case OverlayEnv::kPitch: diff --git a/tests/test_component_state_io.cpp b/tests/test_component_state_io.cpp index 75da1fd..60d5c51 100644 --- a/tests/test_component_state_io.cpp +++ b/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"; diff --git a/tests/test_deck_groups.cpp b/tests/test_deck_groups.cpp index d65564b..fbe71e4 100644 --- a/tests/test_deck_groups.cpp +++ b/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 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(); diff --git a/tests/test_knob_deck.cpp b/tests/test_knob_deck.cpp index 95d47f3..c34bdb7 100644 --- a/tests/test_knob_deck.cpp +++ b/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(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 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(); diff --git a/tests/test_spline_egs.cpp b/tests/test_spline_egs.cpp index 5e52b60..75e70c4 100644 --- a/tests/test_spline_egs.cpp +++ b/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();