Skip to content

Fix rotation selector for aspect ratio of the axes values different from 1 #21886

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix rotation selector for aspect ratio of the axes values different from 1 #21886

wants to merge 1 commit into from
+26 −2

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Dec 8, 2021

PR Summary

Rotate counterclockwise to notice that the selector rotate too slowly because of the aspect ratio value difference

import matplotlib.pyplot as plt
from matplotlib.widgets import RectangleSelector, PolygonSelector
import numpy as np

data = np.random.random(size=(24, 32))

fig, ax = plt.subplots()
ax.plot([10, 20, 30], [1, 2, 3])

def dummy(*args):
    pass
tool = RectangleSelector(ax, dummy, interactive=True, drag_from_anywhere=True)
tool.extents = (12, 20, 1.0, 1.6)
tool.add_state('rotate')

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [n/a] New features are documented, with examples if plot related.
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@ericpre ericpre changed the title Fix rotation for aspect ratio of the axes values different from 1 Fix rotation selector for aspect ratio of the axes values different from 1 Dec 8, 2021
@dstansby
Copy link
Member

dstansby commented Dec 9, 2021

This does fix the slow rotation issue for me, but only partially. I can't seem to rotate the selector past +-45 degrees however.

@ericpre
Copy link
Member Author

ericpre commented Dec 9, 2021

This does fix the slow rotation issue for me, but only partially. I can't seem to rotate the selector past +-45 degrees however.

The limitation to +-45 degrees rotation is expected to avoid issues with the handle assignment.

@dstansby dstansby added this to the v3.6.0 milestone Dec 10, 2021
@dhomeier
Copy link

Fixes the angle synchronisation for me as well.

The limitation to +-45 degrees rotation is expected to avoid issues with the handle assignment.

Do you recall what specifically happens, or how to reproduce those issues? I could not notice much when removing the 45˚ limit; only when approaching 180˚ the rotation seems to become discontinuous.
I was also wondering how the handle assignments change in comparison when "flipping" the widget by resizing, i.e. set the center state and drag the handle past the central point.

@ericpre
Copy link
Member Author

ericpre commented Dec 13, 2021

Sorry, I don't know more what I already mentioned in #20839 (comment).

@dhomeier
Copy link

I can confirm your expectation on the self._active_handle – when tracking it e.g. through the onselect function

def onselect(epress, erelease=None):
    print(tool._active_handle, tool.rotation)

a given corner handle keeps its label through any number of rotations, whereas e.g. inverting the rectangle – click the SW corner and drag it to the opposite position at (20.0, 1.6) with ctrl pressed, it remains SW through this operation, but becomes NE on the next event. So in this case it is obviously possible to update the handle labels, but I haven't found yet if the same could be done for abs(self.rotation) > 45 (which would also need investigation if the angle should then be reset to self.rotation % 45...).
The main practical problem I have found so far is that the resizing becomes more and more unintuitive through the trochoidal reflex motion of the centre handle, though personally I do not find that problematic up to 90˚ angles.

@ericpre
Copy link
Member Author

ericpre commented Dec 13, 2021

@dhomeier, is it worth discussing this in a dedicated issue or PR? If not, the discussion will get lost as this PR is not about increasing the rotation limits.

@dhomeier
Copy link

I saw that @dstansby mentioned #21921 as part of an effort to extend the rotation range, but have not found an associated issue for that.

@dstansby
Copy link
Member

Yes, lets move to another issue - I'll open one.

@dhomeier dhomeier mentioned this pull request Dec 13, 2021
Open
@dstansby
Copy link
Member

I now have a working version of RectangleSelector that removes the 45 degree limitation, and also fixes scaling using an edge handle when rotated. It needs a bit more work to iron out (and add some more!) tests, but I think it's worth holding off on this for now since it would be redundant with my newer version.

@ericpre
Copy link
Member Author

ericpre commented Dec 14, 2021

I now have a working version of RectangleSelector that removes the 45 degree limitation, and also fixes scaling using an edge handle when rotated. It needs a bit more work to iron out (and add some more!) tests, but I think it's worth holding off on this for now since it would be redundant with my newer version.

Yes, makes sense, I am happy to give an early review, even if this is not finish yet! :)

@jklymak
Copy link
Member

jklymak commented Dec 16, 2021

I guess I'm not sure about merging this? Feel free to do so if you think it doesn't need a Whats new entry, and it is consistent with the current state of the docs...

@ericpre
Copy link
Member Author

ericpre commented Dec 16, 2021

It makes sense to put this on hold for now, because it will not be necessary after #21945 is merged and #21945 is already in good shape.

@ericpre ericpre marked this pull request as draft December 16, 2021 10:12
@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022
@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 16, 2022
@tacaswell
Copy link
Member

Pushing to 3.8 as the last comment suggests waiting for #21945

@ksunden ksunden modified the milestones: v3.8.0, future releases Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
status: needs rebase status: waiting for other PR topic: widgets/UI
Projects
None yet
Milestone
future releases
Development

Successfully merging this pull request may close these issues.
7 participants
@ericpre @dstansby @dhomeier @jklymak @tacaswell @QuLogic @ksunden