feat: Add ROI selection tool

[gselzer]

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.

ROIPrototype.mp4

TODO/Wishlist

Backend

• 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

• 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...
• Write tests
• Clean up code, format, resolve TODOs

[codecov]

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 (436fa7c).
Report is 74 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ndv/viewer/_backends/_vispy.py	68.24%	74 Missing ⚠️
src/ndv/viewer/_backends/_pygfx.py	68.72%	71 Missing ⚠️
src/ndv/viewer/_viewer.py	41.93%	54 Missing ⚠️

Additional details and impacted files

@@            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     

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

[tlambert03]

Super clear ❤️. I like where this is going

[tlambert03]

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]

i like where this is going. The behavior is lovely.

I want to spend a little more time thinking about the implementation. It does "get the thing done", but it's mixing together some concerns that i would prefer to keep separate (in an ideal world).

As we discussed, I think much of it would be made cleaner (and easier on your end) if ndv provided you more existing infrastructure to communicate mouse clicks and canvas coordinates to the backends. So, if it's ok with you, i'd like to pause and think about that bit, specifically with this PR in mind.

[tlambert03]

😂

[tlambert03]

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:

Untitled.mov

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

[tlambert03]

yes, I'm very excited about how the _viewer.py looks here!

I know i still need to dig into the backend implementations, but how are you feeling about all this? As far as you're concerned is it just waiting for review? I want to have this in :)

[tlambert03]

would be fun to think of ways to overload this method, so you could also pass something like any indexing expression (even something like np.s_[1:20, 30:60]) and get an roi.

also.. I expected the to see something about the number of vertices allowed, either in the type hint or the docs. I eventually found later in the vispy code:

        if len(vertices) != 4 or any(len(v) != 2 for v in vertices):
            raise Exception("Only 2D rectangles are currently supported")

should that be enforced here for now? or is there an alternate path that does currently support other than 4

[gselzer]

would be fun to think of ways to overload this method, so you could also pass something like any indexing expression (even something like np.s_[1:20, 30:60]) and get an roi.

Sure! I can't say I have the experience to know what would be nice here, though...

also.. I expected the to see something about the number of vertices allowed, either in the type hint or the docs. I eventually found later in the vispy code:

        if len(vertices) != 4 or any(len(v) != 2 for v in vertices):
            raise Exception("Only 2D rectangles are currently supported")

should that be enforced here for now? or is there an alternate path that does currently support other than 4

I personally prefer the check within the backend(s), because I wouldn't be surprised if some backends could support more ROI shapes than others later on. We could make the error message more specific, though...

Since we're talking about this method, do you think that the specification is general enough? For example, if we later want elliptical ROIs, how could we use viewer.set_roi to accomplish that? Similarly, how might we differentiate between polygons and polylines?

[gselzer]

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.

[tlambert03]

going in! Thank you so much for all the work on this @gselzer ... really great stuff.