From a153b73ead93e8a768c99c379e170059e9d0b412 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 28 Sep 2026 17:58:42 +0200 Subject: [PATCH] fix(input): clamp a dragged ruler on an ordered y range and share the orientation test --- include/plot/plot/cursor.hpp | 11 +++++++++++ src/input/controller.cpp | 11 ++++++++--- src/plot/plot.cpp | 19 ++++++++++--------- src/render/renderer.cpp | 4 ++-- tests/test_controller.cpp | 31 +++++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+), 14 deletions(-) diff --git a/include/plot/plot/cursor.hpp b/include/plot/plot/cursor.hpp index 438d33c..2020edd 100644 --- a/include/plot/plot/cursor.hpp +++ b/include/plot/plot/cursor.hpp @@ -66,6 +66,17 @@ struct Cursor { u32 axis_x = 0; ///< the x axis `x` is in. Dead for `Horizontal`. u32 axis_y = 0; ///< the y axis `y` is in. Dead for `Vertical`. bool visible = true; + + /// True when this is a horizontal ruler (spans the axes' width, reads its `y`). The one + /// place the orientation becomes a branch, so every consumer takes the same decision and + /// the two orientations cannot drift apart as new ones are added. + bool is_horizontal() const noexcept { return orientation == CursorOrientation::Horizontal; } + + /// The live data coordinate: `y` for a horizontal ruler, `x` for a vertical marker. + f64 coordinate() const noexcept { return is_horizontal() ? y : x; } + + /// The axis `coordinate()` is in: `axis_y` for a horizontal ruler, `axis_x` for a vertical. + u32 coordinate_axis() const noexcept { return is_horizontal() ? axis_y : axis_x; } }; } // namespace plot diff --git a/src/input/controller.cpp b/src/input/controller.cpp index ae30b1a..539cab9 100644 --- a/src/input/controller.cpp +++ b/src/input/controller.cpp @@ -505,7 +505,7 @@ void Controller::move_cursor(const PointerEvent& e) { // A grabbed cursor mutates the coordinate its own orientation owns and nothing else, and // each half names the one setter for it -- so a ruler dragged along y can never disturb a // marker's x, and the dead coordinate of either is untouched by construction. - if (cursor.orientation == CursorOrientation::Horizontal) { + if (cursor.is_horizontal()) { // Clamped to the ruler's own y axis: a pointer that has left the axes has no data under // it, and the coordinate it extrapolates to is past the end of the axis the ruler reads // values off -- a position where the ruler would either vanish (the renderer skips a row @@ -514,7 +514,12 @@ void Controller::move_cursor(const PointerEvent& e) { // verbatim, so a *caller* that means to park a ruler off range still can. const Axis& axis = plot_->y_axis(cursor.axis_y); const f64 y = plot_->y_transform(cursor.axis_y).to_data_y(static_cast(e.y)); - plot_->set_cursor_y(id, std::clamp(y, axis.range.min, axis.range.max)); + // `std::clamp` needs an ordered pair and a y range may be inverted -- descending depth, + // row index or time is ordinary -- because `set_y_range()` stores it verbatim and only + // the transform widens it. Order the bounds rather than assuming `min <= max`, or an + // inverted axis would collapse the dragged ruler onto one end (and the call is UB). + plot_->set_cursor_y(id, std::clamp(y, std::min(axis.range.min, axis.range.max), + std::max(axis.range.min, axis.range.max))); return; } plot_->set_cursor_x(id, plot_->x_transform(cursor.axis_x).to_data_x(static_cast(e.x))); @@ -611,7 +616,7 @@ u64 Controller::cursor_at(f32 px, f32 py) const { // either way, which is what keeps hover and grab agreeing about a ruler exactly as // they do about a marker. A point within reach of one of each resolves to the nearer, // so neither orientation widens the other's target. - const bool horizontal = cursor.orientation == CursorOrientation::Horizontal; + const bool horizontal = cursor.is_horizontal(); const f32 d = std::fabs(plot_->cursor_screen_coordinate(i) - (horizontal ? py : px)); if (d > radius) continue; if (best == kInvalid || d < best_d) { diff --git a/src/plot/plot.cpp b/src/plot/plot.cpp index 179358a..5ec3065 100644 --- a/src/plot/plot.cpp +++ b/src/plot/plot.cpp @@ -239,13 +239,14 @@ const Cursor& Plot::cursor(u32 id) const { f32 Plot::cursor_screen_coordinate(u32 id) const { if (id >= cursors_.size()) return 0.0f; const Cursor& cursor = cursors_[id]; - // Which coordinate is the marker's, decided once, here: the axis reached for is the one - // the cursor's own orientation names, and the other orientation's coordinate is not + // Which coordinate is the marker's, decided once, in `Cursor`: the axis reached for is the + // one the cursor's own orientation names, and the other orientation's coordinate is not // consulted at all -- a ruler is never placed by a dead x. - if (cursor.orientation == CursorOrientation::Horizontal) { - return static_cast(y_transform(cursor.axis_y).to_screen_y(cursor.y)); + if (cursor.is_horizontal()) { + return static_cast( + y_transform(cursor.coordinate_axis()).to_screen_y(cursor.coordinate())); } - return static_cast(x_transform(cursor.axis_x).to_screen_x(cursor.x)); + return static_cast(x_transform(cursor.coordinate_axis()).to_screen_x(cursor.coordinate())); } const Annotation& Plot::annotation(u32 id) const { @@ -504,7 +505,7 @@ void Plot::set_cursor_x(u32 id, f64 x) { // `x` is the vertical cursor's coordinate and nobody else's: a ruler's `x` is dead, and // writing it would leave the struct holding two coordinates that disagree about where the // cursor is (see CursorOrientation). - if (cursor.orientation != CursorOrientation::Vertical) return; + if (cursor.is_horizontal()) return; cursor.x = x; cursor.x = snapped_x(cursor); mark_measures_dirty(); @@ -516,7 +517,7 @@ void Plot::set_cursor_snap(u32 id, SnapMode mode) { // The vertical snap, on a vertical cursor: a ruler's snap is `HRulerSnap`, and the two // must not drift into meaning each other's job -- `NearestSample` is a sample of an x // grid, which a ruler has no notion of. - if (cursor.orientation != CursorOrientation::Vertical) return; + if (cursor.is_horizontal()) return; cursor.snap = mode; cursor.x = snapped_x(cursor); mark_measures_dirty(); @@ -525,7 +526,7 @@ void Plot::set_cursor_snap(u32 id, SnapMode mode) { void Plot::set_cursor_y(u32 id, f64 y) { if (id >= cursors_.size()) return; Cursor& ruler = cursors_[id]; - if (ruler.orientation != CursorOrientation::Horizontal) return; + if (!ruler.is_horizontal()) return; ruler.y = y; ruler.y = snapped_y(ruler); // No mark_measures_dirty(), for `add_h_ruler()`'s reason: a measurement is read from two @@ -535,7 +536,7 @@ void Plot::set_cursor_y(u32 id, f64 y) { void Plot::set_cursor_h_snap(u32 id, HRulerSnap mode) { if (id >= cursors_.size()) return; Cursor& ruler = cursors_[id]; - if (ruler.orientation != CursorOrientation::Horizontal) return; + if (!ruler.is_horizontal()) return; ruler.h_snap = mode; ruler.y = snapped_y(ruler); } diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index 8ddad47..6ea24d6 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -939,7 +939,7 @@ void Renderer::draw_cursors(const Plot& plot, const PlotLayout& layout) { // stroke (and which axis maps the span it is drawn along), and where the label is // written. Everything else -- the skip at the far edge of the axes, the highlight, the // text path -- is this one loop's, which is the point of one type rather than two. - const bool horizontal = cursor.orientation == CursorOrientation::Horizontal; + const bool horizontal = cursor.is_horizontal(); const f32 at = plot.cursor_screen_coordinate(i); // A cursor has no position on the plot when it is strictly outside its range's mapped // span -- a marker scrolled past the right edge, a ruler past the bottom -- and the @@ -985,7 +985,7 @@ void Renderer::draw_cursors(const Plot& plot, const PlotLayout& layout) { void Renderer::draw_cursor_label(const Plot& plot, const PlotLayout& layout, const Cursor& cursor, f32 at) { const f32 gap = style_.device(3.0f); - if (cursor.orientation == CursorOrientation::Horizontal) { + if (cursor.is_horizontal()) { // A ruler's text is the value it is *for*, written the way the y axis writes its own // tick labels, so a value read off a ruler reads exactly like the tick beside it. The // caller's caption, when there is one, is staged first on the same line, so neither the diff --git a/tests/test_controller.cpp b/tests/test_controller.cpp index 3f8eb18..bbb92ee 100644 --- a/tests/test_controller.cpp +++ b/tests/test_controller.cpp @@ -874,6 +874,37 @@ PLOT_TEST(horizontal_ruler_is_grabbed_and_dragged_along_y) { PLOT_CHECK(f.plot.cursor(ruler).y > 40.0); } +/// An inverted (descending) y range is ordinary -- depth, row index, descending time -- because +/// `set_y_range()` stores the range verbatim and only the transform widens it. The drag's clamp +/// must therefore order the bounds: `std::clamp` requires `min <= max`, so a raw +/// `clamp(y, range.min, range.max)` collapses the ruler onto one end on such an axis (and is +/// undefined behaviour besides). The ruler has to follow the pointer here, and stay inside the +/// range. +PLOT_TEST(horizontal_ruler_follows_the_pointer_on_an_inverted_y_range) { + Fixture f; + // Descending: the layout maps `range.min` (100) to the axes' bottom and `range.max` (0) to its + // top, so a pointer further down is a *larger* y. + f.ay = f.plot.add_y_axis({"v", {100.0, 0.0}, false, plot::AxisPlace::Left, false}); + const u32 ruler = f.plot.add_h_ruler(50.0, plot::Color::hex(0x35D0A8)); + f.plot.layout(); + const f32 column = screen_x_of(f.plot, f.ax, 50.0); + const f32 row = row_of(f.plot, f.ay, 50.0); + + hover(f.controller, column, row); + PLOT_CHECK(f.controller.hovered_cursor() == ruler); + press(f.controller, column, row); + PLOT_CHECK(f.controller.grabbed_cursor() == ruler); + + const f32 moved = row + 60.0f; + move_to(f.controller, column, moved); + // Exactly the data under the pointer, not a collapsed bound, and inside the range. + PLOT_CHECK_NEAR(f.plot.cursor(ruler).y, data_at_row(f.plot, f.ay, moved), 1e-9); + PLOT_CHECK(f.plot.cursor(ruler).y > 50.0); + PLOT_CHECK(f.plot.cursor(ruler).y < 100.0); + release(f.controller, column, moved); + PLOT_CHECK(f.plot.cursor(ruler).y > 50.0); +} + /// Nearest-first resolution *across* orientations: a pointer within reach of both a marker and a /// ruler takes the nearer of the two, so neither widens the other's target -- which is what lets /// one hit test serve two instruments. Each press below is 1 px from its winner and 4 px from the