From a08b184481d7ae1e1539aed694210286b53d4a41 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 28 Sep 2026 03:42:34 +0200 Subject: [PATCH] fix(plot,render,gl): live streaming measurements, handed-back GL state 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. --- .gitignore | 3 + CMakeLists.txt | 9 +- .../specs/2026-09-27-plot-library-design.md | 44 ++++- include/plot/gl/api.hpp | 2 + include/plot/gl/handle.hpp | 5 +- include/plot/plot/detail/measure_count.hpp | 30 ++++ include/plot/plot/plot.hpp | 44 +++-- include/plot/render/renderer.hpp | 36 +++- include/plot/render/style.hpp | 8 +- src/plot/plot.cpp | 19 +- src/render/renderer.cpp | 60 ++++++- tests/CMakeLists.txt | 3 +- tests/test_measure.cpp | 41 +++-- tests/test_plot.cpp | 66 +++++++ tests/test_render_smoke.cpp | 162 ++++++++++++++++++ 15 files changed, 478 insertions(+), 54 deletions(-) create mode 100644 include/plot/plot/detail/measure_count.hpp diff --git a/.gitignore b/.gitignore index fd0be13..bfa809e 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,5 @@ .superpowers/ build/ +# Not this project's and not generated by it: a Solarus game-engine message that +# happens to sit in the repo root. +error.txt diff --git a/CMakeLists.txt b/CMakeLists.txt index 67583a1..e3ae60c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -78,9 +78,16 @@ if(PLOT_BUILD_TESTS) # and one fence wait at most, at the start of the frame -- are invisible in the # pixels, so a test reads counters instead (see # include/plot/render/detail/ring_stats.hpp). + # + # PLOT_COUNT_MEASURE_RECOMPUTES is the same for the measurement cache: whether + # `Plot::measurement()` recomputed or returned a cached result is invisible in + # the value it returns, so a test that has to tell a live cache from one frozen + # on a stale window counts re-evaluations (see + # include/plot/plot/detail/measure_count.hpp). target_compile_definitions(plot_core PRIVATE PLOT_PYRAMID_COUNT_COVERS PLOT_PYRAMID_COUNT_MERGES - PLOT_RING_STATS) + PLOT_RING_STATS + PLOT_COUNT_MEASURE_RECOMPUTES) enable_testing() add_subdirectory(tests) diff --git a/docs/superpowers/specs/2026-09-27-plot-library-design.md b/docs/superpowers/specs/2026-09-27-plot-library-design.md index 6901ecf..01611d6 100644 --- a/docs/superpowers/specs/2026-09-27-plot-library-design.md +++ b/docs/superpowers/specs/2026-09-27-plot-library-design.md @@ -127,14 +127,27 @@ samples, at every level, for every series that uses the measurement index. series rasterises ≈ `2 · viewport_width` vertices instead of 100M. - Buckets preserve extremes, so spikes are never dropped by decimation. -### 4.3 Hybrid line rasterisation +### 4.3 One rasteriser, two sampling levels -Chosen per trace, per frame, from observed samples-per-pixel: +Every trace, every frame, is drawn by one path: instanced anti-aliased quads. +One instance per segment, four corners expanded from `gl_VertexID` in the vertex +shader, a fragment shader that does distance-to-segment SDF for smooth edges and +joins. There is no second primitive anywhere in the renderer — in particular no +`GL_LINE_STRIP` path — so a sparse trace and a dense one differ in the geometry +*staged*, never in the call that draws it. -| samples/px | path | +What the samples-per-pixel threshold selects is therefore the gather level: + +| samples/px | geometry staged | |---|---| -| `< 1.5` | `GL_LINE_STRIP`, 1px, no AA — dense data hides aliasing; ~10× cheaper | -| `>= 1.5` | instanced anti-aliased quads: one instance per segment, 4 verts expanded in the vertex shader, fragment shader does distance-to-segment SDF for smooth edges and joins | +| `< dense_threshold_spp` (1.5) | the visible window's raw samples: a level-0 `Series::gather()`, a polyline through the window, one instance per sample pair | +| `>= dense_threshold_spp` (1.5) | the pyramid level whose buckets are ~1 per pixel (`pyramid().level_for(n, samples_per_pixel)`): a min/max envelope pair per bucket, so O(pixels) instances instead of O(samples) | + +`Style::dense_threshold_spp` is that threshold and nothing more: it picks the +gather level, changes no draw call, and the two sides of it are the same shader. +The dense side is where the work is saved (a 100M-sample window stages O(pixels) +vertices); the sparse side stages the window itself, which at ~1 sample per pixel +is ~1 instance per column anyway. A geometry-shader expansion path was rejected: poor performance and it is the one stage macOS cannot be relied on for. @@ -364,11 +377,24 @@ namespace input { ## 8. Error handling -- No exceptions cross the public boundary. Fallible operations return `Status` - or `bool` plus an out-parameter `Status*`. +- Library-originated failures never throw: every one of them returns `Status` + (or `bool` plus an out-parameter `Status*`), and no code path reports an error + by throwing. An allocation failure is the host's process running out of memory, + not a library error, and the entry points that allocate host memory -- + constructing a series, `Plot::add_*`, a frame that stages geometry, + `Renderer::dump_frame_ppm()` -- propagate `std::bad_alloc` from the container + that ran out. They are therefore not annotated `noexcept`; the entries that can + neither fail nor allocate are, so the surface says per entry point what it does + rather than claiming a blanket `noexcept`. - GL failures are surfaced as `ErrorCode::GLFailure` from `Renderer::init` and - recorded on the first occurrence per frame; `PLOT_GL_CHECK()` is a no-op in - release builds. + recorded for the frame: `Renderer::end_frame()` drains `glGetError()` in a loop + and sets the flag `last_frame_had_gl_error()` reports, so the first error cannot + hide the ones after it and a stale error cannot be attributed to a later frame. + The instance stores report their own refusals through the same flag: a storage, + map or fence the driver refuses drops that store to the orphan-and-refill + baseline (see §4.5) and marks the frame. There is no `PLOT_GL_CHECK()` macro and + no build-dependent error checking -- the drain runs in every build, and it is + the flag, not a macro, that a host observes. - Unsupported-but-optional capabilities (compute, persistent mapping) degrade silently by selecting the 3.3 path; unsupported-and-required ones return `ErrorCode::Unsupported` from `init`. diff --git a/include/plot/gl/api.hpp b/include/plot/gl/api.hpp index 92b7210..8d91a33 100644 --- a/include/plot/gl/api.hpp +++ b/include/plot/gl/api.hpp @@ -37,6 +37,7 @@ enum : GLenum { // Clear state. GL_COLOR_BUFFER_BIT = 0x00004000, + GL_COLOR_CLEAR_VALUE = 0x0C22, // Data types (unsigned short is the index/16-bit data type). GL_UNSIGNED_BYTE = 0x1401, @@ -116,6 +117,7 @@ enum : GLenum { X(const GLubyte*, glGetString, (GLenum)) \ X(const GLubyte*, glGetStringi, (GLenum, GLuint)) \ X(void, glGetIntegerv, (GLenum, GLint*)) \ + X(void, glGetFloatv, (GLenum, float*)) \ X(GLenum, glGetError, (void)) \ X(void, glFinish, (void)) \ X(void, glViewport, (GLint, GLint, GLsizei, GLsizei)) \ diff --git a/include/plot/gl/handle.hpp b/include/plot/gl/handle.hpp index 2f5d8dd..6deef26 100644 --- a/include/plot/gl/handle.hpp +++ b/include/plot/gl/handle.hpp @@ -172,7 +172,10 @@ class Texture final : public Handle { void upload_rgba(u32 width, u32 height, const void* pixels) const noexcept; /// Replaces the whole image with `width * height` bytes of single-channel - /// data (row padding is disabled, so any width is valid). + /// data (row padding is disabled, so any width is valid). It sets + /// `GL_UNPACK_ALIGNMENT` to 1 to do so and leaves it there -- a caller that + /// shares the context with other uploaders saves and restores it, as + /// `Renderer` does around its atlas and its frames. void upload_r8(u32 width, u32 height, const void* pixels) const noexcept; /// Binds to `unit` (e.g. `GL_TEXTURE0`). diff --git a/include/plot/plot/detail/measure_count.hpp b/include/plot/plot/detail/measure_count.hpp new file mode 100644 index 0000000..0dfdb0d --- /dev/null +++ b/include/plot/plot/detail/measure_count.hpp @@ -0,0 +1,30 @@ +#pragma once + +#ifdef PLOT_COUNT_MEASURE_RECOMPUTES + +#include + +#include + +namespace plot::detail { + +/// Test-only instrumentation. Counts the measurements `Plot::measurement()` has +/// actually re-evaluated since the counter was last reset. +/// +/// Whether a call was a cache hit is invisible in the result -- a measurement +/// frozen on an old window and a current one are both "a Measurement" -- so a +/// test that has to tell "the cache recomputed because the window slid" from +/// "the cache is stuck on the first saturated frame" counts re-evaluations. The +/// counter exists only when PLOT_COUNT_MEASURE_RECOMPUTES is defined (the test +/// build defines it), so every normal build carries no counter and no increment +/// on the measurement path. +/// +/// Not part of the public surface and not meant for production use. +inline std::atomic& measure_recomputes() { + static std::atomic count{0}; + return count; +} + +} // namespace plot::detail + +#endif // PLOT_COUNT_MEASURE_RECOMPUTES diff --git a/include/plot/plot/plot.hpp b/include/plot/plot/plot.hpp index e7b9966..3789216 100644 --- a/include/plot/plot/plot.hpp +++ b/include/plot/plot/plot.hpp @@ -68,17 +68,28 @@ class Plot { void set_viewport(Rect px) noexcept { viewport_ = px; } const Rect& viewport() const noexcept { return viewport_; } + /// Set from the renderer, which owns presentation: `Renderer::draw()` copies + /// its own `style()` in here every frame, so a style set on the plot is what a + /// plot drawn *without* a renderer lays out with -- a frame drawn by one always + /// uses the renderer's values. void set_style(const render::Style& s) noexcept { style_ = s; } const render::Style& style() const noexcept { return style_; } - /// The font layout() measures tick labels with. Borrowed, not owned, and may - /// be null -- then the label areas fall back to PlotLayout's defaults and no - /// string is measured. The caller (the renderer) owns the backend; Plot never - /// creates one and never touches GL. + /// The font layout() measures tick labels with. Borrowed, not owned, and may be + /// null -- then the label areas fall back to PlotLayout's defaults and no string + /// is measured: a plot with no renderer lays out with whatever the host sets + /// here, which is for consumers that draw the plot themselves. + /// + /// A plot drawn by a `Renderer` is the resolution the other way round: the + /// renderer draws the labels with its own bitmap font, so it installs that font + /// here on every `draw()` and the strips are measured with the metrics the + /// labels are actually drawn with. A backend installed before the first draw is + /// therefore replaced, with a debug-build diagnostic naming the mismatch. void set_text_backend(const render::TextBackend* text) noexcept { text_ = text; } - /// The backend layout() measures labels with, or null when none is set. The - /// renderer reads it to install its own font only when nobody else has. + /// The backend layout() measures labels with, or null when none is set. A + /// renderer replaces whatever it finds here with its own font (see + /// set_text_backend()); a plot nobody draws keeps it. const render::TextBackend* text_backend() const noexcept { return text_; } // --- construction ----------------------------------------------------- @@ -157,10 +168,13 @@ class Plot { void set_cursor_snap(u32 id, SnapMode mode); /// The measurement for `id`. Re-evaluated only when a cursor moved, when the - /// measure was added, or when its series changed (its pointer or its sample - /// count) -- never once per frame. An unknown id, a null-series trace, an - /// inverted cursor pair or an empty series yields `valid == false`. const - /// because recomputing fills a cache: nothing a caller can observe changes. + /// measure was added, or when the series underneath it changed: a different + /// series, a longer one, or a sliding window whose sample count stayed the same + /// (`x0()` is the third key -- see MeasureSlot). Never once per frame. A + /// saturated stream therefore keeps reporting live values with a stationary + /// cursor pair. An unknown id, a null-series trace, an inverted cursor pair or + /// an empty series yields `valid == false`. const because recomputing fills a + /// cache: nothing a caller can observe changes. const Measurement& measurement(u32 id) const; /// The cursor pair `id` was created over. The renderer needs it to place the @@ -203,9 +217,12 @@ class Plot { const Grid& grid() const noexcept { return grid_; } private: - /// One measure plus its cache. The dirty flag is what keeps measurement() - /// from re-evaluating every frame; `last_series`/`last_size` catch a trace - /// being pointed at a different (or grown) series without anyone telling us. + /// One measure plus its cache. The dirty flag is what keeps measurement() from + /// re-evaluating every frame; `last_series`, `last_size` and `last_x0` catch a + /// trace being pointed at a different series, a series that grew, and a ring + /// whose window slid under a constant sample count -- the last of which neither + /// of the other two sees, since a saturated stream keeps its size while every + /// sample it holds changes. struct MeasureSlot { u32 cursor_a = 0; u32 cursor_b = 0; @@ -214,6 +231,7 @@ class Plot { mutable bool dirty = true; mutable const data::Series* last_series = nullptr; mutable u64 last_size = 0; + mutable f64 last_x0 = 0.0; }; /// Fit one axis to its traces' bounds, leaving auto_scale alone. False when the diff --git a/include/plot/render/renderer.hpp b/include/plot/render/renderer.hpp index 557c4b6..cf39d40 100644 --- a/include/plot/render/renderer.hpp +++ b/include/plot/render/renderer.hpp @@ -90,11 +90,13 @@ class Renderer { /// Draws `plot`, refreshing its layout, ticks and grid first. /// - /// The renderer owns the presentation style: `draw()` copies `style()` into the - /// plot, so the strips and tick densities the plot lays out with are the values - /// the renderer draws with -- set colours and metrics here, not on the plot. - /// The plot also borrows this renderer's font (when it has no backend of its - /// own), so the renderer must outlive the plot's use of it. + /// The renderer owns the presentation, style and text both: `draw()` copies + /// `style()` into the plot and installs its own font backend on it, so the strips + /// and tick densities the plot lays out with are the values the renderer draws + /// with -- set colours and metrics here, not on the plot, and expect a backend + /// set with `Plot::set_text_backend()` to be replaced on the first frame (a + /// debug build names the one it dropped). The renderer must therefore outlive the + /// plot's use of it. /// /// `Plot::layout()` is computed exactly once, here, and handed to every helper: /// it runs the auto scale, the strip fixed-point loop and the grid build, so @@ -112,6 +114,13 @@ class Renderer { /// vertex array 0, program 0, `GL_ARRAY_BUFFER` 0, texture unit 0 active and /// nothing bound to `GL_TEXTURE_2D` on it, blending off, scissoring off, and the /// viewport left at the frame's rect (the caller sets its own for the next pass). + /// Two pieces of global state the frame *changed* are handed back rather than + /// left where the frame wanted them: `GL_UNPACK_ALIGNMENT`, which `init()` set + /// to 1 for the glyph atlas' single-channel rows, and the clear colour, which is + /// `style().background` only for the duration of the frame. GL keeps no history + /// for the clear colour, so the host's value is queried on the first + /// `begin_frame()`; that first value is what comes back here, so a colour the + /// host changes after the first frame is restored over by it. /// The framebuffer binding is never touched -- it is the caller's, and so is the /// pixel-pack alignment if `dump_frame_ppm()` ran. void end_frame() noexcept; @@ -152,7 +161,13 @@ class Renderer { /// window. Reads the framebuffer that is bound now, which is the one the frame /// was drawn into. Returns false, with `st` filled in, on a bad argument or an /// unreadable pixel. - bool dump_frame_ppm(const char* path, Status* st = nullptr) const noexcept; + /// + /// The one entry point here that is not `noexcept`: it sizes its readback hole + /// and its output row, so an allocation failure propagates `std::bad_alloc` + /// rather than terminating a process that could have carried on without a + /// screenshot. A dump is a diagnostic, and no `Status` code could say more than + /// the exception already does. + bool dump_frame_ppm(const char* path, Status* st = nullptr) const; private: /// The bitmap font as the plot layer sees it, at the size the text pipeline @@ -250,6 +265,15 @@ class Renderer { std::vector gather_; ///< scratch for one gathered window, re-used mutable std::vector readback_; ///< dump_frame_ppm's row buffer + /// The host's `GL_UNPACK_ALIGNMENT`, saved by `init()` before the atlas upload + /// set it to 1 and handed back by `end_frame()`. GL's default is 4. + gl::GLint host_unpack_alignment_ = 4; + /// The host's `GL_COLOR_CLEAR_VALUE`, queried on the first `begin_frame()` (GL + /// has no other way to recover it once the frame has set it) and handed back by + /// `end_frame()`. + Color host_clear_{}; + bool host_clear_saved_ = false; + Rect viewport_; ///< the frame's region, top-left origin, framebuffer px Rect gl_viewport_; ///< the same rect flipped for GL, and for readback u32 fb_w_ = 0; diff --git a/include/plot/render/style.hpp b/include/plot/render/style.hpp index 78e7854..03b79bb 100644 --- a/include/plot/render/style.hpp +++ b/include/plot/render/style.hpp @@ -17,8 +17,12 @@ struct Style { f32 axis_font_px = 12.0f; f32 grid_label_pad = 4.0f; f32 line_width_px = 1.5f; - f32 dense_threshold_spp = 1.5f; ///< spec §4.3: samples per pixel above which - ///< a trace is drawn from the LOD pyramid. + f32 dense_threshold_spp = 1.5f; ///< spec §4.3: samples per pixel at or above + ///< which a trace is gathered from the LOD + ///< pyramid instead of from its raw samples. + ///< It selects the gather level, never a + ///< primitive: both sides draw through the one + ///< instanced anti-aliased quad path. f32 tick_min_px = 60.0f; ///< minimum spacing between major ticks. }; diff --git a/src/plot/plot.cpp b/src/plot/plot.cpp index ce0fa8d..4037060 100644 --- a/src/plot/plot.cpp +++ b/src/plot/plot.cpp @@ -1,4 +1,5 @@ #include +#include #include #include @@ -271,11 +272,20 @@ const Measurement& Plot::measurement(u32 id) const { const data::Series* series = nullptr; if (slot.trace < traces_.size()) series = traces_[slot.trace].series; const u64 samples = series != nullptr ? series->size() : 0; + // A saturated ring keeps `samples` constant while its window slides: x0() is + // what moves, and with it the sample each cursor snaps to and the interval they + // measure. Caching on (pointer, count) alone would freeze the readout at the + // first saturated frame until a cursor moved. + const f64 x0 = series != nullptr ? series->x0() : 0.0; - // Recompute only when someone moved a cursor, replaced the trace's series, or - // grew it. Everything else is a cache hit, which is what keeps a steady frame - // from re-evaluating every measure. - if (slot.dirty || series != slot.last_series || samples != slot.last_size) { + // Recompute only when someone moved a cursor, replaced the trace's series, grew + // it, or slid the window under it (x0 above). Everything else is a cache hit, + // which is what keeps a steady frame from re-evaluating every measure. + if (slot.dirty || series != slot.last_series || samples != slot.last_size || + x0 != slot.last_x0) { +#ifdef PLOT_COUNT_MEASURE_RECOMPUTES + detail::measure_recomputes().fetch_add(1, std::memory_order_relaxed); +#endif slot.result = Measurement{}; if (series != nullptr && samples > 0 && slot.cursor_a < cursors_.size() && slot.cursor_b < cursors_.size()) { @@ -287,6 +297,7 @@ const Measurement& Plot::measurement(u32 id) const { } slot.last_series = series; slot.last_size = samples; + slot.last_x0 = x0; slot.dirty = false; } return slot.result; diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index ff8bcbf..dcf5085 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -76,9 +76,21 @@ f32 walk_text_clipped(Vec2 pen, const char* utf8, f32 budget_px, const render::B return used; } -} // namespace +#ifndef NDEBUG +/// A host that installed its own `TextBackend` is told its metrics are gone: the +/// renderer draws every label with its artwork's font, so laying the strips out +/// with a different one puts the labels wherever that font's metrics happen to +/// fall. A diagnostic rather than an abort -- a host's frame must still draw -- +/// and debug-only, like `plot::gl::load()`'s unresolved-entry-point notice. +void warn_replaced_text_backend(const render::TextBackend* previous) noexcept { + std::fprintf(stderr, + "plot::Renderer::draw: replacing the Plot's TextBackend (%p) with the renderer's " + "own bitmap font; a plot drawn by a Renderer measures the labels it draws.\n", + static_cast(previous)); +} +#endif -// --------------------------------------------------------------- AxisFont +} // namespace void Renderer::AxisFont::reset(const render::BitmapFont& font, f32 requested_px) noexcept { font_ = &font; @@ -125,6 +137,12 @@ bool Renderer::init(Status* st) noexcept { font_backend_.reset(font_, style_.axis_font_px); + // The atlas upload needs GL_UNPACK_ALIGNMENT 1 (single-channel rows have no + // padding), which is the only global state `init()` changes; the host's value is + // read first so `end_frame()` can hand it back rather than leave the host's own + // uploads to the alignment this renderer wanted. + gl::glGetIntegerv(gl::GL_UNPACK_ALIGNMENT, &host_unpack_alignment_); + if (!atlas_.create_r8(render::BitmapFont::kAtlasWidth, render::BitmapFont::kAtlasHeight, st)) { return false; } @@ -182,6 +200,19 @@ void Renderer::begin_frame(u32 fb_w, u32 fb_h, Rect viewport_px) noexcept { // already painted keeps everything outside the rect. The scissor box is in // framebuffer coordinates and is not affected by glViewport, which is why the // clear happens before the viewport is set. + // + // The host's clear colour is queried once, before the first frame sets its own: + // GL keeps no history for it, so this is the only moment the value can be + // recovered for `end_frame()` to hand back. A host that changes its own clear + // colour *after* the first frame therefore gets the first frame's value restored + // -- the renderer cannot tell its own writes from the host's -- which is still + // the host's colour, and stated as such in end_frame()'s contract. + if (!host_clear_saved_) { + f32 rgba[4] = {0.0f, 0.0f, 0.0f, 0.0f}; + gl::glGetFloatv(gl::GL_COLOR_CLEAR_VALUE, rgba); + host_clear_ = Color{rgba[0], rgba[1], rgba[2], rgba[3]}; + host_clear_saved_ = true; + } gl::glEnable(gl::GL_SCISSOR_TEST); gl::glScissor(vx, vy, vw, vh); const Color& background = style_.background; @@ -206,9 +237,18 @@ void Renderer::draw(Plot& plot) { // measure with the size the text pipeline draws with, and re-read the style // each frame so a changed axis_font_px takes effect on the next one. font_backend_.reset(font_, style_.axis_font_px); - if (plot.text_backend() == nullptr) { - plot.set_text_backend(&font_backend_); +#ifndef NDEBUG + // The renderer owns the text of the frames it draws: every label goes through + // the text pipeline in `font_`, so the plot is handed this font's metrics + // whatever a host installed. A host backend is therefore replaced on the first + // draw -- said out loud in a debug build, since its disappearance is otherwise + // silent -- and a Plot nobody draws keeps it (see Plot::set_text_backend()). + if (const render::TextBackend* previous = plot.text_backend(); + previous != nullptr && previous != &font_backend_) { + warn_replaced_text_backend(previous); } +#endif + plot.set_text_backend(&font_backend_); // One style, not two: the plot reserves its strips and picks its tick density // from the values the renderer draws with, so the label pad is never one number // for the layout and another for the text placed inside it. @@ -245,8 +285,10 @@ void Renderer::end_frame() noexcept { gl_error_ = true; } // Leave the host's context as close to neutral as a frame can: nothing bound to - // a pipeline stage, and off every state bit the frame turned on. What remains - // is the viewport (the caller's to set for its next pass), the pixel-pack + // a pipeline stage, off every state bit the frame turned on, and the two values + // the frame *changed* rather than merely used handed back -- the unpack + // alignment `init()` set for the atlas, and the host's clear colour. What + // remains is the viewport (the caller's to set for its next pass), the pixel-pack // alignment a PPM dump may have set, and the vertex buffers' own contents. gl::glBindVertexArray(0); gl::glUseProgram(0); @@ -254,6 +296,10 @@ void Renderer::end_frame() noexcept { gl::glActiveTexture(gl::GL_TEXTURE0); gl::glBindTexture(gl::GL_TEXTURE_2D, 0); gl::glDisable(gl::GL_BLEND); + gl::glPixelStorei(gl::GL_UNPACK_ALIGNMENT, host_unpack_alignment_); + if (host_clear_saved_) { + gl::glClearColor(host_clear_.r, host_clear_.g, host_clear_.b, host_clear_.a); + } // Drain, do not peek: the first error hides every later one, and a frame that // produced several must be reported as having produced one. while (gl::glGetError() != gl::GL_NO_ERROR) { @@ -647,7 +693,7 @@ void Renderer::draw_annotations(const Plot& plot) { // -------------------------------------------------------------------- dump -bool Renderer::dump_frame_ppm(const char* path, Status* st) const noexcept { +bool Renderer::dump_frame_ppm(const char* path, Status* st) const { if (path == nullptr || path[0] == '\0') { return fail(st, ErrorCode::InvalidArgument, "PPM path is empty"); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 15d3798..2262367 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -69,7 +69,8 @@ target_include_directories(plot_tests PRIVATE "${CMAKE_SOURCE_DIR}/adapters") # behind them (see include/plot/data/detail/{cover,merge}_count.hpp). target_compile_definitions(plot_tests PRIVATE PLOT_PYRAMID_COUNT_COVERS PLOT_PYRAMID_COUNT_MERGES - PLOT_RING_STATS) + PLOT_RING_STATS + PLOT_COUNT_MEASURE_RECOMPUTES) plot_apply_warnings(plot_tests) add_test(NAME plot_tests COMMAND plot_tests) diff --git a/tests/test_measure.cpp b/tests/test_measure.cpp index b645758..f55711f 100644 --- a/tests/test_measure.cpp +++ b/tests/test_measure.cpp @@ -1,8 +1,9 @@ #include "harness.hpp" #include #include +#include #include -#include +#include #include #include @@ -25,15 +26,35 @@ PLOT_TEST(measurement_over_interval_matches_direct_computation) { PLOT_CHECK_NEAR(m.dx, 699.0, 1e-9); } PLOT_TEST(measurement_uses_pyramid_not_scan) { - // Guard against a future regression that linear-scans the series: a huge - // series must evaluate a 1-sample-wide interval in well under a millisecond. - std::vector y(1u << 22, 1.0f); // 4M samples - plot::data::ArraySeries s(std::move(y), 0.0, 1.0); - const auto t0 = std::chrono::steady_clock::now(); - for (int i = 0; i < 1000; ++i) (void)plot::Measure::evaluate(s, 1'000'000, 1'000'001); - const auto us = std::chrono::duration_cast( - std::chrono::steady_clock::now() - t0).count(); - PLOT_CHECK(us < 50'000); // 1000 evals in < 50 ms + // Guard against a future regression that linear-scans the series, counted rather + // than timed: a query's cost is the pyramid's covering decomposition -- at most + // two entries per level -- so the entry count must depend on the *interval* and + // never on the series length. The wall-clock bound this replaced (1000 evals in + // < 50 ms) failed on a loaded machine and passed vacuously on a fast one, while + // saying nothing about the shape of the work. + std::vector big(1u << 22, 1.0f); // 4M samples + plot::data::ArraySeries s_big(std::move(big), 0.0, 1.0); + std::vector small(1u << 13, 1.0f); // 8k samples: 512x shorter + plot::data::ArraySeries s_small(std::move(small), 0.0, 1.0); + + auto covers = [](const plot::data::Series& s, u64 begin, u64 end) { + plot::data::detail::cover_entry_count().store(0, std::memory_order_relaxed); + (void)plot::Measure::evaluate(s, begin, end); + return plot::data::detail::cover_entry_count().load(std::memory_order_relaxed); + }; + + // The same absolute interval over both series walks the same run of level-0 + // buckets, so it must fold the same entries: a decomposition that degenerated + // with series length -- one bucket at a time, or a scan -- shows up here. + const u64 big_covers = covers(s_big, 1024, 7000); + PLOT_CHECK(big_covers == covers(s_small, 1024, 7000)); + PLOT_CHECK(big_covers > 0u); // the interior came from the pyramid + PLOT_CHECK(big_covers <= 32u); // ~2 entries per level, few levels here + + // A 1-sample interval needs no whole bucket at all: its answer is the two partial + // edge buckets a pyramid of complete buckets cannot cover (at most 2 * kBucket raw + // samples through copy_raw), so it folds nothing whatever the series holds. + PLOT_CHECK(covers(s_big, 1'000'000, 1'000'001) == 0u); } PLOT_TEST(empty_and_single_sample_intervals_are_valid_false) { std::vector y{3.0f}; diff --git a/tests/test_plot.cpp b/tests/test_plot.cpp index b79f117..0668048 100644 --- a/tests/test_plot.cpp +++ b/tests/test_plot.cpp @@ -1,5 +1,6 @@ #include "harness.hpp" #include +#include #include #include #include @@ -310,6 +311,71 @@ PLOT_TEST(measurement_recomputes_when_the_series_grows) { PLOT_CHECK_NEAR(p.measurement(m).dx, 18.0, 1e-9); } +/// A saturated ring pins size() at capacity while x0() advances every acquire(), +/// so a cache keyed on (pointer, sample count) alone would freeze this measurement +/// on the first saturated frame -- the one case the demos never reach, because they +/// move their cursors (and therefore `dirty`) every frame. +/// +/// The signal is a ramp in absolute x, so the value a cursor names once the window +/// has slid past it is the current window's oldest sample: a live cache tracks +/// x0(), a frozen one keeps reporting the first frame's numbers. The counter is the +/// other half of the contract -- a frame in which nothing moved is still a cache +/// hit, or the fix would have traded a stale readout for a per-frame recompute. +PLOT_TEST(saturated_stream_measurement_tracks_the_sliding_window) { + constexpr u64 kCapacity = 64; + plot::data::StreamSeries s(kCapacity, 0.0, 1.0); + for (u64 i = 0; i < kCapacity; ++i) s.push(f32(i)); // y == absolute x + s.acquire(); + PLOT_CHECK(s.size() == kCapacity); // published window is full + + plot::Plot p; + p.set_viewport({0.0f, 0.0f, 800.0f, 400.0f}); + const u32 ax = p.add_x_axis({"t", {0.0, 100.0}, false, plot::AxisPlace::Bottom, false}); + const u32 ay = p.add_y_axis({"v", {0.0, 100.0}, false, plot::AxisPlace::Left, false}); + p.add_trace({&s, plot::Color::hex(0xFFFFFF), 1.0f, ax, ay}); + const u32 a = p.add_cursor(10.0, plot::Color::hex(0xFFB020)); + const u32 b = p.add_cursor(60.0, plot::Color::hex(0xFFB020)); + const u32 m = p.add_measure(a, b); + + // First window, both cursors inside it: y over absolute x 10..60. + PLOT_CHECK(p.measurement(m).valid); + PLOT_CHECK_NEAR(p.measurement(m).ymin, 10.0, 1e-9); + PLOT_CHECK_NEAR(p.measurement(m).ymax, 60.0, 1e-9); + PLOT_CHECK_NEAR(p.measurement(m).mean, 35.0, 1e-9); + PLOT_CHECK_NEAR(p.measurement(m).dx, 50.0, 1e-9); + + auto counter = [] { + return plot::detail::measure_recomputes().load(std::memory_order_relaxed); + }; + plot::detail::measure_recomputes().store(0, std::memory_order_relaxed); + // Neither cursor moves and neither does the window: eight reads, no recompute. + for (int i = 0; i < 8; ++i) (void)p.measurement(m); + PLOT_CHECK(counter() == 0u); + + // Slide the window past cursor `a` -- 40 more samples and an acquire, with the + // sample count unchanged (still capacity) and neither cursor touched. + for (u64 i = kCapacity; i < kCapacity + 40; ++i) s.push(f32(i)); + s.acquire(); + PLOT_CHECK(s.size() == kCapacity); // size() cannot be the key + PLOT_CHECK_NEAR(s.x0(), 40.0, 1e-9); // x0() moved under it + + const plot::Measurement& slid = p.measurement(m); // one acquire, one recompute + PLOT_CHECK(counter() == 1u); + PLOT_CHECK(slid.valid); + PLOT_CHECK_NEAR(slid.ymin, 40.0, 1e-9); // `a` clamped onto the new x0() + PLOT_CHECK_NEAR(slid.ymax, 60.0, 1e-9); + PLOT_CHECK_NEAR(slid.mean, 50.0, 1e-9); + PLOT_CHECK_NEAR(slid.dx, 20.0, 1e-9); + + // And it keeps tracking, one recompute per acquire with the cursors still. + for (u64 i = 0; i < 5; ++i) { + s.push(f32(kCapacity + 40 + static_cast(i))); + s.acquire(); + PLOT_CHECK_NEAR(p.measurement(m).ymin, 41.0 + static_cast(i), 1e-9); + PLOT_CHECK(counter() == 2u + i); + } +} + PLOT_TEST(inverted_and_out_of_range_measurements_are_invalid) { plot::data::ArraySeries s(ramp(10), 0.0, 1.0); plot::Plot p; diff --git a/tests/test_render_smoke.cpp b/tests/test_render_smoke.cpp index ff76fd5..23fb1d2 100644 --- a/tests/test_render_smoke.cpp +++ b/tests/test_render_smoke.cpp @@ -115,6 +115,50 @@ bool pixel_is(const plot::u8 px[4], plot::Color c) { return near(px[0], c.r) && near(px[1], c.g) && near(px[2], c.b); } +/// A host's `TextBackend` that measures one pixel per string, when the labels the +/// renderer draws are tens of pixels wide: installed on a plot, it asks for a label +/// strip far too narrow for anything the text pipeline draws. +class OnePxPerStringBackend final : public plot::render::TextBackend { + public: + plot::f32 measure(const char*) const override { return 1.0f; } + plot::f32 height() const override { return 1.0f; } + plot::f32 ascent() const override { return 1.0f; } +}; + +/// No column held ink. +constexpr plot::u32 kNoColumn = 0xFFFFFFFFu; + +/// What a text strip's pixels say: the leftmost column holding ink, and the widest +/// run of ink columns any single row holds -- one drawn label's extent, since a +/// label has a row band to itself. +struct TextInk { + plot::u32 leftmost = kNoColumn; + plot::u32 widest_row_run = 0; +}; + +/// Scans the top-down pixels of [x0, x1) x [y, y + rows) for fully covered text +/// pixels in `text`. +TextInk scan_text_ink(plottest::HeadlessContext& ctx, plot::u32 x0, plot::u32 x1, plot::u32 y, + plot::u32 rows, plot::Color text) { + TextInk ink; + for (plot::u32 r = 0; r < rows; ++r) { + plot::u32 first = kNoColumn; + plot::u32 last = 0; + for (plot::u32 x = x0; x < x1; ++x) { + plot::u8 px[4] = {}; + PLOT_CHECK(ctx.read_pixel(x, ctx.height - 1u - (y + r), px)); + if (!pixel_is(px, text)) continue; + if (first == kNoColumn) first = x; + last = x; + if (ink.leftmost == kNoColumn) ink.leftmost = x; + } + if (first != kNoColumn && last - first + 1u > ink.widest_row_run) { + ink.widest_row_run = last - first + 1u; + } + } + return ink; +} + /// Holds one GL entry point at a stub for as long as it lives. The entry points /// are the library's own function pointers (`plot/gl/api.hpp`), so a driver /// refusal -- a map or a fence that fails while the caps still advertise support @@ -336,6 +380,124 @@ PLOT_TEST(clears_only_the_viewport_region) { ctx.destroy(); } +// A host embeds the plot in its own frame loop, so the two pieces of global state +// the renderer has to change must come back as the host left them: the unpack +// alignment `init()` sets for the glyph atlas (whose single-channel rows are +// 1-byte aligned), and the clear colour the frame paints its background with. +PLOT_TEST(frame_hands_back_the_hosts_clear_colour_and_unpack_alignment) { + plottest::HeadlessContext ctx(kW, kH); + plot::Status st; + PLOT_CHECK(ctx.create(&st)); + PLOT_CHECK(plot::gl::load(nullptr)); + plot::Renderer r; + PLOT_CHECK(r.init(&st)); + + // The host's clear colour, set before the first frame. Nothing touches the + // unpack alignment: the context starts at GL's default of 4, and that is what + // the frame has to leave behind -- with the atlas' 1 still in place, a host's + // next padded RGBA upload would read the wrong rows. + plot::gl::glClearColor(0.5f, 0.25f, 0.75f, 1.0f); + + plot::data::ArraySeries s(sine_samples(), 0.0, 1.0); + plot::Plot p; + p.set_viewport({0, 0, static_cast(kW), static_cast(kH)}); + const plot::u32 ax = p.add_x_axis({"t", s.x_bounds(), true, plot::AxisPlace::Bottom, false}); + const plot::u32 ay = p.add_y_axis({"v", {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay}); + + r.begin_frame(kW, kH, {0, 0, static_cast(kW), static_cast(kH)}); + r.draw(p); + r.end_frame(); + PLOT_CHECK(!r.last_frame_had_gl_error()); + + plot::f32 clear[4] = {}; + plot::gl::glGetFloatv(plot::gl::GL_COLOR_CLEAR_VALUE, clear); + PLOT_CHECK_NEAR(clear[0], 0.5, 1e-6); + PLOT_CHECK_NEAR(clear[1], 0.25, 1e-6); + PLOT_CHECK_NEAR(clear[2], 0.75, 1e-6); + PLOT_CHECK_NEAR(clear[3], 1.0, 1e-6); + plot::gl::GLint alignment = 0; + plot::gl::glGetIntegerv(plot::gl::GL_UNPACK_ALIGNMENT, &alignment); + PLOT_CHECK(alignment == 4); + + // Not vacuous: the frame really did clear to its own background rather than the + // host's grey, so the value above is a restoration and not an untouched leftover. + plot::u8 px[4] = {}; + PLOT_CHECK(ctx.read_pixel(2, kH - 3u, px)); + PLOT_CHECK(px[0] + px[1] + px[2] < 200); + ctx.destroy(); +} + +// The renderer draws every label with its own bitmap font, so it has to lay the +// strips out with that font's metrics. A host backend whose metrics are nothing +// like the artwork's would otherwise reserve a strip too narrow for the labels the +// text pipeline then draws -- here, one pixel per string against ~40px labels. +PLOT_TEST(renderer_measures_the_labels_it_draws) { + plottest::HeadlessContext ctx(kW, kH); + plot::Status st; + PLOT_CHECK(ctx.create(&st)); + PLOT_CHECK(plot::gl::load(nullptr)); + plot::Renderer r; + PLOT_CHECK(r.init(&st)); + + plot::data::ArraySeries s(sine_samples(), 0.0, 1.0); + plot::Plot p; + // The sub-rect viewport the adapters use, so the label strip's left edge is not + // the framebuffer edge: with a narrower strip than the labels need, the renderer + // stages them left of the viewport, where GL's own clip removes all but their + // last few pixels. + const plot::Rect vp{60.0f, 0.0f, 500.0f, 300.0f}; + p.set_viewport(vp); + const plot::u32 ax = p.add_x_axis({"t", s.x_bounds(), true, plot::AxisPlace::Bottom, false}); + // No y title: the title strip sits left of the label strip, and this test reads + // the label strip's own columns for ink. + const plot::u32 ay = p.add_y_axis({"", {-1000.0, 1000.0}, false, plot::AxisPlace::Left, false}); + p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay}); + OnePxPerStringBackend host_font; + p.set_text_backend(&host_font); + // With the host's metrics the strip is 1px of label: what the frame must replace. + PLOT_CHECK_NEAR(p.layout().y_label_area.w, 1.0f + 2.0f * p.style().grid_label_pad, 1e-6); + + plot::gl::glClearColor(0.0f, 0.0f, 0.0f, 1.0f); + plot::gl::glClear(plot::gl::GL_COLOR_BUFFER_BIT); + r.begin_frame(kW, kH, vp); + r.draw(p); + r.end_frame(); + PLOT_CHECK(!r.last_frame_had_gl_error()); + + // Layout and drawing measure with one backend: the renderer's font, at the size + // the text pipeline draws (whole multiples of the glyph cell -- see AxisFont). + const plot::render::TextBackend* back = p.text_backend(); + PLOT_CHECK(back != nullptr); + PLOT_CHECK(back != &host_font); + PLOT_CHECK_NEAR(back->height(), r.axis_font_px(), 1e-6); + plot::f32 widest = 0.0f; + for (const plot::Tick& tick : p.y_axis(ay).ticks) { + if (!tick.major || tick.label.empty()) continue; + widest = std::max(widest, back->measure(tick.label.c_str())); + } + PLOT_CHECK(widest > 8.0f); // real labels in the real font, not the host's 1px + const plot::Rect labels = p.layout().y_label_area; + PLOT_CHECK_NEAR(labels.w, widest + 2.0f * p.style().grid_label_pad, 1e-3); + + // And the pixels agree with the strip: the drawn labels occupy the width the + // layout reserved and stay inside it. Kept on the host's 1px metrics the strip is + // ~9px wide while the renderer right-aligns its ~40px label against it, so all + // but the label's last few pixels land left of the viewport and are clipped -- + // the ink run collapses instead of filling the width that was reserved. + const plot::Color text = r.style().axis_text; + const plot::u32 y = static_cast(labels.y); + const plot::u32 rows = static_cast(labels.h); + const plot::u32 strip_x = static_cast(labels.x); + const TextInk ink = + scan_text_ink(ctx, strip_x, strip_x + static_cast(labels.w) + 16u, y, rows, text); + PLOT_CHECK(ink.leftmost != kNoColumn); // labels were drawn + PLOT_CHECK(ink.leftmost >= strip_x); // inside their strip + PLOT_CHECK(static_cast(ink.widest_row_run) >= 0.6f * widest); // full width drawn + PLOT_CHECK(static_cast(ink.widest_row_run) <= widest + 1.0f); // and no wider + ctx.destroy(); +} + // The overlay layer: cursor markers in their own colours, the shaded measurement // band and its readout, and an annotation pin. These are the pixels the adapters' // demos are read for, and none of them goes through the trace path.