feat: Add ROI selection tool

gselzer commented 3 months ago

This PR adds tooling for drawing (rectangular) ROIs onto the NDViewer. There are still many things left to do (list follows below), but I'm writing this PR to foster discussion and revision.

https://github.com/pyapp-kit/ndv/assets/29754838/1e5439c1-b3cb-4e54-ab51-dfd343b94f04

TODO/Wishlist

Backend

[x] Write a pygfx implementation

Features

[ ] Color widget to alter color as needed for the image. But there's a lot of unknowns here. @tlambert03 is there any color widget in any of the pyapp-kit repos?
[ ] Different ROI types (ellipse, polyline, polygon, ...) might be nice.
[ ] Provide API to restrict to a certain maximum number of ROIs. For example, you might only want the user to draw one ROI, and for the old ROI to be deleted when a new ROI is drawn.
[ ] For the usages, it would be nice for the viewer to expose an event that fires whenever a ROI changes, if that ROI is being used for processing somehow. We likely want a similar event for when the data is edited.

Bugs

[x] Currently, it is very hard for me to select the ROI, because the ROI can only be selected when you're right on the border. Notably, this only happens when the rectangle is fully transparent. We should figure out whether VisPy provides any aid to make selection easier (there is canvas.visuals_at, but I haven't investigated. I'm not sure how it handles order, either)
[ ] Editing the ROI shows a low FPS, significantly lower than when the camera is moved around. That may just be solved by the next point, and maybe also isn't worth worrying about right now...
[x] Write tests
[ ] Clean up code, format, resolve TODOs

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 65.45139% with 199 lines in your changes missing coverage. Please review.

Project coverage is 77.77%. Comparing base (356bb94) to head (0fad530).

Files	Patch %	Lines
src/ndv/viewer/_backends/_vispy.py	68.24%	74 Missing :warning:
src/ndv/viewer/_backends/_pygfx.py	68.72%	71 Missing :warning:
src/ndv/viewer/_viewer.py	41.93%	54 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #23 +/- ## ========================================== - Coverage 83.24% 77.77% -5.47% ========================================== Files 13 13 Lines 1259 1813 +554 ========================================== + Hits 1048 1410 +362 - Misses 211 403 +192 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tlambert03 commented 3 months ago

writing future wishes as I have them here. don't need to be in this PR:

[ ] option to keep ROIs aligned to data grid (like fiji behavior, to make it very clear which pixels are in the selection)

tlambert03 commented 3 months ago

wow, this is really shaping up nicely!

just pulled and played with it. really feels nice, and I think I like the new CanvasElement protocol a lot.

Still digging through it now, but could you add docstrings to _protocols.CanvasElement.start_move and move? (really, I should have docstrings on all the methods defined in there, making it clear what a backend is supposed to accept and return to satisfy the method). This thought came while trying to determine exactly what start_move vs move gives me

also: amazing that you have the pygfx backend working too. I did see a slight inconsistency with the handle box. In pygfx, the "threshold" for how far your mouse can be from the cursor seems to be in data coordinates rather than canavs coords (or something like that) ... if you zoom in, it gets too easy to select the handle:

https://github.com/pyapp-kit/ndv/assets/1609449/6b835a48-a8ba-44b5-b067-32bf1df43b4a

Also, side-note, they look nicer now in pygfx 0.3.0 👍

gselzer commented 3 months ago

just pulled and played with it. really feels nice, and I think I like the new CanvasElement protocol a lot.

🥳

Still digging through it now, but could you add docstrings to _protocols.CanvasElement.start_move and move? (really, I should have docstrings on all the methods defined in there, making it clear what a backend is supposed to accept and return to satisfy the method). This thought came while trying to determine exactly what start_move vs move gives me

Added!

also: amazing that you have the pygfx backend working too. I did see a slight inconsistency with the handle box. In pygfx, the "threshold" for how far your mouse can be from the cursor seems to be in data coordinates rather than canavs coords (or something like that) ... if you zoom in, it gets too easy to select the handle:

Yup, as I mentioned privately there's a FIXME here about that. As far as I can tell, pygfx defines Point size by specifying the diameter of points in pixels. The problem is that the math for determining handle selection assumes canvas position and pixels are similarly scaled, which breaks down upon zooming. It's likely that with smarter math, or with a working Renderer.get_pick_info, we could fix this.

pyapp-kit / ndv