From 2b4b316ad87822f85608d30cec402ee5fdfb88a4 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Wed, 4 Nov 2026 00:43:41 +0100 Subject: [PATCH] figure: shared-y rows draw one y axis, on the first cell Co-Authored-By: Claude Opus 4.6 --- include/plot/plot/plot.hpp | 15 +++++++++ src/plot/figure.cpp | 30 ++++++++++++++++- src/plot/plot.cpp | 12 +++++-- src/render/renderer.cpp | 7 +++- tests/test_figure.cpp | 67 +++++++++++++++++++++++++++++++++++++ tests/test_render_smoke.cpp | 46 +++++++++++++++++++++++++ 6 files changed, 172 insertions(+), 5 deletions(-) diff --git a/include/plot/plot/plot.hpp b/include/plot/plot/plot.hpp index 4a3f6cd..df0b38c 100644 --- a/include/plot/plot/plot.hpp +++ b/include/plot/plot/plot.hpp @@ -40,6 +40,13 @@ namespace plot { /// are for a host that wants one without the other. enum class XStrip { Both, LabelsOnly, TitleOnly, None }; +/// Which parts of the left y chrome `Plot::layout()` reserves. `Both` is a plot drawn on +/// its own. `None` reserves no label width, no title column and ignores the figure's common +/// gutter: it is the mode of every cell of a split row but the first when the figure shares +/// the row's y axis (`Figure::set_share_row_y`), where the first cell draws the one axis. +/// The cell's ticks still refresh and its grid still builds. +enum class YStrip { Both, None }; + /// Whether a strip mode reserves the label strip. One rule, read by `Plot::layout()` /// when it sizes the strip and by the renderer when it decides whether to stage the /// labels -- a row that reserves none would otherwise stage them over its own data. @@ -238,6 +245,12 @@ class Plot { void set_x_strip_mode(XStrip mode) noexcept { x_strip_ = mode; } XStrip x_strip_mode() const noexcept { return x_strip_; } + /// Which parts of the left y chrome `layout()` reserves; `YStrip::Both` by default. With + /// `YStrip::None` the y label width, the y title column and the figure's common gutter are + /// all zero, and the renderer stages no y labels or title (`y_label_area.w == 0`). + void set_y_strip_mode(YStrip mode) noexcept { y_strip_ = mode; } + YStrip y_strip_mode() const noexcept { return y_strip_; } + /// 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 @@ -1074,6 +1087,8 @@ class Plot { std::string x_note_; /// Which parts of the bottom x chrome are reserved at all; see set_x_strip_mode(). XStrip x_strip_ = XStrip::Both; + /// Which parts of the left y chrome are reserved at all; see set_y_strip_mode(). + YStrip y_strip_ = YStrip::Both; /// The floor a figure imposes on the left gutter; see set_min_y_gutter(). 0.0 means /// none, which is what a plot standing on its own keeps. f32 min_y_gutter_ = 0.0f; diff --git a/src/plot/figure.cpp b/src/plot/figure.cpp index 56da84f..81518ff 100644 --- a/src/plot/figure.cpp +++ b/src/plot/figure.cpp @@ -641,10 +641,14 @@ const FigureLayout& Figure::layout() { // everywhere, and then there is no shared axis to speak of: each plot has its own. A // split bottom row labels each of its cells, each against its own x axis. const XStrip strip = labels_bottom_only_ && row != bottom_row ? XStrip::None : XStrip::Both; + // A split row whose cells share one y range also shares one y axis, drawn by its first + // cell: every other cell reserves no y chrome at all (see `YStrip::None`). + const bool shared_y = share_row_y_ && cells_per_row_[row] > 1u; for (u32 i = row_first_[row]; i < row_first_[row + 1u]; ++i) { Plot& p = plots_[i]; p.set_viewport(layout_.cells[i]); p.set_x_strip_mode(strip); + p.set_y_strip_mode(shared_y && i != row_first_[row] ? YStrip::None : YStrip::Both); // Before either pass below, and the same value for every plot: nothing is measured on // the right, so unlike the left gutter this needs no measurement pass -- the plots // end on a common column because they are all told the one width. @@ -687,16 +691,40 @@ const FigureLayout& Figure::layout() { // Over the flattened plots: every cell of every row takes part, so the y axes of a split // row line up with each other and with the rows above and below. f32 gutter = min_y_gutter_; + // + // A `YStrip::None` cell draws no y axis, so it has no width to contribute and takes no part + // in the maximum: the gutter is the widest of the cells that do draw one. for (Plot& p : plots_) { p.set_min_y_gutter(0.0f); const PlotLayout& row_layout = p.layout(); + if (p.y_strip_mode() == YStrip::None) continue; gutter = std::max(gutter, row_layout.y_axis_width + row_layout.y_title_width); } + // Rows that share one y axis divide their *plot* widths, not their cell widths: the first + // cell's y strip would otherwise make its plot narrower than its neighbours'. The gutter is + // taken off the row first, the remainder is split by `place_cells` (so the weights, the gap + // and the floor all apply to plot widths), and the first cell then grows left by the gutter + // to hold the axis. The gutter is capped at half the row, so a row too narrow for it + // degrades to ordinary cells instead of producing a negative rect. The cells still tile the + // row: the first starts on its left edge and the last still ends on its right edge. + for (u32 row = 0; row < rows; ++row) { + const u32 n = cells_per_row_[row]; + if (!share_row_y_ || n <= 1u) continue; + const Rect& r = layout_.rows[row]; + const f32 g_eff = std::max(0.0f, std::min(gutter, r.w / 2.0f)); + Rect* cells = &layout_.cells[row_first_[row]]; + const f32 used = place_cells(Rect{r.x + g_eff, r.y, r.w - g_eff, r.h}, n, cell_weights_[row], + col_gap_, floor_scale, cells); + cells[0].x -= g_eff; + cells[0].w += g_eff; + layout_.col_gap = used < col_gap_ ? 0.0f : layout_.col_gap; + for (u32 i = row_first_[row]; i < row_first_[row + 1u]; ++i) plots_[i].set_viewport(layout_.cells[i]); + } // A one-row figure imposes exactly what that row measured, so it lays out as the plot // would on its own; a figure whose rows all need the same strip imposes that, which is // also what each would have done alone. for (Plot& p : plots_) { - p.set_min_y_gutter(gutter); + p.set_min_y_gutter(p.y_strip_mode() == YStrip::None ? 0.0f : gutter); p.layout(); } diff --git a/src/plot/plot.cpp b/src/plot/plot.cpp index ce601e3..924b94e 100644 --- a/src/plot/plot.cpp +++ b/src/plot/plot.cpp @@ -1319,6 +1319,11 @@ const PlotLayout& Plot::layout() { // the strip wants and this overrides the want, not the measurement. const bool x_labels = x_strip_has_labels(x_strip_); if (!x_labels) x_height = 0.0f; + // The y strip likewise: a cell that gives its y axis to a neighbour (`YStrip::None`) reserves + // no label width, no title column and no share of the figure's gutter, so every y-strip + // quantity below collapses to zero and `axes` starts at the viewport's left edge. + const bool y_none = y_strip_ == YStrip::None; + if (y_none) y_width = 0.0f; // The title strips, though, are needed before the loop and never change, unlike // the label strips: a strip holds exactly one text line per titled axis and no // tick label can change that. Nothing at all is reserved without a text backend @@ -1332,7 +1337,7 @@ const PlotLayout& Plot::layout() { // no title reserves no row for a title either, and one that reserves the title and no // labels still gets its row -- below where the labels would have been. const u32 x_titles = x_strip_has_title(x_strip_) ? titled_x_axis_count() : 0u; - const f32 y_title_w = text_ != nullptr ? y_title_strip_width() : 0.0f; + const f32 y_title_w = text_ != nullptr && !y_none ? y_title_strip_width() : 0.0f; const f32 x_title_h = text_ != nullptr && x_titles != 0u ? static_cast(x_titles) * text_->height() + 2.0f * pad : 0.0f; @@ -1353,7 +1358,7 @@ const PlotLayout& Plot::layout() { // -- and it is applied before the title column below, so the pair still fits the cap // (the floor is clamped to the same budget) and the invariant `axes.w >= share` holds. // `min_y_gutter_` is 0.0 unless a figure set it, so this is a no-op for a lone plot. - const f32 y_floor = std::min(min_y_gutter_, y_budget); + const f32 y_floor = y_none ? 0.0f : std::min(min_y_gutter_, y_budget); y_width = std::min(std::max(y_width, y_floor - y_title_w), y_budget); x_height = std::min(x_height, x_budget); const f32 y_title_width = std::min(y_title_w, y_budget - y_width); @@ -1386,7 +1391,8 @@ const PlotLayout& Plot::layout() { if (text_ == nullptr || pass + 1 >= kMaxLayoutPasses) break; // The same caps as above, so this compares like with like and the loop still // reaches its fixed point on the second pass. - const f32 need_y = std::min(std::clamp(y_label_width(), 0.0f, vw), y_budget); + const f32 need_y = + y_none ? 0.0f : std::min(std::clamp(y_label_width(), 0.0f, vw), y_budget); // Overridden, not merely unmeasured: the strip is not reserved, so the fixed // point has to be zero for a row that gave its strip away. const f32 need_x = diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index 9ef680c..b5d85c9 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -902,9 +902,14 @@ void Renderer::draw_tick_labels(const Plot& plot, const PlotLayout& layout) { // Y labels: right-aligned in their strip, centred on the tick and pulled back // inside the strip at the two extremes (a label centred on the top tick would // otherwise hang above the plot area). + // + // Unless the plot gave its y strip away (`YStrip::None`, a later cell of a split row that + // shares one y axis): its label area is then zero wide, and labels right-aligned against it + // would be drawn in the neighbouring cell. Keyed on the area rather than the mode so a strip + // the layout clamped to nothing stages nothing either. const f32 top_limit = std::max(layout.y_label_area.y, layout.y_label_area.bottom() - line_h); - for (u32 i = 0; i < plot.y_axis_count(); ++i) { + for (u32 i = 0; layout.y_label_area.w > 0.0f && i < plot.y_axis_count(); ++i) { const Transform transform = plot.y_transform(i); for (const Tick& tick : plot.y_axis(i).ticks) { if (!tick.major || tick.label.empty()) continue; diff --git a/tests/test_figure.cpp b/tests/test_figure.cpp index 3e8812a..8f9c92b 100644 --- a/tests/test_figure.cpp +++ b/tests/test_figure.cpp @@ -1341,3 +1341,70 @@ PLOT_TEST(figure_row_y_sharing_off_keeps_cells_independent) { PLOT_CHECK_NEAR(figure.plot(0u).y_range(0u).max, 1.0, 1e-9); PLOT_CHECK(!figure.share_row_y()); } + +PLOT_TEST(figure_row_y_only_the_first_cell_has_a_gutter) { + plot::Figure figure = split_figure({1, 3}, false); + for (plot::u32 i = 0; i < figure.plot_count(); ++i) give_magnitude_row(figure.plot(i), 0u); + figure.set_share_row_y(true); + figure.layout(); + PLOT_CHECK(figure.plot(0u).layout().y_axis_width > 0.0f); // unsplit row keeps its axis + PLOT_CHECK(figure.plot(1u).layout().y_axis_width > 0.0f); // first cell of the split row + for (plot::u32 i = 2; i < 4u; ++i) { + const plot::PlotLayout& l = figure.plot(i).layout(); + PLOT_CHECK(l.y_axis_width == 0.0f && l.y_title_width == 0.0f && l.y_label_area.w == 0.0f); + PLOT_CHECK(l.axes.w > 0.0f); + } + // The gutter is one figure-wide value: the unsplit row and the split row's first cell agree. + PLOT_CHECK_NEAR(figure.plot(0u).layout().axes.x - figure.layout().cells[0].x, + figure.plot(1u).layout().axes.x - figure.layout().cells[1].x, 0.01); +} + +PLOT_TEST(figure_row_y_equal_weights_give_equal_plot_widths_and_cells_tile) { + plot::Figure figure = split_figure({3}, false); + for (plot::u32 i = 0; i < 3u; ++i) give_magnitude_row(figure.plot(i), 0u); + figure.set_col_gap(8.0f); + figure.set_share_row_y(true); + const plot::FigureLayout& fl = figure.layout(); + const double w0 = figure.plot(0u).layout().axes.w; + PLOT_CHECK_NEAR(figure.plot(1u).layout().axes.w, w0, 0.6); + PLOT_CHECK_NEAR(figure.plot(2u).layout().axes.w, w0, 0.6); + PLOT_CHECK_NEAR(fl.cells[0].x, fl.rows[0].x, 0.001); + PLOT_CHECK_NEAR(fl.cells[2].right(), fl.rows[0].right(), 0.001); + PLOT_CHECK_NEAR(fl.cells[1].x - fl.cells[0].right(), 8.0, 0.01); + PLOT_CHECK_NEAR(fl.cells[2].x - fl.cells[1].right(), 8.0, 0.01); +} + +PLOT_TEST(figure_row_y_weights_apply_to_plot_widths) { + plot::Figure figure = split_figure({2}, false); + for (plot::u32 i = 0; i < 2u; ++i) give_magnitude_row(figure.plot(i), 0u); + figure.set_col_gap(0.0f); + figure.set_share_row_y(true); + const std::vector w{3.0f, 1.0f}; + PLOT_CHECK(figure.set_cell_weights(0u, w)); + figure.layout(); + const double a0 = figure.plot(0u).layout().axes.w; + const double a1 = figure.plot(1u).layout().axes.w; + PLOT_CHECK_NEAR(a0 - 64.0, (a1 - 64.0) * 3.0, 1.0); +} + +PLOT_TEST(figure_row_y_narrow_row_degrades_without_negative_rects) { + plot::Figure figure = split_figure({4}, false); + figure.set_viewport(plot::Rect{0.0f, 0.0f, 30.0f, 100.0f}); + for (plot::u32 i = 0; i < 4u; ++i) give_magnitude_row(figure.plot(i), 0u); + figure.set_col_gap(10.0f); + figure.set_share_row_y(true); + double sum = 0.0; + for (const plot::Rect& c : figure.layout().cells) { + PLOT_CHECK(c.w >= 0.0f && c.h >= 0.0f); + sum += c.w; + } + PLOT_CHECK_NEAR(sum, 30.0, 0.05); +} + +PLOT_TEST(figure_row_y_sharing_off_still_gives_every_cell_a_gutter) { + plot::Figure figure = split_figure({2}, false); + give_magnitude_row(figure.plot(0u), 0u); + give_magnitude_row(figure.plot(1u), 1u); + figure.layout(); + PLOT_CHECK(figure.plot(1u).layout().y_axis_width > 0.0f); +} diff --git a/tests/test_render_smoke.cpp b/tests/test_render_smoke.cpp index bf17f35..0e65842 100644 --- a/tests/test_render_smoke.cpp +++ b/tests/test_render_smoke.cpp @@ -5059,6 +5059,52 @@ PLOT_TEST(figure_x_title_is_drawn_on_the_bottom_row_only) { ctx.destroy(); } +/// A split row that shares one y axis draws it once, on its first cell: every later cell is +/// `YStrip::None`, reserves no y strip, and the renderer must stage no y tick labels for it. +/// Staged text is not observable through `FrameStats`, so this reads the pixels. The column gap +/// is wide and empty chrome, and a label staged for the second cell would be right-aligned +/// against that cell's (zero-width) label area, i.e. land in the gap: the gap holds no ink. +PLOT_TEST(figure_shared_y_second_cell_stages_no_y_labels) { + 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 cells{2u}; + plot::Figure figure{std::span(cells)}; + figure.set_viewport({0, 0, static_cast(kW), static_cast(kH)}); + for (plot::u32 i = 0; i < 2u; ++i) { + plot::Plot& p = figure.plot(i); + p.add_x_axis({"t", {0.0, 100.0}, false, plot::AxisPlace::Bottom, false}); + p.add_y_axis({"", {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + } + figure.set_col_gap(80.0f); + figure.set_share_row_y(true); + + 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()); + const plot::FigureLayout layout = figure.layout(); + const plot::Color ink = r.style().axis_text; + + const plot::PlotLayout& first = figure.plot(0u).layout(); + const plot::PlotLayout& second = figure.plot(1u).layout(); + PLOT_CHECK(second.y_label_area.w == 0.0f); + PLOT_CHECK(layout.cells[1].x - layout.cells[0].right() > 70.0f); + // The first cell still draws the row's one axis ... + PLOT_CHECK(scan_rect_ink(ctx, first.y_label_area, ink).pixels > 0u); + // ... and nothing is staged in the gap on the second cell's left. + // Inset a few pixels each side: the cells' own frame lines sit on the gap's two edges. + const plot::Rect gap{layout.cells[0].right() + 4.0f, first.axes.y, + layout.cells[1].x - layout.cells[0].right() - 8.0f, first.axes.h}; + PLOT_CHECK(scan_rect_ink(ctx, gap, ink).pixels == 0u); + + ctx.destroy(); +} + // ---------------------------------------------------- the axes clip (spec ยง13) // The reported defect this section is about: a trace is staged in *data* space and