From dd067a192e954948425dcc6a0a6e412826e97f78 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 13:31:50 -0400 Subject: [PATCH] =?UTF-8?q?=CE=A9-W1-T3=20review=20fixes:=20skip=20meter?= =?UTF-8?q?=20invalidate=20when=20covered;=20push=20fast-path=20predicates?= =?UTF-8?q?=20into=20core?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the critical finding — Browse/curve-popup no longer trigger 60 Hz whole-client repaints when the meter is invisible. Pushes the fast-path and self-containment geometry into master_meter with tests; batches the four minor findings. --- src/core/instrument/CLAUDE.md | 2 +- src/core/instrument/ui/master_meter.cpp | 18 +++++++++++++++ src/core/instrument/ui/master_meter.h | 13 +++++++++++ src/shell/instrument/CLAUDE.md | 6 ++++- src/shell/instrument/editor_paint.cpp | 11 ++++----- src/shell/instrument/editor_paint_deck.cpp | 4 ++++ src/shell/instrument/editor_platform.cpp | 18 ++++++++++++++- src/shell/instrument/reasampler_editor.h | 24 +++++++++---------- tests/test_master_meter.cpp | 27 ++++++++++++++++++++++ 9 files changed, 102 insertions(+), 21 deletions(-) diff --git a/src/core/instrument/CLAUDE.md b/src/core/instrument/CLAUDE.md index 6fa1f27..1e25341 100644 --- a/src/core/instrument/CLAUDE.md +++ b/src/core/instrument/CLAUDE.md @@ -362,7 +362,7 @@ anything for a trigger shape. drag the bank model and the WAV codec in behind it. The shell keeps only the controls the parameter set does not carry (key-track, voice count, master gain, preview velocity) and the labels for them. -- `master_meter` — the MASTER column's interior, split from `knob_deck` on the axis `sample_chrome` has to `sample_bands`: that says where the column is, this lays out inside it (22 px numeral gutter · 4 · 36 px bar field) and holds the per-instance UI state the bars draw from. `kMeterColumnW` is the SUM of those three, exported so `deck_groups`' MASTER descriptor reserves exactly what the interior consumes — the column is banked to grow, and a reserve that did not track it would underfill or overrun silently. **Bar count takes a RESOLVED `LaneSplit`, the same value `waveform_view`'s `resolveLaneSplit` answers** — a mono source under stereo mode is dual-mono, and two identical bars would be a lie. Also owns `meterTickNumeralled` (the spec-pinned 0/−12/−24/−36/−48/−60 numeral set, beside the tick step it derives from), `meterNumeralRect` (bottom-clamped, so the floor tick's numeral cannot hang out of the gutter), and `meterSingleLaneState` — the one bar folds both channels PER FIELD, never picking a whole channel by level. Composes `engine/meter_ballistics` per channel and gives the gain-reduction lamp the peak tick's own hold-then-release, without which a catch smaller than 20 dB × the UI period is dark again before it has been drawn twice; the audio thread's clip flag is ORed in because it is the only latch that sees every block. `meterDrawEqual` is what lets the UI tick repaint on change alone. +- `master_meter` — the MASTER column's interior, split from `knob_deck` on the axis `sample_chrome` has to `sample_bands`: that says where the column is, this lays out inside it (22 px numeral gutter · 4 · 36 px bar field) and holds the per-instance UI state the bars draw from. `kMeterColumnW` is the SUM of those three, exported so `deck_groups`' MASTER descriptor reserves exactly what the interior consumes — the column is banked to grow, and a reserve that did not track it would underfill or overrun silently. **Bar count takes a RESOLVED `LaneSplit`, the same value `waveform_view`'s `resolveLaneSplit` answers** — a mono source under stereo mode is dual-mono, and two identical bars would be a lie. Also owns `meterTickNumeralled` (the spec-pinned 0/−12/−24/−36/−48/−60 numeral set, beside the tick step it derives from), `meterNumeralRect` (bottom-clamped, so the floor tick's numeral cannot hang out of the gutter), and `meterSingleLaneState` — the one bar folds both channels PER FIELD, never picking a whole channel by level. Composes `engine/meter_ballistics` per channel and gives the gain-reduction lamp the peak tick's own hold-then-release, without which a catch smaller than 20 dB × the UI period is dark again before it has been drawn twice; the audio thread's clip flag is ORed in because it is the only latch that sees every block. `meterDrawEqual` is what lets the UI tick repaint on change alone. Also owns the editor's two paint-dispatch predicates, so neither lives as a bare comment in the shell: `meterFastPathEligible` (is a WM_PAINT dirty rect wholly inside the field?) and `meterBarsWithinField` (the self-containment invariant that fast path rests on), both asserted in `test_master_meter.cpp`. - `deck_groups` — also home to `deckParamCommit` and `liveCommitFor`, the editor's whole commit-tier routing decision (see "Live parameter delivery" above), and to `OverlayEnv` + `nextOverlaySelection`/`overlayEnvEnabled`/`overlayEnvInert`, the whole overlay-selection state machine (exclusivity, the none resting state, and which selections a disabled or DRAWN group makes inert); WHICH groups the Sample face's deck carries, split from `knob_deck`'s HOW they lay out: the `DeckParam` control-id space (the editor's `ParamControl` is an alias of it), the `DeckGroupId` list, `sampleDeckGroups` in signal-flow order (**pitch → filter → amp**, then velocity/voice/master), and the deck's bipolar-knob law. Reads `PlayMode` for the AMP group's Gate/Trigger face, which is why this and not `knob_deck` is the module that touches the engine's value layer. Also home to `CurveTarget` + `curveTargetFor` — the VELOCITY group's three cells are popup openers, not dials, and that predicate is the ONE place they are named, so paint, hit-test routing and the popup's title all agree. MASTER is reserved for post-voice-mixer concerns, which is why the curves sit in their own group immediately left of VOICE rather than there; it now discharges that reservation as the double-height bus deck — gain, the limiter enable, one reserved slot, the meter column and the GR lamp. FILTER's `Band|Notch` rides its caption slack rather than the knob row: that is the −92 px that makes the SOUND row fit its block, and putting it back breaks the fit. VOICE's `Retrig|Legato` deliberately stays in the knob row — VOICE's caption row is the binding side, so moving it there makes the group 226 rather than 164. - `spline_edit` — THE point-editing grammar, and the one place it is written down: left-click grabs a node and adds one in empty space, right-click deletes, control-click toggles hard/smooth. Both spline consumers — the velocity-curve popup and the spline EG overlay — route their mouse-down through `resolveSplineEdit`, so the two cannot drift into two grammars. The endpoint and point-count rules are NOT restated here: `deletePoint` and `addPoint` own them, and the caller applies the resolved action to the curve. Also home to `splineOverlayBox`, the contour's mapping box inside the waveform overlay — the FULL area, no inset, so the drawn contour stays 1:1 with the sample's time axis. Spline points are excluded from `param_taper`'s Shift/Ctrl modifier law like waveform markers are: a point is a normalized position with no displayed unit, and control-click there is already claimed by the hard/smooth toggle above. - `curve_popup` — pure curve-popup geometry + dismissal test (FB1): centered sheet over the Sample face — width/height clamps, title row, Close button rect, curve-box rect, outside-sheet dismissal test. Mirror of `overflow_menu`; no LICE or REAPER types. diff --git a/src/core/instrument/ui/master_meter.cpp b/src/core/instrument/ui/master_meter.cpp index 06318e4..c19ff5b 100644 --- a/src/core/instrument/ui/master_meter.cpp +++ b/src/core/instrument/ui/master_meter.cpp @@ -26,6 +26,24 @@ MeterRects meterRects(const Rect& column, LaneSplit split) { return r; } +bool meterFastPathEligible(const MeterRects& m, const Rect& dirty) { + return !m.field.empty() && dirty.x >= m.field.x && dirty.y >= m.field.y && + dirty.right() <= m.field.right() && dirty.bottom() <= m.field.bottom(); +} + +namespace { +bool rectWithin(const Rect& outer, const Rect& inner) { + return inner.empty() || + (inner.x >= outer.x && inner.right() <= outer.right() && inner.y >= outer.y && + inner.bottom() <= outer.bottom()); +} +} // namespace + +bool meterBarsWithinField(const MeterRects& m) { + if (m.field.empty()) return m.barA.empty() && m.barB.empty(); + return rectWithin(m.field, m.barA) && rectWithin(m.field, m.barB); +} + Rect meterNumeralRect(const Rect& labels, int y) { if (labels.empty()) return {}; int top = y - 5; diff --git a/src/core/instrument/ui/master_meter.h b/src/core/instrument/ui/master_meter.h index 39042f7..6c7fda1 100644 --- a/src/core/instrument/ui/master_meter.h +++ b/src/core/instrument/ui/master_meter.h @@ -42,6 +42,19 @@ struct MeterRects { // A column narrower than kMeterColumnW yields nothing rather than an overrunning field. MeterRects meterRects(const Rect& column, LaneSplit split); +// Whether `dirty` (a WM_PAINT update rect, already client-clipped) lies wholly inside the bar +// field — the editor's meter-frame fast-path test. Geometric, not a flag: the caller decides +// per paint from the rect Windows actually handed it, never from a remembered "was this a +// meter tick" bit, because Windows unions a meter invalidate with any other pending one into a +// single rcPaint and a flag would then paint a meter frame over a face that had really changed. +bool meterFastPathEligible(const MeterRects& m, const Rect& dirty); + +// The structural invariant drawMeterField's self-containment rests on: every bar it paints +// stays inside the field it fills first. The ticks, the held-peak tick and the clip cap are all +// drawn directly off `field`'s own coordinates and so are bounded by construction; the bars are +// the one independently-computed rect that isn't. +bool meterBarsWithinField(const MeterRects& m); + // y of `db` inside the bar field — kMeterTopDb at the top edge, kMeterFloorDb at the bottom, // linear in dB between, clamped outside. int meterDbToY(const Rect& field, double db); diff --git a/src/shell/instrument/CLAUDE.md b/src/shell/instrument/CLAUDE.md index 344b241..b45b9ec 100644 --- a/src/shell/instrument/CLAUDE.md +++ b/src/shell/instrument/CLAUDE.md @@ -284,7 +284,11 @@ against a performance budget — they are there because `VoiceEngine::applyLiveT a flag would paint a meter frame over a face that had really changed. What stays outside the field stays on the full path: the static numeral gutter (AA text re-blended onto itself every frame thickens) and the GR lamp (drawn straight onto the deck group's gradient, which a - sub-rect fill cannot reproduce), so a lamp transition takes a whole-client repaint. + sub-rect fill cannot reproduce), so a lamp transition takes a whole-client repaint. **When the + meter is covered or absent** (Browse, the curve popup, the empty state) `invalidateMeter` + skips invalidating anything at all rather than falling back to a whole-client repaint — the + ballistics still advance on `onMeterTimer`'s own clock, but nothing visible changed, so a + 60 FPS whole-client repaint under a modal sheet would be pure cost for zero pixels shown. - The bake's availability probe runs on the SAME tick that paints the button, so the control can never be enabled on one tick and refuse on the next. The bake Hold control's applicability (`resolveBakeHoldNeeded`) rides the same tick for the same reason, and diff --git a/src/shell/instrument/editor_paint.cpp b/src/shell/instrument/editor_paint.cpp index 6e8f3ff..5b5b5a4 100644 --- a/src/shell/instrument/editor_paint.cpp +++ b/src/shell/instrument/editor_paint.cpp @@ -51,11 +51,9 @@ void ReaSamplerEditor::paint(HDC hdc, const RECT& dirty) { // A meter frame's dirty rect lies wholly inside the bar field, and nothing else is drawn // there — so the field is all that needs redrawing, and the rest of the face is still in the - // back buffer from the last full compose. Decided GEOMETRICALLY rather than by a flag, - // because Windows coalesces a meter invalidate with any other pending one into a single - // union rect: a flag would then paint a meter frame over a face that had really changed. - const Rect& mf = meterRects_.field; - if (!mf.empty() && dx >= mf.x && dy >= mf.y && dr <= mf.right() && db <= mf.bottom()) { + // back buffer from the last full compose. Decided GEOMETRICALLY rather than by a flag — see + // meterFastPathEligible (master_meter) for why. + if (meterFastPathEligible(meterRects_, Rect::ltrb(dx, dy, dr, db))) { paintMeterField(bmp); } else { // Repopulated by paintDeck when the meter is on screen; left empty by the empty state. @@ -83,7 +81,8 @@ void ReaSamplerEditor::paint(HDC hdc, const RECT& dirty) { } // A sheet layered over the face covers the meter, so the fast path must not paint - // through it — dropping the rect routes the next meter tick down this path instead. + // through it even if something else invalidates a rect that happens to match the + // stale field bounds — invalidateMeter itself already skips while covered (see there). if (view_ == View::kBrowse || curvePopup_ != CurveTarget::kNone) meterRects_ = {}; } diff --git a/src/shell/instrument/editor_paint_deck.cpp b/src/shell/instrument/editor_paint_deck.cpp index e116608..5282110 100644 --- a/src/shell/instrument/editor_paint_deck.cpp +++ b/src/shell/instrument/editor_paint_deck.cpp @@ -54,6 +54,10 @@ void drawMeterNumerals(LICE_IBitmap* bmp, const MeterRects& m) { // covers every pixel the rest of it then draws, so a meter frame can redraw this rect alone. // Two bars or one is read off the rects (barB is empty exactly when the split is Single), so // the lane decision has one representation here rather than two. +// This self-containment depends on Role::BgCell being fully OPAQUE at InteractionState::Rest +// (theme.cpp: alpha 255) — fillGradient blends rather than overwrites at alpha < 255, so a +// translucent BgCell would make every meter frame re-blend over whatever the last frame left, +// the exact AA-thickening the numeral gutter comment below is guarding against. void drawMeterField(LICE_IBitmap* bmp, const MeterRects& m, const MasterMeterUi& state) { // A column narrower than the interior needs yields all-empty rects, which under rect.h's // contract means suppressed — not a zero-height field to fill, tick twelve times and cap. diff --git a/src/shell/instrument/editor_platform.cpp b/src/shell/instrument/editor_platform.cpp index c285095..080d256 100644 --- a/src/shell/instrument/editor_platform.cpp +++ b/src/shell/instrument/editor_platform.cpp @@ -77,8 +77,20 @@ void ReaSamplerEditor::invalidate() { void ReaSamplerEditor::invalidateMeter() { if (!childHwnd_) return; + // The meter is covered (Browse, the curve popup) or has nothing to draw (empty state) — + // ballistics still advance in onMeterTimer, but nothing on screen changed, so invalidating + // anything here would only buy a whole-client repaint of chrome/waveform/deck the meter + // never touches. Distinct from "bounds not resolved yet" below: this is a fact about what + // the face is showing, not about whether meterRects_ happens to be populated. + const bool meterOnScreen = + view_ == View::kSample && curvePopup_ == CurveTarget::kNone && !selectedId_.empty(); + if (!meterOnScreen) return; + const Rect& f = meterRects_.field; - if (f.empty()) { // nothing cached (see meterRects_) — the whole face has to answer + if (f.empty()) { + // On screen, but no full paint has resolved its bounds yet (first paint, or a resize + // just dropped the cache) — the whole-client fallback is cheap here because the window + // is already fully invalid from the resize/creation that caused this. invalidate(); return; } @@ -155,6 +167,10 @@ void ReaSamplerEditor::removedFromParent() { childHwnd_ = nullptr; } releaseBackBuffer(); // a client-area bitmap outlives nothing here + // Unreachable today (WM_PAINT outranks WM_TIMER, so a reopen's first paint resolves this + // before any meter tick can read it stale) — cleared anyway so the invariant is structural, + // not a timing accident, the same reason onSize clears it on resize. + meterRects_ = {}; } tresult PLUGIN_API ReaSamplerEditor::onSize(ViewRect* newSize) { diff --git a/src/shell/instrument/reasampler_editor.h b/src/shell/instrument/reasampler_editor.h index 46ad7bb..ffc843b 100644 --- a/src/shell/instrument/reasampler_editor.h +++ b/src/shell/instrument/reasampler_editor.h @@ -1,9 +1,8 @@ // reasampler_editor.h — VST3 IPlugView LICE editor for the ReaSampler 9000 UI. Thin shell: -// hosts a LICE child window, routing host paint/mouse into the pure geometry modules -// (sample_bands, sample_chrome, capture_browser, keyboard_strip, sample_map). The Sample -// face is a three-band stack — chrome, waveform, decks — and the shell TUs split on that -// same axis; Browse is a modal picker over it. All layout/hit-test/drag math lives in the -// pure modules; every edit commits off the audio thread via reloadInstrument. +// hosts a LICE child window, routing host paint/mouse into the pure geometry modules. The +// Sample face is a three-band stack — chrome, waveform, decks — and the shell TUs split on +// that same axis; Browse is a modal picker over it. All layout/hit-test/drag math lives in +// the pure modules; every edit commits off the audio thread via reloadInstrument. #pragma once @@ -65,6 +64,9 @@ public: // always supplies one). explicit ReaSamplerEditor(ReaSamplerProcessor* processor); ~ReaSamplerEditor() override; + // backBuffer_ owns a raw LICE bitmap with no refcount behind it — a copy would double-free. + ReaSamplerEditor(const ReaSamplerEditor&) = delete; + ReaSamplerEditor& operator=(const ReaSamplerEditor&) = delete; Steinberg::tresult PLUGIN_API isPlatformTypeSupported( Steinberg::FIDString type) override; @@ -302,8 +304,8 @@ private: struct ChannelPcm { std::vector interleaved; // frame-interleaved source frames int channelCount = 0; // 0 = nothing decoded - Envelope heroEnv; // binned from `interleaved`, so every path that drops the - std::size_t heroBins = 0; // decode drops it too. 0 = not computed + Envelope heroEnv; // binned from `interleaved`; cleared whenever it is + std::size_t heroBins = 0; // 0 = not computed std::int64_t frameCount() const { return channelCount > 0 ? static_cast(interleaved.size()) / channelCount @@ -467,9 +469,7 @@ private: // up. One note at a time — a fresh press releases the prior. int previewingNote_ = -1; - // Resample bake. The click only ARMS it; the sync tick runs it. Running it inline - // would nest a synchronous REAPER action — which re-points this very instance — inside - // a mouse handler with the capture held. + // Resample bake — the click only ARMS it (see this directory's CLAUDE.md Gotchas for why). bool bakePending_ = false; // Whether the extension's bake action is registered, resolved on the same tick that // governs the button's paint, so the control is never enabled and then refusing. @@ -494,8 +494,8 @@ private: // The MASTER deck's meter, advanced from the published block magnitudes on its own timer // (see onMeterTimer for why it runs mid-drag too, and why the first read is discarded). // A default-constructed meterTick_ means never ticked. meterRects_ is the column interior - // resolved at the last FULL paint; an empty field means "not on screen" (no layout yet, the - // empty state, a modal sheet over it) and sends the tick down the whole-client path. + // resolved at the last FULL paint; an empty field is ambiguous alone (no layout yet, or + // genuinely off screen) — invalidateMeter asks view_/curvePopup_/selectedId_ to tell which. instrument::ui::MasterMeterUi masterMeter_; std::chrono::steady_clock::time_point meterTick_{}; instrument::ui::MeterRects meterRects_; diff --git a/tests/test_master_meter.cpp b/tests/test_master_meter.cpp index f97c2fb..36ac450 100644 --- a/tests/test_master_meter.cpp +++ b/tests/test_master_meter.cpp @@ -5,6 +5,8 @@ // the mono bar taking the whole field, the two stereo bars, all inside the column. // * bar count — the SAME LaneSplit resolveLaneSplit folds, over channel mode x source // channel count, so it can never become a second rule. +// * the editor's dirty-rect fast path — meterFastPathEligible's wholly-inside test, and +// meterBarsWithinField, the self-containment invariant the fast path rests on. // * the dB axis — top/floor on the field's edges, an interior value, and the clamps. // * ballistics — instantaneous rise, 20 dB/s fall, the 1.5 s hold and its release AT RATE; // the audio thread's clip latch surviving a UI frame; the per-field single-lane fold. @@ -287,6 +289,29 @@ static void testDrawEqualityCoversTheDrawnQuantities() { CHECK(meterDrawEqual(a, graze)); } +// The editor's WM_PAINT fast-path test: a dirty rect wholly inside the field takes the +// meter-only redraw; anything that pokes outside it, or a field that hasn't been laid out yet, +// falls back to the full compose. +static void testFastPathEligibleOnlyForADirtyRectInsideTheField() { + const MeterRects m = meterRects(kColumn, LaneSplit::Single); + CHECK(meterFastPathEligible(m, m.field)); // the whole field + CHECK(meterFastPathEligible( + m, Rect::ltrb(m.field.x + 2, m.field.y + 2, m.field.right() - 2, m.field.bottom() - 2))); + CHECK(!meterFastPathEligible(m, Rect::ltrb(m.field.x - 1, m.field.y, m.field.right(), + m.field.bottom()))); // pokes left of the field + CHECK(!meterFastPathEligible( + m, Rect::ltrb(kColumn.x, kColumn.y, kColumn.right(), kColumn.bottom()))); // whole column + CHECK(!meterFastPathEligible(MeterRects{}, m.field)); // no cached layout at all +} + +// The self-containment invariant drawMeterField's redraw-the-field-alone shortcut rests on: +// every bar meterRects() hands back stays inside the field, in both lane splits. +static void testFieldBoundsEveryBarItPaints() { + CHECK(meterBarsWithinField(meterRects(kColumn, LaneSplit::Single))); + CHECK(meterBarsWithinField(meterRects(kColumn, LaneSplit::Stereo))); + CHECK(meterBarsWithinField(MeterRects{})); // degenerate: nothing to bound, nothing drawn +} + static void testDegenerateColumnYieldsNothing() { const MeterRects m = meterRects(Rect::ltrb(0, 0, 0, 0), LaneSplit::Stereo); CHECK(m.field.empty() && m.barA.empty() && m.barB.empty()); @@ -300,6 +325,8 @@ int main() { testEveryOtherTickCarriesANumeral(); testTheFloorNumeralStaysInsideTheGutter(); testAColumnTooNarrowForTheInteriorDrawsNothing(); + testFastPathEligibleOnlyForADirtyRectInsideTheField(); + testFieldBoundsEveryBarItPaints(); testPeakRisesAtOnceAndFallsAtTwentyDbPerSecond(); testPeakHoldSitsForItsFullWindowThenReleases(); testClipLatchesFromThePublishedFlagAndClearsOnDemand();