fix(render): gate TextAtlas::sync on the loader, and cover three review findings
Review of the atlas refactor found one Important and three Minor issues: - Important: TextAtlas::sync called glGetIntegerv before any available() check, so a host that synced before plot::gl::load() -- which is legal for a public class Tasks 2 and 8 build backends on -- segfaulted on a null entry point instead of receiving the Status the interface promises. It now follows the handle-constructor convention (available(), then the entry point itself) and reports Unsupported. - empty() no longer means "a GL name exists": releasing one is a glDeleteTextures call, so the no-atlas path forgets the atlas and only resets the texture when there is a table to release it with. - end_frame's comment still claimed the frame hands back the unpack alignment init() set, contradicting the code below it and renderer.hpp. - The orientation case's texel probe mapped the atlas' u along the cell's +v axis and vice versa, which is a no-op for its two diagonal probes -- so it could not fail on a transposed UV mapping, and its own comment misstated the rule. Probes now follow the shader's rule and include an off-diagonal pair. - The null-glyph path of add_text (and its twin in walk_text_clipped) was unreached by any case. Cases: text_atlas_reports_a_status_when_the_entry_points_are_missing, add_text_skips_a_codepoint_the_backend_cannot_draw.
This commit is contained in:
Martino Ferrari committed
1 parent
e1f6b61b7d
commit
3ff7b9c978
5 files changed
+230
-16
No files matched your search
@@ -484,3 +484,92 @@ PLOT_TEST(text_batch_takes_its_cell_from_the_backend) {
|
||||
PLOT_CHECK_NEAR(b.glyph_cell_uv().x, 0.0, 1e-6);
|
||||
PLOT_CHECK_NEAR(b.glyph_cell_uv().y, 0.0, 1e-6);
|
||||
}
|
||||
|
||||
/// A backend with a hole in its artwork: it can draw 'A' and 'B' and publishes no
|
||||
/// glyph at all for anything else, which is what a font whose coverage falls short
|
||||
/// of a label looks like. `measure()` counts only what it can draw, so "what a
|
||||
/// string is worth" and "what it stages" stay the same thing -- the pipeline draws
|
||||
/// nothing for the missing codepoint and moves the pen nowhere, rather than
|
||||
/// inventing a box or a width.
|
||||
class HoleInCoverageBackend final : public plot::render::TextBackend {
|
||||
public:
|
||||
plot::f32 measure(const char* utf8) const override {
|
||||
plot::f32 width = 0.0f;
|
||||
for (const char* p = utf8; p != nullptr && *p != '\0';) {
|
||||
const plot::render::Glyph* g = glyph(plot::render::next_codepoint(p));
|
||||
if (g != nullptr) width += g->advance;
|
||||
}
|
||||
return width;
|
||||
}
|
||||
plot::f32 height() const override { return 8.0f; }
|
||||
plot::f32 ascent() const override { return 6.0f; }
|
||||
plot::Vec2 cell_uv() const override { return plot::Vec2{0.5f, 1.0f}; }
|
||||
plot::u32 revision() const override { return 1u; }
|
||||
const plot::render::Glyph* glyph(plot::u32 codepoint) const override {
|
||||
if (codepoint == static_cast<plot::u32>('A')) return &a_;
|
||||
if (codepoint == static_cast<plot::u32>('B')) return &b_;
|
||||
return nullptr; // no artwork at all: not even a fallback box
|
||||
}
|
||||
const plot::u8* atlas_pixels() const override { return nullptr; }
|
||||
plot::u32 atlas_width() const override { return 0u; }
|
||||
plot::u32 atlas_height() const override { return 0u; }
|
||||
|
||||
private:
|
||||
const plot::render::Glyph a_{0.0f, 0.0f, 0.5f, 1.0f, 0.0f, 0.0f, 6.0f};
|
||||
const plot::render::Glyph b_{0.5f, 0.0f, 1.0f, 1.0f, 0.0f, 0.0f, 4.0f};
|
||||
};
|
||||
|
||||
// A backend that publishes no glyph for a codepoint: `add_text` stages nothing for
|
||||
// it and does not advance the pen, so a run stays exactly as long as the same
|
||||
// backend measured it and the glyphs it *can* draw keep their places. A backend
|
||||
// that would rather show something publishes a fallback box as that codepoint's
|
||||
// glyph (which is what `BitmapFont` does); this is the other legal answer, and the
|
||||
// staging path has to honour it rather than draw whatever was staged last or
|
||||
// advance by a width nobody published.
|
||||
PLOT_TEST(add_text_skips_a_codepoint_the_backend_cannot_draw) {
|
||||
HoleInCoverageBackend backend;
|
||||
const plot::render::Glyph* a = backend.glyph(static_cast<plot::u32>('A'));
|
||||
const plot::render::Glyph* b = backend.glyph(static_cast<plot::u32>('B'));
|
||||
PLOT_CHECK(a != nullptr);
|
||||
PLOT_CHECK(b != nullptr);
|
||||
PLOT_CHECK(backend.glyph(static_cast<plot::u32>('?')) == nullptr);
|
||||
|
||||
const plot::Vec2 pen{10.0f, 4.0f};
|
||||
plot::render::TextBatch batch;
|
||||
batch.begin();
|
||||
const plot::Vec2 end = batch.add_text("A?B", pen, 1.0f, backend);
|
||||
|
||||
// Two glyphs staged for three codepoints, and the pen moved by exactly the two
|
||||
// advances the backend published.
|
||||
PLOT_CHECK(batch.glyph_count() == 2);
|
||||
PLOT_CHECK_NEAR(end.x, pen.x + a->advance + b->advance, 1e-6);
|
||||
PLOT_CHECK_NEAR(end.y, pen.y, 1e-6);
|
||||
// Which is the length the backend measured for the same string, so a label's
|
||||
// reserved space still covers the run drawn into it.
|
||||
PLOT_CHECK_NEAR((end - pen).length(), backend.measure("A?B"), 1e-6);
|
||||
PLOT_CHECK_NEAR(backend.measure("A?B"), a->advance + b->advance, 1e-6);
|
||||
// 'B' starts where 'A' ended -- the skipped codepoint left no gap and drew no
|
||||
// glyph of its own, which the second instance's atlas rect says outright.
|
||||
PLOT_CHECK_NEAR(batch.data()[0], pen.x, 1e-6);
|
||||
PLOT_CHECK_NEAR(batch.data()[2], a->u0, 1e-6);
|
||||
PLOT_CHECK_NEAR(batch.data()[6], pen.x + a->advance, 1e-6);
|
||||
PLOT_CHECK_NEAR(batch.data()[8], b->u0, 1e-6);
|
||||
|
||||
// A run of nothing but undrawable codepoints stages nothing at all and leaves
|
||||
// the pen where it was, rather than drawing at the pen.
|
||||
batch.begin();
|
||||
const plot::Vec2 same = batch.add_text("??", pen, 1.0f, backend);
|
||||
PLOT_CHECK(batch.glyph_count() == 0);
|
||||
PLOT_CHECK(batch.data().empty());
|
||||
PLOT_CHECK_NEAR(same.x, pen.x, 1e-6);
|
||||
PLOT_CHECK_NEAR(same.y, pen.y, 1e-6);
|
||||
PLOT_CHECK_NEAR(backend.measure("??"), 0.0, 1e-6);
|
||||
|
||||
// A real font never hits this path (every codepoint maps to its box), so the
|
||||
// same string through it stages one glyph per codepoint: the two answers to
|
||||
// "no glyph" are the backend's choice, not the pipeline's.
|
||||
plot::render::BitmapFont font;
|
||||
batch.begin();
|
||||
batch.add_text("A?B", pen, 1.0f, font);
|
||||
PLOT_CHECK(batch.glyph_count() == 3);
|
||||
}
|
||||
@@ -336,6 +336,24 @@ class RefusePersistentMap {
|
||||
decltype(plot::gl::glMapBufferRange) saved_;
|
||||
};
|
||||
|
||||
/// Holds `plot::gl::glGetIntegerv` at null for as long as it lives: the entry
|
||||
/// point `TextAtlas::sync` reaches first, so a sync with it missing has to report
|
||||
/// a `Status` rather than call through a null pointer. Saved on construction,
|
||||
/// restored on destruction, exactly like the two stubs above -- so a failed check
|
||||
/// cannot leak it into the next case.
|
||||
class RefuseGetIntegerv {
|
||||
public:
|
||||
RefuseGetIntegerv() noexcept : saved_(plot::gl::glGetIntegerv) {
|
||||
plot::gl::glGetIntegerv = nullptr;
|
||||
}
|
||||
~RefuseGetIntegerv() { plot::gl::glGetIntegerv = saved_; }
|
||||
RefuseGetIntegerv(const RefuseGetIntegerv&) = delete;
|
||||
RefuseGetIntegerv& operator=(const RefuseGetIntegerv&) = delete;
|
||||
|
||||
private:
|
||||
decltype(plot::gl::glGetIntegerv) saved_;
|
||||
};
|
||||
|
||||
/// Same, for `glFenceSync`: the sync object the ring's slot guard needs is
|
||||
/// refused while the caps and the entry point still say the mapped path is on.
|
||||
class RefuseFenceSync {
|
||||
@@ -1294,12 +1312,15 @@ PLOT_TEST(rotated_glyph_orientation_is_not_mirrored) {
|
||||
const plot::Vec2 uy{-ux.y, ux.x}; // (1, 0): the cell's own +v axis
|
||||
|
||||
// Where the centre of the glyph's texel (col, row) is drawn. Every cell is
|
||||
// `kGlyphPx` texels across, and the cell's top-left is the pen because the
|
||||
// bitmap font's bearings are zero.
|
||||
// `kGlyphPx` texels across, the cell's top-left is the pen because the bitmap
|
||||
// font's bearings are zero, and the rule is the shader's own: the atlas' u runs
|
||||
// along the cell's +x axis (`ux` here, `a_axis` there) and its v along the
|
||||
// perpendicular (`v_axis`), which is what `v_uv = a_uv + corner * u_glyph_uv`
|
||||
// maps corner by corner.
|
||||
auto texel_centre = [&](plot::f32 col, plot::f32 row, plot::Vec2 v_axis) {
|
||||
const plot::f32 du = (col + 0.5f) * px;
|
||||
const plot::f32 dv = (row + 0.5f) * px;
|
||||
return plot::Vec2{pen.x + ux.x * dv + v_axis.x * du, pen.y + ux.y * dv + v_axis.y * du};
|
||||
return plot::Vec2{pen.x + ux.x * du + v_axis.x * dv, pen.y + ux.y * du + v_axis.y * dv};
|
||||
};
|
||||
auto inked = [&](plot::Vec2 at) {
|
||||
plot::u8 pxv[4] = {};
|
||||
@@ -1314,6 +1335,13 @@ PLOT_TEST(rotated_glyph_orientation_is_not_mirrored) {
|
||||
// its foot. Both corners are where the basis puts them ...
|
||||
PLOT_CHECK(inked(texel_centre(0.0f, 0.0f, uy)));
|
||||
PLOT_CHECK(inked(texel_centre(6.0f, 6.0f, uy)));
|
||||
// ... and the two *off-diagonal* texels of its ink state the rule the diagonal
|
||||
// pair cannot: the stem meets the foot at (0, 6), which is where the cell's
|
||||
// first texel row and last texel column meet, while (6, 0) is blank. A shader
|
||||
// that swapped the cell's two axes would trade those two, and both of these
|
||||
// would fail -- the diagonal probes would not.
|
||||
PLOT_CHECK(inked(texel_centre(0.0f, 6.0f, uy)));
|
||||
PLOT_CHECK(!inked(texel_centre(6.0f, 0.0f, uy)));
|
||||
// ... and the mirrored basis -- the other perpendicular a shader could build --
|
||||
// would put the stem's top on the opposite side of the pen, where this frame has
|
||||
// no ink at all.
|
||||
@@ -1456,3 +1484,46 @@ PLOT_TEST(staging_a_glyph_that_is_not_the_cell_aborts_in_a_debug_build) {
|
||||
PLOT_CHECK(WTERMSIG(status) == SIGABRT); // and it was the assertion
|
||||
#endif
|
||||
}
|
||||
|
||||
// The loader invariant (see `plot::gl::loader.hpp`): while the entry-point table
|
||||
// is unresolved no `gl*` call may be made at all, and `TextAtlas` is public, so a
|
||||
// host may hold one and sync it before `plot::gl::load()` -- Tasks 2 and 8 build
|
||||
// backends on this class. Every `gl*` call it makes is therefore gated, and a sync
|
||||
// in that state reports the same `ErrorCode::Unsupported` every fallible
|
||||
// `plot::gl` handle constructor reports, instead of calling a null pointer.
|
||||
//
|
||||
// `available()` itself cannot be driven false in this suite: `load()` is
|
||||
// process-global and one-way (no API makes the table unresolved again) and it
|
||||
// falls back to `dlsym` on the system GL library, so there is no state a test can
|
||||
// restore the process to -- faking it would poison every later case's GL. What is
|
||||
// reachable is the same guard from the entry point it protects: with the first
|
||||
// entry point it calls missing, the sync must report and leave the atlas empty.
|
||||
PLOT_TEST(text_atlas_reports_a_status_when_the_entry_points_are_missing) {
|
||||
plottest::HeadlessContext ctx(kW, kH);
|
||||
plot::Status st;
|
||||
PLOT_CHECK(ctx.create(&st));
|
||||
PLOT_CHECK(plot::gl::load(nullptr));
|
||||
PLOT_CHECK(plot::gl::available());
|
||||
|
||||
plot::render::BitmapFont font;
|
||||
plot::render::TextAtlas atlas;
|
||||
{
|
||||
RefuseGetIntegerv refused;
|
||||
PLOT_CHECK(!atlas.sync(font, &st));
|
||||
PLOT_CHECK(st.code == plot::ErrorCode::Unsupported);
|
||||
PLOT_CHECK(st.message != nullptr);
|
||||
PLOT_CHECK(atlas.empty());
|
||||
PLOT_CHECK_NEAR(atlas.texel_size().x, 0.0, 1e-6);
|
||||
PLOT_CHECK_NEAR(atlas.texel_size().y, 0.0, 1e-6);
|
||||
}
|
||||
|
||||
// With the table back the same atlas uploads, so the refusal was the missing
|
||||
// entry point and not a poisoned owner, and the retry is what a caller does with
|
||||
// the Status it got.
|
||||
PLOT_CHECK(atlas.sync(font, &st));
|
||||
PLOT_CHECK(!atlas.empty());
|
||||
PLOT_CHECK_NEAR(atlas.texel_size().x, static_cast<plot::f32>(plot::render::BitmapFont::kAtlasWidth),
|
||||
1e-6);
|
||||
PLOT_CHECK(plot::gl::glGetError() == plot::gl::GL_NO_ERROR);
|
||||
ctx.destroy();
|
||||
}
|
||||
Reference in new issue
Block a user