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

Types not detected correctly from emtpy typed vectors of polygons #4267

Open
3 tasks done
rafaqz opened this issue Aug 27, 2024 · 12 comments · May be fixed by #4615
Open
3 tasks done

Types not detected correctly from emtpy typed vectors of polygons #4267

rafaqz opened this issue Aug 27, 2024 · 12 comments · May be fixed by #4615
Labels
bug conversions Mainly `convert_arguments` Makie Backend independent issues (Makie core) mesh (and poly)

Comments

@rafaqz
Copy link
Contributor

rafaqz commented Aug 27, 2024

  • what version of Makie are you running? (]st -m Makie) v0.21.7
  • can you reproduce the bug with a fresh environment ? (]activate --temp; add Makie)
  • What platform + GPU are you on? linux/nvidia

This errors because the mesh is using Float32:

polygons = Observable(Polygon{2,Float64}[])
poly(polygons)
push!(polygons[], Polygon([Point(1.0, 2.0), Point(2.0, 3.0), Point(3.0, 2.0)]))
notify(polygons)

Seems there is no way to set the type without passing in objects. So building up the geometries manually is not possible except in Float32.

julia> notify(polygons)
ERROR: MethodError: Cannot `convert` an object of type 
  GeometryBasics.Mesh{2,Float64,GeometryBasics.Ngon{2,Float64,3,Point{2,Float64}},FaceView{GeometryBasics.Ngon{2,Float64,3,Point{2,Float64}},Point{2,Float64},NgonFace{3,OffsetInteger{-1,UInt32}},Array{Point{2
,Float64},1},Array{NgonFace{3,OffsetInteger{-1,UInt32}},1}}} to an object of type 
  GeometryBasics.Mesh{2,Float32,GeometryBasics.Ngon{2,Float32,3,Point{2,Float32}},FaceView{GeometryBasics.Ngon{2,Float32,3,Point{2,Float32}},Point{2,Float32},NgonFace{3,OffsetInteger{-1,UInt32}},Array{Point{2
,Float32},1},Array{NgonFace{3,OffsetInteger{-1,UInt32}},1}}}

Closest candidates are:
  convert(::Type{T}, ::T) where T
   @ Base Base.jl:84
  convert(::Type{T}, ::T) where T<:AbstractArray
   @ Base abstractarray.jl:16
  convert(::Type{T}, ::LinearAlgebra.Factorization) where T<:AbstractArray
   @ LinearAlgebra /opt/julia/share/julia/stdlib/v1.10/LinearAlgebra/src/factorization.jl:108
  ...

Stacktrace:
  [1] setindex!(A::Vector{GeometryBasics.Mesh{…}}, x::GeometryBasics.Mesh{2, Float64, GeometryBasics.Ngon{…}, SimpleFaceView{…}}, i1::Int64)
    @ Base ./array.jl:1021
  [2] _unsafe_copyto!(dest::Vector{GeometryBasics.Mesh{…}}, doffs::Int64, src::Vector{GeometryBasics.Mesh{…}}, soffs::Int64, n::Int64)
    @ Base ./array.jl:299
  [3] unsafe_copyto!
    @ ./array.jl:353 [inlined]
  [4] _copyto_impl!
    @ ./array.jl:376 [inlined]
  [5] copyto!
    @ ./array.jl:363 [inlined]
  [6] copyto!
    @ ./array.jl:385 [inlined]
  [7] copyto_axcheck!
    @ ./abstractarray.jl:1177 [inlined]
  [8] Vector{GeometryBasics.Mesh{2, Float32, GeometryBasics.Ngon{…}, SimpleFaceView{…}}}(x::Vector{GeometryBasics.Mesh{2, Float64, GeometryBasics.Ngon{…}, SimpleFaceView{…}}})
    @ Base ./array.jl:673
  [9] convert(::Type{Vector{GeometryBasics.Mesh{…}}}, a::Vector{GeometryBasics.Mesh{2, Float64, GeometryBasics.Ngon{…}, SimpleFaceView{…}}})
    @ Base ./array.jl:665
 [10] setproperty!(x::Observable{Vector{GeometryBasics.Mesh{…}}}, f::Symbol, v::Vector{GeometryBasics.Mesh{2, Float64, GeometryBasics.Ngon{…}, SimpleFaceView{…}}})
    @ Base ./Base.jl:40
 [11] setindex!(observable::Observable, val::Any)
    @ Observables ~/.julia/packages/Observables/YdEbO/src/Observables.jl:122
 [12] (::Observables.MapCallback)(value::Any)
    @ Observables ~/.julia/packages/Observables/YdEbO/src/Observables.jl:436
 [13] #invokelatest#2
    @ ./essentials.jl:892 [inlined]
 [14] invokelatest
    @ ./essentials.jl:889 [inlined]
 [15] notify
    @ ~/.julia/packages/Observables/YdEbO/src/Observables.jl:206 [inlined]
 [16] #62
    @ ./tuple.jl:627 [inlined]
 [17] BottomRF
    @ ./reduce.jl:86 [inlined]
 [18] afoldl
    @ ./operators.jl:544 [inlined]
 [19] _foldl_impl
    @ ./reduce.jl:68 [inlined]
 [20] foldl_impl
    @ ./reduce.jl:48 [inlined]
 [21] mapfoldl_impl
    @ ./reduce.jl:44 [inlined]
 [22] mapfoldl
    @ ./reduce.jl:175 [inlined]
 [23] foldl
    @ ./reduce.jl:198 [inlined]
 [24] foreach
    @ ./tuple.jl:627 [inlined]
 [25] (::Makie.var"#299#300"{UnionAll, Tuple{Observable{}}})(kw::@NamedTuple{}, args::Vector{Polygon{2, Float64, P, L, V} where {P<:AbstractPoint{…}, L<:(AbstractVector{…}), V<:AbstractVector{…}}})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/interfaces.jl:186
 [26] invokelatest(::Any, ::Any, ::Vararg{Any}; kwargs::@Kwargs{})
    @ Base ./essentials.jl:892
 [27] invokelatest(::Any, ::Any, ::Vararg{Any})
    @ Base ./essentials.jl:889
 [28] (::Observables.OnAny)(value::Any)
    @ Observables ~/.julia/packages/Observables/YdEbO/src/Observables.jl:420
 [29] #invokelatest#2
    @ ./essentials.jl:892 [inlined]
 [30] invokelatest
    @ ./essentials.jl:889 [inlined]
 [31] notify(observable::Observables.AbstractObservable)
    @ Observables ~/.julia/packages/Observables/YdEbO/src/Observables.jl:206
 [32] top-level scope
    @ REPL[86]:1
Some type information was truncated. Use `show(err)` to see complete types.

See: MakieOrg/MakieDraw.jl#14

@rafaqz rafaqz added the bug label Aug 27, 2024
@rafaqz
Copy link
Contributor Author

rafaqz commented Aug 27, 2024

This is breaking MakieDraw.jl pretty badly now I've tried to switch things to Float64.

@rafaqz rafaqz changed the title Types not detected correctly from emtpy typed vectors of geometries Types not detected correctly from emtpy typed vectors of polygons Aug 27, 2024
@asinghvi17
Copy link
Member

Looks like a bad type constraint somewhere in convert_arguments to me. Converting number type in a mesh should also not be a problem so I'll ping this to JuliaGeometry/GeometryBasics.jl#173 as a goal there (ideally)...

@rafaqz
Copy link
Contributor Author

rafaqz commented Aug 27, 2024

Seems to me there is a if length(polys) > 0 ... check somewhere that switches to Float32, when it should instead look at the type

@asinghvi17
Copy link
Member

haha...

# TODO is this a problem with Float64 meshes?
isempty(geometries) && return typeof(GeometryBasics.Mesh(Point2f[], GLTriangleFace[]))[]

@rafaqz
Copy link
Contributor Author

rafaqz commented Aug 28, 2024

Lol, yes it is a problem

@rafaqz
Copy link
Contributor Author

rafaqz commented Aug 28, 2024

So we just need a method to get N and T from the Polygon type, and use 2 and Float32 if there really is no information available.

Seems like 2 lines of code if I'm not missing something...

@kescobo
Copy link
Contributor

kescobo commented Aug 28, 2024

Can you just pull those from eltype or something?

@ffreyer ffreyer added Makie Backend independent issues (Makie core) conversions Mainly `convert_arguments` mesh (and poly) labels Aug 28, 2024
@rafaqz
Copy link
Contributor Author

rafaqz commented Aug 29, 2024

Yes, just pass eltype(geometries) to e.g. a _meshfromtype(::Type{<:Polygon{N,T}) method

@asinghvi17
Copy link
Member

I tried the basic fix but it looks like there are still issues elsewhere. Will be a bit longer, I think.

@asinghvi17
Copy link
Member

I'm trying this on Makie master, but run into this:

julia> polygons = Observable(Polygon{2,Float64}[])
Observable(Polygon{2, Float64, P, L, V} where {P<:AbstractPoint{2, Float64}, L<:(AbstractVector{<:GeometryBasics.Ngon{2, Float64, 2, P}}), V<:AbstractVector{L}}[])


julia> f, a, p = poly(polygons)

julia> p.args
1-element Vector{Any}:
 Observable(Polygon{2, Float64, P, L, V} where {P<:AbstractPoint{2, Float64}, L<:(AbstractVector{<:GeometryBasics.Ngon{2, Float64, 2, P}}), V<:AbstractVector{L}}[])

julia> p[1]
Observable(Polygon{2, Float64, P, L, V} where {P<:AbstractPoint{2, Float64}, L<:(AbstractVector{<:GeometryBasics.Ngon{2, Float64, 2, P}}), V<:AbstractVector{L}}[])
    0 => map(Makie.poly_convert)
    0 => map(to_lines(polygon) @ Makie ~/.julia/dev/Makie/src/basic_recipes/poly.jl:123)


julia> p.converted
1-element Vector{Observable}:
 Observable(Polygon{2, Float64, P, L, V} where {P<:AbstractPoint{2, Float64}, L<:(AbstractVector{<:GeometryBasics.Ngon{2, Float64, 2, P}}), V<:AbstractVector{L}}[])

julia> p.plots[1]
MakieCore.Mesh{Tuple{Vector{GeometryBasics.Mesh{2, Float64, GeometryBasics.Ngon{2, Float64, 3, Point{2, Float64}}, SimpleFaceView{2, Float64, 3, OffsetInteger{-1, UInt32}, Point{2, Float64}, NgonFace{3, OffsetInteger{-1, UInt32}}}}}}}

julia> p.plots[1].plots
1-element Vector{Plot}:
 MakieCore.Mesh{Tuple{GeometryBasics.Mesh{2, Float32, GeometryBasics.Ngon{2, Float32, 3, Point{2, Float32}}, SimpleFaceView{2, Float32, 3, OffsetInteger{-1, UInt32}, Point{2, Float32}, NgonFace{3, OffsetInteger{-1, UInt32}}}}}}

note that the last plot has float32 meshes as input. am tracking this down now - suspect it is in the convert_arguments that this happens.

@asinghvi17
Copy link
Member

g*ddamnit

bigmesh = lift(plot, meshes, transform_func) do meshes, tf
        if isempty(meshes)
            # TODO: Float64
            return GeometryBasics.Mesh(Point2f[], GLTriangleFace[])
        else
            triangle_meshes = map(mesh -> poly_convert(mesh, tf), meshes)
            return merge(triangle_meshes)
        end
    end

@asinghvi17 asinghvi17 linked a pull request Nov 21, 2024 that will close this issue
5 tasks
@rafaqz
Copy link
Contributor Author

rafaqz commented Nov 21, 2024

Ah yeah I've seen that sorry should have linked

It's just hard coded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug conversions Mainly `convert_arguments` Makie Backend independent issues (Makie core) mesh (and poly)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants