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 some missing From impls between Srgb and LinSrgb types #137

Merged
merged 2 commits into from Nov 24, 2019

Conversation

mitchmindtree
Copy link
Contributor

I ran into an issue downstream when attempting to write a generic
function that would accept any type of colour that may be converted to
Srgb<T>. I noticed implementations seemed to exist for all of the
colour types I cared about apart from LinSrgb<T>, so I thought I'd
implement the missing conversions and open a PR!

Let me know if there's a reason that these have been omitted that I'm
overlooking, or if you have a better way in mind for adding these
conversions! Unfortunately, conversions between entirely generic
Rgb<S, T> and Rgb<St, U> are impossible due to the existing blanket
impl From<T> for T, but adding individual implementations for the RGB
standards we care about doesn't seem so bad :)

@Ogeon
Copy link
Owner

Ogeon commented Jul 13, 2019

Hi! There were no particular reasons for not adding these, other than that I tend to not add too many convenience things from the start. There are a lot of possible combinations that can be made and it becomes hard to keep track of all of them after a while. But these are pretty standard so I see no harm in having them.

fn from(lin_srgb: LinSrgb<T>) -> Self {
let color = lin_srgb.into();
let alpha = U::max_intensity();
Alpha { color, alpha }
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm... You should not have to construct the Alpha like this. You should be able to use from_linear and into_format on these too. Otherwise that's also a hole in the API.

@Ogeon
Copy link
Owner

Ogeon commented Nov 16, 2019

Hi! Just checking to see if you are still interested in picking this up again. My only reason for not merging straight away is that I would like to know if it can be implemented in terms if from, {from,into}_linear and {from,into}_format. They should be available for every Alpha<Rgb> type.

I ran into an issue downstream when attempting to write a generic
function that would accept any type of colour that may be converted to
`Srgb<T>`. I noticed implementations seemed to exist for all of the
colour types I cared about apart from `LinSrgb<T>`, so I thought I'd
implement the missing conversions and open a PR!

Let me know if there's a reason that these have been omitted that I'm
overlooking, or if you have a better way in mind for adding these
conversions!  Unfortunately, conversions between entirely generic
`Rgb<S, T>` and `Rgb<St, U>` are impossible due to the existing blanket
`impl From<T> for T`, but adding individual implementations for the RGB
standards we care about doesn't seem so bad :)
This improves some conversion trait implementations by taking advantage
of the existing `{into/from}_{linear/format}` API as recommended by
@Ogeon.
@mitchmindtree
Copy link
Contributor Author

Sorry about the delay @Ogeon, I think I've now addressed the issues you mentioned!

@Ogeon
Copy link
Owner

Ogeon commented Nov 24, 2019

No worries, this looks great! Thanks!

@Ogeon
Copy link
Owner

Ogeon commented Nov 24, 2019

bors r+

bors bot added a commit that referenced this pull request Nov 24, 2019
137: Add some missing `From` impls between `Srgb` and `LinSrgb` types r=Ogeon a=mitchmindtree

I ran into an issue downstream when attempting to write a generic
function that would accept any type of colour that may be converted to
`Srgb<T>`. I noticed implementations seemed to exist for all of the
colour types I cared about apart from `LinSrgb<T>`, so I thought I'd
implement the missing conversions and open a PR!

Let me know if there's a reason that these have been omitted that I'm
overlooking, or if you have a better way in mind for adding these
conversions!  Unfortunately, conversions between entirely generic
`Rgb<S, T>` and `Rgb<St, U>` are impossible due to the existing blanket
`impl From<T> for T`, but adding individual implementations for the RGB
standards we care about doesn't seem so bad :)

Co-authored-by: mitchmindtree <mitchell.nordine@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 24, 2019

Build succeeded

@bors bors bot merged commit e538ab1 into Ogeon:master Nov 24, 2019
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.

None yet

2 participants