Add the ability to change the focal length of the camera for 3D plots #22046
Conversation
There was a problem hiding this comment.
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?
|
Got to fix some things next week. I'm learning as I go here, but wikipedia 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. |
|
Basic functionality is working now, check out the comparison plot (still a bit of a wip): |
|
I'm a little conflicted on whether to do input range checking for the focal length. The focal_length=0 is a (in reality as well as in code) degenerate case, but I'm not sure errors here should be blocking. The negative focal lengths essentially rotate the camera location 180 deg about the horizontal view line. A physical camera will never have a negative focal length, but maybe there's a use case for it? A focal length of -np.inf is also a weird edge case to handle, since we'd have to do some transforms before passing it to the orthographic projection function. |
|
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 to
1c8b836
Compare
|
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 options Typo Test fix, example fix, and linting What's new, example fix Merge conflict Offset zooming of focal length Code review comments, consolidate projection functions Try and fix zooming Try to fix the focal length zooming Update example Cleanup example cleanup more example tweaks more example tweak swap plot order Enforce a positive focal length focal lentgh tests flake8 linting docstring tweak
1c8b836 to
e8a3f57
Compare
|
This is good to go, here's the new gallery image: |
|
@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 (see bottom of this page), but we don't have that capability. |
f8c5550 to
6994846
Compare
Make focal_length a private attr linting
fac3a18 to
c1e5e0a
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.
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.
|
Sweet, added tests and addressed those comments. |
There was a problem hiding this comment.
I'll assume the math is right if @WeatherGod is okay with it.
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
pytestpasses).flake8-docstringsand 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).