Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add the ability to change the focal length of the camera for 3D plots#22046
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
I am intrigued, and I can think of some use-cases for this.
It does need tests, and it probably needs a bit more polish. Also, based on my very limited knowledge of 3D transformations, there is a relationship between orthogonal and perspective transformations through the focal length parameter. Perhaps a bit of documentation expanding on this concept might be useful?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Got to fix some things next week. I'm learning as I go here, butwikipedia says that an orthographic projection (ie parallel light rays) happens when you have an infinite focal length along with an infinite view distance. So I think it makes physical sense to only have the single projection function, and for set_proj_type('ortho'), simply have this set the focal distance to infinity. Then check for infinity in persp_transformation and break it out to ortho_transformation if needed for the numerics. An argument against this would be if we wanted to add oblique projections such as are used in some realms of drafting, but I think that's a little out of scope for now. |
Uh oh!
There was an error while loading.Please reload this page.
scottshambaugh commentedDec 31, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I don't think there is a compelling case to keep negative focal lengths if the same effect can be achieved with a positive focal length and a twist. It just seems like it will confuse folks. |
882cb97
to1c8b836
Comparescottshambaugh commentedJan 2, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The failed tests are only the ones identified in#22076, and aren't related to this PR. |
Create a gallery example showing the different proj_type optionsTypoTest fix, example fix, and lintingWhat's new, example fixMerge conflictOffset zooming of focal lengthCode review comments, consolidate projection functionsTry and fix zoomingTry to fix the focal length zoomingUpdate exampleCleanupexample cleanupmore example tweaksmore example tweakswap plot orderEnforce a positive focal lengthfocal lentgh testsflake8 lintingdocstring tweak
1c8b836
toe8a3f57
CompareThere 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.
Can we add some mathematical tests of the transformation functions? I can't really confirm or deny that the image test is correct.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
scottshambaugh commentedJan 5, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@QuLogic The real proof of the pudding is in the output plots since the "correctness" of the projection matrices depends on the rest of our implementation. IMO the new test image with a few focal lengths is enough to test that code path. We have at least two datapoints that we can look at within matplotlib to confirm that it's working as intended: plots which match the existing focal length of 1, and plots which approach the orthographic projection as the focal length goes to large numbers (I can't spot a visual difference between ortho and focal_length=1000). There is also the qualitative behavior that the plot "deepens" as it approaches 0, and starts to blow up as expected for very small focal lengths (on the order of <0.01). It would be nice to test the math directly, but I don't know of canonical projection matrices for certain values that we can compare against. Certainly we can see that it matches what's expected by inspection (below). Other implementations have had OpenGL as a baseline renderer to compare pixels against (seebottom of this page), but we don't have that capability. |
f8c5550
to6994846
CompareMake focal_length a private attrlinting
fac3a18
toc1e5e0a
Compare@WeatherGod you have a block on this - I think its pretty close, so can you take a second look? |
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.
Also needs a couple of error-check tests as well to make sure the error-handling paths are exercised and throws exceptions.
This is looking really neat! The api isn't as elegant as I'd like, but that's largely because of poor foresight on my part years ago. I can't imagine a better way to handle it.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
scottshambaugh commentedJan 18, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sweet, added tests and addressed those comments. |
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.
I'll assume the math is right if@WeatherGod is okay with it.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Resolves#22035 by adding a focal_length parameter to 3D axes, which changes the projection matrix for 'persp' projections to better replicate real-world cameras.
The default focal_length of 1 is equivalent to the current projection.
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).