fix nan handling in WGLMakie

[EdsterG]

Description

Fixes #4109
Fixes #3672

This PR is a proof of concept, mainly to solicit feedback from the developers. Instead of reading the instance id from the shader, use the index that's already computed when handling nans and creating line loops. It's not ideal that the index needs to be serialized and passed to the front end as it adds additional overhead, but I don't think the shaders current have enough information to infer the index otherwise.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Added an entry in CHANGELOG.md (for new features and breaking changes)

[EdsterG]

Would be nice to get some feedback/direction. @ffreyer

[ffreyer]

I don't have time to think about this more deeply atm because of the GeometryBasics refactor.

I remember that there were some issues with the number of (varying) buffer variables when we reworked the WGLMakie line shaders. I think that was with vertex -> fragment shader buffers, but I'm not sure anymore. That would be something to check.

If there are no issues with those limits then I think this approach is fine for now. The code in Shaders.js looks like there is some encoding or decoding missing for ints, I'd guess either with serialize_tree on the Julia side or that attribute_type function?

[EdsterG]

Thank you for the context switch and reply to this PR.

I haven't run into any issues with the number of buffer variables in the shaders, and I've been plotting complex figures with this change for over a week now. Was there a more direct check you had in mind?

I took another look at Shaders.js and was able to find where the missing encoding was. Cleaned up the code and tested. Everything works well.

[ffreyer]

Did some googling again to figure out what I'm actually talking about. There is a GPU-dependent maximum number of vec4's that can go into a shader stage. I thought it was (also) WebGL Version dependent, maybe not? All GPUs allow 8+, most 12+, see https://opengles.gpuinfo.org/displaycapability.php?name=GL_MAX_VARYING_VECTORS&esversion=2

We have:

• 4x vec3 pos (or was it vec4?)
• 2x vec4 color
• 2x float lastlen
• 2x linewidth, counting as 0.5x vec4

where vec4 and vec3 count for 1 slot, vec2 for 0.5 slots and float for 0.25 slots, so 4+2+0.5+0.5 = 7 total. (See https://stackoverflow.com/questions/26682631/webgl-shaders-maximum-number-of-varying-variables) You're adding another pair of uints, which should count as half a vec4 slot if I understand things correctly. So I think we're fine here.

Is this producing the same indices as GLMakie?

[EdsterG]

Is this producing the same indices as GLMakie?

This is producing the same indices, though maybe in a round-about way. The code returns lineindex_start and then 1 is added in pick_native which makes the index match GLMakie.

Makie.jl/WGLMakie/src/picking.jl

Line 22 in 8617742

return (lookup[uuid], Int(index) + 1)

I chose to leave the +1 to minimize the overall diff but can remove it and have the code return lineindex_end instead. Let me know which you prefer, if any.

Also, it looks like the tests are failing because offset arrays can have negative indices. I can revert lineindex to be an int but that'll immediately cut the max number of line points in half. It looks like the shader only has 16 bits of precision for the index, so going from uint to int is quite significant in my opinion, 65535 points down to 32767 points.

Makie.jl/WGLMakie/src/wglmakie.js

Line 539 in 8617742

const index = reinterpret_view.getUint16(i * 4 + 2);

I'll need to think about how to do this in a way that'll preserve the full uint range. Please let me know if you have any suggestions.

[ffreyer]

Now that I think about it I wonder why we let OffsetArrays get this far. @asinghvi17 Is there any good reason to not convert them in convert_arguments?

[asinghvi17]

Ideally offset arrays should be converted in convert_arguments IMO. I had a PR to do that generically but it's pretty outdated now I think. Will see about resurrecting it though.

[ffreyer]

Ok, I think we should handle OffsetArray in convert_arguments then and not complicate picking by considering OffsetArrays there. I guess we can have something like enumerate(eachindex(ps)) as a bandaid fix here until we move the conversion? Or maybe work with values(ps)?

[EdsterG]

Ok, I added a convert_argument definition for point based offset vectors. Tests pass now.

[EdsterG]

I've increased code coverage and the coverage check is still complaining. I think the code coverage pipeline is broken because it shows 40% and 80% coverage, but when I look at the detailed view all I see is 80%.

[ffreyer]

I tested this locally, seems to be working fine.

@SimonDanisch Is there a way we can test picking in WGLMakie?

@asinghvi17 Do you think we need any other OffsetArray conversions for lines/PointBased?

[SimonDanisch]

I think pick(scene, mousepos) should just work fine? Unless it requires advanced opengl which electron on CI doesn't offer (unlikely I think, since it renders the Volumes just fine).

[EdsterG]

@SimonDanisch would you point me to a working example? I need a screen object to test pick and I can't find a single WGLMakie test that generates a screen object in CI. The following code just hangs in a headless machine:

    fig, ax, plt = lines(1:5, [1:2; NaN; 4:5]);
    screen = display(fig); # hangs
    screen = display(WGLMakie.Screen(fig.scene), fig); # hangs
    screen = display(WGLMakie.Screen(fig.scene, visible=false), fig); # also hangs
    screen = Makie.getscreen(fig.scene) # return nothing

[SimonDanisch]

On headless you need:

using Bonito
import Electron
edisplay = Bonito.use_electron_display()

Which should already be part of the WGLMakie CI!

[EdsterG]

Hmm, I can't seem to get this working locally. I just get the following error:

julia> plot,idx = pick(ax.scene, point_px)
ERROR: BoundsError: attempt to access 0×0 Matrix{Tuple{Union{Nothing, AbstractPlot}, Int64}} at index [1, 1]
Stacktrace:
 [1] throw_boundserror(A::Matrix{Tuple{Union{Nothing, AbstractPlot}, Int64}}, I::Tuple{Int64, Int64})
   @ Base ./essentials.jl:14
 [2] checkbounds
   @ ./abstractarray.jl:699 [inlined]
 [3] getindex(::Matrix{Tuple{Union{Nothing, AbstractPlot}, Int64}}, ::Int64, ::Int64)
   @ Base ./array.jl:918
 [4] pick(::Scene, screen::WGLMakie.Screen, xy::Vec{2, Float64})
   @ WGLMakie ~/.julia/packages/WGLMakie/80NMe/src/picking.jl:67
 [5] pick(scene::Scene, xy::Vec{2, Float64})
   @ Makie ~/.julia/packages/Makie/aQT24/src/interaction/interactive_api.jl:73
 [6] top-level scope
   @ REPL[29]:1

Would you please post a MWE for pick without nan and I can then modify it to test the NaN case.

[ffreyer]

Maybe we should just handle that in a separate pr then. We'll need to make one for picking other plots anyway

[EdsterG]

What do you think @SimonDanisch? Shall we merge as is?

[SimonDanisch]

I'm fine with that :)