Improve text bounding boxes (fix empty Menu, Textbox issues)

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Changelog
 
+- Improved handling of empty strings in text bounding boxes, fixing errors and broken layouting with empty strings in `Label`, `Menu` and `Textbox` [#5420](https://github.com/MakieOrg/Makie.jl/pull/5420)
+
 ## Unreleased
 
 - Fixes for is_same to work with missing [#5327](https://github.com/MakieOrg/Makie.jl/pull/5327).

CairoMakie/src/scatter.jl

@@ -32,7 +32,7 @@ function draw_atomic(scene::Scene, screen::Screen, plot::Text)
     attr = plot.attributes
     # input -> markerspace
     # TODO: We're doing per-string/glyphcollection work per glyph here
-    cairo_unclipped_indices!(attr)
+    cairo_unclipped_indices!(attr, :per_char_positions_transformed_f32c)
     Makie.register_positions_projected!(
         scene.compute, attr, Point3d;
         input_name = :positions_transformed_f32c, output_name = :positions_in_markerspace,
@@ -137,6 +137,8 @@ function draw_text(ctx, attr::NamedTuple)
     for (block_idx, glyph_indices) in enumerate(text_blocks)
         Cairo.save(ctx)  # Block save
 
+        glyph_pos = positions[block_idx]
+
         for glyph_idx in glyph_indices
             glyph_idx in valid_indices || continue
 
@@ -149,8 +151,6 @@ function draw_text(ctx, attr::NamedTuple)
             strokecolor = Makie.sv_getindex(text_strokecolor, glyph_idx)
             scale = Makie.sv_getindex(text_scales, glyph_idx)
 
-            glyph_pos = positions[glyph_idx]
-
             # Not renderable by font (e.g. '\n')
             glyph == 0 && continue
 
@@ -193,10 +193,10 @@ function draw_text(ctx, attr::NamedTuple)
     return
 end
 
-function cairo_unclipped_indices!(attr::Makie.ComputeGraph)
+function cairo_unclipped_indices!(attr::Makie.ComputeGraph, position_name = :positions_transformed_f32c)
     return Makie.register_computation!(
         attr,
-        [:positions_transformed_f32c, :model_f32c, :space, :clip_planes],
+        [position_name, :model_f32c, :space, :clip_planes],
         [:unclipped_indices]
     ) do (transformed, model, space, clip_planes), changed, outputs
         return (unclipped_indices(to_model_space(model, clip_planes), transformed, space),)

GLMakie/src/plot-primitives.jl

@@ -432,7 +432,7 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Text)
         # is projectionview enough to trigger on scene resize in all cases?
         register_computation!(
             attr,
-            [:positions_transformed_f32c, :projectionview, :model_f32c],
+            [:per_char_positions_transformed_f32c, :projectionview, :model_f32c],
             [:gl_depth_cache, :gl_indices]
         ) do (pos, projectionview, space, model), changed, last
             pvm = projectionview * model
@@ -441,12 +441,12 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Text)
             return depthsort!(pos, depth_vals, indices, pvm)
         end
     else
-        register_computation!(attr, [:positions_transformed_f32c], [:gl_indices]) do (ps,), changed, last
+        register_computation!(attr, [:per_char_positions_transformed_f32c], [:gl_indices]) do (ps,), changed, last
             return (length(ps),)
         end
     end
 
-    register_computation!(attr, [:positions_transformed_f32c], [:gl_len]) do (ps,), changed, last
+    register_computation!(attr, [:per_char_positions_transformed_f32c], [:gl_len]) do (ps,), changed, last
         return (Int32(length(ps)),)
     end
 
@@ -456,7 +456,7 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Text)
 
     # Simple forwards
     uniforms = [
-        :positions_transformed_f32c,
+        :per_char_positions_transformed_f32c,
         :text_color, :text_strokecolor, :text_rotation,
         :marker_offset, :quad_offset, :sdf_uv, :quad_scale,
         :lowclip_color, :highclip_color, :nan_color,
@@ -476,7 +476,7 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Text)
     # O(1) and only takes ~4ns
     input2glname = Dict{Symbol, Symbol}(
         :text_rotation => :rotation,
-        :positions_transformed_f32c => :position,
+        :per_char_positions_transformed_f32c => :position,
         :text_color => :color,
         :sdf_uv => :uv_offset_width,
         :gl_markerspace => :markerspace,

Makie/src/basic_recipes/text.jl

@@ -372,13 +372,6 @@ function register_text_computations!(attr::ComputeGraph)
     # And :glyphcollection if applicable
     compute_glyph_collections!(attr)
 
-    map!(attr, [:text_blocks, :positions], :text_positions) do blocks, pos
-        if length(blocks) != length(pos)
-            error("Text blocks and positions have different lengths: $(length(blocks)) != $(length(pos)). Please use `update!(plot_object; arg1/arg2/text/position/color/etc...) to update multiple attributes together.")
-        end
-        return [p for (b, p) in zip(blocks, pos) for i in b]
-    end
-
     map!(attr, [:atlas, :glyphindices, :font_per_char], :sdf_uv) do atlas, gi, fonts
         return glyph_uv_width!.((atlas,), gi, fonts)
     end
@@ -408,7 +401,15 @@ function register_text_computations!(attr::ComputeGraph)
     end
     # TODO: remapping positions to be per glyph first generates quite a few
     # redundant transform applications and projections in CairoMakie
-    register_position_transforms!(attr, input_name = :text_positions, transformed_name = :positions_transformed)
+    register_position_transforms!(attr, input_name = :positions, transformed_name = :positions_transformed)
+
+    map!(attr, [:text_blocks, :positions_transformed_f32c], :per_char_positions_transformed_f32c) do blocks, pos
+        if length(blocks) != length(pos)
+            error("Text blocks and positions have different lengths: $(length(blocks)) != $(length(pos)). Please use `update!(plot_object; arg1/arg2/text/position/color/etc...) to update multiple attributes together.")
+        end
+        return [p for (b, p) in zip(blocks, pos) for i in b]
+    end
+
     return
 end
 
@@ -445,7 +446,7 @@ function tex_linesegments!(plot)
         markerspace_positions = _project(preprojection * model_f32c, positions, clip_planes, space)
         # TODO: avoid repeated apply_transform and use block_idx?
         return map(linesegments, indices) do seg, (block_idx, glyph_idx)
-            return seg + markerspace_positions[glyph_idx]
+            return seg + markerspace_positions[block_idx]
         end
     end
 
@@ -479,6 +480,36 @@ function register_markerspace_positions!(plot::Text, ::Type{OT} = Point3f; kwarg
     )
 end
 
+struct PerCharIterator{T}
+    blocks::Vector{UnitRange{Int64}}
+    data::Vector{T}
+    is_per_block::Bool
+end
+function PerCharIterator(blocks, data)
+    return PerCharIterator(blocks, data, length(blocks) == length(data))
+end
+
+function Base.iterate(iter::PerCharIterator, state = (1, 1))
+    char_idx, block_idx = state
+    if block_idx > length(iter.blocks) || char_idx > last(last(iter.blocks))
+        return nothing
+    end
+
+    if iter.is_per_block
+
+        if char_idx in iter.blocks[block_idx]
+            return iter.data[block_idx], (char_idx + 1, block_idx)
+        else
+            return iterate(iter, (char_idx, block_idx + 1))
+        end
+    else
+        return iter.data[char_idx], (char_idx + 1, 0)
+    end
+end
+
+Base.length(iter::PerCharIterator) = last(last(iter.blocks))
+
+
 # TODO: anything per-string should include lines?
 
 function register_raw_glyph_boundingboxes!(plot)
@@ -535,11 +566,11 @@ function register_glyph_boundingboxes!(plot)
         register_markerspace_positions!(plot)
         map!(
             plot.attributes,
-            [:raw_glyph_boundingboxes, :marker_offset, :text_rotation, :markerspace_positions],
+            [:raw_glyph_boundingboxes, :marker_offset, :text_rotation, :text_blocks, :markerspace_positions],
             :glyph_boundingboxes
-        ) do bbs, origins, rotations, positions
+        ) do bbs, origins, rotations, blocks, positions
 
-            return map(bbs, origins, rotations, positions) do bb, o, rotation, position
+            return map(bbs, origins, rotations, PerCharIterator(blocks, positions)) do bb, o, rotation, position
                 glyphbb3 = Rect3d(to_ndim(Point3d, origin(bb), 0), to_ndim(Point3d, widths(bb), 0))
                 return rotate_bbox(glyphbb3, rotation) + o + position
             end
@@ -574,7 +605,7 @@ function register_fast_string_boundingboxes!(plot)
         ) do blocks, bbs, origins, rotation, segments, linewidths, lineindices
 
             text_bbs = map(blocks) do idxs
-                output = Rect3d()
+                output = Rect3d(Point3d(NaN), Vec3d(0))
                 for i in idxs
                     glyphbb = bbs[i]
                     glyphbb3 = Rect3d(to_ndim(Point3d, origin(glyphbb), 0), to_ndim(Point3d, widths(glyphbb), 0))
@@ -613,15 +644,16 @@ function register_string_boundingboxes!(plot)
         # project positions to markerspace, add them
         map!(
             plot.attributes,
-            [:text_blocks, :fast_string_boundingboxes, :markerspace_positions],
+            [:fast_string_boundingboxes, :markerspace_positions],
             :string_boundingboxes
-        ) do text_blocks, bbs, positions
-
-            return map(enumerate(text_blocks)) do (i, idxs)
-                if isempty(idxs)
-                    return Rect3d(Point3d(NaN), Vec3d(0))
-                else
-                    return bbs[i] + positions[first(idxs)]
+        ) do bbs, positions
+
+            return map(bbs, positions) do bb, pos
+                mini = minimum(bb)
+                if isfinite(mini)
+                    return bb + pos
+                else # empty bboxes end up as Rect3d(Point3d(Inf), Vec3d(-Inf))
+                    return Rect3d(pos, Vec3d(0))
                 end
             end
         end

Makie/src/makielayout/helpers.jl

@@ -3,9 +3,11 @@ Shorthand for `isnothing(optional) ? fallback : optional`
 """
 @inline ifnothing(optional, fallback) = isnothing(optional) ? fallback : optional
 
+make_finite(v, fallback = 0) = ifelse.(isfinite.(v), v, fallback)
+
 function round_to_IRect2D(r::Rect{2})
-    newori = round.(Int, minimum(r))
-    othercorner = round.(Int, maximum(r))
+    newori = round.(Int, make_finite(minimum(r)))
+    othercorner = round.(Int, make_finite(maximum(r)))
     newwidth = othercorner .- newori
     return Rect{2, Int}(newori, newwidth)
 end

Makie/test/boundingboxes.jl

@@ -1,9 +1,3 @@
-function Base.isapprox(r1::Rect{D}, r2::Rect{D}; kwargs...) where {D}
-    left = vcat(minimum(r1), widths(r1))
-    right = vcat(minimum(r2), widths(r2))
-    return all((isnan.(left) .& isnan.(right)) .| (left .≈ right))
-end
-
 @testset "data_limits(plot)" begin
     ps = Point2f[(0, 0), (1, 1)]
 

Makie/test/issues.jl

@@ -100,7 +100,7 @@
         menu.options[] = zip(labels, values)
         p = menu.blockscene.children[1].plots[2]
         # should not error
-        p.text_positions[]
+        p.positions_transformed_f32c[]
         p.quad_offset[]
         p.sdf_marker_shape[]
         p.sdf_uv[]

Makie/test/plots/text.jl

@@ -144,3 +144,52 @@ end
         end
     end
 end
+
+@testset "text boundingboxes" begin
+    @testset "empty string" begin
+        scene = Scene(camera = campixel!)
+        p = text!(scene, 30, 50, text = "")
+        @test Makie.raw_glyph_boundingboxes(p) == Rect2d[]
+        @test Makie.fast_glyph_boundingboxes(p) == Rect3d[]
+        @test Makie.glyph_boundingboxes(p) == Rect2d[]
+        @test length(Makie.fast_string_boundingboxes(p)) == 1
+        @test Makie.fast_string_boundingboxes(p)[1] ≈ Rect3d(Point3d(NaN), Vec3d(0))
+        @test Makie.string_boundingboxes(p) == [Rect3d(Point3d(30, 50, 0), Vec3d(0))]
+        @test Makie.full_boundingbox(p) == Rect3d(Point3d(30, 50, 0), Vec3d(0))
+    end
+
+    @testset "single string" begin
+        scene = Scene(camera = campixel!)
+        p = text!(scene, 30, 50, text = "val")
+
+        charbbs = [Rect2d(0, -3.0519999265670776, 7.0, 16.309999465942383), Rect2d(0, -3.0519999265670776, 7.783999919891357, 16.309999465942383), Rect2d(0, -3.0519999265670776, 3.1080000400543213, 16.309999465942383)]
+        @test all(Makie.raw_glyph_boundingboxes(p) .≈ charbbs)
+        charbbs = [Rect3d(0.0, 1.1920928955078125e-7, 0, 7.0, 16.309999465942383, 0), Rect3d(7.0, 1.1920928955078125e-7, 0.0, 7.783999919891357, 16.309999465942383, 0.0), Rect3d(14.784000396728516, 1.1920928955078125e-7, 0.0, 3.1080000400543213, 16.309999465942383, 0.0)]
+        @test all(Makie.fast_glyph_boundingboxes(p) .≈ charbbs)
+        @test all(Makie.glyph_boundingboxes(p) .≈ [bb + Point3d(30, 50, 0) for bb in charbbs])
+
+        @test all(Makie.fast_string_boundingboxes(p) .≈ [Rect3d(0.0, 1.1920928955078125e-7, 0.0, 17.892000436782837, 16.309999465942383, 0.0)])
+        @test all(Makie.string_boundingboxes(p) .≈ [Rect3d(30.0, 50.00000011920929, 0.0, 17.892000436782837, 16.309999465942383, 0.0)])
+
+        @test Makie.full_boundingbox(p) ≈ Rect3d(30.0, 50.00000011920929, 0.0, 17.892000436782837, 16.309999465942383, 0.0)
+    end
+
+    @testset "multi string" begin
+        scene = Scene(camera = campixel!)
+        p = text!(scene, [30, 100, 50], [50, 20, 100], text = ["val", "b", ""])
+
+        charbbs = [Rect2d(0.0, -3.0519999265670776, 7.0, 16.309999465942383), Rect2d(0.0, -3.0519999265670776, 7.783999919891357, 16.309999465942383), Rect2d(0.0, -3.0519999265670776, 3.1080000400543213, 16.309999465942383), Rect2d(0.0, -3.0519999265670776, 7.783999919891357, 16.309999465942383)]
+        @test all(Makie.raw_glyph_boundingboxes(p) .≈ charbbs)
+        charbbs = [Rect3d(0.0, 1.1920928955078125e-7, 0.0, 7.0, 16.309999465942383, 0.0), Rect3d(7.0, 1.1920928955078125e-7, 0.0, 7.783999919891357, 16.309999465942383, 0.0), Rect3d(14.784000396728516, 1.1920928955078125e-7, 0.0, 3.1080000400543213, 16.309999465942383, 0.0), Rect3d(0.0, 1.1920928955078125e-7, 0.0, 7.783999919891357, 16.309999465942383, 0.0)]
+        @test all(Makie.fast_glyph_boundingboxes(p) .≈ charbbs)
+        charbbs = [charbbs[1] + Point3d(30, 50, 0), charbbs[2] + Point3d(30, 50, 0), charbbs[3] + Point3d(30, 50, 0), charbbs[4] + Point3d(100, 20, 0)]
+        @test all(Makie.glyph_boundingboxes(p) .≈ charbbs)
+
+        stringbbs = [Rect3d(0.0, 1.1920928955078125e-7, 0.0, 17.892000436782837, 16.309999465942383, 0.0), Rect3d(0.0, 1.1920928955078125e-7, 0.0, 7.783999919891357, 16.309999465942383, 0.0), Rect3d(NaN, NaN, NaN, 0.0, 0.0, 0.0)]
+        @test all(Makie.fast_string_boundingboxes(p) .≈ stringbbs)
+        stringbbs = [Rect3d(30.0, 50.00000011920929, 0.0, 17.892000436782837, 16.309999465942383, 0.0), Rect3d(100.0, 20.00000011920929, 0.0, 7.783999919891357, 16.309999465942383, 0.0), Rect3d(50.0, 100.0, 0.0, 0.0, 0.0, 0.0)]
+        @test all(Makie.string_boundingboxes(p) .≈ stringbbs)
+
+        @test Makie.full_boundingbox(p) ≈ Rect3d(30.0, 20.00000011920929, 0.0, 77.78399991989136, 79.99999988079071, 0.0)
+    end
+end

Makie/test/runtests.jl

@@ -12,6 +12,14 @@ using Makie.IntervalSets
 using GeometryBasics: Pyramid
 using Makie.ComputePipeline: ResolveException
 using Makie: volume
+
+# for texting bounding boxes
+function Base.isapprox(r1::Rect{D}, r2::Rect{D}; kwargs...) where {D}
+    left = vcat(minimum(r1), widths(r1))
+    right = vcat(minimum(r2), widths(r2))
+    return all((isnan.(left) .& isnan.(right)) .| (left .≈ right))
+end
+
 # COV_EXCL_STOP
 
 @testset "Unit tests" begin

WGLMakie/assets/sprites.vert

@@ -89,7 +89,7 @@ void main(){
     mat4 pview = projection * view;
     mat4 trans = get_transform_marker() ? model_f32c : mat4(1.0);
 
-    vec4 position_world = model_f32c * vec4(tovec3(get_positions_transformed_f32c()), 1);
+    vec4 position_world = model_f32c * vec4(tovec3(get_wgl_positions()), 1);
     process_clip_planes(position_world.xyz);
 
     // Compute centre of billboard in clipping coordinates

WGLMakie/src/plot-primitives.jl

@@ -173,7 +173,8 @@ function handle_color_getter!(uniform_dict)
 end
 
 function assemble_particle_robj!(attr, data)
-    data[:positions_transformed_f32c] = attr.positions_transformed_f32c
+    pos_key = haskey(attr, :wgl_positions) ? :wgl_positions : :positions_transformed_f32c
+    data[pos_key] = getproperty(attr, pos_key)
     handle_color!(data, attr)
     handle_color_getter!(data)
 
@@ -190,7 +191,7 @@ function assemble_particle_robj!(attr, data)
     data[:transform_marker] = attr.transform_marker
     per_instance_keys = Set(
         [
-            :positions_transformed_f32c, :converted_rotation, :quad_offset, :quad_scale, :vertex_color,
+            pos_key, :converted_rotation, :quad_offset, :quad_scale, :vertex_color,
             :intensity, :sdf_uv, :converted_strokecolor, :marker_offset, :markersize,
         ]
     )
@@ -277,11 +278,12 @@ function create_shader(scene::Scene, plot::Scatter)
 
     # ComputePipeline.alias!(attr, :rotation, :converted_rotation)
     ComputePipeline.alias!(attr, :strokecolor, :converted_strokecolor)
+    ComputePipeline.alias!(attr, :positions_transformed_f32c, :wgl_positions)
 
     Makie.add_computation!(attr, scene, Val(:meshscatter_f32c_scale))
     backend_colors!(attr, :scatter_color)
     inputs = [
-        :positions_transformed_f32c,
+        :wgl_positions,
 
         :vertex_color, :uniform_color, :uniform_colormap,
         :uniform_colorrange, :nan_color, :highclip_color,
@@ -365,8 +367,9 @@ function create_shader(scene::Scene, plot::Makie.Text)
 
     ComputePipeline.alias!(attr, :text_rotation, :converted_rotation)
     ComputePipeline.alias!(attr, :text_strokecolor, :converted_strokecolor)
+    ComputePipeline.alias!(attr, :per_char_positions_transformed_f32c, :wgl_positions)
     inputs = [
-        :positions_transformed_f32c,
+        :wgl_positions,
 
         :vertex_color, :uniform_color, :uniform_colormap, :uniform_colorrange,
         :nan_color, :highclip_color, :lowclip_color, :pattern,