Fix cycle in _update_draw/_set_highlight for Points and Shapes (high CPU background usage) #6425
Conversation
…ints and Shapes).
| @@ -1610,7 +1615,7 @@ def _update_draw( | |||
| super()._update_draw( | |||
| scale_factor, corner_pixels_displayed, shape_threshold | |||
| ) | |||
| self._set_highlight(force=True) | |||
There was a problem hiding this comment.
Hm. Is this change not enough? There is a reason why someone use force here. Has this changed?
There was a problem hiding this comment.
maybe just
prev_scale = self.scale_factor
super()._update_draw(
scale_factor, corner_pixels_displayed, shape_threshold
)
self._set_highlight(force=(prev_scale != scale_factor))To avoid additional parameters?
There was a problem hiding this comment.
Ha - this was my initial proposal in #6223 (comment).
I like it better as well, though 188438f perhaps matches more closely how other parameters are handled in _set_highlight so I could go either way. I'll update the PR and see if anyone else has feedback.
|
I can confirm this fixes the issue on my end. |
Codecov Report
@@ Coverage Diff @@
## main #6425 +/- ##
==========================================
- Coverage 92.16% 92.09% -0.07%
==========================================
Files 597 597
Lines 52896 52898 +2
==========================================
- Hits 48749 48716 -33
- Misses 4147 4182 +35
|
|
I propose not cherry-picking. Let's get 0.4.19 out and focus our efforts on getting 0.5.0 out soon too. |
|
(But I see @Czaki has already set the milestone — as he is leading the cherry-picking I'll let him change this or not as he chooses.) |
…CPU background usage) (#6425) I agree something like #6234 is a better long-term fix, but we should do something in the meantime because the high CPU is pretty severe. # References and relevant issues Closes #6223 # Description This breaks a cycle when updating the highlight on Points and Shapes layers. The problem was `_set_highlight` being called with `force=True` in `_update_draw`. This caused a cycle as redrawing the highlights would again call `_update_draw`. This was added to fix the appearance of the highlights during interaction - see #6223 for more discussion. I'm hoping @brisvag can confirm this PR maintains the correct behavior. This PR tracks `scale_factor` the same way `selected_data` and other properties are stored in order to conditionally update the highlight instead of forcing it.
…CPU background usage) (#6425) I agree something like #6234 is a better long-term fix, but we should do something in the meantime because the high CPU is pretty severe. # References and relevant issues Closes #6223 # Description This breaks a cycle when updating the highlight on Points and Shapes layers. The problem was `_set_highlight` being called with `force=True` in `_update_draw`. This caused a cycle as redrawing the highlights would again call `_update_draw`. This was added to fix the appearance of the highlights during interaction - see #6223 for more discussion. I'm hoping @brisvag can confirm this PR maintains the correct behavior. This PR tracks `scale_factor` the same way `selected_data` and other properties are stored in order to conditionally update the highlight instead of forcing it.
# References and relevant issues closes #7071 # Description Currently, on main, we omit a highlight event every time when `_set_highlight` method 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.
I agree something like #6234 is a better long-term fix, but we should do something in the meantime because the high CPU is pretty severe.
References and relevant issues
Closes #6223
Description
This breaks a cycle when updating the highlight on Points and Shapes layers. The problem was
_set_highlightbeing called withforce=Truein_update_draw. This caused a cycle as redrawing the highlights would again call_update_draw. This was added to fix the appearance of the highlights during interaction - see #6223 for more discussion. I'm hoping @brisvag can confirm this PR maintains the correct behavior.This PR tracks
scale_factorthe same wayselected_dataand other properties are stored in order to conditionally update the highlight instead of forcing it.