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

Cairo (scene) render order #4724

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Cairo (scene) render order #4724

wants to merge 4 commits into from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jan 15, 2025

Description

This is effectively #4150 for CairoMakie. The problems I'm trying to solve in that pr are:

  1. (after display) scenes are not inserted immediately, but when plotted to. This can cause scene order to differ from later displaying and it can cause scene backgrounds to just be ignored
  2. all scene clearing happens before plot rendering, resulting in plots drawing over scenes that they should be behind

Since there is no interactive adding of scene to the CairoMakie screen, Problem (1) doesn't apply here. Problem (2) is fairly easily fixed by just drawing scene-by-scene and only clearing the currently picked scene. This change is implemented here.

I added a refimg test that highlights these issues. For CairoMakie:

before after (correct?)
before after

For reference this is what the other backends produce: (#4150 reproduces CairoMakie after, but AA is broken iirc)

GLMakie WGLMakie
GLMakie WGLMakie

The lack of z sorting across multiple scenes breaks a bunch of assumptions made in MakieLayout. This includes:

  • "Legend draw order" test being broken
  • plot vs grid order changing in PolarAxis
  • axis frame drawing behind plots
  • Axis3 axis labels may now get clipped

Most of these issues could be fixed by adding an overlay scene and moving the respective plots there. Some cases, like the PolarAxis grid would be more complicated. Currently the grid can be moved behind or in front of plots, which would require moving the grid from a background scene to a foreground scene or vice versa.

Perhaps a better, less breaking option would be to "merge" scenes with clear = false. I the currently rendered clear = true scene has children with clear = false, consider all their plots part of the current scene, sorting them like we would now. (I think this would also be a good optimization for GLMakie if we don't want to rely on stencil buffers.)

This pr is currently more for testing plans/discussion/getting a CI run to look at. Maybe it gets to a point where we consider CairoMakie correct without breaking other backends, and maybe we can merge this then. Getting the other backends to match will take more effort I think.

Type of change

Delete options that do not apply:

  • 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 CHANGELOG.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.

@MakieBot
Copy link
Collaborator

MakieBot commented Jan 15, 2025

Benchmark Results

SHA: 1a11599b35f529a7e0c34166b6447a0e1d81e8a1

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

2 participants