[WGLMakie] Constantly getting Invalid text boundingbox errors.

[EdsterG]

• What version of Makie are you running? 0.21.8
• Can you reproduce the bug with a fresh environment ? Yes
• What platform + GPU are you on? MacOS

Unfortunately, I don't have a MWE right now. It's much easier to repro with a complex figure. I've been trying to debug this locally and would like some pointers from the devs. See #4109 (comment) for MWE.

I've narrowed it down to this function:

function string_boundingbox(x::Text{<:Tuple{<:AbstractArray{<:GlyphCollection}}})
    if x.space[] == x.markerspace[]
        pos = to_ndim.(Point3d, x.position[], 0)
    else
        cam = (parent_scene(x).camera,)
        transformed = apply_transform(x.transformation.transform_func[], x.position[])
        pos = Makie.project.(cam, x.space[], x.markerspace[], transformed) # TODO: vectorized project
    end
    return string_boundingbox(x[1][], pos, to_rotation(x.rotation[]))
end

Specifically the first case, pos is invalid because x.position[] is [NaN, NaN, NaN] when x.space[] == x.markerspace[]. What are some reasons that make x.position[] invalid and where can I poke around next?

[ffreyer]

That's just the position of the text plot. And text plots can come from a lot of places, like ticks and labels used in the files under makielayout/blocks, lineaxis.jl, some plot recipes like barplot or tooltip...

[asinghvi17]

Are you using some kind of transform function, or axis scale? Are there any annotations or text plotted directly into the axis?

[EdsterG]

Alright here's the MWE to repo:

import WGLMakie

x = [1, 2, 2, 3]
fig, ax, plt = WGLMakie.stairs(x, 1:4)
WGLMakie.DataInspector(ax)
fig

Seems to be caused by repeated x values, when plotting stairs. The error happens when mousing over the (2, 3) point.

[EdsterG]

Might be related to #3672, as the error doesn't happen with GLMakie.

[EdsterG]

Yep, 100% related. I can fix it locally by modifying position_on_plot. Replace these lines:

Makie.jl/src/interaction/ray_casting.jl

Lines 284 to 288 in 12d0e67

	function position_on_plot(plot::Union{Lines, LineSegments}, idx, ray::Ray; apply_transform = true)
	if idx == 1
	idx = 2
	end
	p0, p1 = apply_transform_and_model(plot, plot[1][][(idx-1):idx])

with:

function position_on_plot(plot::Union{Lines, LineSegments}, idx, ray::Ray; apply_transform = true)
    p0, p1 = apply_transform_and_model(plot, plot[1][][idx:idx+1])

The work around is to monkey patch position_on_plot when running WGLMakie.

The real fix is to make the index returned by the GLMakie/WGLMakie shaders consistent. Given that the code in main works with GLMakie and the monkey patched version works with WGLMakie, it really doesn't matter which convention is used. It just needs to be consistent.

[EdsterG]

I've done a bit more testing with this patch. As a bandaid can't position_on_plot for line plots be dispatched based on the backend?

[ffreyer]

I'd rather just fix #3672 directly. That's probably just adding a +1 to f_instance_id in the vertex shaders in https://github.com/MakieOrg/Makie.jl/blob/master/WGLMakie/src/Lines.js. (To apply those changes you need to remove the bundled file and let it regenerate.)

In GLMakie these are set in https://github.com/MakieOrg/Makie.jl/blob/master/GLMakie/assets/shader/lines.vert and https://github.com/MakieOrg/Makie.jl/blob/master/GLMakie/assets/shader/line_segment.vert as g_id. GLMakie sets them based on line points, WGLMakie based on segments. (To reload shaders you need to call GLMakie.closeall().)

[EdsterG]

That would be best, I have no experience with shaders so I had no sense of scope for the real fix. Out of curiosity, why does WGLMakie use gl_InstanceID and GLMakie use gl_VertexID?

[EdsterG]

I think fixing WGLMakie is probably easier, but that still leaves an open question about the "intuitiveness" of the index.

From #3672:

It shows a hover window with the text "hello" in the right position. Mousing over the blue segment now correctly shows a hover window with the correct text "nice".

Would imply that adding 1 to the WGLMakie index would break what the user expects. To correct this, should show_data be updated to subtract 1 from idx before passing it to inspector_label?

[ffreyer]

That would be best, I have no experience with shaders so I had no sense of scope for the real fix. Out of curiosity, why does WGLMakie use gl_InstanceID and GLMakie use gl_VertexID?

Ultimately because WebGL doesn't support geometry shaders. In short GLMakie pushes the positions to the GPU, which then generates quads for the line segments in the geometry shader. WGLMakie generates a quad on CPU and pushes that with positions etc to the GPU, where it gets adjusted to fit the current segment.
With that the positions are vertices in GLMakie and there is only one instance, because it doesn't repeatedly render those vertices. In WGLMakie the vertices refer to the quad, with different instances being the different line segments.

I think fixing WGLMakie is probably easier, but that still leaves an open question about the "intuitiveness" of the index.

From #3672:

It shows a hover window with the text "hello" in the right position. Mousing over the blue segment now correctly shows a hover window with the correct text "nice".

Would imply that adding 1 to the WGLMakie index would break what the user expects. To correct this, should show_data be updated to subtract 1 from idx before passing it to inspector_label?

I would say matching GLMakie would be best for now. We tend to see GLMakie as the source of truth for backend differences, so it's easier to call this a bug fix if we just make WGLMakie match. Changing GLMakie/both to be intuitive would be breaking imo and probably involves aa good amount of discussion on what's intuitive

[EdsterG]

Looks like WGLMakie shaders need more fixing. When mousing over the last segment, the index returned by WGLMakie is larger than the step array. For this specific example, the stairs data is 15 element long but the index returned by pick_closest is 18.

import WGLMakie

x = [1, 2, NaN, 3, 4, NaN, 5, 6]
fig, ax, plt = WGLMakie.stairs(x, 1:length(x))
WGLMakie.DataInspector(ax)
fig

This snippet works well in GLMakie.

[ffreyer]

Oh I forgot about NaNs. Three.js (or WebGL?) refuses to even try rendering when there is a NaN involved in vertex/instance data, so when there is we pad out the inputs. If that's part of the issue I should probably work on this myself...

More details

Each line segment uses 4 line points for rendering with lines, because joints require information about the previous and next segment. With that each segment needs a valid (previous, start, end, next) line point in order to draw.

The points stairs generates are:

 [1.0, 1.0]
 [1.0, 2.0]
 [2.0, 2.0]
 [2.0, 3.0]
 [NaN, 3.0]
 [NaN, 4.0]
 [3.0, 4.0]
 [3.0, 5.0]
 [4.0, 5.0]
 [4.0, 6.0]
 [NaN, 6.0]
 [NaN, 7.0]
 [5.0, 7.0]
 [5.0, 8.0]
 [6.0, 8.0]

With padding these become:

 x    [1.0, 1.0]
 0 [1.0, 1.0]
 1 [1.0, 2.0]
 2 [2.0, 2.0]
 3 [2.0, 3.0]
 4    [2.0, 3.0]
 5 [NaN, 3.0]
 6 [NaN, 4.0]
 7    [3.0, 4.0]
 8 [3.0, 4.0]
 9 [3.0, 5.0]
10 [4.0, 5.0]
11 [4.0, 6.0]
12    [4.0, 6.0]
13 [NaN, 6.0]
14 [NaN, 7.0]
15    [5.0, 7.0]
16 [5.0, 7.0]
17 [5.0, 8.0]
 x [6.0, 8.0]
 x    [6.0, 8.0]

The numbers should match the instance index where the point is considered the start point. I'm guessing the 0 index -> 1 index happens in pick. Otherwise this matches up with the indices you see when picking your example.

[EdsterG]

Thanks for the details explanation about padding! If I'm not mistaken, shaders can have meta data attached to them. In which case the padding count/offset can be attached to each point and then subtracted from the index before it's returned to pick_native, which then as you said applies to 0 -> 1 index conversion.

[EdsterG]

By the way, as an alternative to padding NaN's what's wrong with replacing them with Inf before passing to Three.js? From a rendering perspective, it seems to have the same effect.

[EdsterG]

Replacing NaN with Inf right before rending seems to work quite well.

    points_transformed = lift(
            plot, f32_conversion_obs(scene), transform_func_obs(plot), plot[1], plot.space
        ) do f32c, tf, ps, space

        transformed_points = apply_transform_and_f32_conversion(f32c, tf, ps, space)
        # TODO: Do this in javascript?
        empty!(indices[])
        if isempty(transformed_points)
            notify(indices)
            return transformed_points
        else

            sizehint!(indices[], length(transformed_points) + 2)
            append!(indices[], 1, 1:length(transformed_points), length(transformed_points))

            # NOTE: replacing NaN with Inf correctly renders lines separated by NaN
            # and avoids having an incorrect pick point index due to padding.
            return map(indices[]) do i
                map(x -> isnan(x) ? Inf32 : x, transformed_points[i])
            end

        end
    end

What pitfalls do you see with this approach?

[ffreyer]

I'd have to get back into that code to give you a good answer, but the duplication is important to detect line starts/ends and the NaN is important to separate the line pieces atm. That information probably needs to get pushed through a new instance buffer for lines to work correctly. Closed line loops may also need some special care here

[EdsterG]

NaN is important to separate the line pieces atm

Three.js handles Inf as you would expect, visually the lines are separate. I was able to fix the out of bounds index by updating serialize_three with the snippet I posted above. My snippet needs to be cleaned up and made a little more memory efficient, but it seems to solve the problem.