| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Pass roll angle to view_init() in degrees (not radians)
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
|
Thanks for catching this! Is there a way that you can modify the existing mouse drag tests or add a new one to check for this? Those tests are a bit tricky to write / understand since they spoof mouse movements, so if there isn't an easy way to do that I think I would be fine with skipping it. |
Sorry, something went wrong.
I suppose I could try - but which existing tests did you have in mind? It seems most tests are for roll=0, with the exception of test_bar3d_shaded(), test_shared_view() and test_format_coord(), but these are clearly for other purposes than roll specifically. |
Sorry, something went wrong.
|
I was thinking about test_pan and test_toolbar_zoom_pan, but it looks like we don't actually have an equivalent test for rotating the view. If you want to take a shot at writing a similar test for rotating go for it, but it might be tricky so if you're not comfortable with that then we can merge as-is and I'll open an issue to add those tests. |
Sorry, something went wrong.
|
I'll give it a try and let you know, it might take me a day or two |
Sorry, something went wrong.
Add tests for rotation using the mouse (test_rotate), with and without roll. The test with nonzero roll fails if the roll angle is passed with wrong units (issue matplotlib#28256).
There was a problem hiding this comment.
This looks good to me, approved and once another maintainer adds their approval as well it will be merged in. The "codecov/project/tests" CI failure is more informational and not a blocker.
Thank you and congratulations on your first contribution to matplotlib! We hope to see more of you, and contributions are always welcome!
Sorry, something went wrong.
Well, now that we're at it...
And similarly:
That is to say, the ordering you chose is not the common one. And if you consider that searching for both yields
(some links are non-committal), then it becomes clear that the elevation, azimuth order is really a minority position: (41-37)/(291-37) < 2%. Should we raise an issue to change it? |
Sorry, something went wrong.
|
I had a comment and it was mostly wrong when I actually went back to look, so ignore that email. I forget the reasoning behind changing the argument order, but I believe it’s to match the order in which the rotations are applied. I’d be hesitant to change it again. Matching Matlab convention is nice, but not an actual goal of the matplotlib library. The kwargs are clear and unambiguous as-is, and we try to minimize API changes per this policy: https://matplotlib.org/devdocs/devel/api_changes.html |
Sorry, something went wrong.
I understand the hesitation, and I don't particularly care about what Matlab's convention is either. But I do care that all the rest of the world appears to do something else than what matplotlib does at the moment. And the matplotlib order emphatically does not match the order in which the rotations are applied: that is azim, elev, roll. |
Sorry, something went wrong.
|
Elevation and azimuth are applied at the same time, in effect neither comes before the other. https://github.com/matplotlib/matplotlib/blob/main/lib/mpl_toolkits/mplot3d/axes3d.py#L1220-L1222 |
Sorry, something went wrong.
|
Thinking about it more, I think you’re more right about the rotation order if this is conceptualized as Euler angles. If thinking about extrinsic rotations, then the rotation order is roll, elev, then azim. If thinking about intrinsic rotations then the rotation order is azim, elev, then roll. The definitions in the docs are still unambiguous so I still think this is fairly marginal to make a change of API, but maybe @oscargus can weigh in. |
Sorry, something went wrong.
Well actually, to be precise, mplot3d's azim, elev, roll are a kind of Tait-Bryan angles, not classic Euler angles (there exist 12 kinds of each, not counting the signs...)
Ah, yes, now we're talking!
Ah yes. So perhaps it is time to open a separate Issue and close this PR ;) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
In mplot3d.Axes3D._on_move(), pass roll angle to view_init() in degrees (as it expects; not converted to radians, mistakenly).
Resolves #28256.