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

LinSrgba, Srgba and Hsla color types #1870

Closed
wants to merge 5 commits into from

Conversation

lassade
Copy link
Contributor

@lassade lassade commented Apr 10, 2021

The bevy 0.5 made colors operations undefined, this directly affects our ability to animate any color property.

There is a ongoing discussion here about this.

So this PR does the following:

  1. Adds LinSrgba, Srgba, Hsla color types.

Using colors as types allows for predictable behaviour, easer readability, compile time optimizations:

// Lerp happens in HSL space but result is stored as sRGB
// a is converted to HSL, b is a HSL, 2 conversions are made (just count the into calls)
let c: sRGB = HSL::lerp(a.into(), b, 0.5).into(); 

When conversions from SRGB to HSL and back to SRGB are done often, cache the HSL;

  1. type Color = LinSrgba

By default the renderer should operate in the linear color space so we define the Color type to be in this same space, to avoid any unnecessary conversions;

  1. Fixes color constants for both LinSrgba and Srgba,

I just used a python script to convert the color constants between each space;

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Apr 10, 2021
@mockersf
Copy link
Member

Before this, I think we should agree on wether to roll our own colors, use palette, use kolor or rgb

@lassade
Copy link
Contributor Author

lassade commented Apr 12, 2021

I made this PR, because I really want to animate colors and to make easier to test out this approach (which, I think is the best), so nothing is set stone.

I think roiling with our own type is simpler because we can implement any conversions or traits we want;

If you notice I aligned the colors to 16 bytes so they can be ready to use with simd instructions (SSE2), thats because I assumed everything have a alpha, and that is not something I saw in any other crate;

Plus I'm thinking in using the kolor crate (once it's ready), because it allows to use our own storage types while having theirs conversions methods;

@mockersf
Copy link
Member

mockersf commented Apr 12, 2021

Sound good to me, I would just like to avoid a breaking change in colors in 0.6 and in 0.7. But even that is not a blocker, we offer no guarantee on that... Breaking twice before 0.6 should be ok.

@kabergstrom when do you feel kolor could be ready?

@kabergstrom
Copy link

I think it's getting there, I added HSV/HSI/HSL, ICtCp and CIELuv this weekend. Did some more work on the docs. What would you like to see in the library?
I need to write the readme, set up CI and add better reference unit tests.

@lassade
Copy link
Contributor Author

lassade commented Apr 14, 2021

I'm blocked by some animator system stuff before I can do some more testing about colors. Do way I think adding using kolor won't break anything, will just add more color types;

return self;
}

if self <= 0.0031308 {
Copy link
Contributor

Choose a reason for hiding this comment

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

For floating-point equality checks, should we be using some sort of epsilon or a different crate like float-cmp?

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
@alice-i-cecile
Copy link
Member

#4648 contains further design direction on color. It's probably going to be easier to start from scratch, but I'll leave that decisison to you.

@nicopap
Copy link
Contributor

nicopap commented Feb 22, 2024

Superseded by #12013

@nicopap nicopap closed this Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants