From e211701d05677a58c5a284794f47d8e141fc880b Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 28 Sep 2026 11:26:24 +0200 Subject: [PATCH] fix(render): drop y title columns the chrome clamp cut short --- src/render/renderer.cpp | 22 ++++- tests/test_render_smoke.cpp | 160 ++++++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+), 3 deletions(-) diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index 53a439a..9a9a71b 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -622,11 +622,27 @@ void Renderer::draw_titles(const Plot& plot, const PlotLayout& layout) { const Axis& axis = plot.y_axis(i); if (axis.label.empty()) continue; const Rect& strip = layout.y_title_area; + // A column must *fit* before it is staged, and its own cell is what has to fit: + // with this basis the cell occupies [pen.x, pen.x + line_h], so the strip's + // width -- the one dimension the run never advances along -- is the whole of a + // column's cross extent, and nothing else in this path can see it. `walk_text_clipped()` + // cuts along the run and would happily stage a column that is wider than the + // strip it belongs to, drawing its cells over whatever is beside them. + // + // The layout clamps the strip against the viewport's chrome allowance (see + // `Plot::layout()`), so a viewport narrow enough to spend that allowance on the + // tick labels leaves room for fewer columns than there are titled axes, or for + // none at all. Columns are laid out left to right, so the ones the clamp cut + // short are the trailing ones: keep the leading columns that fit and stage no + // more. A dropped title is chrome that had no room, which is the same bargain + // the label strip already makes -- never drawn outside its strip, and never + // inside `axes`. + const f32 x = strip.x + pad + static_cast(column) * line_h; + if (x + line_h > strip.right()) break; const f32 budget = strip.h - 2.0f * pad; const f32 length = clipped_text_width(axis.label.c_str(), budget, kTextUp); - stage_text_clipped({strip.x + pad + static_cast(column) * line_h, - strip.bottom() - 0.5f * (strip.h - length)}, - axis.label.c_str(), budget, kTextUp); + stage_text_clipped({x, strip.bottom() - 0.5f * (strip.h - length)}, axis.label.c_str(), budget, + kTextUp); ++column; } } diff --git a/tests/test_render_smoke.cpp b/tests/test_render_smoke.cpp index c1a20c3..a76c73c 100644 --- a/tests/test_render_smoke.cpp +++ b/tests/test_render_smoke.cpp @@ -411,6 +411,20 @@ std::vector ink_column_bands(plottest::HeadlessContext& ctx, const plot return bands; } +/// Every pixel of the framebuffer, four bytes each, top row first -- the same order +/// the Rect-based helpers above read in. Comparing two frames pixel for pixel is how +/// a case asserts that a change had *no* effect anywhere in the target: nothing was +/// drawn, rather than drawn somewhere else. +std::vector read_frame(plottest::HeadlessContext& ctx) { + std::vector frame(plot::usize(ctx.width) * ctx.height * 4u); + for (plot::u32 y = 0; y < ctx.height; ++y) { + for (plot::u32 x = 0; x < ctx.width; ++x) { + PLOT_CHECK(ctx.read_pixel(x, ctx.height - 1u - y, &frame[(plot::usize(y) * ctx.width + x) * 4u])); + } + } + return frame; +} + /// 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 @@ -1387,6 +1401,152 @@ PLOT_TEST(over_long_y_title_is_clipped_to_its_strip) { ctx.destroy(); } +// The chrome allowance (`Plot::layout()`) caps the y label strip at half the viewport +// and gives the title strip what is left of that allowance, so a viewport narrow +// enough to spend all of it on the tick labels leaves the title strip narrower than +// one text line -- or, as here, zero wide. Nothing about a *rotated* title is bounded +// by that width: its budget is the strip's height, its pen is the strip's padding and +// `walk_text_clipped()` clips along the run, so before the fit gate the glyph cells +// were staged into a strip that cannot hold them -- over the tick labels, which are +// right-aligned across exactly those columns, and (once the columns step past +// `axes.x`) inside the data area. +// +// The property is "not drawn, not relocated": the same scene drawn with and without +// y titles must produce identical frames. Comparing the whole target catches the +// overprint wherever it lands, and cannot be satisfied by drawing the title somewhere +// else. Three titled axes are what it takes to reach `axes` in a strip this narrow. +PLOT_TEST(narrow_viewport_draws_no_y_title_it_cannot_fit) { + constexpr plot::u32 kNarrowW = 64; + constexpr plot::u32 kNarrowH = 200; + plottest::HeadlessContext ctx(kNarrowW, kNarrowH); + plot::Status st; + PLOT_CHECK(ctx.create(&st)); + PLOT_CHECK(plot::gl::load(nullptr)); + plot::Renderer r; + PLOT_CHECK(r.init(&st)); + + const auto scene = [&](const char* title) { + plot::Plot plot; + plot.set_viewport({0, 0, static_cast(kNarrowW), static_cast(kNarrowH)}); + plot.add_x_axis({"", {0.0, 1.0}, false, plot::AxisPlace::Bottom, false}); + plot.add_y_axis({title, {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + plot.add_y_axis({title, {-1.0, 1.0}, false, plot::AxisPlace::Right, false}); + plot.add_y_axis({title, {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + return plot; + }; + const auto render = [&](plot::Plot& plot) { + r.begin_frame(kNarrowW, kNarrowH, + {0, 0, static_cast(kNarrowW), static_cast(kNarrowH)}); + r.draw(plot); + r.end_frame(); + PLOT_CHECK(!r.last_frame_had_gl_error()); + }; + + plot::Plot titled = scene("millivolts per division"); + render(titled); + const std::vector with_titles = read_frame(ctx); + const plot::PlotLayout layout = titled.layout(); // a copy: draw() mutates the plot + const plot::Color ink = r.style().axis_text; + + // The clamp bit exactly the way this case needs it to: the strip has less room + // than the one column its three titled axes ask for, so no column can be staged. + PLOT_CHECK(titled.titled_y_axis_count() == 3u); + PLOT_CHECK(layout.y_title_area.w < r.axis_font_px() + r.style().grid_label_pad); + // ... and the plot kept its floor in a viewport this narrow, so nothing here is + // degenerate for some other reason. + PLOT_CHECK(layout.axes.w >= 0.5f * static_cast(kNarrowW)); + PLOT_CHECK(layout.axes.h > 0.0f); + // No title ink in the data area. This is the direct read of the invariant; the + // frame comparison below is the same statement about the whole target. + PLOT_CHECK(!rect_has_exact(ctx, layout.axes, ink)); + + plot::Plot plain = scene(""); + render(plain); + const std::vector without_titles = read_frame(ctx); + + plot::u32 differing = 0u; + plot::u32 first = 0u; + for (plot::u32 i = 0; i < static_cast(with_titles.size() / 4u); ++i) { + bool same = true; + for (plot::u32 c = 0; c < 4u && same; ++c) { + same = with_titles[4u * i + c] == without_titles[4u * i + c]; + } + if (!same) { + if (differing == 0u) first = i; + ++differing; + } + } + if (differing != 0u) { + std::printf("[narrow_y_title] %u of %zu pixels differ; first at x=%u y=%u\n", + differing, with_titles.size() / 4u, first % kNarrowW, first / kNarrowW); + } + PLOT_CHECK(differing == 0u); + ctx.destroy(); +} + +// The same clamp, in a viewport wide enough that the strip holds *some* columns: the +// ones the clamp cut short must be dropped, not drawn beside the ones that fit. Three +// titled y axes ask for three columns, the strip has room for fewer, and the columns +// that survive are the leading ones -- which is what "keep the leading columns" +// means, since the renderer lays them out left to right from the strip's padding. +PLOT_TEST(narrow_viewport_keeps_only_the_y_title_columns_that_fit) { + // 170 px is where the clamp leaves room for exactly one column (the label strip + // takes 51.2 px of the 85 px allowance, leaving 33.8 px against the 19.7 px a + // column needs); the assertions below re-derive that rather than assume it. + constexpr plot::u32 kNarrowW = 170; + constexpr plot::u32 kNarrowH = 200; + plottest::HeadlessContext ctx(kNarrowW, kNarrowH); + plot::Status st; + PLOT_CHECK(ctx.create(&st)); + PLOT_CHECK(plot::gl::load(nullptr)); + plot::Renderer r; + PLOT_CHECK(r.init(&st)); + + plot::Plot p; + p.set_viewport({0, 0, static_cast(kNarrowW), static_cast(kNarrowH)}); + p.add_x_axis({"", {0.0, 1.0}, false, plot::AxisPlace::Bottom, false}); + p.add_y_axis({"volts", {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + p.add_y_axis({"amps", {-1.0, 1.0}, false, plot::AxisPlace::Right, false}); + p.add_y_axis({"watts", {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + r.begin_frame(kNarrowW, kNarrowH, + {0, 0, static_cast(kNarrowW), static_cast(kNarrowH)}); + r.draw(p); + r.end_frame(); + PLOT_CHECK(!r.last_frame_had_gl_error()); + dump_if_requested(r, "y_title_columns_clamped.ppm"); + + const plot::PlotLayout layout = p.layout(); + const plot::Color ink = r.style().axis_text; + const plot::f32 line = r.axis_font_px(); + const plot::f32 pad = r.style().grid_label_pad; + const plot::Rect& strip = layout.y_title_area; + + // A column occupies the strip's leading padding plus one line, so this is how many + // of the three fit -- the rule the renderer applies, and the reason the trailing + // ones are the ones with nowhere to go. The regime this case is about needs a + // strip that holds at least one column and fewer than all three. + PLOT_CHECK(p.titled_y_axis_count() == 3u); + const plot::u32 fits = static_cast(std::max(0.0f, (strip.w - pad) / line)); + PLOT_CHECK(fits >= 1u); + PLOT_CHECK(fits < 3u); + PLOT_CHECK(layout.axes.w >= 0.5f * static_cast(kNarrowW)); + PLOT_CHECK(layout.axes.h > 0.0f); + + // One band of inked columns per column that fits, each inside its own column's cell + // (band i starts at the padding plus i lines and is one line wide). + const std::vector columns = ink_column_bands(ctx, strip, ink); + PLOT_CHECK(columns.size() == fits); + for (plot::u32 i = 0; i < static_cast(columns.size()); ++i) { + PLOT_CHECK(columns[i].first >= + static_cast(strip.x + pad + static_cast(i) * line)); + PLOT_CHECK(columns[i].last <= + static_cast(strip.x + pad + static_cast(i + 1u) * line)); + } + // ... and none of it reached the data area. + PLOT_CHECK(!rect_has_exact(ctx, layout.axes, ink)); + ctx.destroy(); +} + // Finding 4 of the review: `mark_drawn()` dropped a failed fence's status, so the // slot stayed unsynchronised and the next upload memcpy'd into storage the GPU may // still have been reading -- exactly the tearing the ring exists to prevent. A