-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Improve RectangleSelector rotation #21945
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b983b4e
to
4a86576
Compare
4a86576
to
1a5d9bd
Compare
This looks very promising! |
1a5d9bd
to
6f763fa
Compare
6f763fa
to
0aedc3a
Compare
This is ready for review! |
Confused about the status here - is it waiting, or is it ready? I'll assume waiting and set to draft, but feel free to pop back on queue. |
Best to sort out #21977 first before continuing on this PR. |
45b5625
to
4d8da12
Compare
Fix drawing a new box Fix minspan calculations in data coords Fix resize logic Save center on press for rotation PEP8 fix Update what's new
caaf0ae
to
baea572
Compare
Looking at it from the data side, so to speak, I would rather give preference to conserving area in data space – holding width and height fixed in data coords would of course be a sufficient, but perhaps not necessary condition for that. |
What are your thoughts in the instance where the selector is rotated, where it's impossible to keep the selector rectangular (ie. have right angles as corners in display coordinates) and conserve both width, height in data coordinates? |
Intuitively it certainly makes interaction easier if angles and aspect ratio are conserved in display coordinates. |
@dstansby have you considered making a simpler "RotatingRectangleSelector" that is mostly meant for the fixed-aspect ratio case? I really don't see the point of rotating a rectangle if the data dimensions are different in x and y, so this is really mostly for the image processing case?. If you spin it, it can have whatever whacky behaviour it wants if the data aspect is not 1:1 without introducing confusing behaviour in the normal RectangleSelector? |
I am not adverse to doing that, but as it currently stands in the main branch (from #20839) it is currently possible to rotate a rectangle with axes aspect!=1 and the rectangle stays rectangular in display coordinates, not data coordinates. This PR removes limitations on that approach and fixes bugs with it. I am happy to either:
Either will take significant effort though, so it would be good to know which of these options has support! Personally I think it makes sense to have the rectangle always rectangular in display coords (this PR), but am not against creating a new selector to do this. |
@ericpre @anntzer both of your names come to mind to comment on how to proceed here... I can't see ever using this, so I don't have a strong opinion about what is supposed to happen if we rotate a rectangle selector in non one-to-one aspect space. It seems ill defined. Maybe the solution is to not offer that option if the aspect ratio is not 1:1? |
I added to the weekly meeting this week. However @dstansby if you would like to attend to discuss we can defer until you are available. Meetings are Thu 19:00 UTC : https://hackmd.io/49a-u44CTja02xQRaG88JA |
We discussed on the meeting, and the consensus that the only sane thing is for the rectangle to stay a rectangle in display coordinates. Anything else doesn't work in any sort of non-cartesian co-ordinate system. It also seems that resizing the window, etc is a bit beyond the point. If you make a selection hopefully something happens with that selection before any other GUI actions are made. I guess that depends not he use case, but it seems that GUI rectangles can't be expected to respond to window resizing, and anything built on the widget should deal with it another way. |
thanks for discussing! I'm afraid I don't have the time to dig this up at the moment, as you can probably see it's quite a complex PR! |
@dstansby What is the state / fate of this? |
I think the status is it should definitely go in, but the rebase needs handling, and the what's new and/or bugfixes probably need updating or adding. I think one of @keflavich or @dhomeier might have some funding from astropy to get this into a mergeable state? Sorry if I'm wrong there, but if I'm right would one of you be happy picking this PR up? |
There is funding in astropy to do this, but I can't put in the effort. Someone from Aperio, possibly @dhomeier, may be able to but we haven't worked out details yet. |
I am planning to work on this along with astropy/regions#390, but certainly won't be able to start before next year. |
PR Summary
This modifies
RectangleSelector
andEllipseSelector
to be drawn in display coordinates instead of display coordinates. This has a number of advantages:_aspect_ratio_correction
.Code to test:
Fixes #21937
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).