Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
tacaswell merged 3 commits intomatplotlib:masterfrompwuertz:qt5_fixes_combined
Sep 1, 2015
Merged

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

tacaswell merged 3 commits intomatplotlib:masterfrompwuertz:qt5_fixes_combined
Sep 1, 2015

Conversation

pwuertz
Copy link
Contributor

Follow up from#4962

@tacaswell
Copy link
Member

This needs a rebase, sorry.

@pwuertz
Copy link
ContributorAuthor

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 aremove_rubberband() handler tobackend_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 twodraw_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 lastdraw_rubberband() call before releasing the mouse might get delayed until after thedraw_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
Copy link
ContributorAuthor

@jrevans Do you have an example for the situation you described wherepaintEvent is called whileself.renderer doesn't exist yet?

@tacaswell
Copy link
Member

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

@tacaswelltacaswell added this to thenext point release milestoneAug 22, 2015
@tacaswell
Copy link
Member

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
Copy link
Member

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

@astrofrog
Copy link
Contributor

@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
Copy link
Member

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

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

@tacaswellhttps://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
Copy link
Contributor

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

@astrofrog
Copy link
Contributor

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

Before (master):

screen shot 2015-08-27 at 6 13 11 pm

After (this PR):

screen shot 2015-08-27 at 6 09 44 pm

@pwuertz
Copy link
ContributorAuthor

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

@tacaswell
Copy link
Member

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

@pwuertz
Copy link
ContributorAuthor

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...

@@ -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,
Copy link
Member

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.

Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

Reverted to original class order and rebased to current master.

@tacaswell
Copy link
Member

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

@@ -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):
Copy link
Member

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?

Copy link
ContributorAuthor

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.

Copy link
Member

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

Copy link
Member

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

Copy link
ContributorAuthor

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.

Copy link
Member

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
Copy link
Contributor

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

tacaswell added a commit that referenced this pull requestSep 1, 2015
MNT: Move agg draw to main thread and fix rubberband in Qt
@tacaswelltacaswell merged commit3122457 intomatplotlib:masterSep 1, 2015
@tacaswelltacaswell mentioned this pull requestOct 6, 2015
@pwuertzpwuertz deleted the qt5_fixes_combined branchFebruary 24, 2016 18:45
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.

5 participants
@pwuertz@tacaswell@astrofrog@fariza@OceanWolf

[8]ページ先頭

©2009-2025 Movatter.jp