-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add set luminance using a binary search #105
Conversation
Thank you very much for your contribution! I would prefer if we extract the binary search code into a separate function and add some tests for it. Without having looked at the code in detail: if we modify a color in Lab space (which is what we should do for this problem 👍), we have to make sure not to leave the sRGB gamut. The library will handle this for us but one often ends up with strangely saturated colors. Maybe this is not a problem here, since we are only modifying the lightness channel here which probable means that the sRGB gamut surface will be reached when the color is completely white or black. |
lab.l = (max_l + min_l) / 2.0; | ||
|
||
// If we haven't made any changes, get out before we loop infinitely | ||
if past_l == lab.l { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really the only thing that could happen? Couldn't the L
value also toggle between two values? To be safe, we could also use an iteration counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I'll put in an iteration counter.
Should I put the function in the same file when extracting it? |
yes, I think that's fine. |
Well, there's a small issue with the binary search. It never manages to get all the way to #FFFFFF white or #000000 black. If I change the min and max range for the binary search to a past 0 and 100, so -1 and 101. It will get to pure white and black |
Do you think this could be related to #108? |
I know the |
Closing for now since this is rather old. We can always resurrect it later. |
Closes #89