docs: a spec for the square wave's riser being drawn only sometimes
Two complaints, not one: the glitch is a row dropped before any style knob is consulted, and the thinness is a choice udpscope made and never revisited -- it asks deps/plotter for a trace thinner than the library's own default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
1 parent
723d1db3d2
commit
219ae91fd8
1 file changed
+353
@@ -0,0 +1,353 @@
|
||||
<!-- SPDX-License-Identifier: MIT -->
|
||||
<!-- SPDX-FileCopyrightText: 2026 Martino Ferrari -->
|
||||
|
||||
# A square wave's riser is sometimes drawn and sometimes not, and the trace is thinner than the library's own default
|
||||
|
||||
The report is that a vertical line — the rising or falling edge of `square` in
|
||||
`tools/mock_udpstreamer.py` — is too thin to see, and that it *glitches in and out of
|
||||
existence*. Those are two complaints, not one, and they have different causes. The second is a
|
||||
defect with a derivable mechanism and a single line of code behind it. The first is a styling
|
||||
choice this app made and then never revisited: udpscope asks for a trace **thinner** than
|
||||
`deps/plotter`'s own default.
|
||||
|
||||
The three remedies named in `todo.md` — grade with more strength, thicken the lines, enlarge the
|
||||
sample dots — are all taken. They are not, on their own, enough: the glitch is a row being
|
||||
dropped *before* any of those knobs is consulted, so a thicker, brighter trace would glitch in
|
||||
and out at a thicker, brighter width.
|
||||
|
||||
## The defect: `lit_floor` decides existence, and `intensity_floor` only decides alpha
|
||||
|
||||
`Renderer::stage_trace_density()` (`deps/plotter/src/render/renderer.cpp`) accumulates one
|
||||
column's dwell into `column_rows_`, where **a segment deposits one unit spread over the rows it
|
||||
spans** — `w = 1 / (r1 - r0 + 1)`. It then decides which rows are inked at all:
|
||||
|
||||
```cpp
|
||||
const f32 lit_floor = 0.01f * peak;
|
||||
...
|
||||
const bool lit = r < rows && cover > lit_floor;
|
||||
```
|
||||
|
||||
`peak` is the column's own maximum cover. That makes the existence test a *share of the
|
||||
brightest row in the same column*, and for a square wave the brightest row in a column is a
|
||||
flat level, whose segments each deposit a full 1.0 into one row.
|
||||
|
||||
Take a column that holds both a flat level and the edge. At ~105 samples a column the gather is
|
||||
on raw samples, so the column holds ~105 segments: all but one are flat and deposit a full 1.0
|
||||
into one row, while the riser is a single segment spread over the whole axis and deposits
|
||||
`1/312 = 0.0032` into each of 312 rows. `peak` is then ~50 and `lit_floor` ~0.5 — **over a
|
||||
hundred times the riser's own deposit**. Every row of the riser is unlit. The connector at the
|
||||
bottom of `emit_column()` does not rescue it either: the top run and the bottom run are both lit,
|
||||
so the column's `band_lo…band_hi` already spans the full height, neither gap test fires, and no
|
||||
connector is staged. The column is drawn as a bright mark at the top and a bright mark at the
|
||||
bottom with nothing between them.
|
||||
|
||||
In the phase where the riser lands in a column *by itself*, `peak` is the riser's own deposit,
|
||||
the floor is a hundredth of it, every row clears it, and the riser is drawn full height. Which
|
||||
case a frame lands in depends on the phase of the bucket grid against the pixel grid, and the
|
||||
trace scrolls, so the phase sweeps. That is the whole of "glitches in and out of existence" — not
|
||||
a redraw race and not a data gap.
|
||||
|
||||
### Measured, before writing any of this down
|
||||
|
||||
A true square wave (period 2098 samples, so ~20 columns a period), 60 000 x-units over the 572 px
|
||||
axis of `deps/plotter`'s own headless EGL harness — 104.8 samples a column — drawn twice, once
|
||||
with `intensity_grading` off and once on, with the pixels read back both times over the **same**
|
||||
axes rect. The reading counts the columns the *ungraded* frame inked as a full-amplitude band
|
||||
(the riser columns) and then asks how many of exactly those rows the graded frame left dark:
|
||||
|
||||
| | riser columns | inked rows over them | intact | gutted | worst dark run | segments |
|
||||
| --- | --- | --- | --- | --- | --- | --- |
|
||||
| ungraded | 116 | 35 884 | — | — | — | 627 |
|
||||
| graded, before | 116 | **2 916** | 10 | **106** | **311 of 312 rows** | 684 |
|
||||
| graded, after | 116 | 35 572 | 115 | 1 | — | 732 |
|
||||
|
||||
So the graded path was drawing **8 %** of the ungraded path's ink over the riser columns, and 106
|
||||
of 116 of them were gutted outright while 10 survived — the 10 being the phase that put a riser
|
||||
in a column of its own, which is the flicker, measured. The fix restores 99.1 % of the ink and
|
||||
costs 48 segments on a 572 px axis.
|
||||
|
||||
Two notes on that table, both of which the committed test must carry rather than smooth over:
|
||||
|
||||
- **The one column still gutted after the fix is column 11**, the second column of the window's
|
||||
first riser. The ungraded path inks *two* full-height columns per riser (its per-column min/max
|
||||
plus the connectivity extension, and the gathered window's slack sample) where the graded path
|
||||
inks one, which is also why 116 riser columns appear for ~58 risers. The graded path is the
|
||||
more precise of the two here; the test must assert against the riser's own column rather than
|
||||
against every column the flat path happened to fill.
|
||||
- **The existing grading test is unchanged by the fix** — `a_dense_trace_can_be_graded_by_dwell`
|
||||
still reads `flat edge=191 middle=191` against `graded edge=191 middle=131` at `seg=8303`, the
|
||||
documented figures to the digit, and all 376 cases pass. The fix only reaches rows that were
|
||||
below the share floor, which is what makes it a correction and not a restyling.
|
||||
|
||||
The app-level A/B this design originally proposed — `UDPSCOPE_NO_PHOSPHOR=1` against the mock's
|
||||
`square` — was dropped in favour of the above: udpscope has no probe that can see a dropped row,
|
||||
so the app-level run could only have shown that the picture differed, while the library harness
|
||||
reads the pixels and names the rows. The mechanism is measured at the layer it lives in.
|
||||
|
||||
`Style::intensity_floor`'s own documentation already promised what `lit_floor` broke:
|
||||
|
||||
> Not 0: a column the signal merely passed through is still a column the signal *reached*, and a
|
||||
> riser dimmed out of existence is a square wave drawn as two disconnected lines.
|
||||
|
||||
`intensity_floor` guards a lit row's **alpha**. Nothing guarded its **existence**, and the two
|
||||
floors were never reconciled.
|
||||
|
||||
### The fix, already applied and measured
|
||||
|
||||
`lit_floor` may not exceed the smallest deposit the column actually made, so that **a row one
|
||||
segment covers is always lit**. The column's minimum `w` is tracked as the deposits are written
|
||||
— one scalar, reset by `emit_column()`, no allocation, one `min` added to the per-segment path —
|
||||
and the floor becomes:
|
||||
|
||||
```cpp
|
||||
const f32 lit_floor = std::min(0.01f * peak, 0.5f * w_min);
|
||||
```
|
||||
|
||||
The share term stays: it is what stops a row inking on the running-sum reconstruction's residue
|
||||
alone. The second term is what makes the test *about a measurement* rather than about a ratio to
|
||||
an unrelated row. The half is slack against that reconstruction, not a tuning knob.
|
||||
|
||||
This changes no geometry bound — the runs are still maximal stretches of equal tier, O(rows) per
|
||||
column at worst — and it touches neither the gather level, the pyramid, nor
|
||||
`stage_trace_columns()`. It is in the working tree (`deps/plotter/src/render/renderer.cpp`) with
|
||||
the figures above in the comment at its site, which is what Task 2 below is reduced to reviewing.
|
||||
|
||||
## The second defect, found by testing the first: grading was gated on the column reduction
|
||||
|
||||
The `lit_floor` fix was built and run against the real app, and the report back was that the
|
||||
edges are now always visible — and that a new thing is wrong: *"when I zoom in on an edge the
|
||||
brightness increases a lot compared to when I am looking at the whole waveform."* Clarified: the
|
||||
dimming itself is wanted, **a discontinuity should never be drawn as brightly as a flat top**, at
|
||||
any zoom. So the complaint is not about the amount of dimming but about its *inconsistency*.
|
||||
|
||||
Measured in the same harness, the riser's own ink at mid-height (the rows neither flat level can
|
||||
reach) against the flat level's 191, over a zoom sweep from 101 samples a pixel down to 0.01:
|
||||
|
||||
| samples/pixel | 101 | 33.7 | 10.1 | 3.4 | 1.01 | 0.34 | 0.10 |
|
||||
| --- | --- | --- | --- | --- | --- | --- | --- |
|
||||
| riser ink, before | 71 | 71 | 71 | 71 | **191** | **191** | **191** |
|
||||
| riser ink, after | 71 | 71 | 71 | 71 | 71 | 71 | 68 |
|
||||
|
||||
A 2.7× jump in a single step, at `dense_threshold_spp = 1.5`. The cause is a coupling, not a
|
||||
constant: grading is staged *inside* `stage_trace_density()`, which only runs when the column
|
||||
reduction runs, and the reduction is gated on two gathered points per pixel column. That gate is
|
||||
a **density condition whose only purpose is removing the moiré beat** between the bucket pitch
|
||||
and the pixel pitch. Dwell has nothing to do with that beat, so zooming past the threshold
|
||||
switched the whole phosphor model off — and switched it off exactly where the user is looking at
|
||||
an edge closely, which is why it was reported as the edge *lighting up*.
|
||||
|
||||
### The fix: grade the polyline by each segment's slope
|
||||
|
||||
`Renderer::stage_trace_slopes()` stages the sparse polyline into the same `intensity_steps`
|
||||
batches, differing from the dense path only where it must:
|
||||
|
||||
- **The measure is `1 / (1 + |Δy_screen|)`** — dwell per row for a single segment. The `1 +` is
|
||||
not a division guard: a flat segment, span 0 and the commonest case in a sparse window, must be
|
||||
the *brightest* tier, and this makes the measure dimensionless and **absolute**. There is no
|
||||
per-column peak down here to normalise against, and that is an improvement — a sparse frame's
|
||||
tiers mean the same thing from frame to frame, so panning does not reshade the trace.
|
||||
- **No gather mode is excluded.** `grade` is not reused as the gate: it requires `Envelope`,
|
||||
because turning a bucket's min/max into a band answers a different question from the one a
|
||||
`Nearest` or `Mean` host asked. A segment's slope is a property of the polyline itself.
|
||||
- **The geometry is bit-for-bit `stage_xy()`'s**, same `snap = false`; only the batch differs. A
|
||||
segment with one non-finite end is dropped rather than clamped, which is the conclusion
|
||||
`add_polyline_xy()` reaches too.
|
||||
- **`FrameStats::slope_traces` is disjoint from `column_traces`**, so a host asserting
|
||||
`graded_traces == column_traces` keeps that reading unchanged.
|
||||
|
||||
Tier distribution sanity-checked by hand against the defaults (4 steps, gamma 0.45, floor 0.3):
|
||||
span 0 → tier 3, span 1 → 2, span 3 → 2, span 10 → 1, span 312 → 0. A gently sloping sparse sine
|
||||
therefore stays near the top tier and is not dimmed into invisibility, which was the risk.
|
||||
|
||||
### One false alarm, recorded because the metric was the bug
|
||||
|
||||
A first reading of this sweep showed the *frame's brightest pixel* falling from 191 to 167–172 on
|
||||
the sparse path whenever grading was on, which the arithmetic said was impossible (a flat segment
|
||||
is tier 3, alpha exactly 1.0). It was the measurement: the y range was −1…1, so both flat levels
|
||||
sat **on** the boundary rows and were half clipped, and the 191 the ungraded frame reported came
|
||||
from pixels near the riser where two segments overlap and double-composite — which grading splits
|
||||
across tiers, so they no longer saturate. With the range at −2…2 and the level read well clear of
|
||||
the riser, the level is 191 with grading on or off at every zoom. No regression existed. The
|
||||
committed test uses −2…2 and reads the level and the riser as two separate figures for this
|
||||
reason, stated at the site.
|
||||
|
||||
## The styling: the host asked for a thinner trace than the library's default
|
||||
|
||||
`RowSet::install()` (`src/plot_host.cpp:175`) builds every trace with `1.2f`. The library's
|
||||
`Trace::width` default is `1.5f`, and `Style::crosshair_width`'s documentation treats 1.5 as *the
|
||||
trace width a crosshair must read as thinner than*. So the app inverted its own stroke hierarchy
|
||||
at the only place it states it, and `1.2f` is also, in both staging paths, the **minimum span a
|
||||
short run is stroked at** — the `if (hi - lo < width)` widen — so it sets the floor on how
|
||||
visible a one-row run can ever be.
|
||||
|
||||
`marker_px` (3.0) and `marker_below_samples` (50) are library defaults udpscope never restated;
|
||||
`ui.cpp`'s one-shot style statement sets `intensity_grading` and nothing else.
|
||||
|
||||
## Scope
|
||||
|
||||
**In:**
|
||||
|
||||
- `deps/plotter`: the `lit_floor` correction in `stage_trace_density()`, a library test that fails
|
||||
without it, and the `PLOT_TEST_FILTER` the measurement needed.
|
||||
- `deps/plotter`: `Renderer::stage_trace_slopes()` — grading for the sparse polyline — plus
|
||||
`FrameStats::slope_traces` and a library test that fails without it.
|
||||
- `deps/plotter`: `Style::line_aa_px` and the grading constants remain *fields* — no new knob is
|
||||
added, and `intensity_grading` stays **one** switch covering both rasterisation paths. The one
|
||||
new field considered and rejected is named under **Out**.
|
||||
- udpscope: the trace width at `src/plot_host.cpp:175`, and the style one-shot in `ui.cpp`
|
||||
extended to state `marker_px`, `marker_below_samples`, `intensity_floor` and `line_aa_px`
|
||||
rather than inheriting them.
|
||||
- One new probe over the mock's `square`, and the `frame path:` reading extended to report the
|
||||
dropped-row case so a regression is visible rather than merely ugly.
|
||||
- `docs/claude/invariants-rendering.md`, `README.md`, and upstream's
|
||||
`docs/data-and-performance.md` + `README.md`.
|
||||
- `SPECS.md` is not changed.
|
||||
|
||||
**Out, and deliberately:**
|
||||
|
||||
- **A `Style` field for "always light a riser".** The dropped row is a bug in a test, not a
|
||||
policy a host should get to choose; a flag would preserve the broken behaviour as an option and
|
||||
oblige every host to know about it.
|
||||
- **Changing the connector's tier.** Drawing the inferred bridge at the faintest step is a
|
||||
deliberate honesty claim ("this is not a place the signal was measured to dwell"). Once
|
||||
`lit_floor` is fixed the riser is *real* ink at its own tier and the connector is back to
|
||||
bridging only genuine disjointness, which is what it was written for.
|
||||
- **Per-trace or per-row width.** One width for every trace in the app, as now.
|
||||
- **A separate switch for sparse grading.** Two switches would let a host reproduce the very
|
||||
inconsistency this fixes, and the user's own framing settles it: the dimming is wanted *at all
|
||||
zooms*, so it is one look and not two.
|
||||
- **Normalising the sparse tiers against the frame.** The dense path normalises per column
|
||||
because a frame-wide scale needs the whole image before anything can be drawn; the sparse
|
||||
measure needs no normalisation at all, and leaving it absolute is what makes a pan stop
|
||||
reshading the trace. Matching the dense path here would be matching a compromise.
|
||||
- **A user-facing preference for any of these values.** `todo.md` item 9 owns the preferences
|
||||
dialog; this work states defaults in code and leaves hooks for nothing.
|
||||
- No change to the gather level, `MinMaxPyramid`, `dense_threshold_spp`,
|
||||
`intensity_sub_columns`, the decimation plan, or the two-thread split.
|
||||
|
||||
## Global constraints
|
||||
|
||||
These bind every task below and are not restated in each one.
|
||||
|
||||
- **Every new source file carries the SPDX MIT header**, matching the existing files.
|
||||
- **`stage_trace_density()` stays allocation-free on the frame path.** `column_rows_` is already
|
||||
sized once per call and reused; the minimum-deposit tracking adds a scalar, not a container.
|
||||
- **No figure is written into a document unless it was measured on this machine.** The arithmetic
|
||||
above is derived and is labelled as derived; every number that reaches `README.md` or
|
||||
`docs/claude/` comes from a probe run quoted from that run.
|
||||
- **There is no unit test suite in udpscope.** Every assertion here is either a `deps/plotter`
|
||||
test (that project has one) or a probe line on stderr driven through the real widgets, read
|
||||
against the documented baselines: the documented build-warning count and zero `Gtk-CRITICAL`.
|
||||
- **Run one probe at a time.**
|
||||
- **The look is the user's call, the drop is not.** The `lit_floor` fix is correct or incorrect
|
||||
and a test says which. The four style values are judgments; they are stated in one place, with
|
||||
the before-value in a comment, so a second opinion is a one-line change.
|
||||
|
||||
## Task 0 — the test harness filter, already applied
|
||||
|
||||
`plottest::run_all()` ran all 376 cases unconditionally, so reading one case's output meant
|
||||
running the suite. `PLOT_TEST_FILTER` is a substring match honoured in `run_all()`
|
||||
(`deps/plotter/tests/harness.hpp`), unset meaning "all", which is what made the A/B above cheap
|
||||
enough to iterate on. Documented in upstream's `docs/building-and-testing.md`, including the trap
|
||||
that the summary line still counts the whole suite — so a filtered run says `377 cases, 0 failed`
|
||||
having run one, and is never evidence that the suite passes.
|
||||
|
||||
## Task 1 — the library test that fails now
|
||||
|
||||
Upstream, beside `a_dense_trace_can_be_graded_by_dwell`:
|
||||
`a_graded_column_keeps_a_riser_it_shares_with_a_flat_level`.
|
||||
|
||||
The scratch measurement above is the shape of it, and was deliberately **not** committed: it
|
||||
reports numbers rather than asserting, and it leans on the flat path as its reference, which the
|
||||
column-11 note shows is not exact. The committed test keeps the construction and replaces the
|
||||
reference.
|
||||
|
||||
The series and window are the measured ones — a true square of period 2098 over 60 000 x-units,
|
||||
~105 samples a column — because that density puts the gather on raw samples and makes a flat
|
||||
level's deposit a hundred times the riser's, which is the condition the defect needs. The riser
|
||||
must **share** its column, pinned by construction rather than by luck: it is the phase that
|
||||
survived in 10 of 116 columns, so a test that lets the phase float passes for the wrong reason
|
||||
about one time in twelve.
|
||||
|
||||
The assertion is *continuity over the riser's own column*, not a brightness profile — the
|
||||
existing grading test already profiles fifths and passes either way. Prefer the staged geometry
|
||||
over a pixel readback so the failure names the dropped rows rather than reporting that the
|
||||
picture changed, and have the failure print the gap's row range.
|
||||
|
||||
It must fail against the pre-fix `lit_floor`; verify that by reverting the one line, not by
|
||||
trusting the figures in this document.
|
||||
|
||||
**Done.** `a_graded_column_keeps_a_riser_it_shares_with_a_flat_level`, beside the dwell test.
|
||||
It chooses its riser columns by the property at stake — a band spanning more than half the axis,
|
||||
which on a square wave is a transition and nothing else — from the ungraded frame, then re-reads
|
||||
exactly those columns from the graded one and asserts 95 % of them are still tall. Reading
|
||||
`kept=115` of `flat=116`, and `kept=10` with `lit_floor` reverted to `0.01f * peak`, which is the
|
||||
required failure verified rather than assumed. The column-11 slack is in the assertion's 95 % and
|
||||
its reason is in the comment.
|
||||
|
||||
## Task 1b — the library test for slope grading
|
||||
|
||||
`a_sparse_trace_is_graded_by_slope_like_a_dense_one`: one window either side of
|
||||
`dense_threshold_spp` over the same transition at the same place, asserting that the two frames
|
||||
took the two paths (`graded_traces` against `slope_traces`), that **neither dims the level**
|
||||
(191 either way), that **both dim the riser** to under 55 % of it, and that the two riser
|
||||
readings agree to within one tier. Reading 71/191 against 71/191; with the slope staging gated
|
||||
off the sparse frame reads 191/191 and the path assertion fails first.
|
||||
|
||||
## Task 2 — review the fix
|
||||
|
||||
The change is in the tree. What is left is to read the comment at its site against the measured
|
||||
numbers it quotes, and to confirm `Style::intensity_floor`'s doc comment now describes reality,
|
||||
since it is cited above as having promised what the code did not deliver.
|
||||
|
||||
## Task 3 — the four stated values
|
||||
|
||||
`src/plot_host.cpp:175` from `1.2f` to at least the library's `1.5f`. The comment records that
|
||||
the app had been asking for less than the default and that this width is also the minimum span a
|
||||
short run is stroked at.
|
||||
|
||||
`ui.cpp`'s style one-shot (`src/ui.cpp:6149`) states, each with the inherited value in a comment:
|
||||
|
||||
- `marker_px` — `todo.md` 1.3, "the sample points should be bigger". The dots only appear below
|
||||
`marker_below_samples` raw samples in the window, so this is the deep-zoom reading and is
|
||||
independent of everything above.
|
||||
- `marker_below_samples` — raising it brings the dots back at windows that currently show a bare
|
||||
polyline.
|
||||
- `intensity_floor` and `line_aa_px` — the two readings of `todo.md` 1.1, "the antialiasing
|
||||
strategy with phosphore effect should be applied with more stregth", and they are different
|
||||
things: `intensity_floor` lifts the *faintest graded tier*, which is what a riser is drawn at
|
||||
even once it is lit; `line_aa_px` widens the anti-aliased fringe on **every** stroke in the
|
||||
library, which is literally more anti-aliasing and makes a thin line read fatter at the cost of
|
||||
a softer grid and axis. Both are stated; the values are the ones to look at on screen and
|
||||
adjust, and the task's report says what each one did on its own.
|
||||
|
||||
Nothing here is conditional on Task 2, and nothing in Task 2 is conditional on these.
|
||||
|
||||
## Task 4 — the probe
|
||||
|
||||
`UDPSCOPE_PROBE_EDGEINK_MS`: plot the mock's `square`, settle on a window dense enough for the
|
||||
graded path, and report — per the line reference in `docs/claude/probe-readings.md` — the edge
|
||||
column's staged span against the band it should cover, and the count of unlit rows inside it.
|
||||
**It must read 0 dropped rows**, and it must be run with and without `UDPSCOPE_NO_PHOSPHOR=1`:
|
||||
the ungraded path is the control, and the two must agree about where the trace is.
|
||||
|
||||
`frame path:` gains nothing new if the probe above covers the drop; the brief for this task
|
||||
decides that from the probe's output, and whichever way it goes,
|
||||
`docs/claude/probe-readings.md` gets the per-line entry.
|
||||
|
||||
## Task 5 — the documents
|
||||
|
||||
`docs/claude/invariants-rendering.md` already carries the grading invariant and the sentence
|
||||
"the floor so that a fast riser, one sub-bucket deep everywhere, is never dimmed out of
|
||||
existence". That is the claim this defect falsified, so the bullet gains the distinction the code
|
||||
was missing — **an alpha floor is not an existence floor** — and names the symptom, the phase
|
||||
dependence, and `UDPSCOPE_NO_PHOSPHOR=1` as the A/B that separates the two.
|
||||
|
||||
`README.md` records the stroke and marker values as deliberate deviations from the library's
|
||||
defaults, under the existing deviations section.
|
||||
|
||||
Upstream `docs/data-and-performance.md` and `README.md` carry the `lit_floor` rule and the slope
|
||||
path, since both are the library's contract and not this app's. **Done**, together with
|
||||
`Style::intensity_grading`'s own doc comment (which described the dense path as the whole of the
|
||||
feature), `docs/building-and-testing.md`'s `PLOT_TEST_FILTER` entry, and the case counts in all
|
||||
three places that quote them — 350 → 377, and 333 → 360 for the coverage preset.
|
||||
Reference in new issue
Block a user