Qt5: Move agg draw to main thread and fix rubberband #4972

pwuertz · 2015-08-20T14:31:25Z

Follow up from #4962

tacaswell · 2015-08-20T14:35:04Z

This needs a rebase, sorry.

pwuertz · 2015-08-20T14:41:06Z

This PR contains two fixes.

The first one moves the agg draw back to the main thread. Qt uses a dedicated render thread for paintEvent. Although the agg buffer should be updated as closely to the qt repaint as possible in order to take all the latest events into account, accessing the scenegraph from the render thread is dangerous since the scenegraph has no locking mechanism.

The second and third commit address problems with the rubberband in qt. The second adds a remove_rubberband() handler to backend_bases that is invoked when the zoom rectangle is released.

The last commit changes the rubberband handling in the qt backend to utilize the rubberband removal handler. The previous solution was to simply remove the rectangle after each draw. This method fails when there is an additional paintEvent between two draw_rubberband() calls because it clears the rubberband while it is still being dragged (flicker). Also, there is the chance that the paintEvent requested by the last draw_rubberband() call before releasing the mouse might get delayed until after the draw_idle() call for zooming in. This causes the rubberband to be drawn on top of the zoomed-in figure (bad) and since there are no more events after the release the rubberband will stay on screen (worse).
Also, changing the state of the rubberband doesn't trigger an agg buffer redraw anymore, freeing some CPU cycles while dragging.

pwuertz · 2015-08-20T15:22:36Z

@jrevans Do you have an example for the situation you described where paintEvent is called while self.renderer doesn't exist yet?

tacaswell · 2015-08-20T16:35:17Z

My reading of @jrevans comments is that was his guess as to why the draw was there. That conditional can probably be removed.

tacaswell · 2015-08-22T19:05:17Z

I would like confirmation from @jrevans that this does not break any of their use cases.

I gave this a read over and it seems reasonable, but need sink more time into it. I am slightly worried about the MI re-ordering and making sure all of our classes are correctly cooperating when they need to be (one of the big changes between pyQt4 and pyQt5 is that their classes are now cooperating) so everything works with both qt4 and qt5.

attn @mfitzp (you clearly need a distraction from thesis writing 😈 )

tacaswell · 2015-08-23T14:26:30Z

@astrofrog Can you have a look at how this impacts glueviz?

astrofrog · 2015-08-23T15:27:52Z

@tacaswell - we don't yet use Qt5 in glueviz, so I'm not able to test this yet (unless this impacts Qt4 in some way?)

tacaswell · 2015-08-23T15:29:30Z

It does, qt4 is implemented as a shim on to of qt5.

On Sun, Aug 23, 2015, 11:27 AM Thomas Robitaille notifications@github.com
wrote:

@tacaswell https://github.com/tacaswell - we don't yet use Qt5 in
glueviz, so I'm not able to test this yet (unless this impacts Qt4 in some
way?)

—
Reply to this email directly or view it on GitHub
#4972 (comment)
.

astrofrog · 2015-08-23T22:14:02Z

@tacaswell - ok, in that case I will test this out tomorrow with glueviz

astrofrog · 2015-08-27T17:14:43Z

@tacaswell - sorry for the delay. Unfortunately this does break glueviz:

Before (master):

After (this PR):

pwuertz · 2015-08-27T17:19:50Z

Any idea why this would have an impact on glueviz while the regular gui operates normally?

tacaswell · 2015-08-27T17:23:45Z

glueviz does some monkey patching to try and control the draw cadence (if I recall correctly). I suspect that someplace they are calling draw instead of draw_idle.

pwuertz · 2015-08-27T18:06:17Z

Strange, that shouldnt stop the Widget from painting. Anything hardcoded to the order of the base and Mixin-classes? I will probably look into installing glueviz also...

tacaswell · 2015-08-27T18:10:33Z

lib/matplotlib/backends/backend_qt4agg.py

@@ -54,8 +54,8 @@ def new_figure_manager_given_figure(num, figure):
    return FigureManagerQT(canvas, num)


-class FigureCanvasQTAgg(FigureCanvasQTAggBase,
-                        FigureCanvasQT, FigureCanvasAgg):
+class FigureCanvasQTAgg(FigureCanvasQT, FigureCanvasQTAggBase,


I worry about this change a bit.

Indeed, both FigureCanvasQT and FigureCanvasQTAggBase define draw() methods, and the one from FigureCanvasQT is supposed to be overwritten. So FigureCanvasQTAggBase must be the first class for now.

pwuertz · 2015-08-27T18:47:42Z

Yea, seems to be a problem with the python MRO. The glueviz problem is caused by making FigureCanvasQT the first class instead of FigureCanvasQTAggBase, although making the QObject the first class this is the preferred layout according to the PyQt documentation.

… init.

pwuertz · 2015-08-27T19:14:47Z

Reverted to original class order and rebased to current master.

tacaswell · 2015-08-29T03:42:28Z

@astrofrog @jrevans Can you both please check the impact of this branch on your applications?

tacaswell · 2015-08-29T03:47:01Z

lib/matplotlib/backend_bases.py

@@ -2763,6 +2763,10 @@ def draw_rubberband(self, event, x0, y0, x1, y1):
        """Draw a rectangle rubberband to indicate zoom limits"""
        pass

+    def remove_rubberband(self):


Do any other backends implement this?

Not at this time. I found a class named RubberbandBase in backend_tools.py that looks like a template for rubberband functionality, including a remove_rubberband function. Gtk3 and TK seem to implement this class. However, this code path is never used. Seems to me that this solution has been abandoned at some time and now only draw_rubberband implemented by the canvas is used.

Other way around, the toolbase work is functionality that is just starting to be built out by @fariza and @OceanWolf

@pwuertz It's not abandoned, It's just the beginning. For now only Gtk3 and Tk are implemented, we are waiting for MEP27 to land to finish the other backends

Ah, well then porting this solution to the toolbase framework should be straight forward since it implements the same draw/remove concept.

Yes, the codepath does get used see FigureManagerGTK3 and FigureManagerTkAgg which calls this codepath via backend_tools.default_tools. In the upcoming 1.5 release this will go out as an experimental proof of concept, then we will begin work on the final polishing, MEP27 does this a bit (see #4143), and other commits related to MEP22 (see #3652). So for 2.1 we should have this ready to go out fully.

Feel free to test, just set rcParam['toolbar'] = 'toolmanager', but as we say, only on GTK3 and TKAgg for now.

astrofrog · 2015-08-29T12:43:34Z

@tacaswell @pwuertz - just confirming that this now works with glueviz

MNT: Move agg draw to main thread and fix rubberband in Qt

tacaswell added the status: needs review label Aug 20, 2015

pwuertz mentioned this pull request Aug 20, 2015

qt backend has more draws than necessary #4943

Merged

pwuertz mentioned this pull request Aug 20, 2015

Qt5: Add flag that informs paintEvent if the agg buffer needs to updated, fix rubberband #4962

Closed

tacaswell added this to the next point release milestone Aug 22, 2015

tacaswell reviewed Aug 27, 2015
View reviewed changes

pwuertz added 3 commits August 27, 2015 21:11

Qt: Redraw agg buffer in main thread. Implement FigureCanvasQTAggBase…

6a74cda

… init.

Add handler for removing the rubberband when zoom is released

f744e7c

Fix rubberband removal in Qt backend

d27ad98

tacaswell reviewed Aug 29, 2015
View reviewed changes

tacaswell added a commit that referenced this pull request Sep 1, 2015

Merge pull request #4972 from pwuertz/qt5_fixes_combined

3122457

MNT: Move agg draw to main thread and fix rubberband in Qt

tacaswell merged commit 3122457 into matplotlib:master Sep 1, 2015

tacaswell removed the status: needs review label Sep 1, 2015

tacaswell mentioned this pull request Oct 6, 2015

pyside is broken #5196

Closed

pwuertz deleted the qt5_fixes_combined branch February 24, 2016 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qt5: Move agg draw to main thread and fix rubberband #4972

Qt5: Move agg draw to main thread and fix rubberband #4972

pwuertz commented Aug 20, 2015

tacaswell commented Aug 20, 2015

pwuertz commented Aug 20, 2015

pwuertz commented Aug 20, 2015

tacaswell commented Aug 20, 2015

tacaswell commented Aug 22, 2015

tacaswell commented Aug 23, 2015

astrofrog commented Aug 23, 2015

tacaswell commented Aug 23, 2015

astrofrog commented Aug 23, 2015

astrofrog commented Aug 27, 2015

pwuertz commented Aug 27, 2015

tacaswell commented Aug 27, 2015

pwuertz commented Aug 27, 2015

tacaswell Aug 27, 2015

pwuertz Aug 27, 2015

pwuertz commented Aug 27, 2015

pwuertz commented Aug 27, 2015

tacaswell commented Aug 29, 2015

tacaswell Aug 29, 2015

pwuertz Aug 29, 2015

tacaswell Aug 29, 2015

fariza Aug 29, 2015

pwuertz Aug 29, 2015

OceanWolf Aug 29, 2015

astrofrog commented Aug 29, 2015

Qt5: Move agg draw to main thread and fix rubberband #4972

Qt5: Move agg draw to main thread and fix rubberband #4972

Conversation

pwuertz commented Aug 20, 2015

tacaswell commented Aug 20, 2015

pwuertz commented Aug 20, 2015

pwuertz commented Aug 20, 2015

tacaswell commented Aug 20, 2015

tacaswell commented Aug 22, 2015

tacaswell commented Aug 23, 2015

astrofrog commented Aug 23, 2015

tacaswell commented Aug 23, 2015

astrofrog commented Aug 23, 2015

astrofrog commented Aug 27, 2015

pwuertz commented Aug 27, 2015

tacaswell commented Aug 27, 2015

pwuertz commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwuertz commented Aug 27, 2015

pwuertz commented Aug 27, 2015

tacaswell commented Aug 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Aug 29, 2015