Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix centre and square state and add rotation for rectangle selector#20839
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
Fix centre and square state and add rotation for rectangle selector#20839
Uh oh!
There was an error while loading.Please reload this page.
Conversation
86511fe
to15fbb09
Compare0f2e907
to2a68321
Compare@dstansby, since this PR add selector rotation, would you be interested in having a look/reviewing it? The approach used in this PR is slightly different: support for rotation around around the centre is added to the Rectangle patch, which simplifies the changes and also match the behaviour of the ellipse patch. It works with aspect ratio != 1 and the rotation is done around the centre. For the aspect ratio != 1, the default is to use figure coordinates - data coordinates can selected using the |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
2a68321
to0cb15cb
CompareThanks@timhoffm, I addressed your comments. |
Now the PR title needs updating. |
Sorry, I update the description of this PR but forgot to update the title! |
0cb15cb
to58bf650
CompareRebase to fix the merge conflicts. |
58bf650
to4102b08
Compare4102b08
to3816a92
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I haven't had time to go through everything, but left some comments and questions that are hopefully helpful!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Thanks@dstansby for the review, I shall come back to this PR beginning of Oct to address the comments. |
Moved to draft for now. |
3816a92
to03648c8
CompareRebased and addressed@dstansby's review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The PR has grown to a size that is difficult to review. I did not have time to go through the changes inwidgets.py
andtest_widgets.py
yet. Part of the problem is that the PR does many things (c.f. the bullet list in the first post).
Review effort scales super-linearly with PR size. So many smaller PRs are better than one large PR.@ericpre is there a chance you can extract some changes into small separate PRs to make review easier?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cdfd423
toc394be8
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/widgets.py Outdated
if 'data_coordinates' in state: | ||
refx, refy = dx, dy | ||
else: | ||
refx = event.xdata / (eventpress.xdata + 1e-6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you sure 1e-6 is a universal solution? Caneventpress.xdata
not become -1e-6?
Maybe better to
xdata_press = eventpress.xdata if eventpress.xdata != 0 else 1-e6refx = event.xdata / xdata_press
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
The `~matplotlib.widgets.RectangleSelector` and | ||
`~matplotlib.widgets.EllipseSelector` can now be rotated interactively between | ||
0 and 45°. The rotation is enabled or disable by striking the *r* key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
0 and 45°. The rotation is enabled ordisable by striking the *r* key | |
0 and 45°. The rotation is enabled ordisabled by striking the *r* key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should give a reason for these somewhat particular limits. Please add a sentence:
The range limits are currently dictated by the implementation.
or similar.
One a side note: Shouldn't it be possible to cover the range -45°...45°, at least there the corner order does not change. One only has to add a sign, which may be possible to infer from the mouse vs start_point location. But this does not have to be in this PR. It's already very big and almost ready to go in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea, I change to -45 to 45 because it is a easy change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That would be great as it allowed at least to cover the entire range of orientations with some axis tricks!
Could you also add a note what specially was not working with larger angles, so this might be considered in a follow-up as discussed in#20839 (comment)?
In particular, did the problems occur only with specific backends? As I mentioned, I did not notice any serious problems with up to 90 (and in fact now tested up to ±180˚, which effectively allows any number of full rotations). The only obvious issue was, on subsequent resizing of the rotated rectangle, the centre seems to start moving on some kind of trochoid, but whole weird-looking behaviour on resize can of course be avoided by clicking to'center'
state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you also add a note what specially was not working with larger angles, so this might be considered in a follow-up as discussed in#20839 (comment)?
I don't remember but I would expect the value ofself._active_handle
to be incorrect when rotated more than -+45, e.g.'W
is picked while it should be aS
, etc.
The only obvious issue was, on subsequent resizing of the rotated rectangle, the centre seems to start moving on some kind of trochoid, but whole weird-looking behaviour on resize can of course be avoided by clicking to 'center' state.
I have noticed it recently and I think this is related to_get_aspect_ratio
and it would be good to improve it in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't remember but I would expect the value of self._active_handle to be incorrect when rotated more than -+45, e.g. 'W is picked while it should be a S, etc.
Thanks; this will be a very useful starting point! Yes, I would expect that the point originally labelledSW
could end up somewhere on theE
side etc., but could not figure out where that would cause trouble. Any other hints what to watch out for with a follow-up are definitely appreciated.
The motion I think is inevitably somewhat confusing as the fix point for resizing is not rotated along with the bounding box, but that seems quite tricky to address. It is certainly recommendable to use'center'
mode for resizing of the rotated shapes.
Uh oh!
There was an error while loading.Please reload this page.
6a4abf3
to56710f5
CompareThanks@ericpre this is a useful addition! |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
data_coordinates
state to define if the square ratio should be calculated in data or figure coordinatesRectangleSelector
andEllipseSelector
taking into account aspect ratio of the axes and using the center of the shape as rotation center.I would expect that this PR conflicts with#19864 and I would be interested to take over#19864 to rebase and fix the outstanding issues.The selector rotation is added in this PR.PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).