Core: Remove artist-friendly factor of PI from LightMap shader

[WestLangley]

Follow-on to #22393 and #22397.

Related to #21912.

This PR is an alternate implementation to #23197.

The objective of this PR is two-fold:

1. To move the artist-friendly factor of PI for light maps into the renderer, and out of the shader
2. To modify the behavior of light maps for MeshBasicMaterial so it is more consistent with the other materials.

With the exception of MeshBasicMaterial, the built-in materials supporting light maps should continue to render the same as they did previously.

@netpro2k @memelotsqui Please have a look. Is this doing what you what?

[WestLangley]

@Mugen87 Perhaps a refactoring is in order... but let's first see if the behavior is acceptable. 😇

[WestLangley]

@Mugen87 webgl_materials_lightmap.html uses a custom shader. Maybe replace it with an example using MeshBasicMaterial?

[Mugen87]

@Mugen87 Perhaps a refactoring is in order.

One thing I would do is to pass a reference of the renderer to WebGLMaterials. Meaning your create the instance in initGLContext() like so:

materials = new WebGLMaterials( _this, properties );

You can then directly access physicallyCorrectLights in WebGLMaterials which requires less changes compared to now.

[WestLangley]

@Mugen87 done! :-)

[Mugen87]

webgl_materials_lightmap.html uses a custom shader. Maybe replace it with an example using MeshBasicMaterial?

It only uses the custom shader for the skydom. I think that's okay.

[mrdoob]

Thanks!

[Mugen87]

With the exception of MeshBasicMaterial, the built-in materials supporting light maps should continue to render the same as they did previously.

@WestLangley How would you formulate a note for the migration guide regarding MeshBasicMaterial?

[memelotsqui]

Hello @WestLangley, I think Im a bit late but still Im going to post the comment :)

I tested the code but it still darkens the lightmaps on meshBasicMaterials, I believe it should only be affecting when lights can affect the material (lambert above)

right now this happens to mesh basic material with defined lightmap when I turn on physically correct lights off (left) and on (right)

To cancel the effect I changed the line of code in meshbasic.glsl.js:

reflectedLight.indirectDiffuse += lightMapTexel.rgb * lightMapIntensity * RECIPROCAL_PI;

to:

reflectedLight.indirectDiffuse += lightMapTexel.rgb * lightMapIntensity;
#ifdef PHYSICALLY_CORRECT_LIGHTS
	reflectedLight.indirectDiffuse *= PI;
#endif

And leave the rest of the pr as it is, this way, meshBasicMaterial lightmap intensity will always be multiplied by PI, even when physicallyCorrectLights is defined, here is the final comparison

Thanks for the follow up on the topic! :)

[WestLangley]

@memelotsqui There seems to be different views on what the behavior should be... Let's wait to see what @netpro2k has to say...

[WestLangley]

@WestLangley How would you formulate a note for the migration guide regarding MeshBasicMaterial?

@Mugen87 The implementation of this legacy feature is arbitrary. Let's see how this plays out. I'll keep your question in mind...

[netpro2k]

For us in Hubs on r133, with physicallyCorrectLights always on, coming out of Blender we currently have to set our lightmapIntensity to PI (but this blows out the lightmap on MeshBasicMaterial).

This PR (and #23197) fix the inconsistency between MeshStandardMaterial and MeshBasicMaterial which seems correct to me. As discussed previously, its a bit weird that MeshBasicMaterial has support for lightmaps in the first place, but since it does I would think it should be effected in the same way by physicallyCorrectLights.

If I am not mistaken for our usecase even with this PR (or #23197) we will still need to set our lightmap intensity to PI for the lightmaps we are getting out of Blender (and apparently Unity according to #23197 (comment)) but that seems sane enough to do in our client code or three fork if needed.

If I am understanding the problem correctly, the reason we still have to do this is that what we are generating in our lightmaps is not actually the incoming light but the diffuse light reflecting off the surface (without its color #21912 (comment)), so we end up with lightmaps that are already multiplied by 1/PI, these then get multiplied by 1/PI again by BRDF_Lambert. We set our lightmapIntensity to PI to cancel out this second multiplication by 1/PI.

I do want to know what toolchain people are actually using though that results in lightmaps that will work correctly with a lightmapIntensity of 1. If there isn't a good path for this, it might make sense to have the default be to consider the lightmap data to already have 1/PI baked in, and document + act accordingly.

Pretty sure everything I said above is correct, but I will try and test this change with some of our assets soon and reply if anything is off. Thanks for iterating on this with us.

[WestLangley]

its a bit weird that MeshBasicMaterial has support for lightmaps in the first place

Right. It shouldn't. A light map is just per-uv ambient light. But MeshBasicMaterial doesn't respond to ambient light. So why does the material support light maps? (The answer is: it is a legacy thing.)

As I have said previously, what makes sense to me is to remove support for light maps from MeshBasicMaterial. Users can use MeshLamberMaterial, instead.

[memelotsqui]

I think I might have diverged from the main issue :O, but testing it again, it does now have consistency when going from standard material to mesh basic when multiplying meshBasic by Reciprocal_PI :), as mentioned by netpro2k lightmap was being blown out before with physicalCorrectLights turned on.

If I am not mistaken for our usecase even with this PR (or #23197) we will still need to set our lightmap intensity to PI

Totally, I will be multiplying by PI to when physicallyCorrectLights are turned on

I do want to know what toolchain people are actually using though that results in lightmaps that will work correctly with a lightmapIntensity of 1

Based on what ive seen (I might be wrong), If we want to have a lightmap working correctly with a value of 1, we would need to use HDR lightmaps instead of LDR (but thats totally not a good idea due to hdr image size). Since lightmaps are multiplied to the main color and LDR cant go above value of 1, we would only be adding shadows, if we multiply it by PI, we will actually have a maximum value of light of PI in the lightmaps :) (at least thats what I think, Im not sure if Im correct here)

As I have said previously, what makes sense to me is to remove support for light maps from MeshBasicMaterial. Users can use MeshLamberMaterial, instead.

I think its a cool really cheap material that fakes illumination in no light scenes :), Im not sure how much difference there is in performance between meshBasic and meshLambert, but it would be nice to keep lightmaps on meshBasic :D

Thanks for the follow up and clarifications :D

[netpro2k]

If we want to have a lightmap working correctly with a value of 1, we would need to use HDR lightmaps instead of LDR

We are actually already doing this in Hubs. They are certainly larger but the tradeoff can be made on a case by case basis by artists creating content. In many cases it is worth trading resolution for dynamic range for a given GPU memory footprint. We also use HDR environment maps, though those tend to be very low resolution.

The issue for us is the lightmaps we are generating in Blender seem to already have their values multiplied by 1/PI (just as a result of the final surface bounce in path tracing). So when using in THREE with physicalCorrectLights turned on they will appear darker than expected (since BRDF_Lambert again multiplies the value by 1/PI). To negate this second multiplication we set our lightmap intensity to PI.

Like I said, I am fine doing this in our client code or branch, but I would still like to know what workflow we actually want people to be using to not have to work around it like this. It might be we just need to make workflow adjustments instead.

[WestLangley]

@netpro2k

1. In three.js, a light map of 1 everywhere, with intensity 1, should render the same as an 0xffffff ambient light of intensity 1. Is Blender consistent in that regard?

2. How do you feel about Core: Remove artist-friendly factor of PI from LightMap shader #23613 (comment)?

[netpro2k]

1. In three.js, a light map of 1 everywhere, with intensity 1, should render the same as an 0xffffff ambient light of intensity 1. Is Blender consistent in that regard?

Not sure exactly how to mirror this... If I set the world background "strength" to 1 (no clue what unit this is in), with 0xffffff color, and bake a "lightmap", the values in the lightmap for that object are around 1.0 (range of ~0.99983 - 1.0002 probably just due to raytrace sampling) so this seems consistent with example you are proposing.

In Blender this object renders completely white, matching the background:

If I bring this object into Hubs with that HDR lightmap applied with intensity 1, no tonemapping, white scene background, no environment map, and physicallyCorrectLights on, it looks like this:

With the same settings but lightmap intensity set to PI it looks like this:

And to sanity check the material in the output GLTF:

    "materials" : [
        {
            "doubleSided" : true,
            "extensions" : {
                "MOZ_lightmap" : {
                    "intensity" : 1,
                    "index" : 0,
                    "texCoord" : 1
                }
            },
            "name" : "Material",
            "pbrMetallicRoughness" : {
                "metallicFactor" : 0
            }
        }
    ],

How do you feel about #23613 (comment)?

Since we use MeshBasicMaterial as a sort of graceful degradation on mobile we would likely keep some form of lightamp support for it in our fork/cleint code, but we are already extending it in our code anyway to add emissive map support (https://github.com/mozilla/hubs/blob/master/src/utils/material-utils.js#L102), so I don't have any strong opinion on lightamp support for MeshBasicMaterial remaining in core. My gut is it can be removed to avoid further confusion.

[netpro2k]

@WestLangley forgot to tag you in the above. Bumping for visibility so this thread doesn't get lost since its already a merged PR.

[WestLangley]

I don't have any strong opinion on light map support for MeshBasicMaterial remaining in core. My gut is it can be removed to avoid further confusion.

I do not like to remove 'features', but in hindsight, I do not think support for light maps should have been added to MeshBasicMaterial in the first place. The implementation is arbitrary, confusing, and a hack.

---

EDIT: Resolved: MeshBasicMaterial will continue to support light maps. See #21912 (comment). In hindsight, I think that is OK.

[WestLangley]

@netpro2k See this thread #6263 (comment).

It will help clarify some terminology, and may explain the differences between three.js and blender.