This fix changes_draw_text_as_path and_draw_text_as_text inlib/matplotlib/backends/backend_svg.py, so that whencolor is supplied tomatplotlib.pyplot.text with an RGBA value, the alpha channel is used. Previously, the alpha channel fromcolor was being ignored. The changes added include extra conditions to checkGraphicsContextBase's_forced_alpha value before setting text opacity based on theGraphicsContextBase's alpha value.

Fixes#10419

People to notify:@anntzer @tacaswell @mdboom

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant

anntzer reviewed

Mar 13, 2018

View reviewed changes

lib/matplotlib/backends/backend_svg.py Outdated

		ifcolor!='#000000':
		style['fill']=color
		ifgc.get_alpha()!=1.0:
		ifgc.get_forced_alpha()andgc.get_alpha()!=1.0:

Copy link

Contributor

anntzerMar 13, 2018

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

thuvejanMar 13, 2018

Choose a reason for hiding this comment

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

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?

Copy link

Contributor

anntzerMar 13, 2018

Choose a reason for hiding this comment

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

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)

Copy link

ContributorAuthor

thuvejanMar 13, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Contributor

anntzer commentedMar 13, 2018

Nice, looks good modulo small fix.

Copy link

Contributor

anntzer commentedMar 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.

Copy link

ContributorAuthor

thuvejan commentedMar 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?

Copy link

Contributor

anntzer commentedMar 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).

Changed SVG text drawing logic to consider alpha of RGBA color

cd9d84f

thuvejan force-pushed the10419-svg-alpha-text branch fromb78ae5a tocd9d84fCompare

March 13, 2018 19:57

Copy link

Contributor

anntzer commentedMar 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).