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 Oklab support #200

Merged
merged 2 commits into from
Jun 25, 2021
Merged

Add Oklab support #200

merged 2 commits into from
Jun 25, 2021

Conversation

lunacookies
Copy link
Contributor

@lunacookies lunacookies commented Jan 6, 2021

Oklab assumes the D65 colour space. In my first attempt at implementing this, the Oklab struct didn’t have a Wp parameter. This caused all FromColorUnclamped derives to not compile, so I added the parameter back with comments warning about how it doesn’t have any effect. Ideally this wouldn’t be needed.

Closes #222

palette/src/oklab.rs Outdated Show resolved Hide resolved
Copy link
Owner

@Ogeon Ogeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good, white point problems aside, and it seems like a nice color space. I suppose we can go one step further too and add an "Oklch" variant of it too, for completeness' sake.

What were the errors you got from the derive macro when you tried to lock the white point? I would like to have that one figured out, rather than a workaround.

palette/src/oklab.rs Outdated Show resolved Hide resolved
palette/src/oklab.rs Outdated Show resolved Hide resolved
palette/src/oklab.rs Outdated Show resolved Hide resolved
@Ogeon
Copy link
Owner

Ogeon commented Jan 6, 2021

I saw some of your comments over at pale-fire, by the way. Once again, don't hesitate to ask for help or clarifications! This library has a lot of complexity under the hood, as you noticed, and the conversion system is the worst of them all. It's like one of those frontend/backend meme pictures... but with three layers of increasing weirdness.

@Ogeon Ogeon mentioned this pull request Jan 21, 2021
@Ogeon Ogeon mentioned this pull request May 10, 2021
@Ogeon
Copy link
Owner

Ogeon commented May 29, 2021

Hi! I hope all is well. I just wanted to check in and ask if you are still interested in completing this, or if you would rather hand it over to me (or someone else) to finish it. Either option is fine and all I want to know is how to proceed with this PR. There's no shame in wanting to do other things! 😄

The things I think are left to do are:

  • Rebase on top of the latest master, to catch up and solve conflicts.
  • Make it pass the tests by removing the left-over white points in the random generation code.
  • Add an implementation of bytemuck::Pod and bytemuck::Zeroable. (support for them was added today)
  • Add an Oklch color space. This would more or less be a copy of Lch, but may need a bit more of poking around in the derive macro.

I will help and guide you if you need it, so don't worry about that.

@lunacookies
Copy link
Contributor Author

Hey, thanks for the kind words! Sorry for disappearing for so long. I’d be interested in finishing this PR.

I just rebased on master, but the commit history is quite ugly and broken in some places; would you like me to squash? I’ve also fixed the tests and added the bytemuck trait implementations.

Do you think it would be better to keep an Oklch implementation in a separate PR, or add it in this one?

@Ogeon
Copy link
Owner

Ogeon commented May 30, 2021

Great! Don't worry about being away. It happens. I should also have checked in earlier, but time ended up passing faster than anticipated. But then I thought that it would be a good idea to have this sorted out before I get around to make the next release and all.

Either way, a squash sounds like a good idea! And I think it would be good to have Oklch here in the same PR, mostly just to see that they work well together before merging, because of the special casing in the derive macro.

@lunacookies
Copy link
Contributor Author

I’ve squashed the commits, brought the PR up to date with master and added an Oklch implementation. There are still some failing tests, though. @Ogeon would you mind taking a look?

@lunacookies lunacookies marked this pull request as draft June 16, 2021 11:17
@Ogeon
Copy link
Owner

Ogeon commented Jun 16, 2021

Great! Thanks! I will have a look later when I get off work. 💼

Comment on lines +343 to +351
impl<T> FromColorUnclamped<Oklch<T>> for Xyz<D65, T>
where
T: FloatComponent,
{
fn from_color_unclamped(color: Oklch<T>) -> Self {
let oklab: Oklab<T> = color.into_color_unclamped();
Self::from_color_unclamped(oklab)
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were there any issues with letting the macro derive this automatically?

@Ogeon
Copy link
Owner

Ogeon commented Jun 16, 2021

I think it looks great! I had one question, above, but that should be all. 🎉

@Ogeon
Copy link
Owner

Ogeon commented Jun 16, 2021

Oh, it looks like the random distribution is a bit off for Oklch. I didn't see it earlier because the connection to travis broke when they shut down the .org version. Here's a link until it starts working again https://travis-ci.com/github/Ogeon/palette/jobs/514952866

Comment on lines 719 to 723
Oklch<f32> as crate::Oklab {
l: (0.0, 1.0),
a: (-1.0, 1.0),
b: (-1.0, 1.0),
},
Copy link
Owner

@Ogeon Ogeon Jun 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I know what's going on with the random numbers. I remember having the same issue before. You need to make the limits for a and b smaller, to +/-0.7 (cos 45°) to make it look for a square that will fit inside the Oklch circle. Otherwise it will not generate any points in the corners and the average density becomes uneven.

@Ogeon
Copy link
Owner

Ogeon commented Jun 20, 2021

I have migrated the CI over to github actions, by the way, so you will need to rebase this branch again. Sorry about that, but the new setup should at least be more responsive.

@lunacookies
Copy link
Contributor Author

@Ogeon any idea why GitHub Actions aren’t running?

@Ogeon
Copy link
Owner

Ogeon commented Jun 25, 2021

I just have to manually approve them for first time contributors...

@lunacookies lunacookies marked this pull request as ready for review June 25, 2021 07:44
@Ogeon
Copy link
Owner

Ogeon commented Jun 25, 2021

Alright, all the tests are passing and the code looks good! I think it's time to finally merge this. Thank you for all the work and also for coming back and finishing it!

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 25, 2021

Build succeeded:

@bors bors bot merged commit 7223627 into Ogeon:master Jun 25, 2021
@lunacookies
Copy link
Contributor Author

Thank you ❤️

@lunacookies lunacookies deleted the add-oklab branch June 25, 2021 08:18
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.

Implement Oklab and Oklch
3 participants