Do less copies of Observables in Attributes + plot pipeline

MakieCore/src/attributes.jl

@@ -39,13 +39,8 @@ function Base.iterate(x::Attributes, state...)
     return (s[1] => x[s[1]], s[2])
 end
 
-function Base.copy(attributes::Attributes)
-    result = Attributes()
-    for (k, v) in attributes
-        # We need to create a new Signal to have a real copy
-        result[k] = copy(v)
-    end
-    return result
+function Base.copy(attr::Attributes)
+    return Attributes(copy(attributes(attr)))
 end
 
 function Base.deepcopy(obs::Observable)
@@ -251,7 +246,7 @@ end
 function merge_attributes!(input::Attributes, theme::Attributes)
     for (key, value) in theme
         if !haskey(input, key)
-            input[key] = copy(value)
+            input[key] = value
         else
             current_value = input[key]
             if value isa Attributes && current_value isa Attributes

src/interfaces.jl

@@ -208,6 +208,7 @@ function (PlotType::Type{<: AbstractPlot{Typ}})(scene::SceneLike, attributes::At
     ArgTyp = typeof(to_value(args))
     # construct the fully qualified plot type, from the possible incomplete (abstract)
     # PlotType
+
     FinalType = Combined{Typ, ArgTyp}
     plot_attributes = merged_get!(
         ()-> default_theme(scene, FinalType),
@@ -411,7 +412,7 @@ function plot!(scene::SceneLike, P::PlotFunc, attributes::Attributes, input::NTu
     if haskey(attributes, :textsize)
         throw(ArgumentError("The attribute `textsize` has been renamed to `fontsize` in Makie v0.19. Please change all occurrences of `textsize` to `fontsize` or revert back to an earlier version."))
     end
-    plot_object = P(scene, copy(attributes), input, args)
+    plot_object = P(scene, attributes, input, args)
     # transfer the merged attributes from theme and user defined to the scene
     for (k, v) in scene_attributes
         error("setting $k for scene via plot attribute not supported anymore")

src/theming.jl

@@ -195,7 +195,14 @@ function with_theme(f, theme = Theme(); kwargs...)
     end
 end
 
-theme(::Nothing, key::Symbol) = deepcopy(current_default_theme()[key])
+function theme(::Nothing, key::Symbol)
+    val = to_value(CURRENT_DEFAULT_THEME[key])
+    if val isa Attributes
+        return val
+    else
+        Observable{Any}(val)
+    end
+end
 
 """
     update_theme!(with_theme::Theme; kwargs...)

test/pipeline.jl

@@ -0,0 +1,36 @@
+# extracted from interfaces.jl
+function test_copy(; kw...)
+    scene = Scene()
+    return Makie.merged_get!(
+        ()-> Makie.default_theme(scene, Lines),
+        :lines, scene, Attributes(kw)
+    )
+end
+
+function test_copy2(attr; kw...)
+    return merge!(Attributes(kw), attr)
+end
+
+@testset "don't copy in theme merge" begin
+    x = Observable{Any}(1)
+    res=test_copy(linewidth=x)
+    res.linewidth === x
+end
+
+@testset "don't copy observables in when calling merge!" begin
+    x = Observable{Any}(1)
+    res=test_copy2(Attributes(linewidth=x))
+    res.linewidth === x
+end
+
+@testset "don't copy attributes in recipe" begin
+    fig = Figure()
+    ax = Axis(fig[1, 1])
+    list = Observable{Any}([1, 2, 3, 4])
+    xmax = Observable{Any}([0.25, 0.5, 0.75, 1])
+
+    p = hlines!(ax, list, xmax = xmax, color = :blue)
+    @test getfield(p, :input_args)[1] === list
+    @test p.xmax === xmax
+    fig
+end

test/runtests.jl

@@ -18,6 +18,7 @@ using Makie: volume
         @test all(hi .>= (8,8,10))
     end
 
+    include("pipeline.jl")
     include("record.jl")
     include("scenes.jl")
     include("conversions.jl")