-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
Add zooming & translation controls to Axis3 #4131
base: breaking-0.22
Are you sure you want to change the base?
Conversation
Compile Times benchmarkNote, 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(fig)
|
Zooming for Axis3 is basically just shrinking |
I don't know yet. In the prototype code it is, but I don't know if that's a good way to do it. that code was really just to test if |
# TODO: This crashes? But is also necessary... | ||
for i in max(0, N):7 | ||
glDisable(GL_CLIP_DISTANCE0 + UInt32(i)) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure in what context this was crashing but it's not crashing anymore on my laptop or PC.
Without this turning clip_planes on once will turn them on forever, with random data. This causes plots to disappear randomly. Maybe this should be a hotfix instead of being part of this pr...
Example for this: using FileIO, GLMakie
cat = load(Makie.assetpath("cat.obj"))
fig = Figure()
Box(fig[1, 1:3], color = :lightblue)
Box(fig[3, 1:3], color = :lightblue)
Box(fig[2, 1], color = :lightblue)
Box(fig[2, 3], color = :lightblue)
ax = Axis3(fig[2, 2], viewmode = :free)
mesh!(ax, cat, color = :orange)
fig # and some zooming/interacting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Not sure if @jkrumbiegel has time to look at this, but may make sense?
Looks pretty good @ffreyer! The only thing I didn't like while trying it out was that if you're zoomed and panned to some position off-center, you lose your position quickly when further scaling or rotating because the reference point is still the center of the axis. I think it would be better to see if any plot objects in the axis can be picked, and if so, use the picked point as the pivot. |
I think I avoided doing these kinds of things because a lot of the times I tried to be smart and fancy with Camera3D it backfired. Some options for rotating are: (for viewmode = :free)
Zooming is somewhat similar. You can:
I would be fine adding a switch for scene vs axis centered rotation and zooming, and also separating the axis center from the rotation + zoom center and adding extra interactions to control it (like Camera3D's alt-click, a secondary translation, a secondary reset). Those sound like relatively "dumb" interactions to me and therefore things you can relatively easily reason with. |
I realized that I could also just reuse the
Also did a bit of cleanup and fixed a rendering bug caused by |
Which comes from just doing xyz/w in inversions. This is not valid if w <= 0, at least with perspective projection
I took the easy way out and just moved them to 3D space/ |
@@ -390,7 +393,7 @@ function project(proj_view::Mat4{T1}, resolution::Vec2, point::Point{N, T2}) whe | |||
# at this point the visible range is strictly -1..1 so FLoat64 doesn't matter | |||
p = (clip ./ clip[4])[Vec(1, 2)] | |||
p = Vec2{T}(p[1], p[2]) | |||
return (0.5 .* (p .+ 1) .* (resolution .- 1)) .+ 1 | |||
return 0.5 .* (p .+ 1) .* resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tick alignment with frame lines got worse when I moved the frame to data space. So I spent too long trying to figure out why.
I thought it might be a Float32 vs Float64 thing, so cleaned up the remaining Float32 types in Axis3. Didn't help. I tried forcing lines to convert on the CPU to make sure conversions don't happen till the end. Didn't help.
Then I got frustrated and moved the CairoMakie line point projection to Makie so I could revert moving the frame to data space without having clipping issues. And the alignment issues were still there. Then I tried using that code for ticks as well and it finally went away.
So i tried to figure out why and started comparing. Didn't notice the resolution - 1
. Noticed that clip space points were very different, due to the CairoMakie code clipping to a -1..1 box. So I changed things up to avoid it and there was still a 0.5-1px difference. And then I finally noticed this line, changed it and I the issue seems to be fixed now.
Anyway, fixes #3302.
Description
Now that #3958 has been merged we can implement zooming and translation for Axis3 without data spilling out of he Axis limits.
Old Description
I don't aim to work on this immediately. Instead I'm hoping that this pr can spark some discussions on how zooming and translations should work with Axis3, so that the end result is more polished.Some questions:
Camera3D
/LScene? E.g.:I extended the following interactions to work with Axis3:
ScrollZoom
which comes withx/y/zzoomlock
andx/y/zzoomkey
Axis3 attributes like Axis does, and additionally azoommode
which controls whether zoom is centered on the Axis3 area (default) or on the cursor (1)DragPan
which comes withx/y/ztranslationlock
andx/y/ztranslationkey
rather than...pan...
as pan technically refers to a rotation of the camera.LimitReset
(1) "zoom centered on the cursor" really means that we want to keep a point under the cursor at the same location as we zoom in or out. With perspective projection in 3D this depends on the depth value of the point, which changes as we zoom. Without knowing exactly where that point is, we cannot fulfill that condition. And even if we ray-cast to find the coordinate, another point at a different depth that initially overlapped will move away as we zoom. Because of that I think it's better to just zoom towards the well-defined center.
TODO:
maybe add interaction for "center on clicked object"Discuss whether clipping related changes count as breaking or bug fixesI think it doesn't actually change anything...?Related issues: #3792, #1655, #1667
Type of change
Checklist