Add primitive picking filters for Mesh and Markers visuals #2500

aganders3 · 2023-06-14T15:15:30Z

This adds a filter for picking individual faces from a mesh. It works similarly to the picking mode already in use for picking visuals but instead colors the individual faces (triangles) of the mesh based on an index.

I also added an example (heavily borrowing from the mesh shading example) to illustrate how the feature works and how to use it. In the example the mouse will "paint" faces as it moves over a mesh. Pressing 'p' will show the face picking view, that is it will show the colors used to encode the face indices.

Screen.Recording.2023-06-14.at.11.09.53.AM.mov

vispy/visuals/filters/mesh.py

brisvag

Awesome @aganders3! This is a lot smaller than I expected in terms of actual changes :) And I see how this can be adapted to other visuals!

One thing I noticed in the picking mode builtin in the canvas is that you can actually render a specific region and/or crop of the canvas. Maybe we can use the same here to improve performance, by only rendering a small area around the cursor. Or am I misunderstanding?

aganders3 · 2023-06-14T15:38:26Z

Thanks!

Regarding cropping - you're probably right but I have not tested! I can try this and do some profiling. I'll note right now this is pretty much left up to the downstream developer because I didn't add anything like visual_at (face_at). The picking-by-mouse logic is only in the example.

brisvag · 2023-06-14T15:42:47Z

Yep, I like tis approach, it can be used in many ways. We can handle the actually rendering/picking downstream. Would still be nice to see it in the example just to show it how works!

brisvag · 2023-06-14T15:59:42Z

As far as I can tell, this filter could be used almost as-is for Markers, you just need to change _update_data :)

…ents

examples/scene/face_picking.py

aganders3 · 2023-06-15T01:35:14Z

As far as I can tell, this filter could be used almost as-is for Markers, you just need to change _update_data :)

I think I see what you mean - same with some of the other visuals. Would you like me to try to add that to this PR? I guess that would need a separate but similar filter (more or less) for each visual type? Or do you have an idea for how it could be a single class -- maybe it could dispatch _update_data based on the visual type?

brisvag · 2023-06-15T08:53:40Z

It's actually slightly more complex, because here you also rely on mesh_data and some events that are not present in other places. Still, it shouldn't be too complex to adapt.

I guess that would need a separate but similar filter (more or less) for each visual type?

Yea, I think the easiest way would be to have a main class with the shader logic, and each subclass has the logic to go from Visual to Number of primitives and how/when to update it... It's really a pity that glPrimitiveID is not available always, it would be so much easier :(

Would you like me to try to add that to this PR?

Not necessarily other visuals, but if you could set this up so it's extensible in a followup PR it would be great! On the other hand, this is already mergeable on its own, so feel free to choose where to put your energy :)

jni · 2023-06-15T09:25:58Z

Just dropping by to say 😍!

brisvag · 2023-06-15T09:28:37Z

Wait, I just noticed something in the initial video: when you display the picking, quads are colored rather than triangles. Something's off with indexing in that mode?

aganders3 · 2023-06-15T10:47:11Z

I think they just look like quads because the triangles are ordered sort of in pairs? If triangles are next to each other (in the index space) they will be very close in color. Here it is with the indices multiplied by 10_000 to show the differences.

It does seem like a strange ordering. Maybe this mesh comes from a quad mesh that was triangulated or something?

Edit: also I can shuffle the faces and everything still works and the "picking view" looks as I'd expect. :)

aganders3 · 2023-06-15T15:52:30Z

It was not too bad to add marker picking. I'm going to clean it up and see if there's a good way to abstract the base class

Screen.Recording.2023-06-15.at.11.49.48.AM.mov

aganders3 · 2023-06-20T18:51:32Z

@brisvag this is now refactored into a base class and two picking filters for Markers and Mesh visuals. Please take another look when you have a chance!

brisvag

Awesome! A few comments here and there, but nothing major. Supert excited to use this in napari, will be a huge speed up and improvement in rensponsiveness!

brisvag · 2023-06-28T16:50:41Z

vispy/visuals/filters/base_filter.py

+        self._id_colors = VertexBuffer(np.zeros((0, 4), dtype=np.float32))
+        vfunc['ids'] = self._id_colors
+        self._n_primitives = 0
+        super().__init__(vcode=vfunc, fcode=ffunc)


Might be a good idea to set fpos to a high value (like 1e9 or so), since we really want this color change to happen at the end of any other filter.

This is definitely a good idea. One question is how it should be set relative to the existing PickingFilter, which only sets fpos=10.

I just played with this a bit and it's a small problem if fpos is higher than that for PickingFilter (10), then visual picking (i.e. visuals_at) is broken. This doesn't feel like a common use case, but still.

My first instinct is to set fpos on the existing PickingFilter to something high like 1e9 and something like 1e9-1 below that on the PrimitivePickingFilter ABC, but I think this would be a breaking change.

Otherwise maybe just set fpos here to 9 but also it to __init__ in case that's not sufficient. (edit: this is in 6034cba)

What do you think?

brisvag · 2023-06-28T16:50:44Z

vispy/visuals/filters/base_filter.py

@@ -120,3 +125,98 @@ def _detach(self, visual):

        self._attached = False
        self._visual = None
+
+
+class PrimitivePickingFilter(Filter, metaclass=ABCMeta):


Any reason to use ABCMeta other than just inherit from ABC? Not really important, just curious.

I think I chose to specify the metaclass directly because it was already inheriting from another class so this felt more explicit. I was a little back-and-forth on this because it does the exact same thing and even seeing the word "metaclass" can be a bit ominous, haha.

brisvag · 2023-06-28T16:52:59Z

vispy/visuals/filters/base_filter.py

+        Generally, this method should be implemented to:
+            1. Calculate the number of primitives in the visual, stored in
+            `self._n_primitives`.
+            2. Pack the picking IDs into a 4-component float array (RGBA
+            VertexBuffer), stored in `self._id_colors`.


Would be awesome if the packing was done here instead of in the subclasses, since it's error prone and anyways it's always the same... Youy probbaly nee dto juggle some extra submethods calling each other, but it would be useful. not blocking though!

I agree but didn't immediately have a way to do it that felt clean/intuitive so I punted and tried to compensate in the docstring. I'll give it a bit more thought because it does seem possible.

An alternative would be to move the packing into the shader and instead just pass the IDs in the VertexBuffer. I avoided this for now just because I'm more comfortable with numpy than I am with GLSL but I don't think it would be too difficult.

Since these are also "just" implementation details I'm also happy to explore both options in a new PR if we want this to (potentially) be available a bit quicker.

See 36db4fc for one idea

I was thinking more something like:

@staticmethod def _pack_ids_into_rgba(ids): # pack ids ... return packed @abstractmethod def _get_picking_ids(self): # this is the method that subclasses will change, which simply # calculates the ids and spits them out as normal numpy array # and it could for example return None if no change is needed # to improve performance return None def _update_id_colors(self): # this should remain untouched ids = self._get_picking_ids() if ids is None: return id_colors = self._pack_ids_into_rgba(ids) self._id_colors.set_data(id_colors) def _on_data_updated(self, event=None): if not self.attached: return self._update_id_colors()

A bit more complicated, but removes the need to think about packing from the sublcasses, and should make it easier to switch to a shader implementation later as well.

Nice, thanks. This looks good. I'll update.

Since these are also "just" implementation details I'm also happy to explore both options in a new PR if we want this to (potentially) be available a bit quicker.

To be clear, perfectly happy with this as well!

No worries, it was a pretty quick refactor. The latest commit (6022b98) has these changes plus the discard_transparent property.

brisvag · 2023-06-28T16:54:41Z

vispy/visuals/markers.py

@@ -573,6 +574,8 @@ def __init__(self, scaling="fixed", alpha=1, antialias=1, spherical=False,
                          blend_func=('src_alpha', 'one_minus_src_alpha'))
        self._draw_mode = 'points'

+        self.events.add(data_updated=Event)


Should we add this to the base visual to ensure it always exists? @djhoese

I wouldn't be against that. I'd be a little worried that users seeing it would assume that it is properly emitted/handled by all visuals which won't be true after a lot of work. Updating every set_data, right? Regardless, moving it to the base classes and documenting that it is new in version X and may not be used/emitted by all Visuals would be a good start.

Yeah, definitely document it. But I guess it's also not too much work to simply emit this in every visual's set_data.

The problem becomes the Visuals that have other properties for setting data like .color = new_color and don't end up calling a .set_data method at all.

If it's okay I would just leave it as-is for now. I can look into moving this to the base visual (and updating visuals to emit it) in another PR.

Sounds good to me!

aganders3 · 2023-06-28T18:42:41Z

Welcome back and thanks for the great comments!

I have another question now that I have looked more closely at the existing PickingFilter - should we include something like this in the fragment shader?

if( gl_FragColor.a == 0.0 )
    discard;

brisvag · 2023-06-29T11:56:35Z

I have another question now that I have looked more closely at the existing PickingFilter - should we include something like this in the fragment shader?

This stuff should already be handled by the visuals themselves, imo, if required. If a visual does exist somewhere but is fully transparent, I can imagine cases where you'd still want to detect it with the picking mode... Maybe having a flag for it (pick_transparent) would allow this to work both ways?

djhoese · 2023-06-29T13:18:14Z

This stuff should already be handled by the visuals themselves, imo, if required. If a visual does exist somewhere but is fully transparent, I can imagine cases where you'd still want to detect it with the picking mode... Maybe having a flag for it (pick_transparent) would allow this to work both ways?

I think vispy's handling of transparent/invalid/discarded pixels is not consistent between Visuals. Some, like the ImageVisual's NaN handling, I think discard NaN floating point values from floating point textures. But I think the same Visual uses alpha=0 for 8-bit integer textures. The picking shaders are applied after the Visual's shader, right? So if a Visual's fragment shader did discard the picking shader would never get called anyway.

brisvag · 2023-06-29T13:34:55Z

Exactly, which is why I think it's useful to be able to both include or exclude transparent fragments.

brisvag · 2023-06-29T14:34:40Z

Btw, some inspiration for doing this in the shader (here or later): https://stackoverflow.com/questions/28910861/opengl-unique-color-for-each-triangle

aganders3 · 2023-06-29T16:19:27Z

I tried moving this into the shader, but I'm stuck unable to pas the ids as uint32 in the VertexBuffer:

    on line 339: int can't be an in in the vertex shader
      attribute int a_ids;

I tried some workarounds, but I don't think you can do casting in glsl, and some possibly useful features (floatBitsToUint) are not available in this version of GLSL. I'm happy to leave this as-is and explore moving more into the shader later.

alisterburt · 2023-07-03T21:06:42Z

also jumping in to say 😍

brisvag

Ok, I think we're good to go! @djhoese, feel free to take a last look and/or merge!

vispy/visuals/filters/markers.py

aganders3 · 2023-07-11T19:20:32Z

Thanks again for the reviews. I was away all last week but am available now to address any remaining issues.

djhoese

I downloaded the CI artifact (a zip of the rendered HTML) and yeah this looks good. Very cool. Thanks for figuring it out and for getting this all working.

# Description Just a simple bump to our vispy dependency, now that 0.14 is out. This comes with a few fixes and new features relevant for napari: - Fix for wrong spherical marker depth: vispy/vispy#2506 - Fix for wrong shading of surfaces with flipped normals: vispy/vispy#2493 - picking filters for mesh and markers by @aganders, which can now be used in napari to drastically speed up our get-status: vispy/vispy#2500 --------- Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> Co-authored-by: napari-bot <napari-bot@users.noreply.github.com>

Just a simple bump to our vispy dependency, now that 0.14 is out. This comes with a few fixes and new features relevant for napari: - Fix for wrong spherical marker depth: vispy/vispy#2506 - Fix for wrong shading of surfaces with flipped normals: vispy/vispy#2493 - picking filters for mesh and markers by @aganders, which can now be used in napari to drastically speed up our get-status: vispy/vispy#2500 --------- Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> Co-authored-by: napari-bot <napari-bot@users.noreply.github.com>

aganders3 added 5 commits May 10, 2023 11:55

Face picking prototype

0ff3abf

Implement face picking as a mesh filter

54094e1

Add test for face picking

07e3f8d

Clean up example

29b0f5d

Add clearing control, more cleanup

b19c229

aganders3 force-pushed the face-picking branch from 20c71e9 to b19c229 Compare June 14, 2023 15:15

aganders3 mentioned this pull request Jun 14, 2023

get_status for surface (and probably other layers) should be monitored, debounced/throttled, and run on a separate thread napari/napari#5755

Open

aganders3 commented Jun 14, 2023

View reviewed changes

vispy/visuals/filters/mesh.py Outdated Show resolved Hide resolved

djhoese requested a review from brisvag June 14, 2023 15:25

djhoese assigned brisvag Jun 14, 2023

djhoese added type: enhancement component: visuals labels Jun 14, 2023

brisvag reviewed Jun 14, 2023

View reviewed changes

Mark test as requires_application

7d07618

aganders3 force-pushed the face-picking branch from c6e3e5c to 7d07618 Compare June 14, 2023 15:41

aganders3 mentioned this pull request Jun 14, 2023

Unwanted blending update with picking #2501

Closed

Render into a region for face picking example, also throttle mouse ev…

0f66ab8

…ents

aganders3 commented Jun 14, 2023

View reviewed changes

examples/scene/face_picking.py Show resolved Hide resolved

Marker picking with working example

2dd3374

aganders3 changed the title ~~Add FacePickingFilter for mesh visuals~~ Add primitive picking filters for Mesh and Markers visuals Jun 15, 2023

Refactor primitive picking filter base class

1dd2094

Fix marker picking example

6eadcb8

aganders3 requested a review from brisvag June 20, 2023 18:42

brisvag reviewed Jun 28, 2023

View reviewed changes

aganders3 added 2 commits June 28, 2023 16:45

Set default fpos so primitive picking is just before visual picking

6034cba

Add a convenience method for packing IDs into RGBA colors

36db4fc

brisvag mentioned this pull request Jun 29, 2023

Fix point selection highlight napari/napari#5737

Merged

Refactor primitive picking ABC, add 'discard_transparent' option

6022b98

brisvag approved these changes Jul 4, 2023

View reviewed changes

djhoese reviewed Jul 4, 2023

View reviewed changes

vispy/visuals/filters/markers.py Outdated Show resolved Hide resolved

Use sphinx gallery references instead of GitHub links for examples

5a1da93

djhoese approved these changes Jul 12, 2023

View reviewed changes

djhoese merged commit 00cc125 into vispy:main Jul 12, 2023
15 checks passed

akuten1298 mentioned this pull request Jul 31, 2023

Face picking filter in Napari akuten1298/napari#10

Draft

15 tasks

brisvag mentioned this pull request Sep 8, 2023

Vispy 0.14 napari/napari#6214

Merged

aganders3 mentioned this pull request Feb 18, 2024

Selecting points with TurntableCamera and Markers Visual #2236

Open

Add primitive picking filters for Mesh and Markers visuals #2500

Add primitive picking filters for Mesh and Markers visuals #2500

Conversation

aganders3 commented Jun 14, 2023

brisvag left a comment

Choose a reason for hiding this comment

aganders3 commented Jun 14, 2023

brisvag commented Jun 14, 2023

brisvag commented Jun 14, 2023

aganders3 commented Jun 15, 2023

brisvag commented Jun 15, 2023

jni commented Jun 15, 2023

brisvag commented Jun 15, 2023

aganders3 commented Jun 15, 2023 • edited

aganders3 commented Jun 15, 2023

aganders3 commented Jun 20, 2023

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 Jun 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 commented Jun 28, 2023

brisvag commented Jun 29, 2023

djhoese commented Jun 29, 2023

brisvag commented Jun 29, 2023

brisvag commented Jun 29, 2023

aganders3 commented Jun 29, 2023

alisterburt commented Jul 3, 2023

brisvag left a comment

Choose a reason for hiding this comment

aganders3 commented Jul 11, 2023

djhoese left a comment

Choose a reason for hiding this comment

aganders3 commented Jun 15, 2023 •

edited

aganders3 Jun 28, 2023 •

edited