Pinging@leejjoon.

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,

   forpathinpath_collection:forpeinpath_effects:pe.draw_path(path)

   where it should be

   forpeinpath_effects:forpathinpath_collection:pe.draw_path(path)

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!

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 thedraw_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

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.

classWhiteBgEffect(PathEffects.Stroke):def__init__(self,linewidth=3):self._gc=dict(linewidth=linewidth)defdraw_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.

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

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

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

instead of

ifself.get_path_effects():frommatplotlib.patheffectsimportPathEffectRendererrenderer=PathEffectRenderer(self.get_path_effects(),renderer)

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

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 thiswhen there is a requirement for a renderer subclass to optimise.

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 pltimport numpy as npimport matplotlib.patheffects as mpath_effectsl, = 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()

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.

What about this?

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

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.

Yes, please go ahead and merge it.

Before merging, can we add a brief entry towhats_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.

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

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

Thanks. Merging.