diff --git a/include/plot/render/renderer.hpp b/include/plot/render/renderer.hpp index 3ee8817..45672a9 100644 --- a/include/plot/render/renderer.hpp +++ b/include/plot/render/renderer.hpp @@ -284,15 +284,14 @@ class Renderer { /// Writes the framebuffer's viewport region as a binary PPM (P6, 8 bits per /// channel, rows top-down) so a headless render can be looked at without a /// 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. - /// - /// 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; + /// was drawn into. Returns false, with `st` filled in, on a bad argument, an + /// unreadable pixel, or an allocation the readback could not make -- which is + /// reported as `ErrorCode::OutOfMemory`, exactly as `TrueTypeFont::from_file()` + /// reports the same failure. `noexcept` like every other entry point here: the + /// buffers are sized before anything is on disk, so the two failures that can + /// arise from sizing them are a `Status`, not a `std::bad_alloc` across the API + /// boundary and a half-written file. + bool dump_frame_ppm(const char* path, Status* st = nullptr) const noexcept; private: /// The backend the plot layer sees, at the size the text pipeline draws it: diff --git a/src/input/controller.cpp b/src/input/controller.cpp index 17b9f13..aa4cc97 100644 --- a/src/input/controller.cpp +++ b/src/input/controller.cpp @@ -608,10 +608,18 @@ void Controller::axis_zoom(const PointerEvent& e) { // A pointer drag reports pixels, not wheel notches, so it uses its own // sensitivity: 200 px is one decade. The gain is a physical distance the hand // moves, so the device-pixel delta is converted to logical pixels first -- the - // same drag must zoom the same amount on a 2x display. A scale no host should - // ever set (0, or negative) makes the factor non-positive, and the guard below - // turns that into no zoom at all rather than an axis re-centred on infinity. - const f64 logical_dy = static_cast(e.dy) / static_cast(plot_->ui_scale()); + // same drag must zoom the same amount on a 2x display. + // + // A scale that is not a positive number is not a scale -- a host that has not + // measured its display, a field nobody filled in -- the same state + // `Style::device()` tolerates. Here it means *no zoom*: dividing by it would make + // `logical_dy` infinite and the factor infinite (or NaN), and that factor reaches + // `set_x_range()`/`set_y_range()`, which store it verbatim and clear `auto_scale` + // -- an axis left non-finite until an auto-fit. A negative scale would silently + // invert the zoom direction too, so the whole class is refused here. + const f64 scale = static_cast(plot_->ui_scale()); + if (!(scale > 0.0) || !std::isfinite(scale)) return; + const f64 logical_dy = static_cast(e.dy) / scale; const f64 factor = std::exp(-logical_dy * kZoomPerPixel); if (!(factor > 0.0)) return; if (mode_ == DragMode::AxisZoomX) { diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index 22e7f21..cab991f 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include namespace plot { @@ -1123,6 +1124,10 @@ void Renderer::draw_measures(const Plot& plot, const PlotLayout& layout) { void Renderer::draw_annotations(const Plot& plot) { const f32 line_h = font_backend_.height(); + // The clip every other overlay is drawn under, read here because the caption is staged + // into the frame's single text batch -- flushed after `clear_clip()` -- so the scissor that + // keeps the rest of this pass inside the axes does not reach it. + const Rect& axes = plot.layout().axes; for (u32 i = 0; i < plot.annotation_count(); ++i) { const Annotation& annotation = plot.annotation(i); if (!annotation.visible) continue; @@ -1134,7 +1139,14 @@ void Renderer::draw_annotations(const Plot& plot) { lines_.add_segment({px - style_.device(3.0f), py}, {px + style_.device(3.0f), py}); lines_.add_segment({px, py - style_.device(3.0f)}, {px, py + style_.device(3.0f)}); flush_lines(pixel_mapping(), annotation.color, style_.device(1.0f)); - if (!annotation.text.empty()) { + // A caption whose anchor is outside the axes is skipped with its cross, which the clip + // has already removed: a caption with no cross is a label for a point the frame does not + // show, and -- because the text batch is drawn *after* the clip is released -- it would + // otherwise paint outside the axes it is measured in, into the neighbouring row of a + // `Figure` whose rows tile the figure. Splitting the frame's one text draw call to clip + // it instead would cost a draw call per row and mis-clip the tick labels, which are + // staged outside `axes` on purpose and share the batch after this one. + if (!annotation.text.empty() && axes.contains(px, py)) { stage_text({px + style_.device(5.0f), py - 0.5f * line_h}, annotation.text.c_str()); } } @@ -1262,7 +1274,7 @@ void Renderer::draw_crosshair_readout(const Plot& plot, const PlotLayout& layout // -------------------------------------------------------------------- dump -bool Renderer::dump_frame_ppm(const char* path, Status* st) const { +bool Renderer::dump_frame_ppm(const char* path, Status* st) const noexcept { if (path == nullptr || path[0] == '\0') { return fail(st, ErrorCode::InvalidArgument, "PPM path is empty"); } @@ -1275,9 +1287,20 @@ bool Renderer::dump_frame_ppm(const char* path, Status* st) const { return fail(st, ErrorCode::InvalidArgument, "no frame has been drawn yet"); } - // RGBA, not RGB: 4-byte pixels have no row padding whatever the pack alignment, - // so a host's GL_PACK_ALIGNMENT cannot make the read overrun the buffer. - readback_.assign(usize(width) * height * 4u, 0u); + // The two buffers are sized -- and can fail to size -- before anything is on disk, so an + // allocation failure is reported through the `Status` rather than throwing `std::bad_alloc` + // across the public boundary (which is `noexcept`) and leaving a half-written file behind. + // The readback hole is RGBA, not RGB: 4-byte pixels have no row padding whatever the pack + // alignment, so a host's GL_PACK_ALIGNMENT cannot make the read overrun the buffer. The row + // is the PPM's own 3-byte stride, one line at a time. + std::vector row; + try { + readback_.assign(usize(width) * height * 4u, 0u); + row.assign(usize(width) * 3u, 0u); + } catch (const std::bad_alloc&) { + return fail(st, ErrorCode::OutOfMemory, "out of memory sizing the PPM readback"); + } + gl::glPixelStorei(gl::GL_PACK_ALIGNMENT, 4); gl::glReadPixels(static_cast(gl_viewport_.x), static_cast(gl_viewport_.y), static_cast(width), static_cast(height), gl::GL_RGBA, @@ -1288,7 +1311,6 @@ bool Renderer::dump_frame_ppm(const char* path, Status* st) const { return fail(st, ErrorCode::InvalidArgument, "cannot open the PPM path for writing"); } std::fprintf(file, "P6\n%u %u\n255\n", width, height); - std::vector row(usize(width) * 3u); bool ok = true; for (u32 y = 0; y < height && ok; ++y) { // glReadPixels hands back rows bottom-up; a PPM's rows run top-down. diff --git a/tests/test_controller.cpp b/tests/test_controller.cpp index f0eb2a9..5cd99c3 100644 --- a/tests/test_controller.cpp +++ b/tests/test_controller.cpp @@ -697,6 +697,63 @@ PLOT_TEST(drag_zoom_is_logical_the_wheel_is_not_scaled) { } } +/// `Style::device()` documents a non-positive `ui_scale` as host state that must be +/// tolerated, but `axis_zoom()` divided by it directly. With `ui_scale == 0` and a +/// negative drag delta, `logical_dy` was `-inf`, the factor `+inf`, and the +/// `!(factor > 0.0)` guard did not reject it: `zoom_x_axis`/`zoom_y_axis` computed +/// `anchor + (bound - anchor) * inf`, and `set_y_range()` stored `inf`/`NaN` verbatim and +/// cleared `auto_scale` -- an axis left non-finite until an auto-fit. A negative scale +/// silently inverted the zoom direction. Both now mean no zoom at all, exactly the state +/// `device()` documents for a scale that is not one. +PLOT_TEST(axis_zoom_refuses_a_non_positive_ui_scale) { + for (const f32 scale : {0.0f, -1.0f}) { + plot::Plot p; + p.set_ui_scale(scale); + p.set_viewport({0.0f, 0.0f, 800.0f, 400.0f}); + // `auto_scale` on, and no trace bound: `layout()` has nothing to fit, so the range is + // the one below and clearing `auto_scale` is a change this case can see. + const u32 ax = p.add_x_axis({"t", {0.0, 100.0}, true, plot::AxisPlace::Bottom, false}); + const u32 ay = p.add_y_axis({"v", {0.0, 100.0}, true, plot::AxisPlace::Left, false}); + plot::input::Controller c(p); + c.set_viewport({0.0f, 0.0f, 800.0f, 400.0f}); + const plot::Range xb = p.x_range(ax); + const plot::Range yb = p.y_range(ay); + const plot::PlotLayout& l = p.layout(); + + // A middle drag on the x label strip, dragged *up* (a negative dy): the sign that + // made the factor infinite before. + const f32 strip_x = l.axes.x + 0.5f * l.axes.w; + const f32 strip_y = l.x_label_area.y + 0.5f * l.x_label_area.h; + c.on_pointer({strip_x, strip_y, 0, 0, plot::input::Button::Middle, 0, true, true, false}); + PLOT_CHECK(c.drag_mode() == plot::input::DragMode::AxisZoomX); + c.on_pointer({strip_x, strip_y - 30.0f, 0, -30.0f, plot::input::Button::Middle, 0, true, false, + false}); + c.on_pointer({strip_x, strip_y - 30.0f, 0, 0, plot::input::Button::Middle, 0, false, false, + true}); + PLOT_CHECK(std::isfinite(p.x_range(ax).min)); + PLOT_CHECK(std::isfinite(p.x_range(ax).max)); + PLOT_CHECK(p.x_range(ax).min == xb.min); + PLOT_CHECK(p.x_range(ax).max == xb.max); + + // And the y strip, where the infinite factor was stored verbatim. + const f32 y_strip_x = l.y_label_area.x + 0.5f * l.y_label_area.w; + const f32 y_strip_y = l.axes.y + 0.5f * l.axes.h; + c.on_pointer({y_strip_x, y_strip_y, 0, 0, plot::input::Button::Middle, 0, true, true, false}); + PLOT_CHECK(c.drag_mode() == plot::input::DragMode::AxisZoomY); + c.on_pointer({y_strip_x, y_strip_y - 30.0f, 0, -30.0f, plot::input::Button::Middle, 0, true, + false, false}); + c.on_pointer({y_strip_x, y_strip_y - 30.0f, 0, 0, plot::input::Button::Middle, 0, false, false, + true}); + PLOT_CHECK(std::isfinite(p.y_range(ay).min)); + PLOT_CHECK(std::isfinite(p.y_range(ay).max)); + PLOT_CHECK(p.y_range(ay).min == yb.min); + PLOT_CHECK(p.y_range(ay).max == yb.max); + // The range setters never ran, so the axes' own auto scale stands untouched. + PLOT_CHECK(p.x_axis(ax).auto_scale); + PLOT_CHECK(p.y_axis(ay).auto_scale); + } +} + /// The consequences of a degenerate range on a live axis, from the gesture side: the /// wheel, the pan and the axis zoom all refuse a span that is not a span, so the /// frozen axis a stored zero-span left behind was one no interaction could move -- diff --git a/tests/test_render_smoke.cpp b/tests/test_render_smoke.cpp index af8830b..f3eb2a4 100644 --- a/tests/test_render_smoke.cpp +++ b/tests/test_render_smoke.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include namespace { @@ -113,6 +114,13 @@ void dump_if_requested(const plot::Renderer& r, const char* default_name) { PLOT_CHECK(ok); } +/// The public-boundary contract (review finding 3d): `dump_frame_ppm()` reports an +/// allocation failure through its `Status` -- `ErrorCode::OutOfMemory`, as +/// `TrueTypeFont::from_file()` does -- rather than letting `std::bad_alloc` cross the +/// boundary, so it is `noexcept` like every other entry point here. +static_assert(noexcept(std::declval().dump_frame_ppm(nullptr)), + "dump_frame_ppm must not throw across the public boundary"); + /// True when `px` is (within a couple of levels) the straight-alpha colour `c`, /// which is what a fully covered text pixel is. The colour comes from the style, /// so no predicate here has to guess what the renderer draws with. @@ -4237,6 +4245,81 @@ PLOT_TEST(figure_row_overshoot_stays_out_of_the_neighbouring_row) { ctx.destroy(); } +/// An annotation's **caption** is the one overlay the per-row clip did not reach: the +/// cross is staged as line geometry inside the axes clip, while the caption is staged +/// into the frame's single text batch, which is flushed *after* `clear_clip()`. An +/// annotation anchored outside the visible range therefore kept a caption with no cross, +/// and -- in a `Figure`, whose rows tile the figure -- that caption painted into the +/// neighbouring row, the bleeding the clip exists to prevent. The caption is now skipped +/// with its cross when the anchor is outside the axes; an in-range annotation still draws +/// its caption, so the skip is not a blanket refusal to draw text. +PLOT_TEST(figure_annotation_caption_stays_out_of_the_neighbouring_row) { + 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)); + + const std::vector samples = wave(640u, 0.0f); + plot::data::ArraySeries series(samples, 0.0, 1.0); + plot::Figure figure(2u); + figure.set_viewport({0.0f, 0.0f, static_cast(kW), static_cast(kH)}); + add_row_trace(figure.plot(0u), series, kRowBlue); + + plot::Plot& bottom = figure.plot(1u); + const plot::u32 ax = + bottom.add_x_axis({"t", series.x_bounds(), false, plot::AxisPlace::Bottom, false}); + const plot::u32 ay = bottom.add_y_axis({"v", {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + bottom.add_trace({&series, kRowMagenta, 1.5f, ax, ay}); + + // The geometry the out-of-range anchor is chosen against: a y whose pixel row lands + // *inside the row above*, well above this row's own axes. + figure.layout(); + const plot::FigureLayout layout = figure.layout(); + PLOT_CHECK(layout.rows.size() == 2u); + const plot::Rect& axes0 = figure.plot(0u).layout().axes; + const plot::Rect& axes1 = bottom.layout().axes; + PLOT_CHECK(axes0.h > 0.0f && axes1.h > 0.0f); + const plot::f32 target_y = axes0.y + 0.4f * axes0.h; + + plot::Annotation outside; + outside.x = 320.0; // in range for x, so the caption is staged at a real column + outside.y = bottom.y_transform(ay).to_data_y(static_cast(target_y)); + outside.text = "outside"; + outside.color = plot::Color::hex(0xE8E8E8); + outside.axis_x = ax; + outside.axis_y = ay; + bottom.add_annotation(outside); + + // The control: an annotation actually inside row 1's axes, whose caption must still + // be drawn -- so the assertion below is about the skip, not about text being absent. + plot::Annotation inside; + inside.x = 320.0; + inside.y = 0.0; + inside.text = "inside"; + inside.color = plot::Color::hex(0xE8E8E8); + inside.axis_x = ax; + inside.axis_y = ay; + bottom.add_annotation(inside); + + r.begin_frame(kW, kH, {0, 0, static_cast(kW), static_cast(kH)}); + r.draw(figure); + r.end_frame(); + PLOT_CHECK(!r.last_frame_had_gl_error()); + dump_if_requested(r, "clip_annotation_caption.ppm"); + + const std::vector frame = read_frame(ctx); + + // Row 0's axes hold no text of their own -- its labels live in its y strip, outside + // `axes` -- so any text ink here would be the neighbour's escaped caption. + PLOT_CHECK(predicate_pixels(frame, kW, axes0, text_pixel) == 0u); + // The in-range caption proves the text pipeline ran and captions are drawn at all. + PLOT_CHECK(predicate_pixels(frame, kW, axes1, text_pixel) > 0u); + + ctx.destroy(); +} + /// The fix's neighbours: the overlay geometry that already declined to draw over the /// strips did so by its own column rules -- a region's bounds outside the window are /// skipped by `draw_region_edge`, a cursor scrolled out of it by `draw_cursors`, and a