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

Fix PathEffect rendering on some backends #5217

Merged
merged 3 commits into from Oct 9, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Oct 9, 2015

The special sub-GC that is created by the PathEffectRenderer was of
GraphicsContextBase, rather than the backend-specific GC.

Fix #5049, Fix #4024

Note that while this fixes the crashes in #5049, it does not mean that the sketch effect behind xkcd actually works in all backends -- that's more of a feature enhancement than a bugfix, because it never worked on all backends.

The special sub-GC that is created by the PathEffectRenderer was of
GraphicsContextBase, rather than the backend-specific GC.

Fix matplotlib#5049, Fix matplotlib#4024
@mdboom mdboom added this to the next point release (1.5.0) milestone Oct 9, 2015
@mdboom
Copy link
Member Author

mdboom commented Oct 9, 2015

This is a tricky one to test -- it all involves running backends we don't currently test on Travis. Actually a PS test with a PathEffect would be sufficient, and wouldn't require installing anything special.

@tacaswell
Copy link
Member

Might want to consider backing out #4201 and #4542

@mdboom
Copy link
Member Author

mdboom commented Oct 9, 2015

That's a good point, but I think #4201 and #4542 are unrelated, actually. They are all part of the same cluster of bugs, but I don't think this would have fixed either of those.

@jkseppan
Copy link
Member

jkseppan commented Oct 9, 2015

The docstring of backend_bases.py calls GraphicsContextBase an "abstract base class" so it seems wrong to me that RendererBase.new_gc returns GraphicsContextBase instead of raising NotImplementedError, which would help us catch errors like this. Does any backend depend on this behavior?

@mdboom
Copy link
Member Author

mdboom commented Oct 9, 2015

@jkseppan: That's a good point. It does seem the Agg backend uses the new_gc from the base class, but none of the other renderers do. I'd be happy to make the base new_gc NotImplemented and add an implementation to RendererAgg.

@tacaswell
Copy link
Member

can we hold off on that bit of refactoring until 2.1? Worried about (possibly vaporware) external backends which also rely on this.

@mdboom
Copy link
Member Author

mdboom commented Oct 9, 2015

@tacaswell: Sure. That makes sense. It's not really a requirement that a renderer needs a custom GraphicsContext subclass to get things done, so it's quite possible some third-party ones don't and removing the base method would certainly break that.

@jkseppan
Copy link
Member

jkseppan commented Oct 9, 2015

I agree, this PR is a minimal bugfix and changing the method in the base class is an API change.

@mdboom
Copy link
Member Author

mdboom commented Oct 9, 2015

Added a test.

@cleanup
def test_patheffects():
with matplotlib.rc_context():
matplotlib.rcParams['path.effects'] = [patheffects.withStroke(linewidth=4, foreground='w')]
Copy link
Member

Choose a reason for hiding this comment

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

This line does not appease pep8

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully fixed now.

tacaswell added a commit that referenced this pull request Oct 9, 2015
Fix: PathEffect rendering on some backends
@tacaswell tacaswell merged commit fe6da77 into matplotlib:master Oct 9, 2015
tacaswell added a commit that referenced this pull request Oct 9, 2015
Fix: PathEffect rendering on some backends
@tacaswell
Copy link
Member

back-ported as ff2e4b9

@mdboom mdboom deleted the xkcd-broken branch November 10, 2015 02:46
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.

xkcd plots stopped working on Mac OS X. Path effects applied to annotation text containing \n
3 participants