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 shader built-in functions to convert colors between RGB, HSV, HSL and OKHSL #6454

Open
hsandt opened this issue Mar 7, 2023 · 5 comments

Comments

@hsandt
Copy link

hsandt commented Mar 7, 2023

Describe the project you are working on

A platformer where the character becomes brighter or changes color when hurt or invincible

Describe the problem or limitation you are having in your project

This is an augmented reboot of godotengine/godot#18920

I had to write my own conversion functions in my shader to convert RGB to HSL then increase lightness, although Godot can already do this internally.

Then I discovered OK HSL (used by Godot itself with the Color Wheel and in GDScript / C# - see from_ok_hsl method), so it looks like I'll now have to rewrite my conversion methods to be able to use this new color space... While it has already been implemented in Godot C++ and exposed at least in one way with the OKHSL -> RGB conversion in GDScript and C#.

Other examples of people needing this and writing their own functions:

Describe the feature / enhancement and how it helps to overcome the problem or limitation

It would be faster to expose all these conversions into the shader language (and while we're at it, the HSV/HSL/OKHSL -> RGB conversion methods to GDScript and C# too, although this is not my main purpose).

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Main request: shader language could add the following built-ins:

  • rgb2hsv(vec3): vec3
  • hsv2rgb(vec3): vec3
  • rgb2hsl(vec3): vec3
  • hsl2rgb(vec3): vec3
  • rgb2okhsl(vec3): vec3
  • okhsl2rgb(vec3): vec3

Or, if you prefer, name in the format old_to_new.

Sub-request: since from_ methods already exist in GDScript and C#, we could just add the rgb2... conversion methods to those languages as well.

It looks like we can define a custom function with the same name as a built-in, it will hide it, so technically it shouldn't break existing shaders already defining them; although we should notify users that they don't need their custom implementation anymore (if they read the changelog they'll see it anyway).

If this enhancement will not be used often, can it be worked around with a few lines of script?

I'm already doing this for HSL. I could do this for OK HSL too, but I fear I'll have to get through the formulas myself this time (as it's much harder to find existing functions on this new space).

Is there a reason why this should be core and not an add-on in the asset library?

If somebody coded that in some library, honestly I'll be glad to just download it and #include it in my own shader.

The advantage of Godot developers doing it is that they can plug the C++ implementation in color.cpp and ok_color.h (assuming they can use C++ code in their shader language).

If they need to copy their C++ implementation to pure shader language to make it work, then I suppose it would be equal work to write it for Godot itself or in an asset library.

@Calinou
Copy link
Member

Calinou commented Mar 8, 2023

The issue with adding built-in functions to text shaders is that they increase the compilation time of every shader, even if the function isn't used.

This is not an issue with VisualShader nodes, as their code is only present in the compiled shader if the node is present in the graph.

Edit: godotengine/godot#77239 will resolve this once it's merged.

@paddy-exe
Copy link

@hsandt I created this addon now for this purpose: https://github.com/paddy-exe/ShaderFunction-Extras
I already added the rgb_to_hsv and hsv_to_rgb functions. I hope to get to the other variations eventually as well.

@hsandt
Copy link
Author

hsandt commented Mar 14, 2023

@Calinou: OK, then we'll go with extensions unless you estimate the most common ones to not add much compile time
@paddy-exe: Great, I'll try them. In the case of RGB to HSL, I have tried 2-step conversions via HSV (RGB to HSV and HSV to HSL) but observed issues in edge cases (black and white). This may not happen with all implementations, so I'm gonna try and see how it works. Eventually we'll probably have all conversions anyway, I can even send a PR for HSL conversions (it's just that I working with a mix of snippets found on the Internet that I didn't formally verify except for the most trivial conversions, so I'm a bit wary of sharing them verbatim).

@lowagner
Copy link

i would love to have built-ins for RGB/HSV/HSL. could we work around the "increase the compilation time of every shader" issue by effectively inlining them only in the shaders that use them?

@Calinou
Copy link
Member

Calinou commented Jul 18, 2023

could we work around the "increase the compilation time of every shader" issue by effectively inlining them only in the shaders that use them?

This will be supported once godotengine/godot#77239 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants