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.

Sign up for GitHub

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

Jump to bottom

Add Color::as_lcha #7757

Closed
LiamGallagher737 opened this issue Feb 20, 2023 · 2 comments
Closed

Add Color::as_lcha #7757

LiamGallagher737 opened this issue Feb 20, 2023 · 2 comments
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Milestone
0.10

Comments

@LiamGallagher737
Copy link
Member

LiamGallagher737 commented Feb 20, 2023 •
edited
Loading

Problem

Other color spaces have a as_xxx with returns a Color and a as_xxx_f32 that returns a [f32; 4], the lch color space only has a as_lch_f32 that returns a [f32; 4].

Solution

Add a new method, as_lcha, for bevy_render::Color for converting any Color type into a Color::Lcha variant.

Note

Alongside this it would also make sense to rename as_lch_f32 to as_lcha_f32 as it also returns the alpha, not really big enough for its own issue.
@LiamGallagher737 LiamGallagher737 added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Feb 20, 2023
@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Feb 20, 2023
@iwek7
Copy link
Contributor

iwek7 commented Feb 20, 2023

I can try to do it if it is still free

@LiamGallagher737
Copy link
Member Author

Still free

iwek7 added a commit to iwek7/bevy that referenced this issue Feb 20, 2023
@iwek7
Add 'Color::as_lcha' function (bevyengine#7757)
78e28f6
iwek7 added a commit to iwek7/bevy that referenced this issue Feb 20, 2023
@iwek7
Add 'Color::as_lcha' function (bevyengine#7757)
0cb2bbb
@iwek7 iwek7 mentioned this issue Feb 20, 2023
Closed
iwek7 added a commit to iwek7/bevy that referenced this issue Feb 20, 2023
@iwek7
Add 'Color::as_lcha' function (bevyengine#7757)
ff325a0
iwek7 added a commit to iwek7/bevy that referenced this issue Feb 20, 2023
@iwek7
Add 'Color::as_lcha' function (bevyengine#7757)
22f5327
iwek7 added a commit to iwek7/bevy that referenced this issue Feb 20, 2023
@iwek7
Add 'Color::as_lcha' function (bevyengine#7757)
f8bb4ca
@bors bors bot closed this as completed in 04b4213 Mar 5, 2023
@james7132 james7132 added this to the 0.10 milestone Mar 5, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this issue Mar 19, 2023
@iwek7 @Shfty
Add 'Color::as_lcha' function (bevyengine#7757) (bevyengine#7766)
dacc36b 
# Objective
Fixes bevyengine#7757

New function `Color::as_lcha` was added and `Color::as_lch_f32` changed name to `Color::as_lcha_f32`.

----
As a side note I did it as in every other Color function, that is I created very simillar code in `as_lcha` as was in `as_lcha_f32`. However it is totally possible to avoid this code duplication in LCHA and other color variants by doing something like :
```
 pub fn as_lcha(self: &Color) -> Color {
    let (lightness, chroma, hue, alpha) = self.as_lcha_f32();
    return Color::Lcha { lightness, chroma, hue, alpha };
}
```
This is maybe slightly less efficient but it avoids copy-pasting this huge match expression which is error prone. Anyways since it is my first commit here I wanted to be consistent with the rest of code but can refactor all variants in separate PR if somebody thinks it is good idea.
Shfty pushed a commit to shfty-rust/bevy that referenced this issue Mar 19, 2023
@iwek7 @Shfty
Add 'Color::as_lcha' function (bevyengine#7757) (bevyengine#7766)
586812e 
# Objective
Fixes bevyengine#7757

New function `Color::as_lcha` was added and `Color::as_lch_f32` changed name to `Color::as_lcha_f32`.

----
As a side note I did it as in every other Color function, that is I created very simillar code in `as_lcha` as was in `as_lcha_f32`. However it is totally possible to avoid this code duplication in LCHA and other color variants by doing something like :
```
 pub fn as_lcha(self: &Color) -> Color {
    let (lightness, chroma, hue, alpha) = self.as_lcha_f32();
    return Color::Lcha { lightness, chroma, hue, alpha };
}
```
This is maybe slightly less efficient but it avoids copy-pasting this huge match expression which is error prone. Anyways since it is my first commit here I wanted to be consistent with the rest of code but can refactor all variants in separate PR if somebody thinks it is good idea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Milestone
0.10
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - Add 'Color::as_lcha' function (#7757) iwek7/bevy
4 participants
@james7132 @alice-i-cecile @iwek7 @LiamGallagher737