From 47f2a063e79d42631aa4398ae299e725c8218a62 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sat, 1 Aug 2026 10:01:40 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20close=20five=20=CE=98-W6-T1=20review=20m?= =?UTF-8?q?inors=20=E2=80=94=20headroom=20figure,=20knob-face=20radius,=20?= =?UTF-8?q?degenerate=20band=20clamp?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligns inKnobFace's hit radius with computeKnob's draw-side min(w,h) rule and adds a non-square-rect test; clamps halfSpan for degenerate waveform bands with a test; fixes stale docs/comments and annotates an uncommitted perf measurement. --- docs/product/visual-design-language.md | 2 +- src/core/instrument/CLAUDE.md | 2 +- src/core/instrument/map/sample_map.h | 4 ++-- src/core/instrument/ui/knob_deck.cpp | 4 +++- src/core/instrument/ui/knob_deck.h | 8 +++++--- src/core/ui/component_geometry.cpp | 8 +++++++- tests/test_component_geometry.cpp | 17 ++++++++++++++++- tests/test_knob_deck.cpp | 22 ++++++++++++++++++++++ 8 files changed, 57 insertions(+), 10 deletions(-) diff --git a/docs/product/visual-design-language.md b/docs/product/visual-design-language.md index 15a58ee..000aeea 100644 --- a/docs/product/visual-design-language.md +++ b/docs/product/visual-design-language.md @@ -793,7 +793,7 @@ concession. Everything with a slope or a curve must draw through a primitive tha | Spline (drawn EG) contour | `editor_paint_waveform.cpp` `paintSplineOverlay` | **Fixed** — same treatment, one trace grammar. | | Velocity-curve popup trace | `editor_paint_curve.cpp` | **Fixed** — same treatment. | | Velocity-curve mini thumbnail | `editor_paint_curve.cpp` | Left at 1 px AA `LICE_Line` — a 2 px trace blots at thumbnail scale. | -| Waveform min/max columns | `draw_kit.cpp` `drawWaveform` | **Fixed** — column fill unchanged (it cannot alias), plus an AA `LICE_FLine` stroke joining each column's extremes to its neighbour's, in the same ink. Shared with the docked bank panel and the browser cards. **Measured cost** (Release, MSVC 14.44, real LICE, 24 stereo cards × 136 columns = 6528 columns): fill alone 0.070 ms per full-grid repaint, fill+stroke 0.48 ms — the stroke is ~0.41 ms, about 2.5% of a 60 Hz frame, and the grid repaints on hover/scroll/drag, not continuously. | +| Waveform min/max columns | `draw_kit.cpp` `drawWaveform` | **Fixed** — column fill unchanged (it cannot alias), plus an AA `LICE_FLine` stroke joining each column's extremes to its neighbour's, in the same ink. Shared with the docked bank panel and the browser cards. **Measured cost** (Release, MSVC 14.44, real LICE, 24 stereo cards × 136 columns = 6528 columns): fill alone 0.070 ms per full-grid repaint, fill+stroke 0.48 ms — the stroke is ~0.41 ms, about 2.5% of a 60 Hz frame, and the grid repaints on hover/scroll/drag, not continuously. One-off scratchpad measurement, 2026-08-01, harness not committed — not a standing regression guard; re-measure before relying on it again. | | Preview play triangle | `editor_paint_chrome.cpp` | **Fixed** — `LICE_FillTriangle` has no `aa`; its two sloped edges are re-stroked with AA `LICE_FLine`. | | Envelope/spline node handles (squares) | `editor_paint_waveform.cpp` | Already clean — axis-aligned `LICE_FillRect`. | | Envelope curve knots (circles) | `editor_paint_waveform.cpp` | Already clean — `LICE_FillCircle` with `aa=true`. | diff --git a/src/core/instrument/CLAUDE.md b/src/core/instrument/CLAUDE.md index 9c16dc3..d7fc685 100644 --- a/src/core/instrument/CLAUDE.md +++ b/src/core/instrument/CLAUDE.md @@ -310,7 +310,7 @@ anything for a trigger shape. - `browser_scroll` — scroll + type-to-filter layered over `capture_browser`: vertical scroll offset, scrollbar thumb, thumb-drag mapping, and name-substring search. - `param_slider` — parameter control-panel: vertical stack of TOGGLE (two-segment selector) and SLIDER (horizontal track) rows; maps normalized value to/from handle pixel. - `embed_strip` — compact single-row control layout for embed mode in the track FX chain. -- `knob_deck` — pure knob-deck layout + hit-test (FB1): group-box / caption-row / compact-toggle / knob-cell geometry, deterministic whole-group wrap, `DeckLayout` / `DeckHit`. Mirror of `action_bar`/`param_slider`; no LICE or REAPER types. Carries a SECOND hit-test, `hitTestKnobFace`, resolved against the drawn CIRCLES rather than the cell: a double-click reset is aimed at a dial, so the label band and the cell margins must miss where a drag grab deliberately does not, and only a radial resolve can tell the inner curve dial from the outer ring it sits inside. **The cell/knob/label sizes and `sample_bands`' editor floor move as a pair** — wider cells need a wider floor width or the deck wraps to a fourth row. A group carries TWO caption-toggle slots, laid right-to-left: the second exists because a group whose knob row is wider than its caption row has caption slack a toggle can occupy for free, and the deck has six pixels of headroom on its first row at the editor's floor width — a `rowToggle` would widen the GROUP and wrap the deck to a fourth row, past what the minimum window holds. **A group's cell run is a RESERVED WIDTH, not a fixed cell size**: a `-1` id reserves one cell's width without a cell, and the cells present divide the whole run between them at one uniform integer width (residue in symmetric end margins). That is what lets a mode flip drop controls from a face — Trigger's AMP and FILTER ENV lose their Sustain/Release stages — without either reflowing the deck or leaving dead slots in the box; a face with fewer controls simply gets roomier cells. Do not reintroduce fixed-width cells with blank slots. +- `knob_deck` — pure knob-deck layout + hit-test (FB1): group-box / caption-row / compact-toggle / knob-cell geometry, deterministic whole-group wrap, `DeckLayout` / `DeckHit`. Mirror of `action_bar`/`param_slider`; no LICE or REAPER types. Carries a SECOND hit-test, `hitTestKnobFace`, resolved against the drawn CIRCLES rather than the cell: a double-click reset is aimed at a dial, so the label band and the cell margins must miss where a drag grab deliberately does not, and only a radial resolve can tell the inner curve dial from the outer ring it sits inside. **The cell/knob/label sizes and `sample_bands`' editor floor move as a pair** — wider cells need a wider floor width or the deck wraps to a fourth row. A group carries TWO caption-toggle slots, laid right-to-left: the second exists because a group whose knob row is wider than its caption row has caption slack a toggle can occupy for free, and the deck has fourteen pixels of headroom on its first row at the editor's floor width — a `rowToggle` would widen the GROUP and wrap the deck to a fourth row, past what the minimum window holds. **A group's cell run is a RESERVED WIDTH, not a fixed cell size**: a `-1` id reserves one cell's width without a cell, and the cells present divide the whole run between them at one uniform integer width (residue in symmetric end margins). That is what lets a mode flip drop controls from a face — Trigger's AMP and FILTER ENV lose their Sustain/Release stages — without either reflowing the deck or leaving dead slots in the box; a face with fewer controls simply gets roomier cells. Do not reintroduce fixed-width cells with blank slots. - `deck_values` — the deck's control-id ↔ parameter-set BINDING and its display units, split from the editor shell on the same axis `deck_groups` was split from `knob_deck`: `deck_groups` says which controls exist, this says what each one's value MEANS. Holds `deckParamNorm` / diff --git a/src/core/instrument/map/sample_map.h b/src/core/instrument/map/sample_map.h index 18eb7a0..57da0df 100644 --- a/src/core/instrument/map/sample_map.h +++ b/src/core/instrument/map/sample_map.h @@ -4,8 +4,8 @@ // The bank is read over the live-state seam, audio over the file seam; both raw inputs // cross the bridge/file boundary in the shell, everything after (bank parse via the shared // bank_book JSON path, sample pick, channel policy, SampleData build) is pure and -// unit-tested here. Links bank_book, wav_codec, and play_params (all pure) — deliberately -// NOT the voice engine: the build's product is plain SampleData. +// unit-tested here. Links bank_book, wav_codec, play_params, and play_seconds (all pure) — +// deliberately NOT the voice engine: the build's product is plain SampleData. #include #include diff --git a/src/core/instrument/ui/knob_deck.cpp b/src/core/instrument/ui/knob_deck.cpp index 89c9ca2..66480ea 100644 --- a/src/core/instrument/ui/knob_deck.cpp +++ b/src/core/instrument/ui/knob_deck.cpp @@ -193,7 +193,9 @@ DeckHit hitTestDeck(const DeckLayout& layout, int x, int y) { bool inKnobFace(const Rect& knob, int x, int y) { const double dx = x - (knob.x + knob.width / 2.0); const double dy = y - (knob.y + knob.height / 2.0); - const double r = knob.width / 2.0; + // Matches computeKnob's radius rule (param_slider.cpp) so the hit rule never claims more + // circle than is actually drawn when a caller's rect is non-square (e.g. a squashed chrome row). + const double r = (std::min)(knob.width, knob.height) / 2.0; return dx * dx + dy * dy < r * r; } diff --git a/src/core/instrument/ui/knob_deck.h b/src/core/instrument/ui/knob_deck.h index e618eee..f13b995 100644 --- a/src/core/instrument/ui/knob_deck.h +++ b/src/core/instrument/ui/knob_deck.h @@ -153,9 +153,11 @@ struct DeckFaceHit { bool inner = false; // inside the concentric inner disc }; -// A point inside the circle inscribed in `knob`, boundary-EXCLUSIVE. THE target rule for the -// reset gesture wherever a radial knob is drawn — the deck's own faces and the chrome's -// preview-velocity dial both resolve through it, so one gesture cannot grow two target rules. +// A point inside the circle inscribed in `knob` — radius is min(width, height)/2, same rule as +// computeKnob's draw-side circle, so a non-square rect can never claim a hit past the drawn disc +// — boundary-EXCLUSIVE. THE target rule for the reset gesture wherever a radial knob is drawn — +// the deck's own faces and the chrome's preview-velocity dial both resolve through it, so one +// gesture cannot grow two target rules. bool inKnobFace(const Rect& knob, int x, int y); // Resolved against the drawn CIRCLES, not the cell: a reset is aimed at a dial, so the label diff --git a/src/core/ui/component_geometry.cpp b/src/core/ui/component_geometry.cpp index 8a9a8a1..48f6173 100644 --- a/src/core/ui/component_geometry.cpp +++ b/src/core/ui/component_geometry.cpp @@ -102,7 +102,13 @@ WaveformBand waveformBand(int bandTop, int bandHeight) { b.top = bandTop; b.height = bandHeight; b.midY = bandTop + bandHeight / 2; - b.halfSpan = (bandHeight / 2) - 2; // matches drawWaveform's 2px vertical breathing room + // matches drawWaveform's 2px vertical breathing room; clamped at 0 so a degenerate band + // (height <= 3, where this would otherwise go negative) can't flip a positive peak below + // the zero line. + const int rawHalfSpan = (bandHeight / 2) - 2; + b.halfSpan = rawHalfSpan > 0 ? rawHalfSpan : 0; + // lo/hi (waveformColumnSpan) are midY-height/2 .. midY+height/2-1 — asymmetric by one px for + // even heights. Unreachable while |compressAmplitudeForDisplay| <= 1, so left as-is. return b; } diff --git a/tests/test_component_geometry.cpp b/tests/test_component_geometry.cpp index bb5e764..4a5a5b8 100644 --- a/tests/test_component_geometry.cpp +++ b/tests/test_component_geometry.cpp @@ -197,7 +197,9 @@ static void testWaveformColumnCount() { // The regression this exists to catch: rounding applied to the resulting y instead of to the // scaled amplitude draws a symmetric column one pixel taller above the zero line than below. static void testSymmetricColumnDrawsEqualHeightAboveAndBelowTheZeroLine() { - const WaveformBand band = waveformBand(0, 41); // odd height -> fractional half-span + // Odd height so midY sits equidistant from lo/hi — the fractional half-span below comes + // from the amplitude product, not the height's parity. + const WaveformBand band = waveformBand(0, 41); // Sweep amplitudes whose scaled value is fractional, which is where the two edges can // round in opposite directions. for (int i = 1; i <= 20; ++i) { @@ -256,6 +258,18 @@ static void testBandMetricsMirrorTheDrawnInset() { CHECK(b.halfSpan == 18.0); // half the band, less the 2px breathing room } +// A band too short for the 2px breathing room (height <= 4, so height/2 - 2 <= 0) must clamp +// halfSpan to 0 rather than go negative and mirror every positive column below the zero line. +static void testDegenerateBandClampsHalfSpanToZero() { + CHECK(waveformBand(0, 4).halfSpan == 0.0); + CHECK(waveformBand(0, 3).halfSpan == 0.0); + CHECK(waveformBand(0, 0).halfSpan == 0.0); + // A zero half-span collapses every column onto the zero line regardless of amplitude sign. + const WaveformBand b = waveformBand(10, 4); + const WaveformColumnSpan s = waveformColumnSpan(b, 1.0, -1.0); + CHECK(s.top == b.midY && s.bottom == b.midY); +} + int main() { testHitTestBoxHalfOpen(); testButtonBoxInset(); @@ -279,6 +293,7 @@ int main() { testTallerAmplitudeNeverDrawsAShorterColumn(); testBothEdgesAndTheStrokeClampToTheBand(); testBandMetricsMirrorTheDrawnInset(); + testDegenerateBandClampsHalfSpanToZero(); if (g_fail == 0) std::printf("component_geometry: all tests passed\n"); else std::printf("component_geometry: %d CHECK(s) FAILED\n", g_fail); diff --git a/tests/test_knob_deck.cpp b/tests/test_knob_deck.cpp index 0ce3f89..23fe0d4 100644 --- a/tests/test_knob_deck.cpp +++ b/tests/test_knob_deck.cpp @@ -382,6 +382,27 @@ static void testKnobFaceResolvesInnerRingOuterRingAndMisses() { CHECK(hitTestKnobFace(dl, cx + diag, cy + diag).id == -1); } +// A non-square knob rect (e.g. a squashed chrome row clamps knob height below its width) must +// resolve against min(width, height)/2 — the same radius computeKnob draws — never against +// width alone, or the hit disc would claim territory above/below where nothing is drawn. +static void testInKnobFaceUsesTheSmallerDimensionOnANonSquareRect() { + const Rect wide{0, 0, 40, 20}; // width > height: draws a 10px-radius disc, not 20px + const int cx = wide.x + wide.width / 2; + const int cy = wide.y + wide.height / 2; + CHECK(inKnobFace(wide, cx, cy)); // dead centre always hits + CHECK(inKnobFace(wide, cx, cy + 9)); // just inside the drawn (height-limited) radius + CHECK(!inKnobFace(wide, cx, cy + 10)); // on the drawn rim: exclusive miss + CHECK(!inKnobFace(wide, cx, cy + 15)); // inside the RECT but outside the smaller-radius disc + CHECK(!inKnobFace(wide, cx + 15, cy)); // same check along the wider axis + + const Rect tall{0, 0, 20, 40}; // height > width: draws a 10px-radius disc, not 20px + const int tcx = tall.x + tall.width / 2; + const int tcy = tall.y + tall.height / 2; + CHECK(inKnobFace(tall, tcx, tcy)); + CHECK(!inKnobFace(tall, tcx + 15, tcy)); + CHECK(!inKnobFace(tall, tcx, tcy + 15)); +} + static void testEmptyDeck() { const std::vector none; CHECK(deckRowCount(none, 800) == 0); @@ -401,6 +422,7 @@ int main() { testCaptionRadioGeometryAndHit(); testInnerDialHit(); testKnobFaceResolvesInnerRingOuterRingAndMisses(); + testInKnobFaceUsesTheSmallerDimensionOnANonSquareRect(); testCaptionToggle2(); testEmptyDeck(); if (g_fail) {