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

Add set luminance using a binary search #105

Closed
wants to merge 1 commit into from

Conversation

JuanPotato
Copy link

Closes #89

@sharkdp
Copy link
Owner

sharkdp commented Oct 22, 2019

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 {
Copy link
Owner

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.

Copy link
Author

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.

@JuanPotato
Copy link
Author

Should I put the function in the same file when extracting it?

@sharkdp
Copy link
Owner

sharkdp commented Oct 22, 2019

Should I put the function in the same file when extracting it?

yes, I think that's fine.

@JuanPotato
Copy link
Author

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

@sharkdp
Copy link
Owner

sharkdp commented Oct 29, 2019

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?

@JuanPotato
Copy link
Author

I know the lab = lab.into_color().to_lab(); line is due to that issue. Fixing it would definitely help.

@sharkdp
Copy link
Owner

sharkdp commented May 26, 2022

Closing for now since this is rather old. We can always resurrect it later.

@sharkdp sharkdp closed this May 26, 2022
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.

Add "pastel set luminance"
2 participants