-
Notifications
You must be signed in to change notification settings - Fork 61
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
Adding whitepoint for colors spaces #36
Conversation
I think it's best to implement |
Copy is already implemented for PhantomData. So i assumed it would work. |
The derive macro will still require type parameters to implement it. It can't know if it's necessary or not. |
I know this is a work in progress, but keep in mind what I said about dividing things into smaller parts. It will be significantly harder to review it if a lot of things are changed. |
I was first going to change only the cie colors. But then I realized that this was going to affect a lot more code like the Color enum and macros. Thats why I added the Rgb change as well since a lot of this was going to change as well. I didn't think from a reviewing point of view. |
I see, and I hope you don't see me as some kind of party pooper, who's just holding you back. That's not my intention at all. I do appreciate your help and it's really nice to have someone to exchange ideas with. I'm just doing my best to apply the principle where large problems are broken down into smaller problems to get a better overview and to make them easier to reason about. I prefer short incremental steps over long leaps, since many changes -> more complexity -> more to keep track of -> higher risk of bugs and errors. This applies to both the author and the reviewer. Now, in this particular case we have the white point part and the RGB generalization part, which is trivial to separate. The basic design of the white point part is more or less done, I would say, but the RGB part is still a bit fuzzy. There are still some unanswered questions, like what exactly the switchable RGB types represents. I would therefore prefer that we keep them separate to allow us to land the white point part without worrying about the RGB design. It's still possible to prepare the macros for the additional complexities. |
No Worries!! I got a bit stuck trying to think of a way around the Color Enum and the alpha_from and from_color macros. For Color enum as discussed, there is no other option but having to specify all the types. For the alpha_from and from_color macros. I can't think of a better way than separating the macros for the different trait signature cases i.e. one for and one for <WP, T>. For example |
The |
☔ The latest upstream changes (presumably #43) made this pull request unmergeable. Please resolve the merge conflicts. |
closed in favor of #56 |
This is a work in progress.
Implemented so far:
White points for lab and lch. I won't add the white points for Xyz and Yxy for now. Lets see how things shape up and take a call on that.
Issues:
Unable to derive copy for the struct with phantom data. Hence impleminting the Limited trait errors. Here is a link to the playground showing the issue. http://is.gd/tHCm5C
Notes
Had to comment the color enum and the from_color and alpha_color macros. These assume a single trait for all colors. This will show a lot of noise in the git diff.
Next
Make rgb, color space and white point aware.