Cleanup nested attributes (docs, validation, shared_attributes, updating, infrastructure)

CHANGELOG.md

@@ -25,8 +25,12 @@
     - `update_indicator_plot!()` for drawing indicator plots
     - removed `inspector_clear` and `inspector_hover` attributes which are now handled by `update_indicator_plot!()`
   - Added functionality for persistent tooltips
-- Added a system to simulate nesting in compute graphs to allow for nested attributes.
-  - **minor breaking** `nested_attributes = Attributes(...)` in `@recipe` are now mapped to nested nodes in a compute graph. As a result `plot.nested_attributes[]` is of type `::ComputeGraphView` instead of `::Attributes`. The contents can still be handled like before, i.e. `map/on/lift(..., plot.nested_attributes[].attribute)`.
+- Added support for nested attributes [#5482](https://github.com/MakieOrg/Makie.jl/pull/5482), [#5620](https://github.com/MakieOrg/Makie.jl/pull/5620)
+  - ComputePipeline now has a system for simulating nested nodes, e.g. `add_input!(graph, :outer, :inner, 1); graph.outer.inner` [#5482](https://github.com/MakieOrg/Makie.jl/pull/5482)
+  - `@recipe ... begin ... end`, `@Block` and `@DocumentedAttributes` now support nesting via `key = @attributes begin ... end` blocks. These can also be documented and allow `mixin()...` expressions [#5620](https://github.com/MakieOrg/Makie.jl/pull/5620)
+  - `@recipe ... do scene ... end` supports nested attributes via `key = Attributes(...)` [#5620](https://github.com/MakieOrg/Makie.jl/pull/5620)
+  - `@recipe ... do scene ... end` is now marked as deprecated [#5620](https://github.com/MakieOrg/Makie.jl/pull/5620)
+  - **mildly breaking** Internal attribute processing for plots and blocks has been reworked and merged, which includes the removal of various unexported functions [#5620](https://github.com/MakieOrg/Makie.jl/pull/5620)
 - Fixed the precedence of keys in `Base.merge!` and `Base.merge` for `Attributes` arguments [#5332](https://github.com/MakieOrg/Makie.jl/pull/5332)
 - Reworked `Block/@Block` infrastructure to support complex/block recipes. The infrastructure mostly mirrors the `@recipe` infrastructure from plots: [#5465](https://github.com/MakieOrg/Makie.jl/pull/5465)
   - The names (and types) of converted arguments can be defined in `@Block MyBlock (arg1::Vector, arg2)`.

ComputePipeline/src/ComputePipeline.jl

@@ -124,11 +124,9 @@ mutable struct Computed
     parent_idx::Int # index of parent.outputs this value refers to
     Computed(name) = new(name, false)
     function Computed(name, value::RefValue)
-        validate_node_value(value)
         return new(name, false, value)
     end
     function Computed(name, value::RefValue, parent::AbstractEdge, idx::Integer)
-        validate_node_value(value)
         return new(name, false, value, parent, idx)
     end
     function Computed(name, edge::AbstractEdge, idx::Integer)
@@ -275,8 +273,7 @@ end
 Base.setproperty!(::Input, ::Symbol, ::Observable) = error("Setting the value of an ::Input to an Observable is not allowed")
 Base.setproperty!(::Input, ::Symbol, ::Computed) = error("Setting the value of an ::Input to a Computed is not allowed")
 
-function Input(graph, name, value, f, output, force_update = false)
-    validate_node_value(value)
+function Input(graph, name, @nospecialize(value), f, output, force_update = false)
     return Input{ComputeGraph}(
         graph, name, value, f, output, true, ComputeEdge[], force_update
     )
@@ -321,13 +318,6 @@ struct ComputeGraph <: AbstractComputeGraph
     obs_to_update::Vector{Observable}
 end
 
-validate_node_value(x) = nothing
-validate_node_value(x::RefValue) = isassigned(x) ? validate_node_value(x[]) : nothing
-# shouldn't have those in input.value or computed.value[]
-function validate_node_value(::Union{T, RefValue{T}}) where {T <: Union{Computed, Input, ComputeGraph, ComputeEdge}}
-    error("The value of a compute node is not allowed to be of type ::$T.")
-end
-
 is_node_value_valid(x) = true
 is_node_value_valid(x::RefValue) = isassigned(x) ? is_node_value_valid(x[]) : true
 # shouldn't have those in input.value or computed.value[]
@@ -651,15 +641,12 @@ function _update!(attr::ComputeGraph, values)
     return attr
 end
 
-function Base.haskey(attr::ComputeGraph, key::Symbol)
-    return haskey(attr.outputs, key) || haskey(attr.nesting.keytables[1], key)
-end
-function Base.haskey(graph::ComputeGraph, key::Symbol, keys::Symbol...)
-    return haskey(graph.nesting, key, keys...)
-end
-function Base.haskey(graph::ComputeGraph, keys::Tuple{Vararg{Symbol}})
-    return haskey(graph, keys...)
-end
+Base.haskey(attr::ComputeGraph, key::Symbol) = haskey(attr.outputs, key) || haskey(attr.nesting, key)
+Base.haskey(graph::ComputeGraph, key::Symbol, keys::Symbol...) = haskey(graph.nesting, key, keys...)
+Base.haskey(graph::ComputeGraph, keys::Tuple{Vararg{Symbol}}) = haskey(graph, keys...)
+
+has_nested_key(graph::ComputeGraph, key::Symbol) = haskey(graph.nesting, key)
+has_leaf_key(graph::ComputeGraph, key::Symbol) = haskey(graph.outputs, key)
 
 Base.get(attr::ComputeGraph, key::Symbol, default) = get(attr.outputs, key, default)
 Base.keys(graph::ComputeGraph) = keys(graph.outputs)
@@ -745,7 +732,7 @@ function Base.show(io::IO, ::MIME"text/plain", view::ComputeGraphView)
     print(io, "Nested view of ComputeGraph at graph.$base_key containing:")
     for (key, val) in level_dict
         full_key = Symbol(base_key, :(.), key)
-        if val == -1
+        if is_final_level(val)
             node = get(attr.inputs, full_key, attr.outputs[full_key])
             print(io, "\n  ", key, " => ", node)
         else
@@ -760,10 +747,13 @@ end
 
 Base.keys(view::ComputeGraphView) = keys(view.nested_trace)
 recursive_keys(view::ComputeGraphView) = recursive_keys(view.nested_trace)
+merged_key(view::ComputeGraphView) = merged_key(view.nested_trace)
 
 Base.haskey(view::ComputeGraphView, keys::Symbol...) = haskey(view.nested_trace, keys...)
 Base.haskey(view::ComputeGraphView, keys::Tuple{Vararg{Symbol}}) = haskey(view.nested_trace, keys...)
 
+has_leaf_key(view::ComputeGraphView, key::Symbol) = isfinal(view.nested_trace[key])
+
 # Generates pairs for `foo(; kwargs...)`
 function Base.iterate(view::ComputeGraphView)
     ks = keys(view)
@@ -810,6 +800,10 @@ function Base.getproperty(attr::ComputeGraphView, key::Symbol)
     return getindex(attr, key)
 end
 
+function Base.getindex(attr::AbstractComputeGraph, keys::Tuple{Vararg{Symbol}})
+    return getindex(attr, keys...)
+end
+
 function Base.getindex(attr::AbstractComputeGraph, key1::Symbol, key2::Symbol, keys::Symbol...)
     return getindex(getindex(attr, key1), key2, keys...)
 end

ComputePipeline/src/nesting.jl

@@ -1,3 +1,18 @@
+#=
+This struct tracks nested keys. Each element in `keytables` represents a local
+set of keys which may point to integer. If that integer < 0 the key is considered
+an end point, i.e. there is no more nesting. If it is > 0 it points to another
+element of keytables that acts as the next level of nesting.
+`keytables[1]` is always the root level of nesting.
+
+With this, `graph.a.b.c` is represented as:
+    index1 = keytables[1][:a]
+    index2 = keytables[index1][:b]
+    ketables[index2][:c] < 0
+
+Endpoint indices can be set to any value < 0 so they can be used to index into
+other collections.
+=#
 struct NestedSearchTree
     keytables::Vector{Dict{Symbol, Int}}
 end
@@ -6,33 +21,31 @@ NestedSearchTree() = NestedSearchTree([Dict{Symbol, Int}()])
 
 has_root_key(tree::NestedSearchTree, key::Symbol) = has_key_in_level(tree, 1, key)
 function has_key_in_level(tree::NestedSearchTree, level::Int, key::Symbol)
-    if length(tree.keytables) >= level
-        return haskey(tree.keytables[level], key)
-    else
-        return false
-    end
+    return (length(tree.keytables) >= level) && haskey(tree.keytables[level], key)
 end
 
 add_key!(tree::NestedSearchTree, args...) = add_key!(tree, args)
 add_key!(tree::NestedSearchTree, args::Tuple) = add_key!(tree, 1, args)
 add_path!(tree::NestedSearchTree, args...) = add_path!(tree, args)
 add_path!(tree::NestedSearchTree, args::Tuple) = add_key!(tree, 1, args, false)
-function add_key!(tree::NestedSearchTree, level, args::Tuple, points_to_value = true)
+
+is_final_level(x::Int) = x < 0
+
+function add_key!(tree::NestedSearchTree, level, args::Tuple, points_to = -1)
     key_to_insert = first(args)
     tail = Base.tail(args)
     if has_key_in_level(tree, level, key_to_insert)
 
         next_level = tree.keytables[level][key_to_insert]
-        if next_level == -1 && !isempty(tail)
+        if is_final_level(next_level) && !isempty(tail)
             error("Cannot insert (...).$key_to_insert.(...) - (...).$key_to_insert is already set to a value.")
-        elseif next_level != -1 && isempty(tail)
+        elseif !is_final_level(next_level) && isempty(tail)
             error("Cannot insert (...).$key_to_insert - (...).$key_to_insert is already set to a nested graph.")
         elseif isempty(tail)
             error("The given nested path (...).$key_to_insert already exists.")
         else
-            add_key!(tree, next_level, tail)
+            return add_key!(tree, next_level, tail)
         end
-        return
     else
 
         @assert length(tree.keytables) >= level - 1
@@ -41,22 +54,24 @@ function add_key!(tree::NestedSearchTree, level, args::Tuple, points_to_value =
         end
 
         if isempty(tail)
-            if points_to_value
-                tree.keytables[level][key_to_insert] = -1
+            if points_to < 0
+                tree.keytables[level][key_to_insert] = points_to
+                return level
             else
                 next_level = length(tree.keytables) + 1
                 @assert length(tree.keytables) == next_level - 1
                 tree.keytables[level][key_to_insert] = next_level
                 push!(tree.keytables, Dict{Symbol, Int}())
+                return length(tree.keytables)
             end
-            return
         else
             next_level = length(tree.keytables) + 1
             tree.keytables[level][key_to_insert] = next_level
-            add_key!(tree, next_level, tail)
-            return
+            return add_key!(tree, next_level, tail)
         end
     end
+    error("Unreachable")
+    return -1
 end
 
 delete_key!(tree::NestedSearchTree, args...) = delete_key!(tree, args)
@@ -67,7 +82,7 @@ function delete_key!(tree::NestedSearchTree, level, args::Tuple)
     if has_key_in_level(tree, level, current_key)
         next_level = pop!(tree.keytables[level], current_key)
 
-        if next_level != -1 # on path to leaf node
+        if !is_final_level(next_level) # on path to leaf node
             delete_key!(tree, next_level, tail)
         end
 
@@ -96,7 +111,7 @@ keys_in_level(tree::NestedSearchTree, level) = keys(tree.keytables[level])
 
 function recursive_keys(tree::NestedSearchTree, level, root = tuple(), allkeys = Tuple[])
     for (key, next_level) in tree.keytables[level]
-        if next_level == -1
+        if next_level < 0
             push!(allkeys, (root..., key))
         else
             path = (root..., key)
@@ -132,7 +147,7 @@ function Base.getindex(temp::TemporarySearchResult, key::Symbol)
     end
 end
 
-isfinal(temp::TemporarySearchResult) = temp.next_index == -1
+isfinal(temp::TemporarySearchResult) = is_final_level(temp.next_index)
 
 merged_key(temp::TemporarySearchResult) = merged_key(temp.keys)
 merged_key(keys::Symbol...) = merged_key(keys)

ComputePipeline/test/unit_tests.jl

@@ -798,27 +798,6 @@ end
     end
 end
 
-@testset "Validation" begin
-    graph = ComputeGraph()
-    add_input!(graph, :a, 1)
-
-    @test_throws ErrorException add_input!(graph, :b, Ref(graph.a))
-    @test_throws ErrorException add_input!(graph, :c, Ref(graph.inputs[:a]))
-    @test_throws ErrorException add_input!(graph, :d, graph.inputs[:a])
-
-    # add_input!() processes this, so we need to check more manually
-    graph.outputs[:dummy] = ComputePipeline.Computed(:dummy)
-    @test_throws ErrorException ComputePipeline.Input(graph, :e, graph.a, identity, graph.dummy)
-
-    @test_throws ErrorException ComputePipeline.Computed(:f, Ref(Ref(graph.a)))
-    @test_throws ErrorException ComputePipeline.Computed(:g, Ref(graph.a))
-    @test_throws ErrorException ComputePipeline.Computed(:h, Ref(Ref(graph.inputs[:a])))
-    @test_throws ErrorException ComputePipeline.Computed(:i, Ref(graph.inputs[:a]))
-
-    map!(x -> graph.a, graph, :a, :j)
-    @test_throws ResolveException{ErrorException} graph.j[]
-end
-
 @testset "mixed-map" begin
     graph1 = ComputeGraph()
     add_input!(graph1, :a1, 1)

GLMakie/src/GLAbstraction/GLRender.jl

@@ -15,53 +15,6 @@ function setup_clip_planes(N::Integer)
     return
 end
 
-# Note: context required in renderloop, not per renderobject here
-
-# """
-# When rendering a specialised list of Renderables, we can do some optimizations
-# """
-# function render(list::Vector{RenderObject{Pre}}) where {Pre}
-#     error("I'm not dead yet!")
-#     isempty(list) && return nothing
-#     first(list).prerender()
-#     vertexarray = first(list).vertexarray
-#     program = vertexarray.program
-#     glUseProgram(program.id)
-#     bind(vertexarray)
-#     for renderobject in list
-#         renderobject.visible || continue # skip invisible
-#         setup_clip_planes(to_value(get(renderobject.uniforms, :num_clip_planes, 0)))
-#         # make sure we only bind new programs and vertexarray when it is actually
-#         # different from the previous one
-#         if renderobject.vertexarray != vertexarray
-#             vertexarray = renderobject.vertexarray
-#             if vertexarray.program != program
-#                 program = renderobject.vertexarray.program
-#                 glUseProgram(program.id)
-#             end
-#             bind(vertexarray)
-#         end
-#         for (key, value) in program.uniformloc
-#             if haskey(renderobject.uniforms, key)
-#                 if length(value) == 1
-#                     gluniform(value[1], renderobject.uniforms[key])
-#                 elseif length(value) == 2
-#                     gluniform(value[1], value[2], renderobject.uniforms[key])
-#                 else
-#                     error("Uniform tuple too long: $(length(value))")
-#                 end
-#             end
-#         end
-#         renderobject.postrender(instructions.vertexarray)
-#     end
-#     # we need to assume, that we're done here, which is why
-#     # we need to bind VertexArray to 0.
-#     # Otherwise, every glBind(::GLBuffer) operation will be recorded into the state
-#     # of the currently bound vertexarray
-#     glBindVertexArray(0)
-#     return
-# end
-
 """
 Renders a RenderObject
 Note, that this function is not optimized at all!