feat: Camera Projection Transforms #27
Conversation
Defines how 2D view NDC are mapped to vectors in 3D space
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
+ Coverage 80.83% 81.96% +1.12%
==========================================
Files 42 43 +1
Lines 1529 1558 +29
==========================================
+ Hits 1236 1277 +41
+ Misses 293 281 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| projection: Transform = Field( | ||
| default_factory=Transform, | ||
| description="Describes how 3D points are mapped to a 2D canvas", | ||
| ) |
There was a problem hiding this comment.
this is indeed a very important thing for us to represent somehow here. And, the most important (and difficult) thing here is to represent this in a way such that the model has a single source of truth. zoom and center, for example, (and fov which would eventually have had if we were mimicking the vispy model) are also just ways to represent the projection transform.
I do very much like the concept of having a single projection transform as the fundamental source of truth, however. And I would be fully in support of removing any conflicting fields (like zoom) from the model, and instead replace them with convenience methods like set_zoom() or look_at() which in turn update the projection transform. (and, because those decomposed things like fov, aspect, etc... are all things that users very often want to know, it is convenient to have ways to go back and forth between composed and decomposed components... similar to what pygfx does in their Affine model)
so: high level. I'm a fan of this change, but want to see it replace zoom and everything else that it can
There was a problem hiding this comment.
by the way ... we're touching on similar issues here to tlambert03/microvis#47 ... and related discussions in tlambert03/microvis#38
not that any of that necessarily had it "figure out" in a nice clean model. but it's possible the discussions/code have some interest here (buy perhaps not :))
There was a problem hiding this comment.
so: high level. I'm a fan of this change, but want to see it replace zoom and everything else that it can
Fully agree with you. I'm thinking that for now I'd like to remove zoom from the camera model entirely (and probably center as well, although it's less related to this change), and we can add computed fields back into the model later if they make sense.
In the pursuit of a single source of truth, let's resort to the projection matrix (when/if possible?) for this.
For some reason, if I don't do this, the Matrix3D constructor RESURRECTS some previous matrix I used in a previous test
gselzer
force-pushed
the
camera-projection
branch
from
7b30c5f to
09e5b4d
Compare
Thanks to @tlambert03 for the suggestion. Could probably be cleaned up further, but at least this is a step in the right direction!
Eventually, we'll want to compute grids ourselves on the model side, I think, but for now this works
...man, this feels so good
It's worse :)
There was a problem hiding this comment.
@tlambert03 I'm getting pretty happy with this, do you want to take more of a look?
There are definitely semi-related features that I'd like to add outside of this PR, including:
- more utilities for matrix construction
- Model-based computation of zoom-to-fit
- Model-based view sizing
There are also a few scattered FIXMEs still that I want to get to in the coming days, but I don't think their presence will affect your review:
- Remove the
Camera.typefield from the Camera model - just need to address its disappearance in the pygfx adaptor. - Fix the
Camera.projectiondefault. - Add a
tests/adaptors/_pygfx/test_camerafile analogous to the vispy one. Those tests should be super simple.
| # Translate the camera to the center of the volume, and distance the camera from the | ||
| # volume in the z dimension (important for perspective transforms) | ||
| view.camera.transform = Transform().translated((127.5, 127.5, 300)) | ||
|
|
||
| # view.camera.projection = projections.orthographic( | ||
| # 1.1 * data.shape[1], | ||
| # 1.1 * data.shape[2], | ||
| # 1000, | ||
| # ) | ||
|
|
||
| view.camera.projection = projections.perspective( | ||
| # TODO: Create a helper function for this. | ||
| fov=2 * atan(data.shape[1] / 2 / 300) * 180 / pi, | ||
| near=300, | ||
| far=1_000_000, # Just need something big | ||
| ) |
There was a problem hiding this comment.
Wonder whether some documentation/comments here would be helpful - I can't think of much right now though
gselzer
changed the title
Notably, this limits the type of pygfx-specific interaction available. But we need a scenex version of this anyways - planning to implement the beginnings of this with events
gselzer
force-pushed
the
camera-projection
branch
from
f477110 to
8f91699
Compare
#29 now adds this functionality |
|
definitely very enthusiastic about this change. but I think it needs to be a bit farther before a thorough review. I pulled and ran our basic_scene example, and rather than zooming on mouse wheel, it translates. And on basic_volume, mouse wheel gave me: and then the image disappeared. I do know that wheel events are not in the scope of this PR (and I don't think they should be)... but do you think it would be possible to get this to a point where the new model code is there and waiting to be used, but without losing functionality of our basic examples? Or is this just too big of a change and too precarious of a middle point to expect functioning mouse wheel events after this? |
It is definitely a precarious middle point, yeah - both pygfx and vispy's event systems are naturally heavily related to their concepts of camera projections. For example, for orthographic projections you basically need a
Without the I did spend an hour or two looking attempting a fix, but it really seems like wasted time with #28 hot on the heels of this PR (which avoids both of the errors you sae). For now, I'm thinking we just quickly patch up |
do you have a version that includes #28 that does work with mouse events? or would we be entering a period where the demos no longer work and we're not immediately sure when they will start working again? |
Yep, #28 actually already includes this PR in the commit history, so you can just run the examples on that branch. Note that there's only a pan/zoom camera event filter right now, but it's currently enabled on both |
|
Hmm, does actually look like the examples on this branch do not work right now with vispy...looking into that |
|
Okay, vispy's breakages are due to problems with Essentially, this is another case where the current breakage is actually solved in the "correct" way, with #29. As that changeset is also built atop these commits, I do not see the same errors when running our examples on that branch. It may be worth merging that PR into this one... |
2 participants
This PR adds the
Camera.projectionfield, which enables the arbitrary customization of the camera frustum. This is a huge step towards solving #16, as it provides a uniform way to unproject a canvas position into a ray passing through the world.The changeset needs a fair bit of work before merge:
Immediate discussion points:
src/scenex/utils/projections.py, however I'm still looking for something better.Camerafields could be computed from theprojectionmatrix and/or removed?typeshould likely be removed, as it ties heavily into the idea of the projection matrixzoommight become a computed field?TODOs:
CameraAdaptor._snx_zoom_to_fit, in favor of mathematically computing the bounds and setting the camera transform and projection