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
Implement all non-trivial color traits on Color
type
#13214
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
Comments
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
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. |
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. |
Agreed. |
github-merge-queue bot
pushed a commit
that referenced
this issue
May 14, 2024
# 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
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
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:
they must write
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.
The text was updated successfully, but these errors were encountered: