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

Path effects update #2462

Merged
merged 2 commits into from Nov 4, 2013
Merged

Path effects update #2462

merged 2 commits into from Nov 4, 2013

Conversation

pelson
Copy link
Member

@pelson pelson commented Sep 25, 2013

Yet again, I've created a monolithic PR trying to get to the bottom of some of the cool functionality available in mpl.

Essentially this PR re-factors the way Path Effects are hooked together with the artist's draw method. The result is that this PR is predominately a tidy-up/deletion of existing code and a few fixes to handle the fallout from the re-factor.

My intention from here is to go on and write up a "Path effects" user guide section (and add some new path effects at the same time), but I haven't yet got on to that and think it would make sense to do it in a separate PR.

@pelson
Copy link
Member Author

pelson commented Sep 25, 2013

Pinging @leejjoon.

"""An abstract base class to handle drawing/rendering operations.

The following methods *must* be implemented in the backend:

* :meth:`draw_path`
* :meth:`draw_image`
* :meth:`draw_text`
* :meth:`get_text_width_height_descent`
* :meth:`_draw_text_as_path`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this. It looks like none of the backends (including on this branch) implement _draw_text_as_path, but it's listed here as required. Isn't _draw_text_as_path implemented in the base, calling draw_path in the concrete implementation? And the change here is that draw_text is no longer required? And isn't get_text_width_height_descent required if draw_text is implemented?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right _draw_text_as_path is not needed to be implemented - I think it is reasonable to just remove this line all together. I've made no fundamental change to the renderer to mean that one must/mustn't implement draw_text - it is just this documentation didn't reflect the reality of implementing your own renderer.

@leejjoon
Copy link
Contributor

I think the proposed refactoring is good overall. Here are some concerns though.

  1. My original intention was that we let patheffects to override not only the "draw_path" method but also
    "draw_path_collections", etc. I still think we should have this flexibility although this may complicate the code a bit.

  2. This is somewhat related to the above issue. Unless PathEffectRenderer override the draw_path_collection
    method explicitly, I think the drawing order of paths becomes incorrect (at least in my view).

    Currently, when PathEffectRender.draw_path_collection is called, the drawing order is something like below,

    for path in path_collection:
       for pe in path_effects:
           pe.draw_path(path)

    where it should be

    for pe in path_effects:
      for path in path_collection:
           pe.draw_path(path)

@pelson
Copy link
Member Author

pelson commented Sep 26, 2013

Thanks @leejjoon - great feedback! Looks like your biggest concern is the draw order of paths with path collections. I'll take a look at implementing the appropriate draw_path_collection method to address this concern.

I've now started writing up the path effects userguide, so will include it here once I've got an update.

Thanks again!

@pelson
Copy link
Member Author

pelson commented Sep 26, 2013

My original intention was that we let patheffects to override not only the "draw_path" method but also
"draw_path_collections"

Do you have a use case for that? I'm keen to make this code meet the use case requirements, and nothing more - provided the interface wouldn't need to change substantially (I don't think it would) I'm happy to leave the draw_path_collections idea in the "nice to have in the future" basket.

I've now updated the PR - I think there is some tidying up left to do, but I think this is starting to get rounded quite nicely.

Thanks again for your useful feedback @leejjoon

@leejjoon
Copy link
Contributor

Do you have a use case for that?

I guess one of the primary use case would be to optimize the performance by directly calling the renderer.draw_path_collection.

Here is a simple path effect that strokes a thick white line. For example, this can be used with contour lines.

class WhiteBgEffect(PathEffects.Stroke):
    def __init__(self, linewidth=3):
        self._gc = dict(linewidth=linewidth)

    def draw_path_collection(self, renderer, ...):
        edgecolors = np.array([(1, 1, 1, 1)])
        linewidths = np.array([[self._gc["linewidth"]]])

        renderer.draw_path_collection(... , edgecolors, linewidths, ...)

I think it is not just draw_path_collection. There could be cases where calling the renderer's draw_text, draw_marker, etc method directly is better, instead of falling back to draw_path.

@leejjoon
Copy link
Contributor

Similarly to the draw_path_collection, the draw_marker method, which can call draw_path multiple times, also need to be overridden.

@leejjoon
Copy link
Contributor

If we expect the path effects become widely used by users, I think it would be better if the path effects are more tightly connected with the Renderer class. For example, as we reimplement methods like "draw_path_collection" for performance reason, a certain backend may try to optimize the drawing with path effects. One way of doing this would be to do something like below

if self.get_path_effects():
        renderer = renderer.get_path_effect_renderer(self.get_path_effects())

instead of

if self.get_path_effects():
        from matplotlib.patheffects import PathEffectRenderer
        renderer = PathEffectRenderer(self.get_path_effects(), renderer)

And let renderers optionally override the get_path_effect_renderer to return a specialized PathEffectRenderer.

@pelson
Copy link
Member Author

pelson commented Sep 26, 2013

And let renderers optionally override the get_path_effect_renderer to return a specialized PathEffectRenderer.

Seems sensible to me. I'd be happy enough to implement this, but it is worth remembering that your suggestion doesn't fundamentally change the path effects implementation - meaning we could actually do this when there is a requirement for a renderer subclass to optimise.

@pelson
Copy link
Member Author

pelson commented Sep 26, 2013

Similarly to the draw_path_collection, the draw_marker method, which can call draw_path multiple times, also need to be overridden.

I'm trying to come up with a case where I can visually see a problem, I would have expected the following to cause a problem, but it renders as expected:

import matplotlib.pyplot as plt
import numpy as np
import matplotlib.patheffects as mpath_effects

l, = plt.plot(np.sin(np.linspace(0, 4 * np.pi, 100)), 'ob', linewidth=4, mec='red', mew=2, ms=20)
l.set_path_effects([mpath_effects.withSimplePatchShadow((5, -5))])
plt.show()

@leejjoon
Copy link
Contributor

I'm trying to come up with a case where I can visually see a problem, I would have expected the following to cause a problem, but it renders as expected:

What about this?

l.set_path_effects([mpath_effects.withSimplePatchShadow((-5, -5), alpha=1.)])

I can see artifacts.

@pelson
Copy link
Member Author

pelson commented Sep 26, 2013

What about this?

Actually, I think these artifacts have always been there with the with* effects. I've now fixed this so that for the non with* path effects, you get the expected result. Both approaches should now be backwards compatible.

@pelson
Copy link
Member Author

pelson commented Sep 27, 2013

Ok. I'm now happy with the result of this PR - I do intend to add some new PathEffects (and therefore an entry in the what's new) in the next couple of weeks, but I'm going to wait until this PR is in before doing that.

@leejjoon - would you be happy to merge this as it stands? I'm keen to get this in, and we can iterate further developments as and when the requirement arises.

Many thanks.

@leejjoon
Copy link
Contributor

Yes, please go ahead and merge it.

@mdboom
Copy link
Member

mdboom commented Sep 30, 2013

Before merging, can we add a brief entry to whats_new? I don't care if it says "TODO: Write me", it would just make my life a lot easier leading up to a release to not have to track down all significant changes and make sure they got an entry.

@pelson
Copy link
Member Author

pelson commented Oct 29, 2013

Just waiting for travis results to come in, but I think this is now ready for merge. @mdboom - would you do the honours?

@pelson
Copy link
Member Author

pelson commented Nov 4, 2013

@mdboom - I addressed your tuple(facecolors[i % Nfacecolors]) comment. I'm happy for this to be merged when you are.

@mdboom
Copy link
Member

mdboom commented Nov 4, 2013

Thanks. Merging.

mdboom added a commit that referenced this pull request Nov 4, 2013
@mdboom mdboom merged commit 84e0063 into matplotlib:master Nov 4, 2013
@pelson pelson deleted the path_effects_update branch January 9, 2014 14:05
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
Deprecated in matplotlib#2462 / 84e0063

attn @pelson had to known-fail a test which was using the
proxy renderer to verify that PathEffectRender was working
correctly.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
Deprecated in matplotlib#2462 / 84e0063

attn @pelson had to known-fail a test which was using the
proxy renderer to verify that PathEffectRender was working
correctly.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
Deprecated in matplotlib#2462 / 84e0063

attn @pelson had to known-fail a test which was using the
proxy renderer to verify that PathEffectRender was working
correctly.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 11, 2015
Deprecated in matplotlib#2462 / 84e0063

attn @pelson had to known-fail a test which was using the
proxy renderer to verify that PathEffectRender was working
correctly.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 22, 2015
Deprecated in matplotlib#2462 / 84e0063

attn @pelson had to known-fail a test which was using the
proxy renderer to verify that PathEffectRender was working
correctly.
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