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

Color palettes should be defined in terms of Color, not specific color spaces #12188

Closed
alice-i-cecile opened this issue Feb 28, 2024 · 9 comments
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR
Milestone

Comments

@alice-i-cecile
Copy link
Member

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

In the course of #12163, I've been working to use the polymorphic Color enum as the primary way for end users to supply colors.

Because the palettes are stored in Srgba, this results in a ton of .into() or Color::from calls.

What solution would you like?

Just define these palettes in terms of Color.

Additional context

Migration tracking issue can be found at #12056.

I don't particularly care whether this or #12163 is done first.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 28, 2024
@viridia
Copy link
Contributor

viridia commented Feb 28, 2024

I was really hoping to minimize the use of Color.

@alice-i-cecile
Copy link
Member Author

Yeah, I'm not thrilled by it either. But if all of our APIs where we might use a color palette take a Color, it seems silly to store them in any other format. As to whether we should use that particular form of API, I suppose that's the controversial question in #12163.

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Feb 28, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Feb 28, 2024
@viridia
Copy link
Contributor

viridia commented Feb 28, 2024

Well, in my original plan the Gizmo APIs (for example) would take an Into<LinearRgba>, not an Into<Color>.

The problem with using Color everywhere is that it negates some of the advantages of making bevy_color in the first place: you can no longer do color operations like mix, darken etc., you have runtime checks and so on.

@pablo-lua
Copy link
Contributor

So, we would use some type of color as the library default? For example, in gizmos, the default would be LinearRgba?

@viridia
Copy link
Contributor

viridia commented Feb 28, 2024

Also, UI themes are certainly going to be defined in Srgba. For example, I have code that looks like this:

if hovering {
    BUTTON_BG.lighten(0.1)
} else if pressed {
    BUTTON_BG.lighten(0.2)
} else {
    BUTTON_BG
}

I can't do this if BUTTON_BG is Color (at least, not cheaply). So now we have two different kinds of palette - one for UIs, one for non-UIs.

I can't speak for everyone else, but for me having a lot of .into() calls isn't necessarily a bad thing, if it tells me "Hey, there's a conversion cost here you should be aware of."

@alice-i-cecile
Copy link
Member Author

Well, in my original plan the Gizmo APIs (for example) would take an Into<LinearRgba>, not an Into<Color>.

The problem with using Color everywhere is that it negates some of the advantages of making bevy_color in the first place: you can no longer do color operations like mix, darken etc., you have runtime checks and so on.

I'd love to be able to do this, but it doesn't work for bundle types. We're constructing the fields directly :(

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Feb 28, 2024
@viridia
Copy link
Contributor

viridia commented Feb 28, 2024

Also, I want to point out that the decision of whether to use Color or one of the other types also affects when the conversion is done. Using Color means that the conversion is deferred until the color is dereferenced - in other words, lazily. Using Srgba or LinearRgba means that the conversion is done eagerly.

This can affect performance, although the specific impact will depend on how often the color field is evaluated. If the conversion is eager, it happens when the user calls the API. If the conversion is lazy, it happens during extraction or whatever internal system Bevy uses for processing the color. The question is, which of those operations happens more often? The answer of course is that it depends on which part of Bevy we're talking about.

@viridia
Copy link
Contributor

viridia commented Feb 28, 2024

One final (hopefully) comment: I think it's important to accept that there is no perfect solution here. There's an inherent tradeoff between these choices, and we have to pick one or the other and accept the consequences. Using Color as the basis for APIs does make the code more succinct, at least for the examples. But are we optimizing for examples, or for real-world use cases?

Real games aren't going to use the standard palettes much, if at all. The main user will be debugging displays and gizmos, but even these may opt to define their own color constants.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Feb 29, 2024
@viridia
Copy link
Contributor

viridia commented Mar 6, 2024

Well, I said I was going to shut up about this, but I have one more thought: There's a reason why these colors are defined in sRGB and not in other color spaces, because this is what sRGB is for. The CSS colors, for example, are defined in terms of sRGB values, as are the tailwind colors. If the user needs them in another space, we've made it easy enough to convert.

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 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-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

3 participants