Skip to content

Conversation

@afonsolage
Copy link
Contributor

Objective

  • Simplify Srgba hex string parsing using std hex parsing functions and removing loops in favor of bitwise ops.

This is a follow-up of the bevy_color upstream PR review: #12013 (comment)

Solution

  • Reworked Srgba::hex to use from_str_radix and some bitwise ops;

@bushrat011899 bushrat011899 added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Feb 24, 2024
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Nice change, might as well tidy these kinds of functions while we're making a new crate!

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Would be nice to add some test cases for RGBA and RRGGBBAA variants where the colors have different values.

assert_eq!(Srgba::hex("11223344"), Ok(Srgba::rgba_u8(17, 34, 51, 68)));
assert_eq!(Srgba::hex("1234"), Ok(Srgba::rgba_u8(17, 34, 51, 68)));

@afonsolage
Copy link
Contributor Author

Would be nice to add some test cases for RGBA and RRGGBBAA variants where the colors have different values.

assert_eq!(Srgba::hex("11223344"), Ok(Srgba::rgba_u8(17, 34, 51, 68)));
assert_eq!(Srgba::hex("1234"), Ok(Srgba::rgba_u8(17, 34, 51, 68)));

Added more tests to make sure RGBA is right.

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Less code, more tests, and significantly faster than the old implementation.

@rparrett rparrett added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 25, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 25, 2024
Merged via the queue into bevyengine:main with commit bc2ddce Feb 25, 2024
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- Simplify `Srgba` hex string parsing using std hex parsing functions
and removing loops in favor of bitwise ops.

This is a follow-up of the `bevy_color` upstream PR review:
bevyengine#12013 (comment)

## Solution

- Reworked `Srgba::hex` to use  `from_str_radix` and some bitwise ops;

---------

Co-authored-by: Rob Parrett <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- Simplify `Srgba` hex string parsing using std hex parsing functions
and removing loops in favor of bitwise ops.

This is a follow-up of the `bevy_color` upstream PR review:
bevyengine#12013 (comment)

## Solution

- Reworked `Srgba::hex` to use  `from_str_radix` and some bitwise ops;

---------

Co-authored-by: Rob Parrett <[email protected]>
@BD103 BD103 added the A-Color Color spaces and color math label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Color Color spaces and color math A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants