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

Respect depth_shift with respect to z level in CairoMakie #4694

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

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: Work in progress
Development

Successfully merging this pull request may close these issues.

3 participants