From 5ef7c0fde42d27113430fab01371245f07a042b2 Mon Sep 17 00:00:00 2001 From: t-bltg Date: Mon, 4 Sep 2023 18:30:41 +0200 Subject: [PATCH 1/8] fix bb with NaNs --- src/basic_recipes/contours.jl | 5 ++++- src/layouting/boundingbox.jl | 16 ++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index ed02afc1e69..54b921a9a7b 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -280,8 +280,11 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} labels || return return broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot # drop the depth component of the bounding box for 3D + any(isnan, pt) && return Rect2f() px_pos = project(scene, apply_transform(transform_func(plot), pt, space)) - Rect2f(boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot))) + bb = unsafe_boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot)) + isfinite_rect(bb) || return Rect2f() + Rect2f(bb) end end diff --git a/src/layouting/boundingbox.jl b/src/layouting/boundingbox.jl index 18757dcb7a5..fdf53d0dacf 100644 --- a/src/layouting/boundingbox.jl +++ b/src/layouting/boundingbox.jl @@ -48,11 +48,11 @@ end _inkboundingbox(ext::GlyphExtent) = ext.ink_bounding_box -function boundingbox(glyphcollection::GlyphCollection, position::Point3f, rotation::Quaternion) - return boundingbox(glyphcollection, rotation) + position +function unsafe_boundingbox(glyphcollection::GlyphCollection, position::Point3f, rotation::Quaternion) + return unsafe_boundingbox(glyphcollection, rotation) + position end -function boundingbox(glyphcollection::GlyphCollection, rotation::Quaternion) +function unsafe_boundingbox(glyphcollection::GlyphCollection, rotation::Quaternion) if isempty(glyphcollection.glyphs) return Rect3f(Point3f(0), Vec3f(0)) end @@ -69,11 +69,10 @@ function boundingbox(glyphcollection::GlyphCollection, rotation::Quaternion) bb = union(bb, charbb) end end - !isfinite_rect(bb) && error("Invalid text boundingbox") return bb end -function boundingbox(layouts::AbstractArray{<:GlyphCollection}, positions, rotations) +function unsafe_boundingbox(layouts::AbstractArray{<:GlyphCollection}, positions, rotations) if isempty(layouts) return Rect3f((0, 0, 0), (0, 0, 0)) else @@ -85,11 +84,16 @@ function boundingbox(layouts::AbstractArray{<:GlyphCollection}, positions, rotat bb = union(bb, boundingbox(layout, pos, rot)) end end - !isfinite_rect(bb) && error("Invalid text boundingbox") return bb end end +function boundingbox(args...) + bb = unsafe_boundingbox(args...) + isfinite_rect(bb) || error("Invalid text boundingbox") + bb +end + function boundingbox(x::Text{<:Tuple{<:GlyphCollection}}) if x.space[] == x.markerspace[] pos = to_ndim(Point3f, x.position[], 0) From a35d51d0ac1ed111473105624dcbde9a7630e673 Mon Sep 17 00:00:00 2001 From: t-bltg Date: Tue, 5 Sep 2023 10:20:58 +0200 Subject: [PATCH 2/8] add test --- src/basic_recipes/contours.jl | 6 +++--- src/layouting/boundingbox.jl | 37 +++++++++++++++-------------------- test/boundingboxes.jl | 8 +++++++- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index 54b921a9a7b..1818f965663 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -277,12 +277,12 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} end bboxes = lift(labels, texts.text; ignore_equal_values=true) do labels, _ - labels || return - return broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot + labels || return Rect2f() + broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot # drop the depth component of the bounding box for 3D any(isnan, pt) && return Rect2f() px_pos = project(scene, apply_transform(transform_func(plot), pt, space)) - bb = unsafe_boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot)) + bb = unchecked_boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot)) isfinite_rect(bb) || return Rect2f() Rect2f(bb) end diff --git a/src/layouting/boundingbox.jl b/src/layouting/boundingbox.jl index fdf53d0dacf..0a9226d16b1 100644 --- a/src/layouting/boundingbox.jl +++ b/src/layouting/boundingbox.jl @@ -48,14 +48,11 @@ end _inkboundingbox(ext::GlyphExtent) = ext.ink_bounding_box -function unsafe_boundingbox(glyphcollection::GlyphCollection, position::Point3f, rotation::Quaternion) - return unsafe_boundingbox(glyphcollection, rotation) + position -end +unchecked_boundingbox(glyphcollection::GlyphCollection, position::Point3f, rotation::Quaternion) = + unchecked_boundingbox(glyphcollection, rotation) + position -function unsafe_boundingbox(glyphcollection::GlyphCollection, rotation::Quaternion) - if isempty(glyphcollection.glyphs) - return Rect3f(Point3f(0), Vec3f(0)) - end +function unchecked_boundingbox(glyphcollection::GlyphCollection, rotation::Quaternion) + isempty(glyphcollection.glyphs) && return Rect3f(Point3f(0), Vec3f(0)) glyphorigins = glyphcollection.origins glyphbbs = gl_bboxes(glyphcollection) @@ -72,24 +69,22 @@ function unsafe_boundingbox(glyphcollection::GlyphCollection, rotation::Quaterni return bb end -function unsafe_boundingbox(layouts::AbstractArray{<:GlyphCollection}, positions, rotations) - if isempty(layouts) - return Rect3f((0, 0, 0), (0, 0, 0)) - else - bb = Rect3f() - broadcast_foreach(layouts, positions, rotations) do layout, pos, rot - if !isfinite_rect(bb) - bb = boundingbox(layout, pos, rot) - else - bb = union(bb, boundingbox(layout, pos, rot)) - end +function unchecked_boundingbox(layouts::AbstractArray{<:GlyphCollection}, positions, rotations) + isempty(layouts) && return Rect3f((0, 0, 0), (0, 0, 0)) + + bb = Rect3f() + broadcast_foreach(layouts, positions, rotations) do layout, pos, rot + if !isfinite_rect(bb) + bb = boundingbox(layout, pos, rot) + else + bb = union(bb, boundingbox(layout, pos, rot)) end - return bb end + return bb end -function boundingbox(args...) - bb = unsafe_boundingbox(args...) +function boundingbox(x::Union{GlyphCollection,AbstractArray{<:GlyphCollection}}, args...) + bb = unchecked_boundingbox(x, args...) isfinite_rect(bb) || error("Invalid text boundingbox") bb end diff --git a/test/boundingboxes.jl b/test/boundingboxes.jl index 266548725d8..aef409e337e 100644 --- a/test/boundingboxes.jl +++ b/test/boundingboxes.jl @@ -55,4 +55,10 @@ bb = boundingbox(p) @test bb.origin ≈ Point3f(340, 341, 0) @test bb.widths ≈ Vec3f(32.24, 23.3, 0) -end \ No newline at end of file +end + +@testset "invalid contour bounding box" begin + a = b = 1:3 + c = [0 1 2;1 2 3;4 5 NaN] + contour(a, b, c; levels = collect(1:3), labels = true) +end From e1e090dd3de3cd6fc45c287f885f46bc03e32f90 Mon Sep 17 00:00:00 2001 From: t-bltg Date: Tue, 5 Sep 2023 10:25:35 +0200 Subject: [PATCH 3/8] update --- src/basic_recipes/contours.jl | 2 +- test/boundingboxes.jl | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index 1818f965663..c684459b8bb 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -280,7 +280,7 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} labels || return Rect2f() broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot # drop the depth component of the bounding box for 3D - any(isnan, pt) && return Rect2f() + any(isnan, pt) || return Rect2f() px_pos = project(scene, apply_transform(transform_func(plot), pt, space)) bb = unchecked_boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot)) isfinite_rect(bb) || return Rect2f() diff --git a/test/boundingboxes.jl b/test/boundingboxes.jl index aef409e337e..6f189a8cddd 100644 --- a/test/boundingboxes.jl +++ b/test/boundingboxes.jl @@ -59,6 +59,9 @@ end @testset "invalid contour bounding box" begin a = b = 1:3 - c = [0 1 2;1 2 3;4 5 NaN] - contour(a, b, c; levels = collect(1:3), labels = true) + levels = collect(1:3) + c = [0 1 2; 1 2 3; 4 5 NaN] + contour(a, b, c; levels, labels = true) + c = [0 1 2; 1 2 3; 4 5 Inf] + contour(a, b, c; levels, labels = true) end From 503082c853e9a1908be4c501adecbd9a34c15d10 Mon Sep 17 00:00:00 2001 From: t-bltg Date: Wed, 6 Sep 2023 17:15:25 +0200 Subject: [PATCH 4/8] update --- src/basic_recipes/contours.jl | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index c684459b8bb..86e8c5d48a9 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -62,6 +62,7 @@ angle(p1::Union{Vec2f,Point2f}, p2::Union{Vec2f,Point2f})::Float32 = function label_info(lev, vertices, col) mid = ceil(Int, 0.5f0 * length(vertices)) + # take 3 pts around half segment pts = (vertices[max(firstindex(vertices), mid - 1)], vertices[mid], vertices[min(mid + 1, lastindex(vertices))]) ( lev, @@ -194,11 +195,9 @@ end function has_changed(old_args, new_args) length(old_args) === length(new_args) || return true for (old, new) in zip(old_args, new_args) - if old != new - return true - end + old != new && return true end - return false + false end function plot!(plot::T) where T <: Union{Contour, Contour3d} @@ -228,8 +227,7 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} # We need to copy, since the values may get mutated in place if has_changed(old_values, args) old_values = map(copy, args) - _points, _colors, _lev_pos_col = contourlines(args..., T) - points[] = _points; colors[] = _colors; lev_pos_col[] = _lev_pos_col + points[], colors[], lev_pos_col[] = contourlines(args..., T) return end end @@ -270,7 +268,10 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} push!(col, labelcolor === nothing ? color : to_color(labelcolor)) push!(rot, rot_from_vert) push!(lbl, labelformatter(lev)) - push!(pos, p1) + p = p2 # try to position label around center + isnan(p) && (p = p1) + isnan(p) && (p = p3) + push!(pos, p) end notify(texts.text) return @@ -280,7 +281,7 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} labels || return Rect2f() broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot # drop the depth component of the bounding box for 3D - any(isnan, pt) || return Rect2f() + isnan(pt) || return Rect2f() px_pos = project(scene, apply_transform(transform_func(plot), pt, space)) bb = unchecked_boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot)) isfinite_rect(bb) || return Rect2f() From 82ac67c9b57bb85061af69f576f0b7f7aa03066d Mon Sep 17 00:00:00 2001 From: t-bltg Date: Fri, 8 Sep 2023 10:25:26 +0200 Subject: [PATCH 5/8] fix regression --- src/basic_recipes/contours.jl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index 86e8c5d48a9..dfee3596449 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -278,10 +278,9 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} end bboxes = lift(labels, texts.text; ignore_equal_values=true) do labels, _ - labels || return Rect2f() + labels || return broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot # drop the depth component of the bounding box for 3D - isnan(pt) || return Rect2f() px_pos = project(scene, apply_transform(transform_func(plot), pt, space)) bb = unchecked_boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot)) isfinite_rect(bb) || return Rect2f() From e1f461c2b2a34fc1a1ceb226dbb0009718045fb4 Mon Sep 17 00:00:00 2001 From: t-bltg Date: Fri, 8 Sep 2023 22:32:38 +0200 Subject: [PATCH 6/8] add heuristic to turn off masking --- src/basic_recipes/contours.jl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index dfee3596449..532d5a28581 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -290,6 +290,8 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} masked_lines = lift(labels, bboxes, points) do labels, bboxes, segments labels || return segments + # simple heuristic to turn off masking segments (≈ less than 10 pts per contour) + count(isnan, segments) > length(segments) / 10 && return segments n = 1 bb = bboxes[n] nlab = length(bboxes) From a7ce76736e169f1699c70ecd60cd57b29d78dba3 Mon Sep 17 00:00:00 2001 From: t-bltg Date: Sat, 23 Dec 2023 14:36:23 +0100 Subject: [PATCH 7/8] fix glyph with NaN --- CairoMakie/src/primitives.jl | 113 +++++++++++++---------------------- 1 file changed, 43 insertions(+), 70 deletions(-) diff --git a/CairoMakie/src/primitives.jl b/CairoMakie/src/primitives.jl index 3c0f45fc64c..f00f3577f58 100644 --- a/CairoMakie/src/primitives.jl +++ b/CairoMakie/src/primitives.jl @@ -86,24 +86,21 @@ function draw_bezierpath_lines(ctx, bezierpath::BezierPath, scene, color, space, Cairo.set_source_rgba(ctx, rgbatuple(color)...) Cairo.set_line_width(ctx, linewidth) Cairo.stroke(ctx) - return + nothing end -function project_command(m::MoveTo, scene, space, model) +project_command(m::MoveTo, scene, space, model) = MoveTo(project_position(scene, space, m.p, model)) -end -function project_command(l::LineTo, scene, space, model) +project_command(l::LineTo, scene, space, model) = LineTo(project_position(scene, space, l.p, model)) -end -function project_command(c::CurveTo, scene, space, model) +project_command(c::CurveTo, scene, space, model) = CurveTo( project_position(scene, space, c.c1, model), project_position(scene, space, c.c2, model), project_position(scene, space, c.p, model), ) -end project_command(c::ClosePath, scene, space, model) = c @@ -130,9 +127,7 @@ function draw_single(primitive::Lines, ctx, positions) end function draw_single(primitive::LineSegments, ctx, positions) - @assert iseven(length(positions)) - @inbounds for i in 1:2:length(positions)-1 p1 = positions[i] p2 = positions[i+1] @@ -150,14 +145,12 @@ function draw_single(primitive::LineSegments, ctx, positions) end # if linewidth is not an array -function draw_multi(primitive, ctx, positions, colors::AbstractArray, linewidth, dash) +draw_multi(primitive, ctx, positions, colors::AbstractArray, linewidth, dash) = draw_multi(primitive, ctx, positions, colors, [linewidth for c in colors], dash) -end # if color is not an array -function draw_multi(primitive, ctx, positions, color, linewidths::AbstractArray, dash) +draw_multi(primitive, ctx, positions, color, linewidths::AbstractArray, dash) = draw_multi(primitive, ctx, positions, [color for l in linewidths], linewidths, dash) -end function draw_multi(primitive::LineSegments, ctx, positions, colors::AbstractArray, linewidths::AbstractArray, dash) @assert iseven(length(positions)) @@ -175,9 +168,8 @@ function draw_multi(primitive::LineSegments, ctx, positions, colors::AbstractArr Cairo.line_to(ctx, positions[i+1]...) Cairo.set_line_width(ctx, linewidths[i]) - !isnothing(dash) && Cairo.set_dash(ctx, dash .* linewidths[i]) - c1 = colors[i] - c2 = colors[i+1] + isnothing(dash) || Cairo.set_dash(ctx, dash .* linewidths[i]) + c1, c2 = colors[i], colors[i+1] # we can avoid the more expensive gradient if the colors are the same # this happens if one color was given for each segment if c1 == c2 @@ -204,14 +196,10 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin prev_nan = isnan(prev_position) prev_continued = false - if !prev_nan - # first is not nan, move_to - Cairo.move_to(ctx, positions[begin]...) - else - # first is nan, do nothing - end + # first is not nan, move_to + prev_nan || Cairo.move_to(ctx, positions[begin]...) - for i in eachindex(positions)[2:end] + for i in eachindex(positions)[begin+1:end] this_position = positions[i] this_color = colors[i] this_nan = isnan(this_position) @@ -221,7 +209,7 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin if prev_continued # and this is prev_continued, so set source and stroke to finish previous line Cairo.set_line_width(ctx, this_linewidth) - !isnothing(dash) && Cairo.set_dash(ctx, dash .* 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)) Cairo.stroke(ctx) else @@ -230,17 +218,14 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin end if prev_nan # previous was nan - if !this_nan - # but this is not nan, so move to this position + if this_nan # this is also nan, do nothing + else # this is not nan, so move to this position Cairo.move_to(ctx, this_position...) - else - # and this is also nan, do nothing end else if this_color == prev_color # this color is like the previous - if !this_nan - # and this is not nan, so line_to and set prev_continued + if !this_nan # this is not nan, so line_to and set prev_continued this_linewidth != prev_linewidth && error("Encountered two different linewidth values $prev_linewidth and $this_linewidth in `lines` at index $(i-1). Different linewidths in one line are only permitted in CairoMakie when separated by a NaN point.") Cairo.line_to(ctx, this_position...) prev_continued = true @@ -248,23 +233,21 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin if i == lastindex(positions) # this is the last element so stroke this Cairo.set_line_width(ctx, this_linewidth) - !isnothing(dash) && Cairo.set_dash(ctx, dash .* 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)) Cairo.stroke(ctx) end - else - # but this is nan, so do nothing end else prev_continued = false - if !this_nan + if !this_nan # this is not nan, so line_to and set prev_continued this_linewidth != prev_linewidth && error("Encountered two different linewidth values $prev_linewidth and $this_linewidth in `lines` at index $(i-1). Different linewidths in one line are only permitted in CairoMakie when separated by a NaN point.") # this is not nan # and this color is different than the previous, so move_to prev and line_to this # create gradient pattern and stroke Cairo.move_to(ctx, prev_position...) Cairo.line_to(ctx, this_position...) - !isnothing(dash) && Cairo.set_dash(ctx, dash .* this_linewidth) + isnothing(dash) || Cairo.set_dash(ctx, dash .* this_linewidth) Cairo.set_line_width(ctx, this_linewidth) pat = Cairo.pattern_create_linear(prev_position..., this_position...) @@ -275,8 +258,6 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin Cairo.destroy(pat) Cairo.move_to(ctx, this_position...) - else - # this is nan, do nothing end end end @@ -308,9 +289,8 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Scat space = primitive.space[] transfunc = Makie.transform_func(primitive) - return draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokecolor, strokewidth, marker, - marker_offset, rotations, model, positions, size_model, font, markerspace, - space) + draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokecolor, strokewidth, marker, + marker_offset, rotations, model, positions, size_model, font, markerspace, space) end function draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokecolor, strokewidth, marker, marker_offset, rotations, model, positions, size_model, font, markerspace, space) @@ -339,7 +319,7 @@ function draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokeco end Cairo.restore(ctx) end - return + nothing end function draw_marker(ctx, marker::Char, font, pos, scale, strokecolor, strokewidth, marker_offset, rotation) @@ -520,11 +500,9 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Text scene, ctx, position, glyph_collection, remove_billboard(rotation), model, space, markerspace, offset, primitive.transformation, transform_marker ) - nothing end - function draw_glyph_collection( scene, ctx, positions, glyph_collections::AbstractArray, rotation, model::Mat, space, markerspace, offset, transformation, transform_marker @@ -587,6 +565,11 @@ function draw_glyph_collection( # offsets and scale apply in markerspace gp3 = glyph_pos[Vec(1, 2, 3)] ./ glyph_pos[4] .+ model33 * (glyphoffset .+ p3_offset) + if any(isnan, gp3) + Cairo.restore(ctx) + return + end + scale3 = scale isa Number ? Point3f(scale, scale, 0) : to_ndim(Point3f, scale, 0) # the CairoMatrix is found by transforming the right and up vector @@ -632,7 +615,7 @@ function draw_glyph_collection( end Cairo.restore(ctx) - return + nothing end ################################################################################ @@ -647,15 +630,15 @@ If not, returns array unchanged. function regularly_spaced_array_to_range(arr) diffs = unique!(sort!(diff(arr))) step = sum(diffs) ./ length(diffs) - if all(x-> x ≈ step, diffs) + return if all(x-> x ≈ step, diffs) m, M = extrema(arr) if step < zero(step) m, M = M, m end # don't use stop=M, since that may not include M - return range(m; step=step, length=length(arr)) + range(m; step=step, length=length(arr)) else - return arr + arr end end @@ -673,19 +656,15 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Unio ctx = screen.context image = primitive[3][] xs, ys = primitive[1][], primitive[2][] - if !(xs isa AbstractVector) - l, r = extrema(xs) - N = size(image, 1) - xs = range(l, r, length = N+1) + xs = if xs isa AbstractVector + regularly_spaced_array_to_range(xs) else - xs = regularly_spaced_array_to_range(xs) + range(extrema(xs)..., length = size(image, 1) + 1) end - if !(ys isa AbstractVector) - l, r = extrema(ys) - N = size(image, 2) - ys = range(l, r, length = N+1) + ys = if ys isa AbstractVector + regularly_spaced_array_to_range(ys) else - ys = regularly_spaced_array_to_range(ys) + range(extrema(ys)..., length = size(image, 2) + 1) end model = primitive.model[]::Mat4f interpolate = to_value(primitive.interpolate) @@ -712,12 +691,8 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Unio end if interpolate - if !regular_grid - error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-regular grid is not supported right now.") - end - if !identity_transform - error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-identity transform is not supported right now.") - end + regular_grid || error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-regular grid is not supported right now.") + identity_transform || error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-identity transform is not supported right now.") end imsize = ((first(xs), last(xs)), (first(ys), last(ys))) @@ -817,7 +792,7 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Maki end draw_mesh3D(scene, screen, primitive, mesh) end - return nothing + nothing end function draw_mesh2D(scene, screen, @nospecialize(plot), @nospecialize(mesh)) @@ -829,7 +804,7 @@ function draw_mesh2D(scene, screen, @nospecialize(plot), @nospecialize(mesh)) cols = per_face_colors(color, nothing, fs, nothing, uv) space = to_value(get(plot, :space, :data))::Symbol transform_func = Makie.transform_func(plot) - return draw_mesh2D(scene, screen, cols, space, transform_func, vs, fs, model) + draw_mesh2D(scene, screen, cols, space, transform_func, vs, fs, model) end function draw_mesh2D(scene, screen, per_face_cols, space::Symbol, transform_func, @@ -866,7 +841,6 @@ function draw_mesh2D(scene, screen, per_face_cols, space::Symbol, transform_func Cairo.paint(ctx) Cairo.destroy(pattern) end - return nothing end function average_z(positions, face) @@ -982,7 +956,7 @@ function draw_mesh3D( zorder = filter(i -> any(last.(ns[meshfaces[i]]) .> faceculling), zorder) draw_pattern(ctx, zorder, shading, meshfaces, ts, per_face_col, ns, vs, lightdirection, light_color, shininess, diffuse, ambient, specular) - return + nothing end function _calculate_shaded_vertexcolors(N, v, c, lightdir, light_color, ambient, diffuse, specular, shininess) @@ -1070,7 +1044,7 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Maki end draw_mesh3D(scene, screen, primitive, mesh) primitive[:color] = old - return nothing + nothing end @@ -1123,6 +1097,5 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Maki rotation = rotation ) end - - return nothing + nothing end From 316040d959ccc4de728bab2a1c8860f1a26ec740 Mon Sep 17 00:00:00 2001 From: t-bltg Date: Sat, 23 Dec 2023 17:54:19 +0100 Subject: [PATCH 8/8] cleanup --- CairoMakie/src/primitives.jl | 105 ++++++++++++++++++++++------------ src/basic_recipes/contours.jl | 2 +- 2 files changed, 69 insertions(+), 38 deletions(-) diff --git a/CairoMakie/src/primitives.jl b/CairoMakie/src/primitives.jl index f00f3577f58..a37415d761b 100644 --- a/CairoMakie/src/primitives.jl +++ b/CairoMakie/src/primitives.jl @@ -86,21 +86,24 @@ function draw_bezierpath_lines(ctx, bezierpath::BezierPath, scene, color, space, Cairo.set_source_rgba(ctx, rgbatuple(color)...) Cairo.set_line_width(ctx, linewidth) Cairo.stroke(ctx) - nothing + return end -project_command(m::MoveTo, scene, space, model) = +function project_command(m::MoveTo, scene, space, model) MoveTo(project_position(scene, space, m.p, model)) +end -project_command(l::LineTo, scene, space, model) = +function project_command(l::LineTo, scene, space, model) LineTo(project_position(scene, space, l.p, model)) +end -project_command(c::CurveTo, scene, space, model) = +function project_command(c::CurveTo, scene, space, model) CurveTo( project_position(scene, space, c.c1, model), project_position(scene, space, c.c2, model), project_position(scene, space, c.p, model), ) +end project_command(c::ClosePath, scene, space, model) = c @@ -127,7 +130,9 @@ function draw_single(primitive::Lines, ctx, positions) end function draw_single(primitive::LineSegments, ctx, positions) + @assert iseven(length(positions)) + @inbounds for i in 1:2:length(positions)-1 p1 = positions[i] p2 = positions[i+1] @@ -145,12 +150,14 @@ function draw_single(primitive::LineSegments, ctx, positions) end # if linewidth is not an array -draw_multi(primitive, ctx, positions, colors::AbstractArray, linewidth, dash) = +function draw_multi(primitive, ctx, positions, colors::AbstractArray, linewidth, dash) draw_multi(primitive, ctx, positions, colors, [linewidth for c in colors], dash) +end # if color is not an array -draw_multi(primitive, ctx, positions, color, linewidths::AbstractArray, dash) = +function draw_multi(primitive, ctx, positions, color, linewidths::AbstractArray, dash) draw_multi(primitive, ctx, positions, [color for l in linewidths], linewidths, dash) +end function draw_multi(primitive::LineSegments, ctx, positions, colors::AbstractArray, linewidths::AbstractArray, dash) @assert iseven(length(positions)) @@ -168,8 +175,9 @@ function draw_multi(primitive::LineSegments, ctx, positions, colors::AbstractArr Cairo.line_to(ctx, positions[i+1]...) Cairo.set_line_width(ctx, linewidths[i]) - isnothing(dash) || Cairo.set_dash(ctx, dash .* linewidths[i]) - c1, c2 = colors[i], colors[i+1] + !isnothing(dash) && Cairo.set_dash(ctx, dash .* linewidths[i]) + c1 = colors[i] + c2 = colors[i+1] # we can avoid the more expensive gradient if the colors are the same # this happens if one color was given for each segment if c1 == c2 @@ -196,8 +204,12 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin prev_nan = isnan(prev_position) prev_continued = false - # first is not nan, move_to - prev_nan || Cairo.move_to(ctx, positions[begin]...) + if !prev_nan + # first is not nan, move_to + Cairo.move_to(ctx, positions[begin]...) + else + # first is nan, do nothing + end for i in eachindex(positions)[begin+1:end] this_position = positions[i] @@ -209,7 +221,7 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin if prev_continued # and this is prev_continued, so set source and stroke to finish previous line Cairo.set_line_width(ctx, this_linewidth) - isnothing(dash) || Cairo.set_dash(ctx, dash .* 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)) Cairo.stroke(ctx) else @@ -218,14 +230,17 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin end if prev_nan # previous was nan - if this_nan # this is also nan, do nothing - else # this is not nan, so move to this position + if !this_nan + # but this is not nan, so move to this position Cairo.move_to(ctx, this_position...) + else + # and this is also nan, do nothing end else if this_color == prev_color # this color is like the previous - if !this_nan # this is not nan, so line_to and set prev_continued + if !this_nan + # and this is not nan, so line_to and set prev_continued this_linewidth != prev_linewidth && error("Encountered two different linewidth values $prev_linewidth and $this_linewidth in `lines` at index $(i-1). Different linewidths in one line are only permitted in CairoMakie when separated by a NaN point.") Cairo.line_to(ctx, this_position...) prev_continued = true @@ -233,21 +248,23 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin if i == lastindex(positions) # this is the last element so stroke this Cairo.set_line_width(ctx, this_linewidth) - isnothing(dash) || Cairo.set_dash(ctx, dash .* 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)) Cairo.stroke(ctx) end + else + # but this is nan, so do nothing end else prev_continued = false - if !this_nan # this is not nan, so line_to and set prev_continued + if !this_nan this_linewidth != prev_linewidth && error("Encountered two different linewidth values $prev_linewidth and $this_linewidth in `lines` at index $(i-1). Different linewidths in one line are only permitted in CairoMakie when separated by a NaN point.") # this is not nan # and this color is different than the previous, so move_to prev and line_to this # create gradient pattern and stroke Cairo.move_to(ctx, prev_position...) Cairo.line_to(ctx, this_position...) - isnothing(dash) || Cairo.set_dash(ctx, dash .* this_linewidth) + !isnothing(dash) && Cairo.set_dash(ctx, dash .* this_linewidth) Cairo.set_line_width(ctx, this_linewidth) pat = Cairo.pattern_create_linear(prev_position..., this_position...) @@ -258,6 +275,8 @@ function draw_multi(primitive::Lines, ctx, positions, colors::AbstractArray, lin Cairo.destroy(pat) Cairo.move_to(ctx, this_position...) + else + # this is nan, do nothing end end end @@ -289,8 +308,9 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Scat space = primitive.space[] transfunc = Makie.transform_func(primitive) - draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokecolor, strokewidth, marker, - marker_offset, rotations, model, positions, size_model, font, markerspace, space) + return draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokecolor, strokewidth, marker, + marker_offset, rotations, model, positions, size_model, font, markerspace, + space) end function draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokecolor, strokewidth, marker, marker_offset, rotations, model, positions, size_model, font, markerspace, space) @@ -319,7 +339,7 @@ function draw_atomic_scatter(scene, ctx, transfunc, colors, markersize, strokeco end Cairo.restore(ctx) end - nothing + return end function draw_marker(ctx, marker::Char, font, pos, scale, strokecolor, strokewidth, marker_offset, rotation) @@ -500,6 +520,7 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Text scene, ctx, position, glyph_collection, remove_billboard(rotation), model, space, markerspace, offset, primitive.transformation, transform_marker ) + nothing end @@ -615,7 +636,7 @@ function draw_glyph_collection( end Cairo.restore(ctx) - nothing + return end ################################################################################ @@ -630,15 +651,15 @@ If not, returns array unchanged. function regularly_spaced_array_to_range(arr) diffs = unique!(sort!(diff(arr))) step = sum(diffs) ./ length(diffs) - return if all(x-> x ≈ step, diffs) + if all(x-> x ≈ step, diffs) m, M = extrema(arr) if step < zero(step) m, M = M, m end # don't use stop=M, since that may not include M - range(m; step=step, length=length(arr)) + return range(m; step=step, length=length(arr)) else - arr + return arr end end @@ -656,15 +677,19 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Unio ctx = screen.context image = primitive[3][] xs, ys = primitive[1][], primitive[2][] - xs = if xs isa AbstractVector - regularly_spaced_array_to_range(xs) + if !(xs isa AbstractVector) + l, r = extrema(xs) + N = size(image, 1) + xs = range(l, r, length = N+1) else - range(extrema(xs)..., length = size(image, 1) + 1) + xs = regularly_spaced_array_to_range(xs) end - ys = if ys isa AbstractVector - regularly_spaced_array_to_range(ys) + if !(ys isa AbstractVector) + l, r = extrema(ys) + N = size(image, 2) + ys = range(l, r, length = N+1) else - range(extrema(ys)..., length = size(image, 2) + 1) + ys = regularly_spaced_array_to_range(ys) end model = primitive.model[]::Mat4f interpolate = to_value(primitive.interpolate) @@ -691,8 +716,12 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Unio end if interpolate - regular_grid || error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-regular grid is not supported right now.") - identity_transform || error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-identity transform is not supported right now.") + if !regular_grid + error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-regular grid is not supported right now.") + end + if !identity_transform + error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-identity transform is not supported right now.") + end end imsize = ((first(xs), last(xs)), (first(ys), last(ys))) @@ -792,7 +821,7 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Maki end draw_mesh3D(scene, screen, primitive, mesh) end - nothing + return nothing end function draw_mesh2D(scene, screen, @nospecialize(plot), @nospecialize(mesh)) @@ -804,7 +833,7 @@ function draw_mesh2D(scene, screen, @nospecialize(plot), @nospecialize(mesh)) cols = per_face_colors(color, nothing, fs, nothing, uv) space = to_value(get(plot, :space, :data))::Symbol transform_func = Makie.transform_func(plot) - draw_mesh2D(scene, screen, cols, space, transform_func, vs, fs, model) + return draw_mesh2D(scene, screen, cols, space, transform_func, vs, fs, model) end function draw_mesh2D(scene, screen, per_face_cols, space::Symbol, transform_func, @@ -841,6 +870,7 @@ function draw_mesh2D(scene, screen, per_face_cols, space::Symbol, transform_func Cairo.paint(ctx) Cairo.destroy(pattern) end + return nothing end function average_z(positions, face) @@ -956,7 +986,7 @@ function draw_mesh3D( zorder = filter(i -> any(last.(ns[meshfaces[i]]) .> faceculling), zorder) draw_pattern(ctx, zorder, shading, meshfaces, ts, per_face_col, ns, vs, lightdirection, light_color, shininess, diffuse, ambient, specular) - nothing + return end function _calculate_shaded_vertexcolors(N, v, c, lightdir, light_color, ambient, diffuse, specular, shininess) @@ -1044,7 +1074,7 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Maki end draw_mesh3D(scene, screen, primitive, mesh) primitive[:color] = old - nothing + return nothing end @@ -1097,5 +1127,6 @@ function draw_atomic(scene::Scene, screen::Screen, @nospecialize(primitive::Maki rotation = rotation ) end - nothing + + return nothing end diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index 413198a2506..861b9e249ed 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -283,7 +283,7 @@ function plot!(plot::T) where T <: Union{Contour, Contour3d} bboxes = lift(labels, texts.text; ignore_equal_values=true) do labels, _ labels || return - broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot + return broadcast(texts.plots[1][1].val, texts.positions.val, texts.rotation.val) do gc, pt, rot # drop the depth component of the bounding box for 3D px_pos = project(scene, apply_transform(transform_func(plot), pt, space)) bb = unchecked_boundingbox(gc, to_ndim(Point3f, px_pos, 0f0), to_rotation(rot))