MakieOrg / Makie.jl

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

Dimension conversions defaults for non dimensional data #4144

Open BambOoxX opened 2 months ago

BambOoxX commented 2 months ago

I have recipes built with @recipe that worked perfectly fine until the introduction of the dim conversion pipeline, and now Makie tries to convert my custom types with a method that does not exist. The axes I'm plotting into have no conversion set up whatsoever, so I thought everything would be dispatched to a NoConversion() by default, but it was not the case.

In my specific case, to make my recipes work, I had to add the following lines

Makie.should_dim_convert(::Type{<:Annotations}, ::Any) = false # (1) No default in Makie for this
Makie.should_dim_convert(::Type{<:Mesh}, ::Any) = false # (2) No default in Makie for this
Makie.MakieCore.should_dim_convert(::NoConversion, ::Any) = false # (3) Seems fairly natural
Makie.MakieCore.should_dim_convert(::<MyRecipe>, ::Any) = false # (4) For all my recipes (which do not rely on dim conversions)
Makie.should_dim_convert(::Type{<:Plot}, ::Any) = false # (5) Seems to be the most generic signature to get no conversion by default

I'm not entirely sure if this is a bug, or a lack of documentation about recipes with the recent changes.

Given https://github.com/MakieOrg/Makie.jl/blob/9e156624877e6d5e51648c47d735dac5fd540823/src/dim-converts/dim-converts.jl#L188

Shouldn't lines (1) - (3) , or even (5) of the above block be added to dim-convert.jl as defaults, or am I missing something ? I don't see how this would forbid people from adding further conversions, while not result in a breaking change for people relying on this.

SimonDanisch commented 2 months ago

Could be a bug. Do you have a minimal working example I can look at?

BambOoxX commented 2 months ago

@SimonDanisch I tried to reproduce this with a simple MWE, but it does not lead to any problems so far. I'll try to reproduce tomorrow with something more akin to my current code.

BambOoxX commented 1 month ago

Damn ! That took much longer than anticipated ! I just managed to make a MWE.

The problem seems specific to Axis3, LScene is okay it seems, and in combination with parametric types.

using CairoMakie
using CairoMakie.GeometryBasics

struct Element{N,T<:Integer}
    nodes::Vec{N,T}
end
Makie.@recipe(TestPlot, points, elements) do scene
    Attributes()
end

function Makie.plot!(plt::TestPlot{<:Tuple{AbstractVector{<:AbstractPoint},AbstractVector{<:Element}}})
    (; points, elements) = plt
    meshscatter!(plt, points[][first.(getproperty.(elements[],:nodes))])
    return plt
end

f = Figure();
ax = Axis3(f[1, 1]);
testplot!(rand(Point3f, 10), Element[Element(Vec{4,Int64}(rand(1:10, 4)...)) for i in 1:10])  # Fails
testplot!(rand(Point3f, 10), [Element(Vec{4,Int64}(rand(1:10, 4)...)) for i in 1:10])  # Works
f

f = Figure();
ax = LScene(f[1, 1]);
testplot!(rand(Point3f, 10), Element[Element(Vec{4,Int64}(rand(1:10, 4)...)) for i in 1:10]) # Works
testplot!(rand(Point3f, 10), [Element(Vec{4,Int64}(rand(1:10, 4)...)) for i in 1:10]) # Works
f
BambOoxX commented 1 month ago

@ffreyer did you have a chance to test the code I shared ?

ffreyer commented 4 weeks ago

The example works and fails like you noted for me