fix nan handling in WGLMakie (#4282)

* fix nan handling in WGLMakie * update CHANGELOG * fix attribute type encoding in Shader.jl * fix: add `convert_arguments` for `OffsetVector` * fix: typo in `buffers.jl` * add OffsetVector tests * fix offsetarray test * minimize diff * add `convert_arguments` for OffsetVector
MakieOrg · Sep 27, 2024 · b6658b9 · b6658b9
1 parent dc20012
commit b6658b9
Show file tree

Hide file tree

Showing 9 changed files with 53 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,7 @@
 # Changelog
 
 ## [Unreleased]
+- Fix NaN handling in WGLMakie [#4282](https://github.com/MakieOrg/Makie.jl/pull/4282).
 
 - Show DataInspector tooltip on NaN values if `nan_color` has been set to other than `:transparent` [#4310](https://github.com/MakieOrg/Makie.jl/pull/4310)
 - Fix `linestyle` not being used in `triplot` [#4332](https://github.com/MakieOrg/Makie.jl/pull/4332)
@@ -17,7 +18,7 @@
 - Make sure we wait for the screen session [#4316](https://github.com/MakieOrg/Makie.jl/pull/4316).
 - Fix for absrect [#4312](https://github.com/MakieOrg/Makie.jl/pull/4312).
 - Fix attribute updates for SpecApi and SpecPlots (e.g. ecdfplot) [#4265](https://github.com/MakieOrg/Makie.jl/pull/4265).
-- Bring back `poly` convert arguments for matrix with points as row [#4266](https://github.com/MakieOrg/Makie.jl/pull/4258).
+- Bring back `poly` convert arguments for matrix with points as row [#4258](https://github.com/MakieOrg/Makie.jl/pull/4258).
 - Fix gl_ClipDistance related segfault on WSL with GLMakie [#4270](https://github.com/MakieOrg/Makie.jl/pull/4270).
 - Added option `label_position = :center` to place labels centered over each bar [#4274](https://github.com/MakieOrg/Makie.jl/pull/4274).
 - `plotfunc()` and `func2type()` support functions ending with `!` [#4275](https://github.com/MakieOrg/Makie.jl/pull/4275).

diff --git a/WGLMakie/src/Lines.js b/WGLMakie/src/Lines.js
@@ -188,7 +188,7 @@ function lines_vertex_shader(uniforms, attributes, is_linesegments) {
                 // used to compute width sdf
                 f_linewidth = halfwidth;
 
-                f_instance_id = uint(2 * gl_InstanceID);
+                f_instance_id = lineindex_start; // NOTE: this is correct, no need to multiple by 2
 
                 // we restart patterns for each segment
                 f_cumulative_length = 0.0;
@@ -640,7 +640,7 @@ function lines_vertex_shader(uniforms, attributes, is_linesegments) {
                 // used to compute width sdf
                 f_linewidth = halfwidth;
 
-                f_instance_id = uint(gl_InstanceID);
+                f_instance_id = lineindex_start;
 
                 f_cumulative_length = lastlen_start;
 

diff --git a/WGLMakie/src/Shaders.js b/WGLMakie/src/Shaders.js
@@ -1,12 +1,22 @@
 function typedarray_to_vectype(typedArray, ndim) {
-    if (ndim === 1) {
-        return "float";
-    } else if (typedArray instanceof Float32Array) {
-        return "vec" + ndim;
+    if (typedArray instanceof Float32Array) {
+        if (ndim === 1) {
+            return "float";
+        } else {
+            return "vec" + ndim;
+        }
     } else if (typedArray instanceof Int32Array) {
-        return "ivec" + ndim;
+        if (ndim === 1) {
+            return "int";
+        } else {
+            return "ivec" + ndim;
+        }
     } else if (typedArray instanceof Uint32Array) {
-        return "uvec" + ndim;
+        if (ndim === 1) {
+            return "uint";
+        } else {
+            return "uvec" + ndim;
+        }
     } else {
         return;
     }

diff --git a/WGLMakie/src/lines.jl b/WGLMakie/src/lines.jl
@@ -43,7 +43,7 @@ function serialize_three(scene::Scene, plot::Union{Lines, LineSegments})
     # involved point are not NaN, i.e. p1 -- p2 is only drawn if all of
     # (p0, p1, p2, p3) are not NaN. So if p3 is NaN we need to dublicate p2 to
     # make the p1 -- p2 segment draw, which is what indices does.
-    indices = Observable(Int[])
+    indices = Observable(UInt32[])
     points_transformed = lift(
             plot, f32c, transform_func_obs(plot), plot.model, plot[1], plot.space
         ) do f32c, tf, model, ps, space
@@ -118,7 +118,10 @@ function serialize_three(scene::Scene, plot::Union{Lines, LineSegments})
         end
     end
     positions = lift(serialize_buffer_attribute, plot, points_transformed)
-    attributes = Dict{Symbol, Any}(:linepoint => positions)
+    attributes = Dict{Symbol, Any}(
+        :linepoint => positions,
+        :lineindex => lift(_ -> serialize_buffer_attribute(indices[]), plot, points_transformed),
+    )
 
     # TODO: in Javascript
     # NOTE: clip.w needs to be available in shaders to avoid line inversion problems

diff --git a/WGLMakie/src/wglmakie.bundled.js b/WGLMakie/src/wglmakie.bundled.js
@@ -20196,14 +20196,24 @@ function getErrorMessage(version) {
     return element;
 }
 function typedarray_to_vectype(typedArray, ndim) {
-    if (ndim === 1) {
-        return "float";
-    } else if (typedArray instanceof Float32Array) {
-        return "vec" + ndim;
+    if (typedArray instanceof Float32Array) {
+        if (ndim === 1) {
+            return "float";
+        } else {
+            return "vec" + ndim;
+        }
     } else if (typedArray instanceof Int32Array) {
-        return "ivec" + ndim;
+        if (ndim === 1) {
+            return "int";
+        } else {
+            return "ivec" + ndim;
+        }
     } else if (typedArray instanceof Uint32Array) {
-        return "uvec" + ndim;
+        if (ndim === 1) {
+            return "uint";
+        } else {
+            return "uvec" + ndim;
+        }
     } else {
         return;
     }
@@ -21473,7 +21483,7 @@ function lines_vertex_shader(uniforms, attributes, is_linesegments) {
                 // used to compute width sdf
                 f_linewidth = halfwidth;
 
-                f_instance_id = uint(2 * gl_InstanceID);
+                f_instance_id = lineindex_start; // NOTE: this is correct, no need to multiple by 2
 
                 // we restart patterns for each segment
                 f_cumulative_length = 0.0;
@@ -21920,7 +21930,7 @@ function lines_vertex_shader(uniforms, attributes, is_linesegments) {
                 // used to compute width sdf
                 f_linewidth = halfwidth;
 
-                f_instance_id = uint(gl_InstanceID);
+                f_instance_id = lineindex_start;
 
                 f_cumulative_length = lastlen_start;
 

diff --git a/src/basic_recipes/buffers.jl b/src/basic_recipes/buffers.jl
@@ -86,7 +86,7 @@ function push!(tb::Annotations, text::String, position::VecTypes{N}; kw_args...)
 end
 
 function append!(tb::Annotations, text::Vector{String}, positions::Vector{Point{N, Float32}}; kw_args...) where N
-    text_positions = convert_argument(Annotations, text, positions)[1]
+    text_positions = convert_arguments(Annotations, text, positions)[1]
     append!(tb, text_positions; kw_args...)
     return
 end

diff --git a/src/conversions.jl b/src/conversions.jl
@@ -55,6 +55,7 @@ end
 # Leave concretely typed vectors alone (AbstractArray{<:Union{Missing, <:Real}} also dispatches for `Vector{Float32}`)
 convert_single_argument(a::AbstractArray{T}) where {T<:Real} = a
 convert_single_argument(a::AbstractArray{<:Point{N, T}}) where {N, T} = a
+convert_single_argument(a::OffsetArray{<:Point}) = OffsetArrays.no_offset_view(a)
 
 
 ################################################################################
@@ -88,6 +89,10 @@ function convert_arguments(::PointBased, positions::AbstractVector{<: VecTypes{N
     return (elconvert(Point{N, float_type(_T)}, positions),)
 end
 
+function convert_arguments(T::PointBased, positions::OffsetVector)
+   return convert_arguments(T, OffsetArrays.no_offset_view(positions))
+end
+
 function convert_arguments(::PointBased, positions::SubArray{<: VecTypes, 1})
     # TODO figure out a good subarray solution
     (positions,)

diff --git a/src/interfaces.jl b/src/interfaces.jl
@@ -134,6 +134,8 @@ expand_dimensions(trait, args...) = nothing
 
 expand_dimensions(::PointBased, y::VecTypes) = nothing # VecTypes are nd points
 expand_dimensions(::PointBased, y::RealVector) = (keys(y), y)
+expand_dimensions(::PointBased, y::OffsetVector{<:Real}) =
+    (OffsetArrays.no_offset_view(keys(y)), OffsetArrays.no_offset_view(y))
 
 function expand_dimensions(::Union{ImageLike, GridBased}, data::AbstractMatrix{<:Union{<:Real, <:Colorant}})
     # Float32, because all ploteable sizes should fit into float32

diff --git a/test/convert_arguments.jl b/test/convert_arguments.jl
@@ -121,6 +121,7 @@ Makie.convert_arguments(::PointBased, ::MyConvVector) = ([Point(10, 20)],)
                 nan = vcat(xs[1:4], NaN, zs[6:end])
                 r = T_in(1):T_in(1):T_in(10)
                 i = T_in(1)..T_in(10)
+                ov = Makie.OffsetVector(ys, -5:4)
 
                 ps2 = Point2.(xs, ys)
                 ps3 = Point3.(xs, ys, zs)
@@ -172,6 +173,7 @@ Makie.convert_arguments(::PointBased, ::MyConvVector) = ([Point(10, 20)],)
 
                         # because indices are Int we end up converting to Float64 no matter what
                         @test apply_conversion(CT, xs)         isa Tuple{Vector{Point2{Float64}}}
+                        @test apply_conversion(CT, ov)         isa Tuple{Vector{Point2{Float64}}}
 
                         @test apply_conversion(CT, xs, ys)     isa Tuple{Vector{Point2{T_out}}}
                         @test apply_conversion(CT, xs, v32)    isa Tuple{Vector{Point2{T_out}}}