Skip to content

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

Draft
ericpre wants to merge 1 commit intomatplotlib:mainfrom
ericpre:fix_rotation_aspect_ratio
Draft

Fix rotation selector for aspect ratio of the axes values different from 1#21886
ericpre wants to merge 1 commit intomatplotlib:mainfrom
ericpre:fix_rotation_aspect_ratio

Conversation

@ericpre
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

dhomeier commented Dec 13, 2021

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
Copy Markdown
Member Author

ericpre commented Dec 13, 2021

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

@dhomeier
Copy link
Copy Markdown

dhomeier commented Dec 13, 2021

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
Copy Markdown
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
Copy Markdown

dhomeier commented Dec 13, 2021

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
Copy Markdown
Member

dstansby commented Dec 13, 2021

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

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

dstansby 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.

@ericpre
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

tacaswell commented Dec 16, 2022

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