Introduce natural 3D rotation with mouse #28290

MischaMegens2 · 2024-05-24T06:27:15Z

PR summary

Addresses Issue [ENH]: Natural 3D rotation with mouse #28288, i.e.:
- Makes three-dimensional rotation by mouse independent of the particular orientation at hand, which is natural and intuitive (no 'gimbal lock')
- Extends the rotation to all three rotational degrees of freedom
Introduces three-dimensional rotation by mouse using a variation on Ken Shoemake's ARCBALL
Provides a minimal Quaternion class, to avoid an additional dependency on a large package like 'numpy-quaternion'

PR checklist

"Addresses [ENH]: Natural 3D rotation with mouse #28288" is in the body of the PR description
new and changed code is tested
[N/A] Plotting related features are demonstrated in an example
New Features are noted with a directive and release note
[N/A] Documentation complies with general and docstring guidelines

MischaMegens2 · 2024-05-26T05:50:56Z

This what the mouse rotation looks like on the surface3d.py example, before the patch (play the movie):

elev_azim.mov

(you can see the figure sometimes rotates, when you'd think it should tilt; or it moves in the opposite direction as the mouse).
After the patch:

arcball.mov

(Now the movement is much more consistent: the figure tilts up/down, left/right if you move the mouse up/down, left/right near the center; near the edge, the mouse rotates the figure).

jklymak · 2024-05-26T15:13:01Z

I don't have any problem with the proposed change, but wonder if others would.

If we use a private Quaternion class, please name _Quaternion so we don't have to maintain it. Is numpy-quaternion actually a heavy dependency if you don't install numba or scipy? Or will it not install without those? One could imagine using quaternions elsewhere in the 3d code.

Is a "cardan" angle the same as an Euler angle? Why not use the more common name? So far as I can see in numpy-quaternion, they do not use the term "cardan".

MischaMegens2 · 2024-05-26T19:09:42Z

Is a "cardan" angle the same as an Euler angle?

No, it is different. mplot3d's azimuth, elevation, roll angles are a kind of Tait-Bryan angles, not classic Euler angles; see also the wikipedia page on the subject. (It mentions Cardan angles too.) Furthermore, some signs (left or right handed) are different. So I considered that a different name was in order.

Why not use the more common name?

Because it is not the same thing. Euler angles are not simply a more common name, these are something else altogether.

So far as I can see in numpy-quaternion, they do not use the term "cardan".

They do have something to say about Euler and Tait-Bryan and other such angles, though - https://github.com/moble/quaternion/wiki/Euler-angles-are-horrible. The documentation of from_cardan_angles() does explain what the "cardan" angles are, in some detail, with this in mind.

MischaMegens2 · 2024-05-26T19:25:41Z

Is numpy-quaternion actually a heavy dependency if you don't install numba or scipy?

I'd think so, considering that all that we really need is 1) quaternion multiplication, and 2) our funny angle<>quaternion conversion, which is not part of numpy-quaternion, so it would have to be added anyway.

One could imagine using quaternions elsewhere in the 3d code.

That would be a great idea, but it is for another PR.

anntzer · 2024-05-27T11:09:57Z

This is really great, a much needed improvement. I agree with @jklymak that the quaternion class should be private, but wouldn't bother with adding an external dependency for a class less than 100 lines long.
As a minor point, I note that your quaternion class seems to be written to handle arrays of angles/of quaternions, but it may be simpler(?) to drop that and just work with scalars (and have a class with 4 scalar attributes w/x/y/z), which is the only case we need. (Not insisting of this, though.)

MischaMegens2 · 2024-05-27T17:25:25Z

This is really great, a much needed improvement.

Thank you :)

I agree with @jklymak that the quaternion class should be private, but wouldn't bother with adding an external dependency for a class less than 100 lines long.

I'll try and stash it away then.

As a minor point, I note that your quaternion class seems to be written to handle arrays of angles/of quaternions,

Well it comes for free when using numpy, the only part where it is actually noticeable is in as_cardan_angles(), where it costs us three dots and an extra comma, if you will allow the indulgence then the class stays fully general.

but it may be simpler(?) to drop that and just work with scalars (and have a class with 4 scalar attributes w/x/y/z), which is the only case we need. (Not insisting of this, though.)

Simpler? You mean, you'd prefer your multiplication to be

    return Quaternion(
        (self.w * other.w) - (self.x * other.x) - (self.y * other.y) - (self.z * other.z),
        (self.w * other.x) + (self.x * other.w) + (self.y * other.z) - (self.z * other.y),
        (self.w * other.y) - (self.x * other.z) + (self.y * other.w) + (self.z * other.x),
        (self.w * other.z) + (self.x * other.y) - (self.y * other.x) + (self.z * other.w))

rather than

    return Quaternion(
        self.scalar*other.scalar - np.dot(self.vector, other.vector),
        self.scalar*other.vector + self.vector*other.scalar + np.cross(self.vector, other.vector))

Why would you consider the former simpler? A whole bunch of + and - signs that you have to take at face value (and indeed, some people refuse to, and define their quaternions with ijk=+1, "JPL convention" at NASA, apparently...)?
In contrast, describing a quaternion as a scalar together with a vector emphasizes meaning (the vector part naturally connects with rotations) rather than focusing on the representation: the cross product is not obscured by components x, y, z. And mistakes like ijk=+1 are avoided.

anntzer · 2024-05-27T18:35:33Z

I would probably write

w0, x0, y0, z0 = self.w, self.x, self.y, self.z
w1, x1, y1, z1 = other.w, other.x, other.y, other.z

followed by formulas that directly match those at https://en.wikipedia.org/wiki/Quaternion#Hamilton_product, but again, I don't really mind. I can definitely see why you consider the scalar/vector representation more meaningful.

A side motivation for switching to scalars only is that numpy arrays tend to have enormous overheads (compared to python scalars, not to numpy scalars) for such small sizes, but performance hardly matters here.

MischaMegens2 · 2024-05-28T06:54:00Z

Good to go now, hopefully...

scottshambaugh

I want to echo the other comments that this is great work and a welcome change! Left a number of comments on some details, but I think the structure of how this is implemented works well.

One additional suggestion I want to make is that I personally see the new behavior as strictly superior, but I can imagine some implementations where someone would want the old control scheme. I would suggest that we keep the old behavior as an option, accessible through an rcparam to allow users to switch. But I think this new mode is fine to make the default.

lib/mpl_toolkits/mplot3d/axes3d.py

lib/mpl_toolkits/mplot3d/tests/test_axes3d.py

doc/api/toolkits/mplot3d/view_angles.rst

scottshambaugh · 2024-05-29T11:59:47Z

I'd think so, considering that all that we really need is 1) quaternion multiplication, and 2) our funny angle<>quaternion conversion, which is not part of numpy-quaternion, so it would have to be added anyway.

Yes, I agree with this.

but it may be simpler(?) to drop that and just work with scalars (and have a class with 4 scalar attributes w/x/y/z), which is the only case we need. (Not insisting of this, though.)

I don't think this is so important as long as it's well documented. Could go either way, I think it's fine as-is and we can always change it later for a private class.

scottshambaugh · 2024-05-29T12:02:52Z

I would also consider adding two more methods that might be useful in the future, though maybe it doesn't make sense to add until/unless we need them:

.inv() to invert quaternions
.apply(v) to rotate a 3-vector v by the quaternion

MischaMegens2 · 2024-05-30T05:14:21Z

I would also consider adding two more methods that might be useful in the future, though maybe it doesn't make sense to add until/unless we need them:
* `.inv()` to invert quaternions

* `.apply(v)` to rotate a 3-vector v by the quaternion

That's right - maybe later. Or if we would make the class public instead of private, then it might serve a purpose.

MischaMegens2 · 2024-05-30T06:03:14Z

One additional suggestion I want to make is that I personally see the new behavior as strictly superior, but I can imagine some implementations where someone would want the old control scheme. I would suggest that we keep the old behavior as an option, accessible through an rcparam to allow users to switch. But I think this new mode is fine to make the default.

The idea has crossed my mind too. Presumably, it would be straightforward to implement. I already was adding more options (Holroyd's trackball, in addition to Shoemake's; also, you could consider the 'original' Shoemake's, at double the rotation rate; etc., etc.) But just because we can, does not mean we should. Maybe it is better to be brave, and make a choice (instead of getting lost in feeping creaturism).
We could see the PR in a different light: the trackball is not a enhancement - the gimbal lock is a bug that needs fixing.
But if I'm overlooking a compelling use case, maybe you can enlighten me.

timhoffm · 2024-05-30T06:19:03Z

+1 for later. If we don’t need it now, don’t bother with it now.

MischaMegens2 · 2024-06-03T19:58:34Z

It should be good to go again (provided we can do without an rcparams for the trackball).

lib/mpl_toolkits/mplot3d/axes3d.py

timhoffm · 2024-06-04T07:49:37Z

lib/mpl_toolkits/mplot3d/axes3d.py

+            current_point = np.array([self._sx/w, self._sy/h])
+            new_point = np.array([x/w, y/h])
+            current_vec = self._arcball(current_point)
+            new_vec = self._arcball(new_point)


With above changes, this can be simplified to

Suggested change

current_point = np.array([self._sx/w, self._sy/h])

new_point = np.array([x/w, y/h])

current_vec = self._arcball(current_point)

new_vec = self._arcball(new_point)

current_vec = self._arcball(self._sx/w, self._sy/h)

new_vec = self._arcball(x/w, y/h)

Yes.
I'll try and combine the changes and rebase.

Seems something went wrong with that...

- Addresses Issue matplotlib#28288 - Introduces three-dimensional rotation by mouse using a variation on Ken Shoemake's ARCBALL - Provides a minimal Quaternion class, to avoid an additional dependency on a large package like 'numpy-quaternion'

- makes axes3d.Quaternion a private _Quaternion class - shortens as_cardan_angles() - adds two extra tests to test_axes3d::test_quaternion(): from_cardan_angles() should return a unit quaternion, and as_cardan_angles() should be insensitive to quaternion magnitude - updates "mplot3d View Angles" documentation (the mouse can control both azimuth, elevation, and roll; and matlab does have a roll angle nowadays) - put in a reference to quaternion multiplication using scalar and vector parts (wikipedia) - rename class method that constructs a quaternion from two vectors to `rotate_from_to()` - clarify docstring: "The quaternion for the shortest rotation from vector r1 to vector r2" - issue warning when vectors are anti-parallel: "shortest rotation is ambiguous" - construct a perpendicular vector for generic r2 == -r1 - add test case for anti-parallel vectors - add test for the warning - add reference to Ken Shoemake's arcball, in axes3d.py - point out that angles are in radians, not degrees, in quaternion class docstrings - in test_axes3d, add an import for axes3d._Quaternion, to avoid repetition - add Quaternion conjugate(), and tests for it - add Quaternion norm, and tests - add Quaternion normalize(), and tests - add Quaternion reciprocal(), and tests - add Quaternion division, and tests - add Quaternion rotate(vector), and a test

- change argument from 2 element numpy array to x, y - add type hints

doc/api/toolkits/mplot3d/view_angles.rst

timhoffm

I've added a line break back in that was lost and made the line too long.

Looks good now. Anybody can merge after CI pass. - Please squash before merging.

@MischaMegens2 Thanks for your contribution and the stamina to keep going through the somewhat lengthy review process. 🎉

MischaMegens2 · 2024-06-04T23:12:42Z

@MischaMegens2 Thanks for your contribution and the stamina to keep going through the somewhat lengthy review process. 🎉

It did get better :)

MischaMegens2 · 2024-06-04T23:19:11Z

Looks good now. Anybody can merge after CI pass. - Please squash before merging.

Not sure about the CI - is there still an issue?

scottshambaugh

This looks like it’s in a good spot now to me as well! Thanks @MischaMegens2 for accommodating our reviews.

I’ve been traveling the past 2 weeks so haven’t been able to actually check out the branch and try the behavior, but the videos look great and I’m excited to do so.

* Natural 3D rotation with mouse - Addresses Issue matplotlib#28288 - Introduces three-dimensional rotation by mouse using a variation on Ken Shoemake's ARCBALL - Provides a minimal Quaternion class, to avoid an additional dependency on a large package like 'numpy-quaternion' * Suggestions from reviewers - makes axes3d.Quaternion a private _Quaternion class - shortens as_cardan_angles() - adds two extra tests to test_axes3d::test_quaternion(): from_cardan_angles() should return a unit quaternion, and as_cardan_angles() should be insensitive to quaternion magnitude - updates "mplot3d View Angles" documentation (the mouse can control both azimuth, elevation, and roll; and matlab does have a roll angle nowadays) - put in a reference to quaternion multiplication using scalar and vector parts (wikipedia) - rename class method that constructs a quaternion from two vectors to `rotate_from_to()` - clarify docstring: "The quaternion for the shortest rotation from vector r1 to vector r2" - issue warning when vectors are anti-parallel: "shortest rotation is ambiguous" - construct a perpendicular vector for generic r2 == -r1 - add test case for anti-parallel vectors - add test for the warning - add reference to Ken Shoemake's arcball, in axes3d.py - point out that angles are in radians, not degrees, in quaternion class docstrings - in test_axes3d, add an import for axes3d._Quaternion, to avoid repetition - add Quaternion conjugate(), and tests for it - add Quaternion norm, and tests - add Quaternion normalize(), and tests - add Quaternion reciprocal(), and tests - add Quaternion division, and tests - add Quaternion rotate(vector), and a test * Update axes3d.py's arcball - change argument from 2 element numpy array to x, y - add type hints * Update doc/api/toolkits/mplot3d/view_angles.rst --------- Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

scottshambaugh · 2024-06-14T14:25:47Z

Got to play around with this finally, it feels good!

Without digging into the math to double check if the naming is right, it seems like this is matching up with "trackball" controls rather than "arcball" controls. See here for an example:

Personally I prefer the latter, since moving the mouse back the original point on the screen with arcball controls will return the view to the original view, whereas the trackball controls will seem to drift (ie, click and spin the mouse in a circle, and the view will precess in one direction or the other). But I know both are pretty common. @MischaMegens2 thoughts?

MischaMegens2 · 2024-06-14T18:16:35Z

Got to play around with this finally, it feels good!

Glad that you liked it...

Without digging into the math to double check if the naming is right, it seems like this is matching up with "trackball" controls rather than "arcball" controls.

Well most certainly it is not matching up with the "trackball", the matplotlib control is indeed an "arcball". Just try the examples once more, paying attention to how it responds, rather than to how it looks superficially (the matplotlib arcball does not show the circles, like the threejs arcball):

https://threejs.org/examples/#misc_controls_trackball

https://threejs.org/examples/#misc_controls_arcball

In particular: drag the mouse tangentially near an edge or corner; the trackball will still change the 'tilt' (similar to what happens near the center of the figure), whereas an arcball will rotate the scene ('roll').

Personally I prefer the latter, since moving the mouse back the original point on the screen with arcball controls will return the view to the original view, whereas the trackball controls will seem to drift (ie, click and spin the mouse in a circle, and the view will precess in one direction or the other). But I know both are pretty common. @MischaMegens2 thoughts?

Well the philosophy is different. One might argue that that 'precession' of the trackball is a feature, rather than a bug - a feature that is much needed to be able to adjust the roll at all. It is just unfortunate that the figure then rotates in the opposite direction of the circular motion of the mouse...

As for the math of it: the 'arcball' as implemented does not strictly follow the arcball recipe of Ken Shoemake; Ken's arcball uses vectors on the (unit) sphere to directly define a rotation quaternion q = v1 v2', and then rotates the figure using v_rotated = q v_original q'. As a consequence, it rotates by twice the angle subtended on the sphere. (This is particurlarly noticeable when adjusting the 'roll', near the edge.) In contrast, the matplotlib arcball takes a square root: q = sqrt(v1 v2'), avoiding the double-speed rotation.

scottshambaugh · 2024-06-16T01:07:03Z

The behavior I was trying to get at isn't so much the tilt vs roll at the edge of the screen (doesn't matter so much IMO), but the property that during a single mouse click, returning to the original position on the screen will restore the original view. The threejs arcball has this property, but the matplotlib arcball doesn't right now and precesses:

2024-06-15.19-01-52-1.mp4

MischaMegens2 · 2024-06-16T03:44:54Z

The behavior I was trying to get at is [...] that during a single mouse click, returning to the original position on the screen will restore the original view. The threejs arcball has this property, but the matplotlib arcball doesn't right now and precesses.

Aha! I think this is an unfortunate consequence of the sqrt() that I threw in against the double-speed. I can see that this might not be desirable, and that Ken's original arcball has its advantages.
Shall I add an rcParams after all, then? And we can throw in a trackball option, and Holroyd's arcball as well, for good measure.
I think you have me convinced.
New issue#, new PR?

scottshambaugh · 2024-06-16T04:14:54Z

Think it's up to you if you want to change the default behavior or add rcparams! Since this feature hasn't been released in v3.10.0 yet, there's pretty wide latitude to make changes before that release. Yeah, new issue & PR I would think.

github-actions bot added topic: mplot3d status: needs rebase and removed status: needs rebase labels May 24, 2024

MischaMegens2 mentioned this pull request May 26, 2024

[ENH]: Natural 3D rotation with mouse #28288

Closed

jklymak requested a review from scottshambaugh May 26, 2024 15:13

github-actions bot added the Documentation: API files in lib/ and doc/api label May 28, 2024

scottshambaugh requested changes May 29, 2024

View reviewed changes

MischaMegens2 force-pushed the avoid-gimbal-lock branch 2 times, most recently from 6c2267b to cdcc9d8 Compare June 3, 2024 18:36

MischaMegens2 requested a review from scottshambaugh June 3, 2024 19:35

timhoffm reviewed Jun 4, 2024

View reviewed changes

MischaMegens2 added 3 commits June 4, 2024 09:04

Natural 3D rotation with mouse

72774ca

- Addresses Issue matplotlib#28288 - Introduces three-dimensional rotation by mouse using a variation on Ken Shoemake's ARCBALL - Provides a minimal Quaternion class, to avoid an additional dependency on a large package like 'numpy-quaternion'

Update axes3d.py's arcball

a0c22ed

- change argument from 2 element numpy array to x, y - add type hints

github-actions bot added the Documentation: build building the docs label Jun 4, 2024

MischaMegens2 force-pushed the avoid-gimbal-lock branch from 654f8a3 to a0c22ed Compare June 4, 2024 16:56

github-actions bot removed the Documentation: build building the docs label Jun 4, 2024

timhoffm added this to the v3.10.0 milestone Jun 4, 2024

timhoffm reviewed Jun 4, 2024

View reviewed changes

doc/api/toolkits/mplot3d/view_angles.rst Outdated Show resolved Hide resolved

Update doc/api/toolkits/mplot3d/view_angles.rst

99a6d81

timhoffm approved these changes Jun 4, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

scottshambaugh approved these changes Jun 5, 2024

View reviewed changes

scottshambaugh merged commit 57e187a into matplotlib:main Jun 5, 2024
40 of 41 checks passed

MischaMegens2 deleted the avoid-gimbal-lock branch June 5, 2024 15:47

MischaMegens2 mentioned this pull request Jun 17, 2024

[ENH]: mplot3d mouse rotation style #28408

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce natural 3D rotation with mouse #28290

Introduce natural 3D rotation with mouse #28290

MischaMegens2 commented May 24, 2024 •

edited

Loading

MischaMegens2 commented May 26, 2024

jklymak commented May 26, 2024

MischaMegens2 commented May 26, 2024

MischaMegens2 commented May 26, 2024 •

edited

Loading

anntzer commented May 27, 2024

MischaMegens2 commented May 27, 2024

anntzer commented May 27, 2024 •

edited

Loading

MischaMegens2 commented May 28, 2024

scottshambaugh left a comment

scottshambaugh commented May 29, 2024 •

edited

Loading

scottshambaugh commented May 29, 2024 •

edited

Loading

MischaMegens2 commented May 30, 2024

MischaMegens2 commented May 30, 2024 •

edited

Loading

timhoffm commented May 30, 2024

MischaMegens2 commented Jun 3, 2024

timhoffm Jun 4, 2024

MischaMegens2 Jun 4, 2024 •

edited

Loading

MischaMegens2 Jun 4, 2024

timhoffm left a comment •

edited

Loading

This comment was marked as resolved.

MischaMegens2 commented Jun 4, 2024 •

edited

Loading

MischaMegens2 commented Jun 4, 2024

scottshambaugh left a comment

scottshambaugh commented Jun 14, 2024

MischaMegens2 commented Jun 14, 2024

scottshambaugh commented Jun 16, 2024 •

edited

Loading

MischaMegens2 commented Jun 16, 2024 •

edited

Loading

scottshambaugh commented Jun 16, 2024

Introduce natural 3D rotation with mouse #28290

Introduce natural 3D rotation with mouse #28290

Conversation

MischaMegens2 commented May 24, 2024 • edited Loading

PR summary

PR checklist

MischaMegens2 commented May 26, 2024

jklymak commented May 26, 2024

MischaMegens2 commented May 26, 2024

MischaMegens2 commented May 26, 2024 • edited Loading

anntzer commented May 27, 2024

MischaMegens2 commented May 27, 2024

anntzer commented May 27, 2024 • edited Loading

MischaMegens2 commented May 28, 2024

scottshambaugh left a comment

Choose a reason for hiding this comment

scottshambaugh commented May 29, 2024 • edited Loading

scottshambaugh commented May 29, 2024 • edited Loading

MischaMegens2 commented May 30, 2024

MischaMegens2 commented May 30, 2024 • edited Loading

timhoffm commented May 30, 2024

MischaMegens2 commented Jun 3, 2024

timhoffm Jun 4, 2024

Choose a reason for hiding this comment

MischaMegens2 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

MischaMegens2 Jun 4, 2024

Choose a reason for hiding this comment

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

MischaMegens2 commented Jun 4, 2024 • edited Loading

MischaMegens2 commented Jun 4, 2024

scottshambaugh left a comment

Choose a reason for hiding this comment

scottshambaugh commented Jun 14, 2024

MischaMegens2 commented Jun 14, 2024

scottshambaugh commented Jun 16, 2024 • edited Loading

MischaMegens2 commented Jun 16, 2024 • edited Loading

scottshambaugh commented Jun 16, 2024

MischaMegens2 commented May 24, 2024 •

edited

Loading

MischaMegens2 commented May 26, 2024 •

edited

Loading

anntzer commented May 27, 2024 •

edited

Loading

scottshambaugh commented May 29, 2024 •

edited

Loading

scottshambaugh commented May 29, 2024 •

edited

Loading

MischaMegens2 commented May 30, 2024 •

edited

Loading

MischaMegens2 Jun 4, 2024 •

edited

Loading

timhoffm left a comment •

edited

Loading

MischaMegens2 commented Jun 4, 2024 •

edited

Loading

scottshambaugh commented Jun 16, 2024 •

edited

Loading

MischaMegens2 commented Jun 16, 2024 •

edited

Loading