Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Faster path drawing for the cairo backend (cairocffi only)#8787
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
fe66d3b to37775fbCompare| cur=ctx.get_current_point() | ||
| ctx.curve_to( | ||
| *np.concatenate([cur/3+points[:2]*2/3, | ||
| points[:2]*2/3+points[-2:]/3])) |
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 sure why this is different than before?
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.
previous version was actually incorrect (see basicallyhttps://en.wikipedia.org/wiki/B%C3%A9zier_curve#Degree_elevation)..
| def_convert_paths(ctx,paths,transforms,clip=None): | ||
| return (_convert_paths_fastifHAS_CAIRO_CFFIelse_convert_paths_slow)( |
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.
Maybeif at top-level would be quicker/simpler?
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.
By top-level, I meantif HAS_CAIRO_CFFI: _convert_paths = _convert_paths_fast ...
| def_convert_paths_fast(ctx,paths,transforms,clip=None): | ||
| # We directly convert to the internal representation used by cairo, for | ||
| # which ABI compatibility is guaranteed. The layout is for each item is |
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.
s/is for/for/
| transform= (transform | ||
| +Affine2D().scale(1.0,-1.0).translate(0,self.height)) | ||
| +Affine2D().scale(1,-1).translate(0,self.height)) |
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.
Could be+= to make one line?
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.
transforms are mutable but don't implement iadd (so += actually doesn't modify the transform in place), but I was too lazy to check and want to make clear that we're indeed not mutating the transform.
| transform= (transform | ||
| +Affine2D().scale(1.0,-1.0).translate(0,self.height)) | ||
| +Affine2D().scale(1,-1).translate(0,self.height)) |
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.
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.
as above
| self.convert_path( | ||
| ctx,marker_path,marker_trans+Affine2D().scale(1.0,-1.0)) | ||
| _convert_path( | ||
| ctx,marker_path,marker_trans+Affine2D().scale(1,-1)) |
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 fits on one line, no?
| vars(gc).update(gc_vars) | ||
| fork,vingc_vars.items(): | ||
| try: | ||
| getattr(gc,"set"+k)(v) |
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.
No underscore is needed?
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.
Actually no, I'm just copying the__dict__ of the original gc, and (kind of an implementation detail...) GraphicsContextBase stores "property"foo (accessed by get/set_foo) in._foo). Not very elegant I admit.
| antialiaseds,urls,offset_position): | ||
| path,transform=path_id | ||
| transform= (Affine2D(transform.get_matrix()).translate(xo,yo) | ||
| +Affine2D().scale(1,-1).translate(0,self.height)) |
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.
Why do you need the secondAffine2D?
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 don't.
| ifnew_key==reuse_key: | ||
| grouped_draw.append((path,transform)) | ||
| else: | ||
| _draw_paths() |
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 we need to worry about draw order and such 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.
The draw order is maintained, it's just that I batch the draws that use the same gc together.
| as_int[::4][cairo_type_positions]=codes | ||
| as_int[1::4][cairo_type_positions]=cairo_type_sizes | ||
| mask[::2][cairo_type_positions]=mask[1::2][cairo_type_positions]=False | ||
| as_float[mask]=vertices.ravel() |
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.
Could be re-arranged slightly to keep theint andfloat bits together.
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 sure what you meant?
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.
Well, you start with a buffer, make anint version, then afloat version, then a mask (which is forfloat). But then go back to filling theint version, modifying the mask (which isfloat stuff again), then fill thefloat version. So I mean more like:
buf=np.empty(cairo_num_data*16,np.uint8)as_int=np.frombuffer(buf.data,np.int32)as_int[::4][cairo_type_positions]=codesas_int[1::4][cairo_type_positions]=cairo_type_sizesas_float=np.frombuffer(buf.data,np.float64)mask=np.ones_like(as_float,bool)mask[::2][cairo_type_positions]=mask[1::2][cairo_type_positions]=Falseas_float[mask]=vertices.ravel()
anntzer commentedFeb 3, 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.
Comments addressed. |
a103612 to245f5deCompareanntzer commentedFeb 6, 2018
comments addressed |
QuLogic commentedFeb 8, 2018
Generally, I don't see a problem with this change, but using your microbenchmark, I can't seem to reproduce the speedup you see: vs. |
anntzer commentedFeb 8, 2018
Are you certain your gtk3cairo is using cairocffi rather than pycairo (which is not accelerated by this PR)? |
QuLogic commentedFeb 15, 2018
OK, I'm not really sure what happened, but I just tried again and I get 12.705427 vs. 17.470702 now. |
timhoffm left a comment
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 a cario expert, but look good to me.
Not really clear with the naming ofconvert_paths. To me convert feels like it should return the converted object. Maybeapply_paths? If that's not good, please add a short docstring to the convert functions.
flatten() always makes a copy, whereas ravel() does not.
The removed pair of ctx.save and ctx.restore was clearly unnecessary(the outer one is still there, and the font is reset at each loopiteration).
Improves the performance of mplot3d/wire3d_animation on the gtk3cairobackend from ~8.3fps to ~10.5fps (as a comparison, gtk3agg is at~16.2fps).
Further increase the performance of mplot3d/wire3d_animation on thegtk3cairo backend from ~10.5fps to ~11.6fps (as a comparison, gtk3agg isat ~16.2fps).
For the slow code path, implement the degree elevation formula.For the fast code path, the path cleaner was already handling this forus, converting everything to lines.
anntzer commentedMay 6, 2018
Renamed to _append_path, which matches the name of the function in the cairo API:https://cairographics.org/manual/cairo-Paths.html#cairo-append-path |
lazka commentedMay 28, 2018
Would it help you if pycairo had something like |
anntzer commentedMay 28, 2018
I moved my efforts to just doing everything in C(++) (https://github.com/anntzer/mplcairo) but I guess such a method can't hurt in general... |
lazka commentedMay 28, 2018
ok
yeah, but it's unmaintained atm |
Uh oh!
There was an error while loading.Please reload this page.
When using cairocffi only:
Accelerate Path drawing by directly constructing a cairo_path_t using numpy operations rather than repeatedly calling the cairo API to add one point at a time.
Accelerate PathCollection drawing by concatenating Paths that have the same drawing parameters (color, etc., i.e. GraphicsContext attributes).
Minor additional performance improvements (remove an unneeded ndarray copy and an unneeded context save/restore pair).
Overall, this brings the performance of examples/mplot3d/wire3d_animation.py (used as a microbenchmark) from ~8.3fps to ~11.6fps on the same computer (just optimizing Paths instead of PathCollections gives ~10.5fps), using the gtk3cairo backend. As a comparison point, the gtk3agg backend renders ~16.2fps for that example.
From profiling, I believe that further improvements (on that same microbenchmark) could be achieved by improving the text drawing (currently, we use the "toy" API, which is unfortunately the only one exposed by cairocffi so far; additionally, I am not certain whether ft2font exposes pointers to the underlying freetype structures).
Or (for other performance tests) draw_markers could probably also benefit from a similar approach as this PR.