From 1cb59707579170a45dfa8aed981e20a82290a75d Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 28 Sep 2026 10:54:00 +0200 Subject: [PATCH] fix(render): environment-aware atlas drift guard, pinned text scale, shipped-atlas coverage --- include/plot/render/detail/text_scale.hpp | 26 ++++ include/plot/render/text.hpp | 8 +- src/render/renderer.cpp | 25 ++-- tests/test_primitives.cpp | 168 ++++++++++++++++++++-- tests/test_render_smoke.cpp | 55 +++++++ tools/fonts/gen_atlas.py | 6 +- 6 files changed, 257 insertions(+), 31 deletions(-) create mode 100644 include/plot/render/detail/text_scale.hpp diff --git a/include/plot/render/detail/text_scale.hpp b/include/plot/render/detail/text_scale.hpp new file mode 100644 index 0000000..2f5ede4 --- /dev/null +++ b/include/plot/render/detail/text_scale.hpp @@ -0,0 +1,26 @@ +#pragma once + +#include + +namespace plot::render::detail { + +/// The factor a text run is drawn by: `requested_px / em_px`, a plain ratio. +/// +/// It is a ratio rather than a whole multiple because the renderer's built-in +/// face is anti-aliased: a style asking for 12 px from a 13 px bake is drawn at +/// 12/13, where a 1-bit glyph would have had to snap to the nearest multiple of +/// its cell. Everything the renderer stages is scaled by this one factor, so the +/// labels a plot laid out and the glyphs the pipeline draws cannot disagree. +/// +/// A backend that cannot state an em -- `em_px()` at or below zero, or NaN -- +/// yields a 1:1 scale rather than dividing by it or propagating a NaN into every +/// vertex. The requested size being NaN, zero or negative does the same. The rule +/// lives here, not inline in the renderer, so it is one function and can be +/// asserted without a GL context; it is instrumentation-free and part of no +/// public surface (`*/detail/` headers stay out of the umbrella). +inline f32 text_scale(f32 requested_px, f32 em_px) noexcept { + const f32 ratio = requested_px / em_px; + return (em_px > 0.0f && ratio > 0.0f) ? ratio : 1.0f; +} + +} // namespace plot::render::detail diff --git a/include/plot/render/text.hpp b/include/plot/render/text.hpp index 978859f..ae3a082 100644 --- a/include/plot/render/text.hpp +++ b/include/plot/render/text.hpp @@ -115,9 +115,11 @@ class TextBackend { /// backend was baked at: the renderer scales by `requested_px / em_px()`, so a /// backend re-baked at another size -- or swapped for a different face -- keeps /// drawing `Style::axis_font_px`, and the renderer never has to know which font - /// it holds. A backend with no meaningful em (one whose "font pixels" are an - /// arbitrary unit) returns something proportional, `height()` being the - /// natural choice. + /// + /// A backend that *wraps* another to draw it at a size scales this with the + /// rest of its metrics -- it is in the wrapper's units, like `height()` -- so a + /// caller that reads the backend back and draws at `requested_px / em_px()` + /// gets a ratio of two screen-pixel quantities and cannot scale twice. virtual f32 em_px() const = 0; /// The glyph for `codepoint`, or null when this backend has none -- the caller diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index b7fda10..fb02902 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -97,15 +98,12 @@ void warn_replaced_text_backend(const render::TextBackend* previous) noexcept { void Renderer::AxisFont::reset(const render::TextBackend& font, f32 requested_px) noexcept { font_ = &font; - // The requested size is an em size, exactly as `em_px()` reports the size the - // atlas was baked at, so the ratio between them is all the scaling there is: - // an anti-aliased face is legible at any ratio, and a style asking for 12 px - // from a 13 px bake gets 12/13 rather than a snapped whole multiple. A backend - // that cannot say (`em_px()` at or below zero, or NaN) keeps a 1:1 scale rather - // than dividing by it. - const f32 baked = font.em_px(); - const f32 ratio = requested_px / baked; - scale_ = (baked > 0.0f && ratio > 0.0f) ? ratio : 1.0f; + // An anti-aliased face is legible at any ratio, so a style asking for 12 px + // from a 13 px bake is drawn at 12/13 rather than snapped to a whole multiple + // of the cell. `detail::text_scale` owns that rule, including the 1:1 fallback + // for a backend that cannot state an em -- it is one function, and the test + // suite asserts it without a GL context. + scale_ = render::detail::text_scale(requested_px, font.em_px()); cell_px_ = font.height() * scale_; } @@ -122,9 +120,12 @@ f32 Renderer::AxisFont::ascent() const { } f32 Renderer::AxisFont::em_px() const { - // The em is the one metric that does not scale: it is the face's own unit, and - // it is what the scale above was computed from. - return font_ != nullptr ? font_->em_px() : 0.0f; + // Scaled with the rest of the metrics, not forwarded raw: `TextBackend::em_px()` + // is documented to be in the same units as `height()` and `ascent()`, and this + // wrapper's are all screen pixels. A caller that reads the backend back off the + // plot and draws at `requested_px / em_px()` therefore gets 1:1 rather than + // scaling a second time (see the base-class contract). + return font_ != nullptr ? font_->em_px() * scale_ : 0.0f; } const render::Glyph* Renderer::AxisFont::glyph(u32 codepoint) const { diff --git a/tests/test_primitives.cpp b/tests/test_primitives.cpp index 952c0f3..5a9e466 100644 --- a/tests/test_primitives.cpp +++ b/tests/test_primitives.cpp @@ -3,12 +3,14 @@ // Nothing here touches GL, so it runs on any machine that can build the tests. #include "harness.hpp" +#include #include #include #include #include #include +#include #include #include #include @@ -221,6 +223,83 @@ PLOT_TEST(embedded_atlas_covers_every_expected_codepoint) { PLOT_CHECK(f.glyph(0x4E00u)->advance > 0.0f); } +// The *shipped table* -- not the generator, and not a lookup -- is what the +// renderer uploads, so the coverage and quality contract is asserted on it +// directly rather than only on the codepoint lookups above. Slot by slot, the +// atlas holds exactly the documented codepoints in order, every cell is the one +// declared size and lies inside the image, every cell but the space carries ink, +// and the fallback box is a real inked glyph at its fixed final slot. +// +// It is the case the drift guard beside it cannot replace: `--check` compares the +// committed bytes to a bake made with whatever coverage list the generator +// currently holds, so dropping a codepoint from that list keeps it green. These +// assertions are what make a dropped codepoint (say U+00B5) or a blank cell fail +// loudly, on the data that actually ships. +PLOT_TEST(embedded_atlas_matches_its_documented_coverage) { + const EmbeddedMonoFont f; + const plot::Vec2 cell = f.cell_uv(); + + // The documented coverage set, in the documented slot order: printable ASCII, + // the three codepoints `plot::format_si` emits plus the two label dashes, then + // the fallback box appended last. + std::vector documented; + for (plot::u32 cp = 0x20u; cp <= 0x7Eu; ++cp) documented.push_back(cp); + for (plot::u32 cp : {0x00B0u, 0x00B5u, 0x03A9u, 0x2013u, 0x2014u}) documented.push_back(cp); + PLOT_CHECK(EmbeddedMonoFont::glyph_count() == documented.size() + 1u); + + const plot::u8* atlas = f.atlas_pixels(); + const plot::u32 atlas_w = f.atlas_width(); + const plot::u32 atlas_h = f.atlas_height(); + const std::size_t atlas_texels = static_cast(atlas_w) * atlas_h; + plot::u32 inked_cells = 0u; + + for (plot::u32 slot = 0; slot < EmbeddedMonoFont::glyph_count(); ++slot) { + const plot::u32 expected = + slot < documented.size() ? documented[slot] : EmbeddedMonoFont::kFallbackCodepoint; + const plot::u32 cp = EmbeddedMonoFont::codepoint_of_slot(slot); + PLOT_CHECK(cp == expected); // the table is the documented one, in order + if (cp != EmbeddedMonoFont::kFallbackCodepoint) { + PLOT_CHECK(EmbeddedMonoFont::glyph_index(cp) == slot); + } + + // Its rect is the one declared cell, and it lies inside the image ... + const plot::render::Glyph* g = f.glyph(cp); + PLOT_CHECK(g != nullptr); + PLOT_CHECK(plot::render::glyph_is_cell(*g, cell)); + PLOT_CHECK(g->u0 >= 0.0f && g->u1 <= 1.0f && g->v0 >= 0.0f && g->v1 <= 1.0f); + + // ... and the cell's texels are all inside the image with ink in them, bar + // the one documented blank. The bounds check is what a table claiming more + // bytes than it ships fails on. + const plot::Vec2 origin = EmbeddedMonoFont::atlas_origin(slot); + const plot::u32 ox = static_cast(origin.x); + const plot::u32 oy = static_cast(origin.y); + PLOT_CHECK(ox + EmbeddedMonoFont::cell_width() <= atlas_w); + PLOT_CHECK(oy + EmbeddedMonoFont::cell_height() <= atlas_h); + plot::u32 ink = 0u; + for (plot::u32 y = 0; y < EmbeddedMonoFont::cell_height(); ++y) { + for (plot::u32 x = 0; x < EmbeddedMonoFont::cell_width(); ++x) { + const std::size_t index = static_cast(oy + y) * atlas_w + (ox + x); + PLOT_CHECK(index < atlas_texels); + ink += atlas[index]; + } + } + if (cp == 0x20u) { + PLOT_CHECK(ink == 0u); // the space is the one blank cell + } else { + PLOT_CHECK(ink > 0u); // every covered glyph, and the box, has ink + ++inked_cells; + } + } + PLOT_CHECK(inked_cells == EmbeddedMonoFont::glyph_count() - 1u); + + // The fallback sits at the fixed final slot, as documented: a regeneration that + // inserted or dropped a glyph would move it, and this catches that where the + // lookups alone would not. + PLOT_CHECK(fallback_slot() == EmbeddedMonoFont::glyph_count() - 1u); + PLOT_CHECK(EmbeddedMonoFont::glyph_index(0x10FFFFu) == fallback_slot()); +} + // The atlas carries more than one coverage value: an anti-aliased raster has grey // edges, and a 1-bit table could only ever be 0 or 255. This is the data-level // half of the anti-aliasing evidence (the frame-level half is in the render @@ -363,27 +442,86 @@ PLOT_TEST(embedded_atlas_is_uploadable) { static_cast(EmbeddedMonoFont::cell_width()), 1e-9); } -// The committed atlas is the generator's output, not a hand edit, and the -// generator is deterministic: the same source font, em size and supersample -// factor produce it byte for byte. The check runs the committed script against -// the committed file, so a table touched by hand, a stale bake after a coverage -// change, or a non-deterministic generator all fail here. -// -// `fontTools` is an authoring-time tool and the test machine is not required to -// have it. Its absence is *reported*, not silently passed: the generator exits -// with a distinct code for it, and this case says so on stdout. +// The size the renderer draws a run by is a plain ratio, `requested_px / em_px`, +// and not a whole multiple of the backend's cell. This is the rule the whole +// anti-aliasing change rests on: a style asking for 12 px from a 13 px bake is +// drawn at 12/13, where the old 1-bit font had to snap to a multiple of its 8 px +// cell (16 px here). `detail::text_scale` is that rule, so it is asserted without +// a GL context; the frame-level half is in the render smoke test. +PLOT_TEST(text_scale_is_a_ratio_not_a_whole_multiple) { + using plot::render::detail::text_scale; + const plot::f32 em = 13.0f; // what the committed atlas is baked at + + // 12/13 = 0.9231, so the 17-px cell is drawn 15.69 px tall -- not the 16 px the + // old rule would have chosen, and not the 17 px of a whole multiple of it. + PLOT_CHECK_NEAR(text_scale(12.0f, em), 12.0f / 13.0f, 1e-6); + PLOT_CHECK(std::fabs(text_scale(12.0f, em) * 17.0f - 16.0f) > 0.2f); + PLOT_CHECK(std::fabs(text_scale(12.0f, em) * 17.0f - 17.0f) > 0.2f); + + // A ratio that is not a multiple is not rounded to one ... + PLOT_CHECK_NEAR(text_scale(20.0f, em), 20.0f / 13.0f, 1e-6); + PLOT_CHECK(text_scale(20.0f, em) > 1.5f && text_scale(20.0f, em) < 1.6f); + + // ... and an exact multiple is exactly that multiple, so nothing is lost when + // the request does line up with the bake. + PLOT_CHECK_NEAR(text_scale(13.0f, em), 1.0f, 1e-6); + PLOT_CHECK_NEAR(text_scale(26.0f, em), 2.0f, 1e-6); + PLOT_CHECK_NEAR(text_scale(39.0f, em), 3.0f, 1e-6); +} + +// A backend that cannot state an em must not take the frame with it: the scale +// falls back to 1:1 rather than dividing by zero or propagating a NaN into every +// vertex, and the same holds for a nonsensical request. No shipped backend can +// reach this -- `EmbeddedMonoFont::em_px()` is a positive constant -- which is +// exactly why the rule is a function with its own case instead of a bare +// division inside `Renderer::AxisFont::reset`. +PLOT_TEST(text_scale_guards_a_backend_without_an_em) { + using plot::render::detail::text_scale; + const plot::f32 nan = std::numeric_limits::quiet_NaN(); + + PLOT_CHECK_NEAR(text_scale(12.0f, 0.0f), 1.0f, 1e-6); // no em at all + PLOT_CHECK_NEAR(text_scale(12.0f, -13.0f), 1.0f, 1e-6); // nonsense em + PLOT_CHECK_NEAR(text_scale(12.0f, nan), 1.0f, 1e-6); // NaN em + PLOT_CHECK_NEAR(text_scale(nan, 13.0f), 1.0f, 1e-6); // NaN request + PLOT_CHECK_NEAR(text_scale(0.0f, 13.0f), 1.0f, 1e-6); // zero request + PLOT_CHECK_NEAR(text_scale(-12.0f, 13.0f), 1.0f, 1e-6); // negative request + + // The guard does not fire for a healthy pair: the ratio is still exact. + PLOT_CHECK_NEAR(text_scale(26.0f, 13.0f), 2.0f, 1e-6); + PLOT_CHECK_NEAR(text_scale(12.0f, 13.0f), 12.0f / 13.0f, 1e-6); +} + +// Three exit codes say something about the *machine* rather than about the +// committed bytes, and none of them can tell a drifted table from a good one: +// `gen_atlas.py`'s EXIT_NO_FONTTOOLS (fontTools not installed, 3) and +// EXIT_BAD_FONT (no source face -- the adwaita-fonts package -- or an +// unparseable one, 2), and the shell's "command not found" for a missing +// `python3` (127). Each is *reported* with what is missing and skipped, rather +// than failed, so an environment difference does not look like atlas drift -- +// a machine with fontTools from pip but no distro font package would otherwise +// fail here and send someone hunting for a corruption that is not there. Only a +// real mismatch between the committed atlas and a fresh bake (1) fails. The +// data cases above still assert the shipped table on such a machine, so the +// only thing lost is the drift comparison itself. PLOT_TEST(generator_is_deterministic_and_current) { - const std::string command = std::string("python3 ") + kSourceDir + - "/tools/fonts/gen_atlas.py --check --out " + kSourceDir + - "/src/render/embedded_mono_font.cpp"; + // Both paths are quoted: `kSourceDir` is a build-time value, and a source tree + // whose path contains a space would otherwise split into extra arguments. + const std::string command = "python3 \"" + std::string(kSourceDir) + + "/tools/fonts/gen_atlas.py\" --check --out \"" + kSourceDir + + "/src/render/embedded_mono_font.cpp\""; const int status = std::system(command.c_str()); PLOT_CHECK(status != -1); PLOT_CHECK(WIFEXITED(status)); const int code = WEXITSTATUS(status); - if (code == 3) { // `gen_atlas.py`'s "fontTools is not installed" code + const char* missing = code == 3 ? "fontTools is not installed" + : code == 2 ? "the Adwaita Mono face is not installed (adwaita-fonts)" + : code == 127 ? "python3 is not on PATH" + : nullptr; + if (missing != nullptr) { std::printf( - "SKIPPED: generator_is_deterministic_and_current -- fontTools is not " - "installed, so the atlas could not be re-baked\n"); + "SKIPPED: generator_is_deterministic_and_current -- %s, so the committed " + "atlas could not be re-baked\n", + missing); return; } PLOT_CHECK(code == 0); diff --git a/tests/test_render_smoke.cpp b/tests/test_render_smoke.cpp index 6b881c0..66b0a68 100644 --- a/tests/test_render_smoke.cpp +++ b/tests/test_render_smoke.cpp @@ -426,6 +426,61 @@ class CountFenceWaits { } // namespace +// The renderer draws a run at the size the style asked for, not at a whole +// multiple of the backend's cell, and it reports what it drew. `axis_font_px()` +// is that report, and the committed atlas is baked at 13 px, so the default +// style's 12 px is a 12/13 scale -- exactly the case the old rule snapped to +// 16 px. No other case sets a size other than the default, and the ones that +// compare a measure compare values scaled by the same factor, so they hold under +// a snapping rule too; this is the one that does not. +PLOT_TEST(axis_text_is_drawn_at_the_requested_em_not_a_whole_multiple) { + plottest::HeadlessContext ctx(kW, kH); + plot::Status st; + PLOT_CHECK(ctx.create(&st)); + PLOT_CHECK(plot::gl::load(nullptr)); + + // The renderer's own backend, for the raw metrics the ratio is taken against. + const plot::render::EmbeddedMonoFont font; + const plot::f32 baked = font.em_px(); + const plot::f32 cell = font.height(); + + plot::Renderer r; + r.style().axis_font_px = 12.0f; // the default style's em, against a 13 px bake + PLOT_CHECK(r.init(&st)); + PLOT_CHECK_NEAR(r.axis_font_px(), cell * (12.0f / baked), 1e-3); + PLOT_CHECK(std::fabs(r.axis_font_px() - 16.0f) > 0.2f); // 2 x the old 8 px glyph + PLOT_CHECK(std::fabs(r.axis_font_px() - 17.0f) > 0.2f); // 1 x this cell + + plot::Plot p; + p.set_viewport({0.0f, 0.0f, static_cast(kW), static_cast(kH)}); + p.add_x_axis({"t", {0.0, 1000.0}, true, plot::AxisPlace::Bottom, false}); + p.add_y_axis({"v", {-1.0, 1.0}, true, plot::AxisPlace::Left, false}); + const plot::Rect viewport{0.0f, 0.0f, static_cast(kW), static_cast(kH)}; + r.begin_frame(kW, kH, viewport); + r.draw(p); + r.end_frame(); + + // The plot lays out with the backend the renderer installed, so the size it + // measured with is the size it drew with. + const plot::render::TextBackend* backend = p.text_backend(); + PLOT_CHECK(backend != nullptr); + PLOT_CHECK_NEAR(backend->em_px(), 12.0f, 1e-3); + PLOT_CHECK_NEAR(backend->height(), cell * (12.0f / baked), 1e-3); + // `em_px()` is in the wrapper's units, like `height()`: a caller drawing at + // `requested_px / em_px()` reads 12/12 = 1 and cannot scale a second time. + PLOT_CHECK_NEAR(backend->em_px() / backend->height(), baked / cell, 1e-6); + + // An exact multiple still lands exactly, and the frame re-reads the style. + r.style().axis_font_px = 26.0f; // 2 x 13 + r.begin_frame(kW, kH, viewport); + r.draw(p); + r.end_frame(); + PLOT_CHECK_NEAR(p.text_backend()->em_px(), 26.0f, 1e-3); + PLOT_CHECK_NEAR(p.text_backend()->height(), cell * 2.0f, 1e-3); + PLOT_CHECK(plot::gl::glGetError() == plot::gl::GL_NO_ERROR); + ctx.destroy(); +} + PLOT_TEST(renders_visible_trace_across_viewport) { plottest::HeadlessContext ctx(kW, kH); plot::Status st; diff --git a/tools/fonts/gen_atlas.py b/tools/fonts/gen_atlas.py index 90d9193..1f9a1c6 100755 --- a/tools/fonts/gen_atlas.py +++ b/tools/fonts/gen_atlas.py @@ -617,9 +617,13 @@ def main(argv=None): if args.check: out = Path(args.out) committed = out.read_bytes() if out.is_file() else None - fresh, _ = generate(args.font, args.em, args.supersample) + # A missing committed file is drift whatever else is wrong with the + # machine, so it is reported before the font is read: otherwise a checkout + # with no atlas *and* no installed face would look like an environment + # gap (EXIT_BAD_FONT) instead of the failure it is. if committed is None: return _fail(EXIT_MISMATCH, f"{out} does not exist; run {SCRIPT} to generate it") + fresh, _ = generate(args.font, args.em, args.supersample) if committed != fresh.encode(): return _fail( EXIT_MISMATCH,