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

[Merged by Bors] - Improve Color::hex performance #6940

Closed
wants to merge 13 commits into from
Closed

[Merged by Bors] - Improve Color::hex performance #6940

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4af9988
Optimization Color::hex
wyhaya Dec 13, 2022
584adb0
Fix clippy
wyhaya Dec 13, 2022
6665776
Fix cargo fmt
wyhaya Dec 13, 2022
cb4d1b8
Fix cargo clippy
wyhaya Dec 13, 2022
097d405
Improve readability
wyhaya Dec 13, 2022
f0ee617
Add decode hex comment
wyhaya Dec 13, 2022
6f24313
Update crates/bevy_render/src/color/mod.rs
wyhaya Dec 14, 2022
e2ff7c8
Update decode_hex and test
wyhaya Dec 14, 2022
7c578a6
Fix doc
wyhaya Dec 14, 2022
4fadb22
Add hex color comment
wyhaya Dec 15, 2022
869e852
Update comment
wyhaya Dec 16, 2022
2070d20
Merge branch 'main' into hex_color
wyhaya Jan 5, 2023
87dfb20
Add hex prefix color test
wyhaya Jan 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/bevy_render/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ thread_local = "1.1"
thiserror = "1.0"
futures-lite = "1.4.0"
anyhow = "1.0"
hex = "0.4.2"
hexasphere = "8.0"
parking_lot = "0.12.1"
regex = "1.5"
Expand Down
143 changes: 66 additions & 77 deletions crates/bevy_render/src/color/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,39 +253,29 @@ impl Color {
/// ```
///
pub fn hex<T: AsRef<str>>(hex: T) -> Result<Color, HexColorError> {
let hex = hex.as_ref();

// RGB
if hex.len() == 3 {
let mut data = [0; 6];
for (i, ch) in hex.chars().enumerate() {
data[i * 2] = ch as u8;
data[i * 2 + 1] = ch as u8;
match *hex.as_ref().as_bytes() {
Copy link
Member

Choose a reason for hiding this comment

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

How would this interact with more complex unicode inputs? Does this assume that the input is always ASCII?

Copy link
Contributor Author

@wyhaya wyhaya Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does this assume that the input is always ASCII?

Input must be 0-9 a-z A-Z, anything other than that is an error.
Input must be 0-9 a-f A-F

Copy link

Choose a reason for hiding this comment

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

a-f/A-F, though you did write the code correctly.

// RGB
[r, g, b] => {
let [r, g, b, ..] = decode_hex([r, r, g, g, b, b])?;
Ok(Color::rgb_u8(r, g, b))
}
return decode_rgb(&data);
}

// RGBA
if hex.len() == 4 {
let mut data = [0; 8];
for (i, ch) in hex.chars().enumerate() {
data[i * 2] = ch as u8;
data[i * 2 + 1] = ch as u8;
// RGBA
[r, g, b, a] => {
let [r, g, b, a, ..] = decode_hex([r, r, g, g, b, b, a, a])?;
Ok(Color::rgba_u8(r, g, b, a))
}
return decode_rgba(&data);
}

// RRGGBB
if hex.len() == 6 {
return decode_rgb(hex.as_bytes());
}

// RRGGBBAA
if hex.len() == 8 {
return decode_rgba(hex.as_bytes());
// RRGGBB
[r1, r2, g1, g2, b1, b2] => {
let [r, g, b, ..] = decode_hex([r1, r2, g1, g2, b1, b2])?;
Ok(Color::rgb_u8(r, g, b))
}
// RRGGBBAA
[r1, r2, g1, g2, b1, b2, a1, a2] => {
let [r, g, b, a, ..] = decode_hex([r1, r2, g1, g2, b1, b2, a1, a2])?;
Ok(Color::rgba_u8(r, g, b, a))
}
_ => Err(HexColorError::Length),
}

Err(HexColorError::Length)
}

/// New `Color` from sRGB colorspace.
Expand Down Expand Up @@ -1332,38 +1322,49 @@ impl encase::private::CreateFrom for Color {

impl encase::ShaderSize for Color {}

#[derive(Debug, Error)]
#[derive(Debug, Error, PartialEq, Eq)]
pub enum HexColorError {
#[error("Unexpected length of hex string")]
Length,
#[error("Error parsing hex value")]
Hex(#[from] hex::FromHexError),
#[error("Invalid hex char")]
Char(char),
}

fn decode_rgb(data: &[u8]) -> Result<Color, HexColorError> {
let mut buf = [0; 3];
match hex::decode_to_slice(data, &mut buf) {
Ok(_) => {
let r = buf[0] as f32 / 255.0;
let g = buf[1] as f32 / 255.0;
let b = buf[2] as f32 / 255.0;
Ok(Color::rgb(r, g, b))
}
Err(err) => Err(HexColorError::Hex(err)),
/// Converts hex bytes to an array of rgb\[a\] components
///
/// # Example
/// For RGB: *b"ffffff" -> [255, 255, 255, ..]
/// For RGBA: *b"E2E2E2FF" -> [226, 226, 226, 255, ..]
const fn decode_hex<const N: usize>(mut bytes: [u8; N]) -> Result<[u8; N], HexColorError> {
let mut i = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This is rather opaque on what it's doing and how it works. Please leave a comment on how it works internally, and a doc comment on it's purpose.

while i < bytes.len() {
// Convert single hex character to u8
let val = match hex_value(bytes[i]) {
Ok(val) => val,
Err(byte) => return Err(HexColorError::Char(byte as char)),
};
bytes[i] = val;
i += 1;
}
// Modify the original bytes to give an `N / 2` length result
i = 0;
while i < bytes.len() / 2 {
// Convert u8 to r/g/b/a
Copy link

Choose a reason for hiding this comment

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

I'm just a random dude. Happy to provide feedback if it's asked of me, though. I think the comment could say "pairs of u8" or something like that. Capitalize r, g, b, a (elsewhere too). Maybe "hex digit" instead of "hex character"? None of these are really necessary; more a matter of style.

// e.g `ff` -> [102, 102] -> [15, 15] = 255
bytes[i] = bytes[i * 2] * 16 + bytes[i * 2 + 1];
i += 1;
}
Ok(bytes)
}

fn decode_rgba(data: &[u8]) -> Result<Color, HexColorError> {
let mut buf = [0; 4];
match hex::decode_to_slice(data, &mut buf) {
Ok(_) => {
let r = buf[0] as f32 / 255.0;
let g = buf[1] as f32 / 255.0;
let b = buf[2] as f32 / 255.0;
let a = buf[3] as f32 / 255.0;
Ok(Color::rgba(r, g, b, a))
}
Err(err) => Err(HexColorError::Hex(err)),
/// Parse a single hex character (a-f/A-F/0-9) as a `u8`
const fn hex_value(b: u8) -> Result<u8, u8> {
match b {
Copy link
Member

Choose a reason for hiding this comment

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

This is rather opaque on what it's doing and how it works. Please leave a comment on how it works internally, and a doc comment on it's purpose.

b'0'..=b'9' => Ok(b - b'0'),
b'A'..=b'F' => Ok(b - b'A' + 10),
b'a'..=b'f' => Ok(b - b'a' + 10),
// Wrong hex character
_ => Err(b),
}
}

Expand All @@ -1373,29 +1374,17 @@ mod tests {

#[test]
fn hex_color() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add one test that uses the # prefix just to make sure someone doesn't break this in the future.

assert_eq!(Color::hex("FFF").unwrap(), Color::rgb(1.0, 1.0, 1.0));
assert_eq!(Color::hex("000").unwrap(), Color::rgb(0.0, 0.0, 0.0));
assert!(Color::hex("---").is_err());

assert_eq!(Color::hex("FFFF").unwrap(), Color::rgba(1.0, 1.0, 1.0, 1.0));
assert_eq!(Color::hex("0000").unwrap(), Color::rgba(0.0, 0.0, 0.0, 0.0));
assert!(Color::hex("----").is_err());

assert_eq!(Color::hex("FFFFFF").unwrap(), Color::rgb(1.0, 1.0, 1.0));
assert_eq!(Color::hex("000000").unwrap(), Color::rgb(0.0, 0.0, 0.0));
assert!(Color::hex("------").is_err());

assert_eq!(
Color::hex("FFFFFFFF").unwrap(),
Color::rgba(1.0, 1.0, 1.0, 1.0)
);
assert_eq!(
Color::hex("00000000").unwrap(),
Color::rgba(0.0, 0.0, 0.0, 0.0)
);
assert!(Color::hex("--------").is_err());

assert!(Color::hex("1234567890").is_err());
assert_eq!(Color::hex("FFF"), Ok(Color::WHITE));
assert_eq!(Color::hex("FFFF"), Ok(Color::WHITE));
assert_eq!(Color::hex("FFFFFF"), Ok(Color::WHITE));
assert_eq!(Color::hex("FFFFFFFF"), Ok(Color::WHITE));
assert_eq!(Color::hex("000"), Ok(Color::BLACK));
assert_eq!(Color::hex("000F"), Ok(Color::BLACK));
assert_eq!(Color::hex("000000"), Ok(Color::BLACK));
assert_eq!(Color::hex("000000FF"), Ok(Color::BLACK));
assert_eq!(Color::hex("03a9f4"), Ok(Color::rgb_u8(3, 169, 244)));
assert_eq!(Color::hex("yy"), Err(HexColorError::Length));
assert_eq!(Color::hex("yyy"), Err(HexColorError::Char('y')));
}

#[test]
Expand Down