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

NodeMaterial: Added energy preservation flag #17163

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Aug 3, 2019

Description

Energy Preservation is a characteristic of global light, not only IBL PREM how is today defined. This makes customized materials like NodeMaterial do not use these important equations for Physically-Based Material.

Although it is a subtle difference, it can be noted an improvement in the incidence of lights in the fresnel area. Now, this can be applied in materials than use dynamic lights too, not only if have IBL PREM for example.

This is easy to enforce in a shading system: one simply subtracts reflected light before allowing the diffuse shading to occur. This means highly reflective objects will show little to no diffuse light, simply because little to no light penetrates the surface, having been mostly reflected. The converse is also true: if an object has bright diffusion, it cannot be especially reflective.

Energy conservation of this sort is an important aspect of physically-based shading. It allows the artist to work with reflectivity and albedo values for a material without accidentally violating the laws of physics (which tends to look bad). While enforcing these constraints in code isn’t strictly necessary to producing good looking art, it does serve a useful role as a kind of “nanny physicist” that will prevent artwork from bending the rules too far or becoming inconsistent under different lighting conditions.

Quote: https://marmoset.co/posts/basic-theory-of-physically-based-rendering/

References

Energy Compensation in #16977 Enterprise PBR
Energy conservation in https://learnopengl.com/PBR/Theory
ENERGY CONSERVATION in https://marmoset.co/posts/basic-theory-of-physically-based-rendering/

Others

Issue #17102

Images bellow in this case is used a simple cube map.

/ping @WestLangley @bhouston


material.energyPreservation = true;

energyPreservation=on

material.energyPreservation = false;

energyPreservation=off

@mrdoob mrdoob changed the title energy-preservation flag NodeMaterial: Added energy preservation flag Aug 5, 2019
@mrdoob mrdoob added this to the r108 milestone Aug 5, 2019
@mrdoob mrdoob merged commit d4a7c78 into mrdoob:dev Aug 5, 2019
@mrdoob
Copy link
Owner

mrdoob commented Aug 5, 2019

Thanks!

@WestLangley
Copy link
Collaborator

It does not make sense for a proper PBR model to have energy preservation as an option.

At some point I expect the shaders will be corrected or improved and this property will be removed.

@sunag
Copy link
Collaborator Author

sunag commented Aug 5, 2019

It does not make sense for a proper PBR model to have energy preservation as an option.
At some point I expect the shaders will be corrected or improved and this property will be removed.

I agree with your thinking, we could leave this option by default and remove this define and property?

@mrdoob
Copy link
Owner

mrdoob commented Aug 6, 2019

@WestLangley Should we remove the non-ENERGY_PRESERVATION code?

@sunag
Copy link
Collaborator Author

sunag commented Aug 6, 2019

I create a PR in case of @WestLangley approved the changes.

@WestLangley
Copy link
Collaborator

I think @jsantell is going to have to be involved in this decision as he was the one who added that code block.

#17102 (comment)

@sunag
Copy link
Collaborator Author

sunag commented Aug 6, 2019

I think that material.energyPreservation = false is only a process of optimization in case of non-use irradiance.

@WestLangley
Copy link
Collaborator

@sunag. So you understand the issue, please do an experiment and make the following change:

In lights_fragment_maps.glsl.js

#if defined( USE_ENVMAP ) && defined( PHYSICAL ) && ( defined( ENVMAP_TYPE_CUBE_UV ) || defined( ENVMAP_TYPE_CUBE ) )

	irradiance += getLightProbeIndirectIrradiance( /*lightProbe,*/ geometry, maxMipLevel );

#endif
  1. Now create MeshStandardMaterial( { roughness: 0, metalness: 0, envMap: envMap } ) where envMap is any JPG cube map.
  2. Add no other light sources.
  3. Vary metalness from 0 to 1 and roughness from 0 to 1.

If your browser implements seamless mipmapping, it should look pretty good; if not, it will look terrible.

Since seamless mipmapping was not supported, the irradiance term was removed. Maybe we can add it back now for the non-PMREM use case.

  1. Now revert the above change, repeat the experiment, and compare the differences.

@mrdoob
Copy link
Owner

mrdoob commented Aug 6, 2019

If your browser implements seamless mipmapping, it should look pretty good; if not, it will look terrible.

Did I remember correctly that seamless mipmapping is something WebGL2 supported?

@sunag
Copy link
Collaborator Author

sunag commented Aug 6, 2019

Did I remember correctly that seamless mipmapping is something WebGL2 supported?

@mrdoob I think that @WestLangley is referring at extension EXT_shader_texture_lod? I did not get to test still, but I do this to see if this is what I am thinking...

@WestLangley
Copy link
Collaborator

On my mac mips do appear to be seamless. Maybe @sunag is right and it is the extension...

@sunag
Copy link
Collaborator Author

sunag commented Aug 6, 2019

@WestLangley if your mips example is related with the availability of EXT_shader_texture_lod you too can use TEXTURE_LOD_EXT in define like:

#if defined( USE_ENVMAP ) && defined( PHYSICAL ) && ( defined( ENVMAP_TYPE_CUBE_UV ) || 
( defined( ENVMAP_TYPE_CUBE ) && defined( TEXTURE_LOD_EXT ) ) )

	irradiance += getLightProbeIndirectIrradiance( /*lightProbe,*/ geometry, maxMipLevel );

#endif

@jsantell
Copy link
Contributor

jsantell commented Aug 6, 2019

Energy preservation as an option makes sense at the very least for a perf improvement if wanted. I think this could be confusing in the future -- is it possible that this would be fixed if the irradiance was only from the environment map?

via @ @WestLangley #17102 (comment)

ENVMAP_TYPE_CUBE has implemented in lights_physical_pars_fragment affecting global light, not only IBL PREM, the problem this, is that the indirect/direct diffuse and specular of PREM blend with others lights, including the analytical AmbientLight, HemisphereLight because they all too generate irradiance diffuse that can be used in energy preservation calculation.

@jsantell Yes, @sunag is correct. The irradiance you pass into your multiple-scattering calculation is from all indirect light sources, not just from the env map. I am not sure if that is correct or not...

Great observation. This is something we should look into if irradiance handles all indirect sources. If that wouldn't affect the node material lighting, but otherwise the energy preservation works for now. What happens when ENVMAP_TYPE_CUBE_UV is not defined (non-PMREM texture) but energy preservation is on?

@sunag
Copy link
Collaborator Author

sunag commented Aug 6, 2019

What happens when ENVMAP_TYPE_CUBE_UV is not defined (non-PMREM texture) but energy preservation is on?

It affects others lights that use irradiation too.
In case of the envmaps_hdr example, no other irradiance light is added, for this reason, the energy conservation flag made no sense at that time. When have some native irradiance lights add-in scene and no IBL PREM, like Ambiente, Hemisphere and Proble, @WestLangley too up an issue about obtain irradiance from cubemap using native TEXTURE LOD #17163 (comment), we could make use of energy-preservation calcs for these others light sources.

@WestLangley I will make some tests to compare the quality of pre-processed map PREM with LOD using the same map.

@sunag
Copy link
Collaborator Author

sunag commented Aug 6, 2019

About optimization, this could not be like this too?
https://github.com/sunag/three.js/blob/7a6666b8f42b6359c1cdb0f1efa4037c891da9e6/src/renderers/shaders/ShaderChunk/lights_physical_pars_fragment.glsl.js#L123

	// Both indirect specular and diffuse light accumulate here
	// if energy preservation enabled, and PMREM provided.
	if ( irradiance > 0. ) {

@bhouston
Copy link
Contributor

bhouston commented Aug 7, 2019

I have previously suggested that we should pass in a single irradiance spherical harmonic into the shader that is a representation of all irradiant light sources (indirect diffuse) here:

#16228 (comment)

And further expanded here:

#16228 (comment)

This would simplify the code and our in-shader model. Irradiance would then not be queried from the PMREM but rather than SH representation. The PMREM would only be used for indirect specular, not indirect diffuse.

@sunag
Copy link
Collaborator Author

sunag commented Aug 7, 2019

The PMREM would only be used for indirect specular, not indirect diffuse.

This justifies my tests here:
https://rawgit.com/sunag/three.js/dev-nodes-irradiance/examples/webgl_materials_nodes.html?e=bias

@sunag
Copy link
Collaborator Author

sunag commented Aug 7, 2019

I expected ProbleLight using IBL (radiance and irradiance) with RTT or predefined CubeMap and tetrahedral tesselations like unity. I am happy you have created this topic. For me today ProbleLight nothing more than SphericalHarmonicLight.

About the name. this not should be ProbeLight instead of LightProbe following the logic of the names of threejs?
#7776 (comment)

@WestLangley
Copy link
Collaborator

I have previously suggested that we should pass in a single irradiance spherical harmonic into the shader that is a representation of all irradiant light sources (indirect diffuse)

@bhouston I have found that HDR and spherical harmonics do not work well together. HDR can have a high dynamic range, and that can not be accommodated by SH3; it leads to ringing and negative irradiance. There have been hacks proposed to counter those artifacts, but they are... hacks.

Instead, users should be able to specify a radiance map and an irradiance map.

In fact, I think this makes sense:

  1. PMREM should be handled automatically for the user -- not in the app itself.
  2. The user may specify a radiance map in cube form.
  3. The user map optionally specify an irradiance map in cube form.
  4. If the irradiance map is not specified, one will be created automatically, or some alternative approximation will be used, as we do now.

@WestLangley
Copy link
Collaborator

For me today ProbeLight nothing more than SphericalHarmonicLight.

A LightProbe can be a cube map. If the probe models irradiance, it can be a small cube map.

In my opinion, SH is not a good technique for modeling irradiance from HDR sources.

@bhouston
Copy link
Contributor

bhouston commented Aug 7, 2019

I believe @WestLangley you are talking about high contrast HDR maps. The issue isn't the range, spherical harmonics handle range well, it is the high contrast that is the problem.

This was discussed in the glTF working group. As a result of this the Babylon project wrote this up -- and I do support this approach:

https://medium.com/@babylonjs/using-high-contrast-image-based-lighting-in-babylon-js-627ac721f20

I know Norbert Nopper though strongly disagreed with this approach -- thus he aligns with you. I love the approach Babylon.js advocates though. The reason I love it is that irradiance maps in my opinion are not supposed to capture the influence of strong punctual or directional light source -- you should instead use a strong punctual/direction light source instead. Otherwise you need to move away from SH in your LightProbe tetrahedralization to diffuse cube maps everywhere -- argh.

There are two strong camps on this both of whom have strong opinions -- I had this argument with Norbert. With glTF this lead to them implementing both approaches.

@WestLangley
Copy link
Collaborator

I believe @WestLangley you are talking about high contrast HDR maps. The issue isn't the range, spherical harmonics handle range well, it is the high contrast that is the problem.

@bhouston I used the phrase "high dynamic range", which to me means what I assume you mean by "high contrast".

So you either have to hack the image or hack the spherical harmonics. In either case, SH does not work properly in this use case.

We can support SH3 for other use cases, but I would not use SH3 to model irradiance from an arbitrary HDR radiance map. I would use a small cube map, instead.

That is just my suggestion, but I don't feel that strongly about it.

@WestLangley
Copy link
Collaborator

@bhouston I am talking about the case where the user provides an HDR cube map, and the library computes both PMREM and the irradiance for the user. (I'm not sure if you support that workflow, but it sounds nice to me.)

I agree, if the user wants to provide the irradiance map, there is no reason we can't support irradiance maps in an SH3 or cube map format.

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

Successfully merging this pull request may close these issues.

5 participants