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

Add support for getting and setting the Chroma value of an LCHA color #10374

Closed
wants to merge 2 commits into from

Conversation

Tam
Copy link

@Tam Tam commented Nov 4, 2023

Objective

Give chroma the attention it deserves

Solution

  • Add a getter and setters for the chroma value of an LCHA color

Changelog

Added

  • Added c(), set_c(), and with_c() to the Color struct

Copy link
Contributor

github-actions bot commented Nov 4, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Nov 5, 2023
@mockersf
Copy link
Member

mockersf commented Nov 5, 2023

Colors methods are getting harder to grok, with more and more methods with a single letter available on all variants...

@inodentry
Copy link
Contributor

To preface, I am a big fan of LCHA colors in Bevy myself, and I use them in my projects. :)

I am not happy about the current state of this for a number of reasons. Without this PR, LCH is kinda neglected and clunky to work with, due to the missing APIs (compared to RGB and HSL). With this PR ... you only add methods for C. Which is weird and confusing. What about the other LCHA components? Why only chroma?

Though ... set_l/set_h/etc already exist, but they are for HSL, not LCH.

Clearly, we have a problem. The L and H letters overlap between HSL and LCH, but mean vastly different things. We can't just add a set_l/set_h/etc. for LCH, because those already exist. We can't/shouldn't make the existing ones check for HSL vs LCH and do the respective thing, because then we would introduce footguns/bugs where users will call them, but unexpectedly end up in the wrong color space.

My proposed solution would be to rename the existing HSL methods, to make it clear that they are for working with HSL, and introduce a full set of methods for LCH using the same naming scheme. One suggestion is to just put the color space in the name:

  • .hsl_h()/.set_hsl_h()/.with_hsl_h()
  • .hsl_s()/.set_hsl_s()/.with_hsl_s()
  • .hsl_l()/.set_hsl_l()/.with_hsl_l()
  • .lch_l()/.set_lch_l()/.with_lch_l()
  • .lch_c()/.set_lch_c()/.with_lch_c()
  • .lch_h()/.set_lch_h()/.with_lch_h()

That way, it is always clear what color space you are working with, and the API is "complete", covering all the possibilities.

I think we shouldn't rename the RGB methods, though. RGB is the most common color space that most Bevy users work with the most. It can be considered the "standard", and HSL/LCH are the "alternatives". The RGB letters are also clear and unambiguous. So I think it is fine to have the short and concise names .set_{r,g,b}, but longer names for .set_hsl_{h,s,l} and .set_lch_{l,c,h}.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Nov 6, 2023
@alice-i-cecile
Copy link
Member

This should be fixed now, with the addition of bevy_color.

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 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.

4 participants