Supporting different alphas for face and edge colours #1954

Merged
merged 8 commits into from May 11, 2013

Conversation

Projects
None yet
4 participants
@Westacular
Contributor

Westacular commented Apr 27, 2013

This is a set of changes that solve the issues raised by #1899 and #1938.

Linestyles

Ignore what I said before in #1899 about linestyles.

Turns out it was working correctly for patches, but the dashes and dots were all overlapping at linewidth=5. The culprit is a single line in Patch.draw() that was setting the gc to use 'projecting' capstyle. This capstyle causes the rendered length of dashes and dots to increase in proportion to the width, however, the spacing between them (which ignores the caps) remains constant. At linewidth=5 the spaces are completely covered by the caps, making it look like linestyle='solid'.

Letting capstyle='butt', the default, fixes this.

(Also, there was a bug in the PGF backend that was causing Collections (or anything with a custom dash pattern) to always use a solid linestyle. Fixed that.)

Also, for some reason, in the SVG header stroke-linecap:square(the translation of capstyle='projecting') was being set as the default, even though the default for most objects is butt. This was causing a problem with the rendering of the PathCollection, because (at least on WebKit) the style for the collection, which did specify butt, was not overriding that default. The net effect was the same as the above problem for patches.

Mixed Edge and Face Alpha Values

There was already stubs of logic (and a _forced_alpha attribute of GraphicsContext) for the notion that if an explicit alpha is set (through gc.set_alpha()) it should override any face and edge alphas, but otherwise, leave them be; however, it was largely ignored by most backends, each of which tended to do its own slightly different thing with regard which it recognized and how it handled them.

I've updated (and tested to varying degrees) the AGG, PDF, SVG, PGF, Mac OS X, and Cairo backends, along with various fixes to the backend bases and to the Patch class. As far as I can tell, that covers everything that's already paying attention to the gc.get_alpha() value.

I also fixed the bug (also discussed at #1938) where setting an alpha value on the patch would override edgecolor='none'. The new behaviour is that the value given to alpha is ignored by the edge colour if and only if edgecolor='none' exactly; edgecolor=(x,x,x,0) still gets overridden by alpha. This is the existing behaviour for facecolor, which I've not changed.

I've included the test (and changes) added by @pelson in #1899, along with correct results, and corrected the results of the clip_to_bbox test (which were previously affected by the alpha=x/edgecolor='none' bug). I've also added another test to check that the alpha attribute is overriding the alpha components of the face and edge colors.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Apr 29, 2013

Member

The test results are looking good - there is a lot of change here which needs to be considered, but I'd like to target this for v1.3.

Amazing work @Westacular.

Member

pelson commented Apr 29, 2013

The test results are looking good - there is a lot of change here which needs to be considered, but I'd like to target this for v1.3.

Amazing work @Westacular.

@Westacular

This comment has been minimized.

Show comment Hide comment
@Westacular

Westacular Apr 29, 2013

Contributor

Thanks.

Just noticed the Travis build had failed. I didn't have inkscape installed, so I didn't notice there was a regression with SVG in my own testing. I've since fixed the problem (hatches were not having their alpha applied in SVGs).

Contributor

Westacular commented Apr 29, 2013

Thanks.

Just noticed the Travis build had failed. I didn't have inkscape installed, so I didn't notice there was a regression with SVG in my own testing. I've since fixed the problem (hatches were not having their alpha applied in SVGs).

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 1, 2013

Member

I didn't have inkscape installed, so I didn't notice there was a regression with SVG in my own testing. I've since fixed the problem (hatches were not having their alpha applied in SVGs).

Wooho! Travis caught a genuine problem! 😄

Member

pelson commented May 1, 2013

I didn't have inkscape installed, so I didn't notice there was a regression with SVG in my own testing. I've since fixed the problem (hatches were not having their alpha applied in SVGs).

Wooho! Travis caught a genuine problem! 😄

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 3, 2013

Owner

@Westacular: Would you mind rebasing this on current master so that this will merge cleanly and the tests will run again?

Owner

mdboom commented May 3, 2013

@Westacular: Would you mind rebasing this on current master so that this will merge cleanly and the tests will run again?

pelson and others added some commits Apr 12, 2013

Fixes bug where if alpha is set and edgecolor='none', edges would sti…
…ll be rendered as black.

Also fixes test case result that hit this bug
Changes backends to support of independent face and edge alphas
Specifically, alters the base, AGG, PDF, PGF, SVG, Cairo, and Mac OS X
backends to better enable the use of RGBA color values for both fills
and edges. An explicit alpha attribute, if set, will override the
alpha channels of the color values.

Updates test results, which are now what would be expected.

Also fixes a couple bugs with handling of linestyles.
@Westacular

This comment has been minimized.

Show comment Hide comment
@Westacular

Westacular May 3, 2013

Contributor

@Westacular: Would you mind rebasing this on current master so that this will merge cleanly and the tests will run again?

Done. (Also squashed some of the commits, where it made sense.)

Contributor

Westacular commented May 3, 2013

@Westacular: Would you mind rebasing this on current master so that this will merge cleanly and the tests will run again?

Done. (Also squashed some of the commits, where it made sense.)

@@ -289,7 +289,7 @@ def _write_default_style(self):
writer = self.writer
default_style = generate_css({
u'stroke-linejoin': u'round',
- u'stroke-linecap': u'square'})
+ u'stroke-linecap': u'butt'})

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 3, 2013

Owner

Good catch -- indeed the "default" according to the SVG spec is butt.

@mdboom

mdboom May 3, 2013

Owner

Good catch -- indeed the "default" according to the SVG spec is butt.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 3, 2013

Owner

Thanks for all of this work. It looks great. I think it's significant enough that it should have an entry in what's new -- and perhaps in API changes, if there's a way of summarizing changes in behavior between old and new (even though old was obviously broken)...

Owner

mdboom commented May 3, 2013

Thanks for all of this work. It looks great. I think it's significant enough that it should have an entry in what's new -- and perhaps in API changes, if there's a way of summarizing changes in behavior between old and new (even though old was obviously broken)...

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 3, 2013

Member

and perhaps in API changes

Definately.

Member

pelson commented May 3, 2013

and perhaps in API changes

Definately.

self._rgb = fg
else:
self._rgb = colors.colorConverter.to_rgba(fg)
- if len(self._rgb) == 4 and not self._forced_alpha:
- self.set_alpha(self._rgb[3])

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 3, 2013

Member

This is subtle, but I think it will break the non forced alpha case.

@pelson

pelson May 3, 2013

Member

This is subtle, but I think it will break the non forced alpha case.

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 3, 2013

Member

Question: Should calling set_alpha turn on the forced_alpha value?

@pelson

pelson May 3, 2013

Member

Question: Should calling set_alpha turn on the forced_alpha value?

This comment has been minimized.

Show comment Hide comment
@Westacular

Westacular May 3, 2013

Contributor

set_alpha does turn on _force_alpha -- in fact, it already did, I didn't need to change anything there. (But prior these changes, practically nothing actually paid attention to it.) Calling gc.set_alpha(None) clears _forced_alpha.

This doesn't break the non-forced alpha case because the backends themselves, where appropriate, now pull the alpha value straight out of _rgb[3] in the non forced alpha case.

@Westacular

Westacular May 3, 2013

Contributor

set_alpha does turn on _force_alpha -- in fact, it already did, I didn't need to change anything there. (But prior these changes, practically nothing actually paid attention to it.) Calling gc.set_alpha(None) clears _forced_alpha.

This doesn't break the non-forced alpha case because the backends themselves, where appropriate, now pull the alpha value straight out of _rgb[3] in the non forced alpha case.

ctx.set_source_rgba (fill_c[0], fill_c[1], fill_c[2], alpha)
else:
- ctx.set_source_rgba (fill_c[0], fill_c[1], fill_c[2], alpha*fill_c[3])

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 3, 2013

Member

Ouch. We don't have cairo tests to check this either (we need to think about testing the cairo backend more). Nice spot.

@pelson

pelson May 3, 2013

Member

Ouch. We don't have cairo tests to check this either (we need to think about testing the cairo backend more). Nice spot.

+ elif fillcolor is None or len(fillcolor) < 4:
+ gc._effective_alphas = (gc._rgb[3], 1.0)
+ else:
+ gc._effective_alphas = (gc._rgb[3], fillcolor[3])

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 3, 2013

Member

Your effective alphas approach here is, IMHO, something that could be done in the GC object. The GC could then just draw an RGBA of fill and an RGBA of line color _and the backend wouldn't need to know anything about forced_alpha.

Do you know of a reason why that wouldn't be a viable approach?

@pelson

pelson May 3, 2013

Member

Your effective alphas approach here is, IMHO, something that could be done in the GC object. The GC could then just draw an RGBA of fill and an RGBA of line color _and the backend wouldn't need to know anything about forced_alpha.

Do you know of a reason why that wouldn't be a viable approach?

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 3, 2013

Member

I guess what I'm suggesting is that we updated the necessary signatures (to draw_path for example) such that we pass through an RGBA rather than an RGB and expect the backend to figure out the final face RGBA color.

@pelson

pelson May 3, 2013

Member

I guess what I'm suggesting is that we updated the necessary signatures (to draw_path for example) such that we pass through an RGBA rather than an RGB and expect the backend to figure out the final face RGBA color.

This comment has been minimized.

Show comment Hide comment
@Westacular

Westacular May 3, 2013

Contributor

That actually is how draw_path is working, now, but I couldn't (or rather, didn't) guarantee that everything else worked that way.

The issue is there are instances where the GC is applying an alpha property to things that have neither fill or draw colours -- e.g., images -- which is done by setting an alpha on the backend's underlying context. (The Mac OS X backend comes to mind as an example.) Supporting that means:

  • The GC still needs to make use of its _alpha in some situations
  • We don't want this to stack on top of the RGBA specified in the colors
  • So we ought to guard against that.

I believe for draw_path itself, this isn't needed because the other changes I've made mean that gc.set_alpha isn't called for that -- the alphas (overridden or not) go directly in the edge and fill colours. But there are many other draw methods that I haven't vetted that could possibly call gc.set_alpha() and give it colours with alpha channel values and I was concerned about them clashing.

@Westacular

Westacular May 3, 2013

Contributor

That actually is how draw_path is working, now, but I couldn't (or rather, didn't) guarantee that everything else worked that way.

The issue is there are instances where the GC is applying an alpha property to things that have neither fill or draw colours -- e.g., images -- which is done by setting an alpha on the backend's underlying context. (The Mac OS X backend comes to mind as an example.) Supporting that means:

  • The GC still needs to make use of its _alpha in some situations
  • We don't want this to stack on top of the RGBA specified in the colors
  • So we ought to guard against that.

I believe for draw_path itself, this isn't needed because the other changes I've made mean that gc.set_alpha isn't called for that -- the alphas (overridden or not) go directly in the edge and fill colours. But there are many other draw methods that I haven't vetted that could possibly call gc.set_alpha() and give it colours with alpha channel values and I was concerned about them clashing.

@Westacular

This comment has been minimized.

Show comment Hide comment
@Westacular

Westacular May 4, 2013

Contributor

Thanks for all of this work. It looks great. I think it's significant enough that it should have an entry in what's new -- and perhaps in API changes, if there's a way of summarizing changes in behavior between old and new (even though old was obviously broken)...

Happy to help. I took a crack a explaining the high-level/user-facing changes, while omitting the finer details of other bugs that were fixed or how backends were changed. Let me know what you think.

Contributor

Westacular commented May 4, 2013

Thanks for all of this work. It looks great. I think it's significant enough that it should have an entry in what's new -- and perhaps in API changes, if there's a way of summarizing changes in behavior between old and new (even though old was obviously broken)...

Happy to help. I took a crack a explaining the high-level/user-facing changes, while omitting the finer details of other bugs that were fixed or how backends were changed. Let me know what you think.

doc/api/api_changes.rst
+ :class:`~matplotlib.patches.Patch` had ``alpha=None``, the alpha component
+ of ``edgecolor`` would be applied to both the edge and face.
+
+* For :class:`~matplotlib.patches.Patch`, the ``capstyle`` used is now

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 7, 2013

Member

This is SVG only, right?

@pelson

pelson May 7, 2013

Member

This is SVG only, right?

+Note that if you set the alpha attribute for the patch object (e.g. using
+:meth:`~matplotlib.patches.Patch.set_alpha` or the ``alpha`` keyword
+argument), that value will override the alpha components set in both the
+face and edge colors.

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 7, 2013

Member

Nicely put. And sounds like sensible behaviour too. - that is a good sign 😉

@pelson

pelson May 7, 2013

Member

Nicely put. And sounds like sensible behaviour too. - that is a good sign 😉

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 7, 2013

Member

I see this as the "open heart surgery" of v1.3 (@mdboom's terminology used for a PR of mine on transforms in v1.2). I think on the whole the behaviour is more consistent, expected and desirable as a result of this PR, and it is for that reason (and the thoroughness of @Westacular's work) that I'd be comfortable merging this.

👍 - awesome work @Westacular!

Member

pelson commented May 7, 2013

I see this as the "open heart surgery" of v1.3 (@mdboom's terminology used for a PR of mine on transforms in v1.2). I think on the whole the behaviour is more consistent, expected and desirable as a result of this PR, and it is for that reason (and the thoroughness of @Westacular's work) that I'd be comfortable merging this.

👍 - awesome work @Westacular!

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 7, 2013

Owner

Agreed! Awesome work!

I know this issue has been brought up by @efiring a number of times over the years, so I thought I'd give him the opportunity to comment before merging, just in case there's anything additional.

Owner

mdboom commented May 7, 2013

Agreed! Awesome work!

I know this issue has been brought up by @efiring a number of times over the years, so I thought I'd give him the opportunity to comment before merging, just in case there's anything additional.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring May 7, 2013

Owner

I'm happy to see this.

There is one change in the formally public API that could be mentioned in API_CHANGES: in GraphicsContext*.set_foreground, the isRGB kwarg is now isRGBA. I think the change is fine, and it is unlikely that user code is calling this low-level method directly, so I don't think a deprecation process is needed.

Owner

efiring commented May 7, 2013

I'm happy to see this.

There is one change in the formally public API that could be mentioned in API_CHANGES: in GraphicsContext*.set_foreground, the isRGB kwarg is now isRGBA. I think the change is fine, and it is unlikely that user code is calling this low-level method directly, so I don't think a deprecation process is needed.

@Westacular

This comment has been minimized.

Show comment Hide comment
@Westacular

Westacular May 8, 2013

Contributor

Ok, I've added a note about the isRGBA change.

Thanks for the kind words... And thanks for trusting me with the scalpel. :)

Contributor

Westacular commented May 8, 2013

Ok, I've added a note about the isRGBA change.

Thanks for the kind words... And thanks for trusting me with the scalpel. :)

efiring added a commit that referenced this pull request May 11, 2013

Merge pull request #1954 from Westacular/alpha-patch
Supporting different alphas for face and edge colours

@efiring efiring merged commit 60e67e0 into matplotlib:master May 11, 2013

1 check passed

default The Travis CI build passed
Details

@pelson pelson referenced this pull request Oct 9, 2013

Merged

Rastized background color #2479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment