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

Jump to bottom

Implement all non-trivial color traits on Color type #13214

Closed
alice-i-cecile opened this issue May 3, 2024 · 3 comments · Fixed by #13285
Closed

Implement all non-trivial color traits on Color type #13214

alice-i-cecile opened this issue May 3, 2024 · 3 comments · Fixed by #13285
Labels
A-Color Color spaces and color math A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Enhancement A new feature D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through
Milestone
0.14

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

If #12212 is closed as won't fix, the developer experience of working with our assorted fields is much worse than it needs to be.

Rather than simply being able to call:

*color = color.lighten(0.2)

they must write

let hsla = Hsla::from(color);
let light_hsla = hsla.lighten(0.2);
*color = Color::from(light_hsla);

What solution would you like?

If an operation is supported in the supplied color space, call the corresponding method.

If it is not, convert to the most reasonable color space for that operation (usually Oklaba), perform the operation, and then convert back.

What alternative(s) have you considered?

We could choose a single opinionated perceptual color space for working with all parts of UI code.

This is limiting, and particularly painful since it would force const definitions for palettes to be done directly in that color space. This isn't feasible because of limitations on const float arithmetic and const traits.
@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 3, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 3, 2024
@alice-i-cecile alice-i-cecile added the A-Color Color spaces and color math label May 3, 2024
@NthTensor
Copy link
Contributor

If the color is wrapped in the polymorphic trait, do we need to convert back after the operation? I feel like maybe we can be lazy here. This behavior would have to be called out in the docs, obviously.

@alice-i-cecile
Copy link
Member Author

I think converting it back is the least surprising thing to do: if people care about performance they can convert it to the "right" color space on initialization.

@NthTensor
Copy link
Contributor

Agreed.

@bushrat011899 bushrat011899 mentioned this issue May 9, 2024
Merged
@alice-i-cecile alice-i-cecile mentioned this issue May 12, 2024
Closed
github-merge-queue bot pushed a commit that referenced this issue May 14, 2024
@bushrat011899
Implement Color Operations for Color (#13285)
6482a03 
# Objective

- Fixes #13214

## Solution

Delegates to internal type when possible, otherwise uses
`ChosenColorSpace` as an intermediary. This _will_ double convert, but
this is considered an acceptable compromise since use of specific colour
types in performance critical colour operations is already encouraged.

`ChosenColorSpace` is `Oklcha` since it's perceptually uniform while
supporting all required operations, and in my opinion is the "best" for
this task. Using different spaces for different operations will make
documenting this double-conversion behaviour more challenging.

## Testing

Changes straightforward enough to not require testing beyond current CI
in my opinion.

---

## Changelog

- Implemented the following traits for `Color`:
  - `Luminance`
  - `Hue`
  - `Mix`
  - `EuclideanDistance`
  - `ClampColor`
- Added documentation to `Color` explaining the behaviour of these
operations (possible conversion, etc.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Color Color spaces and color math A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Enhancement A new feature D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through
Projects
Alice's Work Planning
Status: Active: bevy_color
Milestone
0.14
Development

Successfully merging a pull request may close this issue.

Implement Color Operations for Color bushrat011899/bevy
2 participants
@alice-i-cecile @NthTensor