-
-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
Initial color management support #26869
Conversation
Would this be better suited as an editor setting? This way, it would be preserved when editing a project using the Project Manager. |
Possibly, although I don't think it's very easy to use right now You still have to generate the LUT outside Godot somehow. I think an option for an ICC profile would be more fitting for an editor setting, in that it's an "easy" interface. That requires dependency (or a new ICC profile parser) however. I can make it an editor setting if that's ok. Could you enlighten me a bit on the proper place in the code base to add a new setting? |
#26101 and this PR address different needs. The former is for "in-game" grading (artistic effect), while this one is for global output gamut conversion (sRGB -> device gamut), intended to be used with the editor. |
I am not sure this needs to be in the core. I think it might better suit a plugin. |
This needs to happen in Rasterizer to ensure that all output to screen are processed exactly once. I'm not aware of a way to plug into the Rasterizer. This cannot be a Node because:
Because this feature is for color space conversion, not artistic effect, all the situations above are entirely undesirable. That leaves me no options other than putting it in the core. For more detailed background of this PR please see #26826. |
Minor correctness fix in the shader and rebased on current master. Moved clamp before texel alignment, so in the off case that HDR data is passed to this shader it would not wrap and print bad colors. |
It does not need to be in the rasterizer. You can use a screen shader to do so. |
So where do I apply this screen shader? |
I would say by using a Color rect node, taking the full scree size, and using SCREEN_TEXTURE as input. But indeed that requires that you have only one of them, and that it is put on top of your display. But I mean, it might be OK as built-in, as long as it is a needed feature. Which I am not sure it is in the general case. |
SCREEN_TEXTURE does not take into account stuff using transparent pipeline, though (so anything with alpha won't show) |
@groud in that case I vote for world environment |
Yeah it might make sense there. |
@groud I'm not exactly sure how I can make sure that the ColorRect stays where it should without touching core stuff. AFAIK anyone can just place Nodes where they like. I certainly believe this is needed as I need it, but I do understand that the rest of the community may not care about working with other color managed applications. If the community decides that this is unnecessary I'll respect the verdict. @QbieShay This feature and your contribution work together. Where artist can use your tools to convey their vision, color management ensures that the exact vision is shown on the screen as correctly as possible. |
Yeah, but that's not a problem. It is not up to us to check if users are using a node correctly (we must document things though) It's like, you cannot complain about one of your camera not working if you put two cameras in your tree. Anyway, I think that if it should be built in it has to be a parameter of a WorldEnvironment node, as @QbieShay suggested. |
It appears that you have misunderstood the purpose of color management. This is not related to WorldEnvironment and is not something artists tweak or even put into the end game. It's for the editor itself, so that colors are correctly translated from sRGB to the color space of your monitor, so that they do not appear over-saturated and blown out. WorldEnvironment does not work with 2D and most importantly does not reach the Editor UI, so it doesn't cover the use case at all. |
Ah indeed sorry. If it's required for specific hardwares and in the editor it indeed has to be built in. Sorry for the inconvenience. |
No inconvenience felt. It's sad but true that color management is little known or understood outside the artist/photographer crowd, but it's a great thing and we should work to make it more mainstream! |
Added support for GLES2. I accidentally created a doppelganger commit while rebasing which triggered a review from neikeq because apparently I can't count commits... It's removed now. |
Rebased on current master branch. |
Sounds good. |
Rebased and resolved merge conflicts. |
Some errors compiling on windows:
Will report what happens when I start Godot. |
Here is some research for how to fetch the device's icc profile and links: |
@fire There is an unrelated bug in Godot where the background dim from modal dialogs (e.g. engine settings & file browser) will persist after closing in some cases. That one looks like what you're getting. It may have something to do with the lag that LUT generation introduces triggering the bug, but I don't think fixing that is within this PR's scope.
I'll check the build scripts on Windows as soon as possible.
Thanks for the pointers. I'll get to it after/if the base functionality get merged. |
@toasteater Can you point me to the bug number for the background dim? |
Since this impacts the renderer and conflicts with the work done in the It can still be reviewed and tested as is, but merging it now would make rebasing the |
Use VisualServer::set_screen_lut to set a 3D texture for color management. Its X, Y, Z axes correspond to R, G, B channels. LUT transformation happens in Rasterizer, ensuring that all content blitted to screen are processed exactly once. This implements part of #26826.
This module adds Color Management to engine settings. It depends on LittleCMS for ICC profile handling, and LUT support in Rasterizer for rendering. This completes #26826.
@toasteater Since the vulkan branch has a 2d branch in theory we could try this in the editor. |
@fire I don't see a separate 2d branch. Maybe I don't have the permissions? In any case I'd rather wait until things get more stable, after it's merged in That's said, if anyone wants to pick this feature up and start maintaining it for vulkan right now, please feel free to do so. From what I've seen about the |
Closing because I no longer have the intention to port the feature to 4.0. Again, if anyone is willing to pick this up, please feel free to do so. |
I want to have this feature in 4.0. Is there anything you suggest I can do? |
There are two commits in this PR. The first one adds LUT support at I think if you implement The sampling has to happen at |
@toasteater Is it ok to change the design so the lut is per rendertarget? |
Why would you want to do that? How are you going to make sure the colors are transformed exactly once? Keep in mind that this is for correct display on devices, not artistic effect. |
I was trying to figure out how to do per screen color correction, but godot doesn't have an api for that. |
I believe Godot is single-windowed-only at this moment? I don't think window managers allow us to see which part of the window is on which screen anyway. I have only seen applications that switch profiles depending on which screen it's mostly staying on, but not ones that do split rendering. It's probably too much work to be worthwhile for most cases. |
Also, there were a lot of misunderstandings in the thread, and I see that it still persists, so I think it bears repetition here at the bottom: To whoever it may concern: Please do bear in mind that this is NOT for artistic effects. It DOES NOT replace the environment effect that is called "color correction", which should be used for artistic effects. It should not be applied in any way, other than for each pixel rendered to the screen, uniformly, and exactly once, as the last step of rendering. Literally any other way to use it is a misuse. |
I am salvaging this. |
This adds LUT based color management on the Rasterizer layer, to ensure that all content blitted
to screen are processed exactly once.
Rasterizer::set_screen_lut
andVisualServer::set_screen_lut
are added. Implemented for both GLES3 and GLES2.The feature is accessible under Editor Settings / Interface / Color Management. It depends on LittleCMS to handle ICC profiles. This implements #26826.
EDIT: Updated OP to reflect current state of the branch. Again.