Add line loop rendering (#3907)

* add loop rendering * get loops working in WGLMakie * fix typing * fix pattern offset in WGLMakie * rewrite index generation * update changelog * add test and activate miter limit test * remove line start/end elongation * update tooltip * fix point dublication in poly outline * cleanup print * fix CairoMakie line loops * fix empty array error --------- Co-authored-by: Simon <sdanisch@protonmail.com>
MakieOrg · Jun 19, 2024 · cd1df49 · cd1df49
1 parent 5f70c5b
commit cd1df49
Show file tree

Hide file tree

Showing 11 changed files with 242 additions and 65 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## [Unreleased]
 
+- Add line-loop detection and rendering to GLMakie and WGLMakie [#3907](https://github.com/MakieOrg/Makie.jl/pull/3907)
+
 ## [0.21.3] - 2024-06-17
 
 - Fix stack overflows when using `markerspace = :data` with `scatter` [#3960](https://github.com/MakieOrg/Makie.jl/issues/3960).

diff --git a/CairoMakie/src/primitives.jl b/CairoMakie/src/primitives.jl
@@ -202,17 +202,23 @@ project_command(c::ClosePath, scene, space, model) = c
 
 function draw_single(primitive::Lines, ctx, positions)
     n = length(positions)
+    start = positions[begin]
+
     @inbounds for i in 1:n
         p = positions[i]
         # only take action for non-NaNs
         if !isnan(p)
             # new line segment at beginning or if previously NaN
             if i == 1 || isnan(positions[i-1])
                 Cairo.move_to(ctx, p...)
+                start = p
             else
                 Cairo.line_to(ctx, p...)
                 # complete line segment at end or if next point is NaN
                 if i == n || isnan(positions[i+1])
+                    if p ≈ start
+                        Cairo.close_path(ctx)
+                    end
                     Cairo.stroke(ctx)
                 end
             end
@@ -298,7 +304,8 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin
     prev_position = positions[begin]
     prev_nan = isnan(prev_position)
     prev_continued = false
-
+    start = positions[begin]
+
     if !prev_nan
         # first is not nan, move_to
         Cairo.move_to(ctx, positions[begin]...)
@@ -315,6 +322,7 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin
             # this is nan
             if prev_continued
                 # and this is prev_continued, so set source and stroke to finish previous line
+                (prev_position ≈ start) && Cairo.close_path(ctx)
                 Cairo.set_line_width(ctx, this_linewidth)
                 !isnothing(dash) && Cairo.set_dash(ctx, dash .* this_linewidth)
                 Cairo.set_source_rgba(ctx, red(prev_color), green(prev_color), blue(prev_color), alpha(prev_color))
@@ -328,6 +336,7 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin
             if !this_nan
                 # but this is not nan, so move to this position
                 Cairo.move_to(ctx, this_position...)
+                start = this_position
             else
                 # and this is also nan, do nothing
             end
@@ -342,6 +351,7 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin
 
                     if i == lastindex(positions)
                         # this is the last element so stroke this
+                        (this_position ≈ start) && Cairo.close_path(ctx)
                         Cairo.set_line_width(ctx, this_linewidth)
                         !isnothing(dash) && Cairo.set_dash(ctx, dash .* this_linewidth)
                         Cairo.set_source_rgba(ctx, red(this_color), green(this_color), blue(this_color), alpha(this_color))

diff --git a/GLMakie/assets/shader/line_segment.geom b/GLMakie/assets/shader/line_segment.geom
@@ -82,7 +82,7 @@ void main(void)
     // Set invalid / ignored outputs
     f_truncation = vec2(-1e12);     // no truncated joint
     f_pattern_overwrite = vec4(-1e12, 1.0, 1e12, 1.0); // no joints to overwrite
-    f_extrusion = vec2(0.5);        // no joints needing extrusion
+    f_extrusion = vec2(0.0);        // no joints needing extrusion
     f_linepoints = vec4(-1e12);
     f_miter_vecs = vec4(-1);
 

diff --git a/GLMakie/assets/shader/lines.geom b/GLMakie/assets/shader/lines.geom
@@ -196,22 +196,32 @@ void main(void)
         return;
     }
 
-    // We mark each of the four vertices as valid or not. Vertices can be
-    // marked invalid on input (eg, if they contain NaN). We also mark them
-    // invalid if they repeat in the index buffer. This allows us to render to
-    // the very ends of a polyline without clumsy buffering the position data on the
-    // CPU side by repeating the first and last points via the index buffer. It
-    // just requires a little care further down to avoid degenerate normals.
+    // We mark vertices based on their role in a line segment:
+    //  0: the vertex is skipped/invalid (i.e. NaN)
+    //  1: the vertex is valid (part of a plain line segment)
+    //  2: the vertex is either ..
+    //       a loop target if the previous or next vertex is marked 0
+    //       or a normal valid vertex otherwise
+    // isvalid[0] and [3] are used to discern whether a line segment is part
+    // of a continuing line (valid) or a line start/end (invalid). A line only
+    // ends if the previous / next vertex is invalid
+    // isvalid[1] and [2] are used to discern whether a line segment should be
+    // discarded. This should happen if either vertex is invalid or if one of
+    // the vertices is a loop target.
+    // A loop target is an extra vertex placed before/after the shared vertex to
+    // guide joint generation. Consider for example a closed triangle A B C A.
+    // To cleanly close the loop both A's need to create a joint as if we had
+    // c A B C A b, but without drawing the c-A and A-b segments. c and b would
+    // be loop targets, matching C and B in position, but only being valid in
+    // isvalid[0] and [3], not as a drawn segment in isvalid[1] and [2].
     bool isvalid[4] = bool[](
-        g_valid_vertex[0] == 1 && g_id[0].y != g_id[1].y,
-        g_valid_vertex[1] == 1,
-        g_valid_vertex[2] == 1,
-        g_valid_vertex[3] == 1 && g_id[2].y != g_id[3].y
+        (g_valid_vertex[0] > 0) && g_id[0].y != g_id[1].y,
+        (g_valid_vertex[1] > 0) && !((g_valid_vertex[0] == 0) && (g_valid_vertex[1] == 2)),
+        (g_valid_vertex[2] > 0) && !((g_valid_vertex[2] == 2) && (g_valid_vertex[3] == 0)),
+        (g_valid_vertex[3] > 0) && g_id[2].y != g_id[3].y
     );
 
     if(!isvalid[1] || !isvalid[2]){
-        // If one of the central vertices is invalid or there is a break in the
-        // line, we don't emit anything.
         return;
     }
 
@@ -407,8 +417,8 @@ void main(void)
     // if joint skipped   elongate to new length
     // if normal joint    elongate a lot to let discard/truncation handle joint
     f_extrusion = vec2(
-        !isvalid[0] ? min(AA_RADIUS, halfwidth) : (adjustment[0] == 0.0 ? 1e12 : halfwidth * abs(extrusion[0][0])),
-        !isvalid[3] ? min(AA_RADIUS, halfwidth) : (adjustment[1] == 0.0 ? 1e12 : halfwidth * abs(extrusion[1][0]))
+        !isvalid[0] ? 0.0 : (adjustment[0] == 0.0 ? 1e12 : halfwidth * abs(extrusion[0][0])),
+        !isvalid[3] ? 0.0 : (adjustment[1] == 0.0 ? 1e12 : halfwidth * abs(extrusion[1][0]))
     );
 
     // used to compute width sdf

diff --git a/GLMakie/src/glshaders/lines.jl b/GLMakie/src/glshaders/lines.jl
@@ -29,6 +29,85 @@ gl_color_type_annotation(::Real) = "float"
 gl_color_type_annotation(::Makie.RGB) = "vec3"
 gl_color_type_annotation(::Makie.RGBA) = "vec4"
 
+function generate_indices(positions)
+    valid_obs = Observable(Float32[]) # why does this need to be a float?
+
+    indices_obs = const_lift(positions) do ps
+        valid = valid_obs[]
+        resize!(valid, length(ps))
+
+        indices = Cuint[]
+        sizehint!(indices, length(ps)+2)
+
+        # This loop identifies sections of line points A B C D E F bounded by
+        # the start/end of the list ps or by NaN and generates indices for them:
+        # if A == F (loop):      E A B C D E F B 0
+        # if A != F (no loop):   0 A B C D E F 0
+        # where 0 is NaN
+        # It marks vertices as invalid (0) if they are NaN, valid (1) if they
+        # are part of a continous line section, or as ghost edges (2) used to
+        # cleanly close a loop. The shader detects successive vertices with
+        # 1-2-0 and 0-2-1 validity to avoid drawing ghost segments (E-A from
+        # 0-E-A-B and F-B from E-F-B-0 which would dublicate E-F and A-B)
+
+        last_start_pos = eltype(ps)(NaN)
+        last_start_idx = -1
+
+        for (i, p) in enumerate(ps)
+            not_nan = isfinite(p)
+            valid[i] = not_nan
+
+            if not_nan
+                if last_start_idx == -1
+                    # place nan before section of line vertices
+                    # (or dublicate ps[1])
+                    push!(indices, i-1)
+                    last_start_idx = length(indices) + 1
+                    last_start_pos = p
+                end
+                # add line vertex
+                push!(indices, i)
+
+            # case loop (loop index set, loop contains at least 3 segments, start == end)
+            elseif (last_start_idx != -1) && (length(indices) - last_start_idx > 2) &&
+                    (ps[max(1, i-1)] ≈ last_start_pos)
+
+                # add ghost vertices before an after the loop to cleanly connect line
+                indices[last_start_idx-1] = max(1, i-2)
+                push!(indices, indices[last_start_idx+1], i)
+                # mark the ghost vertices
+                valid[i-2] = 2
+                valid[indices[last_start_idx+1]] = 2
+                # not in loop anymore
+                last_start_idx = -1
+
+            # non-looping line end
+            elseif (last_start_idx != -1) # effective "last index not NaN"
+                push!(indices, i)
+                last_start_idx = -1
+            # else: we don't need to push repeated NaNs
+            end
+        end
+
+        # treat ps[end+1] as NaN to correctly finish the line
+        if (last_start_idx != -1) && (length(indices) - last_start_idx > 2) &&
+                (ps[end] ≈ last_start_pos)
+
+            indices[last_start_idx-1] = length(ps) - 1
+            push!(indices, indices[last_start_idx+1])
+            valid[end-1] = 2
+            valid[indices[last_start_idx+1]] = 2
+        elseif last_start_idx != -1
+            push!(indices, length(ps))
+        end
+
+        notify(valid_obs)
+        return indices .- Cuint(1)
+    end
+
+    return indices_obs, valid_obs
+end
+
 @nospecialize
 function draw_lines(screen, position::Union{VectorTypes{T}, MatTypes{T}}, data::Dict) where T<:Point
     p_vec = if isa(position, GPUArray)
@@ -37,11 +116,13 @@ function draw_lines(screen, position::Union{VectorTypes{T}, MatTypes{T}}, data::
         const_lift(vec, position)
     end
 
+    indices, valid_vertex = generate_indices(p_vec)
+
     color_type = gl_color_type_annotation(data[:color])
     resolution = data[:resolution]
 
     @gen_defaults! data begin
-        total_length::Int32 = const_lift(x-> Int32(length(x)), position)
+        total_length::Int32 = const_lift(x -> Int32(length(x) - 2), indices)
         vertex              = p_vec => GLBuffer
         color               = nothing => GLBuffer
         color_map           = nothing => Texture
@@ -53,10 +134,7 @@ function draw_lines(screen, position::Union{VectorTypes{T}, MatTypes{T}}, data::
         # Duplicate the vertex indices on the ends of the line, as our geometry
         # shader in `layout(lines_adjacency)` mode requires each rendered
         # segment to have neighbouring vertices.
-        indices             = const_lift(p_vec) do p
-            len0 = length(p) - 1
-            return isempty(p) ? Cuint[] : Cuint[0; 0:len0; len0]
-        end => to_index_buffer
+        indices             = indices => to_index_buffer
         transparency = false
         fast         = false
         shader              = GLVisualizeShader(
@@ -70,9 +148,7 @@ function draw_lines(screen, position::Union{VectorTypes{T}, MatTypes{T}}, data::
             )
         )
         gl_primitive        = GL_LINE_STRIP_ADJACENCY
-        valid_vertex        = const_lift(p_vec) do points
-            map(p-> Float32(all(isfinite, p)), points)
-        end => GLBuffer
+        valid_vertex        = valid_vertex => GLBuffer
         lastlen             = const_lift(sumlengths, p_vec, resolution) => GLBuffer
         pattern_length      = 1f0 # we divide by pattern_length a lot.
         debug               = false

diff --git a/ReferenceTests/src/tests/primitives.jl b/ReferenceTests/src/tests/primitives.jl
@@ -84,8 +84,28 @@ end
     fig
 end
 
-#@reference_test "Miter Limit"
-begin
+@reference_test "Line loops" begin
+    # check for issues with self-overlap of line segments with loops, interplay
+    # between loops, lines, nan separation
+    loop(p) = Point2f[p, p .+ Point2f(0.8, 0), p .+ Point2f(0, 0.8), p, Point2f(NaN)]
+    line(p) = Point2f[p, p .+ Point2f(0.8, 0), p .+ Point2f(0, 0.8), Point2f(NaN)]
+
+    nan = [Point2f(NaN)]
+    ps = vcat(
+        nan, nan, nan, loop((0, -1)), loop((1, -1)),
+        line((-1, 0)), line((0, 0)),
+        nan, nan, line((1, 0)), nan,
+        loop((-1, 1)), nan, loop((0, 1)),
+        nan, [Point2f(1, 1)], nan
+    )
+
+    f, a, p = lines(loop((-1, -1)), linewidth = 20, linecap = :round, alpha = 0.5)
+    lines!(ps, linewidth = 20, linecap = :round, alpha = 0.5)
+    lines!(vcat(nan, nan, line((1, 1)), nan), linewidth = 20, linecap = :round, alpha = 0.5)
+    f
+end
+
+@reference_test "Miter Limit" begin
     ps = [Point2f(0, -0.5), Point2f(1, -0.5)]
     for phi in [160, -130, 121, 50, 119, -90] # these are 180-miter_angle
         R = Makie.Mat2f(cosd(phi), sind(phi), -sind(phi), cosd(phi))

diff --git a/WGLMakie/src/Lines.js b/WGLMakie/src/Lines.js
@@ -548,12 +548,12 @@ function lines_vertex_shader(uniforms, attributes, is_linesegments) {
                 }
 
                 // Used to elongate sdf to include joints
-                // if start/end         elongate slightly so that there is no AA gap in loops
+                // if start/end         no elongation
                 // if joint skipped     elongate to new length
                 // if normal joint      elongate a lot to let shape/truncation handle joint
                 f_extrusion = vec2(
-                    !isvalid[0] ? min(AA_RADIUS, halfwidth) : (adjustment[0] == 0.0 ? 1e12 : halfwidth * abs(extrusion[0])),
-                    !isvalid[3] ? min(AA_RADIUS, halfwidth) : (adjustment[1] == 0.0 ? 1e12 : halfwidth * abs(extrusion[1]))
+                    !isvalid[0] ? 0.0 : (adjustment[0] == 0.0 ? 1e12 : halfwidth * abs(extrusion[0])),
+                    !isvalid[3] ? 0.0 : (adjustment[1] == 0.0 ? 1e12 : halfwidth * abs(extrusion[1]))
                 );
 
                 // used to compute width sdf