-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
patheffects for Line2d object : rebase of #1015 #1909
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
My comments in #1015 are alleviated a little by the addition of these very good tests. Thanks for adding them @leejjoon - I think this is a really neat feature to add. My only question then is, is it worth us updating the signature of the "draw" family of methods such that those formats which can apply multiple effects to a single path instance (Agg and SVG spring to mind) can make use of the effects directly, rather than be forced to draw the same paths multiple times with differing effects? |
I am reluctant to change the backend API, which requires changes in all the backends out there. def draw_path_with_effects(self, gc, path, transform, rgbFace=None, path_effects=None):
if path_effects:
for pe in path_effects:
pe.draw_path(self, gc, path, transform, rgbFace)
else:
self.draw_path(gc, path, transform, rgbFace) And let the artists call |
Yeah -- I think given that there are a handful of third-party backends out there, I'm wary of changing the backend api, though extending it with new methods, with a working fallback in the base class has worked well for us in the past. For example, BTW -- this functionality will come in really handy when I finish off the "sketch" (xkcd) style feature in #1329 -- there it would be nice to have a white outline behind the lines as well, so I can probably just use this. |
@leejjoon: I'd like to merge this without the proposed backend changes (more-or-less as-is). Is there any reason you can think of not to? |
@mdboom, I don;t see any reason not to merge it. Do you want me to make another PR? The current one have merge conflict. On the other hand, I have a branch with the proposed backend changes (https://github.com/leejjoon/matplotlib/tree/patheffect-backend-refactor). I have not done thorough test yet, but at least it passes travis. I am planning to make a PR of this shortly but I think it is better for the post 1.3 release. |
This is rebase of #1015 with further changes. I updated the changelog and added tests.