fix: proper perspective aspect ratio preservation#74
Conversation
Turns out that perspective matrices weren't actually having their aspect ratios preserved.
We use that term with the ResizePolicy to do very similar things.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
- Coverage 88.09% 88.08% -0.01%
==========================================
Files 64 64
Lines 3174 3182 +8
==========================================
+ Hits 2796 2803 +7
- Misses 378 379 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes aspect-ratio preservation for zoom_to_fit(..., type="perspective") by introducing/renaming the option to letterbox, documenting its behavior, and adding regression tests to ensure non-square viewports no longer cause anisotropic distortion.
Changes:
- Renamed
preserve_aspect_ratiotoletterboxand updated internal logic to apply letterboxing for both orthographic and perspective projections. - Added a new perspective letterbox test (and renamed the existing orthographic aspect test) to validate correct NDC mapping for wide/tall canvases.
- Updated
scenex.util.show()to use the newletterbox=Trueparameter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/scenex/utils/projections.py |
Implements letterbox behavior (including perspective aspect correction), updates docstring, and adds _aspect_ratio() helper. |
tests/utils/test_projections.py |
Renames orthographic aspect test and adds a new regression test for perspective letterboxing. |
src/scenex/util.py |
Updates show() to call zoom_to_fit(..., letterbox=True). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Thanks Copilot!
Turns out that perspective matrices weren't actually having their aspect ratios preserved. This PR documents what the (newly-renamed)
letterboxparameter does, and adds in a test ensuring it behaves as expected.