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

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

Closed
wants to merge 13 commits into from
1 change: 0 additions & 1 deletion crates/bevy_render/Cargo.toml
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
139 changes: 62 additions & 77 deletions crates/bevy_render/src/color/mod.rs
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

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,45 @@ 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)),
// Convert hex to rgb[a] u8
// For rgb: `fff` -> [u8; 6] -> [r, g, b, ..]
// For rgba: `E2E2E2FF` -> [u8; 8] -> [r, g, b, a, ..]
wyhaya marked this conversation as resolved.
Show resolved Hide resolved
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 hex to u8
// e.g `f` -> 102 -> 15
let val = match hex_ascii_byte(bytes[i]) {
Copy link
Member

Choose a reason for hiding this comment

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

This could also use ? syntax now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? is not allowed in a const fn. note: see issue #74935 rust-lang/rust#74935 for more information

Copy link

Choose a reason for hiding this comment

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

Similarly, for loops currently aren't allowed in const fns, hence the while loops. (see rust-lang/rust#87575 for info)

It would be really cool if the const_generic_exprs feature became mature enough to write

const fn decode_hex<const N: usize>(mut bytes: [u8; N]) -> Result<[u8; N / 2], HexColorError>

(Not that N would be so large in this context so as to be problematic, but I wonder if such a signature would necessarily require a new array to be constructed. I guess it might not be such a positive then.)

Ok(val) => val,
Err(byte) => return Err(HexColorError::Char(byte as char)),
};
bytes[i] = val;
i += 1;
}
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] << 4 | bytes[i * 2 + 1];
Copy link

@vacuus vacuus Dec 13, 2022

Choose a reason for hiding this comment

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

Just write bytes[i * 2] * 16. It's trivial for a compiler to optimize. I don't know about bytes[i * 2] * 16 + bytes[i * 2 + 1], though. Or don't, whatever (a friend seems to think it's more natural as written).

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)),
const fn hex_ascii_byte(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),
_ => Err(b),
}
}

Expand All @@ -1373,29 +1370,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").unwrap(), Color::WHITE);
Copy link

Choose a reason for hiding this comment

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

Don't .unwrap() and use Ok(...) instead, both for the symmetry with the Err(...) cases and because assert_eq! is like .unwrap() already.

assert_eq!(Color::hex("FFFF").unwrap(), Color::WHITE);
assert_eq!(Color::hex("FFFFFF").unwrap(), Color::WHITE);
assert_eq!(Color::hex("FFFFFFFF").unwrap(), Color::WHITE);
assert_eq!(Color::hex("000").unwrap(), Color::BLACK);
assert_eq!(Color::hex("000F").unwrap(), Color::BLACK);
assert_eq!(Color::hex("000000").unwrap(), Color::BLACK);
assert_eq!(Color::hex("000000FF").unwrap(), Color::BLACK);
assert_eq!(Color::hex("03a9f4").unwrap(), 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