From 3fb77027c66b56091d45d828a8c5a276c4a6c9c1 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sat, 1 Aug 2026 13:45:43 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20close=20=CE=98-W7-T1=20review=20?= =?UTF-8?q?=E2=80=94=20scaling=20guard,=20opacity=20claims,=20two=20vacuou?= =?UTF-8?q?s=20test=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guards the stroke blend against LICE_EXT_GET_SCALING, tightens the analytic-stroker's boxes and NaN handling, corrects the opaque-core threshold and inner-dial rationale in the docs, and re-derives two review-flagged tautological tests so they actually fail against the bugs they claim to catch. --- docs/product/visual-design-language.md | 11 ++-- src/core/ui/CLAUDE.md | 2 +- src/core/ui/stroke_aa.cpp | 35 +++++++++--- src/core/ui/stroke_aa.h | 10 ++++ src/shell/instrument/editor_internal.h | 6 +- src/shell/instrument/editor_paint_curve.cpp | 4 +- .../instrument/editor_paint_waveform.cpp | 13 +++-- src/shell/instrument/editor_stroke.cpp | 41 +++++++++++++- src/shell/instrument/editor_stroke.h | 9 +++ tests/test_stroke_aa.cpp | 56 +++++++++++++++++-- tests/test_velocity_curve.cpp | 24 ++++++++ 11 files changed, 181 insertions(+), 30 deletions(-) diff --git a/docs/product/visual-design-language.md b/docs/product/visual-design-language.md index 74b117d..afa4691 100644 --- a/docs/product/visual-design-language.md +++ b/docs/product/visual-design-language.md @@ -808,9 +808,11 @@ concession. Everything with a slope or a curve must draw through a primitive tha | Surface | Where | Disposition | |---|---|---| -| Radial knob track + value arc | `editor_internal.h` `drawKnobFace` | **Fixed (2026-08-01)** — the stacked-radius `LICE_Arc` ring never reached an opaque core and rippled 67% in weight. Now ONE analytic stroke (`strokeArcAA`), outer edge on the knob radius. Measured: peak **255/255** at every cross-section, weight **2.95–3.11 px** (5% ripple). | +| Radial knob track arc | `editor_internal.h` `drawKnobFace` | **Fixed (2026-08-01)** — the stacked-radius `LICE_Arc` ring never reached an opaque core. Now ONE analytic stroke (`strokeArcAA`) at `kKnobTrackArcPx` = 1 px, **below the ≥2 px opaque-core threshold** (`core/ui/CLAUDE.md`) — peak alpha still modulates with the arc's exact pixel-grid alignment (measured ~128–255/255 across the sweep) rather than pinning to 255. An improvement over the old stacked-radius ripple, not full opacity; see `test_stroke_aa.cpp`'s 1 px case for the pinned behaviour. | +| Radial knob value arc | `editor_internal.h` `drawKnobFace` | **Fixed (2026-08-01)** — same stroke, `kKnobValueArcPx` = 3 px, clear of the opaque-core threshold. Measured: peak **255/255** at every cross-section, weight **2.95–3.11 px** (5% ripple). | | Knob needle | `drawKnobFace` | **Fixed (2026-08-01)** — `LICE_ThickFLine`'s minor-axis width thinned it to `cos θ` as the knob swept. Now `strokeLineAA`, 2 px. | -| Inner curve dial arc + needle | `drawInnerDial` | **Fixed (2026-08-01)** — same as the knob: one analytic 2 px arc; needle via `strokeLineAA`. | +| Inner curve dial arc | `drawInnerDial` | **Fixed (2026-08-01)** — the arc shared the knob track/value arc's stacked-radius opacity defect. Same one analytic fix, at `kInnerDialArcPx` = 2 px (at the opaque-core threshold). | +| Inner curve dial needle | `drawInnerDial` | **Converted (2026-08-01), not a defect fix** — this needle was already `LICE_FLine` (float endpoints, always AA), not `LICE_ThickFLine`; a 1 px AA line has no width to lay along a minor axis, so it never had the knob needle's `cos θ` defect. Moved to `strokeLineAA` anyway for one-seam consistency with every other stroke on the editor, not because it was broken. | | Staged envelope segment slopes | `editor_paint_waveform.cpp` | **Fixed (2026-08-01)** — one `strokePolylineAA` over the whole polyline, so the stage joints blend once. Vertices stay INTEGER by design: they are the positions the draggable handles are drawn at. | | Spline (drawn EG) contour | `editor_paint_waveform.cpp` `paintSplineOverlay` | **Fixed (2026-08-01)** — the trace was never gapped; it was fully aliased (every pixel full or empty) because the loop passed INTEGER `cy`, quantizing the slope into alternating 1/2 px steps. Now sub-pixel y (`subpixelFromPoint`) through `strokePolylineAA`. Measured: peak **255/255**, weight **1.95–2.01 px** (3% ripple). | | Velocity-curve popup trace | `editor_paint_curve.cpp` | **Fixed (2026-08-01)** — same cause, same treatment. | @@ -832,9 +834,8 @@ concession. Everything with a slope or a curve must draw through a primitive tha **Analytic stroker cost** (Release, MSVC, real LICE, one-off scratchpad harness 2026-08-01, not committed — re-measure before relying on it): 30 knob arcs **0.113 ms → 0.169 ms**; a 500 px spline contour **0.013 ms → 0.047 ms**. About +0.09 ms per full editor repaint, on a -surface that repaints on interaction rather than continuously. Both figures beat the -prototype's targets (0.285 ms / 0.106 ms). Micro-optimisation, each lever measured in -isolation: writing the blend straight to the bitmap's bits rather than through +surface that repaints on interaction rather than continuously. Micro-optimisation, each lever +measured in isolation: writing the blend straight to the bitmap's bits rather than through `LICE_PutPixel` is the big one (arcs 0.169 vs 0.253 ms); reusing the scratch mask across calls matters on the contour's large bounding box (0.047 vs 0.073 ms); `float` over `double` is small but real (contour coverage 0.045 vs 0.051 ms). The per-row valid-extent bookkeeping diff --git a/src/core/ui/CLAUDE.md b/src/core/ui/CLAUDE.md index 0f334da..6e110d7 100644 --- a/src/core/ui/CLAUDE.md +++ b/src/core/ui/CLAUDE.md @@ -101,7 +101,7 @@ L7 sub-pass, 2026-07-27): - `tooltip` — pure tooltip placement + prefix-strip: strips the `ReaSampler:` display prefix from the registered action phrase; width clamped to the client rect. - `card_drag` — pure drag-gesture precedence + slot hit-test: leave-client → OS drag-out; other-bank → move/copy; same-bank → reorder / Alt-over-occupied → replace. - `card_meta` — pure card-metadata formatters: bars.beats.subdivisions and seconds.milliseconds; blank when the sample is unstamped. -- `stroke_aa` — analytic antialiased thick-stroke COVERAGE (the shell blends it): `StrokeCanvas`, a reusable mask holding distance-to-polyline coverage MAX-accumulated across segments, plus `strokePolyline` / `strokeBounds` / `appendArc`. An arc is just a flattened polyline, so ONE path serves the knob arcs, the inner dial, the envelope polyline and both spline traces. Coverage is `clamp(halfWidth + 0.5 - distance, 0, 1)`, which makes perpendicular weight exactly `2·halfWidth` at every angle and gives an opaque core for any width above 1 px. Long segments are subdivided before rasterizing — EXACT, not an approximation (min-distance to a partition of a segment is min-distance to the whole), purely to keep each piece's bounding box tight, since one long diagonal's box has area O(len²). +- `stroke_aa` — analytic antialiased thick-stroke COVERAGE (the shell blends it): `StrokeCanvas`, a reusable mask holding distance-to-polyline coverage MAX-accumulated across segments, plus `strokePolyline` / `strokeBounds` / `appendArc`. An arc is just a flattened polyline, so ONE path serves the knob arcs, the inner dial, the envelope polyline and both spline traces. Coverage is `clamp(halfWidth + 0.5 - distance, 0, 1)`, which makes perpendicular weight exactly `2·halfWidth` at every angle. **The guaranteed-opaque-core threshold is width ≥ 2 px, not any width above 1 px**: opacity needs `distance <= halfWidth - 0.5`, and the worst-case distance from a pixel centre to the centreline is 0.5, so a 1 px stroke (`halfWidth = 0.5`) has zero slack — its peak alpha modulates with the stroke's exact alignment to the pixel grid instead of pinning to 255 (the knob track arc and the mini curve-thumbnail trace are both 1 px and both live with this). Long segments are subdivided before rasterizing — EXACT, not an approximation (min-distance to a partition of a segment is min-distance to the whole), purely to keep each piece's bounding box tight, since one long diagonal's box has area O(len²). ## Gotchas diff --git a/src/core/ui/stroke_aa.cpp b/src/core/ui/stroke_aa.cpp index 2d65506..f8fc8f0 100644 --- a/src/core/ui/stroke_aa.cpp +++ b/src/core/ui/stroke_aa.cpp @@ -56,16 +56,26 @@ void StrokeCanvas::extendRow(int y, int x0, int x1) { } void StrokeCanvas::addPiece(float ax, float ay, float bx, float by, float halfWidth) { + // Every current caller feeds bounded geometry, but this is a pure module: a NaN/Inf + // coordinate would otherwise reach static_cast below, which is UB rather than a + // clipped no-op. + if (!(std::isfinite(ax) && std::isfinite(ay) && std::isfinite(bx) && std::isfinite(by) && + std::isfinite(halfWidth))) { + return; + } const float reach = halfWidth + 0.5f; // beyond this the coverage is 0 const float dx = bx - ax; const float dy = by - ay; const float len2 = dx * dx + dy * dy; const float invLen2 = len2 > 0.0f ? 1.0f / len2 : 0.0f; - int x0 = static_cast(std::floor((std::min)(ax, bx) - reach)); - int x1 = static_cast(std::ceil((std::max)(ax, bx) + reach)) + 1; - int y0 = static_cast(std::floor((std::min)(ay, by) - reach)); - int y1 = static_cast(std::ceil((std::max)(ay, by) + reach)) + 1; + // A pixel can only take ink when its centre (x+0.5) is within `reach` of the piece, i.e. + // x + 0.5 < maxX + reach — so the exclusive upper bound is floor(maxX + reach + 0.5), not + // ceil(maxX + reach) + 1 (a whole extra pixel of guaranteed-zero coverage on every side). + int x0 = static_cast(std::ceil((std::min)(ax, bx) - reach - 0.5f)); + int x1 = static_cast(std::floor((std::max)(ax, bx) + reach + 0.5f)); + int y0 = static_cast(std::ceil((std::min)(ay, by) - reach - 0.5f)); + int y1 = static_cast(std::floor((std::max)(ay, by) + reach + 0.5f)); x0 = (std::max)(x0, bounds_.x); y0 = (std::max)(y0, bounds_.y); x1 = (std::min)(x1, bounds_.right()); @@ -123,11 +133,20 @@ Rect strokeBounds(const StrokePoint* pts, std::size_t count, float halfWidth, co minY = (std::min)(minY, pts[i].y); maxY = (std::max)(maxY, pts[i].y); } + // Same NaN/Inf guard as addPiece: an unbounded coordinate must clip to nothing, not reach + // the static_cast below as UB. + if (!(std::isfinite(minX) && std::isfinite(maxX) && std::isfinite(minY) && + std::isfinite(maxY) && std::isfinite(halfWidth))) { + return Rect{}; + } const float reach = halfWidth + 0.5f; - const int x0 = (std::max)(clip.x, static_cast(std::floor(minX - reach))); - const int y0 = (std::max)(clip.y, static_cast(std::floor(minY - reach))); - const int x1 = (std::min)(clip.right(), static_cast(std::ceil(maxX + reach)) + 1); - const int y1 = (std::min)(clip.bottom(), static_cast(std::ceil(maxY + reach)) + 1); + // Same tightened box as addPiece (see its comment): a pixel only takes ink when its centre + // is within `reach`, so this is [ceil(min-reach-0.5), floor(max+reach+0.5)) rather than the + // old ceil/floor pair that padded a whole extra pixel on every side. + const int x0 = (std::max)(clip.x, static_cast(std::ceil(minX - reach - 0.5f))); + const int y0 = (std::max)(clip.y, static_cast(std::ceil(minY - reach - 0.5f))); + const int x1 = (std::min)(clip.right(), static_cast(std::floor(maxX + reach + 0.5f))); + const int y1 = (std::min)(clip.bottom(), static_cast(std::floor(maxY + reach + 0.5f))); if (x0 >= x1 || y0 >= y1) return Rect{}; return Rect::ltrb(x0, y0, x1, y1); } diff --git a/src/core/ui/stroke_aa.h b/src/core/ui/stroke_aa.h index a9d1b6d..d93b09e 100644 --- a/src/core/ui/stroke_aa.h +++ b/src/core/ui/stroke_aa.h @@ -84,4 +84,14 @@ void strokePolyline(StrokeCanvas& canvas, const StrokePoint* pts, std::size_t co void appendArc(std::vector& out, float cx, float cy, float radius, float startRad, float endRad, float flatnessPx = kArcFlatnessPx); +// The row-major element offset of row `y` within a `rowSpan`-elements-per-row pixel buffer, +// accounting for a possibly bottom-up (`flipped`) layout. Pulled out of the shell's LICE blend +// so its flipped branch — dead for every bitmap type the shell actually constructs, and +// otherwise unverifiable without a live LICE surface — is pinned by a host-free test. Matches +// LICE's own row math (`lice.cpp`'s `LICE_SysBitmap` pixel accessor: `(h-1-y)*rowspan + x`). +inline std::size_t rasterRowOffset(int y, int height, int rowSpan, bool flipped) { + const int row = flipped ? height - 1 - y : y; + return static_cast(row) * static_cast(rowSpan); +} + } // namespace reasampler::ui diff --git a/src/shell/instrument/editor_internal.h b/src/shell/instrument/editor_internal.h index 561b14a..da82131 100644 --- a/src/shell/instrument/editor_internal.h +++ b/src/shell/instrument/editor_internal.h @@ -117,7 +117,7 @@ inline void drawTitleBand(LICE_IBitmap* bmp, const instrument::ui::Rect& title, inline constexpr float kKnobTrackArcPx = 1.0f; inline constexpr float kKnobValueArcPx = 3.0f; inline constexpr float kInnerDialArcPx = 2.0f; -inline constexpr int kKnobNeedlePx = 2; +inline constexpr float kKnobNeedlePx = 2.0f; // Draws one radial knob face: param_slider owns the value<->angle map; this turns it into // LICE calls. LICE takes radians, and drawing the 7->5 o'clock sweep through the top needs @@ -162,8 +162,8 @@ inline void drawKnobFace(LICE_IBitmap* bmp, const instrument::ui::Rect& knobRect const double iy = kg.centerY + (tip.y - kg.centerY) * 0.35; const ui::Role needleRole = disabled ? ui::Role::TextDim : ui::Role::TextPrimary; strokeLineAA(bmp, static_cast(ix), static_cast(iy), - static_cast(tip.x), static_cast(tip.y), - static_cast(kKnobNeedlePx), toLice(ui::roleColor(needleRole))); + static_cast(tip.x), static_cast(tip.y), kKnobNeedlePx, + toLice(ui::roleColor(needleRole))); } // The concentric INNER dial: a second value on the same cell, drawn in the categorical diff --git a/src/shell/instrument/editor_paint_curve.cpp b/src/shell/instrument/editor_paint_curve.cpp index 8f01b4b..b1ac9ab 100644 --- a/src/shell/instrument/editor_paint_curve.cpp +++ b/src/shell/instrument/editor_paint_curve.cpp @@ -63,7 +63,7 @@ void ReaSamplerEditor::paintCurveButton(LICE_IBitmap* bmp, const Rect& r, CurveT } const LICE_pixel trace = toLice(roleColor(disabled ? Role::LineHairline : Role::AccentSecondary)); - static thread_local std::vector pts; + std::vector& pts = scratchPoints(); pts.clear(); for (int px = 0; px <= mini.width; ++px) { const int mx = mini.left + px; @@ -123,7 +123,7 @@ void ReaSamplerEditor::paintVelocityCurve(LICE_IBitmap* bmp, const Rect& r) { // handles, and the hit-test all share one coordinate system (sub-pixel for the trace — see // subpixelFromPoint). const LICE_pixel line = toLice(roleColor(Role::AccentSecondary)); - static thread_local std::vector pts; + std::vector& pts = scratchPoints(); pts.clear(); for (int px = 0; px <= box.width; ++px) { const int cx = box.left + px; diff --git a/src/shell/instrument/editor_paint_waveform.cpp b/src/shell/instrument/editor_paint_waveform.cpp index 1338f6e..17d5d5c 100644 --- a/src/shell/instrument/editor_paint_waveform.cpp +++ b/src/shell/instrument/editor_paint_waveform.cpp @@ -145,11 +145,13 @@ void ReaSamplerEditor::paintSplineOverlay(LICE_IBitmap* bmp, const OverlayArea& // share the coordinate system the hit-test resolves against (sub-pixel here — see // subpixelFromPoint). const LICE_pixel line = toLice(roleColor(Role::OverlayTrace)); - static thread_local std::vector trace; + std::vector& trace = scratchPoints(); trace.clear(); // < not <=: box.left + box.width is the overlay's own EXCLUSIVE right edge (the box has no - // inset, unlike the popup's), so a <= column paints one pixel into the next band's pad — - // and it is redundant with the clamped endpoint handle below anyway. + // inset, unlike the popup's), so a <= column would re-trace a duplicate vertex one pixel + // past it. This bound does NOT contain the stroke to the box either way — the round cap on + // the last vertex extends halfWidth + 0.5 px past it regardless, same as the top/bottom + // edges the loop never clips against. for (int px = 0; px < box.width; ++px) { const int cx = box.left + px; const double t = curve.pointFromPixel(box, cx, box.top).velocity; @@ -214,13 +216,16 @@ void ReaSamplerEditor::paintEnvelopeOverlay(LICE_IBitmap* bmp, const OverlayArea // — unlike the spline traces above — because they are the same positions the draggable // handles are drawn at, and a sub-pixel trace would sit off its own handles. const LICE_pixel line = toLice(roleColor(Role::OverlayTrace)); - static thread_local std::vector trace; + std::vector& trace = scratchPoints(); trace.clear(); for (const EnvVertex& v : poly) { if (v.knot) continue; const int vx = (std::max)(area.x, (std::min)(area.right() - 1, v.x)); trace.push_back(ui::StrokePoint{static_cast(vx), static_cast(v.y)}); } + // A degenerate envelope (every stage collapsed to zero span) can reduce this to ONE vertex. + // strokePolylineAA's round-cap zero-length case then draws a dot at it, marking the sole + // point rather than drawing nothing — kept deliberately as more legible than a blank trace. strokePolylineAA(bmp, trace, kEnvTracePx, line); // Handles: a square per draggable stage node, a ROUND knot per curvable segment. Every // vertex is guaranteed in-bounds; the handle is additionally clamped inside the band so one diff --git a/src/shell/instrument/editor_stroke.cpp b/src/shell/instrument/editor_stroke.cpp index 7a50ce1..e2fe007 100644 --- a/src/shell/instrument/editor_stroke.cpp +++ b/src/shell/instrument/editor_stroke.cpp @@ -13,20 +13,54 @@ namespace { // the mask and the point list are resized, never reallocated. thread_local ui::StrokeCanvas g_canvas; thread_local std::vector g_arcPoints; +thread_local std::vector g_scratchPoints; + +// Per-pixel fallback through LICE_PutPixel, which itself reads LICE_EXT_GET_SCALING and scales +// its target coordinate (lice.cpp's LICE_PutPixel) — unlike the raw-bits path below, this is +// correct under a scaled bitmap. Only taken when scaling is active (see blendCanvas), so it +// costs nothing at the common unscaled call site. +void blendScaledFallback(LICE_IBitmap* bmp, const ui::StrokeCanvas& canvas, LICE_pixel color, + float alpha) { + const ui::Rect& b = canvas.bounds(); + for (int y = b.y; y < b.bottom(); ++y) { + const int lo = canvas.rowLo(y); + const int hi = canvas.rowHi(y); + if (hi <= lo) continue; + const float* const cov = canvas.rowData(y); + for (int i = lo; i < hi; ++i) { + const float a = cov[i] * alpha; + if (a <= 0.0f) continue; + LICE_PutPixel(bmp, b.x + i, y, color, a, LICE_BLIT_MODE_COPY); + } + } +} // One blend of the finished mask. The arithmetic matches LICE's own mode-0 combine // (src + (dst-src)*(256-a)/256 on all four channels, alpha in .8 fixed point) so a stroke // composites identically to every other kit draw on the same surface — written straight to the // bitmap's bits rather than through LICE_PutPixel, which re-derives the row pointer per pixel. +// +// That raw write assumes getWidth()/getHeight() (LOGICAL) and getRowSpan() (the DIB's PHYSICAL +// stride) agree — true only when unscaled. LICE_Arc/LICE_Line read LICE_EXT_GET_SCALING and +// scale their coordinates before touching the DIB (lice_arc.cpp:543, lice_line.cpp:1932); +// LICE_SysBitmap::__resize keeps m_width logical while sizing the DIB by +// (w*m_draw_scaling)>>8 (lice.cpp:165-173). Under a scale this loop's geometry and its target +// stride would disagree — a scale >256 lands the stroke in the wrong quadrant, a scale <256 +// runs the write past the DIB allocation. Nothing calls SET_SCALING today, but the guard has to +// stay ahead of the day something does. void blendCanvas(LICE_IBitmap* bmp, const ui::StrokeCanvas& canvas, LICE_pixel color, float alpha) { const ui::Rect& b = canvas.bounds(); if (b.empty() || alpha <= 0.0f) return; + if (bmp->Extended(LICE_EXT_GET_SCALING, nullptr) != 0) { + blendScaledFallback(bmp, canvas, color, alpha); + return; + } LICE_pixel* const bits = bmp->getBits(); const int span = bmp->getRowSpan(); if (bits == nullptr || span <= 0) return; const bool flipped = bmp->isFlipped(); - const int lastRow = bmp->getHeight() - 1; + const int height = bmp->getHeight(); const int sr = LICE_GETR(color); const int sg = LICE_GETG(color); @@ -38,8 +72,7 @@ void blendCanvas(LICE_IBitmap* bmp, const ui::StrokeCanvas& canvas, LICE_pixel c if (hi <= lo) continue; const float* const cov = canvas.rowData(y); LICE_pixel* const row = - bits + static_cast(flipped ? lastRow - y : y) * - static_cast(span) + static_cast(b.x); + bits + ui::rasterRowOffset(y, height, span, flipped) + static_cast(b.x); for (int i = lo; i < hi; ++i) { const int ia = static_cast(cov[i] * alpha * 256.0f); if (ia <= 0) continue; @@ -78,6 +111,8 @@ void strokeArcAA(LICE_IBitmap* bmp, float cx, float cy, float radius, float star strokePolylineAA(bmp, g_arcPoints.data(), g_arcPoints.size(), widthPx, color, alpha); } +std::vector& scratchPoints() { return g_scratchPoints; } + } // namespace reasampler::vst #endif // _WIN32 diff --git a/src/shell/instrument/editor_stroke.h b/src/shell/instrument/editor_stroke.h index 338dfaa..6b4da9c 100644 --- a/src/shell/instrument/editor_stroke.h +++ b/src/shell/instrument/editor_stroke.h @@ -45,6 +45,15 @@ inline void strokeLineAA(LICE_IBitmap* bmp, float x0, float y0, float x1, float void strokeArcAA(LICE_IBitmap* bmp, float cx, float cy, float radius, float startRad, float endRad, float widthPx, LICE_pixel color, float alpha = 1.0f); +// The draw-thread-only point-list scratch, shared by every paint site that builds a polyline +// column-by-column before one strokePolylineAA call (the curve thumbnail, the curve-popup +// trace, the two waveform-overlay traces). Reuse only: `clear()` and refill before each use, +// never read across paint calls. This module already owns the draw-thread scratch (the mask + +// the arc point list this header's own functions use internally); routing every external +// point-list consumer through the same accessor keeps that ownership one fact in one place +// instead of four independent function-local statics. +std::vector& scratchPoints(); + } // namespace reasampler::vst #endif // _WIN32 diff --git a/tests/test_stroke_aa.cpp b/tests/test_stroke_aa.cpp index a4d7609..42d4d23 100644 --- a/tests/test_stroke_aa.cpp +++ b/tests/test_stroke_aa.cpp @@ -73,6 +73,8 @@ static float perpendicularWeight(float angleDeg, float halfWidth) { static void testStraightStrokeHasAnOpaqueCore() { // The shipped defect stated numerically: peak alpha must reach full, not 137-192/255. + // 2 and 3 px only: below the >= 2 px opaque-core threshold (core/ui/CLAUDE.md), a stroke + // does NOT reliably reach full alpha — see testSubOpaqueCoreAtOnePixelWidth below. for (float w : {2.0f, 3.0f}) { for (float deg : {0.0f, 17.0f, 45.0f, 63.0f, 90.0f}) { const float a = deg * kPi / 180.0f; @@ -85,6 +87,25 @@ static void testStraightStrokeHasAnOpaqueCore() { } } +static void testSubOpaqueCoreAtOnePixelWidth() { + // Below the >= 2 px opaque-core threshold: a 1 px stroke (halfWidth = 0.5) has zero slack + // against the 0.5 px worst-case pixel-centre distance (core/ui/CLAUDE.md), so peak alpha + // tracks the stroke's alignment to the pixel grid instead of reaching 255 everywhere. Pin + // both ends of that modulation — this is the knob track arc's and the mini curve-trace's + // actual behaviour, not a hypothetical. + const StrokePoint onRowCentre[2] = {{20.0f, 100.5f}, {220.0f, 100.5f}}; // centred on row 100 + StrokeCanvas aligned; + strokePolyline(aligned, onRowCentre, 2, 0.5f, kBig); + CHECK(aligned.coverageAt(120, 100) >= 0.999f); // aligned to the grid: reaches opaque + + const StrokePoint onRowBoundary[2] = {{20.0f, 100.0f}, {220.0f, 100.0f}}; // on the boundary + StrokeCanvas misaligned; + strokePolyline(misaligned, onRowBoundary, 2, 0.5f, kBig); + CHECK(std::fabs(misaligned.coverageAt(120, 99) - 0.5f) < 1e-4f); // split evenly... + CHECK(std::fabs(misaligned.coverageAt(120, 100) - 0.5f) < 1e-4f); // ...across both rows + CHECK(peakCoverage(misaligned) < 0.999f); // and never reaches the opaque core here +} + static void testPerpendicularWeightIsAngleIndependent() { // The criterion that killed the ThickFLine option: it dips to wid*cos(theta) at every 45 // degrees. An axis-aligned-only sample would pass against it, so sample the diagonals. @@ -214,12 +235,23 @@ static void testRevisitedRowGapReadsZeroNotGarbage() { // A circle touches most rows on BOTH sides, leaving an untouched gap between the two spans. // The row's valid extent grows over that gap, so the gap must be zero-filled, not left at // whatever the reused scratch buffer held. - StrokeCanvas c; - c.reset(kBig); - c.addSegment(30.0f, 100.0f, 30.0f, 140.0f, 1.5f); // dirty the buffer on the left - c.reset(kBig); + // + // The dirtying pass must land at the SAME bounds/stride the arc pass will reuse, or the two + // writes address disjoint buffer offsets and the "old" value the gap reads back is just the + // scratch buffer's original zero-init — the guard would then have nothing to prove itself + // against (verified: deleting extendRow's fill at stroke_aa.cpp:48-51 left this test passing + // when the dirtying pass used `kBig` while the arc pass reset to its own tighter bounds). std::vector ring; appendArc(ring, 120.0f, 120.0f, 60.0f, 0.0f, 2.0f * kPi); + const Rect arcBounds = strokeBounds(ring.data(), ring.size(), 1.5f, kBig); + + StrokeCanvas c; + c.reset(arcBounds); + // A horizontal segment straight across the row/columns the assertion below checks, so the + // buffer genuinely holds nonzero ink there before the arc's own pass reuses the canvas. + c.addSegment(100.0f, 120.0f, 140.0f, 120.0f, 1.5f); + CHECK(c.coverageAt(120, 120) > 0.9f); // sanity: the dirtying pass actually landed here + strokePolyline(c, ring.data(), ring.size(), 1.5f, kBig); for (int x = 100; x < 140; ++x) CHECK(c.coverageAt(x, 120) == 0.0f); // hollow middle } @@ -257,6 +289,20 @@ static void testCoverageOutsideTheValidSpanReadsZero() { CHECK(c.coverageAt(1000, 1000) == 0.0f); } +// --- raster row addressing ---------------------------------------------------- + +static void testRasterRowOffsetMatchesUnflippedAndFlippedLayouts() { + // Unflipped: row y is just y*rowSpan (LICE's top-down layout). + CHECK(rasterRowOffset(0, 100, 240, false) == 0u); + CHECK(rasterRowOffset(5, 100, 240, false) == 5u * 240u); + CHECK(rasterRowOffset(99, 100, 240, false) == 99u * 240u); + // Flipped (bottom-up DIBs): row y is (height-1-y)*rowSpan — LICE_SysBitmap's own pixel + // accessor, `(h-1-y)*rowspan + x` (lice.cpp:2262). + CHECK(rasterRowOffset(0, 100, 240, true) == 99u * 240u); + CHECK(rasterRowOffset(99, 100, 240, true) == 0u); + CHECK(rasterRowOffset(40, 100, 240, true) == 59u * 240u); +} + // --- long-segment subdivision ------------------------------------------------ static void testSubdivisionDoesNotChangeTheRenderedStroke() { @@ -381,6 +427,7 @@ static void testKnobArcIsOpaqueAndEvenAllTheWayRound() { int main() { testStraightStrokeHasAnOpaqueCore(); + testSubOpaqueCoreAtOnePixelWidth(); testPerpendicularWeightIsAngleIndependent(); testWeightHoldsAtTheExactDiagonal(); testZeroLengthSegmentIsARoundDot(); @@ -394,6 +441,7 @@ int main() { testBoundsClipToTheClipRectAndCoverTheReach(); testStrokeEntirelyOutsideTheClipDrawsNothing(); testCoverageOutsideTheValidSpanReadsZero(); + testRasterRowOffsetMatchesUnflippedAndFlippedLayouts(); testSubdivisionDoesNotChangeTheRenderedStroke(); testArcPointsLieOnTheCircleAndRespectTheFlatness(); testArcDensityGrowsWithRadius(); diff --git a/tests/test_velocity_curve.cpp b/tests/test_velocity_curve.cpp index fdd5a2c..558e29b 100644 --- a/tests/test_velocity_curve.cpp +++ b/tests/test_velocity_curve.cpp @@ -16,6 +16,7 @@ #include "../src/core/instrument/engine/velocity_curve.h" +#include #include #include #include @@ -398,12 +399,35 @@ static void testSubpixelMapIsTheIntegerMapBeforeRounding() { const Box boxes[] = {{10, 20, 100, 51}, {0, 0, 127, 101}, {7, 3, 33, 17}}; for (const Box& box : boxes) { for (const VelocityCurve& c : {uni(), bip()}) { + const double lo = curveYMin(c.domain()); for (double v = 0.0; v <= 127.0; v += 1.0) { for (double a = -1.0; a <= 1.0; a += 0.125) { const auto ip = c.pixelFromPoint(box, {v, a}); const auto fp = c.subpixelFromPoint(box, {v, a}); + // Relationship guard: int IS round(subpixel), everywhere — a forward guard + // against the trace drifting off its own handles. This alone is a tautology + // against the CURRENT formula (int literally computes this expression), so + // it cannot catch a regression against the PRE-subpixel formula below. CHECK(ip.x == box.left + static_cast(fp.x - box.left + 0.5)); CHECK(ip.y == box.top + static_cast(fp.y - box.top + 0.5)); + + // Independent re-derivation of the formula the int map claims to reproduce — + // box.left + (int)(frac*w + 0.5) — computed here from scratch rather than + // through subpixelFromPoint's box.left-add-then-subtract round trip, so a + // flip introduced by that round trip (however unlikely per Sterbenz) has + // something to fail against. + const double vClamped = (std::min)(kVelMax, (std::max)(kVelMin, v)); + const double fracX = (vClamped - kVelMin) / (kVelMax - kVelMin); + const int oldX = + box.left + static_cast(fracX * static_cast(box.width) + 0.5); + CHECK(ip.x == oldX); + + const double aClamped = (std::min)(kCurveYMax, (std::max)(lo, a)); + const double fracY = (aClamped - lo) / (kCurveYMax - lo); + const int oldY = + box.top + static_cast((1.0 - fracY) * + static_cast(box.height - 1) + 0.5); + CHECK(ip.y == oldY); } } }