From 29824b6919d10b6507fb84640f22094d4f8f90d4 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 28 Sep 2026 19:22:29 +0200 Subject: [PATCH] fix(plot,glfw): layer the crosshair under the handles, sync it across a figure, and harden the lookup --- adapters/glfw/glfw_host.cpp | 73 ++++++++++++++++++--- adapters/glfw/glfw_host.hpp | 35 +++++++++++ include/plot/plot/crosshair.hpp | 2 +- src/plot/plot.cpp | 28 ++++++--- tests/test_render_smoke.cpp | 108 +++++++++++++++++++------------- 5 files changed, 183 insertions(+), 63 deletions(-) diff --git a/adapters/glfw/glfw_host.cpp b/adapters/glfw/glfw_host.cpp index 53d22ad..02ec4ac 100644 --- a/adapters/glfw/glfw_host.cpp +++ b/adapters/glfw/glfw_host.cpp @@ -215,6 +215,12 @@ bool Host::run(const RunConfig& cfg, const FrameFn& frame, Status* st) { for (u32 i = 0; i < row_controllers_.size(); ++i) { row_controllers_[i]->set_viewport(figure_->layout().rows[i]); } + // One hover, every row (spec §15): the crosshair the row controller under the pointer + // placed is applied to the whole figure here, after the pointers of this frame have been + // handled and before anything lays the rows out -- so every row's readout lists its own + // signals at the one x the pointer is over. In the run loop rather than in a callback, so a + // scripted run that injects its events straight into a row controller is synced too. + sync_figure_crosshair(); } else { controller_.set_viewport(viewport); // also Plot::set_viewport, then layout() } @@ -406,6 +412,10 @@ void Host::set_figure(Figure* figure) noexcept { figure_ = figure; row_controllers_.clear(); last_row_ = 0u; + // A new figure holds no crosshair this host has spread: the comparison in + // `sync_figure_crosshair()` starts from "nothing spread", which is the state its rows are in. + spread_active_ = false; + spread_x_ = 0.0; if (figure_ == nullptr) return; const u32 rows = figure_->row_count(); row_controllers_.reserve(rows); @@ -420,9 +430,20 @@ input::Controller* Host::row_controller(u32 row) noexcept { input::Controller& Host::controller_at(f32 x, f32 y) noexcept { if (figure_ == nullptr || row_controllers_.empty()) return controller_; - const FigureLayout& layout = figure_->layout(); - const std::vector& rows = layout.rows; - if (rows.empty() || rows.size() != row_controllers_.size()) return controller_; + const u32 row = row_at(x, y); + if (row >= row_controllers_.size()) return controller_; + last_row_ = row; + input::Controller& controller = *row_controllers_[row]; + // The row's own viewport, so the geometry this controller hit-tests is the geometry + // the last frame drew: the same rect `Figure::layout()` gave the row's plot. + controller.set_viewport(figure_->layout().rows[row]); + return controller; +} + +u32 Host::row_at(f32 x, f32 y) const noexcept { + if (figure_ == nullptr || row_controllers_.empty()) return 0u; + const std::vector& rows = figure_->layout().rows; + if (rows.empty() || rows.size() != row_controllers_.size()) return 0u; // The row the pixel is in, or -- when it is in none of them, which is the row gap a // host has asked for with `Figure::set_row_gap()` or the figure's own margins -- the // row whose rect is nearest. Selection by distance rather than by "the first that @@ -442,12 +463,46 @@ input::Controller& Host::controller_at(f32 x, f32 y) noexcept { } } (void)x; // the rows tile the figure vertically, so y alone picks one - last_row_ = row; - input::Controller& controller = *row_controllers_[row]; - // The row's own viewport, so the geometry this controller hit-tests is the geometry - // the last frame drew: the same rect `Figure::layout()` gave the row's plot. - controller.set_viewport(rows[row]); - return controller; + return row; +} + +void Host::sync_figure_crosshair() noexcept { + const u32 rows = figure_ != nullptr ? figure_->row_count() : 0u; + if (rows == 0u || row_controllers_.empty()) return; + + // Which row's crosshair is the hover. With a pointer of the host's own it is the row that pointer + // is over -- the same rule events are routed by. A scripted run injects its events straight into + // a row controller and the host never sees a pointer, so there the hover is the row whose + // crosshair is *not* the one this host spread last: the figure writes one value to every row, so + // any row holding something else was changed from outside, and that is the one the run touched. + // Nothing differing means every row still holds the last spread -- no new hover -- and the figure + // keeps it. + u32 hovered = rows; + if (have_pointer_) { + hovered = row_at(last_pointer_x_, last_pointer_y_); + } else { + for (u32 i = 0; i < rows; ++i) { + const Crosshair& candidate = figure_->plot(i).crosshair(); + if (candidate.active != spread_active_ || (candidate.active && candidate.x != spread_x_)) { + hovered = i; + break; + } + } + if (hovered >= rows) return; + } + + // Applied to the whole figure: one x, every row. A row whose controller has *no* crosshair (the + // pointer left its axes, which is the only thing that clears one) clears the figure's, so the + // rows cannot be left showing a hover the pointer has abandoned. + const Plot& row = figure_->plot(hovered); + if (row.crosshair().active) { + figure_->set_crosshair(row.crosshair().x, true); + spread_x_ = row.crosshair().x; + spread_active_ = true; + } else { + figure_->clear_crosshair(); + spread_active_ = false; + } } } // namespace plot::glfw diff --git a/adapters/glfw/glfw_host.hpp b/adapters/glfw/glfw_host.hpp index 6f86947..cadab60 100644 --- a/adapters/glfw/glfw_host.hpp +++ b/adapters/glfw/glfw_host.hpp @@ -107,6 +107,12 @@ class Host { /// `Renderer::draw(Figure&)`. The frame callback is handed the figure's **row 0** /// as its `Plot&`, so `plot()` and the frame argument agree. /// + /// The figure's **crosshair** follows the pointer as one crosshair and not one per row: the run + /// loop reads the hover out of the row under the pointer and applies it to the whole figure + /// (`sync_figure_crosshair()`), so every row's readout lists its own signals at the one x the + /// pointer is over. That is what "synced if multiple plots available" means, and the host is the + /// layer that can do it -- it is the one that knows which row the pointer is in. + /// /// The figure is borrowed and must outlive the run. Passing null restores the /// single-plot loop, which is what every other caller uses; the host's own /// `plot_` is then untouched by any of this. @@ -168,6 +174,28 @@ class Host { void on_scroll(f64 dx, f64 dy) noexcept; void on_key(int key, int action, int mods) noexcept; + /// The index of the figure row a framebuffer pixel belongs to, by the rule + /// `controller_at()` routes by: the row the pixel is in, or the nearest row when it is in none + /// (a row gap, or a margin outside the figure). 0 while no figure is set. + u32 row_at(f32 x, f32 y) const noexcept; + + /// Spreads the hover over a whole `Figure` (spec §15). + /// + /// A figure's rows share one x, so the crosshair one row's controller placed is read back here and + /// applied to every row: the controller turns a pointer position into a data coordinate -- which + /// is its job, and the same one it does for a single plot -- and the figure spreads that one + /// coordinate over its rows. Reading it back rather than deriving a second coordinate here is what + /// keeps the hover the row shows and the hover the figure shows the same number. + /// + /// The row it reads is the one under the host's pointer, or -- with no pointer of the host's own, + /// which is how a scripted run injects its events -- the row whose crosshair is not the value this + /// host spread last (`spread_x_`/`spread_active_`), i.e. the row something outside changed. A row + /// whose controller has no crosshair clears the figure's. + /// + /// Called once per frame from the run loop, so a scripted run that bypasses the GLFW callbacks + /// gets the same synced figure a real pointer does. + void sync_figure_crosshair() noexcept; + Plot plot_; input::Controller controller_; ///< forwards its viewport to `plot_` std::optional renderer_; @@ -183,6 +211,13 @@ class Host { /// no position of its own, and the row the pointer is over is the one the user is /// looking at. u32 last_row_ = 0u; + /// The crosshair this host last spread over the figure (see + /// `sync_figure_crosshair()`): what a row is compared against to find the one a + /// scripted run touched, since the figure writes one value to every row. + /// `spread_active_` false means "spread nothing" -- the state before the first + /// sync, and the one a clear leaves. + f64 spread_x_ = 0.0; + bool spread_active_ = false; u32 fb_width_ = 0; ///< framebuffer size of the last refresh_sizes() u32 fb_height_ = 0; diff --git a/include/plot/plot/crosshair.hpp b/include/plot/plot/crosshair.hpp index 9a63b79..8fc4794 100644 --- a/include/plot/plot/crosshair.hpp +++ b/include/plot/plot/crosshair.hpp @@ -87,7 +87,7 @@ struct CrosshairSpan { /// The sample span `x` falls in, on `series`; see `CrosshairSpan` for the rule, for the /// interpolation convention it is half of, and for the cases that yield `valid == false`. /// -/// O(1) and allocation-free: a subtraction, a division and a floor. The series' own sample +/// O(1) and allocation-free: a subtraction, a division and a truncation. The series' own sample /// fetch is the caller's -- `Plot::crosshair_value()` reads the span through /// `Series::gather()` into a scratch buffer it owns, so no lookup on the pointer path /// allocates. diff --git a/src/plot/plot.cpp b/src/plot/plot.cpp index d30ff02..0d7c13a 100644 --- a/src/plot/plot.cpp +++ b/src/plot/plot.cpp @@ -1162,23 +1162,33 @@ CrosshairSpan crosshair_span(const data::Series& series, f64 x) noexcept { if (x < x0 || x > newest) return span; // Index arithmetic, not a search: the grid is uniform, so the sample at or before `x` is - // `floor((x - x0) / dx)` and there is nothing to look up. Two special cases keep the span - // honest at the ends and under rounding -- an index past the last sample collapses onto - // it, and a `t` that rounds up to 1.0 (a query one ulp above a sample) collapses onto the - // sample it has reached rather than reporting an interpolation of width zero. + // `floor((x - x0) / dx)` and there is nothing to look up. The two bounds are checked + // *before* the cast, which is also what keeps it defined: `index` is a quotient of two + // caller-supplied doubles and can overflow to infinity for an extreme origin and spacing, + // and casting that to `u64` is undefined. `!(index > 0.0)` is the NaN test as well. const f64 index = (x - x0) / dx; - const u64 i = index <= 0.0 ? 0u : static_cast(std::floor(index)); - if (i >= n - 1u) { - span.begin = span.end = n - 1u; + if (!(index > 0.0)) { + span.begin = span.end = 0u; // the oldest sample, or a rounding step below it span.t = 0.0; span.valid = true; return span; } + if (index >= static_cast(n - 1u)) { + span.begin = span.end = n - 1u; // the newest sample, or a step past it + span.t = 0.0; + span.valid = true; + return span; + } + // `index` is finite and inside `(0, n-1)`, so truncating it is a floor on a value a `u64` + // holds. Two rounding cases keep the span honest at a sample: a `t` that lands on 0 is the + // query sitting *on* sample `i`, and one that rounds up to 1.0 is it sitting on the sample + // after it -- neither is an interpolation with no width. + const u64 i = static_cast(index); const f64 t = index - static_cast(i); if (!(t > 0.0)) { - span.begin = span.end = i; // exactly on sample `i` + span.begin = span.end = i; } else if (t >= 1.0) { - span.begin = span.end = i + 1u; // exactly on the sample after it + span.begin = span.end = i + 1u; } else { span.begin = i; span.end = i + 1u; diff --git a/tests/test_render_smoke.cpp b/tests/test_render_smoke.cpp index 2f8c90a..bd652cb 100644 --- a/tests/test_render_smoke.cpp +++ b/tests/test_render_smoke.cpp @@ -4606,18 +4606,19 @@ bool orange_pixel(const plot::u8 px[4]) { /// /// * an **in-range** trace, whose line and marker are inside it -- the complement that says the /// clip is the axes rect and not something smaller; -/// * a trace **below the axis**: y = -0.05 on a [0, 1] axis maps some thirteen pixels under the -/// axes' bottom edge, i.e. *into the x label strip*. Nothing in the drawing path tests a -/// sample's y against the visible range, and a crosshair marker is deliberately staged where -/// its trace is drawn rather than skipped, so only Task 1's scissor keeps it off the strip; -/// * a trace **beside the axes**: one bound to a *second* x axis whose range starts past the -/// crosshair's x, so the data x under the pointer maps to a column *left* of the axes -- in -/// the y label strip. The marker is placed through the trace's own axis pair (which is what -/// puts it there, and what the row's geometry asserts as the premise), and again only the clip -/// keeps it out. +/// * a trace **below the axis**: its constant y is five percent under the axis' minimum, so it +/// maps some thirteen pixels beneath the axes' bottom edge -- *into the x label strip*. Nothing +/// in the drawing path tests a sample's y against the visible range, and a crosshair marker is +/// deliberately staged where its trace is drawn rather than skipped, so only Task 1's scissor +/// keeps that ink off the strip; +/// * a trace **beside the axes**: one bound to a *second* x axis whose window starts past the +/// crosshair's x, so the data x under the pointer maps to a column *left* of the axes -- in the +/// y label strip. The marker is placed through the trace's own axis pair (which is what puts it +/// there, and what the row's geometry asserts as the premise), and again only the clip keeps it +/// out. /// /// A build without the scissor puts marker ink in the x and the y label strip respectively, and -/// this case fails on both; the panel's own placement is asserted too, because a panel that +/// this case fails on both. The panel's own placement is asserted as well, because a panel that /// overhung `axes` would be the same defect by a different route. PLOT_TEST(crosshair_ink_stays_inside_the_axes) { plottest::HeadlessContext ctx(kW, kH); @@ -4632,6 +4633,45 @@ PLOT_TEST(crosshair_ink_stays_inside_the_axes) { // in a strip or on the host's own surface, both of which are asserted empty below. const plot::Rect vp{30.0f, 40.0f, 580.0f, 290.0f}; + plot::Plot p; + p.set_viewport(vp); + const plot::u32 ax = p.add_x_axis({"t", {0.0, 639.0}, false, plot::AxisPlace::Bottom, false}); + const plot::u32 ay = p.add_y_axis({"v", {0.0, 1.0}, false, plot::AxisPlace::Left, false}); + + plot::gl::glClearColor(0.5f, 0.5f, 0.5f, 1.0f); + plot::gl::glClear(plot::gl::GL_COLOR_BUFFER_BIT); + // A warm-up frame first, with nothing in the plot: the renderer installs its text backend on the + // first `draw()`, and the strips and the readout panel are measured with it -- so the geometry + // asserted below is a *drawn* frame's, not an empty plot's defaults. + r.begin_frame(kW, kH, vp); + r.draw(p); + r.end_frame(); + PLOT_CHECK(!r.last_frame_had_gl_error()); + + // Nothing added from here on can move the strips: an *untitled* second x axis draws its labels + // on the same strip's single line, and a trace reserves nothing at all. (A titled one would + // reserve a title row and move `axes` -- which is why this axis has no label.) + const plot::Rect axes = p.layout().axes; + const plot::Rect y_strip = p.layout().y_label_area; + PLOT_CHECK(y_strip.w > 4.0f); + const plot::Transform tx = p.x_transform(ax); + const plot::Transform ty = p.y_transform(ay); + const f64 hover_x = tx.to_data_x(axes.right() - 3.0f); + + // The second x axis' window is **derived from that geometry rather than guessed**, so this case + // cannot silently stop testing anything when a font or a padding moves the strip: with a span + // `S` starting at `min`, a data x maps to `axes.x + (x - min) / S * axes.w`, so starting the + // window half a strip's width above the hovered x puts that trace's marker halfway into the y + // label strip. + const f64 span2 = 700.0; + const f64 min2 = + hover_x + (0.5 * static_cast(y_strip.w) / static_cast(axes.w)) * span2; + const plot::u32 ax2 = + p.add_x_axis({"", {min2, min2 + span2}, false, plot::AxisPlace::Bottom, false}); + + // The geometry really is the one asserted below, and the second axis really did not move it. + PLOT_CHECK_NEAR(p.layout().axes.h, axes.h, 1e-4); + // The in-range signal: a wave inside the [0, 1] axis, so its trace and its marker are both // inside the rect. std::vector in_range(640u); @@ -4641,37 +4681,16 @@ PLOT_TEST(crosshair_ink_stays_inside_the_axes) { plot::data::ArraySeries good(in_range, 0.0, 1.0); // Five percent below the bottom of the axis: some thirteen pixels, which is the x label strip. plot::data::ArraySeries below(std::vector(640u, -0.05f), 0.0, 1.0); - // In range on y, but drawn through an x axis the crosshair's x is outside of: its marker lands - // in the y label strip, beside the axes. + // In range on y, but read through an x axis the hovered x is outside of: its marker lands in the + // y label strip, beside the axes. plot::data::ArraySeries aside(std::vector(2000u, 0.25f), 0.0, 1.0); - - plot::Plot p; - p.set_viewport(vp); - const plot::u32 ax = p.add_x_axis({"t", {0.0, 639.0}, false, plot::AxisPlace::Bottom, false}); - const plot::u32 ay = p.add_y_axis({"v", {0.0, 1.0}, false, plot::AxisPlace::Left, false}); - // The second x axis: a window that starts *above* the crosshair's x, so the same data x is a - // column to the left of the axes when the trace reads it through this one. - const plot::u32 ax2 = p.add_x_axis({"t2", {700.0, 1400.0}, false, plot::AxisPlace::Bottom, false}); p.add_trace({&good, kRowMagenta, 1.5f, ax, ay, "good"}); p.add_trace({&below, plot::Color::hex(0x00E070), 1.5f, ax, ay, "below"}); p.add_trace({&aside, plot::Color::hex(0xFFB020), 1.5f, ax2, ay, "aside"}); - plot::gl::glClearColor(0.5f, 0.5f, 0.5f, 1.0f); - plot::gl::glClear(plot::gl::GL_COLOR_BUFFER_BIT); - // A warm-up frame first: the renderer installs its text backend on the first `draw()`, and the - // strips and the panel are measured with it -- so the frame under test is placed and asserted - // against the rects a *drawn* frame has, not an empty plot's defaults. - r.begin_frame(kW, kH, vp); - r.draw(p); - r.end_frame(); - PLOT_CHECK(!r.last_frame_had_gl_error()); - // The crosshair three pixels inside the bottom-right corner of the data area, so its readout // panel would leave the rect on two sides if it were not flipped and clamped. - const plot::Rect axes = p.layout().axes; - const plot::Transform tx = p.x_transform(ax); - const plot::Transform ty = p.y_transform(ay); - p.set_crosshair(tx.to_data_x(axes.right() - 3.0f), ty.to_data_y(axes.bottom() - 3.0f), true); + p.set_crosshair(hover_x, ty.to_data_y(axes.bottom() - 3.0f), true); r.begin_frame(kW, kH, vp); r.draw(p); @@ -4694,9 +4713,10 @@ PLOT_TEST(crosshair_ink_stays_inside_the_axes) { PLOT_CHECK(crosshair.panel.right() <= axes.right()); PLOT_CHECK(crosshair.panel.bottom() <= axes.bottom()); - // The premises: the two off-scale markers are staged where nothing but the clip is between them - // and a strip. Row 1's is below the axes' bottom edge, within the x label strip; row 2's is left - // of the axes' left edge, within the y label strip. + // The premises, which is what makes every pixel assertion below a statement about the defect and + // not about a marker that never went anywhere: the two off-scale markers are staged where only + // the clip stands between them and a strip -- row 1's below the axes' bottom edge, inside the x + // label strip, row 2's left of the axes' left edge, inside the y label strip. PLOT_CHECK(crosshair.rows[0].valid); PLOT_CHECK(crosshair.rows[1].valid); PLOT_CHECK(crosshair.rows[2].valid); @@ -4705,8 +4725,8 @@ PLOT_TEST(crosshair_ink_stays_inside_the_axes) { PLOT_CHECK(crosshair.rows[2].marker.x < axes.x); PLOT_CHECK(crosshair.rows[2].marker.x >= layout.y_label_area.x); - // Every crosshair-coloured pixel of the frame is inside `axes`, and there are some: not one in - // a label or title strip, not one over the host's surface outside the viewport. + // Every crosshair-coloured pixel of the frame is inside `axes`, and there are some: not one in a + // label or title strip, not one over the host's surface outside the viewport. const plot::u32 inside = predicate_pixels(frame, kW, axes, crosshair_pixel); PLOT_CHECK(inside > 0u); PLOT_CHECK(predicate_pixels(frame, kW, kFrameRect, crosshair_pixel) == inside); @@ -4723,21 +4743,21 @@ PLOT_TEST(crosshair_ink_stays_inside_the_axes) { // The trace below the axis: none of its colour anywhere outside `axes` -- the x label strip and // the host's surface included -- while its own swatch in the readout proves the colour is one - // this frame really draws (so the absence is the clip's doing and not a colour nothing draws). + // this frame really draws, so the absence is the clip's doing and not a colour nothing paints. PLOT_CHECK(predicate_pixels(frame, kW, layout.x_label_area, green_pixel) == 0u); PLOT_CHECK(predicate_pixels(frame, kW, kFrameRect, green_pixel) == predicate_pixels(frame, kW, axes, green_pixel)); PLOT_CHECK(predicate_pixels(frame, kW, crosshair.rows[1].swatch, green_pixel) > 0u); - // The trace beside the axes: the same statement for the y label strip, where its marker would - // have been drawn. + // The trace beside the axes: the same statement for the y label strip and the title strip beside + // it, where its marker would have been drawn. PLOT_CHECK(predicate_pixels(frame, kW, layout.y_label_area, orange_pixel) == 0u); PLOT_CHECK(predicate_pixels(frame, kW, layout.y_title_area, orange_pixel) == 0u); PLOT_CHECK(predicate_pixels(frame, kW, kFrameRect, orange_pixel) == predicate_pixels(frame, kW, axes, orange_pixel)); - // The complement: the in-range trace's ink -- its polyline and the marker the crosshair put on - // it -- is inside `axes`, and all of it is, which a clip smaller than the rect could not do. + // The complement: the in-range trace's ink -- its polyline and the marker the crosshair put on it + // -- is inside `axes`, and all of it is, which a clip smaller than the rect could not do. PLOT_CHECK(predicate_pixels(frame, kW, axes, magenta_pixel) > 0u); PLOT_CHECK(predicate_pixels(frame, kW, kFrameRect, magenta_pixel) == predicate_pixels(frame, kW, axes, magenta_pixel));