-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
FIX: Make widget blitting compatible with swapped canvas #30591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
890404a to
48346de
Compare
|
Recent update/commit: Support blitting with changing canvas in some widgets Approach depends a bit on the widget, but generally:
Supported:
Not supported yet:
This should already fix #30575 as Selector widgets are covered. Note: It's probably best to review the commits individually. |
b418b60 to
f7ca7cc
Compare
|
Drat, I did not re-look at this PR before I opened #30605 |
|
Rebased. |
|
@tacaswell How do we want to continue here? I consider this PR complete for the handled widgets, and suggest that we review and merge it. The remaining widgets should be handled in a follow-up PR to keep this PR more manageable. Here's a summary of this PR approach:
|
| """Update the canvas cursor.""" | ||
| self.ax.get_figure(root=True).canvas.set_cursor(cursor) | ||
|
|
||
| def _save_blit_background(self, background): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to pass the bounding box in instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For symmetry reasons and to keep the changes within the PR minimal, I would leave the complete handling to the individual widgets for now.
They do
self._save_blit_background(self.canvas.copy_from_bbox(self.ax.bbox))and
background = self._load_blit_background()
if background is not None:
self.canvas.restore_region(background)It may be reasonable to refactor in a follow-up and move that logic to the base class.
30ca550 to
87fa6db
Compare
Approach depends a bit on the widget, but generally: - `self._useblit` only stores the flag passed to __init__ - canvas.supports_blit is checked dynamically when needed - we sometimes have the property `sef.useblit`, which is the effective value of flag and capability - if animated artists are needed that state is set during __init__ based on the flag. This relies on the fact that the flag cannot be changed later, and that the value of "animated" does not play a role for drawing canvases without blit support. Supported: - Button - _SelectorWidget - SpanSelector - ToolLineHandles - ToolHandles - RectangleSelector - LassoSelector - PolygonSelector Not supported yet: - CheckButtons - RadioButtons - Cursor - MultiCursor - Lasso
|
|
||
| self._useblit = useblit and self.canvas.supports_blit | ||
| self._background = None | ||
| self._useblit = useblit and self.canvas.supports_blit # TODO: make dynamic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timhoffm are you planning on addressing this TODO in this PR or is it a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a follow-up and can land in 3.11 or later.
QuLogic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is semi-private, so it's open to change a bit, but I think it looks good regardless.
|
|
||
| self._useblit = useblit and self.canvas.supports_blit | ||
| self._background = None | ||
| self._useblit = useblit and self.canvas.supports_blit # TODO: make dynamic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO necessary to complete before 3.11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a follow-up and can land in 3.11 or later.
greglucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go with the current implementation and the TODOs can be addressed in follow-up PRs, but will leave that up to Tim whether he wants to get it in now.
First step towards improving blitting - towards #30503 and fixing #30575.
This PR proposes a minimal low-level API to store background information on the canvas.
It also adds higher-level methods
save_blit_background()andload_blit_background()toAxesWidgets. Child classes can execrise managed background storage though this API.I specifically would like to have feedback on the solution idea and API design. Note: This is not intended to be a solution for everything right away, but I would like to learn more on blitting through the case of widgets.
Open topics
useblitfrom statically including canvas blitting capability to dynamic handling.Edit: Tested the following examples all with blitting enabled, and they work as expected (after closing the figure,
plt.figure(fig); plt.show()recrates the figure and the widgets continue to work.