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 Hsluv support #225

Merged
merged 5 commits into from May 23, 2021
Merged

Add Hsluv support #225

merged 5 commits into from May 23, 2021

Conversation

masonium
Copy link
Contributor

@masonium masonium commented May 14, 2021

Round three!

This finally implements the HSLuv color space as Hsluv. A few notes.

  • I didn't implement Uniform generation for Hsluv because I wasn't convinced that the naive version would be anything close to correct. I think you could do something like rejection sample in Lchuv (based on if, after converting that to Xyz, it were in-gamut) and then convert the result to Hsluv, but that's a very different approach than anything I see currently implemented.
  • I also added macros for implementing core::ops::Add and related traits, since they're pretty repetitive. I also added tests to confirm that all of the arithmetic operations are implemented (though there is no correctness checking).
  • I kept l as the field name, rather than lightness, since it is the same as the Lchuv l.

Closes #112

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.

Nice! Good call with the macros. I think they could be used for the other spaces as well. As for the random sampling, I'm pretty sure you can simply use the same method as "regular" HSL uses, which is to sample a bi-cone, where the saturation becomes slimmer towards min and max lightness. There's already code for it in the HSL module. It's going to be its own space in a way, since it's warped from Luv's perspective, but I think that's fine. It's meant to be a more balanced alternative to the naive sampling (which anyone can do on their own), but not necessarily perfect in every way.

/// Implement `Add` and `AddAssign` traits for a color space.
///
/// Both scalars and color arithmetic are implemented.
#[macro_export]
Copy link
Owner

Choose a reason for hiding this comment

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

I always forget the details, but I'm pretty sure #[macro_export] is unnecessary for internal macros, but the order the modules are declared in the lib.rs file matters. Macros and macro modules have to come first. I would suggest putting this module inside the macros module that's already in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I bet the ordering was tripping me up on macros. I'll take your suggestion and just move them to macros.rs, though. I'll also do a bi-cone style implementation for Hsluv.

@Ogeon
Copy link
Owner

Ogeon commented May 22, 2021

Nice, thank you! It looks to me like this is ready to be merged. I just need to make the no_std_test work again. Looks like it requires __aeabi_unwind_cpp_pr0 now, all of a sudden. Could be that some dependency changed.

@Ogeon
Copy link
Owner

Ogeon commented May 22, 2021

It should hopefully be fixed now in the master branch 🤞, so you need to merge it in here or rebase your branch. Whatever you are more comfortable with. 🙂

@masonium
Copy link
Contributor Author

I re-based the branch on top of master.

@Ogeon
Copy link
Owner

Ogeon commented May 23, 2021

Nice and the tests passed this time, so it seem to be all good! Thank you for all of this! I'll just take the liberty to add a "closing" line to the description, so that goes into the machinery.

@Ogeon
Copy link
Owner

Ogeon commented May 23, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented May 23, 2021

Build succeeded:

@bors bors bot merged commit fad9c87 into Ogeon:master May 23, 2021
@Ogeon Ogeon changed the title Hsluv implementation Add Hsluv support Jul 4, 2021
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.

HSLuv support
2 participants