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

qt{4,5}cairo backend: the minimal version.#10210

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
jklymak merged 4 commits intomatplotlib:masterfromanntzer:qtcairo-minimal
Feb 5, 2018

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedJan 10, 2018
edited
Loading

PR Summary

The stripped down (but sufficient) version of#8771. API docs and adding an entry to the backends tutorial are missing but will wait for#10203 to be merged first. Edit: docs added.

The idea is simply to move the parts of the code that were in Qt5Agg but are really only dependent on the GUI toolkit, but not on the renderer, to the base Qt5 classes (so that Qt5Cairo can share them). Note that there is some minimal testing via test_backends_interactive.

[The wxcairo and tkcairo backends PR will follow after this one is merged in order to not to conflict on rcsetup.py.]

[may be worth keeping qt5cairo "private" ("_qt5cairo")]

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzeranntzer added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelJan 13, 2018
@anntzeranntzer added this to thev2.2 milestoneJan 13, 2018
@anntzeranntzer mentioned this pull requestJan 15, 2018
6 tasks
@jklymak
Copy link
Member

jklymak commentedJan 16, 2018
edited
Loading

Usingmpl.use('Qt5Agg') works as expected on my hi-dpi mac for resizing and dragging the window around.

Usingmpl.use('Qt5Cairo') works as expected on my hi-dpi mac, except redraws are quite slow, so moving or resizing are quite laggy compared to Qt5Agg.

I just used a randomish bit of test code that happened to have a small pcolor in it:

importmatplotlibasmplmpl.use('Qt5Cairo')importmatplotlib.pyplotaspltimportmatplotlib.tickerastickerimportnumpyasnpvalues=np.random.rand(290,20)values[:26, :]=np.NaNvalues[ [57,239,253], :]=np.nanvalues=np.ma.masked_invalid(values)h,w=values.shape#h=262, w=20fig,ax=plt.subplots(figsize=(9,7))#fig, ax = plt.subplots()y=np.arange(0,290.5)x=np.arange(0,20.5)pcm=ax.pcolormesh(x,y,values)fig.colorbar(pcm)plt.xticks(np.arange(w),list('PNIYLKCVFWABCDEFGHIJ'))plt.show()

EDIT: Bumping the 290x20 pcolor up to 290x290 madeQt5Cairo completely unusable; It took about 60 seconds to even finish the first render.Qt5Agg was fine.

@anntzer
Copy link
ContributorAuthor

The current (pure-python) cairo backend implementation is not optimized for speed at all (and again, it is not thereal point of this PR :-)). You can play with#8787 if you want to improve the performance a bit, but really the way to do this is to write it in C(++)... which I did :)

@jklymak
Copy link
Member

Great! Just wanted you to know that it worked, but wasn't blazingly fast. and of course that you didn't seem to breakQt5Agg for me at least, including the DPI support. I'll spend a bit of time w/ the actual code to make sure I follow the changes and post a proper review soon.

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This was fine on my machine. But I don't embed GUIs etc...

@anntzeranntzerforce-pushed theqtcairo-minimal branch 2 times, most recently from7bb3819 tof59f0e9CompareJanuary 23, 2018 20:59
surface = cairo.ImageSurface(cairo.FORMAT_ARGB32, width, height)
self._renderer.set_ctx_from_surface(surface)
self._renderer.set_width_height(width, height)
self.figure.draw(self._renderer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Doesn't this result in a double-draw (once from the canvasdraw method and once from here)?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Indeed, this implementation is slighlty inefficient, because it was essentially copied from the implementation in gtk3cairo, which can only do a meaningful draw at the time of paintEvent (on_draw_event, for gtk3) (as that passes in a native draw context).
Here, best (and implemented in mplcairo) would be to check whether the current renderer already has an internal canvas of the correct size, and, if so, to blit it directly. Keeping that on my todo.

@jklymak
Copy link
Member

I'm a little hesitant to merge this if@anntzer is busy in the next week and it breaks a lot of things....

@tacaswell
Copy link
Member

I am comfortable enough with this part of the code to not be worried about this.

jklymak reacted with thumbs up emoji

@anntzer
Copy link
ContributorAuthor

Not sure if it's really an argument but you should in general feel free to revert my PRs if they break stuff and I don't have the bandwidth to take care of the breakage.

Copy link
Contributor

@dopplershiftdopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@tacaswell 's changes look good.

@jklymakjklymak merged commit0f42e10 intomatplotlib:masterFeb 5, 2018
@anntzer
Copy link
ContributorAuthor

Thanks for helping me getting this in! 😃

tacaswell reacted with heart emoji

@anntzeranntzer deleted the qtcairo-minimal branchFebruary 6, 2018 07:35
This was referencedFeb 8, 2018
@DietmarSchwertberger
Copy link
Contributor

DietmarSchwertberger commentedFeb 11, 2018
edited
Loading

Thesuper(FigureCanvasAgg, self).draw() inFigureCanvasAgg breaks the current WxAgg backend implementation. It may break other things as well.

Thedraw() method in FigureCanvasWxAgg callsFigureCanvasAgg.draw().
Then in turnFigureCanvasAgg callssuper(FigureCanvasAgg, self).draw() which here isFigureCanvasWx.draw(), i.e. the non-Agg version.

ReplacingFigureCanvasAgg.draw(self) withself.figure.draw(self.get_renderer()) inbackend_wxagg.FigureCanvasWxAgg.draw does fix things for me and seems the right thing anyway. I'm not sure about side effects, though.

But if ever there was code to callFigureCanvasAgg.draw, then things would fail assuper finds the wrong method.

The inheritance for the wx backends:

class FigureCanvasWxAgg(FigureCanvasAgg, FigureCanvasWx)class FigureCanvasWx(FigureCanvasBase, wx.Panel)class FigureCanvasAgg(FigureCanvasBase)

Without multiple inheritance, would the super class ofFigureCanvasAgg not be theFigureCanvasBase? Sosuper usually would not find the GUI class, right? I'm not familiar with the other backends (and also not yet really with the wx ones). WouldFigureCanvasAgg.draw() ever be called if the derived classes usefigure.draw(renderer)?

@anntzer
Copy link
ContributorAuthor

anntzer commentedFeb 11, 2018
edited
Loading

Can confirm the issue.
Basically this is due to the confusion between FigureCanvasWx being a base class for Wx widgets and the class specifically for RendererWx (if it was just a base class then it should not set the renderer and let the rendering subclass take care of that). In any case RendererWx is slated for removal in 2.2 and I think the correct solution is just to do the removal and rewrite RendererWx accordingly.

In the meantime the following patch seems to work

diff --git a/lib/matplotlib/backends/backend_wx.py b/lib/matplotlib/backends/backend_wx.pyindex 775107a08..85531d956 100644--- a/lib/matplotlib/backends/backend_wx.py+++ b/lib/matplotlib/backends/backend_wx.py@@ -721,7 +721,9 @@ class FigureCanvasWx(FigureCanvasBase, wx.Panel):         previously defined renderer if none is specified.         """         DEBUG_MSG("draw()", 1, self)-        self.renderer = RendererWx(self.bitmap, self.figure.dpi)+        from .backend_wxagg import FigureCanvasWxAgg+        if not isinstance(self, FigureCanvasWxAgg):+            self.renderer = RendererWx(self.bitmap, self.figure.dpi)         self.figure.draw(self.renderer)         self._isDrawn = True         self.gui_repaint(drawDC=drawDC)diff --git a/lib/matplotlib/backends/backend_wxagg.py b/lib/matplotlib/backends/backend_wxagg.pyindex 8383f50cd..482002cbd 100644--- a/lib/matplotlib/backends/backend_wxagg.py+++ b/lib/matplotlib/backends/backend_wxagg.py@@ -37,16 +37,11 @@ class FigureCanvasWxAgg(FigureCanvasAgg, FigureCanvasWx):     size.     """ -    def draw(self, drawDC=None):-        """-        Render the figure using agg.-        """-        DEBUG_MSG("draw()", 1, self)-        FigureCanvasAgg.draw(self)-+    def gui_repaint(self, drawDC=None, origin='WXAgg'):         self.bitmap = _convert_agg_to_wx_bitmap(self.get_renderer(), None)         self._isDrawn = True-        self.gui_repaint(drawDC=drawDC, origin='WXAgg')+        super(FigureCanvasWxAgg, self).gui_repaint(+            drawDC=drawDC, origin=origin)      def blit(self, bbox=None):         """

Note that the slightly ugly instance check in FigureCanvasWx.draw can be removed once the Wx backend is likewise removed.

PS: I think this warrants a release critical issue.@DietmarSchwertberger can you open it?

@DietmarSchwertberger
Copy link
Contributor

DietmarSchwertberger commentedFeb 11, 2018
edited
Loading

Wouldn't it be better to implement aget_renderer() method for theFigureCanvasWx instead of using the instance check?

This here is not exactly peformance-optimized, but should actually behave the same as before:

class FigureCanvasWx:    def get_renderer(self):        self.renderer = RendererWx(self.bitmap, self.figure.dpi)        return self.renderer    def draw(self, drawDC=None):        self.figure.draw( self.get_renderer() )        self._isDrawn = True        self.gui_repaint(drawDC=drawDC)

Later this year, I would like give it a try whether theFigureCanvasWx code could be used to render to aMetaFileDC. Having direct WMF/EMF export again would be nice on Windows. As of now, for rendering to a bitmap and then pasting to the screen, I agree that it does not have an added value.

@DietmarSchwertberger
Copy link
Contributor

Regarding my question from above whetherFigureCanvasAgg.draw() would be called at all.

I had a look atQTAgg.
When the application callscanvas.draw(), actuallyFigureCanvasAgg.draw() is called.
This, after drawing, in turn callssuper(FigureCanvasAgg, self).draw() which is actuallyFigureCanvasQT.draw() due to inheritance.
Somehow I don't like such a setup where calls to methods of the same name go back and forth between the classes. I would prefer a more direct approach with e.g.FigureCanvasQTAgg.draw andFigureCanvasAgg.draw calling eitherself.figure.draw(self.get_renderer()) orFigureCanvasAgg.render. The locking could be done insidefigure.draw().
Well, I don't care too much as this does not touch the Wx backends, but in the long run it would nice to have the same architecture for all (GUI and non-GUI) backends.

@tacaswell
Copy link
Member

Thedraw chain seems like pretty-standard OO style. I think it helps make supporting multiple rasterization backends (Agg, cairo, native, ...) under each GUI framework easier.

@anntzer
Copy link
ContributorAuthor

Regarding the first point:
I guess defining get_renderer on FigureCanvasWx works fine too (haven't looked in depth). The real question is whether we want to keep RendererWx around. A quick look athttps://wxpython.org/Phoenix/docs/html/wx.BitmapType.enumeration.html#wx-bitmaptype doesn't show EMF/WMF support (but I may be missing something)? Does Pillow support EMF/WMF?

@DietmarSchwertberger
Copy link
Contributor

DietmarSchwertberger commentedFeb 11, 2018
edited
Loading

The RendererWx seems to call standard wx.DC Draw methods. So it should be possible to render to a MetaFileDC. The point is to get high quality vector graphics. I would not place bitmap images in e.g. datasheets for publication. So Bitmap or Pillow do not help here. (At the moment the only way is to convert SVG graphics, which is working better with the recent Inkscape releases.)

@anntzer
Copy link
ContributorAuthor

(fwiw it may be ultimately possible to generate emf/wmf files with the cairo backend as well viahttps://cairographics.org/manual/cairo-Win32-Surfaces.html +https://msdn.microsoft.com/en-us/library/windows/desktop/ms535284(v=vs.85).aspx) (but that'll wait for mplcairo to be a bit more settled first...)

@tacaswell
Copy link
Member

We used to have a dedicated emf backend, but it broke and was removed in 2013 (#2026).

I think there exists a third-party EMF backend, but google is failing me.

@DietmarSchwertberger
Copy link
Contributor

DietmarSchwertberger commentedFeb 11, 2018
edited
Loading

I did use the EMF backend a long time ago and did even use it after removal, but it did not clip the lines IIRC. I did not find anything else at that time and so I switched to SVG->Inkscape->WMF...

By default Cairo-Win32-Surfaces seem to be 'raster surface type', even though for printing there seems to be a vector surface. Given the availability of wx vs. Cairo binaries on Windows, a WxEMF would be nicer.

This is not high-priority, though, as I only create datasheets once or twice a year and often I need to post-process them anyway...

@tacaswell
Copy link
Member

Back to the failure we have on master, which of the proposed solutions do we want to go with?

We are now over a week late on our target for 2.2rc1 so I'm happy with any solution that un-breaks the master branch, we can iterate as needed later.

@DietmarSchwertberger
Copy link
Contributor

DietmarSchwertberger commentedFeb 11, 2018
edited
Loading

I would suggest to take Antony's code with my modification forget_renderer.
I could test this locally on Windows and Ubuntu and upload a pull-request today or at latest tomorrow.
(I'm not yet comfortable with writing to master myself...)

@DietmarSchwertberger
Copy link
Contributor

Update: with the merged Cairo PR, thesuper(FigureCanvasAgg, self).draw() is a no-op as it calls the empty implementation fromFigureCanvasBase.
I think that this fixes the issue already and that's maybe why Antony has not seen the failure initially.
I will do some more testing, also with the non-Agg backend.

@anntzer
Copy link
ContributorAuthor

@DietmarSchwertberger We never write to master directly, everything must go through PRs with usually two other devs accepting the PR (the second one committing it). Seehttps://matplotlib.org/devel/coding_guide.html#pr-review-guidelines (although I guess this is not explicitly written there...).

@QuLogicQuLogic modified the milestones:needs sorting,v2.2.0Feb 12, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@dopplershiftdopplershiftdopplershift approved these changes

@jklymakjklymakjklymak approved these changes

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.

6 participants
@anntzer@jklymak@tacaswell@DietmarSchwertberger@dopplershift@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp