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

Implement bytemuck::Zeroable and bytemuck::Pod for every color type #229

Merged
merged 4 commits into from
May 29, 2021

Conversation

gymore-io
Copy link
Contributor

@gymore-io gymore-io commented May 28, 2021

I'm working on an experimental creative framework for Rust and I wanted to use one of palette types in a uniform buffer. This can be easily done through unsafe code but I think it is much cleaner to use the bytemuck crate which provide the Zeroable and Pod traits to convert types from and to raw bytes.

This pull request implements those traits for the color types defined in this crate.

Changes

I did the whole pull request in a single commit, implementing the traits for the following types.

  • Rgb<S, T> where T is Pod/Zeroable

  • Luma<S, T> where T is Pod/Zeroable

  • Hsl<S, T> where T is Pod/Zeroable

  • Hsluv<Wp, T> where T is Pod/Zeroable

  • Hsv<S, T> where T is Pod/Zeroable

  • Hwb<S, T> where T is Pod/Zeroable

  • Lab<Wp, T> where T is Pod/Zeroable

  • Lch<Wp, T> where T is Pod/Zeroable

  • Lchuv<Wp, T> where T is Pod/Zeroable

  • Xyz<Wp, T> where T is Pod/Zeroable

  • Yxy<Wp, T> where T is Pod/Zeroable

  • Alpha<C, T> where C and T are Zeroable

  • Alpha<C<T>, T> where C<T> is a color that is Pod and T is Pod
    eg. Alpha<Rgb<T>, T> is Pod if T is Pod

I also implemented Zeroable and Pod for the Packed<C> rgba struct. This required to make it implement Copy regardless of whether C was Copy.

Note

I really expect this change to be used with the Rgba / Rgb types, but I made the change for every color type for consistency.

Affected types: `Rgb`, `Luma`, `Hsl`, `Hsluv`, `Hsv`, `Hwb`, `Lab`, `Lch`, `Lchuv`, `Luv`, `Xyz`, `Yxy` as well as their `Alpha<C, T>` variant.
This requires `Packed` to be `Copy` regardless of whether `C` is `Copy`.
@Ogeon
Copy link
Owner

Ogeon commented May 29, 2021

I think this is a good idea. Better interoperability is always nice!

For the Alpha implementations, you could require the Pixel trait to be implemented, so some variant of impl<C, T> Pod for Alpha<C, T> where C: Pod, T: Pod, Self: Pixel. It requires safe casting to an array/slice, so Alpha only implements Pixel for when the component type is homogeneous. The odd cases where this doesn't apply will have to make specific, hand crafted implementations anyway.

Also, don't forget PreAlpha, which works like Alpha, but pre-multiplied.

@gymore-io
Copy link
Contributor Author

I did exactly that :) I wasn't aware of that Pixel<T> trait.

@Ogeon
Copy link
Owner

Ogeon commented May 29, 2021

Great! Pixel kind of fulfills the same purpose as Pod, to some extent, except that it's not as general purpose and not as easy to compose into larger structures. That's why I think it's good to have both. But in a case where you just need to go between, say, &[f32] and &[LinSrgb<f32>] the Pixel trait will have the small benefit that it's more type safe.

@Ogeon
Copy link
Owner

Ogeon commented May 29, 2021

Ah, I tried the "update branch" button and it made a merge from master. Completely unnecessary, but oh well. I think this PR is good as it is, so I'll just queue a merge to master. Thank you!

bors r+

@bors
Copy link
Contributor

bors bot commented May 29, 2021

Build succeeded:

@bors bors bot merged commit 0893b39 into Ogeon:master May 29, 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.

2 participants