Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Make mplot3d mouse rotation style adjustable#28841

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

Conversation

MischaMegens2
Copy link
Contributor

PR summary

Makes the plot3d mouse rotation style adjustable (various styles);
addresses Issue#28408; an attempt to improve on PR#28236.

  • matplotlibrc: add axes3d.mouserotationstyle and axes3d.trackballsize
  • lib/matplotlib/rcsetup.py: add validation for axes3d.mouserotationstyle and axes3d.trackballsize
  • axes3d.py: implement various mouse rotation styles
  • update test_axes3d.py::test_rotate()
  • view_angles.rst: add documentation for the mouse rotation styles
  • update next_whats_new/mouse_rotation.rst

It also incorporates some of the improvements proposed in PR#28236; what it does not include (yet) is the introduction ofnp.clip() to circumventfloating point round-off errors on the macos github CI runners.

PR checklist

Addresses Issuematplotlib#28408- matplotlibrc: add axes3d.mouserotationstyle and axes3d.trackballsize- lib/matplotlib/rcsetup.py: add validation for axes3d.mouserotationstyle and axes3d.trackballsize- axes3d.py: implement various mouse rotation styles- update test_axes3d.py::test_rotate()- view_angles.rst: add documentation for the mouse rotation styles- update next_whats_new/mouse_rotation.rst
@MischaMegens2MischaMegens2 mentioned this pull requestSep 19, 2024
1 task
@scottshambaughscottshambaugh linked an issueSep 21, 2024 that may beclosed by this pull request
Copy link
Contributor

@scottshambaughscottshambaugh left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Left some comments on the code & docs, but I think this overall looks good.

The main discussion to have I think is whether we want to provide all these options. I can see uses for locking roll in the'azel' method, and I can see uses for having roll control (and agree that should be the default), but the four roll control option seems to accomplish pretty much the same thing. To that end,'Holroyd' seems strictly better than any of'trackball', 'arcball', 'Shoemake' - better than the first two because of the path independence, and better than'Shoemake' because of avoiding the "edges" in the control region that you andthe paper point out. It definitely feels the nicest to me experimenting with it locally.

Are there specific use cases that you see for the other three roll control methods? I think it would make more sense to provide users just the "best" options rather than all the possible options. Eliminates some sources of potential confusion, and less code reduces the maintenance burden.

@scottshambaughscottshambaugh added this to thev3.10.0 milestoneSep 22, 2024
@MischaMegens2
Copy link
ContributorAuthor

Are there specific use cases that you see for the other three roll control methods? I think it would make more sense to provide users just the "best" options rather than all the possible options. Eliminates some sources of potential confusion, and less code reduces the maintenance burden.

Well I imagine that different people have different preferences, or they already are used to something specific. Why force a particular approach on them, when it is easy enough to provide a choice? The maintenance burden is relatively minor, eliminating all the trackballs-but-one would only save a handful of lines.

@MischaMegens2MischaMegens2force-pushed themplot3d-mouse-rotation-style branch froma32a02a to133c916CompareSeptember 27, 2024 16:18
@MischaMegens2MischaMegens2 marked this pull request as ready for reviewSeptember 27, 2024 18:19
@MischaMegens2
Copy link
ContributorAuthor

@scottshambaugh - an updated PR is waiting for your review

MischaMegens2and others added5 commitsSeptember 30, 2024 22:22
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@scottshambaugh
Copy link
Contributor

Why force a particular approach on them, when it is easy enough to provide a choice? The maintenance burden is relatively minor, eliminating all the trackballs-but-one would only save a handful of lines.

I don't feel too strongly about this, and it wouldn't really hurt. I just don't immediately see any uses where the alternate rotation methods with roll control are valuable. Would appreciate if another maintainer could weigh in with their opinion.

I think we should definitely be very choosey about the default though - can keep talking about that in the comment chain higher up.

@timhoffm
Copy link
Member

Maintenance burden is only on side, and may not be too bad here. But options and choice are also mental load, both for users and maintainers. It makes a difference whether you have to understand two options or four. If there are good reasons/use cases for all the options, we should take them. If they are just added because we've found more mathematical ways to describe rotations, leave them out.

@MischaMegens2
Copy link
ContributorAuthor

MischaMegens2 commentedOct 3, 2024
edited
Loading

I'll make a proposal then: 1) we'll make the width of the transition region in@scottshambaugh 's proposed arcball variant adjustable, rather thanad hoc have it fixed at 60°. In this way, you can have either a) just a true arc ball (with zero transition width but and abrupt edge) or b) a smoothed edge one. The need for the funny hyperbolic Holroyd arcball then all but disappears, so 2) we drop that. This leaves us with 4 variants, each with a clear use case/reason for their existence:

  • the traditional 'azel', with its preferred axis (and no roll control)
  • 'trackball', with uniform response (good), and without an edge (also good); but still not much control over roll (some control, but only if you know the trick)
  • 'arcball', a virtual trackball, with easier control over roll (good), but a noticeable edge/non-uniformity (the price), and the 'path memory' (which you may actually like, since it is 'natural' in a way - as in the trackball, or you may dislike, since it complicates what's happening)
  • 'Shoemake', also with easy control over roll, and orientation independent of the path the mouse took (which you might value highly, or find unnatural), at the price of a disconnect between turning of the trackball and movement of the mouse along the rim of the trackball (the double angle thing)

And then anyone can pick their poison, fair is fair.

Gavin Bell appears to be the accurate reference/attribution, not Holroyd, replace it.
@@ -4021,6 +4042,7 @@ def as_cardan_angles(self):
"""
The inverse of `from_cardan_angles()`.
Note that the angles returned are in radians, not degrees.
The angles are not sensitive to the quaternion's norm().
"""
qw = self.scalar
qx, qy, qz = self.vector[..., :]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It won't let me add this comment to the unmodified line below, but in playing with this PR I do infrequently run into a domain error on arcsin due to floating point errors. A little hard to reproduce reliably, but it does pop up. Clipping the inside value to [-1, 1] should fix this.

/mnt/c/Users/Scott/Documents/Documents/Coding/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:4028: RuntimeWarning: invalid value encountered in arcsin  elev = np.arcsin( 2*( qw*qy+qz*qx)/(qw*qw+qx*qx+qy*qy+qz*qz))  # noqa E201posx and posy should be finite values

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

/mnt/c/Users/Scott/Documents/Documents/Coding/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:4028: RuntimeWarning: invalid value encountered in arcsin  elev = np.arcsin( 2*( qw*qy+qz*qx)/(qw*qw+qx*qx+qy*qy+qz*qz))  # noqa E201posx and posy should be finite values

I'm justso curious about the values of qw, qx, qy, and qz that cause this. I can understand theposx and posy should be finite values complaint, as a consequence of the arcsin argument getting out of range. But theinvalid value encountered in arcsin is still mysterious...
Could you try for me sometime replacing the offending statement with the following:

try:    elev = np.arcsin( 2*( qw*qy+qz*qx)/(qw*qw+qx*qx+qy*qy+qz*qz))  # noqa E201except:    print(repr(qw), repr(qx), repr(qy), repr(qz))    print(repr(qw*qy), repr(qz*qx), repr(qw*qw), repr(qx*qx), repr(qy*qy), repr(qz*qz) )    print(repr( 2*( qw*qy+qz*qx) ), repr(qw*qw+qx*qx+qy*qy+qz*qz) )    print(repr( 2*( qw*qy+qz*qx)/(qw*qw+qx*qx+qy*qy+qz*qz) ))

(I thought the error merits an attempt at diagnosing; not that I'm opposed to the np.clip(), I just thought it would be good to understand what is going on, how this comes about.)

Copy link
Contributor

@scottshambaughscottshambaughOct 8, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I just spent a few minutes trying to reproduce but couldn't - it's a tricky edge case to trigger. When I ran across it before, the value inside arcsin was only 1e-16 (or thereabouts) larger than 1, so it's just a result of numerical round-off. My guess would be a conditioning issue, where one of the values is very small relative to the others and gets rounded off to 0 in the denominator, but is big enough to still impact the numerator. I'll leave the debug lines in and let you know if it happens again, but am not concerned - this stuff end up happening fairly regularly across the codebase.

MischaMegens2 reacted with thumbs up emojiMischaMegens2 reacted with heart emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think the np.clip got missed in your latest commit, possible to add that quickly before merge?

Copy link
ContributorAuthor

@MischaMegens2MischaMegens2Oct 10, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think the np.clip got missed in your latest commit, possible to add that quickly before merge?

Oh sorry, of course; I put the np.clip() back in.

scottshambaugh reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks! Need to move the close paren to fix the test failure but the MR is looking good.

MischaMegens2 reacted with confused emoji
Copy link
ContributorAuthor

@MischaMegens2MischaMegens2Oct 10, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I just spent a few minutes trying to reproduce but couldn't - it's a tricky edge case to trigger.

You had mentioned before that there were floating point round-off errors on the macos github CI runners (#28823 (comment)); I think that was the original motivation to put the np.clip() in in the first place. Any chance we can trigger the edge case there once again?

(Not that I'm against the np.clip(), but I'm still surprised by the occurrence of the quotient >1. I thought the IEEE 754 standard for floating-point arithmetic requires that multiplication and addition should be correctly rounded, so I thought we would be in the clear... unless macos would not conform to IEEE 754, which would be also quite remarkable... Or I don't quite understand all of the correctly, and I should go read some moreKahan)

Copy link
Contributor

@scottshambaughscottshambaughOct 10, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If I happen to catch it again I'm happy to add it as a test! I think I lost the specific test case in the other MR that was erroring, and it was dependent on the specific math operations being performed so it likely wouldn't translate to this new code.

To be clear, I don't think it's a macos-specific problem, the issue is more that there isn't perfect determinism across different platforms / python versions / etc (even with perfect determinismwithin a configuration). Everything is being rounded "correctly", it's just that if we're right on the edge of floating point tolerance, complier/processor/implementation/optimization differences can have different results, and values infinitesimally on the wrong side of 1 will stack up unfavorably with further operations. For a simple concrete example, some systems in calculating(a + b)*c might distribute that multiplication toa*c + b*c, and those can result in different rounding of the results. Especially if for examplea >> b and sob doesn't make it into the mantissa ofa + b (addition and subtraction are not associative in floating point math, which is pretty unintuitive IMO).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[...] it's just that if we're right on the edge of floating point tolerance, complier/processor/implementation/optimization differences can have different results, and values infinitesimally on the wrong side of 1 will stack up unfavorably with further operations. For a simple concrete example, some systems in calculating(a + b)*c might distribute that multiplication toa*c + b*c, and those can result in different rounding of the results.

I'm just trying to wrap my head around what the optimization difference could have been that would lead to the observed result. '(a+b)c' -> 'ac + b*c' does not quite fit the bill... My hope is that an actual example would shed light on it...

@scottshambaugh
Copy link
Contributor

scottshambaugh commentedOct 7, 2024
edited
Loading

I can see how the "single speed" roll that tracks the mouse can be a useful property. I think that's a good argument for including it as an option! So I'm on board with your idea to have the four options of 'azel', 'trackball', 'arcball', and 'matplotlib' (Shoemake-like with the fillet, whatever we call it), and scrapping 'Holroyd'/'Bell'.

Unfortunately it is mutually exclusive with the path independence property. With our differing opinions on which is more intuitive to use (for what it's worth,Usability Analysis of 3D Rotation Techniques found people equally fast with trackball & shoemake), how to rank them? We need a default which will make life easiest for the 99% of users who won't touch this setting.

I think the strongest argument is that path independence allows for easily "undoing" a rotation that the user didn't intend, which fulfills theUX Principle of Forgiveness. It's very easy to otherwise get "lost" in rotation space, especially if your mouse accidentally leaves the axes box and reenters it elsewhere.

I also like that you can reach the full sphere of orientations with a single-click, rather than just the front hemisphere as you can with arcball. But that's a personal preference.

Edit: I did also happen to see this thread on twitter pop up today about several people complaining about 3D precession :)https://x.com/rms80/status/1843338041957687578

2024-10-06.20-21-34-1.mp4

@ksunden
Copy link
Member

@scottshambaugh What is your current opinion on this vs.#28823?

If the only sticking points here are default/docstring here then in discussing with@tacaswell and@QuLogic, we are comfortable sorting those out in follow up PRs during the RC (Though an issue to track that follow up would be good).

We have also said that we are comfortable with you doing a one review merge on this topic if you are satisfied.

- revise _arcball() to soften the edge, according to border width parameter- drop _arcball() 'style' function parameter- add trackballborder rcParam- _arcball() 'border' case: normalize, so result vector is on unit sphere-  math.sin/cos/sqrt -> np.sin/cos/sqrt: use numpy instead of math for consistency- remove 'Bell' style- rename arcball -> sphere, Shoemake -> arcball- update documentation- update test
@scottshambaugh
Copy link
Contributor

scottshambaugh commentedOct 10, 2024
edited
Loading

@MischaMegens2 thank you for those updates, I think this is good to go now! Appreciate the collaboration polishing this up. I think I'm going to try to write up a blog post on the new methods, since it doesn't look like there's been any innovation in these control methods over the past 20 years and I think we came up with some good ideas.

@ksunden I think this is good to merge now. If there's time pressure to get rc1 out, then I'm happy to solo-merge, but want to give other people a last chance to check it out. We can always refine docstrings down the line. I won't be able to make the weekly meeting today - maybe float it for a last look?

Add np.clip() to avoid floating point round-off errors on the macos github CI runners, see alsomatplotlib#28823 (comment)
@MischaMegens2
Copy link
ContributorAuthor

@MischaMegens2 thank you for those updates, I think this is good to go now! Appreciate the collaboration polishing this up.

Thank you too! Itdoes get considerably better with two pairs of eyes, looking at it :)

@MischaMegens2
Copy link
ContributorAuthor

Unfortunately it is mutually exclusive with the path independence property. With our differing opinions on which is more intuitive to use, how to rank them? We need a default which will make life easiest for the 99% of users who won't touch this setting.

I think the safest bet istrackball, it is easiest to understand, no surprises, it works the same in the middle and at the edge of the screen, no guesswork as to what it does.
As for me personally, I kind of likearcball. Hopefully, more advanced users already familiar with other mouse rotation methods will be able to find the option, once they start looking for it. (Your blog entry might help too!)
The question is then more: "Who do we cater to?" (If 99% of our users are already in the latter group, thensphere orarcball makes sense.)

Documentation (view_angles.rst):- Fix: only one of :: and .. code:: is needed- Fix: :rc:`axes3d.trackballsize`/2 + :rc:`axes3d.trackballborder` does not work as a formula, use  `trackballsize/2 + trackballborder` instead-Fix: $\sqrt 2 \approx 1.414$ now reads :math:`\sqrt 2 \approx 1.414`matplotlibrc:axes3d.mouserotationstyle: trackballThe style 'trackball' as default is likely to make life easier for 99% of the users who don't touch this setting (it is easiest to understand, no surprises, it works the same in the middle and at the edge of the screen, no guesswork as to how it works/what it does).
@MischaMegens2
Copy link
ContributorAuthor

@scottshambaugh: There were still a few glitches in the view_angles.rst documentation, I've tried to fix them; and I took the liberty to set the default style totrackball - but if you really think that that is a terrible idea then we can revert that...

@scottshambaugh
Copy link
Contributor

Ireally don't think having a default with hysteresis is a good user experience. If you think arcball is too confusing for users then I would rather keep the default as azel.

Change default mouse rotation style to arcball
@MischaMegens2
Copy link
ContributorAuthor

MischaMegens2 commentedOct 13, 2024
edited
Loading

Ireally don't think having a default with hysteresis is a good user experience.

All right, then let's keep the arcball, it is a nice idea, and it is better not to underestimate our users ;)
I reverted the style change.
Now there is all the more reason for that blog then, what are your plans?

- single ` -> double ``- add description of the border (circular arc instead of hyperbola)
@scottshambaugh
Copy link
Contributor

scottshambaugh commentedOct 15, 2024
edited
Loading

Remaining test failures are unrelated to this change, all comments have been resolved,@ksunden has given the go-ahead for a solo-review merge in the interest of getting this into 3.10, and I pinged the devs inthe gitter channel last week to make sure people had a chance to review. With all that resolved I am merging this in - thanks again@MischaMegens2 for the work that went into the new 3d view control methods!

I started putting together a skeleton of a blog post but it's going to take some time to flesh out. Will definitely let you know when it's done.

MischaMegens2 reacted with thumbs up emoji

@scottshambaughscottshambaugh merged commit77cbe08 intomatplotlib:mainOct 15, 2024
38 of 42 checks passed
@scottshambaugh
Copy link
Contributor

@MischaMegens2 I finished up my post doing a deep dive into virtual trackballs, curious to hear your thoughts!
https://theshamblog.com/virtual-trackballs-a-taxonomy-and-new-method/

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@timhoffmtimhoffmtimhoffm left review comments

@scottshambaughscottshambaughscottshambaugh approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

[ENH]: mplot3d mouse rotation style
5 participants
@MischaMegens2@scottshambaugh@timhoffm@ksunden@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp