fix: close five Θ-W6-T1 review minors — headroom figure, knob-face radius, degenerate band clamp

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.
This commit is contained in:
2026-08-01 10:01:40 -04:00
parent ca464397b2
commit 47f2a063e7
8 changed files with 57 additions and 10 deletions
+1 -1
View File
@@ -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`. |
+1 -1
View File
@@ -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` /
+2 -2
View File
@@ -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 <cstdint>
#include <optional>
+3 -1
View File
@@ -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;
}
+5 -3
View File
@@ -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
+7 -1
View File
@@ -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;
}
+16 -1
View File
@@ -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);
+22
View File
@@ -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<DeckGroupDesc> none;
CHECK(deckRowCount(none, 800) == 0);
@@ -401,6 +422,7 @@ int main() {
testCaptionRadioGeometryAndHit();
testInnerDialHit();
testKnobFaceResolvesInnerRingOuterRingAndMisses();
testInKnobFaceUsesTheSmallerDimensionOnANonSquareRect();
testCaptionToggle2();
testEmptyDeck();
if (g_fail) {