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 5 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
132 changes: 55 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,38 @@ 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)),
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() {
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 {
bytes[i] = bytes[i * 2] << 4 | bytes[i * 2 + 1];
Copy link

@vacuus vacuus 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.

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 +1363,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