From e714df80f86cd71c245aac64bc2df65bf0e859b2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 13:26:44 -0400 Subject: [PATCH] =?UTF-8?q?=CE=A9-W1-T5=20review=20fixes:=20unify=20the=20?= =?UTF-8?q?drag=20frame=20resolve=20onto=20frameToX/xToFrame;=20honest=20c?= =?UTF-8?q?omments=20on=20grabbableMarks=20and=20the=20grey-loop-mark=20co?= =?UTF-8?q?ntrast=20trade?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/core/instrument/ui/waveform_view.cpp | 15 ++++---- src/core/instrument/ui/waveform_view.h | 16 ++++++-- src/shell/instrument/editor_controls.cpp | 8 ++-- .../instrument/editor_paint_waveform.cpp | 8 ++-- tests/test_waveform_view.cpp | 38 +++++++++++++++++-- 5 files changed, 65 insertions(+), 20 deletions(-) diff --git a/src/core/instrument/ui/waveform_view.cpp b/src/core/instrument/ui/waveform_view.cpp index 83a533e..f026edf 100644 --- a/src/core/instrument/ui/waveform_view.cpp +++ b/src/core/instrument/ui/waveform_view.cpp @@ -4,7 +4,6 @@ #include #include -#include // std::abs (int overload) #include "core/instrument/ui/sample_bands.h" // waveformLanes (the band's lane inventory) #include "core/ui/component_geometry.h" // waveformColumnCount (THE draw chain's columns) @@ -170,13 +169,13 @@ std::int64_t resolveDragFrame(const OverlayArea& area, std::int64_t frameCount, if (dxPixels == 0) return start; const int w = std::max(0, area.rect.width); if (frameCount <= 0 || w <= 0) return start; // no room to move - // Proportional shift, rounded to the nearest frame (same linear map as frameToX/xToFrame). - const std::int64_t magnitude = - (static_cast(std::abs(dxPixels)) * frameCount + - static_cast(w) / 2) / - static_cast(w); - const std::int64_t shift = dxPixels > 0 ? magnitude : -magnitude; - return clampFrame(start + shift, frameCount); + // THE unified mapping, not a second one: find the start frame's own column (frameToX), + // walk it by dxPixels, and read the frame back off the resulting pixel (xToFrame) — so a + // drag always resolves to the column under the cursor, never a proportional approximation + // of it. A prior independent linear map here could disagree with frameToX/xToFrame's + // truncating column partition, most visibly when frames < columns. + const int grabX = frameToX(area, frameCount, start); + return xToFrame(area, frameCount, grabX + dxPixels); } std::int64_t nearestZeroCrossing(const AudioSample* pcm, std::int64_t frames, diff --git a/src/core/instrument/ui/waveform_view.h b/src/core/instrument/ui/waveform_view.h index e3e4f4c..5bb84fe 100644 --- a/src/core/instrument/ui/waveform_view.h +++ b/src/core/instrument/ui/waveform_view.h @@ -83,6 +83,14 @@ inline constexpr int kMarkerGrabWidth = 5; // for every f whenever frames <= columns, i.e. exactly where the choice of end is observable). // Above that, several frames share a column and the round trip snaps to the column's first // frame, which is the quantization the shared column already is. +// +// KNOWN CONVENTION MISMATCH, not a bug: in the frames < columns regime a mark lands on the +// LAST column of its frame's span (above), while the spline contour and the staged AHD +// polyline map normalized t in [0,1] straight across the same box and so land on a span's +// FIRST column instead. Both land inside the frame's own span, so the mapping contract holds, +// but the two conventions can disagree by up to a span's width for captures under ~1000 +// frames. Unifying them is a real option, left alone here since it touches the spline/AHD draw +// path rather than this pair. // x pixel of `frame`: the column that draws it. `frameCount` itself is a span's EXCLUSIVE end, // not a frame, and maps to area.right(). Frame is clamped to [0, frameCount] first; @@ -169,9 +177,11 @@ int crossfadeWedgeHeight(int x0, int x1, int x); int markerAtPoint(const OverlayArea& area, std::int64_t frameCount, const std::int64_t* frames, int count, int x, int y); -// Resolves a drag to a new frame: `startFrame` shifted by round(dxPixels * frameCount / -// areaWidth), clamped to [0, frameCount]. The shell applies between-marker clamps (e.g. -// start <= loopEnd) after this per-marker resolve. +// Resolves a drag to a new frame: locates `startFrame`'s own column via frameToX, walks it by +// dxPixels, and reads the frame back via xToFrame — the SAME partition, so the result is always +// the frame the cursor's column actually draws, never a proportional approximation of it. +// Clamped to [0, frameCount]. The shell applies between-marker clamps (e.g. start <= loopEnd) +// after this per-marker resolve. std::int64_t resolveDragFrame(const OverlayArea& area, std::int64_t frameCount, std::int64_t startFrame, int dxPixels); diff --git a/src/shell/instrument/editor_controls.cpp b/src/shell/instrument/editor_controls.cpp index ab484b6..8159a64 100644 --- a/src/shell/instrument/editor_controls.cpp +++ b/src/shell/instrument/editor_controls.cpp @@ -164,9 +164,11 @@ instrument::ui::WaveMarks ReaSamplerEditor::waveMarksFor(const SetupMarkers& m) } instrument::ui::WaveMarks ReaSamplerEditor::grabbableMarks(const SetupMarkers& m) const { - // Drawn IFF grabbable. Kept as its own fold because paint and hit-test stay separate - // questions, but do NOT re-add a suppression here: the Gate-with-loop-off marks are drawn - // grey precisely so they can still be dragged, and dragging one is what turns the enable on. + // Drawn IFF grabbable is the product rule, so this is an exact alias of waveMarksFor and + // cannot currently diverge from it. Kept as its own seam anyway because paint and hit-test + // are separate questions in principle — but do NOT re-add a suppression here: the + // Gate-with-loop-off marks are drawn grey precisely so they can still be dragged, and + // dragging one is what turns the enable on. return waveMarksFor(m); } diff --git a/src/shell/instrument/editor_paint_waveform.cpp b/src/shell/instrument/editor_paint_waveform.cpp index 3fbb2a5..a5ee6bf 100644 --- a/src/shell/instrument/editor_paint_waveform.cpp +++ b/src/shell/instrument/editor_paint_waveform.cpp @@ -46,9 +46,11 @@ constexpr Role kRoleStartMarker = Role::OverlayTrace; constexpr Role kRoleLoopMarker = Role::AccentSecondary; // A loop mark whose enable is off keeps its position and its cap — and its full weight, since // it is still draggable. It changes HUE, not opacity: the dim teal it replaces read as broken -// rather than as off. Grey against the lime is deliberately under the 3:1 state-indicator floor -// the two-neighbour rule (core/ui/CLAUDE.md) sets for a LIVE mark — an inactive control is -// exempt, and that lower contrast is the off cue. +// rather than as off. Grey against the lime deliberately sits under the 3:1 state-indicator +// floor the two-neighbour rule (core/ui/CLAUDE.md) sets — on a mark that stays LIVE and still +// drives the loop enable, so this is NOT the WCAG 1.4.11 inactive-component carve-out. It is a +// deliberate trade (the off cue reads as lower contrast on a control that can still be +// grabbed), pending Daniel's eye in the DAW. Do not change the colour to chase it. constexpr Role kRoleLoopMarkerOff = Role::TextDim; // Mark weights. The crossfade is a SOFT boundary and rides below the loop pair's weight at rest. diff --git a/tests/test_waveform_view.cpp b/tests/test_waveform_view.cpp index 6d1a9f3..c47970d 100644 --- a/tests/test_waveform_view.cpp +++ b/tests/test_waveform_view.cpp @@ -10,9 +10,10 @@ // waveformOverlayArea (the overlay IS the drawn column band, inset symmetrically); // markerAtPoint (grab band, first-match on overlap, off-area + null-array rejection); // markerHandleRect (the top-strip tab that keeps coincident markers independently grabbable); -// resolveDragFrame (round-to-nearest-frame, clamp to [0,frameCount], zero-delta/zero-width -// no-ops); nearestZeroCrossing (nearest sign-change, sample-on-zero, equidistant-tie-to-lower, -// no-crossing keeps target, target clamp, degenerate buffers); the four marks (per-mark cap +// resolveDragFrame (drag lands on the frameToX/xToFrame column under the cursor, clamp to +// [0,frameCount], zero-delta/zero-width no-ops); nearestZeroCrossing (nearest sign-change, +// sample-on-zero, equidistant-tie-to-lower, no-crossing keeps target, target clamp, degenerate +// buffers); the four marks (per-mark cap // resolve, the reverse cap order that keeps a coincident pair separable, label sides/nudging, // the suppression rule and its promoted-first placement, the crossfade wedge ramp); // waveformSurface (two stacked @@ -263,6 +264,36 @@ static void testResolveDragFrameDegenerate() { CHECK(resolveDragFrame(ov, 1000, 5000, 0) == 1000); } +// A drag must land on the column under the cursor — the SAME frameToX/xToFrame partition, never +// a proportional approximation of it. Pins the contract itself (grabX = frameToX(startFrame), +// result = xToFrame(grabX + dxPixels)) rather than a captured number, in the frames < columns +// regime where the two disagree: a prior independent linear map here left a marker at frame 10 +// (1000px/37 frames, start=10, +3px) when a fresh xToFrame(x) at the same cursor column +// resolves to frame 11 — exactly the class of drift a second frame<->pixel map produces. +static void testResolveDragFrameLandsOnCursorColumn() { + const OverlayArea ov = overlayOf(Rect::ltrb(0, 0, 1000, 60)); + const std::int64_t frameCount = 37; + const std::int64_t startFrame = 10; + const int dx = 3; + const int grabX = frameToX(ov, frameCount, startFrame); + const std::int64_t cursorFrame = xToFrame(ov, frameCount, grabX + dx); + CHECK(cursorFrame == 11); // the contract's own derivation + CHECK(resolveDragFrame(ov, frameCount, startFrame, dx) == cursorFrame); + // General form, swept across both framescolumns: the resolved frame's + // OWN column (frameToX) must contain the cursor pixel the drag actually landed on. + const std::int64_t counts[] = {5, 37, 251, 9973}; + const int deltas[] = {-97, -3, -1, 1, 3, 97}; + for (std::int64_t n : counts) { + for (std::int64_t start = 0; start < n; start += (std::max)(1, n / 11)) { + for (int d : deltas) { + const std::int64_t got = resolveDragFrame(ov, n, start, d); + const int wantGrabX = frameToX(ov, n, start); + CHECK(got == xToFrame(ov, n, wantGrabX + d)); + } + } + } +} + // --- nearestZeroCrossing ------------------------------------------------------ static void testZeroCrossingNearest() { @@ -786,6 +817,7 @@ int main() { testResolveDragFrameClamps(); testResolveDragFrameRounds(); testResolveDragFrameDegenerate(); + testResolveDragFrameLandsOnCursorColumn(); testZeroCrossingNearest(); testZeroCrossingSampleOnZero();