MakieOrg / Makie.jl

Interactive data visualizations and plotting in Julia
https://docs.makie.org/stable
MIT License
2.42k stars 313 forks source link

GLMakie throws if @recipe attribute is a Symbol and forwarded to other plot commands #1941

Open fatteneder opened 2 years ago

fatteneder commented 2 years ago

MWE

using GLMakie
GLMakie.activate!()

@recipe(MyText, text) do scene
    Attributes(;
        default_theme(scene)...,
        maxwidth = :a_symbol
    )
end

function Makie.plot!(plot::MyText{<:Tuple{<:AbstractString}})
    display(plot.maxwidth)
    text!(plot, plot.text; plot.attributes...)
    plot
end

f = Figure()
mytext(f[1,1], "Hello, world!")

f

The above throws

julia> include("mwe_attributes.jl")
Observable{Any} with 0 listeners. Value:
:a_symbol
Observable{Any} with 0 listeners. Value:
:a_symbol
Error showing value of type Figure:
ERROR: MethodError: no method matching gl_convert(::Symbol)
Closest candidates are:
  gl_convert(::T) where T<:Number at ~/wd/Makie.jl/GLMakie/src/GLAbstraction/GLUniforms.jl:191
  gl_convert(::GLMakie.GLAbstraction.AbstractLazyShader, ::Any) at ~/wd/Makie.jl/GLMakie/src/GLAbstraction/GLShader.jl:202
  gl_convert(::StaticArrays.SMatrix{N, M, T}) where {N, M, T} at ~/wd/Makie.jl/GLMakie/src/GLAbstraction/GLUniforms.jl:228
  ...
Stacktrace:

The problem is that maxwidth is of type Symbol and it complains about gl_convert(::Symbol) not being defined, although methods(GLMakie.GLAbstraction.gl_convert) shows an overload of type

[12] gl_convert(s::Observable{T}) where T in GLMakie.GLAbstraction at ...

Why is this not enough?

No error is thrown if

Temporary workaround

Don't forward attriubtes of type Symbol that are not consumed by other plotting commands, e.g.

function Makie.plot!(plot::MyText{<:Tuple{<:AbstractString}})
    text_attrs = copy(plot.attributes)
    delete!(text_attrs, :maxwidth)
    text!(plot, plot.text; text_attrs...)
    plot
end

Version

julia> versioninfo()
Julia Version 1.7.2
Commit bf53498635 (2022-02-06 15:21 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.1 (ORCJIT, tigerlake)
(Makie) pkg> st
     Project Makie v0.17.1
      Status `~/wd/Makie.jl/Project.toml`
  [27a7e980] Animations v0.4.1
  [13f3f980] CairoMakie v0.8.1 `CairoMakie`
  [a2cac450] ColorBrewer v0.4.0
  [35d6a980] ColorSchemes v3.18.0
  [3da002f7] ColorTypes v0.11.1
  [5ae59095] Colors v0.12.8
  [d38c429a] Contour v0.5.7
  [31c24e10] Distributions v0.25.58
  [ffbed154] DocStringExtensions v0.8.6
  [c87230d0] FFMPEG v0.4.1
  [5789e2e9] FileIO v1.14.0
  [53c48c17] FixedPointNumbers v0.8.4
  [59287772] Formatting v0.4.2
  [b38be410] FreeType v4.0.0
  [663a7486] FreeTypeAbstraction v0.9.9
  [e9467ef8] GLMakie v0.6.1 `GLMakie`
  [5c1252a2] GeometryBasics v0.4.2
  [3955a311] GridLayoutBase v0.7.5
  [82e4d734] ImageIO v0.6.2
  [8197267c] IntervalSets v0.6.2
  [f1662d9f] Isoband v0.1.1
  [5ab0869b] KernelDensity v0.6.3
  [b964fa9f] LaTeXStrings v1.3.0
  [20f20a25] MakieCore v0.3.1
  [7eb4fadd] Match v1.2.0
  [0a4f8689] MathTeXEngine v0.2.1
  [510215fc] Observables v0.5.1
  [6fe1bfb0] OffsetArrays v1.11.0
  [19eb6ba3] Packing v0.4.2
  [995b91a9] PlotUtils v1.2.0
  [647866c9] PolygonOps v0.1.2
  [05181044] RelocatableFolders v0.1.3
  [992d4aef] Showoff v1.0.3
  [73760f76] SignedDistanceFields v0.4.0
  [2913bbd2] StatsBase v0.33.16
  [4c63d2b9] StatsFuns v1.0.1
  [09ab397b] StructArrays v0.6.7
  [1cfade01] UnicodeFun v0.4.1
  [2a0f44e3] Base64
  [37e2e46d] LinearAlgebra
  [d6f4376e] Markdown
  [de0858da] Printf
  [9a3f8284] Random
  [9e88b42a] Serialization
  [2f01184e] SparseArrays
  [10745b16] Statistics
asinghvi17 commented 2 years ago

This has been the case for a while, I think, since GLMakie wants to convert any existing attribute to an OpenGL-compatible form. The function you're referencing essentially calls lift(gl_convert, s), and AFAIK there is no method to directly convert Symbols.

I ran into this issue back when I wrote MakieRecipes, and there should be some filtering code in there. I think the best way forward might just be to filter out all unused attributes at the GLMakie level.

SimonDanisch commented 2 years ago

I mean, you should really not add attributes to a plot it doesn't support ;)

But, there are still two problems here: 1) Plots should always error nicely for unsupported attributes 2) There should be an easy way to forward attributes in recipes, e.g. something like text!(plot, text_attrs, plot.text) that then filters out applicable attributes.

GLMakie shouldn't filter out any attributes and it should also not throw weird internal errors. Ideally, it will only get plot objects which are already fully checked to only contain the correct attributes.

fatteneder commented 2 years ago

I mean, you should really not add attributes to a plot it doesn't support ;)

Agreed.

asinghvi17 commented 2 years ago

I guess we just need left/right joins on Attributes :D

ffreyer commented 2 months ago

This now errors due to attribute validation. Maybe this could be a useful example for what recipe/attribute tooling should handle for you