From bd315b4490ce8911988c044d1d5368f5b402a00a Mon Sep 17 00:00:00 2001 From: Anshul Singhvi Date: Thu, 13 Jul 2023 07:19:57 -0400 Subject: [PATCH] Fully flatten plots and sort by atomic zorder in all backends (#2793) * Make `get_all_plots` in CairoMakie fully flatten plots I have a recipe which plots multiple polys, and translates them based on a parameter. The idea is that only one poly has color, the rest only have stroke and the color is transparent, and they should all be seen as an overlay. It turns out that CairoMakie only orders by the z-value of plots in `scene.plots`, so any translation within a recipe (or by a user of a recipe's plots) is not respected. This commit fixes that by making `get_all_plots` truly recursive. Now, it only stops if a dispatch is defined on a particular plot type, or if `plot.children` is empty, in which case it's treated as an atomic plot. This seems to produce the correct results for my recipe. * Update NEWS.md * Update CairoMakie/src/overrides.jl * Rework to use `Makie.flatten_plots` * Update CairoMakie/src/infrastructure.jl * refactor atomic plot collection * Fix a couple of typos * Add a test * recipes don't work in `reference_test` blocks * Fix test * this doesn't seem to be needed anymore * Update infrastructure.jl --------- Co-authored-by: SimonDanisch --- CairoMakie/src/infrastructure.jl | 43 ++++++++++++++++++++------ CairoMakie/src/overrides.jl | 16 ++++++++++ GLMakie/src/screen.jl | 4 +-- NEWS.md | 5 ++- ReferenceTests/src/tests/examples2d.jl | 25 +++++++++++++++ WGLMakie/src/display.jl | 2 +- WGLMakie/src/picking.jl | 6 ++-- WGLMakie/src/three_plot.jl | 4 +-- src/interaction/interactive_api.jl | 29 ++--------------- src/scenes.jl | 43 ++++++++++++++++++++++++++ 10 files changed, 131 insertions(+), 46 deletions(-) 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 836977ecbde..ec8aa27e7b2 100644 --- a/ReferenceTests/src/tests/examples2d.jl +++ b/ReferenceTests/src/tests/examples2d.jl @@ -1019,3 +1019,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)