feat(gui): Lean render mode — opaque pan/VFO, capped repaint, WAVE off, meter throttle (#3283)#3286
Conversation
Adds a reversible, persisted "Lean" toggle (button in each panadapter's overlay menu) that bundles the render-cost reductions found during the aethersdr#3283 profiling investigation. Applied app-wide via MainWindow::applyLeanMode, persisted as LeanMode in AppSettings. Effects: - SpectrumWidget::setLeanMode — opaque single layer (skip wallpaper in GPU + software paths), drop translucent FFT fill, cap spectrum/waterfall repaint to ~30 Hz. - VfoWidget::setOpaqueMode — swap #VfoWidgetRoot stylesheet to an opaque background so the panel composites as a cacheable opaque layer. - WaveApplet::setActive — hide the WAVE scope and drop its sample feed. - MeterSmoother::shouldRepaint — shared ~12 Hz gate; meters keep integrating ballistics at full rate but batch their repaints. Wired into the comp/gate/de-ess GR meters and the S-meter needle. Profiling result (matched same-session A/B): main-thread CPU -21% (the reproducible responsiveness win); total CPU flat — the dominant pooled cost is the QRhiWidget full-window backing-store->texture compositing tax, which is data-rate-dominated and not reached by these widget-level changes. See aethersdr#3283 for full trace tables and the remaining structural fix directions (dirty-region upload / GPU UI). WIP backup — not yet reviewed for visual-design sign-off. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks for this, @jensenpat — the implementation is clean and the PR description is refreshingly honest about what Lean does and doesn't buy. It's purely additive and reversible, follows the AppSettings "True"/"False" convention correctly (matches FramelessWindow et al.), is null-safe on the waveApplet()/m_appletPanel paths, and the QSignalBlocker in setLeanChecked() correctly prevents the toggle→apply→sync loop. Nice. A few things worth a look before this leaves draft:
1. MeterSmoother::shouldRepaint() is a single shared gate — it doesn't actually batch meters into one frame (medium)
The comment says it "gates the repaint to ~12 Hz via a single shared clock, so all meters batch into one frame." But because lastMs is advanced by any caller, the first meter to tick in each ~83 ms window returns true and every other meter in that window returns false:
if (lastMs < 0 || now - lastMs >= kGateMs) { lastMs = now; return true; }
return false;So the aggregate repaint rate is ~12 Hz, but each individual meter repaints at roughly 12/N Hz, and which meter wins depends on QTimer phase ordering. The settled frame always paints (good — no final value is ever dropped), but a meter that animates continuously and never settles — the comp/gate GR bars under sustained TX, or the S-meter needle on a fluctuating signal — can end up updating at ~3 Hz or, if it consistently loses the phase race, appear frozen until it settles. That undercuts the "meters keep smooth ballistics" goal in lean. If per-meter ~12 Hz was the intent, the gate needs to be per-widget (track lastMs per caller) rather than one shared static.
2. Repaint cap is 30 Hz but several comments say 60 Hz (minor)
kLeanFrameMs = 33 is ~30 Hz, and the PR body + the kLeanFrameMs comment correctly say 30 Hz. But three doc comments still say 60 Hz and should be aligned:
SpectrumWidget.hsetLeanModedoc — "cap spectrum/waterfall repaints to ~60 Hz"MainWindow.happlyLeanModedoc — "60 Hz repaint cap"MainWindow.cppapplyLeanModebody — "+ 60 Hz cap"
3. "Drop its sample feed" is really "drop the repaint" (minor)
WaveApplet::setActive(false) gates appendScopeSamples, but the upstream AudioEngine::{tx,rx}PostChainScopeReady signals still fire once per audio callback and queue events onto the GUI thread (MainWindow.cpp:1592-1599); only the handler short-circuits. That's fine for the repaint win, but the feed isn't truly dropped — if the per-callback emit/queue is measurable you'd want to disconnect. Also, toggling Lean off calls wave->setActive(true) unconditionally, which re-activates the scope even if the user hadn't been using it — worth confirming that's the intended round-trip.
None of these are blockers and the visual/UX call is the maintainers' per your draft note. (1) is the one I'd most want addressed since it affects the felt smoothness that's the PR's headline win.
🤖 aethersdr-agent · cost: $15.9823 · model: claude-opus-4-8
Relocate the Lean-mode toggle off the panadapter's main slice/overlay button column and into the Display sub-panel, placed directly below the background chooser. The main column stays focused on the primary per-slice actions; Lean is a display preference, so it belongs with the other display controls. - SpectrumOverlayMenu: drop the "Lean" entry + special-case handling from the main button defs[]; create m_leanBtn as a checkable full-width button in buildDisplayPanel() under the Background row (green :checked styling via the panel's btnStyle). Behavior is unchanged — it still emits leanModeToggled() and setLeanChecked() keeps it in sync. - Remove the now-unused kBtnLean constant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
NF0T
left a comment
There was a problem hiding this comment.
The implementation is clean and the PR description is refreshingly honest about what Lean does and doesn't buy — the main-thread −21% win is real, the flat total CPU and the dominant QRhi backing-store cost are documented clearly, and the change is purely additive with no behavior when off. Notes below are technical observations for context; the visual-design and "is this worth shipping?" questions are Jeremy's call per CLAUDE.md's autonomy boundaries, not ours.
Technical items worth addressing regardless of the merge decision
1. MeterSmoother::shouldRepaint() shared gate underdelivers on its own claim (medium)
lastMs is a shared static advanced by any caller. The first meter to tick in each ~83 ms window gets true; every other gets false. With five active meters concurrently, each individual meter repaints at roughly 12/5 ≈ 2.4 Hz under sustained TX — not 12 Hz. A GR bar or S-meter needle that never fully settles can appear near-frozen in Lean mode, which directly undercuts the "smooth ballistics at reduced repaint cost" headline. Fix: track lastMs per calling widget rather than as a shared static.
2. Constitution Principle V — flat LeanMode AppSettings key
AppSettings::instance().value("LeanMode", "False") adds a new flat key to the ~460-entry flat namespace. Principle V requires new feature configuration as nested JSON under a single root key. Small fix, but worth doing before merge to stay consistent with the direction the codebase is moving.
3. Repaint cap comment inconsistency — three places say 60 Hz, actual value is 30 Hz
kLeanFrameMs = 33 (~30 Hz) is correct. The setLeanMode doc in SpectrumWidget.h, the applyLeanMode doc in MainWindow.h, and the body comment in MainWindow.cpp all say 60 Hz. Worth aligning.
4. WaveApplet round-trip
applyLeanMode(false) unconditionally calls wave->setActive(true), which unhides the scope even if the user had manually hidden it before enabling Lean. Worth confirming that's the intended behavior on round-trip.
Marking Changes Requested to surface these technical items before merge, not as a gate on the design direction — that's Jeremy's decision. The visual changes (opaque VFO, no fill/wallpaper, hidden WAVE scope) and the "is the main-thread win sufficient to ship this?" question are maintainer-level calls per the autonomy boundaries in CLAUDE.md.
|
@jensenpat — Claude here, on Jeremy's behalf. Took another pass on top of @NF0T's review and the earlier bot review. Both correctly identified the three substantive items: shared-static meter gate, 60 Hz comments vs 30 Hz cap, and the unconditional Two refinements + a few small things neither earlier review surfaced: Concrete shape for the per-widget gate fixBoth prior reviews say "make the gate per-widget" but neither suggests how. Cleanest path: promote the gate from class MeterSmoother {
// ...
static void setLeanThrottle(bool on) { s_leanThrottle = on; }
bool shouldRepaint() { // <-- instance method now
if (!s_leanThrottle) return true;
constexpr qint64 kGateMs = 1000 / kLeanRepaintHz;
if (!m_gate.isValid()) m_gate.start();
const qint64 now = m_gate.elapsed();
if (m_lastMs < 0 || now - m_lastMs >= kGateMs) {
m_lastMs = now;
return true;
}
return false;
}
private:
QElapsedTimer m_gate;
qint64 m_lastMs{-1};
static inline bool s_leanThrottle = false;
};Bonus: this also matches the per-instance Principle V nested-JSON migrationFor the // Read
const auto display = AppSettings::instance().jsonObject("Display");
const bool leanMode = display.value("leanMode").toBool(false);
// Write
auto display = AppSettings::instance().jsonObject("Display");
display.insert("leanMode", on);
AppSettings::instance().setJsonObject("Display", display);Means future display-related toggles (lean, frameless, theme variants, etc.) live under one Smaller items neither review caught
None of these block on the design direction (opaque VFO, no wallpaper, WAVE off) — that's Jeremy's call per CLAUDE.md. They're all about making the implementation hold up to the headline claims. Want me to push these as a fix-up commit on top of your branch (the same way I did for the KISS TNC PR), or would you rather take a swing yourself? Either's fine. 73, Jeremy KK7GWY & Claude (AI dev partner) |
…Mode Addresses NF0T's four CHANGES_REQUESTED items on aethersdr#3286 plus the three items raised in my 2026-06-05 coordination comment. NF0T blockers: 1. Per-widget MeterSmoother repaint gate (was shared-static). The previous shared-static gate let whichever meter ticked first each ~83 ms window through and starved the other N−1 meters that window. With 5 active meters (S-Meter + 3 client-side dynamics applets + comp strip) the effective per-meter cadence dropped from the intended 12 Hz to ~2.4 Hz — visibly stuttery on GR bars and the S-meter needle. MeterSmoother::shouldRepaint() is now an instance method carrying its own QElapsedTimer + lastMs; every active meter independently hits kLeanRepaintHz. SMeterWidget runs its own custom needle animation and never owned a MeterSmoother — it now holds one solely as the gate source so the throttle policy stays uniform across every meter. 2. Principle V — nested-JSON settings for Lean Mode. The flat AppSettings key "LeanMode" is replaced by a "Display" root object containing { "leanMode": "..." }. New header-only helper DisplaySettings (mirrors CwDecodeSettings.h's pattern) wraps the getter/setter and runs a one-shot migration on first launch that reads any legacy flat "LeanMode" key, writes it into the JSON root, and removes the flat key. MainWindow now reads via DisplaySettings::leanMode() and writes via DisplaySettings::setLeanMode(bool). 3. 60 Hz / 30 Hz comment inconsistency. The repaint cap was changed to ~30 Hz (kLeanFrameMs = 33) during the final tuning pass but four comments still said "60 Hz" — updated in MainWindow.cpp/.h, SpectrumWidget.cpp, and SpectrumWidget.h with the constant cross-referenced so future readers see one number. 4. WaveApplet activation round-trip respects pre-Lean state. Previously toggling Lean on disabled the WAVE scope and toggling Lean off unconditionally re-enabled it — clobbering a user who had already disabled WAVE manually. We now snapshot wave->isActive() into m_preLeanWaveActive on Lean entry and restore that value on Lean exit. WaveApplet exposes a new isActive() getter for the snapshot; its setActive() doc is expanded to clarify that the "drop sample feed" framing means appendScopeSamples short-circuits — the upstream AudioEngine::{tx,rx}PostChainScopeReady signal still fires per audio callback, so the savings are no-m_waveform-work + no-repaint, not signal suppression. This follows the same maintainer-fix-up pattern used on aethersdr#3279 (KISS TNC), aethersdr#3335 (Reboot Radio), aethersdr#3350, and aethersdr#3391 (perf connect): stale community PR, NF0T review with concrete blockers, maintainer pushes the addressing commits back to the author's fork to unblock. Principle XI.
|
Coordination update — pushed What's in the commit:
Build is clean locally (978/978, only pre-existing unrelated warnings). @NF0T — ready for your re-review when you have time. @jensenpat — heads up that I pushed onto your branch under the established maintainer-fix-up pattern; happy to revert any piece you'd rather take a different cut at. Principle X. (self-assigned before pushing.) |
NF0T
left a comment
There was a problem hiding this comment.
All four items from my 2026-06-04 CHANGES_REQUESTED are addressed in `37971c65`.
Verified against the full PR diff, not just the commit description.
Blocker 1 — Per-widget MeterSmoother gate ✓
`shouldRepaint()` is now an instance method; `QElapsedTimer m_gate` and
`qint64 m_lastMs` are per-instance members. All five call sites updated to
`m_smooth.shouldRepaint()`. SMeterWidget correctly holds a dedicated
`MeterSmoother m_smooth` solely as a gate source — the added comment explains
why cleanly. One placement note below.
Blocker 2 — Principle V nested JSON ✓
`DisplaySettings.h` follows the `CwDecodeSettings.h` header-only pattern,
wraps the key under a `"Display"` root blob, and includes a correct
`migrateLegacy()` that runs before any read on startup. `applyLeanMode()`
uses `DisplaySettings::setLeanMode()`. Fresh-install path (no legacy key)
correctly defaults to false.
Blocker 3 — 60 Hz / 30 Hz comment inconsistency ✓
All four stale references corrected to `~30 Hz (kLeanFrameMs = 33)`.
`SpectrumWidget.h` also gained a useful note that `leanCappedUpdate()` only
caps the two high-frequency data paths — interactive paths stay uncapped.
Blocker 4 — WaveApplet round-trip ✓
`m_preLeanWaveActive` snapshot/restore pattern is correct. `isActive()` getter
clean. The `setActive()` doc accurately describes what "drop sample feed" means.
One placement nit worth a quick look:
`MeterSmoother m_smooth` in `SMeterWidget.h` appears to land in the trailing
public / `public slots:` region of the class (after the `ARC_START_DEG` /
`ARC_END_DEG` constants) rather than in `private:`. The member is only used
internally (`animateNeedle`), so it should be private. The code compiles and
works correctly — this is purely a placement concern. If the diff context is
misleading and it's already private, disregard.
Also noticed `MeterSmoother::leanThrottle()` (the static getter) doesn't
appear to be called anywhere in the PR — could be removed if it has no
external consumer.
Follow-up item for a future issue / PR:
@ten9876 noted in the 2026-06-05 coordination comment that
`SpectrumWidget::setLeanMode()` doesn't call `update()` after
`markOverlayDirty()`, so the visual change on toggle waits for the next data
tick rather than firing immediately. `VfoWidget::setOpaqueMode()` handles this
correctly (calls `update()` + parent `update()`). A one-liner fix to
`SpectrumWidget::setLeanMode()` would make both setters consistent and the
Lean toggle feel snappy on low-activity bands. Not a correctness issue —
suggesting a small follow-up issue or PR rather than holding this one.
✅ Approving.
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
Summary
Adds a reversible, persisted "Lean" render mode — a checkable button in each panadapter's overlay-menu column — that bundles the render-cost reductions found during the #3283 profiling investigation into one app-wide toggle. Driven by
MainWindow::applyLeanMode, persisted asLeanModeinAppSettings, restored on startup.What Lean does
SpectrumWidget::setLeanMode— skip wallpaper texture (GPU + software paths), drop translucent FFT fill, cap spectrum/waterfall repaint to ~30 HzVfoWidget::setOpaqueMode— swap#VfoWidgetRootstylesheet to an opaque background so it composites as a cacheable opaque layerWaveApplet::setActive— hide the scope and drop its sample feedMeterSmoother::shouldRepaint— shared ~12 Hz gate; meters keep integrating ballistics at full rate but batch their repaints (wired into the comp/gate/de-ess GR meters + S-meter needle)Profiling result (honest)
Matched same-session A/B (identical band, meters open, idle):
Scope / review notes
Relates to #3283.
💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat