[Merged by Bors] - Add 'Color::as_lcha' function (#7757)#7766
Closed
iwek7 wants to merge 1 commit intobevyengine:mainfrom
Closed
[Merged by Bors] - Add 'Color::as_lcha' function (#7757)#7766iwek7 wants to merge 1 commit intobevyengine:mainfrom
iwek7 wants to merge 1 commit intobevyengine:mainfrom
Conversation
Contributor
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
iwek7
force-pushed
the
as_lcha
branch
2 times, most recently
from
ff325a0 to
22f5327
Compare
alice-i-cecile
added
C-Usability
alice-i-cecile
approved these changes
Feb 21, 2023
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
IIRC we asked about refactoring this previously, and the idea was rejected by Cart as it's in a very hot loop.
In this unique case, I'd prefer a macro to a function.
james7132
approved these changes
Mar 5, 2023
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Mar 5, 2023
# Objective Fixes #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.
Contributor
bors
bot
changed the title
ProfLander
pushed a commit
to Lander-Rust/bevy
that referenced
this pull request
Mar 19, 2023
# 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.
ProfLander
pushed a commit
to Lander-Rust/bevy
that referenced
this pull request
Mar 19, 2023
# 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #7757
New function
Color::as_lchawas added andColor::as_lch_f32changed name toColor::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_lchaas was inas_lcha_f32. However it is totally possible to avoid this code duplication in LCHA and other color variants by doing something like :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.