Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully flatten plots and sort by atomic zorder in all backends #2793

Merged
merged 22 commits into from Jul 13, 2023

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Mar 21, 2023

Description

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.plots is empty, in which case it's treated as an atomic plot.

This seems to produce the correct results for my recipe.

Fixes #2792

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

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.
@MakieBot
Copy link
Collaborator

MakieBot commented Mar 21, 2023

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 11.05s (10.92, 11.26) 0.13+- 1.08s (1.06, 1.11) 0.02+- 810.85ms (792.96, 870.82) 27.49+- 10.07ms (9.95, 10.23) 0.11+- 89.32ms (88.85, 89.70) 0.35+-
master 11.09s (10.95, 11.20) 0.08+- 1.09s (1.06, 1.10) 0.01+- 798.05ms (785.95, 812.25) 9.96+- 10.12ms (9.97, 10.25) 0.11+- 89.05ms (88.48, 89.47) 0.35+-
evaluation -0.34%, -0.04s invariant (-0.35d, 0.52p, 0.10std) -0.39%, -0.0s invariant (-0.26d, 0.64p, 0.02std) +1.58%, 12.79ms invariant (0.62d, 0.28p, 18.73std) -0.55%, -0.06ms invariant (-0.51d, 0.36p, 0.11std) +0.29%, 0.26ms invariant (0.75d, 0.19p, 0.35std)
CairoMakie 10.88s (10.08, 11.50) 0.42+- 1.35s (1.29, 1.49) 0.07+- 271.68ms (248.48, 324.35) 24.89+- 14.98ms (14.35, 16.39) 0.69+- 8.04ms (7.78, 8.54) 0.28+-
master 10.98s (10.73, 11.44) 0.27+- 1.32s (1.27, 1.43) 0.05+- 254.05ms (240.15, 280.86) 13.63+- 15.00ms (14.47, 15.36) 0.31+- 7.84ms (7.63, 8.32) 0.23+-
evaluation -0.96%, -0.1s invariant (-0.29d, 0.60p, 0.35std) +1.53%, 0.02s invariant (0.34d, 0.54p, 0.06std) +6.49%, 17.64ms noisy🤷‍♀️ (0.88d, 0.13p, 19.26std) -0.16%, -0.02ms invariant (-0.05d, 0.93p, 0.50std) +2.52%, 0.2ms invariant (0.79d, 0.16p, 0.25std)
WGLMakie 15.13s (14.91, 15.30) 0.16+- 1.67s (1.58, 1.74) 0.05+- 14.54s (14.16, 15.10) 0.32+- 20.92ms (18.51, 27.04) 2.81+- 1.41s (1.35, 1.58) 0.08+-
master 15.19s (14.78, 15.62) 0.28+- 1.68s (1.63, 1.72) 0.04+- 14.48s (14.12, 15.16) 0.33+- 19.59ms (18.88, 20.79) 0.66+- 1.40s (1.33, 1.48) 0.05+-
evaluation -0.42%, -0.06s invariant (-0.28d, 0.61p, 0.22std) -0.94%, -0.02s invariant (-0.34d, 0.54p, 0.04std) +0.42%, 0.06s invariant (0.19d, 0.73p, 0.32std) +6.36%, 1.33ms noisy🤷‍♀️ (0.65d, 0.26p, 1.73std) +1.21%, 0.02s invariant (0.26d, 0.63p, 0.06std)

@asinghvi17 asinghvi17 marked this pull request as ready for review March 21, 2023 06:48
@SimonDanisch
Copy link
Member

Ok, pushed the desired refactor, hopefully it's ok like this now! :)
@asinghvi17 can we add a test for #2792?

@asinghvi17
Copy link
Member Author

Recipes don't seem to work in @reference_test blocks - specifically,
Got exception outside of a @test
LoadError: syntax: unsupported const declaration on local variable around /home/runner/work/Makie.jl/Makie.jl/MakieCore/src/recipes.jl:174
Stacktrace:
[1] top-level scope
@ ~/work/Makie.jl/Makie.jl/ReferenceTests/src/tests/examples2d.jl:916

@SimonDanisch
Copy link
Member

You can move it outside the @reference_test block?

@github-actions
Copy link
Contributor

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

@asinghvi17
Copy link
Member Author

Yeah, I could have but didn't want to pollute the main namespace, so settled for bootlegging a recipe by plotting into another plot.

@github-actions
Copy link
Contributor

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

@asinghvi17
Copy link
Member Author

asinghvi17 commented Mar 24, 2023

Z-translation within a recipe
The reference image looks correct (this is on CI).

@SimonDanisch
Copy link
Member

I thought this fixes a bug only happening in recipes?

@asinghvi17
Copy link
Member Author

It's general to any nested plot, because of the way they were flattened in CairoMakie - we only sorted by z-level of the topmost recipe (whatever was in scene.plots).

I tested this on a version of CairoMakie without this patch. It showed only blue (the translation on the red plot had no effect).

@asinghvi17 asinghvi17 changed the title Make get_all_plots in CairoMakie fully flatten plots Fully flatten plots and sort by atomic zorder in all backends Mar 28, 2023
@SimonDanisch SimonDanisch reopened this Mar 28, 2023
@SimonDanisch
Copy link
Member

image
Oh what? Did they decrease the amount of ram, or is there some regression going on?
But, 1.8.5 (julia=1) hasn't been updated and neither has MeshIO...

@SimonDanisch SimonDanisch merged commit bd315b4 into master Jul 13, 2023
14 checks passed
@SimonDanisch SimonDanisch deleted the as/cairomakie_proper_flatten branch July 13, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready, but unreviewed
Development

Successfully merging this pull request may close these issues.

CairoMakie does not respect the z translation of plots inside recipes
4 participants