Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b983b4e
to4a86576
CompareThis looks very promising! |
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. |
Fix drawing a new boxFix minspan calculations in data coordsFix resize logicSave center on press for rotationPEP8 fixUpdate what's new
dhomeier commentedJan 24, 2022
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? |
dhomeier commentedJan 24, 2022
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 indisplay 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. |
dhomeier commentedDec 19, 2022
I am planning to work on this along withastropy/regions#390, but certainly won't be able to start before next year. |
Uh oh!
There was an error while loading.Please reload this page.
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).