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

bevy_color: adding 'palettes' module. #12067

Merged
merged 3 commits into from
Feb 24, 2024
Merged

Conversation

viridia
Copy link
Contributor

@viridia viridia commented Feb 23, 2024

Two palettes are added:

  • Basic: A basic palette with 16 colors.
  • CSS: A palette with the colors from the "extended CSS"/X11/HTML4.0 color names.

Two palettes are added:
- Basic: A basic palette with 16 colors.
- X11: A palette with the colors from the X11/HTML4.0 color names.
@viridia viridia changed the title Adding 'palettes' module. bevy_color: adding 'palettes' module. Feb 23, 2024
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Feb 23, 2024
@@ -0,0 +1,38 @@
//! Named colors from the CSS1 specification, also known as
//! [basic colors](https://en.wikipedia.org/wiki/Web_colors#Basic_colors).
//! This is the same set of colors as VGA.
Copy link
Member

Choose a reason for hiding this comment

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

VGA should be spelled out, and a link would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a link. I don't want to spell it out because the name "video graphics array" is actually confusing.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I like namespacing color palettes like this :) And it'll ease the migration. However, I really think we should avoid committing Python code in order to avoid complicating the developer experience.

We should either remove it completely, or swap it for a simple Rust equivalent.

@IceSentry
Copy link
Contributor

IceSentry commented Feb 23, 2024

I'm not personally familiar with the x11 naming and it seems to me like more people would understand if it was called css instead. As in, name the palette css so people can do css::ALICE_BLUE. I think it would be easier to understand where it comes from.

It's very possible it's just a me thing though.

@@ -0,0 +1,301 @@
//! [Extended colors from the CSS4 specification](https://en.wikipedia.org/wiki/Web_colors#Extended_colors),
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should link to the spec
https://www.w3.org/TR/css-color-4/#named-colors

@viridia
Copy link
Contributor Author

viridia commented Feb 23, 2024

Removed python code and changed x11 -> css.

@rparrett
Copy link
Contributor

rparrett commented Feb 23, 2024

Out of curiosity, how much precision is actually needed here? This certainly seems like enough at a glance, just curious.

pub const BURLYWOOD: Srgba = Srgba::new(0.871, 0.722, 0.529, 1.0);

Any reason not to just use the original integer values here? Or consistently format with 1 decimal.

/// <div style="background-color:rgb(87.1%, 72.2%, 52.900000000000006%); width: 10px; padding: 10px; border: 1px solid;"></div>

@viridia
Copy link
Contributor Author

viridia commented Feb 23, 2024

@rparrett My python script rounded everything to the nearest thousandth to make the code prettier. I figured one part in a thousand would be imperceptible to the human eye.

I don't really have an opinion otherwise.

@rparrett
Copy link
Contributor

Seems like thousandths are good enough, hundredths would not be.

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.

I like this. The concept of a colour palette is distinct from the colour types themselves, so I like putting them in separate modules. Also opens up the possibility of adding more palettes with colliding names (e.g. blue in Tailwind might not be the same blue in basic, etc.)

@alice-i-cecile alice-i-cecile 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 24, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 24, 2024
Merged via the queue into bevyengine:main with commit d31de3f Feb 24, 2024
24 checks passed
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
Two palettes are added:
- Basic: A basic palette with 16 colors.
- CSS: A palette with the colors from the "extended CSS"/X11/HTML4.0
color names.
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
Two palettes are added:
- Basic: A basic palette with 16 colors.
- CSS: A palette with the colors from the "extended CSS"/X11/HTML4.0
color names.
@rparrett
Copy link
Contributor

rparrett commented Mar 1, 2024

Just an interesting data point regarding float precision:

Old crimson:

pub const CRIMSON: LegacyColor = LegacyColor::rgb(0.86, 0.08, 0.24);

New crimson:

pub const CRIMSON: Srgba = Srgba::new(0.863, 0.078, 0.235, 1.0);

The increased precision resulted in CI picking up a color change in examples using CRIMSON. I am assuming the new one is actually matching the CSS4 value and the old one is not.

Seeing what I assume is the same situation in the virtual_time example which GOLD to tint a sprite. The increased precision caused a small difference there.

Neat.

@alice-i-cecile
Copy link
Member

Yeah, the new colors actually match CSS4 values, to three decimals I believe. They were machine-generated via a Python script originally.

@rparrett
Copy link
Contributor

rparrett commented Mar 1, 2024

Yeah, I had previously commented on the precision. Just commenting here to confirm that yes, the third decimal matters.

@viridia
Copy link
Contributor Author

viridia commented Mar 1, 2024

I saved a copy of the script, but it's not checked in anywhere. The actual colors were cut and pasted from the CSS standard website, and then converted into Rust source code.

@rparrett
Copy link
Contributor

rparrett commented Mar 1, 2024

Just to be totally clear, I am saying that things are better now with the increased precision.

@viridia
Copy link
Contributor Author

viridia commented Mar 6, 2024

Here's a Gist containing the Python script I used to generate the color palettes: https://gist.github.com/viridia/0143965eaa7805145a115b85b04b79fd

@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-Feature A new feature, making something new possible 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.

6 participants