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

Consider alpha channel from RGBA color of text for SVG backend text opacity rendering #10773

Merged
merged 1 commit into from Mar 14, 2018

Conversation

Projects
None yet
4 participants
@thuvejan
Copy link
Contributor

commented Mar 13, 2018

PR Summary

This fix changes _draw_text_as_path and _draw_text_as_text in lib/matplotlib/backends/backend_svg.py, so that when color is supplied to matplotlib.pyplot.text with an RGBA value, the alpha channel is used. Previously, the alpha channel from color was being ignored. The changes added include extra conditions to check GraphicsContextBase's _forced_alpha value before setting text opacity based on the GraphicsContextBase's alpha value.

Fixes #10419

People to notify: @anntzer @tacaswell @mdboom

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
@@ -901,8 +901,10 @@ def _draw_text_as_path(self, gc, x, y, s, prop, angle, ismath, mtext=None):
style = {}
if color != '#000000':
style['fill'] = color
if gc.get_alpha() != 1.0:
if gc.get_forced_alpha() and gc.get_alpha() != 1.0:

This comment has been minimized.

Copy link
@anntzer

anntzer Mar 13, 2018

Contributor

I would just remove the check on gc.get_alpha() here and have a straight if else... no?

This comment has been minimized.

Copy link
@thuvejan

thuvejan Mar 13, 2018

Author Contributor

I put that in because a similar check appears at line 421, and it sort of makes it clearer that alpha should only be set if explicitly told to do so. Should it be changed?

This comment has been minimized.

Copy link
@anntzer

anntzer Mar 13, 2018

Contributor

OK so we don't want to write the opacity if not needed (reasonable). I think here the flow is clearer with

alpha = gc.get_alpha() if gc.get_forced_alpha() else gc.get_rgb()[3]
if alpha != 1:
    style["opacity"] = short_float_fmt(alpha)

This comment has been minimized.

Copy link
@thuvejan

thuvejan Mar 13, 2018

Author Contributor

Yep, that's cleaner. 👍 Made the fix.

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

Nice, looks good modulo small fix.

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

One last point: one issue we have in general with the mpl repo is the large size of test images, so we try to avoid adding too many images if possible. In this specific case, what takes a lot of room is the "path-as-text" svg. Can you merge the two svgs into a single one, and reduce the path-as-text part to a single character (to reduce the amount of embedded data)? You can always increase the amount of "text-as-text" if you want to keep the image self-explanatory.

Once this is done, please squash out the earlier commits and force-push so that the large images don't stay in the repo.

@thuvejan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

So if I understand correctly, I should reduce the text labels in the svg that uses paths, to a single character because they take a lot space, but the text labels in the svg that uses text is fine because of it doesn't take a lot of space. Is this correct?

Also, I wanted to put both figures into one svg, but I wasn't able to find a solution that allowed me to set different rcParams on two subplots of one figure. Do you know of any other way to make the two svg into one?

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

Yes, your understanding is correct.
Ah, of course, the current implementation of backend_svg doesn't allow mixing at all. Well, let's just reduce the text-as-path to single characters then.
You can probably also gain a bit by calling gca().set_axis_off() (not drawing the axes).

@thuvejan thuvejan force-pushed the TeamChillUTSC:10419-svg-alpha-text branch from b78ae5a to cd9d84f Mar 13, 2018

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

Your email on the commit patch is marked as "thuvejan@users.noreply.github.com", it's up to you whether you want to put a real email there (either way let me know).

@thuvejan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

Yeah, that was intended 🙂

@thuvejan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

@anntzer Thanks for all the feedback. Any idea when this will be merged?

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

All PRs need two reviewers.

@jklymak jklymak merged commit 1707b1e into matplotlib:master Mar 14, 2018

8 checks passed

ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
ci/circleci: docs-python36 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project/library 68.6% (target 50%)
Details
codecov/project/tests 98.57% (+<.01%) compared to ccd6377
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

@jklymak jklymak added this to the v3.0 milestone Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.