Fix 3D rotation precession #28823

scottshambaugh · 2024-09-15T04:17:40Z

PR summary

#28290 introduced "arcball" style rotation to 3D plots, which IMO is a significant enhancement over the previous rotation style. However, there was a change to the original arcball implementation to slow down the speed of rotation, which had the inadvertent side effect that it caused the plot to precess when moving the mouse around. This removed the desirable property that returning the mouse to the same point on the screen restored the original view. See the comments at the end of that issue for some example videos.

This tweaks the math to have the best of both worlds - a 1:1 mapping between screen position and view angle during a single mouse click, and also scaling down the "double angle" high speed rotations to a "single angle" speed.

@MischaMegens2 could you please check the math and weigh in on if you think this makes sense to change for the default?

I think that since this behavior is new for 3.10, it makes sense to prioritize getting it in. The what's new note shouldn't have to change.

See also some more discussion in #28408.

Original arcball paper: https://graphicsinterface.org/wp-content/uploads/gi1992-18.pdf

PR checklist

[n/a] "closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
[n/a] Plotting related features are demonstrated in an example
[n/a] New Features and API Changes are noted with a directive and release note
[n/a] Documentation complies with general and docstring guidelines

scottshambaugh · 2024-09-15T16:36:34Z

Before, showing that returning to the original mouse position has a view that has precessed:

2024-09-15.10-32-43-1.mp4

After, showing that returning to the original mouse position recovers the original view:

2024-09-15.10-31-13-1.mp4

MischaMegens2 · 2024-09-16T03:42:44Z

@MischaMegens2 could you please check the math and weigh in on if you think this makes sense to change for the default?

I took a quick look at it and I don't think it is proper. Both proposed modifications miss the mark.
The first one is (additions in bold):
scale = np.sqrt(2)/2 # slow down the rate of rotation
current_vec = self._arcball(self._sx*scale/w, self._sy*scale/h)
new_vec = self._arcball(x*scale/w, y*scale/h)
The stated purpose is to 'slow down the rate of rotation'. That is not what it does - it shrinks the arcball instead.
(Incidentally, the arcball size is scaled inside the _arcball() method already: the first two lines of it read:
x *= 2; y *= 2
I don't see why you would want to introduce the extra scale; if the arcball scaling is not to your liking, why not modify it there...?)
The arcball size on screen leaves the rotation unchanged (you'd have to click closer to the center of the figure since the arcball has shrunk, but you will then achieve exactly the identical effect as before.) So the proposed change does not do what it is supposed to do.

The second proposed modification is:
q = dq * dq * q
i.e., the incremental rotation is now performed twice. This exacerbates the '"double angle" high speed rotations': instead of achieving a '"single angle" speed', with the proposed PR, we now have it quadrupled... worse! I don't think anyone is looking forward to that.

As for the movie clips, purporting to show that returning to the original mouse position now recovers the original view: you might well be deceived by the shrinking of the arcball - in the clips, the mouse is already somewhat near the edge, and with the shrinking in the 2nd clip, you probably ended up outside the arcball proper, on its border, and consequently, rotations are around a fixed axis (pointing towards you). Of course you return to the original view then.

Would you not rather wait for #28408 (comment), instead of trying to make changes haphazardly in a rush, without really understanding what you are doing?

MischaMegens2

I don't really see the benefit.

MischaMegens2 · 2024-09-16T03:51:16Z

lib/mpl_toolkits/mplot3d/axes3d.py

        azim = np.arctan2(2*(-qw*qz+qx*qy), qw*qw+qx*qx-qy*qy-qz*qz)
-        elev = np.arcsin( 2*( qw*qy+qz*qx)/(qw*qw+qx*qx+qy*qy+qz*qz))  # noqa E201
+        elev = np.arcsin(np.clip(2*(qx*qz + qw*qy), -1, 1))


But why (the np.clip)? I have a hard time imagining how you'd possibly ever get the arcsin argument out of range...

There were floating point round-off errors on the macos github CI runners, which was resulting in values inside the expression of 1.000000001 (for example) and leading to an error being thrown here. I'm not sure of a cleaner robust way to do this than clipping after norming above, but am open to suggestions.

All right, touché. I apologize for being so curt yesterday (it was not a good day for me). Let's see if we can make it work as good as we can.
Is there any chance we can extract the qw, qx, qy, and qz from the macos runners where the error shows up? And some of the intermediate results? Hopefully that will give some insight as to why we're confronted with this. (What hardware is used to do the calculations, what processor?)

Floating point inaccuracies are something that pop up fairly frequently for matplotlib's tests, and unfortunately we won't ever get perfectly reproducible results across architectures. It's why we use assert_allclose a lot of places in testing. That doesn't work in cases like this where you are close to a discrete threshold however, hence the clipping. The good news is that in 99% of cases for matplotlib, the end result is visual so floating point inaccuracies don't matter to the end user.

Well I understand the expediency of adding an np.clip() and moving on, it gets this job done; but I thought we had the IEEE 754 Standard for Floating-Point Arithmetic for quite a while now, to help make things like this reliable and portable. And it is not like we're doing something arcane: just addition, multiplication and division, and not even a whole lot of it. So I'm somewhat surprised by the incompatibility (why only on macos?). Seems you're on to something.

lib/mpl_toolkits/mplot3d/axes3d.py

timhoffm · 2024-09-16T08:39:10Z

@MischaMegens2 thanks for the review.

Would you not rather wait for #28408 (comment), instead of trying to make changes haphazardly in a rush, without really understanding what you are doing?

In my ears, this comes across a bit harsh. I'm sure this PR was well intended. 3.10 will be released in October and it would be great to have the rotation fixed there. Asking you on the status per your above comment would have been the right first step. But I have to admit, I myself had forgotten your comment on planning to fix this.

So to get things straight: We'd very much welcome if you could do the fix. Is there anything we can help you with to move forward? - You said you wanted to wait for #28395. My interpretation of the status is that we're waiting on your rewrite there? But given that it has become documentation-only, can we decouple this from rotation fix. I'd prioritize the fix higher.

scottshambaugh · 2024-09-16T16:08:19Z

Hi @MischaMegens2, addressing the points you have brought up:

The second proposed modification is:
q = dq * dq * q
i.e., the incremental rotation is now performed twice.

This is on purpose, to restore the original Shoemake arcball recipe by undoing the "sqrt" that was done to the rotation quaternion as you explained here #28290 (comment) (quoted below). This could be implemented in other ways but is necessary for the fixed behavior.

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.

The first one is (additions in bold):
scale = np.sqrt(2)/2 # slow down the rate of rotation
current_vec = self._arcball(self._sx*scale/w, self._sy*scale/h)
new_vec = self._arcball(x*scale/w, y*scale/h)
The stated purpose is to 'slow down the rate of rotation'. That is not what it does - it shrinks the arcball instead.

This scaling makes it so that when moving your mouse from the origin in pure x or y directions, the view angle change exactly matches the original behavior you added with the "sqrt" of the rotation quaternion. You can see this in the unchanged tests. This was done with the purpose of "avoiding the double-speed rotation" you called out. The sqrt(2)/2 is squared to 1/2 inside the _arcball function, which is where the doubling factor comes in.

(Incidentally, the arcball size is scaled inside the _arcball() method already: the first two lines of it read:
x *= 2; y *= 2
I don't see why you would want to introduce the extra scale; if the arcball scaling is not to your liking, why not modify it there...?)

I was trying to keep the math inside _arcball matching what's in the Shoemake paper (screenshot below). Our radius of 0.5 results in the factor of 2 in the code. IMO it makes more sense to pull the scaling out of the function, as I think the legibility of the functions is more important than saving multiplications.

As for the movie clips, purporting to show that returning to the original mouse position now recovers the original view: you might well be deceived by the shrinking of the arcball - in the clips, the mouse is already somewhat near the edge, and with the shrinking in the 2nd clip, you probably ended up outside the arcball proper, on its border, and consequently, rotations are around a fixed axis (pointing towards you). Of course you return to the original view then.

Nope, this works whatever your starting position and however you move your mouse. I encourage you to try it out locally!

Would you not rather wait for #28408 (comment)

My understanding of the proposed change was that it was to add the rcparam, not necessarily to fix the default rotation behavior. With 3.10 releasing soon and this new rotation mode going out to users for the first time, I thought a small fix may be appropriate since larger changes can take a while to review. But it's not set in stone, and getting your status and opinion is why I asked for your feedback. If that larger MR is ready I'd be happy to review it and we can discuss whether it makes sense to have this smaller one as a stepping stone at all.

instead of trying to make changes haphazardly in a rush, without really understanding what you are doing?

We expect all participants, regardless of their experience or position, to engage respectfully and assume good faith. Please focus on the technical merits of the proposal rather than making personal accusations. All contributors to matplotlib are expected to abide by our code of conduct.

Fix arcsin domain error Revert to simplified trig Linting tests

MischaMegens2 · 2024-09-19T07:12:26Z

So to get things straight: We'd very much welcome if you could do the fix. Is there anything we can help you with to move forward?

Hi @timhoffm, Hi @scottshambaugh :-
I've come up with a draft PR #28841 for the Issue #28408 ('adjustable mplot3d mouse rotation style'); it incorporates most of the changes proposed in the current PR ('fix 3D rotation precession'). It lets you choose what mouse rotation style you want (the traditional style; or an arcball, with a trade-off: with natural speed, but you pay the price: precession; or an arcball without precession, but you pay the price: double the angular rate; or a trackball).

It also makes the trackball/arcball size an rcparameter; originally, the size was hard-coded, and set a tad large; and then this PR actually grew it even larger (rather than shrinking, as I mistakenly thought initially). (This made the arcball almost work as a trackball.)

What is still absent is the introduction of an np.clip(), I have hesitations about that since I don't understand why it is needed (maybe it is something for the final-merge PR, if it really unavoidable).

In any case, could you take a look and let me know what you think about it?

timhoffm · 2024-09-19T08:00:30Z

Sorry, I'm out of this discussion. As stated I'm a rare and naive 3D user and want the rotation to "just work". From that perspective (which I assume is also that of many users) I would argue that configurability is not necessary and we could have just one fixed algorithm. There may be aspects that I overlook, which make configuration neccessary. But that's that's exactly the point why I leave the discussion to you experts.

MischaMegens2 · 2024-09-19T19:07:43Z

Hi @scottshambaugh,

It's up to the two of us then now, it looks like...
;)

scottshambaugh · 2024-09-23T15:14:21Z

After reviewing #28841, I think it would be most preferable for that to go in instead of this one. But if it's not ready before the v3.10 merge point, I think this PR should go in as a stop-gap to improve the default behavior in the new release.

Add np.clip() to avoid floating point round-off errors on the macos github CI runners, see also matplotlib#28823 (comment)

scottshambaugh · 2024-10-15T17:47:58Z

With #28841 merged in, this PR is no longer necessary. Closing.

scottshambaugh added the topic: mplot3d label Sep 15, 2024

scottshambaugh added this to the v3.10.0 milestone Sep 15, 2024

scottshambaugh force-pushed the 3d_rotation_tweak branch from 29e4c75 to 343b3dc Compare September 15, 2024 16:28

scottshambaugh force-pushed the 3d_rotation_tweak branch 4 times, most recently from 9939481 to e497c77 Compare September 16, 2024 01:41

MischaMegens2 reviewed Sep 16, 2024

View reviewed changes

Fix 3D rotation precession

f059296

Fix arcsin domain error Revert to simplified trig Linting tests

scottshambaugh force-pushed the 3d_rotation_tweak branch from a521ef7 to f059296 Compare September 17, 2024 02:11

QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Sep 18, 2024

MischaMegens2 mentioned this pull request Sep 19, 2024

Make mplot3d mouse rotation style adjustable #28841

Merged

4 tasks

MischaMegens2 added a commit to MischaMegens2/matplotlib that referenced this pull request Oct 10, 2024

np.clip() to avoid floating point round-off errors

6b15663

Add np.clip() to avoid floating point round-off errors on the macos github CI runners, see also matplotlib#28823 (comment)

scottshambaugh closed this Oct 15, 2024

QuLogic added the status: superseded label Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 3D rotation precession #28823

Fix 3D rotation precession #28823

scottshambaugh commented Sep 15, 2024 •

edited

Loading

scottshambaugh commented Sep 15, 2024

MischaMegens2 commented Sep 16, 2024

MischaMegens2 left a comment

MischaMegens2 Sep 16, 2024

scottshambaugh Sep 16, 2024 •

edited

Loading

MischaMegens2 Sep 16, 2024

scottshambaugh Sep 17, 2024

MischaMegens2 Sep 17, 2024

timhoffm commented Sep 16, 2024 •

edited

Loading

scottshambaugh commented Sep 16, 2024 •

edited

Loading

MischaMegens2 commented Sep 19, 2024

timhoffm commented Sep 19, 2024

MischaMegens2 commented Sep 19, 2024 •

edited

Loading

scottshambaugh commented Sep 23, 2024

scottshambaugh commented Oct 15, 2024

Fix 3D rotation precession #28823

Fix 3D rotation precession #28823

Conversation

scottshambaugh commented Sep 15, 2024 • edited Loading

PR summary

PR checklist

scottshambaugh commented Sep 15, 2024

MischaMegens2 commented Sep 16, 2024

MischaMegens2 left a comment

Choose a reason for hiding this comment

MischaMegens2 Sep 16, 2024

Choose a reason for hiding this comment

scottshambaugh Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

MischaMegens2 Sep 16, 2024

Choose a reason for hiding this comment

scottshambaugh Sep 17, 2024

Choose a reason for hiding this comment

MischaMegens2 Sep 17, 2024

Choose a reason for hiding this comment

timhoffm commented Sep 16, 2024 • edited Loading

scottshambaugh commented Sep 16, 2024 • edited Loading

MischaMegens2 commented Sep 19, 2024

timhoffm commented Sep 19, 2024

MischaMegens2 commented Sep 19, 2024 • edited Loading

scottshambaugh commented Sep 23, 2024

scottshambaugh commented Oct 15, 2024

scottshambaugh commented Sep 15, 2024 •

edited

Loading

scottshambaugh Sep 16, 2024 •

edited

Loading

timhoffm commented Sep 16, 2024 •

edited

Loading

scottshambaugh commented Sep 16, 2024 •

edited

Loading

MischaMegens2 commented Sep 19, 2024 •

edited

Loading