diff --git a/include/plot/gl/handle.hpp b/include/plot/gl/handle.hpp index 123dffc..2f5d8dd 100644 --- a/include/plot/gl/handle.hpp +++ b/include/plot/gl/handle.hpp @@ -27,26 +27,50 @@ class Handle { GLuint id() const noexcept { return id_; } explicit operator bool() const noexcept { return id_ != 0; } + /// Identity of the object this handle names, as a counter that only ever + /// advances: every `reset()` that deleted a name, every allocating `create()` + /// and every move advances it. A driver is free to hand out a name it has + /// already used, so state that captured the old object -- the attribute + /// pointers of a vertex array above all -- must compare this counter rather + /// than the name (see `render::detail::InstanceStore::epoch()`). + u32 generation() const noexcept { return generation_; } + /// Deletes the GL object, if any, and empties the handle. void reset() noexcept { if (id_ != 0) { Derived::destroy(id_); id_ = 0; + ++generation_; } } protected: - Handle(Handle&& other) noexcept : id_(other.id_) { other.id_ = 0; } + Handle(Handle&& other) noexcept : id_(other.id_), generation_(other.generation_) { + other.id_ = 0; + ++generation_; // the object is this handle's now + ++other.generation_; // and no longer the other's + } Handle& operator=(Handle&& other) noexcept { if (this != &other) { - reset(); + reset(); // advances the generation when it deleted a name id_ = other.id_; other.id_ = 0; + ++generation_; + ++other.generation_; } return *this; } + /// Advances the generation for a name the derived class has just allocated and + /// stored: every allocating `create()` calls it, so the counter is a complete + /// record of the handle's identity. A failed allocation needs no call -- it has + /// already gone through `reset()`, which advanced it. + void note_allocated() noexcept { ++generation_; } + GLuint id_ = 0; + + private: + u32 generation_ = 0; }; /// Vertex buffer object. All fallible operations report through `Status`; diff --git a/include/plot/plot/plot.hpp b/include/plot/plot/plot.hpp index 6ca9f04..b72aad6 100644 --- a/include/plot/plot/plot.hpp +++ b/include/plot/plot/plot.hpp @@ -16,16 +16,32 @@ namespace plot { /// Pixel geometry of a plot's chrome, all of it in the same pixel space as -/// `Rect` (origin top-left). `axes` is the area data is drawn into; the two label -/// areas are the strips the tick labels live in. +/// `Rect` (origin top-left). `axes` is the area data is drawn into; the label +/// areas are the strips the tick labels live in and the title areas the strips +/// the axis titles live in, each outside `axes` and so never over the data. +/// +/// `y_*` strips are left of `axes` and `x_*` ones below it, whatever an axis' +/// `place` says: an axis placed on the opposite edge still has its labels and +/// its title on the side the layout reserves, exactly as it always has. struct PlotLayout { Rect axes; Rect y_label_area; Rect x_label_area; + /// One text line each, just outside the matching label area. Zero-sized (and + /// unoccupied) when no axis of that direction has a title. + Rect y_title_area; + Rect x_title_area; /// Left strip width. The default is used when no text backend has been set. f32 y_axis_width = 56.0f; /// Bottom strip height. The default is used when no text backend has been set. f32 x_axis_height = 22.0f; + /// Width of `y_title_area` and height of `x_title_area`, both 0.0f when nothing + /// is reserved. An x title is one text line in a strip as wide as the axes; a y + /// title is a horizontal run in a vertical strip, so the strip is as wide as the + /// widest y title (plus the label padding) -- one text line would cut it off. + /// With no text backend there is no title to measure and both stay 0.0f. + f32 y_title_width = 0.0f; + f32 x_title_height = 0.0f; }; /// The public façade: axes, traces, cursors, annotations and their layout. @@ -155,6 +171,12 @@ class Plot { /// layout. The label strips come from the text backend's metrics when one is /// set (`measure()` of the widest y tick label, `height()` for the x strip) and /// from PlotLayout's defaults otherwise. + /// + /// `axes` is inset by the label strip of each occupied edge and, when that + /// direction has a titled axis, by the strip that title lives in (`y_title_width` + /// and `x_title_height` say how much that is): a title can then only be drawn + /// outside the data, whatever the title says. Call this once per frame -- it + /// rebuilds the ticks and the grid -- and pass the result down. const PlotLayout& layout(); /// The layout as of the last refresh. The const overload never recomputes, so /// a lookup on a const Plot cannot move the axes out from under the caller. @@ -197,6 +219,13 @@ class Plot { /// Widest y tick label plus the label padding on both sides: the left strip the /// text backend asks for. Zero with no backend or no y axis. f32 y_label_width() const; + /// True when any axis of that direction carries a title, i.e. when layout() + /// must reserve the strip `Renderer::draw_titles()` draws it in. + bool has_x_title() const noexcept; + bool has_y_title() const noexcept; + /// Widest y axis title plus the label padding on both sides: the vertical strip + /// a horizontal y title fits in. Zero with no backend or no titled y axis. + f32 y_title_strip_width() const; Rect viewport_{}; render::Style style_{}; diff --git a/include/plot/render/buffer.hpp b/include/plot/render/buffer.hpp index 6d8de9a..9761072 100644 --- a/include/plot/render/buffer.hpp +++ b/include/plot/render/buffer.hpp @@ -71,9 +71,13 @@ class InstanceStore { /// Records that the draw the caller just issued read the current slot, so the /// fence guarding that slot's reuse ends up in the command stream behind the - /// draw. A no-op in `Orphan` mode, where nothing reads the store after the - /// orphan. - void mark_drawn() noexcept; + /// draw. True when the slot is guarded, and always in `Orphan` mode, where + /// nothing reads the store after the orphan. + /// + /// False when the driver refused the fence: the slot would stay unsynchronised, + /// so the store drops the mapped ring for the orphan path instead of rotating + /// through it (the caller reports the failure -- see `Renderer`). + bool mark_drawn() noexcept; /// Byte offset the current slot's instances start at: 0 in `Orphan` mode, /// `slot * slot_bytes` in `PersistentMapped` mode. The vertex array re-points @@ -84,7 +88,14 @@ class InstanceStore { /// Changes whenever the staged instances moved (a new slot) or the GL buffer /// was recreated, i.e. whenever attribute pointers into this store went stale. - u32 epoch() const noexcept { return epoch_; } + /// + /// Derived from the two things that can make them stale rather than bumped by + /// hand at every site that replaces the buffer object: `gl::Buffer::generation()` + /// advances on every allocation and deletion (so a fallback that re-creates the + /// buffer cannot leave the vertex array bound to a deleted one), and `slot_epoch_` + /// advances on every upload that lands in a new slot. Both counters only ever + /// increase, so their sum changes whenever either does, and never repeats. + u32 epoch() const noexcept { return buffer_.generation() + slot_epoch_; } void bind() const noexcept { buffer_.bind(); } bool persistent() const noexcept { return mode_ == UploadMode::PersistentMapped; } @@ -103,7 +114,9 @@ class InstanceStore { u32 capacity_ = 0; ///< instances the orphan store can hold usize slot_bytes_ = 0; ///< bytes per slot of the persistent ring u32 slot_ = kSlots - 1u; ///< last slot written; an upload advances it first - u32 epoch_ = 0; + /// Times an upload landed in a new slot. Kept apart from the buffer's own + /// identity (see `epoch()`) so neither has to know about the other. + u32 slot_epoch_ = 0; u8* mapped_ = nullptr; ///< base of the mapped ring, null in `Orphan` mode gl::Fence fences_[kSlots]; }; @@ -119,7 +132,7 @@ class LineBuffer { void upload(const f32* interleaved, u32 count) noexcept { store_.upload(interleaved, count, sizeof(LineInstance)); } - void mark_drawn() noexcept { store_.mark_drawn(); } + bool mark_drawn() noexcept { return store_.mark_drawn(); } usize slot_offset() const noexcept { return store_.slot_offset(); } u32 epoch() const noexcept { return store_.epoch(); } bool persistent() const noexcept { return store_.persistent(); } @@ -138,7 +151,7 @@ class TextBuffer { void upload(const f32* interleaved, u32 count) noexcept { store_.upload(interleaved, count, sizeof(TextInstance)); } - void mark_drawn() noexcept { store_.mark_drawn(); } + bool mark_drawn() noexcept { return store_.mark_drawn(); } usize slot_offset() const noexcept { return store_.slot_offset(); } u32 epoch() const noexcept { return store_.epoch(); } bool persistent() const noexcept { return store_.persistent(); } diff --git a/include/plot/render/renderer.hpp b/include/plot/render/renderer.hpp index 0eaf2bd..9b24a91 100644 --- a/include/plot/render/renderer.hpp +++ b/include/plot/render/renderer.hpp @@ -58,7 +58,9 @@ struct FrameStats { /// persistent-mapped, fence-guarded ring on 4.3+/ARB_buffer_storage, and the /// orphan-and-refill baseline on 3.3. Setting `PLOT_FORCE_GL33_UPLOAD=1` in the /// environment forces the baseline path, which is how both are exercised on one -/// machine. +/// machine. A driver that refuses the storage, a map or a fence drops the store +/// that hit it to the baseline path -- the ring is never rotated through without +/// a guard -- and the frame reports it (see `last_frame_had_gl_error()`). /// /// Not thread-safe, and confined to the thread that owns the current context. class Renderer { @@ -87,6 +89,10 @@ class Renderer { /// the renderer draws with -- set colours and metrics here, not on the plot. /// The plot also borrows this renderer's font (when it has no backend of its /// own), so the renderer must outlive the plot's use of it. + /// + /// `Plot::layout()` is computed exactly once, here, and handed to every helper: + /// it runs the auto scale, the strip fixed-point loop and the grid build, so + /// re-deriving it per helper would rebuild the ticks and the grid per helper. void draw(Plot& plot); /// Ends the frame, draining `glGetError()` in a loop so a stale error cannot be @@ -107,7 +113,10 @@ class Renderer { render::Style& style() noexcept { return style_; } const render::Style& style() const noexcept { return style_; } - /// True when the last `end_frame()` found a GL error. Errors are drained in a + /// True when the last frame reported a GL failure: an error drained from + /// `glGetError()` in `end_frame()`, or a fence the driver refused, which the + /// store reports when the draw that should have been guarded is issued (such a + /// frame has already fallen back to the baseline path). Errors are drained in a /// loop, so a stale one cannot be attributed to a later frame. bool last_frame_had_gl_error() const noexcept { return gl_error_; } @@ -115,9 +124,14 @@ class Renderer { const FrameStats& stats() const noexcept { return stats_; } bool ready() const noexcept { return ready_; } - /// True when `init()` selected the persistent-mapped transfer (4.3+ with - /// ARB_buffer_storage, and `PLOT_FORCE_GL33_UPLOAD` unset). - bool using_persistent_upload() const noexcept { return persistent_; } + /// True while both pipelines are uploading through the persistent-mapped ring: + /// `init()` selects it from the caps and the entry points (4.3+ with + /// ARB_buffer_storage, and `PLOT_FORCE_GL33_UPLOAD` unset), and a driver failure + /// later -- a refused map or fence -- leaves the store that hit it on the + /// baseline path, which this then says. + bool using_persistent_upload() const noexcept { + return line_vao_.persistent() && text_vao_.persistent(); + } /// The axis text size the renderer actually draws with: `style().axis_font_px` /// rounded to the nearest whole multiple of `render::BitmapFont::kGlyphPx`, /// because a 1-bit glyph only scales cleanly by whole factors. @@ -191,11 +205,15 @@ class Renderer { void draw_grid(const Plot& plot); void draw_axis_lines(const Plot& plot); - void draw_tick_labels(const Plot& plot); - void draw_titles(const Plot& plot); - void draw_traces(const Plot& plot); - void draw_cursors(const Plot& plot); - void draw_measures(const Plot& plot); + /// The chrome helpers take the frame's layout: `plot.layout()` recomputes the + /// ticks, the strips and the grid, so it belongs to `draw()` alone. + void draw_tick_labels(const Plot& plot, const PlotLayout& layout); + void draw_titles(const Plot& plot, const PlotLayout& layout); + /// The data helpers take it too; they read the axes rect and the transforms it + /// pins. + void draw_traces(const Plot& plot, const PlotLayout& layout); + void draw_cursors(const Plot& plot, const PlotLayout& layout); + void draw_measures(const Plot& plot, const PlotLayout& layout); void draw_annotations(const Plot& plot); render::BitmapFont font_; @@ -218,7 +236,6 @@ class Renderer { u32 fb_w_ = 0; u32 fb_h_ = 0; bool ready_ = false; - bool persistent_ = false; bool gl_error_ = false; FrameStats stats_{}; }; diff --git a/include/plot/render/vertex_array.hpp b/include/plot/render/vertex_array.hpp index 8158827..614edd8 100644 --- a/include/plot/render/vertex_array.hpp +++ b/include/plot/render/vertex_array.hpp @@ -31,9 +31,14 @@ class LineVertexArray { void bind() const noexcept { vao_.bind(); } /// Records that the draw just issued read the instance buffer's current slot, - /// which is what lets the mapped path reuse it safely. A no-op on the baseline - /// path, so a caller always calls it the same way. - void mark_drawn() noexcept { buffer_.mark_drawn(); } + /// which is what lets the mapped path reuse it safely. True in the baseline + /// mode, so a caller always calls it the same way. False only when the driver + /// refused the fence: the buffer has then fallen back to the baseline path, and + /// `persistent()` says so from the next call on. + bool mark_drawn() noexcept { return buffer_.mark_drawn(); } + /// True while this array's instance buffer is the mapped ring (see + /// `UploadMode`); a failure that falls back clears it. + bool persistent() const noexcept { return buffer_.persistent(); } bool valid() const noexcept { return vao_.id() != 0; } u32 instance_count() const noexcept { return instances_; } @@ -57,7 +62,8 @@ class TextVertexArray { void upload(const f32* interleaved, u32 glyphs) noexcept; void bind() const noexcept { vao_.bind(); } - void mark_drawn() noexcept { buffer_.mark_drawn(); } + bool mark_drawn() noexcept { return buffer_.mark_drawn(); } + bool persistent() const noexcept { return buffer_.persistent(); } bool valid() const noexcept { return vao_.id() != 0; } u32 instance_count() const noexcept { return instances_; } diff --git a/src/gl/handle.cpp b/src/gl/handle.cpp index 33d8372..5ddf280 100644 --- a/src/gl/handle.cpp +++ b/src/gl/handle.cpp @@ -93,6 +93,7 @@ bool Buffer::create(Status* st) noexcept { return fail(st, ErrorCode::GLFailure, "glGenBuffers returned 0"); } id_ = name; + note_allocated(); return true; } @@ -146,6 +147,7 @@ bool VertexArray::create(Status* st) noexcept { return fail(st, ErrorCode::GLFailure, "glGenVertexArrays returned 0"); } id_ = name; + note_allocated(); return true; } @@ -220,6 +222,7 @@ Program Program::from_source(const char* vertex_src, const char* fragment_src, } program.id_ = name; + program.note_allocated(); return program; } @@ -285,6 +288,7 @@ bool Texture::create_rgba(u32 width, u32 height, Status* st) noexcept { return fail(st, ErrorCode::GLFailure, "glGenTextures returned 0"); } id_ = name; + note_allocated(); return true; } @@ -304,6 +308,7 @@ bool Texture::create_r8(u32 width, u32 height, Status* st) noexcept { return fail(st, ErrorCode::GLFailure, "glGenTextures returned 0"); } id_ = name; + note_allocated(); return true; } @@ -350,6 +355,7 @@ bool Framebuffer::create(Status* st) noexcept { return fail(st, ErrorCode::GLFailure, "glGenFramebuffers returned 0"); } id_ = name; + note_allocated(); return true; } diff --git a/src/plot/plot.cpp b/src/plot/plot.cpp index 57c4df6..778cbf6 100644 --- a/src/plot/plot.cpp +++ b/src/plot/plot.cpp @@ -310,18 +310,36 @@ const PlotLayout& Plot::layout() { y_width = y_axes_.empty() ? 0.0f : 2.0f * pad; x_height = text_->height() + 2.0f * pad; } + // The title strips, though, are needed before the loop and never change, unlike + // the label strips: a strip holds exactly one text line, so no tick label can + // make it wider. Nothing at all is reserved without a text backend -- there is + // then no title to measure, which is what keeps the two defaults above + // meaningful for the no-text-backend case. The x strip is one text line tall; + // the y strip is one horizontal run wide (see y_title_strip_width). + const f32 y_title_w = text_ != nullptr ? y_title_strip_width() : 0.0f; + const f32 x_title_h = text_ != nullptr && has_x_title() ? text_->height() + 2.0f * pad : 0.0f; for (u32 pass = 0;; ++pass) { // Clamped to the viewport so a huge label set can never push the axes rect - // outside it (or make its width negative). + // outside it (or make its width negative). The title strips take what is left + // after the label strip, so the two can never add up past the viewport. y_width = std::clamp(y_width, 0.0f, viewport_.w); x_height = std::clamp(x_height, 0.0f, viewport_.h); + const f32 y_title_width = std::clamp(y_title_w, 0.0f, viewport_.w - y_width); + const f32 x_title_height = std::clamp(x_title_h, 0.0f, viewport_.h - x_height); layout_.y_axis_width = y_width; layout_.x_axis_height = x_height; - layout_.axes = Rect{viewport_.x + y_width, viewport_.y, viewport_.w - y_width, - viewport_.h - x_height}; - layout_.y_label_area = Rect{viewport_.x, layout_.axes.y, y_width, layout_.axes.h}; + layout_.y_title_width = y_title_width; + layout_.x_title_height = x_title_height; + layout_.axes = Rect{viewport_.x + y_width + y_title_width, viewport_.y, + viewport_.w - (y_width + y_title_width), + viewport_.h - (x_height + x_title_height)}; + layout_.y_title_area = Rect{viewport_.x, layout_.axes.y, y_title_width, layout_.axes.h}; + layout_.y_label_area = + Rect{layout_.y_title_area.right(), layout_.axes.y, y_width, layout_.axes.h}; layout_.x_label_area = Rect{layout_.axes.x, layout_.axes.bottom(), layout_.axes.w, x_height}; + layout_.x_title_area = Rect{layout_.axes.x, layout_.x_label_area.bottom(), layout_.axes.w, + x_title_height}; refresh_ticks(); @@ -367,6 +385,30 @@ f64 Plot::snapped_x(const Cursor& c) const { return c.x; } +bool Plot::has_x_title() const noexcept { + for (const Axis& a : x_axes_) { + if (!a.label.empty()) return true; + } + return false; +} + +bool Plot::has_y_title() const noexcept { + for (const Axis& a : y_axes_) { + if (!a.label.empty()) return true; + } + return false; +} + +f32 Plot::y_title_strip_width() const { + if (text_ == nullptr) return 0.0f; + f32 widest = 0.0f; + for (const Axis& a : y_axes_) { + if (a.label.empty()) continue; + widest = std::max(widest, text_->measure(a.label.c_str())); + } + return widest > 0.0f ? widest + 2.0f * style_.grid_label_pad : 0.0f; +} + f32 Plot::y_label_width() const { if (text_ == nullptr || y_axes_.empty()) return 0.0f; f32 widest = 0.0f; diff --git a/src/render/buffer.cpp b/src/render/buffer.cpp index c2b536b..f702137 100644 --- a/src/render/buffer.cpp +++ b/src/render/buffer.cpp @@ -58,7 +58,10 @@ bool InstanceStore::grow_persistent(usize bytes) noexcept { if (mapped_ == nullptr) { // The driver refused the storage or the mapping. A frame that draws through // the baseline path beats one that draws nothing, so re-create the empty - // buffer the orphan path needs and let the next upload use it. + // buffer the orphan path needs and let the next upload use it. The vertex + // array's attribute pointers went out with the buffer they captured, and the + // epoch reports it (`gl::Buffer::generation()`), which is what re-points them + // at this buffer before the draw. release_storage(); mode_ = UploadMode::Orphan; capacity_ = 0; @@ -68,7 +71,6 @@ bool InstanceStore::grow_persistent(usize bytes) noexcept { } slot_bytes_ = bytes; slot_ = kSlots - 1u; // fresh storage: the first upload takes slot 0 - ++epoch_; return true; } @@ -102,16 +104,30 @@ void InstanceStore::upload(const void* data, u32 count, usize instance_bytes) no } // MAP_COHERENT storage needs no explicit flush, so this is the whole transfer. std::memcpy(mapped_ + usize(slot_) * slot_bytes_, data, bytes); - ++epoch_; + ++slot_epoch_; } -void InstanceStore::mark_drawn() noexcept { +bool InstanceStore::mark_drawn() noexcept { if (mode_ != UploadMode::PersistentMapped || mapped_ == nullptr) { - return; + return true; // nothing reads the store after the draw } Status st; fences_[slot_].reset(); - (void)fences_[slot_].create(&st); + if (fences_[slot_].create(&st)) { + return true; + } + // A slot whose fence the driver refused can never be reused safely: the next + // upload would memcpy into storage the GPU may still be reading, which is the + // tearing the ring exists to prevent. So the ring is abandoned rather than + // rotated through -- the map failure's treatment, one step later -- and the + // fresh buffer the orphan path needs is created. Deleting the mapped buffer is + // safe while the draw still reads it: GL keeps the storage alive until then. + release_storage(); + mode_ = UploadMode::Orphan; + capacity_ = 0; + Status retry; + (void)buffer_.create(&retry); // a failed retry leaves nothing to draw with + return false; } } // namespace detail diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index 7d917ab..378ddff 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -91,10 +91,11 @@ bool Renderer::init(Status* st) noexcept { // best-effort (see api.hpp), so a context can advertise the extension without // the entry points being reachable. const gl::Caps& caps = gl::caps(); - persistent_ = caps.buffer_storage && caps.persistent_map && gl::glBufferStorage != nullptr && - gl::glMapBufferRange != nullptr && !force_gl33_upload(); + const bool persistent = caps.buffer_storage && caps.persistent_map && + gl::glBufferStorage != nullptr && gl::glMapBufferRange != nullptr && + !force_gl33_upload(); const render::UploadMode mode = - persistent_ ? render::UploadMode::PersistentMapped : render::UploadMode::Orphan; + persistent ? render::UploadMode::PersistentMapped : render::UploadMode::Orphan; font_backend_.reset(font_, style_.axis_font_px); @@ -176,17 +177,20 @@ void Renderer::draw(Plot& plot) { // from the values the renderer draws with, so the label pad is never one number // for the layout and another for the text placed inside it. plot.set_style(style_); - (void)plot.layout(); // autoscale, ticks and grid for the final axes size + // One layout for the whole frame: autoscale, the strip fixed-point loop with + // its tick rebuilds, and the grid build all run in here, and every helper + // below reads the strips, the axes rect and the transforms it pins. + const PlotLayout& layout = plot.layout(); // Draw order: chrome, then data, then the annotations over it, and the text // last (it is texture-bound, so it is one draw call after every line batch). draw_grid(plot); draw_axis_lines(plot); - draw_tick_labels(plot); - draw_titles(plot); - draw_traces(plot); - draw_cursors(plot); - draw_measures(plot); + draw_tick_labels(plot, layout); + draw_titles(plot, layout); + draw_traces(plot, layout); + draw_cursors(plot, layout); + draw_measures(plot, layout); draw_annotations(plot); flush_text(); } @@ -241,8 +245,12 @@ void Renderer::flush_lines(const Mapping& map, Color color, f32 width) noexcept gl::glDrawArraysInstanced(gl::GL_TRIANGLE_STRIP, 0, 4, static_cast(line_vao_.instance_count())); // The draw is in the command stream now, so this is the point at which the - // mapped path can let a fence follow it. - line_vao_.mark_drawn(); + // mapped path can let a fence follow it. A driver that refused the fence has + // already dropped this store to the baseline path; the frame reports it here + // rather than rotating through a slot nothing guards. + if (!line_vao_.mark_drawn()) { + gl_error_ = true; + } stats_.staged_segments += segments; ++stats_.draw_calls; } @@ -265,7 +273,9 @@ void Renderer::flush_text() noexcept { text_vao_.bind(); gl::glDrawArraysInstanced(gl::GL_TRIANGLE_STRIP, 0, 4, static_cast(text_vao_.instance_count())); - text_vao_.mark_drawn(); + if (!text_vao_.mark_drawn()) { + gl_error_ = true; + } ++stats_.draw_calls; } glyphs_.clear(); @@ -363,8 +373,7 @@ void Renderer::draw_axis_lines(const Plot& plot) { } } -void Renderer::draw_tick_labels(const Plot& plot) { - const PlotLayout& layout = plot.layout(); +void Renderer::draw_tick_labels(const Plot& plot, const PlotLayout& layout) { const f32 line_h = font_backend_.height(); // X labels: centred under their tick, on the strip's single text line. @@ -397,34 +406,41 @@ void Renderer::draw_tick_labels(const Plot& plot) { } } -void Renderer::draw_titles(const Plot& plot) { - // The layout reserves one text line per strip for the tick labels, so an axis - // title has no room outside the axes rect. It hugs the inside of its axis line - // instead, at the far end of the axis, where it needs no extra layout space and - // cannot collide with the labels of the axis it names. - const Rect& axes = plot.layout().axes; +void Renderer::draw_titles(const Plot& plot, const PlotLayout& layout) { + // Every title goes in the strip its direction reserved for it, one text line + // outside the tick labels: below the x labels for an x axis, beside the y labels + // for a y axis. The strips are what makes a title over the data impossible -- + // `layout.axes` is inset by them -- rather than merely unlikely, so nothing here + // may fall back to an inside-the-axes position. const f32 line_h = font_backend_.height(); for (u32 i = 0; i < plot.x_axis_count(); ++i) { const Axis& axis = plot.x_axis(i); if (axis.label.empty()) continue; + // Centred on the strip: the tick labels have their own row above it, so the + // title needs no alignment of its own. const f32 width = font_backend_.measure(axis.label.c_str()); - const f32 top = axis.place == AxisPlace::Top ? axes.y + 2.0f : axes.bottom() - line_h - 2.0f; - stage_text({axes.right() - width - 2.0f, top}, axis.label.c_str()); + stage_text({layout.x_title_area.x + 0.5f * (layout.x_title_area.w - width), + layout.x_title_area.y + 0.5f * (layout.x_title_area.h - line_h)}, + axis.label.c_str()); } for (u32 i = 0; i < plot.y_axis_count(); ++i) { const Axis& axis = plot.y_axis(i); if (axis.label.empty()) continue; + // A y title is a horizontal run in the vertical strip the layout sized for + // the widest of them, centred on the axes' height: it reads like the x title, + // only beside the plot instead of under it. A run longer than the axes is + // clipped by the viewport, never folded back over the data. const f32 width = font_backend_.measure(axis.label.c_str()); - const f32 left = axis.place == AxisPlace::Right ? axes.right() - width - 2.0f : axes.x + 2.0f; - stage_text({left, axes.y + 2.0f}, axis.label.c_str()); + stage_text({layout.y_title_area.x + 0.5f * (layout.y_title_area.w - width), + layout.y_title_area.y + 0.5f * (layout.y_title_area.h - line_h)}, + axis.label.c_str()); } } // ------------------------------------------------------------------- data -void Renderer::draw_traces(const Plot& plot) { - const PlotLayout& layout = plot.layout(); +void Renderer::draw_traces(const Plot& plot, const PlotLayout& layout) { if (!(layout.axes.w > 0.0f)) { return; // a collapsed plot area has no pixel to select a level for } @@ -472,8 +488,7 @@ void Renderer::draw_traces(const Plot& plot) { } } -void Renderer::draw_cursors(const Plot& plot) { - const PlotLayout& layout = plot.layout(); +void Renderer::draw_cursors(const Plot& plot, const PlotLayout& layout) { const Range y_range = plot.y_range(0); const bool log_y = plot.y_axis(0).log; @@ -495,8 +510,7 @@ void Renderer::draw_cursors(const Plot& plot) { } } -void Renderer::draw_measures(const Plot& plot) { - const PlotLayout& layout = plot.layout(); +void Renderer::draw_measures(const Plot& plot, const PlotLayout& layout) { const Rect& axes = layout.axes; const Range y_range = plot.y_range(0); const bool log_y = plot.y_axis(0).log; @@ -544,8 +558,9 @@ void Renderer::draw_measures(const Plot& plot) { const f32 limit = std::max(axes.x + 2.0f, axes.right() - width - 2.0f); const f32 left = std::clamp(0.5f * (x0 + x1) - 0.5f * width, axes.x + 2.0f, limit); // The second line of the plot area, not the first: the first belongs to the - // y axis title (and to cursor labels), and a wide readout clamped to the left - // edge would overprint them. + // cursor labels, and a wide readout clamped to the left edge would overprint + // them. The y axis title is not a consideration here -- it lives in its own + // strip outside the axes. stage_text({left, axes.y + line_h + 2.0f}, readout); } } diff --git a/tests/test_plot.cpp b/tests/test_plot.cpp index 9b01f18..10e8d35 100644 --- a/tests/test_plot.cpp +++ b/tests/test_plot.cpp @@ -63,6 +63,12 @@ PLOT_TEST(layout_without_text_backend_uses_default_label_areas) { PLOT_CHECK_NEAR(l.x_label_area.y, 378.0f, 1e-6); PLOT_CHECK_NEAR(l.x_label_area.w, 744.0f, 1e-6); PLOT_CHECK_NEAR(l.x_label_area.h, 22.0f, 1e-6); + // "v" is a title, but no backend can measure it, so nothing is reserved: the + // defaults above are what a plot without a text backend keeps laying out with. + PLOT_CHECK_NEAR(l.y_title_width, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.x_title_height, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.y_title_area.w, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.x_title_area.h, 0.0f, 1e-6); } PLOT_TEST(layout_derives_label_areas_from_font_metrics) { @@ -70,13 +76,45 @@ PLOT_TEST(layout_derives_label_areas_from_font_metrics) { plot::Plot p; p.set_text_backend(&font); p.set_viewport({0.0f, 0.0f, 800.0f, 400.0f}); - const u32 ay = p.add_y_axis({"v", {-1000.0, 1000.0}, false, plot::AxisPlace::Left, false}); + const u32 ay = p.add_y_axis({"volts", {-1000.0, 1000.0}, false, plot::AxisPlace::Left, false}); const plot::PlotLayout& l = p.layout(); const f32 pad = p.style().grid_label_pad; PLOT_CHECK_NEAR(l.y_axis_width, widest_major_label(p.y_axis(ay), font) + 2.0f * pad, 1e-4); PLOT_CHECK_NEAR(l.x_axis_height, font.height() + 2.0f * pad, 1e-4); + // The y axis carries the title "volts": it is drawn horizontally, so the strip + // left of the label strip is as wide as the measured title. There is no x axis, + // so the x direction reserves nothing beyond its label strip. + PLOT_CHECK_NEAR(l.y_title_width, font.measure("volts") + 2.0f * pad, 1e-4); + PLOT_CHECK_NEAR(l.x_title_height, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.axes.x, l.y_title_width + l.y_axis_width, 1e-4); + PLOT_CHECK_NEAR(l.axes.w, 800.0f - (l.y_title_width + l.y_axis_width), 1e-4); + PLOT_CHECK_NEAR(l.axes.h, 400.0f - (l.x_axis_height + l.x_title_height), 1e-4); + // The strips tile the viewport outside `axes`: title, then labels, then data. + PLOT_CHECK_NEAR(l.y_title_area.x, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.y_title_area.right(), l.y_label_area.x, 1e-4); + PLOT_CHECK_NEAR(l.y_label_area.right(), l.axes.x, 1e-4); + PLOT_CHECK_NEAR(l.x_title_area.y, l.x_label_area.bottom(), 1e-4); + PLOT_CHECK_NEAR(l.x_title_area.bottom(), l.axes.bottom() + l.x_label_area.h + l.x_title_height, + 1e-4); + PLOT_CHECK_NEAR(l.x_label_area.y, l.axes.bottom(), 1e-4); +} + +/// An axis with no title reserves nothing: the strips exist so that a titled axis +/// has somewhere outside `axes` to draw in, not as a fixed cost per plot. +PLOT_TEST(layout_reserves_no_title_strip_without_a_title) { + plot::render::BitmapFont font; + plot::Plot p; + p.set_text_backend(&font); + p.set_viewport({0.0f, 0.0f, 800.0f, 400.0f}); + p.add_x_axis({"", {0.0, 1.0}, false, plot::AxisPlace::Bottom, false}); + p.add_y_axis({"", {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + const plot::PlotLayout& l = p.layout(); + PLOT_CHECK_NEAR(l.y_title_width, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.x_title_height, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.y_title_area.w, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.x_title_area.h, 0.0f, 1e-6); + PLOT_CHECK_NEAR(l.y_label_area.x, 0.0f, 1e-6); PLOT_CHECK_NEAR(l.axes.x, l.y_axis_width, 1e-4); - PLOT_CHECK_NEAR(l.axes.w, 800.0f - l.y_axis_width, 1e-4); PLOT_CHECK_NEAR(l.axes.h, 400.0f - l.x_axis_height, 1e-4); } diff --git a/tests/test_render_smoke.cpp b/tests/test_render_smoke.cpp index 0ea9dc8..b3fe4dd 100644 --- a/tests/test_render_smoke.cpp +++ b/tests/test_render_smoke.cpp @@ -101,6 +101,56 @@ void dump_if_requested(const plot::Renderer& r, const char* default_name) { PLOT_CHECK(ok); } +/// 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. +bool pixel_is(const plot::u8 px[4], plot::Color c) { + const auto near = [](plot::u8 got, plot::f32 want) { + const int diff = static_cast(got) - static_cast(want * 255.0f + 0.5f); + return diff >= -3 && diff <= 3; + }; + return near(px[0], c.r) && near(px[1], c.g) && near(px[2], c.b); +} + +/// 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 +/// -- is testable without a broken driver. The pointer is saved on construction +/// and put back in the destructor, which also runs while a failed `PLOT_CHECK` +/// unwinds, so a case can never leak its stub into the next one. +class RefusePersistentMap { + public: + RefusePersistentMap() noexcept : saved_(plot::gl::glMapBufferRange) { + plot::gl::glMapBufferRange = &refuse; + } + ~RefusePersistentMap() { plot::gl::glMapBufferRange = saved_; } + RefusePersistentMap(const RefusePersistentMap&) = delete; + RefusePersistentMap& operator=(const RefusePersistentMap&) = delete; + + private: + static void* refuse(plot::gl::GLenum, plot::gl::GLintptr, plot::gl::GLsizeiptr, + plot::gl::GLbitfield) noexcept { + return nullptr; + } + decltype(plot::gl::glMapBufferRange) 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 { + public: + RefuseFenceSync() noexcept : saved_(plot::gl::glFenceSync) { plot::gl::glFenceSync = &refuse; } + ~RefuseFenceSync() { plot::gl::glFenceSync = saved_; } + RefuseFenceSync(const RefuseFenceSync&) = delete; + RefuseFenceSync& operator=(const RefuseFenceSync&) = delete; + + private: + static plot::gl::GLsync refuse(plot::gl::GLenum, plot::gl::GLbitfield) noexcept { + return nullptr; + } + decltype(plot::gl::glFenceSync) saved_; +}; + } // namespace PLOT_TEST(renders_visible_trace_across_viewport) { @@ -343,3 +393,169 @@ PLOT_TEST(draws_cursors_measure_band_and_annotations) { ctx.destroy(); } + +// Finding 2 of the review: `draw_titles()` placed both titles inside +// `layout.axes`, because the layout reserved no room outside it for them, so in +// the dense frame -- whose envelope spans every column's full height -- a title +// sat on the trace and no pixel test could see it. The layout now insets `axes` by +// a strip per direction and the titles are drawn inside those strips, which is +// what makes a title over the data impossible. This draws known titles over that +// same dense envelope and asserts both halves: no title-coloured pixel in a band +// just inside either horizontal edge of `axes`, and the title colour inside both +// strips. The colour is the style's own `axis_text` -- what the text pipeline is +// tinted with -- never a guess, and no other element of the frame can match it +// (the trace is blue-dominant, the grid grey-dark, the axis line mid-grey). +PLOT_TEST(titles_stay_out_of_the_axes_band) { + 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)); + + plot::data::ArraySeries s(sine_samples(), 0.0, 1.0); + plot::Plot p; + p.set_viewport({0, 0, static_cast(kW), static_cast(kH)}); + const plot::u32 ax = p.add_x_axis({"seconds", s.x_bounds(), true, plot::AxisPlace::Bottom, false}); + const plot::u32 ay = p.add_y_axis({"volts", {-1.0, 1.0}, true, plot::AxisPlace::Left, false}); + p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay}); + + r.begin_frame(kW, kH, {0, 0, static_cast(kW), static_cast(kH)}); + r.draw(p); + r.end_frame(); + PLOT_CHECK(!r.last_frame_had_gl_error()); + dump_if_requested(r, "titles.ppm"); + + const plot::PlotLayout& layout = p.layout(); + const plot::Rect& a = layout.axes; + const plot::Color title = r.style().axis_text; + + // Reads a Rect -- whose rows run top-down, like every Rect here -- and reports + // whether any pixel in it is the title colour. + auto area_has_title = [&](const plot::Rect& area) { + for (plot::u32 row = 0; row < static_cast(area.h); ++row) { + for (plot::u32 column = 0; column < static_cast(area.w); ++column) { + plot::u8 px[4] = {}; + PLOT_CHECK(ctx.read_pixel(static_cast(area.x) + column, + ctx.height - 1u - (static_cast(area.y) + row), px)); + if (pixel_is(px, title)) return true; + } + } + return false; + }; + + // One text line plus the two pixels the old placement inset itself by: tall + // enough for the whole title line that used to be drawn there. + const plot::f32 band = r.axis_font_px() + 2.0f; + PLOT_CHECK(!area_has_title({a.x, a.y, a.w, band})); + PLOT_CHECK(!area_has_title({a.x, a.bottom() - band, a.w, band})); + + // ... while the strips the layout reserved hold both titles. + PLOT_CHECK(layout.x_title_area.h > 0.0f); + PLOT_CHECK(layout.y_title_area.w > 0.0f); + PLOT_CHECK(layout.axes.bottom() <= layout.x_title_area.y); // outside the data + PLOT_CHECK(area_has_title(layout.x_title_area)); + PLOT_CHECK(area_has_title(layout.y_title_area)); + ctx.destroy(); +} + +// Finding 1 of the review: when the mapping failed, the store re-created its +// buffer as the orphan path's -- and nothing told the vertex array, so it stayed +// bound to a buffer whose attribute bindings `glDeleteBuffers` had already +// cleared, and every later frame drew nothing at all. The epoch now carries the +// buffer's identity, so the array re-points itself before the draw. The refusal is +// forced through the entry point (`glMapBufferRange` returns null while the caps +// and the pointer still advertise the mapped path, which is exactly the state the +// fallback exists for), and the frame after it must still rasterise the trace. +PLOT_TEST(mapped_ring_fallback_still_draws_after_refused_map) { + plottest::HeadlessContext ctx(kW, kH); + plot::Status st; + PLOT_CHECK(ctx.create(&st)); + PLOT_CHECK(plot::gl::load(nullptr)); + + plot::Renderer r; + { + RefusePersistentMap guard; // before init(): the mapped path is still selected + PLOT_CHECK(r.init(&st)); + const bool forced = r.using_persistent_upload(); + + plot::data::ArraySeries s(sine_samples(), 0.0, 1.0); + plot::Plot p; + p.set_viewport({0, 0, static_cast(kW), static_cast(kH)}); + const plot::u32 ax = p.add_x_axis({"t", s.x_bounds(), true, plot::AxisPlace::Bottom, false}); + const plot::u32 ay = p.add_y_axis({"v", {-1.0, 1.0}, true, plot::AxisPlace::Left, false}); + p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay}); + p.set_x_range(ax, {0.0, 640.0}); // one sample per pixel: an unmistakable polyline + + // The first upload is the one that grows the ring, so it is the one whose map + // is refused and which ends up on the orphan buffer. + r.begin_frame(kW, kH, {0, 0, static_cast(kW), static_cast(kH)}); + r.draw(p); + r.end_frame(); + // Geometry first: this is the assertion the deleted buffer's attribute + // bindings broke, leaving the draw with no attributes to read at all. + PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15); + if (forced) { + PLOT_CHECK(!r.using_persistent_upload()); // the fallback really ran + } + PLOT_CHECK(!r.last_frame_had_gl_error()); + + // ... and the frame after it, which is where the deleted buffer's attribute + // bindings used to leave the pipeline with nothing bound at all. + r.begin_frame(kW, kH, {0, 0, static_cast(kW), static_cast(kH)}); + r.draw(p); + r.end_frame(); + PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15); + PLOT_CHECK(!r.last_frame_had_gl_error()); + } + 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 +// refused fence now drops the ring for the orphan path and reports it, instead of +// rotating through a slot nothing guards. `glFenceSync` is stubbed to hand back no +// sync object while the caps still advertise the mapped path, so the fence is the +// only thing that fails. +PLOT_TEST(refused_fence_drops_the_mapped_ring) { + plottest::HeadlessContext ctx(kW, kH); + plot::Status st; + PLOT_CHECK(ctx.create(&st)); + PLOT_CHECK(plot::gl::load(nullptr)); + + plot::Renderer r; + { + RefuseFenceSync guard; + PLOT_CHECK(r.init(&st)); + const bool forced = r.using_persistent_upload(); + + plot::data::ArraySeries s(sine_samples(), 0.0, 1.0); + plot::Plot p; + p.set_viewport({0, 0, static_cast(kW), static_cast(kH)}); + const plot::u32 ax = p.add_x_axis({"t", s.x_bounds(), true, plot::AxisPlace::Bottom, false}); + const plot::u32 ay = p.add_y_axis({"v", {-1.0, 1.0}, true, plot::AxisPlace::Left, false}); + p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay}); + p.set_x_range(ax, {0.0, 640.0}); + + r.begin_frame(kW, kH, {0, 0, static_cast(kW), static_cast(kH)}); + r.draw(p); + r.end_frame(); + if (forced) { + // The frame whose slot could not be fenced has stopped using the ring: the + // next upload must not write storage the draw may still be reading, and the + // frame reports the failure rather than swallowing it. + PLOT_CHECK(!r.using_persistent_upload()); + PLOT_CHECK(r.last_frame_had_gl_error()); + } + PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15); // it still drew + + // The next frames draw through the orphan path, which needs no fence. + r.begin_frame(kW, kH, {0, 0, static_cast(kW), static_cast(kH)}); + r.draw(p); + r.end_frame(); + PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15); + PLOT_CHECK(!r.last_frame_had_gl_error()); + } + ctx.destroy(); +}