Undesirable behaviour of MixedModeRenderer #13021
Conversation
| # methods (because things like RendererAgg change their | ||
| # methods on the fly in order to optimise proxying down | ||
| # to the underlying C implementation). | ||
| return getattr(self._renderer, attr) |
There was a problem hiding this comment.
I would prefer explicitly listing the functions that get forwarded to the renderer, whether by checking attr in _methods in __getattr__, or by writing a bunch of forwarding functions
for _attr in _methods:
locals()[_attr] = lambda self, *args, **kwargs: getattr(self._renderer, _attr)(*args, **kwargs)
(in spirit; add a call to functools.wraps, etc.)
The reason is that the Renderer API is already quite large, and while implementing https://github.com/anntzer/mplcairo I found out that there are some methods that are not documented but are actually needed for a fully functional renderer backend; making the list explicit should hopefully avoid letting that list further grow out of control.
There was a problem hiding this comment.
I can see your point of view here @anntzer, but I disagree. In this case we are implementing a renderer proxy. A proxy, IMO, should forward everything on to the thing it is proxying. It is the thing it's proxying's responsibility to define what is and isn't allowed.
In that regard, the list of methods should be documented in the base, not the proxy. Indeed, they are partially documented at https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backend_bases.py#L135-L145, with the remainder being only documented as stub methods.
The simplest argument against explicitly naming all methods is this:
if we update the base renderer to have new capabilities, we should not have to update the proxy renderer
But also:
if a subclass of a base renderer implements it own methods, the proxy renderer should honour (i.e. proxy) them
There was a problem hiding this comment.
I agree with not explicitly listing the methods here.
There was a problem hiding this comment.
Away from serious reviewing for the Christmas period but the complete proxying approach of this PR is fine too even though I think (per the comment below) that a more explicit approach is better.
There was a problem hiding this comment.
I also agree that we should proxy out everything.
|
Other than the style point above, looks reasonable to me. |
|
Looks good. The svg image test fails. |
|
The test currently fails (you probably need to just set extensions=['png', 'pdf'] to exclude svg from testing). However, we tend to prefer not adding more baseline images nowadays, because they make the repo size balloon out of control. The feature demo can then go to examples/, perhaps next to misc/demo_agg_filter. |
| for method in self._methods: | ||
| if hasattr(renderer, method): | ||
| setattr(self, method, getattr(renderer, method)) | ||
| renderer.start_rasterizing = self.start_rasterizing |
There was a problem hiding this comment.
This was stuffing methods from the proxy renderer on to the proxied renderer. Is it a problem that we are going to lose this?
|
I'm worried that mocking an agg_filter application without using agg_filter will be more trouble than it's worth and am fine with these 2 test images going in. Maybe regenerate (and squash) with |
|
Ping @pelson on this one. 3.1 is going in soon... |
|
approval conditional on squashmerge |
…021-on-v3.1.x Backport PR #13021 on branch v3.1.x (Undesirable behaviour of MixedModeRenderer)
PR Summary
I recently discovered the
agg_filterfunctionality. For those who don't know, it is basically a hook to allow you to fiddle with the RGBA rasterization of an artist - way cool for doing lots of artistic style stuff... It is amazing that after all these years stuff still comes out of the woodwork for me with matplotlib 🤣.I was going to answer a long-standing question on StackOverflow which would really benefit from this capability - essentially, it is possible to rasterize anything and then apply your own alpha filter in the
agg_filterstep. Unfortunately, (and to be fair, the comments do state this is the case) it isn't possible to use theagg_filterwhen rasterizing inpdf, even though the rasterization is done with agg!Long-story short, the only good reason I can see that this capability wasn't added to the PDF backend is that it was a right pain to figure out what was wrong and get it working (like several hours of head scratching). It turns out that the MixedModeRenderer (the thing that is used when rasterizing in the PDF backend) contains 2 renderers (the raster one and the vector one) - it then jumps between the two as and when required. Unfortunately, when jumping, it snaffles a reference to the method instance of the renderer. If the renderer were to want to change its own implementation then obviously this would be a problem.
The AggRenderer, it turns out, fiddles with its own method instances when engaging with other renderers (as is the case for the
agg_filterfunctionality). So the MixedModeRenderer ends up calling the wrong renderer to do the drawing, and as a result nothing pops out at the other end...Phew. Still with me 😄? Rather than fix the AggRenderer to do its switching behind the method rather than fiddling with its method instances directly, I decided to fix the MixedModeRenderer to avoid holding references to the renderer's implementation detail. I could have gone either way on this (or frankly, both ways!) but decided to not touch the AggRenderer for fear of having more of a performance impact than I would otherwise have liked (I didn't measure this though).
The change itself is pretty straight-forward, and the test shows nicely what this capability makes possible.
This is my first mpl PR in several years, so go easy on an old-timer 😜
PR Checklist