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 Specular Occlusion to meshes under influence of LightmapGI #86102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guerro323
Copy link

@guerro323 guerro323 commented Dec 13, 2023

Closes godotengine/godot-proposals#8637

image

This pull request implement specular occlusion which reduce the specular light of objects under a lightmap.

It offers two methods and an option to disable it in the project settings:

  • Reduce: Reduce specular light by the brightness of the current lightmap pixel.
  • Conservative (Default): Closest to ground truth, it keeps more energy on highly reflective materials (such as metallic=1.0 && roughness=0.0).
  • Disabled: Existing behavior before this PR.

Which one to choose?

  • Use Reduce at first, while it doesn't give a correct result especially with high reflective materials it's gonna be better than having it reflect the sky while fully occluded.
  • Use Conservative when your materials are well setup (such as having correct PBR textures) and a good scene that has good overall reflection sources (either from reflection probes or with GI reflections).
  • Use Disabled if you want your metallic and shiny materials to act the same as before or if you want a specific art style.

Specular occlusions mixes well and improves:

It support both Forward+, Mobile and Compatibility renderers.

To get started using it, you just need to open an existing scene with my changes and it should work out of the box. Of course if you used materials that act on the specular light (metallic, roughness, specular...) you may need to make some small modifications on them.

Screenshots and Comparisons

Custom Scene

A scene I'll provide later once I'll fix some meshes issues (and improving the textures)

No Lightmaps Current 4.2 Specular Occlusion Reduce Specular Occlusion Conservative
image image image image

Japanese Street

Link: https://sketchfab.com/3d-models/japanese-street-at-night-fb1bdcd71a5544d699379d2d13dd1171
On this one, Conservative has less impact since a majority of the textures aren't metallic and reflective enough.

Current 4.2 Specular Occlusion Reduce Specular Occlusion Conservative
image image image

Box

Note: A fully reflective material on Conservative can still be 100% darkened, it's not happening here because it's not dark enough

metallic=0 metallic=1 metallic=1 roughness=0 reduce metallic=1 roughness=0 conservative
image image image image

Influences by lightmap probes

Characters from: https://github.com/gdquest-demos/godot-4-3D-Characters
The scene used here isn't a good one to compare

metallic=1 roughness=0

Godot 4.2 Specular Occlusion Reduce Specular Occlusion Conservative
image image image

metallic=1 roughness=1

Godot 4.2 Specular Occlusion Reduce Specular Occlusion Conservative
image image image

Dynamic objects on the first scene:
image
(note: probes seems to have issue outside of this PR in Physical Lights mode, I disabled it here which is why it may look different)

Problems

Dynamic lights and dynamic objects will not have a correct specular occlusion as it will be a bit darker. It can be boosted in the code but I just need to find the correct value so that it look good enough. Otherwise you can either reduce metallic or use the `Conservative` method to boost it. Reason is that the lightmapper seems to darken the lightmap a little bit when in dynamic mode.
No Lightmaps Lightmapped Non-Static Light Lightmapped Non-Static Light + Conservative Lightmapped Static Light
image image image image

@guerro323 guerro323 requested review from a team as code owners December 13, 2023 01:31
@jcostello
Copy link
Contributor

Awesome results. I will test it out.

image

In that screenshot, where the reflections comes from?

@jcostello
Copy link
Contributor

I test it. Works incredible well. The results are more realistic and works awesome with reflection probes

@guerro323
Copy link
Author

Awesome results. I will test it out.

In that screenshot, where the reflections comes from?

They come from the middle of the bottom cube!

@guerro323 guerro323 force-pushed the lightmap-gi-specular-occlusion branch from fee4aa8 to 2a7a454 Compare December 13, 2023 13:24
@dsnopek
Copy link
Contributor

dsnopek commented Dec 13, 2023

Is this a feature that makes sense to add to the compatibility renderer too? Now that PR #85120 is merged, LightmapGI will be rendered in the compatibility renderer too.

@guerro323
Copy link
Author

Is this a feature that makes sense to add to the compatibility renderer too? Now that PR #85120 is merged, LightmapGI will be rendered in the compatibility renderer too.

Compatibility support has been added 🙂

Before After Default After Conservative
image image image

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks amazing! Great work.

My only concern is over whether we need two render modes for this. We try to keep options simple as possible.

Looking at the code, the conservative mode only adds a couple of instructions and it appears to be the more "correct" looking option. Would it make sense to only expose one render mode for specular occlusion and have it apply the current "conservative" option?

Basically, is there a situation where the non-conservative option is preferred other than just being slightly less costly?

@jcostello
Copy link
Contributor

I think this looks amazing! Great work.

My only concern is over whether we need two render modes for this. We try to keep options simple as possible.

Looking at the code, the conservative mode only adds a couple of instructions and it appears to be the more "correct" looking option. Would it make sense to only expose one render mode for specular occlusion and have it apply the current "conservative" option?

Basically, is there a situation where the non-conservative option is preferred other than just being slightly less costly?

I didn't get what you suggest. Also I found the default mode more pleasant for indoors and some other scenes that I have (This is an artistic choise)

@guerro323
Copy link
Author

guerro323 commented Dec 13, 2023

@clayjohn
I agree that having two modes for it complicate the whole thing :/
There are two important factors where "conservative" might not be the solution:

  • The project's art style
  • The reflection sources: If there is only the sky for reflections you'll get a too much blown out material, it's the most accurate but may not suit the art style or context of the object.

But "conservative" is the best one when there are enough source of reflections in darker areas.
To illustrate the problem better, here is an example:

Lightmap Only + Reflection Probe in the center (capture mostly the sky) and "Conservative" LM + "Default" LM + SDFGI + "Conservative" LM + SDFGI + "Default"
image image image image

So as shown in the table, if you choose conservative and don't have any possibility for another reflection then it will look too much blown out. But enabling SDFGI here make conservative the best choice.

I'm not really sure what to do if we're going for one setting as it would mostly force realistic art styles, maybe going for a mix of both? Or a way to keep both but make it simpler but how?
Maybe a slider would be better and it would lerp between the two option?

@Calinou
Copy link
Member

Calinou commented Dec 14, 2023

Can we expose the Default/Conservative/Disabled option as a project setting rather than a render mode? As I understand it, you probably won't need to use both modes at the same time in the same project, and probably don't need to change it at runtime either.

This should allow having only a single render mode and avoiding an increase in the number of shader permutations or VGPR usage.

@jcostello
Copy link
Contributor

Can we expose the Default/Conservative/Disabled option as a project setting rather than a render mode? As I understand it, you probably won't need to use both modes at the same time in the same project, and probably don't need to change it at runtime either.

This should allow having only a single render mode and avoiding an increase in the number of shader permutations or VGPR usage.

Could be a Lightmap property (this is the first place I look at it)

@guerro323
Copy link
Author

It has been moved to a project settings.

For now modifying the value require a restart.
I'm not sure if it's possible to do a shader recompile while the editor/game is running as it become useful to swap between Reduce/Conservative mode when SDFGI/VoxelGI is disabled/enabled when combined with my other PR (#86135); it only become a problem with big projects that takes a long time to load.

@guerro323 guerro323 force-pushed the lightmap-gi-specular-occlusion branch from 9dba392 to 4b39a9e Compare December 14, 2023 19:04
@s-ilent
Copy link

s-ilent commented Dec 15, 2023

A few things...

  1. Would it make sense to have the intensity of specular AO from lightmap be a user-adjustable value? What intensity in the lightmap is "dark" depends a lot on the scene. It seems like you're suggesting the simple/complex mode switch should be used for determining whether the user want more or less specular AO, but that seems like it makes what the specular AO is doing less explicit. Plus a project-wide setting is harder to control. I can see the specular AO is attenuated by the exposure value, but I'm worried that wouldn't cover for all scenarios. Having a tweakable value (i.e. scaling the occlusion value before clamping) would also help for different art styles.
  2. Maybe it would make sense to apply a specular occlusion attenuation function like the Frostbite one Filament uses here? That might be a better solution than the current flat adjustment based on the roughness/metallic values. https://github.com/google/filament/blob/main/shaders/src/ambient_occlusion.fs#L85

@Calinou
Copy link
Member

Calinou commented Dec 15, 2023

Maybe it would make sense to apply a specular occlusion attenuation function like the Frostbite one Filament uses here? That might be a better solution than the current flat adjustment based on the roughness/metallic values.

See #50601. This only affects AO maps and SSAO though, not lightmaps.

@guerro323
Copy link
Author

guerro323 commented Dec 15, 2023

Would it make sense to have the intensity of specular AO from lightmap be a user-adjustable value? What intensity in the lightmap is "dark" depends a lot on the scene.

To what it's considered dark depends on what the camera see on the lightmapped object so it takes the current exposure in account.
The way it work, is that if a fully rough and non metallic material pixel is dark from the camera then it should be the same for a metallic and reflective material pixel in Reduce mode; it can be brighter when using the Conservative mode and if the pixel isn't totally black, but if you have good reflection sources then it should be approximately correct.

Having it as an user-adjustable value would complicate it even further and may introduce a slight cost of performance¹ (and this effect is intended to be cheap and used on mobile and low-end PCs, if you need a correct look you need to use directional lightmaps).

It seems like you're suggesting the simple/complex mode switch should be used for determining whether the user want more or less specular AO, but that seems like it makes what the specular AO is doing less explicit.

For the conservative part, I'm not really sure how it's less explicit as it's an approximation of what Blender Cycles does, you can see a quick comparison between blender and godot here:
(top is Blender, and bottom is with conservative specular occlusion and some reflection probes)

Full Diffuse Roughness=0 Metallic=1 Roughness=0 And Metallic=1
image image image image
image image image image
Full Diffuse Roughness=0 Metallic=1 Roughness=0 And Metallic=1
image image image image
image image image image

Note: You really need good reflection sources for interior scenes and sometime reflection probes may not work as expected (like in the last row with roughness set to 0); that's where #86135 will help.

¹ If there will be a need for one in the future, there could be a SPECULAR_OCCLUSION builtin for the light() function in a shader, but I'm not sure how it could be properly implemented as you'll also need to expose the current lightmap value, make sure it's not using directional LM and that it runs at the same lightmaps are applied. Another problem for it is that it calculations for it will only work for lightmapped objects.

@WickedInsignia
Copy link

In my brief tests this works spectacularly well. Would be interesting to see a slider to adjust how much specular occlusion is present, or to have it tied to an individual material rather than the whole scene (if you've ever used Silent's Filamented Shader in Unity, it's a really great example of offering artistic options for this technique). I understand there may be some limitations to those options though.
Regardless, great work on this. It adds a lot of quality to lightmapping for reflective surfaces.

@guerro323 guerro323 force-pushed the lightmap-gi-specular-occlusion branch 2 times, most recently from aa17332 to 73dcf1a Compare January 25, 2024 15:45
@guerro323 guerro323 force-pushed the lightmap-gi-specular-occlusion branch from 73dcf1a to a636046 Compare January 25, 2024 15:48
@atirut-w
Copy link
Contributor

atirut-w commented Jan 27, 2024

(Moved from #86135)

After playing with this PR + #86135 for some time, I don't think doing the occlusion based on indirect lighting lightmap is a good idea. There's the obvious issue with differences between static & dynamic lights, but also scenarios where this won't look accurate.

One of those scenarios would be a dimly lit room with window(s) to the outside:
image

The reflections here are not correct. Here is how it looks with only SDFGI:
image

Maybe we should bake a sort of "occlusion map" instead?

@WickedInsignia
Copy link

@atirut-w Specular occlusion as it's implemented here doesn't seem to handle high dynamic range very well, so having areas of great contrast will mean that too little or too much is being occluded. It would be best to keep scene lighting at a fairly constant brightness when using this technique.
To my knowledge there's no setting present to reduce the light range that is being occluded, but I've seen this done in some shaders for Unity (such as Silent's Filamented).

This technique is fine and needed. It's not very physically accurate but lends much improvement to lightmapped reflections at little cost. Baking out a scene-wide ambient occlusion map might help in instances that a higher dynamic range is needed at the cost of memory footprint, but I don't think it should be used instead.

A slider to adjust what Godot considers dark enough to occlude, PLUS the ability to use a scene-wide AO bake instead of the normal lightmap if desired would probably be best.
It would also be worth keeping in mind that #86135 is available if high dynamic range lighting is needed since it is using DynamicGI for reflections instead, at a higher performance overhead than specular occlusion.

@guerro323
Copy link
Author

guerro323 commented Jan 27, 2024

A slider to adjust what Godot considers dark enough to occlude, PLUS the ability to use a scene-wide AO bake instead of the normal lightmap if desired would probably be best.

For a greater control, I think it would be better to have a better support for it in custom shaders (but it's out of scope for this PR?), like how the light() function works:

void lightmap() {
    DIFFUSE_LIGHT += LIGHTMAP_COLOR * LIGHTMAP_EXPOSURE;
    SPECULAR_LIGHT *= length(LIGHTMAP_COLOR) * LIGHTMAP_EXPOSURE;
}

The problem with that is that there needs to be a lot of stuff exposed for it; and it also needs to work for directional lightmaps, probes and should work with #86135 ...


For a simple slider, while it can be exposed in the LightmapGI node I'm not sure if I could instead expose it in the material shader (eg: SPECULAR_OCCLUSION_INTENSITY) as there were already issues when the two occlusion modes were a render mode (#86102 (comment)).

@WickedInsignia
Copy link

I think it would be better to have a better support for it in custom shaders

Having it as a per-material setting that's exposed in StandardMaterial and custom shaders (whether they be node or code-based) would be ideal, definitely. Sometimes you want to control how each independent material handles specular occlusion, and it also allows for a different setting on duplicate materials that are used in darker areas to overcome high dynamic range issues.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased against master 17e7f85), it works as expected. This is a great visual improvement 🙂

Example with a reflection probe:

Conservative (default)

Screenshot_20240127_202948

Reduce

Screenshot_20240127_203006

Disabled

Screenshot_20240127_203017

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

Successfully merging this pull request may close these issues.

Add a cheap specular occlusion for LightmapGI
9 participants