Merge branch 'master' into ff/voxel-uv

CHANGELOG.md

@@ -17,6 +17,10 @@
 - Fixed `uv_transform = :rotr90` and `:rotl90` being swapped [#4758](https://github.com/MakieOrg/Makie.jl/pull/4758)
 - Cleaned up surface handling in GLMakie: Surface cells are now discarded when there is a nan in x, y or z. Fixed incorrect normal if x or y is nan [#4735](https://github.com/MakieOrg/Makie.jl/pull/4735)
 - Cleaned up `volume` plots: Added `:indexedabsorption` and `:additive` to WGLMakie, generalized `:mip` to include negative values, fixed missing conversions for rgba algorithms (`:additive`, `:absorptionrgba`), fixed missing conversion for `absorption` attribute & extended it to `:indexedabsorption` and `absorptionrgba`, added tests and improved docs. [#4726](https://github.com/MakieOrg/Makie.jl/pull/4726)
+- Fixed integer underflow in GLMakie line indices which may have caused segmentation faults on mac [#4782](https://github.com/MakieOrg/Makie.jl/pull/4782)
+- Added `Axis3.clip` attribute to allow turning off clipping [#4791](https://github.com/MakieOrg/Makie.jl/pull/4791)
+- Fixed `Plane(Vec{N, T}(0), dist)` producing a `NaN` normal, which caused WGLMakie to break. (E.g. when rotating Axis3) [#4772](https://github.com/MakieOrg/Makie.jl/pull/4772)
+- Reverted change to `poly` which disallowed 3D geometries from being plotted [#4738](https://github.com/MakieOrg/Makie.jl/pull/4738)
 
 ## [0.22.1] - 2025-01-17
 

GLMakie/src/GLAbstraction/GLBuffer.jl

@@ -11,9 +11,10 @@ mutable struct GLBuffer{T} <: GPUArray{T, 1}
         switch_context!(context)
         id = glGenBuffers()
         glBindBuffer(buffertype, id)
-        # size of 0 can segfault it seems
-        buff_length = buff_length == 0 ? 1 : buff_length
-        glBufferData(buffertype, buff_length * sizeof(T), ptr, usage)
+        # size of 0 can segfault it seems, but so can a draw call to an index buffer that has garbage data.
+        # Therefore we let the buffer pull some garbage data in to have a GPU size > 0,
+        # but keep the CPU size unchanged. Draw calls are then discarded if the CPU size is 0.
+        glBufferData(buffertype, max(1, buff_length) * sizeof(T), ptr, usage)
         glBindBuffer(buffertype, 0)
 
         obj = new(
@@ -105,7 +106,15 @@ function gpu_data(b::GLBuffer{T}) where T
     bind(b)
     glGetBufferSubData(b.buffertype, 0, sizeof(data), data)
     bind(b, 0)
-    data
+    return data
+end
+
+# for render() debug checks
+function gpu_data_no_unbind(b::GLBuffer{T}) where T
+    data = Vector{T}(undef, length(b))
+    bind(b)
+    glGetBufferSubData(b.buffertype, 0, sizeof(data), data)
+    return data
 end
 
 

GLMakie/src/GLAbstraction/GLRender.jl

@@ -96,75 +96,101 @@ function render(renderobject::RenderObject, vertexarray=renderobject.vertexarray
     return
 end
 
+function vao_boundscheck(target::Integer, current::Integer, vao)
+    if target <= current # assuming 0-based OpenGL indices
+        msg = IOBuffer()
+        print(msg, "BoundsError: OpenGL vertex index $current exceeds the number of vertices $target.\n Occurred with ")
+        show(msg, MIME"text/plain"(), vao)
+        error(String(take!(msg)))
+    end
+end
 
+# multiple index ranges
 """
-Renders a vertexarray, which consists of the usual buffers plus a vector of
-unitranges which defines the segments of the buffers to be rendered
+    render(vao::GLVertexArray[, mode = GL_TRIANGLES])
+
+Renders a vertexarray based on its `vao.indices` type.
 """
 function render(vao::GLVertexArray{T}, mode::GLenum=GL_TRIANGLES) where T <: VecOrSignal{UnitRange{Int}}
+    N_vert = length(vao)
     for elem in to_value(vao.indices)
+        # TODO: Should this exclude last(elem), i.e. shift a:b to (a-1):(b-1)
+        #       instead of (a-1):b?
+        vao_boundscheck(N_vert, last(elem), vao)
         glDrawArrays(mode, max(first(elem) - 1, 0), length(elem) + 1)
     end
     return nothing
 end
 
+# by index range to draw
 function render(vao::GLVertexArray{T}, mode::GLenum=GL_TRIANGLES) where T <: TOrSignal{UnitRange{Int}}
     r = to_value(vao.indices)
     ndraw = length(r)
     ndraw == 0 && return nothing
     offset = first(r) - 1 # 1 based -> 0 based
     nverts = length(vao)
-    if (offset < 0 || offset + ndraw > nverts)
-        error("Bounds error for drawrange. Offset $(offset) and length $(ndraw) aren't a valid range for vertexarray with length $(nverts)")
-    end
+    offset < 0 && error("Range of vertex indices must not be < 0, but is $offset")
+    vao_boundscheck(nverts, offset + nverts, vao)
     glDrawArrays(mode, offset, ndraw)
     return nothing
 end
 
+# by number of triangles
 function render(vao::GLVertexArray{T}, mode::GLenum=GL_TRIANGLES) where T <: TOrSignal{Int}
     r = to_value(vao.indices)
     r == 0 && return nothing
     glDrawArrays(mode, 0, r)
     return nothing
 end
 
-"""
-Renders a vertex array which supplies an indexbuffer
-"""
+# using indexbuffer (faces)
 function render(vao::GLVertexArray{GLBuffer{T}}, mode::GLenum=GL_TRIANGLES) where T <: Union{Integer,AbstractFace}
+    # Note: not discarding draw calls with 0 indices may cause segfaults even if
+    # the draw call is later discarded based on on `mode`. See #4782
     N = length(vao.indices) * cardinality(vao.indices)
     N == 0 && return nothing
+    if GLMAKIE_DEBUG[]
+        data = gpu_data_no_unbind(vao.indices)
+        @assert !isempty(data)
+        # raw() to get 0-based value from Faces, does nothing for Int
+        N_addressed = GeometryBasics.raw(mapreduce(maximum, max, data))
+        vao_boundscheck(length(vao), N_addressed, vao)
+    end
     glDrawElements(mode, N, julia2glenum(T), C_NULL)
     return nothing
 end
 
-"""
-Renders a normal vertex array only containing the usual buffers buffers.
-"""
+# undefined indices, default to rendering all vertices
 function render(vao::GLVertexArray, mode::GLenum=GL_TRIANGLES)
     length(vao) == 0 && return nothing
     glDrawArrays(mode, 0, length(vao))
     return nothing
 end
 
 """
-Render instanced geometry
+    renderinstanced(vao::GLVertexArray, instance_count[, primitive = GL_TRIANGLES])
+
+Render `instance_count` instances of the given vertex array based on the type of
+`vao.indices`.
 """
 renderinstanced(vao::GLVertexArray, a, primitive=GL_TRIANGLES) = renderinstanced(vao, length(a), primitive)
 
-"""
-Renders `amount` instances of an indexed geometry
-"""
+# using index buffer
 function renderinstanced(vao::GLVertexArray{GLBuffer{T}}, amount::Integer, primitive=GL_TRIANGLES) where T <: Union{Integer,AbstractFace}
     N = length(vao.indices) * cardinality(vao.indices)
     N * amount == 0 && return nothing
+    if GLMAKIE_DEBUG[]
+        data = gpu_data_no_unbind(vao.indices)
+        @assert !isempty(data)
+        # raw() to get 0-based value from Faces, does nothing for Int
+        N_addressed = GeometryBasics.raw(mapreduce(maximum, max, data))
+        vao_boundscheck(length(vao), N_addressed, vao)
+    end
     glDrawElementsInstanced(primitive, N, julia2glenum(T), C_NULL, amount)
     return nothing
 end
 
-"""
-Renders `amount` instances of an not indexed geometry geometry
-"""
+# based on number of vertices
 function renderinstanced(vao::GLVertexArray, amount::Integer, primitive=GL_TRIANGLES)
     length(vao) * amount == 0 && return nothing
     glDrawElementsInstanced(primitive, length(vao), GL_UNSIGNED_INT, C_NULL, amount)

GLMakie/src/GLAbstraction/GLTypes.jl

@@ -184,10 +184,16 @@ mutable struct GLVertexArray{T}
 end
 
 """
-returns the length of the vertex array.
-This is amount of primitives stored in the vertex array, needed for `glDrawArrays`
+    length(vao::GLVertexArray)
+
+Returns the length of the vertex array. More specifically this returns the minimum
+of the lengths of each vertex buffer, i.e. the number of addressable vertices. If
+no vertex buffers are present -1 is returned.
 """
-length(vao::GLVertexArray) = length(first(vao.buffers)[2]) # all buffers have same length, so first should do!
+function length(vao::GLVertexArray)
+    isempty(vao.buffers) && return -1
+    return mapreduce(length, min, values(vao.buffers))
+end
 
 GLVertexArray(vao::GLVertexArray) = GLVertexArray(vao.buffers, vao.program)
 
@@ -283,14 +289,17 @@ function bind(va::GLVertexArray)
     glBindVertexArray(va.id)
 end
 
-
-function Base.show(io::IO, vao::GLVertexArray)
-    show(io, vao.program)
+Base.show(io::IO, vao::GLVertexArray) = print(io, "GLVertexArray $(vao.id)")
+function Base.show(io::IO, ::MIME"text/plain", vao::GLVertexArray)
+    # show(io, vao.program)
     println(io, "GLVertexArray $(vao.id):")
-    print(io, "GLVertexArray $(vao.id) buffers: ")
-    writemime(io, MIME("text/plain"), vao.buffers)
-    println(io, "\nGLVertexArray $(vao.id) indices: ", vao.indices)
+    print(io, "buffers: ")
+    show(io, MIME("text/plain"), vao.buffers)
+    _print_indices(io, vao.indices)
 end
+_print_indices(io::IO, n::Integer) = print(io, "\nindices: ", Int64(n))
+_print_indices(io::IO, is::AbstractVector{<: Integer}) = print(io, "\nindices: ", Int64.(is))
+_print_indices(io::IO, fs) = print(io, "\nfaces: ", fs)
 
 ##################################################################################
 

GLMakie/src/GLMakie.jl

@@ -93,6 +93,8 @@ WARN_ON_LOAD[] = true
 
 function __init__()
     activate!()
+    # trigger OpenGL cleanup to avoid errors in debug mode
+    atexit(GLMakie.closeall)
 end
 
 include("precompiles.jl")

GLMakie/src/glshaders/lines.jl

@@ -29,80 +29,96 @@ 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?
+# Observables removed and adjusted to fit Compute Pipeline
+function generate_indices(ps, indices = Cuint[], valid = Float32[])
+    empty!(indices)
+    resize!(valid, length(ps))
+
+    # can't draw a line with less than 2 points so there are no indices to generate
+    # and valid is irrelevant
+    if length(ps) < 2
+        valid .= 0 # just in case random data is problematic
+        return (indices, valid)
+    end
 
-    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 continuous 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 duplicate 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 duplicate 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
+    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 continuous 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 duplicate 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 duplicate ps[1])
+                push!(indices, max(1, 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)
+    # 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
+        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
+
+    indices .-= Cuint(1)
+
+    return (indices, valid)
+end
+
+function generate_indices(positions::Observable)
+    valid_obs = Observable(Float32[]) # why does this need to be a float?
+    indices_obs = Observable(Cuint[])
 
+    on(positions, update = true) do ps
+        generate_indices(ps, indices_obs[], valid_obs[])
         notify(valid_obs)
-        return indices .- Cuint(1)
+        notify(indices_obs)
+        return
     end
 
     return indices_obs, valid_obs