Resolve :data space when deciding whether to connect transformations (#…

…4723)

* always resolve space compatibility of :data based on parent scene/camera

* add show for transformations

* add test

* update changelog

* treat EmptyCamera as :clip

* extend tests

* fix test

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## [Unreleased]
 
+- Fixed an issue with transformations not propagating to child plots when their spaces only match indirectly. [#4723](https://github.com/MakieOrg/Makie.jl/pull/4723)
 - Added a tutorial on creating an inset plot [#4697](https://github.com/MakieOrg/Makie.jl/pull/4697)
 - Enhanced Pattern support: Added general CairoMakie implementation, improved quality, added anchoring, added support in band, density, added tests & fixed various bugs and inconsistencies. [#4715](https://github.com/MakieOrg/Makie.jl/pull/4715)
 - Fixed issue with `voronoiplot` for Voronoi tessellations with empty polygons [#4740](https://github.com/MakieOrg/Makie.jl/pull/4740)

src/camera/camera2d.jl

@@ -327,7 +327,6 @@ struct UpdatePixelCam
     near::Float64
     far::Float64
 end
-get_space(::UpdatePixelCam) = :pixel
 
 function (cam::UpdatePixelCam)(window_size)
     w, h = Float64.(widths(window_size))

src/camera/projection_math.jl

@@ -451,8 +451,14 @@ function get_space(scene::Scene)
     space === :data ? (:data,) : (:data, space)
 end
 get_space(::AbstractCamera) = :data
-# TODO: Should this be less specialized? ScenePlot? AbstractPlot?
-get_space(plot::Plot) = to_value(get(plot, :space, :data))::Symbol
+function get_space(plot::Plot)
+    space = to_value(get(plot, :space, :data))::Symbol
+    # :data should resolve based on the parent scene/camera
+    if (space == :data) && (parent_scene(plot) !== nothing)
+        return get_space(parent_scene(plot))
+    end
+    return space
+end
 
 is_space_compatible(a, b) = is_space_compatible(get_space(a), get_space(b))
 is_space_compatible(a::Symbol, b::Symbol) = a === b

src/interfaces.jl

@@ -156,8 +156,8 @@ function apply_expand_dimensions(trait, args, args_obs, deregister)
             for (obs, arg) in zip(new_obs, expanded)
                 obs.val = arg
             end
-            # It should be enough to trigger the first observable since 
-            # this will go into `map(convert_arguments, new_obs...)` 
+            # It should be enough to trigger the first observable since
+            # this will go into `map(convert_arguments, new_obs...)`
             # Without this, we'll get 3 updates for `notify(data)` in `heatmap(data)`
             notify(new_obs[1])
             return

src/scenes.jl

@@ -646,6 +646,7 @@ end
 parent_scene(x) = parent_scene(get_scene(x))
 parent_scene(x::Plot) = parent_scene(parent(x))
 parent_scene(x::Scene) = x
+parent_scene(::Nothing) = nothing
 
 Base.isopen(x::SceneLike) = events(x).window_open[]
 

src/types.jl

@@ -4,6 +4,7 @@ abstract type AbstractAxis <: Block end
 
 # placeholder if no camera is present
 struct EmptyCamera <: AbstractCamera end
+get_space(::EmptyCamera) = :clip
 
 @enum RaymarchAlgorithm begin
     IsoValue # 0
@@ -369,6 +370,17 @@ function Transformation(parent::Transformable;
     return trans
 end
 
+function Base.show(io::IO, ::MIME"text/plain", t::Transformation)
+    println(io, "Transformation()")
+    println(io, "          parent = ", isassigned(t.parent) ? "Transformation(…)" : "#undef")
+    println(io, "     translation = ", t.translation[])
+    println(io, "           scale = ", t.scale[])
+    println(io, "        rotation = ", t.rotation[])
+    println(io, "          origin = ", t.origin[])
+    println(io, "           model = ", t.model[])
+    println(io, "  transform_func = ", t.transform_func[])
+end
+
 struct ScalarOrVector{T}
     sv::Union{T, Vector{T}}
 end

test/issues.jl

@@ -13,14 +13,14 @@
         f, a, p = scatter(ps)
         Makie.update_state_before_display!(f)
         # sanity check: old behavior should fail
-        M = Makie.Mat4f(Makie.clip_to_space(a.scene.camera, :data)) * 
+        M = Makie.Mat4f(Makie.clip_to_space(a.scene.camera, :data)) *
             Makie.Mat4f(Makie.space_to_clip(a.scene.camera, :data))
         @test !(M ≈ I)
         # this should not
         M = Makie.clip_to_space(a.scene.camera, :data) * Makie.space_to_clip(a.scene.camera, :data)
         @test M ≈ I atol = 1e-4
     end
-    
+
     @testset "#4416 Merging attributes" begin
         # See https://github.com/MakieOrg/Makie.jl/pull/4416
         theme1 = Theme(Axis = (; titlesize = 10))
@@ -30,4 +30,20 @@
         @test !haskey(theme1.Axis, :xgridvisible)
         @test !haskey(theme2.Axis, :titlesize)  # sanity check other argument
     end
+
+    @testset "#4722 Transformation passthrough" begin
+        scene = Scene(camera = campixel!)
+        p = text!(scene, L"\frac{1}{2}")
+        translate!(scene, 0, 0, 10)
+        @test isassigned(p.transformation.parent)
+        @test isassigned(p.plots[1].transformation.parent)
+        @test isassigned(p.plots[1].plots[1].transformation.parent)
+        @test isassigned(p.plots[1].plots[2].transformation.parent)
+
+        @test scene.transformation.model[][3, 4] == 10.0
+        @test p.transformation.model[][3, 4] == 10.0
+        @test p.plots[1].transformation.model[][3, 4] == 10.0
+        @test p.plots[1].plots[1].transformation.model[][3, 4] == 10.0
+        @test p.plots[1].plots[2].transformation.model[][3, 4] == 10.0
+    end
 end

test/transformations.jl

@@ -140,7 +140,7 @@ end
 
 @testset "Model Transforms" begin
     t1 = Transformation()
-    
+
     @testset "defaults" begin
         @test !isassigned(t1.parent)
         @test t1.translation[] == Vec3d(0)
@@ -293,3 +293,29 @@ end
         @test apply_transform(t3, p3, space) == desired_transform(p3, Point3(sqrt(2.0), log(5.0), log10(4.0)))
     end
 end
+
+@testset "space dependent inheritance" begin
+    camfuncs = [identity, campixel!, cam_relative!, cam2d!, cam3d!, old_cam3d!]
+    camtypes = [EmptyCamera, Makie.PixelCamera, Makie.RelativeCamera, Camera2D, Camera3D, Makie.OldCamera3D]
+    spaces = [:clip, :pixel, :relative, :data, :data, :data]
+
+    for (camfunc, camspace, CamType) in zip(camfuncs, spaces, camtypes)
+        scene = Scene()
+        camfunc(scene)
+        @test scene.camera_controls isa CamType
+        @test Makie.get_space(scene.camera_controls) == camspace
+        translate!(scene, 0,0,1)
+
+        # these should only inherit if the camera results in the same space
+        for space in [:clip, :pixel, :relative]
+            p = scatter!(scene, Point2f(0), space = space)
+            @test isassigned(p.transformation.parent) == (space === camspace)
+            @test p.transformation.model[][3, 4] == ifelse(space === camspace, 1.0, 0.0)
+        end
+
+        # data is camera space so transformations should always inherit
+        p = scatter!(scene, Point2f(0), space = :data)
+        @test isassigned(p.transformation.parent)
+        @test p.transformation.model[][3, 4] == 1.0
+    end
+end