Let QPaintEvent tell us what region to repaint. #8951
Conversation
| bbox_queue = [ | ||
| Bbox([[0, 0], [self.renderer.width, self.renderer.height]])] | ||
| self._bbox_queue = [] | ||
|
|
||
| self._bbox_queue = [] |
| self._bbox_queue = [] | ||
| for bbox in bbox_queue: | ||
| if bbox is None: |
There was a problem hiding this comment.
If any of them are None can short circuit the whole thing and just copy once.
|
Actually I just rewrote the entire thing to use QPaintEvent.rect() (the idea you mentioned in #7366), which is much simpler. |
7290244 to
98d5753
Compare
|
Does |
| @@ -134,7 +136,7 @@ def blit(self, bbox=None): | |||
| """ | |||
| # If bbox is None, blit the entire canvas. Otherwise | |||
| # blit only the area defined by the bbox. | |||
| if bbox is None and self.figure: | |||
| if bbox is None: | |||
| bbox = self.figure.bbox | |||
|
|
|||
| self._bbox_queue.append(bbox) | |||
There was a problem hiding this comment.
might as well drop this line too?
There was a problem hiding this comment.
That would technically be backwards incompatible (and the behavior is "mandated" by the docstring of FigureCanvasBase), but a quick grep suggests that we never rely on it (always explicitly passing in the figure bbox when needed) so I'm fine with getting rid of the behavior. You get to decide on the deprecation policy :-)
| bbox_queue = [ | ||
| Bbox([[0, 0], [self.renderer.width, self.renderer.height]])] | ||
| self._bbox_queue = [] | ||
| for bbox in bbox_queue: |
There was a problem hiding this comment.
One concern is that Qt drops some paint events, but that would be surprising.
There was a problem hiding this comment.
If Qt pulls the rug under us, we're cooked anyways, no?
98d5753 to
ef2e361
Compare
|
rebased |
ef2e361 to
dd5424e
Compare
(instead of just updating the blitboxes.)
Alternate approach for #8948.
attn @tacaswell
As for the performance questions, my work on mpl_cairo suggests to me that a big cost actually comes from the fact that we are copying the whole buffer into a string (https://github.com/matplotlib/matplotlib/blob/master/src/_backend_agg_wrapper.cpp#L586 before my recent changes, https://github.com/matplotlib/matplotlib/blob/master/src/_backend_agg_wrapper.cpp#L79 now that we always rely on Regions). Note that the old Py2 version used to create a buffer object, which would avoid the need for copying.
In mpl_cairo, I instead return a numpy array which is a view on the internal buffer (https://github.com/anntzer/mpl_cairo/blob/master/src/_mpl_cairo.cpp#L260) and pass the address of the pointer (which can be retrieve via numpy) into QImage (https://github.com/anntzer/mpl_cairo/blob/master/mpl_cairo/qt.py#L15). (To be clear, I haven't tried with agg, but on mpl_cairo this is much faster than copying the string around.)
This approach also removes the need for a refcount hack on PySide, as there is no buffer object for PySide to hold on anymore.
PR Summary
PR Checklist