Skip to content

Conversation

@asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Dec 31, 2024

Description

Respects the depth_shift kwarg in CairoMakie 2d plots, currently purely as a subtractive factor so we can distinguish between plots at the same z-level.

This lets Tyler maps plot correctly (non-blurry) in CairoMakie.

Needs tests, I'll probably add a simple mockup of a Tyler.jl map via two image plots with different depth shifts. That is if this subtractive approach makes sense. I'm not sure if this is the most correct thing to do though...cc @ffreyer

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)

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.

@asinghvi17 asinghvi17 self-assigned this Dec 31, 2024
@asinghvi17 asinghvi17 requested a review from ffreyer December 31, 2024 03:49
@asinghvi17 asinghvi17 added the CairoMakie This relates to CairoMakie, the vector backend for Makie based on Cairo. label Dec 31, 2024
@MakieBot
Copy link
Collaborator

Benchmark Results

SHA: 3019288e6b626e116032d0666bba1d723b27fd95

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

@ffreyer
Copy link
Collaborator

ffreyer commented Dec 31, 2024

Why not just translate instead?

@asinghvi17
Copy link
Member Author

asinghvi17 commented Dec 31, 2024

I think we were using depth shift to keep things general between 2d and 3d plots, where tiles at different z levels look pretty odd :D

Plus it's robust to transform_func where as in the 2D -> 3D case z translation may do strange things (since it translates after transform_func is applied)

@ffreyer
Copy link
Collaborator

ffreyer commented Dec 31, 2024

I don't like depth_shift creeping into 2D rendering and this plot sorting thing. For 2D you should be able to do everything you want with just translate and plot order. For 3D CairoMakie is just not the right tool. There are some special/niche cases where plot order/depth_shift help, but it doesn't seem particularly useful to me to treat those. As a whole 3D is still not going to work without rasterization.

In terms of correctness, well, plot sorting in the first place isn't correct. It only looks at the z shift from translations and completely ignores the plot (i.e. z in plot args). Which is typically fine in 2D... For 3D neither of those have anything to do with depth anymore.

In 3D using just depth_shift would make more sense than using translations. At least that corresponds to depth, even if it ignores everything. Combining it with translations is mixing apples and oranges. The technically correct thing here would be sorting based on clip_pos[3] / clip_pos[4] + depth_shift per pixel, which isn't possible.

In 2D depth_shift and z translation act on the same axis but on different scales. You could bring them to the same scale with

z = zvalue2d(plot) # grabs plot.transformation.translation[][3] or equivalent atm
pv = scene.projectionview[]
depth = pv[3, 3] * z + pv[3, 4] + depth_shift

if I'm not mistaken. This might result in an opposite sign to what we have now. It should work correctly, but I still think it's redundant with translate.

@ffreyer
Copy link
Collaborator

ffreyer commented Apr 3, 2025

After working on #4879 I'm less opposed to considering depth_shift in CairoMakie, but it should still be done correctly. That means transforming everything to clip space (depth shift included) and doing depth sorting based on clip space z values. That should also be done in GLMakie and WGLMakie where render order can be important to avoid artifacts. For performance sake we probably want to do this after #4630, because that includes caching of world space coordinates and bounding boxes.

I'll close this pr for now since it should be a more general refactor

@ffreyer ffreyer closed this Apr 3, 2025
@github-project-automation github-project-automation bot moved this from Work in progress to Closed in PR review Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CairoMakie This relates to CairoMakie, the vector backend for Makie based on Cairo.

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

4 participants