Fix ±1 px jitter on VFO/TNF/filter edges during tuning (#1272, #1369)#1703
Conversation
…r#1272, aethersdr#1369) The core cause was integer truncation in mhzToX(): for a fixed frequency, the pixel position (f - startMhz) / bw * width() lands at fractional values that round differently depending on the current pan centre. During continuous tuning or pan-follow animation this caused the same frequency to map to different pixel columns on successive frames, producing 1-2 px back-and-forth jitter on every vertical line (VFO marker, TNF notch edges, filter passband edges, grid lines). Fix: use std::round instead of implicit truncation in mhzToX(). This makes the mapping deterministic — a frequency is always assigned to its nearest pixel — eliminating the jitter. Secondary fixes: - Cursor frequency label: snap to exact VFO frequency when the cursor is within 8 px of a slice marker so the readout shows the flag value rather than a pixel-grid approximation. Applied to both the GPU (renderGpuFrame) and software (paintEvent) render paths; previously only the GPU path had this. (aethersdr#1369) - setSliceOverlayFreq(): add missing markOverlayDirty() call so callers get a repaint, and add an early-out for unchanged values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Review — PR #1703
Nice, clean fix. The root cause analysis is spot-on: static_cast<int> truncates toward zero, so the same frequency drifts between pixel columns as startMhz shifts during tuning. std::round is the right call here.
Changes look good
mhzToX → std::round — Correct fix. The clamp stays in place to guard against extreme values, and rounding happens outside the clamp, which is the right order (round(clamp(...))) since we want to round the already-bounded value. This fixes the jitter globally for every caller (VFO, TNF, filter edges, spots, grid lines, etc.) with a single-line change.
setSliceOverlayFreq — missing markOverlayDirty() — Good catch. Without this, callers that update the frequency through this path wouldn't trigger a repaint, so the overlay could show stale positions. The early-out on == is a reasonable optimisation given that frequency values come from the radio protocol at fixed precision.
Cursor frequency snap (both render paths) — The 8 px snap radius is sensible at typical zoom levels. The logic is duplicated between renderGpuFrame and paintEvent, which is consistent with how this codebase handles dual render paths.
Minor observations (non-blocking)
-
Floating-point
==in the early-out:so.freqMhz == freqMhzworks here because both values originate from the same radio protocol parsing (no arithmetic drift), but a brief comment noting that would help future readers understand why exact comparison is safe. -
Snap finds first match, not nearest: The cursor snap loop
breaks on the first slice overlay within 8 px. If two slices are very close together (within 16 px of each other), the snap target depends onm_sliceOverlaysordering rather than proximity. This is a minor edge case — at that zoom level both flags would be nearly on top of each other anyway — but if you wanted to be precise, you could track the minimum distance. Not worth blocking on. -
Overlap with PR #1699: This PR supersedes #1699 (which addresses only #1369 with the same cursor-snap approach). You may want to close #1699 to avoid merge conflicts.
Verdict
Scoped correctly to the stated problem, only touches SpectrumWidget.cpp, no convention issues, no resource leaks or null risks. LGTM — thanks for the well-documented fix, @Chaosuk97!
ten9876
left a comment
There was a problem hiding this comment.
Thanks Ian! Clean fix — rounding vs truncation is exactly right. Merged.
TNF UX rework contributed by Robbie Foust (rfoust). Applied from PR #1547 with the two non-TNF scopes stripped out and two Copilot review items addressed: - Stripped: overlay texture format change (RGBA8 → BGRA8 with fallback, QImage Format_RGBA8888_Premultiplied → Format_ARGB32_Premultiplied). Unrelated to TNF, affects every overlay rendering, no stated reason — revisit separately if a driver-specific need surfaces. - Kept: cursor-tracking refactor (`updateTrackedCursorState`). Intertwined with TNF hover detection (`m_hoveredTnfId`), extracting it would have required rewriting the hover logic. - Fixed: added `#include <algorithm>` to TnfModel.cpp for the newly-used `std::max` / `std::clamp` — relying on transitive includes is fragile. - Fixed: TNF context-menu header `QWidgetAction` is now `WA_TransparentForMouseEvents` + `setEnabled(false)` so clicking the freq/width info block no longer closes the menu. TNF changes preserved intact: - Markers stop at the bottom of the spectrum pane (no waterfall extend). - Hatched depth visualisation (12/8/5 px spacing for depth 1/2/3). - Floating `RF Tracking Notch` popup on hover showing freq + width. - Width/depth submenu checkmarks show current value. - Bidirectional TNF drag: horizontal tunes, vertical resizes via 2^(-dy/48) octaves, clamped 10-12000 Hz. - Preferred-TNF hit-testing for overlapping notches. - Tune guides suppressed while a TNF is hovered or dragged. - TnfModel: optimistic local updates in setters, width parser tolerates both Hz ("width=100") and fractional-MHz forms for firmware variance. Rebased onto current main (#1703 cursor-snap and #1701 filter-passband changes merged cleanly into rfoust's cursor-tracking refactor). Unaddressed follow-ups (intentional deferrals): - #1547 removes `QToolTip::hideText()` when the cursor leaves a spot label; stale tooltip could linger. Needs a small re-add. - `updateTrackedCursorState` is only called for spectrum-area hover; the waterfall path still leaves `m_cursorPos` / `m_hoveredTnfId` stale in the software paintEvent render path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The problem hasn't been solved. |
Problem
Vertical lines — VFO marker, TNF notch edges, filter passband edges — jitter by ±1 pixel during continuous frequency tuning (#1272) and the cursor frequency readout never exactly matches the VFO flag value (#1369).
Root cause:
mhzToX()used integer truncation (static_cast<int>(px)) instead of rounding. For a fixed frequency, the raw pixel value(f − startMhz) / bw × widthis a real number. During tuning or pan-follow animation,startMhzchanges continuously, which shifts the fractional part of that real number. Truncation maps the same frequency to different pixel columns on successive frames — producing 1–2 px back-and-forth jitter on every vertical element drawn withmhzToX.Fix
mhzToX()→std::round— a frequency is now always assigned to its nearest pixel column. This makes the mapping deterministic across frames and eliminates the jitter.This single-line change fixes all affected elements simultaneously: VFO center lines, TNF marker edges, filter passband edges, RIT/XIT lines, band-plan markers, spot markers, grid lines — anything that calls
mhzToX.Secondary changes
Cursor frequency snap (both render paths): when the cursor is within 8 px of a slice marker, the readout snaps to the exact VFO frequency rather than showing a pixel-grid approximation. The GPU path (
renderGpuFrame) already had this; the software path (paintEvent) was missing it (Cursor Calibration and Default Frequency Increment #1369).setSliceOverlayFreqcorrectness: added missingmarkOverlayDirty()so the overlay is redrawn when this function is called, and added an early-out for unchanged values.Verification
Closes #1272
Closes #1369
🤖 Generated with Claude Code