Speed up get_status for Shapes layer by using bounding boxes
#7144
Conversation
|
The Qt benchmark run requested by PR #7144 (153f835 vs 93140b9) has finished with status 'failure'. See the CI logs and artifacts for further details. |
|
The Qt benchmark run requested by PR #7144 (d8d4f32 vs 93140b9) has finished with status 'failure'. See the CI logs and artifacts for further details. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7144 +/- ##
==========================================
- Coverage 92.81% 92.73% -0.09%
==========================================
Files 623 623
Lines 57347 57400 +53
==========================================
+ Hits 53227 53228 +1
- Misses 4120 4172 +52 ☔ View full report in Codecov by Sentry. |
|
The Qt benchmark run requested by PR #7144 (cecb267 vs 93140b9) has finished with status 'failure'. See the CI logs and artifacts for further details. |
|
The Qt benchmark run requested by PR #7144 (83b61b7 vs 93140b9) has finished with status 'failure'. See the CI logs and artifacts for further details. |
|
The Qt benchmark run requested by PR #7144 (abf86c7 vs 35a9936) has finished with status 'failure'. See the CI logs and artifacts for further details.
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
napari/layers/shapes/_shape_list.py
Outdated
| @@ -1018,6 +1020,7 @@ def transform(self, index, transform): | |||
| self.remove(index, renumber=False) | |||
| self.add(shape, shape_index=index) | |||
| self._update_z_order() | |||
| self.__dict__.pop('_bounding_boxes', None) | |||
There was a problem hiding this comment.
I had to infer here that cached_property stores the cache in self.__dict__, and this line is clearing the cache. I think it would be good to abstract this as a one-line private method, self._clear_bounding_box_cache()?
And (optional) rather than manually calling this in each method that changes the data, should we not just connect it to an event that fires any time the data changes?
There was a problem hiding this comment.
It is not an evented model, and we already have performance problems with it.
napari/layers/shapes/_shape_list.py
Outdated
| (bounding_boxes[0] <= coord) * (bounding_boxes[1] >= coord), | ||
| axis=1, | ||
| ) | ||
| inside_indices = np.nonzero(inside)[0] |
There was a problem hiding this comment.
| inside_indices = np.nonzero(inside)[0] | |
| inside_indices = np.flatnonzero(inside) |
napari/layers/shapes/_shape_list.py
Outdated
| axis=1, | ||
| ) | ||
| inside_indices = np.nonzero(inside)[0] | ||
| if not inside_indices.size: |
There was a problem hiding this comment.
For naturally integer values I think it is more readable to compare rather than to check for truthiness.
| if not inside_indices.size: | |
| if inside_indices.size == 0: |
| z_list = self._z_order.tolist() | ||
| order_indices = np.array([z_list.index(m) for m in shapes]) | ||
| ordered_shapes = shapes[np.argsort(order_indices)] | ||
| return ordered_shapes[0] |
There was a problem hiding this comment.
Looking at this deleted code, I notice that you no longer sort the shapes by z_order. Are we sure that doesn't matter? (ie are the shapes in z_order anyway?)
There was a problem hiding this comment.
I do not understand this part of the code. It looks obsolete now, to me. It comes from #100 PR
napari/layers/shapes/_shape_list.py
Outdated
| if not self.shapes: | ||
| return None | ||
| bounding_boxes = self._bounding_boxes | ||
| inside = np.all( |
There was a problem hiding this comment.
I think this should be renamed "maybe_inside" or "in_bbox", reserving "inside" for shapes that actually contain the coordinate.
| @property | ||
| def bounding_box(self) -> np.ndarray: | ||
| """np.ndarray: 9x2 array of vertices of the interaction box.""" | ||
| return self._bounding_box[:, self.dims_displayed] |
There was a problem hiding this comment.
the docstring doesn't seem to match the code?
|
@Czaki before merging, could you double check that this PR works with modified z-order? By default, shapes added later are on top of shapes added first. But you can change the z-ordering of shapes by clicking the "move to front" and "move to back" buttons. We should compute intersections based on the z-order of shapes, not the order in which they were added. See this video: shp-zorder.mp4 |
|
@jni |
|
Awesome, thanks! 🚀 |
get_status for Shapes layer by use bounding boxesget_status for Shapes layer by using bounding boxes
…7332) # References and relevant issues https://napari.zulipchat.com/#narrow/stream/212875-general/topic/Issue.20grabbing.20lines.20in.20shape.20layer/near/477001240 # Description In #7144 we attempted to speed up mouse click testing in shapes by using shapes bounding boxes. However, there were two bugs in that implementation: - it did not account for edge width around shapes, so one could only click on the interior of a shape — which is hard for edge-only shapes like paths 😅 - it did not account for certain classes of shapes data changes, so that the bounding box data was out of date when shapes were transformed, and clicking on the shape became impossible 🫣 This PR fixes both issues by: - supplementing the bounding box by 0.5 times the edge width. - Add missing cache clears. It also adds tests for these missed elements. --------- Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
References and relevant issues
Description
When investigating why napari interface is so lagging with big number of layers, I have spotted that Shapes triangulation is used to determine if the mouse is inside some shape. However, a single shape could be constructed from multiple polygons. Also, edge polygons are used.
This PR adds information about bounding box to Shape object. Then filtering of Shapes based on belonging cursor position to bounding box of shape.
Then checking if the cursor is in such a filtered list of shapes is performed based on triangles (same as previous).