Add OK HSV methods to Color API #76419
Conversation
|
Thanks. I think this is all gdscript bindings so there's nothing to show, but maybe we can find some test color values and a photo. It looks good but waiting for the github actions to pass to be able to test. |
|
You have some errors in the documentation found by the github actions. Please read https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html |
Is there a way to replicate the failing test locally? I don't really understand the error message here. Also while making an example project I noticed there is no |
|
Yeah, it's a good idea to add from_hsl. Smaller prs are faster to review. |
|
The reason it's failing is that you need to put the methods in alphabetical order, I suggest building the code locally and using |
|
Alright I fixed the documentation now I think. As for from_hsl, should I reuse |
|
I would be explicit and use set_hsl_h . The documention seems to be named with the wrong casing. |
I prefer that as well. |
|
The 4.0 existing apis aren’t allowed to change without a deprecation notice as far as I know |
|
I see, then I'll just leave HSV alone when I add HSL, and let someone else figure this out in the future. |
|
I find myself needing a way to convert a color from RGB to OK HSV as well, so I'll try to add that too. |
|
I suppose I'll add ToOkHsv and ToOkHsl methods for C# next. |
|
I don't know whether any of the C# stuff works, because dotnet is not cooperating on my computer. |
|
Alright after much fiddling I have C# working on my end, and thankfully the API seems to behave as it should. |
|
Looks good, but can you merge the commits? "Squash"? |
|
Done, I squashed the changes into one commit with a consise summary of changes. |
|
The color picked zip you loaded doesn't match your photo. It can't load colors. So I can't test that. I think the pr is great, but I wanted to try your demo. |
|
I've updated the demo now, it's starting to look a bit more polished. |
|
Does anything else need to be done before it can be merged? |
|
Something that I did for HSL but forgot to do for OK HSV and OK HSL is return 0 for hue when saturation is 0, so I'll add that. |
|
I'm not an artist, so I'm more than a bit confused. You said HSV and HSL account for human perception, what's the reason there's two of them? |
|
They are two different ways to represent the same idea I believe |
|
"The HSL representation models the way different paints mix together to create color in the real world" |
|
(now we've both learned about these things! I didn't know before) |
|
Oh. In that case bonjorno is kinda right that having one and not the other is... weird. |
|
One could argue that one is more suited to computer graphics than the other, but that would be HSV not HSL by the way I'm understanding the different ideas, though I guess HSL could be more suited for working with UI |
It's the OK part that accounts for human perception; the difference between HSV/HSL and OK HSV/HSL. The difference between (OK) HSV and (OK) HSL is light vs paint, but you already knew that. |
|
Here's how I see it:
|
This commit adds functionality for converting to and from OK HSL and OK HSV color spaces. For GDScript it adds a function to convert from OK HSV, and members for converting to OK HSV and OK HSL. For C# it adds a function to convert from OK HSV, and functions for converting to OK HSV and OK HSL. The conversion isn't always as efficient as it could be, but it's good enough.
|
Conversion to OK HSV/L now behaves correctly with grayscale values.
|
What about a color utility class with all the conversion? that way it can be outside of core and still provided in the engine. |
| ok_color new_ok_color; | ||
| ok_color::RGB rgb = new_ok_color.okhsv_to_srgb(hsv); |
There was a problem hiding this comment.
@fire Why is ok_color a class? 🤔
godot/thirdparty/misc/ok_color.h
Line 25 in 4e1d5be
You've added it like this in #59786 even though in the original source (from this post) it is as namespace.
Suggestion: change ok_color back to be a namespace so the code in here (and similar code in other places) could be changed like:
| ok_color new_ok_color; | |
| ok_color::RGB rgb = new_ok_color.okhsv_to_srgb(hsv); | |
| ok_color::RGB rgb = ok_color::okhsv_to_srgb(hsv); |
Needing to create an instance like new_ok_color just to be able to call a method not operating on that instance makes no sense to me.
Alternatively (if for some reason class is preferred over namespace) such methods could be changed to be static.
There was a problem hiding this comment.
OK HSL conversion was already part of the engine before I came along.
I don't know why it's a class instead of a namespace, I just followed the conventions in the existing code.
I can change it if you'd like, but I'd have to change it in more places too then.
Edit: Oh you weren't talking to me. Perhaps this should be a separate cleanup then.
There was a problem hiding this comment.
@bonjorno7 Indeed might be out of scope of this PR. Just catched my eye, hence the comment. 🙃
OK HSL conversion is already part of core, so I don't think it makes much sense to give OK HSV its own class. |
|
I hope my simple addition to a previously almost complete API doesn't spark a heated debate, ending in its untimely demise. |
|
It's not about demise, I think there's pretty much consensus on the fact that we want this, but not exactly where. Core is a very delicate set of classes. When i say core i don't mean the engine as a whole, i specifically mean the |
I'd be fine having OK HSV and OK HSL in its own converter utility class or global scope functions, but then we should remove OK HSL from the core Color class too. |
I feel the need to clarify my above as I don't want to sound like I am being needlessly dismissive. I'm sorry if I sounded that way. We have to be super careful about what we merge into the core section of Godot. The core is intended to be the lean mean center of the engine. A place to collect the datatypes and functionality that are used throughout the engine and by many of our users. It is not the place to aggregate helpful utility functions that will save users from writing code themselves. Although sometimes if a function is used often enough, it will be included in core for just that reason (see the math namespace for examples of that). It is necessary for the project to be super strict about what we add to the core as once we add something we rarely remove it. So over time the core has a tendency to grow, become more complex, and become more sluggish. Each individual change may be totally acceptable and helpful. But over time we risk suffering from death by a thousand cuts. Accordingly, over time we unfortunately have to become even more strict with what we allow into the core. This PR to me seems like it is exposing another color space because it would be nice to complete the set of HSV, HSL, OKHSL, OKHSV. To me, that isn't enough justification for adding something to the core. OKHSL was added because it was needed for the color picker. OKHSL is a much nicer color space for color pickers. That being said, in my opinion, it was a mistake to add the OKHSL functions into the core Color class. We needed the color space for the color picker, we didn't need to add functions to core in order to do that. But we have added it, so we have to maintain it. That being said, just because we mistakenly added something into core earlier doesn't mean we should continue to do so, the floodgates have not opened up. One of the things we as maintainers fear is that if we merge one new feature, a flood of similar but slightly different features will follow. That can quickly take a class that was lean and make it bloated. In my opinion, we are at risk of that happening here. When faced with new features that would be nice to have, we should instead ask where they would make the most sense for users outside of core and evaluate those options before adding things to core. For example, your stated need is for making a color picker app. Accordingly, these conversion functions could likely be added as part of the ColorPickerAPI (ideally OKHSL would have been exposed there initially as well). Or alternatively, we could expose a ColorUtils class as QbieShay suggested above. That would allow us to be more free with what we add as the ColorUtils class will only be included in the files that need it and not in thousands of files throughout the engine. Additionally, moving to another class also makes it easier for us to add new color spaces according to user demand (like CieLab and others). So what I meant to do above is open a discussion about the need for this feature and how we may be able to meet that need in another way that also respects the needs of the Godot Project. But I think I came off us dismissive instead, so my apologies for that. |
This is probably the golden ticket here (both for this and the HSL that you reverted)
Unfortunately yes you did |
|
I'll get started on a utility class then. |
|
Hey @bonjorno7 , before starting to code it's better to add a feature proposal so we can discuss it there. If you can link it here, then everyone that participated here will have a link to it and we can discuss exactly what we want in there. My uninformed take is that we could make methods for the current conversion methods and perhaps also move the color constants there. I'd make a static class called However, take my suggestion with a grain of salt, I'm not as informed as clay or other contributors on what's the best implementation and the best way to split the class so it's best to get their input first and consider it above mine ^^ |
|
@QbieShay what category should this discussion go in? |
|
Welp here it is anyway: #76976 |
|
Thank you @bonjorno7 for taking this feedback graciously. I will write up my ideas there and hopefully we will obtain a satisfactory solution for everyone. |
|
Superceded by #76976 |
Fixes: godotengine/godot-proposals#6753
I noticed OK HSV was missing from from the API, so I added it.
I've also added some missing API for OK HSL to match.
I might try to implement an OK HSV color picker as well, but that will be its own PR.
Here's a demo project: Color Demo.zip
