Drop pgi support for the GTK3 backend #13014
Conversation
| @@ -11,8 +11,7 @@ | |||
|
|
|||
| import numpy as np | |||
|
|
|||
| # cairocffi is more widely compatible than pycairo (in particular pgi only | |||
| # works with cairocffi) so try it first. | |||
| # cairocffi is more widely compatible than pycairo so try it first. | |||
There was a problem hiding this comment.
Perhaps it makes sense to prefer pycairo now? (as pygobject will go through pycairo regardless).
In fact, does pygobject even work (in this use case) without pycairo?
There was a problem hiding this comment.
I saw that there is code to convert pycairo objects to cairocffi ones for some fastpath implementations, so I assumed it is still preferred.
If there is anything I can do in pycairo to make it the preferred one I'm happy to help.
There was a problem hiding this comment.
In fact, does pygobject even work (in this use case) without pycairo?
No, it can be build without pycairo but then any function using cairo types will not work.
There was a problem hiding this comment.
Which means in particular on_draw_event (connected to the "draw" event), no?
IOW, can we just assume that gtk3 is built with cairo support and that cairo is a dependency, and error out early if not?
I don't think keeping the cairocffi optional dependency just for the sake of a not-even-twofold speedup is worth it (but if you want to make expose something similar in pycairo's side what would be needed is a way to create a cairo Path from a list of cairo_path_data_t (or better, from a buffer (in the memoryview sense), so that no copy at all is done). (Effectively, this runs afoul of the last paragraph of https://cairographics.org/manual/bindings-path.html ("You should not present an API for mutating or for creating new cairo_path_t objects. In the future, these guidelines may be extended to present an API for creating a cairo_path_t from scratch for use with cairo_append_path() but the current expectation is that cairo_append_path() will mostly be used with paths from cairo_copy_path()."), but performance wise it does make some difference.)
There was a problem hiding this comment.
Which means in particular on_draw_event (connected to the "draw" event), no?
yeah
IOW, can we just assume that gtk3 is built with cairo support and that cairo is a dependency, and error out early if not?
We have https://pygobject.readthedocs.io/en/latest/guide/api/api.html?highlight=require_foreign#gi.require_foreign to check for this.
I don't think keeping the cairocffi optional dependency just for the sake of a not-even-twofold speedup is worth it [...]
Thanks, I'll have a look.
Either way I think any changes to the cairo part should be another PR.
There was a problem hiding this comment.
It was originally done to support py3 but I guess yeah I don't know the situation with pypy right now either, @lazka?
There was a problem hiding this comment.
It was originally done to support py3
Yeah, the pycairo in Ubuntu 16.04 for example (the distro provided package) is lacking various API compared to the Python 2 version, and also has a few bugs/crashers.
I don't know the situation with pypy right now either,
PyGObject and Pycairo do work with PyPy now: https://lazka.github.io/posts/2018-04_pypy-pygobject/index.html but pycairo is probably slower than cairocffi with PyPy.
There was a problem hiding this comment.
xenial has pycairo 1.10 but we require 1.11 indeed.
There was a problem hiding this comment.
@anntzer if I just take the code in _append_paths_fast() that computes codes/vertices and loop over them and call the path functions I get the same FPS boost as with the buffer passing approach (unless I'm missing something). So I think the whole cairo_path_data_t thing is not needed and the fast path would also works with pycairo as is.
(tested with wire3d_animation_sgskip)
There was a problem hiding this comment.
ah, the "fast path" cleans up the path first, so they aren't doing the same thing. The main speedup comes from removing the curve segments. So assuming that shortcut is OK this would be faster than the current fast path for both libraries: (turned out to be wrong, see my PRs)
# Mapping from Matplotlib Path codes to cairo path functions.
_MPL_TO_CAIRO_PATH_TYPE = [lambda ctx, *args: None] * (Path.CLOSEPOLY + 1)
_MPL_TO_CAIRO_PATH_TYPE[Path.MOVETO] = cairo.Context.move_to
_MPL_TO_CAIRO_PATH_TYPE[Path.LINETO] = cairo.Context.line_to
_MPL_TO_CAIRO_PATH_TYPE[Path.CLOSEPOLY] = \
lambda ctx, *args: cairo.Context.close_path(ctx)
def _append_path(ctx, path, transform, clip=None):
cleaned = path.cleaned(transform, remove_nans=True, clip=clip)
for points, code in zip(cleaned.vertices, cleaned.codes):
_MPL_TO_CAIRO_PATH_TYPE[code](ctx, *points)|
A note in the changelog would be nice. |
|
@anntzer do you mean in doc/users/next_whats_new? Or somewhere else? |
|
I would put it in https://github.com/matplotlib/matplotlib/tree/master/doc/api/next_api_changes as dropping support for |
0995df8 to
0adcd96
Compare
done |
.travis.yml
Outdated
| echo 'pgi is available' || | ||
| echo 'pgi is not available' | ||
| python -mpip install --upgrade pycairo cairocffi>=0.8 | ||
| python -mpip install --upgrade PyGObject==3.30.4 && |
There was a problem hiding this comment.
Is there a reason PyGObject is pinned to this specific version?
There was a problem hiding this comment.
Just the latest stable release to keep the CI environment stable and in case PyGObject drops Ubuntu 16.04 support, even though there are no plans to do so right now.
I can remove it if you want.
There was a problem hiding this comment.
Not quite sure on our version policy for CI. Keeping it stable and always testing the mos recent one both have their benefits. Nevertheless, I would go with the most recent.
@tacaswell Do you have an opinion on version pinning here?
There was a problem hiding this comment.
I've removed the pinning now.
There was a problem hiding this comment.
a belated 👍 to removing the pinning. Would rather deal with broken CI than finding out after things are released.
It's incomplete and unmaintained and PyGObject is pip installable now and supports Ubuntu Xenial which is used on Travis-CI. matplotlib should preferably test with things that are actually used by end users.
0adcd96 to
41ba610
Compare
|
Thanks, and congratulations on your first contribution to Matplotlib! Usually, I‘d say: Hope to see you back some time. But I know, you‘ve already some quite good PRs in the pipeline. So I‘m quite sure about that 😃 . |
PR Summary
It's incomplete and unmaintained and PyGObject is pip installable now
and supports Ubuntu xenial which is used on travis-ci.
matplotlib should preferably test with things that are actually used by end users.
PR Checklist