consider model transformations in float32 conversions (#4026)

* prototype model fixes * add decomposition for model matrix * update some comments, space for model * add model-on-GPU branch, simplify some code * remove unused code * fix typo * fix typo * fix output type * update WGLMakie * test more than one transformation matrix * make sure z scaling & translation apply * start updating tests * test edge case * keep safe scale+translation model matrix * update text_quads * add refimg test and fix styled lines * use Float64 in apply_transform_and_model and fix some bbox issues * add refimg with model rotation * cleanup heatmap * skip rotated heatmap * fix test * cleanup conversion outputs * update changelog * allow fast past with f32 conversion --------- Co-authored-by: Simon <sdanisch@protonmail.com>
MakieOrg · Aug 9, 2024 · 12d0e67 · 12d0e67
1 parent d7b5a11
commit 12d0e67
Show file tree

Hide file tree

Showing 17 changed files with 714 additions and 149 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [Unreleased]
 
+- Updated handling of `model` matrices with active Float32 rescaling. This should fix issues with Float32-unsafe translations or scalings of plots, as well as rotated plots in Float32-unsafe ranges. [#4026](https://github.com/MakieOrg/Makie.jl/pull/4026)
 - Added `events.tick` to allow linking actions like animations to the renderloop. [#3948](https://github.com/MakieOrg/Makie.jl/pull/3948)
 - Added the `uv_transform` attribute for meshscatter, mesh, surface and image [#1406](https://github.com/MakieOrg/Makie.jl/pull/1406).
 - Added the ability to use textures with `meshscatter` in WGLMakie [#1406](https://github.com/MakieOrg/Makie.jl/pull/1406).

diff --git a/GLMakie/src/drawing_primitives.jl b/GLMakie/src/drawing_primitives.jl
@@ -257,7 +257,11 @@ function cached_robj!(robj_func, screen, scene, plot::AbstractPlot)
             gl_value = lift_convert(key, value, plot, screen)
             gl_key => gl_value
         end)
-        gl_attributes[:model] = map(Makie.patch_model, f32_conversion_obs(plot), plot.model)
+
+        # :f32c should get passed to apply_transform_and_f32_conversion but not
+        # make it to uniforms
+        gl_attributes[:f32c], gl_attributes[:model] = Makie.patch_model(plot)
+
         if haskey(plot, :markerspace)
             gl_attributes[:markerspace] = plot.markerspace
         end
@@ -303,7 +307,7 @@ function cached_robj!(robj_func, screen, scene, plot::AbstractPlot)
         elseif shading == MultiLightShading
             handle_lights(gl_attributes, screen, scene.lights)
         end
-        robj = robj_func(gl_attributes) # <-- here
+        robj = robj_func(gl_attributes)
 
         get!(gl_attributes, :ssao, Observable(false))
         screen.cache2plot[robj.id] = plot
@@ -384,8 +388,7 @@ function draw_atomic(screen::Screen, scene::Scene, @nospecialize(plot::Union{Sca
 
         space = plot.space
         positions = handle_view(plot[1], gl_attributes)
-        positions = apply_transform_and_f32_conversion(scene, plot, positions)
-        # positions = lift(apply_transform, plot, transform_func_obs(plot), positions, space)
+        positions = apply_transform_and_f32_conversion(plot, pop!(gl_attributes, :f32c), positions)
         cam = scene.camera
 
         if plot isa Scatter
@@ -466,21 +469,22 @@ function draw_atomic(screen::Screen, scene::Scene, @nospecialize(plot::Lines))
             data[:pattern] = nothing
             data[:fast] = true
 
-            # positions = lift(apply_transform, plot, transform_func, positions, space)
-            positions = apply_transform_and_f32_conversion(scene, plot, positions)
+            positions = apply_transform_and_f32_conversion(plot, pop!(data, :f32c), positions)
         else
             data[:pattern] = linestyle
             data[:fast] = false
 
-            pvm = lift(*, plot, data[:projectionview], data[:model])
+            # TODO: Skip patch_model() when this branch is used
+            pop!(data, :f32c)
+            pvm = lift(plot, data[:projectionview], plot.model, f32_conversion_obs(scene), space) do pv, model, f32c, space
+                Makie.Mat4d(pv) * Makie.f32_convert_matrix(f32c, space) * model
+            end
             transform_func = transform_func_obs(plot)
-            positions = lift(plot, f32_conversion_obs(scene), transform_func, positions,
-                    space, pvm) do f32c, f, ps, space, pvm
-
-                transformed = apply_transform_and_f32_conversion(f32c, f, ps, space)
+            positions = lift(plot, transform_func, positions, space, pvm) do f, ps, space, pvm
+                transformed = apply_transform(f, ps, space)
                 output = Vector{Point4f}(undef, length(transformed))
                 for i in eachindex(transformed)
-                    output[i] = pvm * to_ndim(Point4f, to_ndim(Point3f, transformed[i], 0f0), 1f0)
+                    output[i] = pvm * to_ndim(Point4d, to_ndim(Point3d, transformed[i], 0.0), 1.0)
                 end
                 output
             end
@@ -503,8 +507,7 @@ function draw_atomic(screen::Screen, scene::Scene, @nospecialize(plot::LineSegme
         end
 
         positions = handle_view(plot[1], data)
-        # positions = lift(apply_transform, plot, transform_func_obs(plot), positions, plot.space)
-        positions = apply_transform_and_f32_conversion(scene, plot, positions)
+        positions = apply_transform_and_f32_conversion(plot, pop!(data, :f32c), positions)
         if haskey(data, :intensity)
             data[:color] = pop!(data, :intensity)
         end
@@ -518,18 +521,15 @@ function draw_atomic(screen::Screen, scene::Scene,
     return cached_robj!(screen, scene, plot) do gl_attributes
         glyphcollection = plot[1]
 
-        transfunc = Makie.transform_func_obs(plot)
-        pos = gl_attributes[:position]
+        pos = apply_transform_and_f32_conversion(plot, pop!(gl_attributes, :f32c), gl_attributes[:position])
         space = plot.space
         markerspace = plot.markerspace
         offset = pop!(gl_attributes, :offset, Vec2f(0))
         atlas = gl_texture_atlas()
 
         # calculate quad metrics
-        glyph_data = lift(
-                plot, pos, glyphcollection, offset, f32_conversion_obs(scene), transfunc, space
-            ) do pos, gc, offset, f32c, transfunc, space
-            return Makie.text_quads(atlas, pos, to_value(gc), offset, f32c, transfunc, space)
+        glyph_data = lift(plot, pos, glyphcollection, offset) do pos, gc, offset
+            return Makie.text_quads(atlas, pos, to_value(gc), offset)
         end
 
         # unpack values from the one signal:
@@ -601,19 +601,17 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Heatmap)
         t = Makie.transform_func_obs(plot)
         mat = plot[3]
         space = plot.space # needs to happen before connect_camera! call
-        xypos = lift(plot, f32_conversion_obs(scene), t, plot[1], plot[2], space) do f32c, t, x, y, space
+        xypos = lift(plot, pop!(gl_attributes, :f32c), t, plot.model, plot[1], plot[2], space) do f32c, t, model, x, y, space
+            # TODO: fix heatmaps for transforms that mix dimensions:
+            # - transform_func's like Polar
+            # - model matrices with rotation & Float32 precisionissues
             x1d = xy_convert(x, size(mat[], 1))
             y1d = xy_convert(y, size(mat[], 2))
-            # Only if transform doesn't do anything, we can stay linear in 1/2D
-            if Makie.is_identity_transform(t)
-                return (Makie.f32_convert(f32c, x1d, 1), Makie.f32_convert(f32c, y1d, 2))
-            else
-                # If we do any transformation, we have to assume things aren't on the grid anymore
-                # so x + y need to become matrices.
-                x1d = Makie.apply_transform_and_f32_conversion(f32c, t, x1d, 1, space)
-                y1d = Makie.apply_transform_and_f32_conversion(f32c, t, y1d, 2, space)
-                return (x1d, y1d)
-            end
+
+            x1d = Makie.apply_transform_and_f32_conversion(f32c, t, model, x1d, 1, space)
+            y1d = Makie.apply_transform_and_f32_conversion(f32c, t, model, y1d, 2, space)
+
+            return (x1d, y1d)
         end
         xpos = lift(first, plot, xypos)
         ypos = lift(last, plot, xypos)
@@ -644,7 +642,7 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Image)
             rect =  Rect2(xmin, ymin, xmax - xmin, ymax - ymin)
             return decompose(Point2d, rect)
         end
-        gl_attributes[:vertices] = apply_transform_and_f32_conversion(scene, plot, position)
+        gl_attributes[:vertices] = apply_transform_and_f32_conversion(plot, pop!(gl_attributes, :f32c), position)
         rect = Rect2f(0, 0, 1, 1)
         gl_attributes[:faces] = decompose(GLTriangleFace, rect)
         gl_attributes[:texturecoordinates] = decompose_uv(rect)
@@ -708,7 +706,7 @@ function mesh_inner(screen::Screen, mesh, transfunc, gl_attributes, plot, space=
 
     # TODO: avoid intermediate observable
     positions = map(m -> metafree(coordinates(m)), mesh)
-    gl_attributes[:vertices] = apply_transform_and_f32_conversion(Makie.parent_scene(plot), plot, positions)
+    gl_attributes[:vertices] = apply_transform_and_f32_conversion(plot, pop!(gl_attributes, :f32c), positions)
     gl_attributes[:faces] = lift(x-> decompose(GLTriangleFace, x), mesh)
     if hasproperty(to_value(mesh), :uv)
         gl_attributes[:texturecoordinates] = lift(decompose_uv, mesh)
@@ -762,11 +760,11 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Surface)
         if all(T -> T <: Union{AbstractMatrix, AbstractVector}, types)
             t = Makie.transform_func_obs(plot)
             mat = plot[3]
-            xypos = lift(plot, f32_conversion_obs(scene), t, plot[1], plot[2], space) do f32c, t, x, y, space
+            xypos = lift(plot, pop!(gl_attributes, :f32c), plot.model, t, plot[1], plot[2], space) do f32c, model, t, x, y, space
                 # Only if transform doesn't do anything, we can stay linear in 1/2D
                 if Makie.is_identity_transform(t) && isnothing(f32c)
                     return (x, y)
-                else
+                elseif Makie.is_translation_scale_matrix(model)
                     matrix = if x isa AbstractMatrix && y isa AbstractMatrix
                         Makie.f32_convert(f32c, apply_transform.((t,), Point.(x, y), space), space)
                     else
@@ -775,6 +773,15 @@ function draw_atomic(screen::Screen, scene::Scene, plot::Surface)
                         [Makie.f32_convert(f32c, apply_transform(t, Point(x, y), space), space) for x in x, y in y]
                     end
                     return (first.(matrix), last.(matrix))
+                else
+                    matrix = if x isa AbstractMatrix && y isa AbstractMatrix
+                        Makie.f32_convert(f32c, apply_transform_and_model.((model,), (t,), Point.(x, y), space, Point2d), space)
+                    else
+                        # If we do any transformation, we have to assume things aren't on the grid anymore
+                        # so x + y need to become matrices.
+                        [Makie.f32_convert(f32c, apply_transform_and_model(model, t, Point(x, y), space, Point2d), space) for x in x, y in y]
+                    end
+                    return (first.(matrix), last.(matrix))
                 end
             end
             xpos = lift(first, plot, xypos)

diff --git a/ReferenceTests/src/tests/float32_conversion.jl b/ReferenceTests/src/tests/float32_conversion.jl
@@ -98,3 +98,62 @@ end
     )
     fig
 end
+
+@reference_test "Float64 model" begin
+    fig = Figure()
+    ax = Axis(fig[1, 1])
+
+    p = heatmap!(ax, -0.75 .. -0.25, -0.75 .. -0.25, [1 2; 3 4], colormap = [:lightblue, :yellow])
+    translate!(p, 1e9, 1e8, 0)
+    p = image!(ax, 0..1, 0..1, [1 2; 3 4], colormap = [:lightblue, :yellow])
+    translate!(p, 1e9, 1e8, 0)
+
+
+    ps = 0.5 .* Makie.Point2d[(-1, -1), (-1, 1), (1, 1), (1, -1)]
+    p = scatter!(ax, ps, marker = '+', markersize = 30)
+    translate!(p, 1e9, 1e8, 0)
+    p = text!(ax, ps, text = string.(1:4), fontsize = 20)
+    translate!(p, 1e9, 1e8, 0)
+
+    p = lines!(ax, [Point2f(cos(x), sin(x)) for x in range(0, 2pi, length=101)])
+    translate!(p, 1e9, 1e8, 0)
+    p = linesegments!(ax, [0.9 * Point2f(cos(x), sin(x)) for x in range(0, 2pi, length=101)])
+    translate!(p, 1e9, 1e8, 0)
+    p = lines!(ax, [0.8 * Point2f(cos(x), sin(x)) for x in range(0, 2pi, length=101)], linestyle = :dash)
+    translate!(p, 1e9, 1e8, 0)
+
+    fig
+end
+
+@reference_test "Float64 model with rotation" begin
+    fig = Figure()
+    ax = Axis(fig[1, 1])
+
+    # TODO: broken in GLMakie (bad placement), CairoMakie (not supported)
+    # p = heatmap!(ax, -0.75 .. -0.25, -0.75 .. -0.25, [1 2; 3 4], colormap = [:lightblue, :yellow])
+    # translate!(p, 1e9, 1e8, 0)
+    # rotate!(p, Vec3f(0,0,1), pi/4)
+    p = image!(ax, 0..1, 0..1, [1 2; 3 4], colormap = [:lightblue, :yellow])
+    translate!(p, 1e9, 1e8, 0)
+    rotate!(p, Vec3f(0,0,1), pi/4)
+
+    ps = 0.5 .* Makie.Point2d[(-1, -1), (-1, 1), (1, 1), (1, -1)]
+    p = scatter!(ax, ps, marker = '+', markersize = 30)
+    translate!(p, 1e9, 1e8, 0)
+    rotate!(p, Vec3f(0,0,1), pi/4)
+    p = text!(ax, ps, text = string.(1:4), fontsize = 20)
+    translate!(p, 1e9, 1e8, 0)
+    rotate!(p, Vec3f(0,0,1), pi/4)
+
+    p = lines!(ax, [Point2f(cos(x), sin(x)) for x in range(0, 2pi, length=101)])
+    translate!(p, 1e9, 1e8, 0)
+    rotate!(p, Vec3f(0,0,1), pi/4)
+    p = linesegments!(ax, [0.9 * Point2f(cos(x), sin(x)) for x in range(0, 2pi, length=101)])
+    translate!(p, 1e9, 1e8, 0)
+    rotate!(p, Vec3f(0,0,1), pi/4)
+    p = lines!(ax, [0.8 * Point2f(cos(x), sin(x)) for x in range(0, 2pi, length=101)], linestyle = :dash)
+    translate!(p, 1e9, 1e8, 0)
+    rotate!(p, Vec3f(0,0,1), pi/4)
+
+    fig
+end
diff --git a/ReferenceTests/src/tests/primitives.jl b/ReferenceTests/src/tests/primitives.jl
@@ -693,8 +693,8 @@ end
 end
 
 @reference_test "Plot transform overwrite" begin
-    # Tests that (primitive) plots can have different transform function to their
-    # parent scene (identity in this case)
+    # Tests that (primitive) plots can have different transform function 
+    # (identity) from their parent scene (log10, log10)
     fig = Figure()
 
     ax = Axis(fig[1, 1], xscale = log10, yscale = log10, backgroundcolor = :transparent)

diff --git a/WGLMakie/src/imagelike.jl b/WGLMakie/src/imagelike.jl
@@ -8,15 +8,12 @@ nothing_or_color(c::Nothing) = RGBAf(0, 0, 0, 1)
 
 function create_shader(mscene::Scene, plot::Surface)
     # TODO OWN OPTIMIZED SHADER ... Or at least optimize this a bit more ...
-    px, py, pz = plot[1], plot[2], plot[3]
-    function grid(x, y, z, f32c, trans, space)
-        Makie.matrix_grid(p -> f32_convert(f32c, apply_transform(trans, p, space), space), x, y, z)
-    end
     # TODO: Use Makie.surface2mesh
-    ps = lift(
-            plot, px, py, pz, f32_conversion_obs(mscene), transform_func_obs(plot), get(plot, :space, :data)
-        ) do x, y, z, f32c, tf, space
-        return grid(x, y, z, f32c, tf, space)
+    px, py, pz = plot[1], plot[2], plot[3]
+    f32c, model = Makie.patch_model(plot)
+    ps = let
+        grid_ps = lift(Makie.matrix_grid, plot, px, py, pz)
+        apply_transform_and_f32_conversion(plot, f32c, grid_ps)
     end
     positions = Buffer(ps)
     rect = lift(z -> Tesselation(Rect2(0f0, 0f0, 1f0, 1f0), size(z)), plot, pz)
@@ -37,7 +34,7 @@ function create_shader(mscene::Scene, plot::Surface)
     end)
 
     per_vertex = Dict(:positions => positions, :faces => faces, :uv => uv, :normals => normals)
-    uniforms = Dict(:uniform_color => color, :color => false)
+    uniforms = Dict(:uniform_color => color, :color => false, :model => model)
 
     # TODO: allow passing Mat{2, 3, Float32} (and nothing)
     uniforms[:uv_transform] = map(plot, plot[:uv_transform]) do x
@@ -54,14 +51,16 @@ function create_shader(mscene::Scene, plot::Surface)
 end
 
 function create_shader(mscene::Scene, plot::Union{Heatmap, Image})
-    mesh = limits_to_uvmesh(plot)
+    f32c, model = Makie.patch_model(plot)
+    mesh = limits_to_uvmesh(plot, f32c)
     uniforms = Dict(
         :normals => Vec3f(0),
         :shading => false,
         :diffuse => Vec3f(0),
         :specular => Vec3f(0),
         :shininess => 0.0f0,
         :backlight => 0.0f0,
+        :model => model,
     )
 
     # TODO: allow passing Mat{2, 3, Float32} (and nothing)
@@ -158,7 +157,7 @@ function fast_uv(nvertices)
     return [Vec2f(x, y) for y in yrange for x in xrange]
 end
 
-function limits_to_uvmesh(plot)
+function limits_to_uvmesh(plot, f32c)
     px, py, pz = plot[1], plot[2], plot[3]
     px = map((x, z) -> xy_convert(x, size(z, 1)), px, pz; ignore_equal_values=true)
     py = map((y, z) -> xy_convert(y, size(z, 2)), py, pz; ignore_equal_values=true)
@@ -168,30 +167,23 @@ function limits_to_uvmesh(plot)
 
     # TODO, this branch is only hit by Image, but not for Heatmap with stepranges
     # because convert_arguments converts x/y to Vector{Float32}
-    if px[] isa StepRangeLen && py[] isa StepRangeLen && Makie.is_identity_transform(t) &&
-            isnothing(f32_conversion(plot))
+    if px[] isa StepRangeLen && py[] isa StepRangeLen && Makie.is_identity_transform(t)
         rect = lift(plot, px, py) do x, y
             xmin, xmax = extrema(x)
             ymin, ymax = extrema(y)
             return Rect2f(xmin, ymin, xmax - xmin, ymax - ymin)
         end
-        positions = Buffer(lift(rect -> decompose(Point2f, rect), plot, rect))
+        ps = lift(rect -> decompose(Point2f, rect), plot, rect)
+        positions = Buffer(apply_transform_and_f32_conversion(plot, f32c, ps))
         faces = Buffer(lift(rect -> decompose(GLTriangleFace, rect), plot, rect))
         uv = Buffer(lift(decompose_uv, plot, rect))
     else
         # TODO: Use Makie.surface2mesh
-        function grid(x, y, f32c, trans, space)
-            return Makie.matrix_grid(
-                p -> f32_convert(f32c, apply_transform(trans, p, space), space),
-                x, y, zeros(length(x), length(y))
-            )
+        positions = let
+            grid_ps = lift((x, y) -> Makie.matrix_grid(x, y, zeros(length(x), length(y))), plot, px, py)
+            Buffer(apply_transform_and_f32_conversion(plot, f32c, grid_ps))
         end
         resolution = lift((x, y) -> (length(x), length(y)), plot, px, py; ignore_equal_values=true)
-        positions = Buffer(lift(
-                plot, px, py, f32_conversion_obs(plot), t, get(plot, :space, :data)
-            ) do x, y, f32c, tf, space
-            return grid(x, y, f32c, tf, space)
-        end)
         faces = Buffer(lift(fast_faces, plot, resolution))
         uv = Buffer(lift(fast_uv, plot, resolution))
     end

diff --git a/WGLMakie/src/lines.jl b/WGLMakie/src/lines.jl
@@ -1,8 +1,9 @@
 function serialize_three(scene::Scene, plot::Union{Lines, LineSegments})
     Makie.@converted_attribute plot (linewidth, linestyle, linecap, joinstyle)
 
+    f32c, model = Makie.patch_model(plot)
     uniforms = Dict(
-        :model => map(Makie.patch_model, f32_conversion_obs(plot), plot.model),
+        :model => model,
         :depth_shift => plot.depth_shift,
         :picking => false,
         :linecap => linecap,
@@ -44,10 +45,10 @@ function serialize_three(scene::Scene, plot::Union{Lines, LineSegments})
     # make the p1 -- p2 segment draw, which is what indices does.
     indices = Observable(Int[])
     points_transformed = lift(
-            plot, f32_conversion_obs(scene), transform_func_obs(plot), plot[1], plot.space
-        ) do f32c, tf, ps, space
+            plot, f32c, transform_func_obs(plot), plot.model, plot[1], plot.space
+        ) do f32c, tf, model, ps, space
 
-        transformed_points = apply_transform_and_f32_conversion(f32c, tf, ps, space)
+        transformed_points = apply_transform_and_f32_conversion(f32c, tf, model, ps, space)
         # TODO: Do this in javascript?
         empty!(indices[])
         if isempty(transformed_points)