From 757e1585d62e4d5e49c91078fe7a578c7d5f983e Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 31 Jul 2026 23:44:05 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20close=20round-3=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20smallest-target-first,=20residue=20test=20fix,=20ex?= =?UTF-8?q?traction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Waveform overlay now resolves node/tab/marker click collisions by target area instead of check order; residue test now uses a distinguishing fixture; Gate-unavailable-while-drawn logic extracted to one pure helper shared by resolvePlay and applyControl. --- src/core/instrument/engine/play_params.h | 11 ++ src/core/instrument/engine/velocity_curve.h | 9 +- src/core/instrument/engine/voice.h | 13 +-- src/core/instrument/map/sample_map.cpp | 8 +- src/core/instrument/ui/CMakeLists.txt | 6 +- src/shell/instrument/editor_controls.cpp | 4 +- .../instrument/editor_input_waveform.cpp | 85 +++++++++----- tests/test_deck_groups.cpp | 27 +---- tests/test_knob_deck.cpp | 33 ++++-- tests/test_spline_egs.cpp | 42 +++++++ tests/test_waveform_view.cpp | 104 +++++++++++++++++- 11 files changed, 255 insertions(+), 87 deletions(-) diff --git a/src/core/instrument/engine/play_params.h b/src/core/instrument/engine/play_params.h index 896a8fe..0309223 100644 --- a/src/core/instrument/engine/play_params.h +++ b/src/core/instrument/engine/play_params.h @@ -195,6 +195,17 @@ bool splineActive(const Play& p) { (p.filter.enabled && p.filterSpline.mode == EnvMode::Spline); } +// The one enforcement of the rule above: forces `p` to Trigger the instant it goes +// splineActive. Every path that can leave `p` in a state splineActive() would newly return true +// for — resolving stored params toward the engine, or an editor control edit that can flip a +// mode/enable toggle — routes through this, so none of them can reopen the Gate+spline hole +// splineActive's own doc comment describes. Header-inline and allocation-free: play_params.h +// sits on the per-voice-per-sample include path. +template +void enforceGateUnavailableWhileDrawn(Play& p) { + if (splineActive(p)) p.playMode = PlayMode::Trigger; +} + // [start, end) frames, half-open. A zero-length loop (start == end) is the "no sustain loop" // marker — a held note past the sample end goes silent rather than looping a zero span. struct SampleLoop { diff --git a/src/core/instrument/engine/velocity_curve.h b/src/core/instrument/engine/velocity_curve.h index a8fb324..9df5be2 100644 --- a/src/core/instrument/engine/velocity_curve.h +++ b/src/core/instrument/engine/velocity_curve.h @@ -232,13 +232,14 @@ public: // 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 + // while about to rise. Voice::tickAmplitude pairs this with segmentEndValue() == 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_; } + // The CURRENT SEGMENT's right endpoint — not a contour-level concept despite the name's + // shape; it is the whole contour's terminal Y only when paired with onFinalSegment() (see + // there). Named for what it returns, not for its one call site's use of it. + double segmentEndValue() 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 5330b41..d1cabe3 100644 --- a/src/core/instrument/engine/voice.h +++ b/src/core/instrument/engine/voice.h @@ -184,18 +184,11 @@ private: // entirely — release() then has no envelope to end, and an active sustain loop rings // forever. 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 - // 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. + // Early-free at a genuine permanent terminus (the spline analogue of a staged AHD's + // finished()) — onFinalSegment()/segmentEndValue()'s own doc comments own the why. amp = ampSplineCur_.eval(splinePhase()); if (amp == 0.0 && ampSplineCur_.onFinalSegment() && - ampSplineCur_.terminalValue() == 0.0) { + ampSplineCur_.segmentEndValue() == 0.0) { amplitudeDone_ = true; } } else if (playMode_ == PlayMode::Gate) { diff --git a/src/core/instrument/map/sample_map.cpp b/src/core/instrument/map/sample_map.cpp index 62c1b77..edc116d 100644 --- a/src/core/instrument/map/sample_map.cpp +++ b/src/core/instrument/map/sample_map.cpp @@ -262,10 +262,10 @@ PlayParams resolvePlay(const PlaySeconds& stored, int sampleRate) { out.ampSpline = stored.ampSpline; out.pitchSpline = stored.pitchSpline; out.filterSpline = stored.filterSpline; - // Gate is unavailable while any EG is drawn — see splineActive (play_params.h) for why. - // The editor refuses the Gate segment for the same reason; enforcing it HERE as well is - // what keeps a hand-edited or downgraded blob from reaching the engine as Gate + spline. - if (splineActive(stored)) out.playMode = PlayMode::Trigger; + // Every field splineActive reads on `out` is already copied from `stored` above, so this + // enforces the same rule enforceGateUnavailableWhileDrawn's doc comment (play_params.h) + // describes — the editor's applyControl is the other caller, so the two cannot drift. + enforceGateUnavailableWhileDrawn(out); return out; } diff --git a/src/core/instrument/ui/CMakeLists.txt b/src/core/instrument/ui/CMakeLists.txt index 21d4881..5c0de91 100644 --- a/src/core/instrument/ui/CMakeLists.txt +++ b/src/core/instrument/ui/CMakeLists.txt @@ -24,8 +24,10 @@ reasampler_pure_library(waveform_view SOURCES waveform_view.cpp LINK PUBLIC editor_geometry peaks PRIVATE sample_bands) # sample_bands is linked directly here because the test exercises the lane metrics that -# waveform_view does not re-export. -reasampler_test(waveform_view LINK waveform_view sample_bands) +# waveform_view does not re-export. velocity_curve is linked for the smallest-target-first +# arbitration tests, which pin the geometric facts editor_input_waveform.cpp's node/tab/marker +# resolution depends on (the shell itself has no test target). +reasampler_test(waveform_view LINK waveform_view sample_bands velocity_curve) reasampler_pure_library(browser_scroll SOURCES browser_scroll.cpp diff --git a/src/shell/instrument/editor_controls.cpp b/src/shell/instrument/editor_controls.cpp index f03bdbf..1a1126d 100644 --- a/src/shell/instrument/editor_controls.cpp +++ b/src/shell/instrument/editor_controls.cpp @@ -274,8 +274,8 @@ void ReaSamplerEditor::applyControl(int id, PlaySeconds& play, double value, // (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; + // the same hole. `resolvePlay` (sample_map.cpp) is the other caller of the shared helper. + enforceGateUnavailableWhileDrawn(play); } double ReaSamplerEditor::liveSampleRate() const { diff --git a/src/shell/instrument/editor_input_waveform.cpp b/src/shell/instrument/editor_input_waveform.cpp index 99a101d..bdaba5a 100644 --- a/src/shell/instrument/editor_input_waveform.cpp +++ b/src/shell/instrument/editor_input_waveform.cpp @@ -12,7 +12,8 @@ #include #include -#include "core/instrument/engine/loop/loop_span.h" // maxCrossfade (the shared drag-clamp bound) +#include "core/instrument/engine/loop/loop_span.h" // maxCrossfade (the shared drag-clamp bound) +#include "core/instrument/engine/velocity_curve.h" // kCurveNodeGrabRadius (target-size arbitration) #include "core/instrument/ui/envelope_edit.h" // nodeAtPoint / resolveNodeDrag #include "core/instrument/ui/spline_edit.h" // the shared point-editing grammar #include "core/instrument/ui/waveform_view.h" // waveformOverlayArea / markerAtPoint / snap @@ -24,6 +25,7 @@ namespace reasampler::vst { using namespace reasampler::ui; using namespace reasampler::instrument::ui; using instrument::engine::loop::maxCrossfade; +using instrument::engine::kCurveNodeGrabRadius; bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) { const std::vector& pcm = monoPcmFor(selectedId_); @@ -59,33 +61,65 @@ 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); + const std::int64_t markerFrames[3] = {m.start, m.loopStart, m.loopEnd}; + + // Three affordances can claim the same pixel: a contour node (a small fixed pick box), the + // crossfade tab (a small clipped top-strip tab), and a marker's full-height grab column + // (waveform_view.h's tab-vs-column split already keeps the tab apart from ITS OWN column; + // this is the cross-affordance case on top of that). Resolving by any fixed check order + // shadows whichever one loses the tie — this seam regressed twice from exactly that fix. + // Instead measure each claimant's own target area and let the SMALLEST hit win: the marker + // column is the odd one out (its target is the whole band height), so it only wins where + // nothing narrower also claims the pixel. Never add here (kAdd is only tried once nothing + // else has claimed the click, below). + struct Candidate { + bool hit = false; + std::int64_t area = 0; + }; + + Candidate node; + if (splineLive) { + const VelocityCurve::Box box = splineOverlayBox(overlay); + // Also require strict in-box, matching splineOverlayClick's own narrowing (spline_edit.h's + // grammar note) — otherwise this candidate could "win" the arbitration below for a click + // splineOverlayClick would then refuse, silently swallowing it instead of falling through + // to the tab/marker checks. + if (contains(overlay.rect, x, y) && splineFor(overlayEnv_).pointAtPixel(box, x, y) >= 0) { + node.hit = true; + constexpr std::int64_t side = 2 * kCurveNodeGrabRadius + 1; + node.area = side * side; + } } - 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 - // (waveform_view.h's handle-vs-column split) and being asked first. The same ambiguity - // recurs whenever ANY marker's frame lands on loopStart - crossfade (most plausibly the - // start marker dragged up against the fade edge), so this check has to run before the - // marker array below regardless of which marker the collision is with. - if (m.hasLoop && - contains(markerHandleRect(overlay, frames, m.loopStart - m.crossfade), x, y)) { + Candidate tab; + const Rect tabRect = + m.hasLoop ? markerHandleRect(overlay, frames, m.loopStart - m.crossfade) : Rect{}; + if (m.hasLoop && contains(tabRect, x, y)) { + tab.hit = true; + tab.area = static_cast(tabRect.width) * tabRect.height; + } + + const int markerHit = markerAtPoint(overlay, frames, markerFrames, 3, x, y); + Candidate marker; + if (markerHit >= 0) { + marker.hit = true; + marker.area = static_cast(2 * kMarkerGrabWidth + 1) * overlay.rect.height; + } + + // Node checked before marker on an area tie so a degenerate (zero-height) overlay still + // prefers the node — unreachable in practice (sample_bands floors the band well above it), + // kept only so this arbitration has one well-defined answer for every input, not just the + // ones the current geometry constants happen to produce. + if (node.hit && (!tab.hit || node.area <= tab.area) && (!marker.hit || node.area <= marker.area)) { + return splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false); + } + if (tab.hit && (!marker.hit || tab.area <= marker.area)) { beginMarkerDrag(WaveMarker::kLoopXfade, m, frames, x); return true; } - const std::int64_t markerFrames[3] = {m.start, m.loopStart, m.loopEnd}; - const int hit = markerAtPoint(overlay, frames, markerFrames, 3, x, y); - if (hit >= 0) { - beginMarkerDrag(static_cast(hit), m, frames, x); + if (marker.hit) { + beginMarkerDrag(static_cast(markerHit), m, frames, x); return true; } // Nothing else wanted the click: now the drawn contour may take the empty space. @@ -98,10 +132,7 @@ bool ReaSamplerEditor::splineOverlayClick(const OverlayArea& waveArea, int x, in const VelocityCurve::Box box = splineOverlayBox(waveArea); VelocityCurve& contour = splineFor(overlayEnv_); SplineEdit edit = resolveSplineEdit(contour, box, gesture, x, y); - // resolveSplineEdit's outside-box grab/delete/toggle allowance (pointAtPixel's radius has - // no box check of its own) was designed for the popup's inset ring; the overlay box has NO - // inset (splineOverlayBox), so honoring it here would extend the grab halo 6px into the - // inter-band pad. kAdd already requires in-box (resolveSplineEdit's own check). + // The overlay's outside-box narrowing — see spline_edit.h's grammar note for why. if (edit.kind != SplineEditKind::kNone && edit.kind != SplineEditKind::kAdd && !(x >= box.left && x < box.left + box.width && y >= box.top && y < box.top + box.height)) { edit = SplineEdit{}; diff --git a/tests/test_deck_groups.cpp b/tests/test_deck_groups.cpp index fbe71e4..cf9f4a4 100644 --- a/tests/test_deck_groups.cpp +++ b/tests/test_deck_groups.cpp @@ -565,30 +565,8 @@ 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 -} +// The "residue lands in symmetric end margins" rule is knob_deck's own (layoutGroup), pinned +// once by its synthetic residue>=2 fixture in test_knob_deck.cpp rather than restated here. // 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. @@ -691,7 +669,6 @@ int main() { testWrappedDeckHeightAtTheEditorFloorWidth(); testDeckFitsInsideTheEnforcedMinimumWindow(); testNoFaceLeavesSlackWhereItsDroppedControlsWere(); - testASyntheticIndivisibleReserveSplitsItsResidueAcrossBothEnds(); testGateModeWidthsAndRowAssignmentAreUnchanged(); testGateSplineGateRoundTripsToTheSameLayout(); testHitTestResolvesTheNewFilterControls(); diff --git a/tests/test_knob_deck.cpp b/tests/test_knob_deck.cpp index c34bdb7..c86bf62 100644 --- a/tests/test_knob_deck.cpp +++ b/tests/test_knob_deck.cpp @@ -148,8 +148,11 @@ static void testHitTest() { h = hitTestDeck(dl, voice.rowToggle.seg1.x + 1, voice.rowToggle.seg1.y + 1); CHECK(h.kind == DeckHitKind::RowToggle && h.id == 104 && h.segment == 1); - // A reserve (id -1) yields no cell of its own, so every point of the knob row lands on a - // real control: no dead rect survives for a grab to fall into. + // A reserve (id -1) yields no cell of its own. This fixture's reserve divides its present + // cells evenly (5 slots / 3 present -> 240/3, no residue), so every point of the knob row + // lands on a real control: no dead rect survives for a grab to fall into. That does NOT + // generalize to an indivisible reserve — a residue leaves a few uncovered margin pixels by + // design (testIndivisibleResidueSplitsSymmetricallyAcrossBothEnds, below). std::vector trig; trig.push_back({0, 78, {}, {100, 44}, {}, {20, 21, 22, -1, -1}, {}}); const DeckLayout tl = layoutDeck(trig, 0, 0, 824); @@ -220,26 +223,32 @@ static void testReservedCellWidthGoesToTheCellsPresent() { } // 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}, {}}; +// actually exercises "residue in symmetric end margins". An 8-slot reserve with 5 present +// (384/5 = 76 r4) does: residue 4 is the smallest case that can tell a symmetric split (2/2) +// apart from a trailing-only one (0/4) — a residue of 1 (0/1 vs 1/0... i.e. 0/1) can't, since +// leadPad = residue/2 rounds to 0 either way, which is exactly why this seam's earlier test +// passed without pinning the rule it was named for. +static void testIndivisibleResidueSplitsSymmetricallyAcrossBothEnds() { + const DeckGroupDesc g{0, 78, {}, {100, 44}, {}, {20, 21, 22, 23, 24, -1, -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 run = 8 * 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 cellW = run / present; // 76: the same integer division the layout uses + const int expectedResidue = run - cellW * present; // 4 + CHECK(expectedResidue == 4); 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(); + // Hard literals, not just the formula: this is the case that actually distinguishes + // symmetric (2/2) from trailing-only (0/4) — see the comment above. + CHECK(leadPad == 2); + CHECK(trailPad == 2); CHECK(leadPad == expectedResidue / 2); CHECK(trailPad == expectedResidue - leadPad); // both ends share it, not one absorbing it } @@ -337,7 +346,7 @@ int main() { testGroupInnerGeometry(); testHitTest(); testReservedCellWidthGoesToTheCellsPresent(); - testIndivisibleResidueLandsInSymmetricEndMargins(); + testIndivisibleResidueSplitsSymmetricallyAcrossBothEnds(); testCaptionRadioGeometryAndHit(); testInnerDialHit(); testCaptionToggle2(); diff --git a/tests/test_spline_egs.cpp b/tests/test_spline_egs.cpp index 75e70c4..16d78a3 100644 --- a/tests/test_spline_egs.cpp +++ b/tests/test_spline_egs.cpp @@ -344,6 +344,25 @@ static void testTwoPointContourRisingFromZeroSoundsForItsFullSpan() { CHECK(v.soundingNote()); // still sounding at the midpoint, rising toward 1 } +// The positive direction of the fix above: a contour whose final segment is flat at 0 (here, +// the whole two-point span) DOES free the voice early, on its very first tick. Nothing exercises +// this without it — a future tightening of the gate (e.g. requiring more than onFinalSegment() + +// segmentEndValue()) could silently turn the early-free off, which is a performance regression +// (a ringing but silent voice) rather than an audible one, so nothing else would catch it. +static void testFlatZeroFinalSegmentStillFreesTheVoiceEarly() { + const VelocityCurve contour = + VelocityCurve::fromPoints({{kVelMin, 0.0}, {kVelMax, 0.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)); + CHECK(!v.soundingNote()); // a genuine permanent terminus, not a mid-contour dip +} + // --- 9. A fresh spline EG opens on the smooth y = 1 - x ---------------------- static void testAFreshSplineEgDefaultsToTheSmoothDownwardSlope() { @@ -424,6 +443,27 @@ static void testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards() { CHECK(!deckKnobInert(DeckParam::kFilterModAmt, gates)); } +// enforceGateUnavailableWhileDrawn (play_params.h) is the ONE enforcement resolvePlay and the +// editor's applyControl both call — resolvePlay's own coverage above only exercises it through +// the frames mirror; pin it directly over BOTH representations it is shared between, closing the +// coverage gap the extraction was for (applyControl has no shell test target of its own). +static void testEnforceGateUnavailableWhileDrawnForcesTriggerOnBothRepresentations() { + PlaySeconds seconds; + seconds.playMode = PlayMode::Gate; + enforceGateUnavailableWhileDrawn(seconds); + CHECK(seconds.playMode == PlayMode::Gate); // not splineActive -> untouched + seconds.ampSpline.mode = EnvMode::Spline; + enforceGateUnavailableWhileDrawn(seconds); + CHECK(seconds.playMode == PlayMode::Trigger); + + PlayParams frames; + frames.playMode = PlayMode::Gate; + frames.filter.enabled = true; + frames.filterSpline.mode = EnvMode::Spline; + enforceGateUnavailableWhileDrawn(frames); + CHECK(frames.playMode == PlayMode::Trigger); +} + // --- 11. The velocity->amp curve is the same grammar ------------------------- static void testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete() { @@ -491,8 +531,10 @@ int main() { testAV12PayloadLoadsWithoutLoss(); testAContourReplaysProportionallyOnADifferentLengthSample(); testTwoPointContourRisingFromZeroSoundsForItsFullSpan(); + testFlatZeroFinalSegmentStillFreesTheVoiceEarly(); testAFreshSplineEgDefaultsToTheSmoothDownwardSlope(); testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards(); + testEnforceGateUnavailableWhileDrawnForcesTriggerOnBothRepresentations(); testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete(); testSplineCursorBinarySearchAgreesWithTheColdReaderOnAJumpingRead(); if (g_fail == 0) std::printf("spline_egs: all tests passed\n"); diff --git a/tests/test_waveform_view.cpp b/tests/test_waveform_view.cpp index ce1989d..bab64a8 100644 --- a/tests/test_waveform_view.cpp +++ b/tests/test_waveform_view.cpp @@ -13,6 +13,7 @@ // stacked height, grabs reaching the lower lane); laneEnvelope (per-lane channel split). #include "../src/core/instrument/ui/waveform_view.h" +#include "../src/core/instrument/engine/velocity_curve.h" // kCurveNodeGrabRadius, VelocityCurve::pointAtPixel #include "../src/core/instrument/ui/sample_bands.h" // kWaveformMinHeight, kLaneGap #include @@ -21,6 +22,7 @@ using namespace reasampler; using namespace reasampler::instrument::ui; +using namespace reasampler::instrument::engine; using reasampler::audio::AudioSample; static int g_fail = 0; @@ -379,10 +381,107 @@ static void testStartMarkerSharesTheHandleStripWhenItSitsAtTheFadeEdge() { // column to the start marker (index 0) at this x/y... CHECK(markerAtPoint(overlayOf(a), 1000, markers, 3, mx, topY) == 0); // ...and the fade handle's rect claims the exact same pixel — the ambiguity the shell - // resolves by asking the handle first, same as it does for the zero-fade/loop-start case. + // resolves by smallest-target-first (the handle's clipped tab is always the narrower + // target), same as it does for the zero-fade/loop-start case. CHECK(contains(markerHandleRect(overlayOf(a), 1000, fadeEdge), mx, topY)); } +// --- Smallest-target-first: the three-way coincidence with a spline contour node ------- +// +// editor_input_waveform.cpp's mouseDownWaveform resolves a click among a contour node (a fixed +// pick box), the crossfade tab, and a marker's full-height column by measuring each candidate's +// own target area and letting the smallest win — this module can't exercise the shell's +// arbitration itself (no shell test target wraps the editor), but it can pin the geometric facts +// that arbitration depends on, over the pure primitives it composes. A realistic band height +// (kWaveformMinHeight, the product's own floor) is used throughout so these numbers are the +// worst case for the node, not a favourable one. +static VelocityCurve::Box boxOf(const Rect& r) { return VelocityCurve::Box{r.x, r.y, r.width, r.height}; } + +// Case (a): a fresh Spline default (rampDown) puts its endpoint 0 at (box.left, box.top) — the +// exact pixel the start marker draws at frame 0. The node's fixed 169px pick box is far smaller +// than a kWaveformMinHeight-tall marker column, so the endpoint stays reachable. +static void testFreshRampDownEndpointBeatsTheStartMarkerAtFrameZero() { + const Rect a = Rect{20, 10, 1000, kWaveformMinHeight}; + const OverlayArea overlay = overlayOf(a); + const std::int64_t frames = 100000; + const VelocityCurve contour = VelocityCurve::rampDown(); + const VelocityCurve::Box box = boxOf(a); + + CHECK(contour.pointAtPixel(box, a.x, a.y) == 0); // endpoint 0 sits at (box.left, box.top) + const std::int64_t markers[1] = {0}; + CHECK(markerAtPoint(overlay, frames, markers, 1, a.x, a.y) == 0); // the coincidence + + constexpr std::int64_t nodeSide = 2 * kCurveNodeGrabRadius + 1; + constexpr std::int64_t nodeArea = nodeSide * nodeSide; // 169, fixed + const std::int64_t markerArea = + static_cast(2 * kMarkerGrabWidth + 1) * a.height; // 11 * band height + CHECK(nodeArea < markerArea); // the node wins: the endpoint stays a genuine grab target +} + +// Case (b): the crossfade tab at zero crossfade sits on loopStart's own pixel column; a node +// dragged to value ~0.98 lands a couple of rows below the box top — inside the tab's own +// top-strip band, where the review found the tab fully shadowed by a node-first pass. +static void testCrossfadeTabBeatsAContourNodeNearItsTopStrip() { + const Rect a = Rect{20, 10, 1000, kWaveformMinHeight}; + const OverlayArea overlay = overlayOf(a); + const std::int64_t frames = 100000; + const std::int64_t loopStart = 40000, crossfade = 0; // zero crossfade -> tab sits on loopStart + const int mx = frameToX(overlay, frames, loopStart - crossfade); + const Rect tabRect = markerHandleRect(overlay, frames, loopStart - crossfade); + CHECK(!tabRect.empty()); + + const VelocityCurve::Box box = boxOf(a); + const int ny = a.y + 3; // ~0.98 up a kWaveformMinHeight-tall box; inside the tab's top strip + VelocityCurve c = VelocityCurve::flat(); + const VelocityPoint p = c.pointFromPixel(box, mx, ny); + c.addPoint(p.velocity, p.value); + CHECK(c.pointAtPixel(box, mx, ny) >= 0); + CHECK(contains(tabRect, mx, ny)); // the coincidence: both claim the same pixel + + constexpr std::int64_t nodeSide = 2 * kCurveNodeGrabRadius + 1; + constexpr std::int64_t nodeArea = nodeSide * nodeSide; // 169, fixed + const std::int64_t tabArea = static_cast(tabRect.width) * tabRect.height; // <= 110 + CHECK(tabArea < nodeArea); // the tab wins: it stays the only affordance at zero crossfade + + // The residual the review names: the node keeps its OUTER columns, one pixel past the tab's + // clipped edge but still inside its own pick radius. + const int outerX = mx + kMarkerHandleHalfWidth + 1; + CHECK(!contains(tabRect, outerX, ny)); + CHECK(c.pointAtPixel(box, outerX, ny) >= 0); +} + +// Case (c): a contour node coincident with a loop marker. At kWaveformMinHeight (the product's +// own floor) the column is already an order of magnitude larger than the node's fixed pick box, +// so the node wins the shared pixel while the column stays reachable everywhere the node isn't. +static void testContourNodeBeatsALoopMarkerAtTheirSharedPixelButNotElsewhere() { + const Rect a = Rect{20, 10, 1000, kWaveformMinHeight}; + const OverlayArea overlay = overlayOf(a); + const std::int64_t frames = 100000; + const std::int64_t loopEnd = 70000; + const int mx = frameToX(overlay, frames, loopEnd); + const std::int64_t markers[1] = {loopEnd}; + + const VelocityCurve::Box box = boxOf(a); + const int ny = a.y + a.height / 2; // mid-height, well clear of any tab + VelocityCurve c = VelocityCurve::flat(); + const VelocityPoint p = c.pointFromPixel(box, mx, ny); + c.addPoint(p.velocity, p.value); + CHECK(c.pointAtPixel(box, mx, ny) >= 0); + CHECK(markerAtPoint(overlay, frames, markers, 1, mx, ny) == 0); // the coincidence + + constexpr std::int64_t nodeSide = 2 * kCurveNodeGrabRadius + 1; + constexpr std::int64_t nodeArea = nodeSide * nodeSide; // 169, fixed + const std::int64_t markerArea = + static_cast(2 * kMarkerGrabWidth + 1) * a.height; // 11 * band height + CHECK(nodeArea < markerArea); // the node wins the shared pixel + + // A few rows clear of the node (outside its 13px pick box, still on the marker's column) + // the marker alone claims the click. + const int farY = ny + kCurveNodeGrabRadius + 4; + CHECK(c.pointAtPixel(box, mx, farY) < 0); + CHECK(markerAtPoint(overlay, frames, markers, 1, mx, farY) == 0); +} + // --- Per-lane envelope content ------------------------------------------------- static void testAsymmetricStereoLanesCarryDifferentContent() { @@ -458,6 +557,9 @@ int main() { testMarkerHandleClipsIntoTheArea(); testMarkerHandleOnDegenerateAreas(); testStartMarkerSharesTheHandleStripWhenItSitsAtTheFadeEdge(); + testFreshRampDownEndpointBeatsTheStartMarkerAtFrameZero(); + testCrossfadeTabBeatsAContourNodeNearItsTopStrip(); + testContourNodeBeatsALoopMarkerAtTheirSharedPixelButNotElsewhere(); testAsymmetricStereoLanesCarryDifferentContent(); testLaneEnvelopeRejectsOutOfRangeLane();