Add view ray and labels selection in 3D

[kevinyamauchi]

Description

In this PR, @alisterburt and I add the ability to select objects in 3D. Following the pattern suggested by @sofroniewn here, we:

• modify the layer. _map_canvas2world to correctly map the canvas coordinates to world coordinates for 3D data. Previously, this method used the vispy camera transform. However, I think this may not be the correct transform between canvas and world coordinates for 3D, but based on my experimentation and this comment.
• add the view orientation in world coordinates to the mouse event in layer._process_mouse_event()
• add a method to transform the orientation vector from world coordinates to layer data coordinates (see here)
• add method to base layer to find intersections of the view direction vector with the axis-aligned bounding box of the displayed data (using layer extents)
• add a method to the labels base layer to find the first non-background voxel encounted along the layer data view ray ("sample ray").

To show the implementation, we have a example script with a layer that has rotation, shear, and scale applied (see below)

Example code

  """
Display two labels layers in 3D to demonstrate interactive 3D functionality
"""

import numpy as np
import napari



volume = np.zeros(shape=(64, 64, 64), dtype=int)
volume[0:5, 0:5, 0:5] = 1
volume[0:5, 0:5, 59:64] = 2
volume[0:5, 59:64, 0:5] = 3
volume[0:5, 59:64, 59:64] = 4
volume[59:64, 0:5, 0:5] = 5
volume[59:64, 0:5, 59:64] = 6
volume[59:64, 59:64, 0:5] = 7
volume[59:64, 59:64, 59:64] = 8
volume[29:34, 29:34, 29:34] = 9

viewer = napari.Viewer()
volume_1 = viewer.add_labels(volume, name='volume 1')
volume_2 = viewer.add_labels(
  volume,
  name='volume 2',
  scale=(3, 2, 1),
  shear=(0, 0.1, 0),
  translate=(64, 0, 0),
  rotate=(90, 0, 0)
)


def click_drag(layer, event):
  near_point, far_point = layer._cursor_ray(event)

  if (len(near_point) > 0) and (len(far_point) > 0):
      print(near_point, far_point)
      value = layer._get_value_ray(near_point, far_point)

      if value is not None:
          print(f'selected label: {value}')

volume_1.mouse_drag_callbacks.append(click_drag)
volume_2.mouse_drag_callbacks.append(click_drag)

napari.run()

label_selection.mov

Some things to improve:

• currently, the algorithm we use to construct the ray passing through the data cube requires the transform mapping canvase coordinates to world coordinates. Currently, this is not available in the layer and it would be nice if it was because it makes it difficult to do interaction in 3D. Fixed, see here
• we aren't totally sure about where to distribute the various functions (e.g., on the viewer vs. on the layer vs. utility functions).

Type of change

• Bug-fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

References

Closes #2653

How has this been tested?

• example: the test suite for my feature covers cases x, y, and z
• example: all tests pass with my change

Final checklist:

• My PR is the minimum possible work for the desired functionality
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• If I included new strings, I have used trans. to make them localizable.
  For more information see our translations guide.

[alisterburt]

❤️ ❤️ ❤️

view_ray.mp4

[kevinyamauchi]

Super cool, @alisterburt . If others want to try it, see the cursor_ray.py example in this PR. I think it's worth noting that the mouse callback is just:

@volume_layer.mouse_drag_callbacks.append
def on_click(layer, event):
    near_point, far_point = layer._cursor_ray(event)
    ray_points = np.linspace(near_point, far_point, n_points, endpoint=True)
    if ray_points.shape[1] != 0:
        ray_layer.data = ray_points

[codecov]

Codecov Report

Merging #3037 (6936310) into master (d228e49) will increase coverage by 0.03%.
The diff coverage is 98.94%.

@@            Coverage Diff             @@
##           master    #3037      +/-   ##
==========================================
+ Coverage   82.69%   82.72%   +0.03%     
==========================================
  Files         509      510       +1     
  Lines       42586    42912     +326     
==========================================
+ Hits        35216    35500     +284     
- Misses       7370     7412      +42     

Impacted Files	Coverage Δ
napari/layers/shapes/_shapes_utils.py	93.46% <ø> (-0.16%)	⬇️
napari/_qt/qt_viewer.py	74.35% <75.00%> (-0.40%)	⬇️
napari/layers/base/base.py	93.03% <94.87%> (+0.99%)	⬆️
napari/_qt/_tests/test_qt_viewer.py	91.57% <100.00%> (+0.66%)	⬆️
napari/_vispy/_tests/test_utils.py	100.00% <100.00%> (ø)
napari/_vispy/utils.py	97.56% <100.00%> (+2.10%)	⬆️
napari/layers/labels/_tests/test_labels.py	100.00% <100.00%> (ø)
napari/layers/labels/labels.py	95.67% <100.00%> (-0.03%)	⬇️
napari/layers/shapes/_shape_list.py	86.64% <100.00%> (+0.03%)	⬆️
napari/utils/_tests/test_geometry.py	100.00% <100.00%> (ø)
... and 43 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d228e49...6936310. Read the comment docs.

[sofroniewn]

@kevinyamauchi @alisterburt this is sooooo amazing!! I love it, I've given a bit of preliminary review now, I know it's a WIP so I hope you don't mind. I was mainly high level, a couple ideas around API/ naming etc, but all in all this is looking great and will be so much fun to play with!! Keep up the great work 🚀

[sofroniewn]

Maybe we use the word ray instead of vector? while i agree vector is reasonable, it also might make folks think of the vector layer, and I think ray is quite nice. We could use it everywhere.

Also how about making this method private, say _world_to_data_ray, and then adding a kwarg to our existing method

def world_to_data(position, ray=False):

I know @jni might be a bit uneasy about a kwarg changing the meaning of the input, so maybe i should be overruled here, but just trying to think about the best naming. As noted in another comment below, you could do the same thing for get_value(position, ray=True) which would be neat!

[kevinyamauchi]

Hmm I'm a bit torn on this one. My current leaning is towards having a separate method (we can choose a different name) for both transforming orientations (vector_world_to_data()) and getting a value from a click since I think they are doing different things. For example, for get_value, in 2D, the click point really is the point in the data the user intends to interrogate. Whereas, in 3D, the click position is actually a position in a plane unknown to the user in the data and there are different ways we can interrogate the data using that point.

edit: I think this may be the same thing as

I know @jni might be a bit uneasy about a kwarg changing the meaning of the input

[sofroniewn]

Ok, let's see what @jni thinks, but if we stick with two methods i might prefer world_to_day_ray as a name and using ray as our word throughout

[jni]

I might be misunderstanding, but I'm actually kinda leaning towards unifying the API. It feels like the same principle — get a 2D point in canvas, then depending on ndisplay, this point implies either a 2D point in the data, or a 3D ray in the data.

In other words, we can use not a keyword argument like ray=True, but the dimensionality of ray/display as the switch.

btw the docstring below seems out of date, as dims_displayed is not an argument of this method...

[sofroniewn]

Is this going to turn into an abstract method on the base class that every layer will have to implement, sort of like some of the _get_value? That makes a lot of sense to me.

If you liked the idea of adding a kwarg world_to_data that I mentioned you could do the same thing with

get_value(self, position, *, world=False, ray=False):

which I think would be quite nice. cc @jni

[kevinyamauchi]

Thank you for the feedback , @jni and @sofroniewn ! I think we are pretty much there in terms of the core code and we now just need to decide how to hook it up.

Just to make sure we're all on the same page, a quick summary:

1. The mouse event how has the following properties:
   • pos: the position of the click in canvas coordinates
   • position: the position of the click in nD world coordinates
   • view_direction: the vector point in the direction of the camera into the scene in nD world coordinates
   • dims_point: the indices of the current slice (from viewer.dims.point)
   • dims_displayed: the ordered dims currently displayed in the (from viewer.dims.displayed)
2. We added a vector_world_to_data() which transforms a vector from world to data coordinates, preserving the orientation.
3. We added cursor_ray() to the base layer, which takes a MouseEvent and returns the start and end point for the ray that intersects the in nD layer data coordinates.
4. On the Labels layer, we've added a _get_value_ray() method, which takes the start and end point of a ray in nD layer data coordinates and returns the first non-zero (i.e., non background value) encountered.

In terms of what I think users/developers will want to interact with:

1. I think developers will want to use layer.cursor_ray() to get the click ray that goes through their data so that they can add custom 3D interactivity
2. I think users will want to be able to select and move items in 3D.
3. I think users will want to be able to inspect their data in 3D (i.e., status bar should update with appropriate value for what is under the cursor).

To achieve this, I propose the following:

• We modify the cursor_ray() signature to instead accept the values it requires as individual arguments (i.e., click_position_world, view_direction_world, and dims_displayed) instead of MouseEvent. This allows people to use it outside of mouse event callbacks (e.g., interrogating their data for some analysis). Also, potentially rename?
• We modify layer.get_value() to accept click_position_world, view_direction_world, and dims_displayed. Based on these values, get_value() can correctly call the 2D or 3D _get_value() function. We can make this a non-breaking change by makingview_direction_world and dims_displayed optional and if they aren't provided, it falls back on the current get_value implementation. We can implement the 3D get_value() function for labels in this PR and add it for the other layers in future PRs.
• To correctly update the status bar, we modify get_status() to pass click_position_world, view_direction_world, and dims_displayed to layer.get_value() so the 3D get_value can be called (when appropriate). This means that we also have to update wherever get_status() is called so that it can pass click_position_world, view_direction_world, and dims_displayed. edit: I had a look and the get_status() is called from a callback connected to the Cursor modell, so I think we need to add view_direction_world to the Cursor model (click_position_world and dims_displayed are already available on the Cursor model / Viewer).
• We rename layer.vector_world_to_data to layer.ray_world_to_data or layer.direction_world_to_data to avoid confusion with the Vectors layer (it's probably a misnomer anyway - see next point).
• I would prefer to keep layer.world_to_data and layer.vector_world_to_data (whatever we rename it to) separate. They are fundamentally different transformations. layer.world_to_data maps positions and layer.vector_world_to_data maps the orientation of the vector (e.g., layer.vector_world_to_data doesn't apply translations).

I think this is pretty in line with what @sofroniewn and @jni suggested, but I wanted to make sure we are all on the same page because I think we are going to want to build a lot on top of the functionality. What do you all think?

[sofroniewn]

I think this is pretty in line with what @sofroniewn and @jni suggested, but I wanted to make sure we are all on the same page because I think we are going to want to build a lot on top of the functionality. What do you all think?

All this sounds pretty good

The mouse event how has the following properties ....

I'm a tad nervous about this as it's somewhat funny to me that the mouse now knows about some of this dims stuff, but on the other hand I can see why it's needed and I don't really see another way right now.

We modify layer.get_value() to accept click_position_world, view_direction_world, and dims_displayed ...
This means that we also have to update wherever get_status() is called so that it can pass click_position_world, view_direction_world, and dims_displayed

Do you want to make a small dedicated PR that makes just these changes (without view_direction_world). Might be easier to review and keep this diff a little smaller.

I saw @kevinyamauchi you've got some good tests inside the labels layer where you mock the mouse event. Can you make a copy of those tests that test the same stuff but put them at the level of the whole viewer and trigger the mouse event manually - I want to make sure that if we changed something like the addition of the mouse_event.dims_displayed we'd get a fail, and i'm not sure if that's already the case.

[kevinyamauchi]

Thanks for the feedback, @sofroniewn !

I'm a tad nervous about this as it's somewhat funny to me that the mouse now knows about some of this dims stuff, but on the other hand I can see why it's needed and I don't really see another way right now.

I agree it's a little funny. I also don't see another way at the moment and I don't think bad, as it's not part of the Cursor model, it's just added to the event by the Viewer to aid in the interpretation of the position and view_direction (I think correctly interpreting even just the cursor position is difficult without the dims_displayed and dims_point).

I saw @kevinyamauchi you've got some good tests inside the labels layer where you mock the mouse event. Can you make a copy of those tests that test the same stuff but put them at the level of the whole viewer and trigger the mouse event manually - I want to make sure that if we changed something like the addition of the mouse_event.dims_displayed we'd get a fail, and i'm not sure if that's already the case.

Good call. Will do!

Do you want to make a small dedicated PR that makes just these changes (without view_direction_world). Might be easier to review and keep this diff a little smaller.

I think this is a great idea! This PR is definitely getting a little bulky. In that case, the last things are:

• to decide on the names for what are now cursor_ray() and layer.vector_world_to_data
• copy of the MouseEvent tests.

[sofroniewn]

I think this is a great idea! This PR is definitely getting a little bulky. In that case, the last things are:

I guess that PR could also come after this one too. Happy to do it either way

to decide on the names for what are now cursor_ray() and layer.vector_world_to_data

How about _cursor_ray -> get_ray_endpoints, and vector_world_to_data -> world_to_data_ray

[jni]

Some minor comments on the discussion above:

How about _cursor_ray -> get_ray_endpoints, and vector_world_to_data -> world_to_data_ray

_world_to_data_ray? But also, how about _world_to_data(point=..., direction=None)? If I follow the discussion above (perhaps I don't!), the issue is that transforming a point and a vector are two different things. But both versions of the function require transforming a point. By giving direction=None, we remove that part of the code in 2D, but use it in 3D.

But, whatever you decide @kevinyamauchi, let's keep it private still, so we can refactor to our heart's content after merge. 😊

I think developers will want to use layer.cursor_ray() to get the click ray that goes through their data so that they can add custom 3D interactivity

Why not update the single Cursor model with position to be equal to the front-face position in 3D and position_end equal to the back face in 3D, or None in 2D? (Or it could be identical to position in 2D.)

Other than that, in general terms, I absolutely love the proposal to make methods less dependent on global state.

[jni]

btw @kevinyamauchi feel free to ping me on Zulip this morning if you work on this and want to bounce ideas off someone. I don't have a pair session scheduled tonight. 😊

[sofroniewn]

Why not update the single Cursor model with position to be equal to the front-face position in 3D and position_end equal to the back face in 3D, or None in 2D? (Or it could be identical to position in 2D.)

Yeah that could be nice. Might want to keep the additional cursor properties private until we figure this out too, but we can always deprecate if we change

[sofroniewn]

Nice work here @kevinyamauchi! Let's get this in!! 🚀

I'm approving now, but will probably let sit one more night in case anyone else wants to review/ has additional feedback - otherwise will plan on merging tomorrow morning. How does that sound?

[kevinyamauchi]

Why not update the single Cursor model with position to be equal to the front-face position in 3D and position_end equal to the back face in 3D, or None in 2D? (Or it could be identical to position in 2D.)

@jni and I discussed this on zulip, but I'll just summarize here in case anybody is curious. It is challenging to put the front/back intersections on the Cursor model because the Cursor model lives on the Viewer. Since the Cursor model is on the Viewer, the front/back face should probably be with respect to the bounding box of the full scene. Currently , we do not have a concept of the scene extents on the Viewer (as far as I know). While not so difficult to implement the scene extents, I propose that add/discuss this in a future PR.

After @tlambert03's and @sofroniewn 's feedback and some conversations with @jni and @alisterburt , I have made the following changes:

• added a test to the qt_viewer to test the properties that are added to the MouseEvent
• renamed layer.cursor_ray() -> get_ray_intersection()
• renamed vector_world_to_data() -> _world_to_data_ray()
• revised the _process_mouse_event() docstring

With all of that, this PR gives the ability to:

• in a mouse click callback, get the intersections of the mouse click ray with the data bounding box for all layers
• adds a function to get the first non-background label along a ray to the Labels layer

In the follow up PRs, we will:

• hook up the 3D interaction to layer.get_value and layer.get_status
• add 3D selection in the GUI to points, labels, and shapes

I think this is pretty much ready to go! Looking forward to any final feedback you all might have!

[kevinyamauchi]

Nice work here @kevinyamauchi! Let's get this in!! 🚀

I'm approving now, but will probably let sit one more night in case anyone else wants to review/ has additional feedback - otherwise will plan on merging tomorrow morning. How does that sound?

Sounds good to me! Thanks everyone!

[sofroniewn]

Currently , we do not have a concept of the scene extents on the Viewer (as far as I know). While not so difficult to implement the scene extents, I propose that add/discuss this in a future PR.

Do you want to use viewer.layers.extent? Would that help? (sorry not sure on details here)

napari/napari/components/layerlist.py

Lines 236 to 244 in 529181d

	@property
	def extent(self) -> Extent:
	"""Extent of layers in data and world coordinates."""
	extent_list = [layer.extent for layer in self]
	return Extent(
	data=None,
	world=self._get_extent_world(extent_list),
	step=self._get_step_size(extent_list),
	)

No need to use that now, but if that's useful and you want to change in this PR let me know - especially if it would change any public API that we're about to create

[kevinyamauchi]

@sofroniewn , cool! I didn’t know about that. It doesn’t affect anything in this PR, but it may inform how we hook the cursor model in the follow up PR. Thanks!

[jni]

@kevinyamauchi @sofroniewn note that we've had lots of performance issues with viewer.layers.extent, so I would stay away from it for these purposes until we've started caching layer extents.

[sofroniewn]

@kevinyamauchi @sofroniewn note that we've had lots of performance issues with viewer.layers.extent, so I would stay away from it for these purposes until we've started caching layer extents.

Ah yes - good point!

[sofroniewn]

Would be worth knowing though it if would be highly useful here though, and the we should cache layer extents - rather than just reimplementing the same thing in parallel

[jni]

Indeed, we should not reimplement. But I think what would be useful here is viewer.layers.selection.active (from memory, might be different). Then we can go on with viewer.cursor.position being the front face and viewer.cursor.position_end being None (2D) or the back face (3D).