Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jklymak commentedJan 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Using Using 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 made |
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 :) |
Great! Just wanted you to know that it worked, but wasn't blazingly fast. and of course that you didn't seem to break |
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.
This was fine on my machine. But I don't embed GUIs etc...
7bb3819
tof59f0e9
Comparesurface = 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) |
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.
Doesn't this result in a double-draw (once from the canvasdraw
method and once from here)?
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, 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.
I'm a little hesitant to merge this if@anntzer is busy in the next week and it breaks a lot of things.... |
I am comfortable enough with this part of the code to not be worried about this. |
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. |
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.
@tacaswell 's changes look good.
Thanks for helping me getting this in! 😃 |
DietmarSchwertberger commentedFeb 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The The Replacing But if ever there was code to call The inheritance for the wx backends:
Without multiple inheritance, would the super class of |
anntzer commentedFeb 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Can confirm the issue. In the meantime the following patch seems to work
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 commentedFeb 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Wouldn't it be better to implement a This here is not exactly peformance-optimized, but should actually behave the same as before:
Later this year, I would like give it a try whether the |
Regarding my question from above whether I had a look at |
The |
Regarding the first point: |
DietmarSchwertberger commentedFeb 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.) |
(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...) |
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 commentedFeb 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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... |
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 commentedFeb 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I would suggest to take Antony's code with my modification for |
Update: with the merged Cairo PR, the |
@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...). |
Uh oh!
There was an error while loading.Please reload this page.
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