godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.58k stars 21.27k forks source link

Color profile management in the editor #26826

Closed ghost closed 4 years ago

ghost commented 5 years ago

Godot version: 201cb8d (3.1 RC1)

OS/device including version: Recent Arch Linux. GTX 970 w/ binary driver 418.43-4.

Issue description:

Expected: Godot is aware of ICC profiles, and render correctly on wide-gamut displays. Actual: Godot is not aware of ICC profiles. Colors are distorted and over-saturated.

Many systems (including Windows and X11 platforms) do not have end-to-end color management, and it's up to applications to convert images to device gamuts for display.

Currently, Godot converts all output to sRGB and pushes that directly to the device. The colors are then interpreted as coordinates in the device color gamut, which is usually not exactly sRGB either, but significantly different in case of wide gamut displays. This causes major color distortions, both in the editor and in the exported games.

Relevance:

Can it be worked around easily:

No. A workaround is possible, but significant work is required:

It's possible to use a custom postprocessing shader and a LUT to convert sRGB (or Linear with sRGB primaries) to whichever color gamut desired. However, custom postprocessing is not currently very wieldy, and it does not apply to UI elements (e.g. color choosers) on the editor side. It's also requires users to somehow generate their own LUTs from ICC profiles.

The Color Correction option in Environment is not fit for this purpose, as it can't be used for 2D, and also conflicts with scene grading within the sRGB gamut.

How can this be implemented:

While arbitrary gamut conversion can be nice, the most currently needed feature is conversion from sRGB to display gamut, in the editor. One possible way is to allow users to specify an ICC profile in preference, which will be used to generate a 3D LUT texture on load. This resulting texture will then be used to transform the entire editor interface as the last step of rendering. This should suffice for the current use case.

Possible further improvements outside the scope of this feature proposal may include:

Can I code this:

Maybe, if it's possible to take LittleCMS (MIT license) on as a dependency for this feature, which I understand won't be a very lightly made decision. ICC profiles are rather complex, so writing one's own parser can be quite slow and error prone. I'm also not experienced with Godot's code base, so I'm seeking feedback before coding anything.

Steps to reproduce:

Expected: Same color rendered. Actual: The color rendered by Godot is much more saturate.

Unfortunately, the effect is much less observable on a normal sRGB-ish display. On my device GIMP renders the sRGB ff0000 as d53b1a, if that puts it into any perspective.

red (Left: Godot, Right: GIMP w/ ICC profile. Screenshot interpreted as sRGB so the difference can be seen with ordinary displays. The actual saturation difference seen is much greater than what this image suggests.)

Minimal reproduction project: Not applicable. Everything rendered by Godot is affected.

unfa commented 5 years ago

Hey! I've been playing around in the Godot 3.1.1 build on Manjaro. I've seen some support for "Color Management" but it really seems like an aftertought rigth now, and more like an "illusion of color management" than the real thing.

Also - inputting colors in the "RAW" mode is still capped at 100 values for R,G,B.

I know Troy Sobotka is someone who knows a lot about this. I guess we really need an expert to make sure that pixels are color-managed form the ground up in Godot, if we want to make it comparable to other modern engines...

Maybe the Vulkan implementation would be a good opportunity to break some compatibility if needed?

sobotka commented 5 years ago

Might as well save you pain; LCMS will be too slow, and likely involves the wrong approach.

You may wish to investigate OpenColorIO, as this also integrates into asset development for game engine work.

unfa commented 5 years ago

Also: I am not sure if GIMP can be used as a ground truth here.

ghost commented 5 years ago

@unfa @sobotka This issue is about "Color Management" as in the screen profiling or "convert <(standard-)display-referred-space> to <(device-)display-referred-space>" part. For "convert \ to <(standard-)display-referred-space>" (OpenColorIO) this is likely the wrong issue, and it's best if you create a new one for it.

sobotka commented 5 years ago

This issue is about "Color Management" as in the screen profiling or "convert <(standard-)display-referred-space> to <(device-)display-referred-space>" part

No, it’s not. It’s actually a top to bottom issue, right to the core of pixel handling.

OpenColorIO can do the exact transform, at speed, on the GPU, to convert from sRGB to Display P3, or ACES, etc.

You don’t have to listen, but I assure you, LCMS is a smouldering dumpster fire if you are thinking you can use it for a game engine. Cut to the chase and use something more appropriate. There’s a good reason Unreal has started integrating OCIO.

ghost commented 5 years ago

You don’t have to listen, but I assure you...

Just FYI: I'm not the person to convince here. I'm just an ordinary user and not a core member of this project. I don't get to decide what get merged or not, and I'm not obliged follow the roadmap either: I only work on what I need, when I want to.

I'm only suggesting that you make a new issue because that way you have the best chance of getting noticed by someone who might want to implement it.

OpenColorIO can do the exact transform, at speed, on the GPU, to convert from sRGB to Display P3, or ACES, etc. ..., LCMS is a smouldering dumpster fire if you are thinking you can use it for a game engine. Cut to the chase and use something more appropriate. There’s a good reason Unreal has started integrating OCIO.

I don't disagree with you: I realize that and I use OCIO, and even filmic-blender for some scene-referred scribbling myself.

However, there's a real logistic problem that most of us using Godot aren't exactly big AAA studios. Cheaper colorimeters only produce ICC profiles and while I made a tool to convert them to spi3ds for myself, I can't see how I'll convince a freelancer to go through the hoops of editing yamls.

I have implemented what I needed in PR #26869, it actually only uses LCMS to generate the LUT, and does the rest on GPU. It's working fair enough for my use case, so I don't currently see a need to do more.

Godot is an open source project with a bazaar development model. If you (or anyone) want to work on integrating OCIO, then by all means feel free to do it!

sobotka commented 5 years ago

I'm pretty sure no one around Godot has much of a clue about pixel management, nor really cares judging from the documentation.

With that said, I believe your code is making assumptions about white point? ICCs use D50 internally, and v4 ICCs really made a mess of things. That is, if you don't consider the destination white point, your transform will be a mess. In a vast majority of cases, your display will rarely be D50.

V2 ICC used to be able to rely on the absolute colorimetric rendering intent to derive the colour, but V4 has made it a confusing mess, and now absolute colorimetric is ICC absolute colorimetric, which is D50. The only remaining option it would seem is to lean on the chad tag if it is present. Confused yet? Good.

At any rate, you'd probably want to make sure that the colours are at least transformed to the proper output white, which again, will rarely be ICC D50.

References that might help: https://sourceforge.net/p/lcms/mailman/message/31985055/

ghost commented 5 years ago

With that said, I believe your code is making assumptions about white point? ICCs use D50 internally, and v4 ICCs really made a mess of things. That is, if you don't consider the destination white point, your transform will be a mess. In a vast majority of cases, your display will rarely be D50.

That's probably true. I'm assuming that LCMS is doing the conversion to the "nominal" (profile) white point, and also that vcgts are loaded so that the monitor white point matches the nominal one. On my setup it gets me outputs that match those from my image manipulation / drawing software. Again, might not be total ground truth, but might as well be for my needs.

At any rate, you'd probably want to make sure that the colours are at least transformed to the proper output white, which again, will rarely be ICC D50. References that might help:...

Thanks a lot for the information. I'll check the reference.

sobotka commented 5 years ago

The ICC situation is a shambling mess regarding this particular issue, and there isn't a terrific way out. Another option would be to skip the whole ICC thing, and use DisplayCal or such to generate a 3D LUT from the ICC, and use that on your GPU. The reason this would succeed over the ICC approach is that Florian wisely made it such that you can specify the white point and force it to do the right thing.

It's really nice code. Keep up the good work.

unfa commented 5 years ago

Posibly related?

29122

clayjohn commented 4 years ago

@fire may want to open the proposal for this. I know you plan on continuing the work for this in Vulkan.

Feature and improvement proposals for the Godot Engine are now being discussed and reviewed in a dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine.

The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, we are now closing all older feature proposals on the main issue tracker.

If you are interested in this feature proposal, please open a new proposal on the GIP tracker following the given issue template (after checking that it doesn't exist already). Be sure to reference this closed issue if it includes any relevant discussion (which you are also encouraged to summarize in the new proposal). Thanks in advance!