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

MakeColor panics when alpha is 0 #21

Closed
mikegleasonjr opened this issue Aug 29, 2017 · 5 comments
Closed

MakeColor panics when alpha is 0 #21

mikegleasonjr opened this issue Aug 29, 2017 · 5 comments

Comments

@mikegleasonjr
Copy link

Because when a is zero, a division by 0 occurs.

@mikegleasonjr mikegleasonjr changed the title MakeColor panics when alpha is 0 MakeColor panics when alpha is 0 Aug 29, 2017
@lucasb-eyer
Copy link
Owner

Ouch, you are right. And it's actually undefined what should be done in that case, as the original RGB values have been lost in the alpha pre-multiplication at that point.

I'm not sure what the right call is here. Either return an error and let the user handle it, or default to black, or to a user-specified default value. Thoughts?

@mikegleasonjr
Copy link
Author

I really don't know how to handle this to be honest.

@lucasb-eyer
Copy link
Owner

I'll leave this issue open until someone comes up with a good suggestion, meanwhile I'll add an entry to the FAQ in the README, linking here.

@muesli
Copy link
Contributor

muesli commented May 17, 2018

Since colorful doesn't handle/implement alpha channels (that fact could be stated more prominently in the README), I'd consider it acceptable if it loses the original RGB values in such a situation, and sets them to be either 0 or 255.

Alternatively, we could return an additional error value from this function. Anything's better than panicking in a library, right? 😁

Just my two cents, happy to provide a PR.

@lucasb-eyer
Copy link
Owner

Yeah these are the two only remotely sensible options, but neither is truly satisfying. The error return would be inconvenient as most people would probably just set it to all-zero, but at least it would force that decision on the user.

How about setting them to 0 (since that's the limit they converge to as alpha goes to 0) and having an ok/error return value, which can be ignored if the default all-zero is desired, but which would have the user at least partially think about it. What do you think?

I'd be happy to accept a PR that also adds a test and updates the README in the corresponding places.

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

No branches or pull requests

3 participants