Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

correctly construct Attributes in get_axis #3283

Closed
wants to merge 1 commit into from

Conversation

hexaeder
Copy link
Contributor

Description

Fixes MakieOrg/GraphMakie.jl#142

During get_axis, the Attributes for the plot recipe are constructed in a "wrong" way, in which nested attributes do not work as expected. I.e. named tuples in the Attributes become Observable{Any} instead of nested Attributes objects.

Was introduced in Makie@0.19.11 as part of #3275

MWE:

using Makie
@recipe(MyPlot, p) do scene
   Attributes(
       nt = (;)
   )
end
function Makie.plot!(mp::MyPlot)
    p = mp[:p]
    @show mp[:not] # see how it looks like in the recipe
    scatter!(mp, p)
end
julia> fig, a, p = myplot(rand(10); nt=(;named=:tuple))
mp[:nt] = Observable{Any}((named = :tuple,)) # "warmup" run
mp[:nt] = Attributes with 1 entry:           # actual run
  named => tuple

Not sure if the splat of the dict is the best way to solve this, suggestions welcome. On a different note: it might be nice to pass another attribute, such as _warmup=true to the recipe. GraphMakie could skip the (possibly expensive) layout algorithm in the warmup run...

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@SimonDanisch
Copy link
Member

Oh, that's an annoying corner case...
Also, those constructors should be equivalent, but guess they aren't -.-

On a different note: it might be nice to pass another attribute, such as _warmup=true to the recipe

We'll remove the whole proxy_scene business in #3113, so no need for a hacky workaround ;)

@hexaeder
Copy link
Contributor Author

hexaeder commented Oct 10, 2023

Also, those constructors should be equivalent, but guess they aren't -.-

Well alternatively, one could define an outer constructor

Attributes(d::Dict) = Attributes(Dict{Symbol, Observable}(node_pairs(d)))

to MakieCore/src/attributes.jl, such that the conversion happens explicitly pairwise instead of implicitly in the default inner constructor of Attributes.

Edit: just added the alternative PR.

@SimonDanisch
Copy link
Member

Fixed in #3284

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node_attr broken using Makie v0.19.11
2 participants