Review of the atlas refactor found one Important and three Minor issues:
- Important: TextAtlas::sync called glGetIntegerv before any available() check,
so a host that synced before plot::gl::load() -- which is legal for a public
class Tasks 2 and 8 build backends on -- segfaulted on a null entry point
instead of receiving the Status the interface promises. It now follows the
handle-constructor convention (available(), then the entry point itself) and
reports Unsupported.
- empty() no longer means "a GL name exists": releasing one is a
glDeleteTextures call, so the no-atlas path forgets the atlas and only resets
the texture when there is a table to release it with.
- end_frame's comment still claimed the frame hands back the unpack alignment
init() set, contradicting the code below it and renderer.hpp.
- The orientation case's texel probe mapped the atlas' u along the cell's +v
axis and vice versa, which is a no-op for its two diagonal probes -- so it
could not fail on a transposed UV mapping, and its own comment misstated the
rule. Probes now follow the shader's rule and include an off-diagonal pair.
- The null-glyph path of add_text (and its twin in walk_text_clipped) was
unreached by any case.
Cases: text_atlas_reports_a_status_when_the_entry_points_are_missing,
add_text_skips_a_codepoint_the_backend_cannot_draw.
The text pipeline stages a glyph's rect *origin* per instance and carries the
rect's *size* once per draw, so a backend whose glyphs have differing rect sizes
-- a TrueType packing trimmed to ink, which is Task 8's shape -- would have drawn
every glyph at whichever size it happened to be told, with nothing downstream
able to tell. Make that constraint structural rather than documented:
- TextBackend::cell_uv() declares the one cell size, and the contract (an atlas
is a fixed-cell grid; a proportional face draws its glyphs *into* a uniform
cell instead of trimming each rect to its ink, and why the pipeline needs it)
is stated on the atlas accessors.
- glyph_is_cell() is the check; add_text()/set_cell() declare the cell from the
backend, and the staging path asserts it for every glyph in a debug build.
A release build draws on with the cell it was told, as before.
- Renderer::flush_text() takes u_glyph_uv from the backend, not from the batch,
so the draw's one rect size can never come from whichever glyph was staged
first.
Final whole-branch review fixes.
1 (Important). Plot::measurement() cached on (series pointer, sample count)
only, so a saturated StreamSeries -- whose size() stays pinned while x0()
advances every acquire() -- froze its readout on the first saturated frame
until a cursor moved. The cache key now carries the series' x0() (new
MeasureSlot::last_x0), so a sliding window recomputes and a static series
still does not: asserted with a new macro-gated counter hook
(plot/detail/measure_count.hpp, PLOT_COUNT_MEASURE_RECOMPUTES, wired like the
pyramid and ring counters, absent from the release library). New case
saturated_stream_measurement_tracks_the_sliding_window: saturated ring,
stationary cursors, ramp in absolute x -- zero recomputes for repeated reads
with nothing moving, one per acquire while it slides.
2-3, 9. Spec: 4.3 described a GL_LINE_STRIP path that does not exist (there is
one rasteriser -- instanced anti-aliased quads -- and dense_threshold_spp
selects the gather level, not a primitive); 8 named a PLOT_GL_CHECK() macro
that exists nowhere, and stated no exception policy. The table, the field's
comment and 8 now describe the real gather-level threshold, the real
glGetError() drain plus the stores' drop-to-orphan fallbacks, and the real
policy: library failures return Status/bool, allocating entry points propagate
std::bad_alloc.
4. The frame handed back neither GL_UNPACK_ALIGNMENT (init() sets 1 for the
atlas) nor the clear colour (queried once with a new glGetFloatv entry).
end_frame() restores both and renderer.hpp lists them in the residue
contract.
5. draw() kept a host TextBackend for layout while drawing with its own font.
The renderer now installs its font unconditionally -- strips and labels measure
and draw with one backend -- with a debug-build diagnostic if a different one
was set, and Plot::set_text_backend() is documented as the no-renderer path.
New case renderer_measures_the_labels_it_draws asserts layout, measuring and
the drawn label's ink agree.
6-10. draw()'s style overwrite stated at the call site; the load-sensitive
wall-clock bound in test_measure.cpp replaced by a counter assertion (same
interval, same fold count over a 512x length ratio); dump_frame_ppm drops
noexcept and says why it may allocate; error.txt ignored (not this project's,
not generated here) rather than deleted.
Verified: debug and release build clean (-Wall -Wextra -Wpedantic -Wconversion
-Wshadow), 4/4 ctest entries pass, 130 cases pass (was 127), umbrella and
header self-containment checks pass, and no counter symbol in the release
archive.
The persistent-mapped instance ring had three slots and recorded a fence per
upload, so a frame recycled a slot whose fence was recorded a few draws earlier
*in that same frame*: from the fourth of the demos' nine uploads on, each one
waited on the frame's own work. It is free on a driver that retires draws
promptly (the GLFW host, 1.07 ms/frame) and ~4 ms of the Qt widget's 7.11 ms
under llvmpipe, and it grows with the trace and cursor counts this library
exists for.
* The ring grows to cover a frame's upload count, the frame's write cursor is
the ring index (so a frame writes 0, 1, 2, ... and never a slot twice), one
fence is recorded after the frame's last draw, and the only wait is at the
start of the next frame. A wait that times out, or a slot written with no
fence ever recorded behind it, drops the store to the orphan path -- the
treatment a refused fence already had -- so nothing is ever written into
storage the GPU may still be reading.
* New test-only counters (PLOT_RING_STATS, the PLOT_PYRAMID_COUNT_* pattern:
free functions, no member, so no consumer's layout changes) pin a frame's
upload and wait counts. The new tests assert the frame's slot offsets are
distinct, that the ring grew past its floor to cover the frame, and that a
frame waits at most once per store -- read from the counters and from
glClientWaitSync itself.
* qt6: print the unresolved entry-point names before the load-failure branch,
which used to return first and leave the one failure this widget can have
reporting nothing but a generic message.
* qt6: the cached pointer anchor is now in logical pixels and a move's delta is
the logical difference scaled by the DPR in effect, so a scale change between
two move events (the window dragged onto another monitor mid-drag) is no
longer a cross-scale jump applied as a pan. The anchor moved into a
toolkit-free unit (adapters/qt6/qt_pointer.hpp) with its own test.
GLFW demo headless: 1.07 -> 0.73 ms/frame. Qt widget under xvfb/llvmpipe:
7.1 -> 3.8 ms at dpr 1.0, 6.7 -> 3.6 ms at 1.25, with both upload paths still
rendering and the demo's scripted pan/box-zoom checks passing at both scales.
Two findings on the pass-1 fix:
- The reserved strips were clamped only to the viewport, so a title wider than a
narrow panel took the rest of it and the axes collapsed: draw_traces() returned
on a zero-width rect (no trace drawn, nothing reported) and a zero-height one
flattened every trace onto a single row. The chrome is now capped at half the
viewport -- the label strip is served first, the title strip takes what is left
of that allowance -- an over-long title is clipped to its strip by a walk that
both measures and stages it, and draw_traces() tests the height as well as the
width.
- Every titled y axis was staged at the same centred position inside one strip, so
a Left/Right pair of titles overprinted. Each titled axis now owns a text line:
the x strip grows one line per titled x axis (x_title_height), and the y runs
stack down the axes-tall strip, which needs no extra width for it.
Tests: `narrow_panel_with_a_long_y_title_still_draws` and
`short_panel_with_an_x_title_still_draws` pin the floor (the axes keeps half the
panel and the trace still covers the columns, with the title clipped into its
strip), `titled_axis_pairs_get_their_own_rows` pins one text row per titled axis
in both directions, and `layout_stacks_one_title_line_per_titled_axis` pins the
strip sizing. All fail against the pass-1 sources.
Four review findings on the Task 7 renderer:
- InstanceStore::epoch() is derived from gl::Buffer::generation() -- advanced by
every allocation, deletion and move -- plus the slot the instances were written
to, so the orphan fallback of grow_persistent() can no longer leave the vertex
array's attribute pointers bound to a deleted buffer: the epoch reports the new
identity and upload() re-points them before the draw.
- PlotLayout reserves y_title_area/x_title_area outside axes (sized from the text
metrics) and draw_titles() draws in them, so a title can no longer land on the
data, whatever the envelope does.
- draw() computes plot.layout() once and hands it to the helpers, instead of six
times per frame, each of which rebuilt the ticks and the grid.
- A refused fence is no longer dropped: mark_drawn() falls back to the orphan path
and reports the failure rather than rotating through an unsynchronised slot.
New smoke cases force both refusals through the GL entry points (glMapBufferRange,
glFenceSync) and assert the pixels plus the live transfer mode; the title case
asserts the title colour is absent from the bands inside axes and present in both
strips. All three fail against the pre-fix sources.
Martino Ferrari