Skip to content

Fix PathEffect rendering on some backends#5217

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

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

Conversation

@mdboom
Copy link
Copy Markdown
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.

@mdboom
Fix PathEffect rendering on some backends
bfab80e 
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
Copy Markdown
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
Copy Markdown
Member

tacaswell commented Oct 9, 2015

Might want to consider backing out #4201 and #4542

@mdboom
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

tacaswell commented Oct 9, 2015

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

mdboom commented Oct 9, 2015

Added a test.

tacaswell
tacaswell reviewed Oct 9, 2015
View reviewed changes
lib/matplotlib/tests/test_backend_ps.py Outdated
Copy link
Copy Markdown
Member

@tacaswell tacaswell Oct 9, 2015

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

@mdboom mdboom Oct 9, 2015

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
@tacaswell
Merge pull request #5217 from mdboom/xkcd-broken
fe6da77 
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
@tacaswell
Merge pull request #5217 from mdboom/xkcd-broken
ff2e4b9 
Fix: PathEffect rendering on some backends
@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Oct 9, 2015

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.5.0

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

@mdboom @tacaswell @jkseppan