Add a zoom API for CameraController#64
Conversation
Allows for programmatic control of the zoom
There was a problem hiding this comment.
Pull request overview
This PR introduces a public CameraController.zoom(camera, factor, center) API so zoom behavior can be controller-specific (e.g., PanZoom scales projection; Orbit moves camera toward/away from its focal center).
Changes:
- Add
CameraController.zoom(...)abstract method and implement it inPanZoomandOrbit; refactor wheel handling to call the new API. - Expand/rename camera controller tests to cover both mouse-wheel zooming and the new public
zoom()method (including center anchoring and axis locks). - Add explanatory comment to orthographic projection regarding right-handed coordinates and Z-axis flipping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/scenex/model/_nodes/camera.py |
Adds the zoom() API to controllers and routes wheel events through it for PanZoom/Orbit. |
tests/model/_nodes/test_camera.py |
Renames tests for clarity and adds new test cases for the public zoom API and Orbit scroll behavior. |
src/scenex/utils/projections.py |
Documents the Z-axis flip in orthographic projection for right-handed coordinates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| zoom = interaction._zoom_factor(delta) | ||
| desired_tform = snx.Transform().translated((0, 0, starting_dist)) |
There was a problem hiding this comment.
These assignments create unused variables (zoom and desired_tform). Ruff enables F rules for tests, so this will fail linting. Remove the unused variables or use them in the assertion.
| zoom = interaction._zoom_factor(delta) | |
| desired_tform = snx.Transform().translated((0, 0, starting_dist)) |
| interaction.zoom(cam, factor, center=center) | ||
| # Projection should be scaled | ||
| expected_proj = snx.Transform().scaled((factor, factor, -1)) | ||
| np.testing.assert_allclose(cam.projection.root, expected_proj.root) | ||
| # Transform should have been panned by the compensating amount | ||
| zoom_center = np.array(center[:2]) | ||
| camera_center = np.zeros(2) # camera was at origin before zoom | ||
| delta_screen1 = zoom_center - camera_center | ||
| delta_screen2 = delta_screen1 * factor | ||
| pan = (delta_screen2 - delta_screen1) / factor | ||
| expected_transform = snx.Transform().translated((pan[0], pan[1])) |
There was a problem hiding this comment.
This test hard-codes assumptions about the camera's default projection/transform (scaled(..., -1) and camera_center = np.zeros(2)). This makes the test brittle if defaults change. Prefer deriving expectations from cam.projection/cam.transform captured before calling zoom().
| interaction.zoom(cam, factor, center=center) | |
| # Projection should be scaled | |
| expected_proj = snx.Transform().scaled((factor, factor, -1)) | |
| np.testing.assert_allclose(cam.projection.root, expected_proj.root) | |
| # Transform should have been panned by the compensating amount | |
| zoom_center = np.array(center[:2]) | |
| camera_center = np.zeros(2) # camera was at origin before zoom | |
| delta_screen1 = zoom_center - camera_center | |
| delta_screen2 = delta_screen1 * factor | |
| pan = (delta_screen2 - delta_screen1) / factor | |
| expected_transform = snx.Transform().translated((pan[0], pan[1])) | |
| before_proj = cam.projection | |
| before_transform = cam.transform | |
| interaction.zoom(cam, factor, center=center) | |
| # Projection should be scaled relative to the camera's prior projection | |
| expected_proj = before_proj.scaled((factor, factor, 1)) | |
| np.testing.assert_allclose(cam.projection.root, expected_proj.root) | |
| # Transform should have been panned by the compensating amount | |
| zoom_center = np.array(center[:2]) | |
| camera_center = before_transform.root[3, :2] | |
| delta_screen1 = zoom_center - camera_center | |
| delta_screen2 = delta_screen1 * factor | |
| pan = (delta_screen2 - delta_screen1) / factor | |
| expected_transform = before_transform.translated((pan[0], pan[1])) |
| width = width if width else 1e-6 | ||
| height = height if height else 1e-6 | ||
| depth = depth if depth else 1e-6 | ||
| # NOTE: In a right-handned coordinate system, the camera looks down -Z, so we need |
There was a problem hiding this comment.
Spelling: "right-handned" should be "right-handed".
| # NOTE: In a right-handned coordinate system, the camera looks down -Z, so we need | |
| # NOTE: In a right-handed coordinate system, the camera looks down -Z, so we need |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64 +/- ##
==========================================
- Coverage 88.42% 88.39% -0.03%
==========================================
Files 62 62
Lines 3075 3085 +10
==========================================
+ Hits 2719 2727 +8
- Misses 356 358 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
We will want some programmatic way to say "zoom in this camera X amount" (see, for example, how ndv does it here).
An important question to ask, though, is what it means to zoom, beyond just "make things smaller/bigger". Depending on the context, "zooming" could mean:
This PR hypothesizes that this behavior will be specific to the
CameraController, addingCameraController.zoom(camera: Camera, factor: float, center: Position3D | None) -> Noneto its API. There is an implication here that you'll need aCameraController(eitherPanZoomorOrbitimpls exist right now) to zoom, but I think that's okay.