diff --git a/CairoMakie/src/infrastructure.jl b/CairoMakie/src/infrastructure.jl index 179750974d3..6a036a84727 100644 --- a/CairoMakie/src/infrastructure.jl +++ b/CairoMakie/src/infrastructure.jl @@ -11,7 +11,7 @@ function cairo_draw(screen::Screen, scene::Scene) Cairo.save(screen.context) draw_background(screen, scene) - allplots = get_all_plots(scene) + allplots = Makie.collect_atomic_plots(scene; is_atomic_plot = is_cairomakie_atomic_plot) zvals = Makie.zvalue2d.(allplots) permute!(allplots, sortperm(zvals)) @@ -23,10 +23,12 @@ function cairo_draw(screen::Screen, scene::Scene) Cairo.save(screen.context) for p in allplots - to_value(get(p, :visible, true)) || continue + check_parent_plots(p) do plot + to_value(get(plot, :visible, true)) + end || continue # only prepare for scene when it changes # this should reduce the number of unnecessary clipping masks etc. - pparent = p.parent::Scene + pparent = Makie.parent_scene(p) pparent.visible[] || continue if pparent != last_scene Cairo.restore(screen.context) @@ -36,8 +38,8 @@ function cairo_draw(screen::Screen, scene::Scene) end Cairo.save(screen.context) - # When a plot is too large to save with a reasonable file size on a vector backend, - # the user can choose to rasterize it when plotting to vector backends, by using the + # When a plot is too large to save with a reasonable file size on a vector backend, + # the user can choose to rasterize it when plotting to vector backends, by using the # `rasterize` keyword argument. This can be set to a Bool or an Int which describes # the density of rasterization (in terms of a direct scaling factor.) # TODO: In future, this can also be set to a Tuple{Module, Int} which describes @@ -54,12 +56,33 @@ function cairo_draw(screen::Screen, scene::Scene) return end -function get_all_plots(scene, plots = AbstractPlot[]) - append!(plots, scene.plots) - for c in scene.children - get_all_plots(c, plots) +""" + is_cairomakie_atomic_plot(plot::Combined)::Bool + +Returns whether the plot is considered atomic for the CairoMakie backend. +This is overridden for `Poly`, `Band`, and `Tricontourf` so we can apply +CairoMakie can treat them as atomic plots and render them directly. + +Plots with children are by default recursed into. This can be overridden +by defining specific dispatches for `is_cairomakie_atomic_plot` for a given plot type. +""" +is_cairomakie_atomic_plot(plot::Combined) = isempty(plot.plots) || to_value(get(plot, :rasterize, false)) != false + +""" + check_parent_plots(f, plot::Combined)::Bool +Returns whether the plot's parent tree satisfies the predicate `f`. +`f` must return a `Bool` and take a plot as its only argument. +""" +function check_parent_plots(f, plot::Combined) + if f(plot) + check_parent_plots(f, parent(plot)) + else + return false end - plots +end + +function check_parent_plots(f, scene::Scene) + return true end function prepare_for_scene(screen::Screen, scene::Scene) diff --git a/CairoMakie/src/overrides.jl b/CairoMakie/src/overrides.jl index d7301ddc8d9..ee6d97a97c2 100644 --- a/CairoMakie/src/overrides.jl +++ b/CairoMakie/src/overrides.jl @@ -12,6 +12,10 @@ function draw_plot(scene::Scene, screen::Screen, poly::Poly) draw_poly(scene, screen, poly, to_value.(poly.input_args)...) end +# Override `is_cairomakie_atomic_plot` to allow `poly` to remain a unit, +# instead of auto-decomposing in lines and mesh. +is_cairomakie_atomic_plot(plot::Poly) = true + """ Fallback method for args without special treatment. """ @@ -182,6 +186,12 @@ function draw_plot(scene::Scene, screen::Screen, nothing end +# Override `is_cairomakie_atomic_plot` to allow this dispatch of `band` to remain a unit, +# instead of auto-decomposing in lines and mesh. +function is_cairomakie_atomic_plot(plot::Band{<:Tuple{<:AbstractVector{<:Point2},<:AbstractVector{<:Point2}}}) + return true +end + ################################################################################# # Tricontourf # # Tricontourf creates many disjoint polygons that are adjacent and form contour # @@ -214,3 +224,9 @@ function draw_plot(scene::Scene, screen::Screen, tric::Tricontourf) return end + +# Override `is_cairomakie_atomic_plot` to allow `Tricontourf` to remain a unit, +# instead of auto-decomposing in lines and mesh. +function is_cairomakie_atomic_plot(plot::Tricontourf) + return true +end diff --git a/GLMakie/src/screen.jl b/GLMakie/src/screen.jl index 1e364ffdaa5..0bf5028cb50 100644 --- a/GLMakie/src/screen.jl +++ b/GLMakie/src/screen.jl @@ -518,7 +518,7 @@ end function Base.delete!(screen::Screen, scene::Scene, plot::AbstractPlot) if !isempty(plot.plots) # this plot consists of children, so we flatten it and delete the children instead - for cplot in Makie.flatten_plots(plot) + for cplot in Makie.collect_atomic_plots(plot) delete!(screen, scene, cplot) end else @@ -919,7 +919,7 @@ function renderloop(screen) end function plot2robjs(screen::Screen, plot) - plots = Makie.flatten_plots(plot) + plots = Makie.collect_atomic_plots(plot) return map(x-> screen.cache[objectid(x)], plots) end diff --git a/NEWS.md b/NEWS.md index b1d1f01ba91..4c54037a4ff 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,10 @@ ## master +- Deprecated `flatten_plots` in favor of `collect_atomic_plots`. Using the new `collect_atomic_plots` fixed a bug in CairoMakie where the z-level of plots within recipes was not respected. [#2793](https://github.com/MakieOrg/Makie.jl/pull/2793) +- Fixed incorrect line depth in GLMakie [#2843](https://github.com/MakieOrg/Makie.jl/pull/2843) +- Fixed incorrect line alpha in dense lines in GLMakie [#2843](https://github.com/MakieOrg/Makie.jl/pull/2843) +- Fixed DataInspector interaction with transformations [#3002](https://github.com/MakieOrg/Makie.jl/pull/3002) - Added option `WGLMakie.activate!(resize_to_body=true)`, to make plots resize to the VSCode plotpane. Resizes to the HTML body element, so may work outside VSCode [#3044](https://github.com/MakieOrg/Makie.jl/pull/3044), [#3042](https://github.com/MakieOrg/Makie.jl/pull/3042). - Fixed DataInspector interaction with transformations [#3002](https://github.com/MakieOrg/Makie.jl/pull/3002). - Fix incomplete stroke with some Bezier markers in CairoMakie and blurry strokes in GLMakie [#2961](https://github.com/MakieOrg/Makie.jl/pull/2961) @@ -11,7 +15,6 @@ - Updated `bracket` when the screen is resized or transformations change [#3012](https://github.com/MakieOrg/Makie.jl/pull/3012). - Added auto-resizing functionality to WGLMakie plot figures [#3042](https://github.com/MakieOrg/Makie.jl/pull/3042) - ## v0.19.6 - Fixed broken AA for lines with strongly varying linewidth [#2953](https://github.com/MakieOrg/Makie.jl/pull/2953). diff --git a/ReferenceTests/src/tests/examples2d.jl b/ReferenceTests/src/tests/examples2d.jl index 6ad7b284f82..9166a08f377 100644 --- a/ReferenceTests/src/tests/examples2d.jl +++ b/ReferenceTests/src/tests/examples2d.jl @@ -1047,3 +1047,28 @@ end end f end + +@reference_test "Z-translation within a recipe" begin + # This is testing whether backends respect the + # z-level of plots within recipes in 2d. + # Ideally, the output of this test + # would be a blue line with red scatter markers. + # However, if a backend does not correctly pick up on translations, + # then this will be drawn in the drawing order, and blue + # will completely obscure red. + + # It seems like we can't define recipes in `@reference_test` yet, + # so we'll have to fake a recipe's structure. + + fig = Figure(resolution = (600, 600)) + # Create a recipe plot + ax, plot_top = heatmap(fig[1, 1], randn(10, 10)) + # Plot some recipes at the level below the contour + scatterlineplot_1 = scatterlines!(plot_top, 1:10, 1:10; linewidth = 20, markersize = 20, color = :red) + scatterlineplot_2 = scatterlines!(plot_top, 1:10, 1:10; linewidth = 20, markersize = 30, color = :blue) + # Translate the lowest level plots (scatters) + translate!(scatterlineplot_1.plots[2], 0, 0, 1) + translate!(scatterlineplot_2.plots[2], 0, 0, -1) + # Display + fig +end diff --git a/WGLMakie/src/display.jl b/WGLMakie/src/display.jl index ee24cbf77e3..05a6a625da1 100644 --- a/WGLMakie/src/display.jl +++ b/WGLMakie/src/display.jl @@ -343,7 +343,7 @@ const DISABLE_JS_FINALZING = Base.RefValue(false) const DELETE_QUEUE = LockfreeQueue{Tuple{Screen,String,Vector{String}}}(delete_plot!) function Base.delete!(screen::Screen, scene::Scene, plot::Combined) - atomics = Makie.flatten_plots(plot) # delete all atomics + atomics = Makie.collect_atomic_plots(plot) # delete all atomics # only queue atomics to actually delete on js if !DISABLE_JS_FINALZING[] push!(DELETE_QUEUE, (screen, js_uuid(scene), js_uuid.(atomics))) diff --git a/WGLMakie/src/picking.jl b/WGLMakie/src/picking.jl index 1bb9e64fb82..4c5589ffe20 100644 --- a/WGLMakie/src/picking.jl +++ b/WGLMakie/src/picking.jl @@ -17,7 +17,7 @@ function pick_native(screen::Screen, rect::Rect2i) if isempty(matrix) return empty else - all_children = Makie.flatten_plots(scene) + all_children = Makie.collect_atomic_plots(scene) lookup = Dict(Pair.(js_uuid.(all_children), all_children)) return map(matrix) do (uuid, index) !haskey(lookup, uuid) && return (nothing, 0) @@ -27,7 +27,7 @@ function pick_native(screen::Screen, rect::Rect2i) end function plot_lookup(scene::Scene) - all_plots = Makie.flatten_plots(scene) + all_plots = Makie.collect_atomic_plots(scene) return Dict(Pair.(js_uuid.(all_plots), all_plots)) end @@ -107,7 +107,7 @@ struct ToolTip if isnothing(plots) plots = scene.plots end - all_plots = js_uuid.(filter!(x-> x.inspectable[], Makie.flatten_plots(plots))) + all_plots = js_uuid.(filter!(x-> x.inspectable[], Makie.collect_atomic_plots(plots))) new(scene, callback, all_plots) end end diff --git a/WGLMakie/src/three_plot.jl b/WGLMakie/src/three_plot.jl index eff03a5515b..94f18f2e9c4 100644 --- a/WGLMakie/src/three_plot.jl +++ b/WGLMakie/src/three_plot.jl @@ -6,7 +6,7 @@ js_uuid(object) = string(objectid(object)) function all_plots_scenes(scene::Scene; scene_uuids=String[], plot_uuids=String[]) push!(scene_uuids, js_uuid(scene)) for plot in scene.plots - append!(plot_uuids, (js_uuid(p) for p in Makie.flatten_plots(plot))) + append!(plot_uuids, (js_uuid(p) for p in Makie.collect_atomic_plots(plot))) end for child in scene.children all_plots_scenes(child, plot_uuids=plot_uuids, scene_uuids=scene_uuids) @@ -15,7 +15,7 @@ function all_plots_scenes(scene::Scene; scene_uuids=String[], plot_uuids=String[ end function JSServe.print_js_code(io::IO, plot::AbstractPlot, context::JSServe.JSSourceContext) - uuids = js_uuid.(Makie.flatten_plots(plot)) + uuids = js_uuid.(Makie.collect_atomic_plots(plot)) # This is a bit more complicated then it has to be, since evaljs / on_document_load # isn't guaranteed to run after plot initialization in an App... So, if we don't find any plots, # we have to check again after inserting new plots diff --git a/src/interaction/interactive_api.jl b/src/interaction/interactive_api.jl index 93aff3479d6..7ea6243d497 100644 --- a/src/interaction/interactive_api.jl +++ b/src/interaction/interactive_api.jl @@ -10,7 +10,7 @@ Returns true if the mouse currently hovers any of `plots`. mouseover(x, plots::AbstractPlot...) = mouseover(get_scene(x), plots...) function mouseover(scene::Scene, plots::AbstractPlot...) p, idx = pick(scene) - return p in flatten_plots(plots) + return p in collect_atomic_plots(plots) end """ @@ -22,7 +22,7 @@ hovered element """ onpick(f, x, plots::AbstractPlot...; range=1) = onpick(f, get_scene(x), plots..., range = range) function onpick(f, scene::Scene, plots::AbstractPlot...; range=1) - fplots = flatten_plots(plots) + fplots = collect_atomic_plots(plots) args = range == 1 ? (scene,) : (scene, range) on(events(scene).mouseposition) do mp p, idx = pick(args...) @@ -33,31 +33,6 @@ end @deprecate mouse_selection pick -function flatten_plots(x::Combined, plots = AbstractPlot[]) - if isempty(x.plots) - # Atomic plot! - push!(plots, x) - else - for elem in x.plots - flatten_plots(elem, plots) - end - end - return plots -end - -function flatten_plots(array, plots = AbstractPlot[]) - for elem in array - flatten_plots(elem, plots) - end - plots -end - -function flatten_plots(scene::Scene, plots = AbstractPlot[]) - flatten_plots(scene.plots, plots) - flatten_plots(scene.children, plots) - plots -end - """ mouse_in_scene(fig/ax/scene[, priority = 0]) diff --git a/src/scenes.jl b/src/scenes.jl index e7132663932..16115de2cc0 100644 --- a/src/scenes.jl +++ b/src/scenes.jl @@ -623,3 +623,46 @@ struct FigureAxisPlot end const FigureLike = Union{Scene, Figure, FigureAxisPlot} + +""" + is_atomic_plot(plot::Combined) + +Defines what Makie considers an atomic plot, used in `collect_atomic_plots`. +Backends may have a different definition of what is considered an atomic plot, +but instead of overloading this function, they should create their own definition and pass it to `collect_atomic_plots` +""" +is_atomic_plot(plot::Combined) = isempty(plot.plots) + +""" + collect_atomic_plots(scene::Scene, plots = AbstractPlot[]; is_atomic_plot = is_atomic_plot) + collect_atomic_plots(x::Combined, plots = AbstractPlot[]; is_atomic_plot = is_atomic_plot) + +Collects all plots in the provided `<: ScenePlot` and returns a vector of all plots +which satisfy `is_atomic_plot`, which defaults to Makie's definition of `Makie.is_atomic_plot`. +""" +function collect_atomic_plots(xplot::Combined, plots=AbstractPlot[]; is_atomic_plot=is_atomic_plot) + if is_atomic_plot(xplot) + # Atomic plot! + push!(plots, xplot) + else + for elem in xplot.plots + collect_atomic_plots(elem, plots; is_atomic_plot=is_atomic_plot) + end + end + return plots +end + +function collect_atomic_plots(array, plots=AbstractPlot[]; is_atomic_plot=is_atomic_plot) + for elem in array + collect_atomic_plots(elem, plots; is_atomic_plot=is_atomic_plot) + end + plots +end + +function collect_atomic_plots(scene::Scene, plots=AbstractPlot[]; is_atomic_plot=is_atomic_plot) + collect_atomic_plots(scene.plots, plots; is_atomic_plot=is_atomic_plot) + collect_atomic_plots(scene.children, plots; is_atomic_plot=is_atomic_plot) + plots +end + +Base.@deprecate flatten_plots(scenelike) collect_atomic_plots(scenelike)