Skip to content
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

Preserve rotation angle under dragging for ellipse and rectangle selection tools #396

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Aug 28, 2023

In Jdaviz, you can rotate ellipse and rectangle ROIs using Subset Tools plugin. After that is done, and user wants to click and drag the shape, without this patch, the rotation resets. This patch preserves any existing rotation. Since I really don't want to implement theta support in JS (e.g., https://github.com/glue-viz/bqplot-image-gl/blob/main/js/lib/BrushEllipseSelector.js), during the dragging phase, the gray shape has no rotation, but the rotation comes back when you finalize the drag.

cc @astrofrog and @dhomeier

🐱

and rectangle selection tools without touching JS
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage: 71.42% and project coverage change: -0.50% ⚠️

Comparison is base (c98cb9d) 87.33% compared to head (4f62514) 86.84%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   87.33%   86.84%   -0.50%     
==========================================
  Files          89       89              
  Lines        5013     5076      +63     
==========================================
+ Hits         4378     4408      +30     
- Misses        635      668      +33     
Files Changed Coverage Δ
glue_jupyter/bqplot/common/tools.py 66.80% <71.42%> (-3.34%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dhomeier dhomeier added enhancement New feature or request bqplot-viewers ui labels Aug 30, 2023
Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad again that moving cannot be tested currently, but otherwise this looks good – as mentioned, I am unable to check if this works for Rectangle, but for Ellipse it does as expected, so I'll trust that the former is solved for you as well.

glue_jupyter/bqplot/common/tools.py Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor Author

pllim commented Aug 30, 2023

Try test it over in Imviz. Install this branch, and then install Jdaviz, and test it there.

@dhomeier
Copy link
Contributor

Try test it over in Imviz. Install this branch, and then install Jdaviz, and test it there.

That's what I did, or do you mean installing in different order makes any difference?
Seems totally non-deterministic anyway – at first moving did not update properly in the regular notebook, but worked in Jupyterlab; now it's functioning normally in the standard notebook, but there is no way to access roi.rotate_by, while in Imviz the rectangles (and only rectangles) can't be moved at all...

@pllim
Copy link
Contributor Author

pllim commented Aug 30, 2023

I think as long as you can prove to yourself using ellipse, it is good enough? The logic is the same in both. If you want to, we can meet at some point and I can screenshare, but not today.

Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I could confirm it in the standalone app, and then after relaunching everything, moving worked in the notebook as well!

@dhomeier dhomeier changed the title Preserve rotation angle for ellipse and rectangle selection tools Preserve rotation angle under dragging for ellipse and rectangle selection tools Aug 31, 2023
@dhomeier dhomeier merged commit 33afdfb into glue-viz:main Aug 31, 2023
21 of 23 checks passed
@pllim pllim deleted the you-spin-me-right-round-right-round branch August 31, 2023 20:10
@pllim
Copy link
Contributor Author

pllim commented Jan 2, 2024

I created an issue upstream for the confusing "shadow." FYI. glue-viz/bqplot-image-gl#106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants