Skip to content

Fix PathEffect rendering on some backends#5217

Merged
tacaswell merged 3 commits into
matplotlib:masterfrom
mdboom:xkcd-broken
Oct 9, 2015
Merged

Fix PathEffect rendering on some backends#5217
tacaswell merged 3 commits into
matplotlib:masterfrom
mdboom:xkcd-broken

Conversation

@mdboom

@mdboom mdboom commented Oct 9, 2015

Copy link
Copy Markdown
Member

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

mdboom commented Oct 9, 2015

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member

Might want to consider backing out #4201 and #4542

@mdboom

mdboom commented Oct 9, 2015

Copy link
Copy Markdown
Member Author

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

jkseppan commented Oct 9, 2015

Copy link
Copy Markdown
Member

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

mdboom commented Oct 9, 2015

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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

mdboom commented Oct 9, 2015

Copy link
Copy Markdown
Member Author

@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

jkseppan commented Oct 9, 2015

Copy link
Copy Markdown
Member

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

@mdboom

mdboom commented Oct 9, 2015

Copy link
Copy Markdown
Member Author

Added a test.

Comment thread lib/matplotlib/tests/test_backend_ps.py Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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