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

implement path_effects for Line2D objects #655

Closed
wants to merge 2 commits into from

Conversation

leejjoon
Copy link
Contributor

@leejjoon leejjoon commented Jan 3, 2012

This patch implements path_effects for Line2D objects.

Here is a related request on the mailing list

http://www.mail-archive.com/matplotlib-users@lists.sourceforge.net/msg22954.html

The current change modifies the Line2D class to take a single path_effects argument. However, there are two paths involved with a single Line2D instance.

  • a path for the line
  • a path for the marker edge & face

And currently, same path_effects is applied to both paths.
I'm not sure if there is a need to apply separate path_effects for those two.
We may need an optional argument (e.g., marker_path_effects).

@jdh2358
Copy link
Collaborator

jdh2358 commented Feb 26, 2012

I would like to test this PR, but it would be easier if there were some example or test. Could you add something simple?

@dmcdougall
Copy link
Member

I tried this and got a nice shadow on the legend! Works fine for me on Mac OS X 10.7, Python 2.7.2 and PDF backend. When I checked out your code I had to fiddle with a conflict in lines.py, just to let you know. Otherwise this looks fine.

@mdboom
Copy link
Member

mdboom commented Jun 1, 2012

Sorry this got "dropped" for so long. I think this needs a regression test (do we have any for path effects yet? Maybe that warrants a new test directory). Other than that and the other comments above, I think this is probably ready to merge.

@mdboom
Copy link
Member

mdboom commented Jun 1, 2012

Also -- can you rebase against the current master?

@jdh2358
Copy link
Collaborator

jdh2358 commented Jul 14, 2012

I second the request to see this rebased against master so we can move this PR forward

@leejjoon
Copy link
Contributor Author

The rebased version of this pull request is available at #1015. I am closing this PR.

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

4 participants