Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Qt5: Move agg draw to main thread and fix rubberband#4972
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
tacaswell commentedAug 20, 2015
This needs a rebase, sorry. |
pwuertz commentedAug 20, 2015
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 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 |
pwuertz commentedAug 20, 2015
@jrevans Do you have an example for the situation you described where |
tacaswell commentedAug 20, 2015
My reading of@jrevans comments is that was his guess as towhy the draw was there. That conditional can probably be removed. |
tacaswell commentedAug 22, 2015
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 commentedAug 23, 2015
@astrofrog Can you have a look at how this impacts glueviz? |
astrofrog commentedAug 23, 2015
@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 commentedAug 23, 2015
It does, qt4 is implemented as a shim on to of qt5. On Sun, Aug 23, 2015, 11:27 AM Thomas Robitaillenotifications@github.com
|
astrofrog commentedAug 23, 2015
@tacaswell - ok, in that case I will test this out tomorrow with glueviz |
astrofrog commentedAug 27, 2015
@tacaswell - sorry for the delay. Unfortunately this does break glueviz: Before (master): After (this PR): |
pwuertz commentedAug 27, 2015
Any idea why this would have an impact on glueviz while the regular gui operates normally? |
tacaswell commentedAug 27, 2015
glueviz does some monkey patching to try and control the draw cadence (if I recall correctly). I suspect that someplace they are calling |
pwuertz commentedAug 27, 2015
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... |
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 worry about this change a bit.
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.
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 commentedAug 27, 2015
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. |
pwuertz commentedAug 27, 2015
Reverted to original class order and rebased to current master. |
tacaswell commentedAug 29, 2015
@astrofrog@jrevans Can you both please check the impact of this branch on your applications? |
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 any other backends implement this?
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.
Not at this time. I found a class namedRubberbandBase in backend_tools.py that looks like a template for rubberband functionality, including aremove_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 onlydraw_rubberband implemented by the canvas is used.
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.
Other way around, the toolbase work is functionality that is just starting to be built out by@fariza and@OceanWolf
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.
@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
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.
Ah, well then porting this solution to the toolbase framework should be straight forward since it implements the same draw/remove concept.
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.
Yes, the codepath does get used seeFigureManagerGTK3 andFigureManagerTkAgg which calls this codepath viabackend_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 setrcParam['toolbar'] = 'toolmanager', but as we say, only onGTK3 andTKAgg for now.
astrofrog commentedAug 29, 2015
@tacaswell@pwuertz - just confirming that this now works with glueviz |
MNT: Move agg draw to main thread and fix rubberband in Qt


Follow up from#4962