Skip to content
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

patheffects for Line2d object : rebase of #1015 #1909

Closed

Conversation

leejjoon
Copy link
Contributor

This is rebase of #1015 with further changes. I updated the changelog and added tests.

@pelson
Copy link
Member

pelson commented Apr 16, 2013

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?

@leejjoon
Copy link
Contributor Author

I am reluctant to change the backend API, which requires changes in all the backends out there.
I know that the signature of draw_text has been changed recently, but I don't think it was best thing to do.
Maybe a better way is to propose new APIs with a good default behavior, similar to draw_path_collection. For example, we may implement RendereBase.draw_path_with_effects

  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 draw_path_with_effects rather than draw_path if needed.
By default, it will work for all the backends. And backends can override draw_path_with_effects method if necessary.
I guess this is something @mdboom needs to comment.

@mdboom
Copy link
Member

mdboom commented Apr 16, 2013

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, draw_path_collection does not need to be implemented in the backend. As long as the backend implements draw_path, collections will just work (albeit without any optimizations). However, a backend can override draw_path_collection if it makes sense to do so to take advantage of an optimization. I think we could do the same here.

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.

@pelson
Copy link
Member

pelson commented Apr 17, 2013

However, a backend can override draw_path_collection if it makes sense to do so to take advantage of an optimization. I think we could do the same here.

Sounds good to me. Thanks @leejjoon & @mdboom.

@mdboom
Copy link
Member

mdboom commented May 13, 2013

@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?

@leejjoon
Copy link
Contributor Author

@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.

@mdboom
Copy link
Member

mdboom commented May 14, 2013

#1329 now incorporates your commits from here (since I used the line path effects to put a white background behind all of the sketch lines, and it looks great). So I think we can just go ahead and close this PR since it will be merged for 1.3 as part of #1329.

@mdboom mdboom closed this May 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants