Emit highlight event only if selection changed #7162
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7162 +/- ##
==========================================
- Coverage 92.99% 92.90% -0.10%
==========================================
Files 619 619
Lines 56729 56733 +4
==========================================
- Hits 52757 52706 -51
- Misses 3972 4027 +55 ☔ View full report in Codecov by Sentry. |
This was referenced Aug 14, 2024
psobolewskiPhD
added a commit
to psobolewskiPhD/napari
that referenced
this pull request
Aug 17, 2024
This reverts commit b7a8d00.
jni
pushed a commit
that referenced
this pull request
Aug 18, 2024
…" (#7201) # References and relevant issues This reverts commit b7a8d00. Closes: #7187 Re-opens: #7071 Alternative to #7188 # Description #7162 resulted in major un-intended (lack of tests 😬) consequences to: 1. selecting points and shapes with the selection tool 2. drawing shapes 3. editing shapes using the vertex selection tool 4. moving shapes using the selection tool I had made #7188 as a naive fix to the first symptoms: points selection, but the issue is more far reaching and requires more thought. See also some discussion here: https://napari.zulipchat.com/#narrow/stream/212875-general/topic/highlight My suggestion here is to just revert #7162 because the unintended consequences are far more severe than the original issue. Then we can return to the drawing board to disentangle highlights from selection and try to address the original event related issue more precisely.
jni
pushed a commit
that referenced
this pull request
Aug 29, 2024
# References and relevant issues This PR fixes highlight performance problems in the Shapes layer. #7162 tried to fix them by reducing the frequency of highlight events, but that had to be reverted (#7187, #7201). Instead, this dramatically speeds up highlighting when the event is fired. # Description For performance reasons, the Shapes layer creates a *single* mesh containing all shapes and shape edges, and this is what is sent to VisPy to draw. When highlighting, prior to this PR, we needed to find the Shape edges that are being highlighted within that giant mesh (linear in the number of vertices, and iterated in a Python for-loop). With this PR, instead, we look at the precomputed mesh stored in each highlighted shape and stack them together. Therefore, the complexity is linear in the number of vertices in the highlighted shapes, and the constant factors should also be reduced. Before this PR: <img width="343" alt="Screenshot 2024-08-29 at 11 50 34" src="https://github.com/user-attachments/assets/bf2fb01d-494a-42c4-8f06-53dc14dbac0b"> After This PR <img width="273" alt="Screenshot 2024-08-29 at 11 50 43" src="https://github.com/user-attachments/assets/52720dad-19bc-42c4-949d-e62bd5544e34">
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References and relevant issues
closes #7071
Description
Currently, on main, we omit a highlight event every time when
_set_highlightmethod is called. However, it is called every time the zoom or canvas size is changed to fix some problems with highlights (see #6425)I think that it should be emitted only if selection is changed.