Updated patch to not override alpha on edgecolor if set to none by cimarronm · Pull Request #1938 · matplotlib/matplotlib · GitHub
Skip to content

Updated patch to not override alpha on edgecolor if set to none - #1938

Closed
cimarronm wants to merge 1 commit into
matplotlib:masterfrom
cimarronm:edgecolor_alpha_fix
Closed

cimarronm wants to merge 1 commit into
matplotlib:masterfrom
cimarronm:edgecolor_alpha_fix

Conversation

@cimarronm

Copy link
Copy Markdown
Contributor

Addresses issue #1934.

@pelson

pelson commented Apr 24, 2013

Copy link
Copy Markdown
Member

@cimarronm

Copy link
Copy Markdown
Contributor Author

@pelson: You are correct. The problem is that there is no special color designation for no edge. Instead 'none' gets translated in set_edgecolor into (0,0,0,0) and it becomes black with alpha=0 (i.e. one loses the context that it was no edge).

The real solution I see is no allow a color designation of none. But that is not straightforward with the current code that I can see. There are three posibilities I can readily see:

  1. Allow color to be a RGBA tuple or 'none' - code handling the color value gets a bit ugly as you have to check if it is a string or list/tuple.
  2. Allow color to be a RGBA tuple or None - much cleaner implementation but the problem is the existing setter/getter UI associates None with a default color rather than no color (it would have been much better to use 'default' or something like that instead of None for the setter IMHO but it is what it is).
  3. Allow color to be a RGBA tuple but use a special tuple value for a no-color designation, e.g., (-1,-1,-1,-1) - this seems like the best solution to me but not sure what your/others opinion on it

Do you think one of the three is the way to go or do you see an alternative approach?

@Westacular

Copy link
Copy Markdown
Contributor

Actually, the way facecolor is handled in Patch served as a template for what edgecolor ought to do -- internally preserve what edgecolor was originally set to, so in cases like edgecolor='none', it knows to not let alpha override that (the logic for this is already present in colorConverter.to_rgba()). Commit 461233c in #1954 makes this change.

@cimarronm

Copy link
Copy Markdown
Contributor Author

Sounds good to me. I will close this PR

@cimarronm cimarronm closed this May 11, 2013
@pelson

pelson commented May 13, 2013

Copy link
Copy Markdown
Member

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.

3 participants