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

High energy lights or emissive materials should approach or become white at very high levels #3214

Closed
atirut-w opened this issue Aug 28, 2021 · 15 comments · Fixed by godotengine/godot#52476

Comments

@atirut-w
Copy link

atirut-w commented Aug 28, 2021

Describe the project you are working on

(Deemed irrelevant)

Describe the problem or limitation you are having in your project

(Deemed irrelevant)

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

As of currently(specifically Godot 3.3.3), very bright lights and/or emissive materials in Godot retain the original emission or light color even at very high levels which can limit how realistic Godot games can look.

Light example(Filmic tonamap, default settings):
Screenshot from 2021-08-28 13-02-29
[This red light have the energy value of 70]

Emission example(Filmic tonamap, default settings):
Screenshot from 2021-08-28 13-05-58
[Same energy value for emission]

(Extra) same emission with glow enabled, also default settings + bicubic upscale:
Screenshot from 2021-08-28 13-07-26

Realistically, when something is very bright, the color will approach or even become white as this answer on Physics Stackexchange explains it. If this is to be implemented in Godot, it could make bright lights look a lot more realistic.

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

I am not a graphics programmer, but I guess you can have Godot internally use something like R11G11B10 for colors, then clamp the values. This should make very bright colors become white after clamping.

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

I don't think this can be implemented as a script because the need to change how the engine internally handle colors.

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

I also don't think addons can change how the engine handle colors.

@Calinou
Copy link
Member

Calinou commented Aug 28, 2021

@JFonS was looking at improving the tonemapping to handle desaturation, but he didn't see anything wrong with the current code.

@atirut-w
Copy link
Author

didn't see anything wrong with the current code.

Maybe the code is not wrong but the formula is? Color math is complicated, after all 🤔

@Zylann
Copy link

Zylann commented Aug 29, 2021

I played with the white parameter a bit, making sure to not use the Linear mode (which is default and said to not take that parameter into effect).
I saw that if you put only R, G or B in the color and other components to zero, it will never reach white.
image

But if you add to the others, it eventually gets white.
image
It happens at 1 here because white defaults to 1, but it changes as you change the parameter. It really seems to behave like a component-wise clamping.

No idea if that's physically correct or not, but from an artistic standpoint maybe it is unexpected?

@Calinou
Copy link
Member

Calinou commented Aug 29, 2021

No idea if that's physically correct or not, but from an artistic standpoint maybe it is unexpected?

How do other engines handle this situation with fully saturated colors?

@atirut-w
Copy link
Author

No idea if that's physically correct or not

According to a page in the Unreal Engine docs, no.

Quoting the docs:

[...] as the emissive power increases the color will become lighter, similarly to how colored lights work in the real world. [...] if the final color is bright enough to start saturating the film / sensor, it will become white.

Screenshot from 2021-08-29 18-57-16

I saw that if you put only R, G or B in the color and other components to zero, it will never reach white.

It would seem that lights work a bit differently. Here's a light with RGB color of 23,255,250 and the energy level of 70(these settings make emissive materials white):
Screenshot from 2021-08-29 19-01-59

Sphere with emission(same color, same energy) for comparison:
Screenshot from 2021-08-29 19-02-30

For lights, it would seem that other components of the color have to be a bit higher for very bright areas to become white.

@Lauson1ex
Copy link

To add to what @atirut-w said, the problem seems to be 4-fold: lights aren't physically based, cameras aren't physically-based, the bloom shader isn't physically based, and the HDR isn't physically based. Parameters for these four entities are completely disjoint from one another in the current rendering pipeline. Naturally, results aren't physically accurate.

To get the accurate values, a physically-based camera model should be supplied in order to get the correct luminance in which the sensor begins to get saturated.

Regardless, there seems to be some sort of component-wise clamping going on in the current bloom and tonemapping shaders, because I was able to replicate the desired, accurate effect with a custom implementation, using the luminance of the pixels.

Godot.Engine.-.Custom.HDR.Shader.-.main.tscn._.30_08_2021.21_39_38.mp4

@JFonS
Copy link

JFonS commented Aug 31, 2021

I checked this a while ago, and IIRC we are using the same code found on this blog post: https://knarkowicz.wordpress.com/2016/01/06/aces-filmic-tone-mapping-curve/

As mentioned there, this is just an ACES approximation and it over-saturates bright colors. I tried replacing the code for the one linked in the post and, indeed, the shift to white was working as expected. I didn't open a PR for it because, as pointed out, there are many factors that come into play in the color pipeline, and I was hoping to take the time and sort most of it out. I'm not confident enough to do such changes yet, so for now I keep reading literature on the topic from time to time :)

@atirut-w
Copy link
Author

4-fold: lights aren't physically based, cameras aren't physically-based, the bloom shader isn't physically based, and the HDR isn't physically based.

This one's gonna be a PITA to get merged and closed isn't it?

@Calinou
Copy link
Member

Calinou commented Aug 31, 2021

This one's gonna be a PITA to get merged and closed isn't it?

See #723. It's not planned for 4.0, but it is for a future 4.x release.

@atirut-w
Copy link
Author

atirut-w commented Sep 1, 2021

for a future 4.x release

Is there a version of this for 3.x? At least implementing what @Lauson1ex did into Filmic would be great.

@Lauson1ex
Copy link

I managed to integrate the more accurate ACES fitted with Godot's exposure_bias and white. The exposure mostly matches the current ACES algorithm's exposure and it all quite works as expected. It mostly matches the current ACES tonemapper in UE4.

Here are two comparison screens from the TPS Demo, one using the current ACES implementation, and the other using the more accurate ACES implementation:

Godot's ACES implementation:
image

ACES fitted:
image
Notice the difference on the grates with yellow light on the bottom-left, and the cylindrical structures with cyan lights.

And here are some pulsing emission spheres which change energy from 0 to 16:

ACES.Fitted.DEBUG.05_09_2021.01_56_23-2.mp4

There are a few usability details that concern me, though. I don't think it's a good idea to replace the current ACES implementation because:

  • there are legitimate artistic use cases where the user might want to keep the oversaturated emissive material color;
  • this more accurate ACES implementation is more expensive since it involves a transform from RGB to ACES color space, and a transform from ACES color space back to RGB, which might not be suitable for VR or mobile (and rendering performance in 3.x is at a premium).

So I propose adding this ACES implementation as a new tonemapper mode instead. So the options would be either:

  • Linear
  • Reinhard
  • Filmic
  • ACES
  • ACES Fitted

or

  • Linear
  • Reinhard
  • Filmic
  • ACES (Legacy)
  • ACES

What do everyone think? If this was just a replaced I could submit a PR right now. Adding a new option will require a little more work. For Godot 4 I think it makes sense for the ACES tonemapper to be replaced by this one.

@clayjohn
Copy link
Member

clayjohn commented Sep 5, 2021

@Lauson1ex I have no preference for keeping the old vs the new. If we merge for 3.x, we should add an additional option (I prefer calling it "ACES Fitted", but that is totally personal preference), but for 4.0 I think it is okay to replace completely unless any users have a strong objection.

In this case because we still have the Reinhard and filmic options which look quite similar, I think it is best to just stick with the higher-quality ACES implementation.

@atirut-w
Copy link
Author

atirut-w commented Sep 5, 2021

I think it is best to just stick with the higher-quality ACES implementation.

Probably off topic but why do people seem to prefer ACES more than Filmic? Is it because the new implementation is made for ACES or is it because ACES is more realistic(or physically accurate)?

Btw, for someone like me who love realistic graphics, the new "ACES Fitted" tonemap just looks sooooo gorgeous.

@atirut-w
Copy link
Author

@Calinou I noticed this just now but why add this to 4.0 milestone after it's already closed(with related PRs merged)?

@Calinou Calinou modified the milestones: 4.0, 3.x Nov 24, 2021
@Calinou
Copy link
Member

Calinou commented Nov 24, 2021

@Calinou I noticed this just now but why add this to 4.0 milestone after it's already closed(with related PRs merged)?

For closed proposals, milestones indicate which Godot version the proposal was first implemented in. The first Godot version to implement this proposal is actually 3.4, now that godotengine/godot#52477 was merged. Therefore, I updated the milestone to 3.x.

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

Successfully merging a pull request may close this issue.

6 participants