Respect physicallyCorrectLights on lightmaps in MeshBasicMaterial

[netpro2k]

Related issue: #21912

Description

MeshBasicMaterial allows the use of lightmaps but these lightmaps do not respect the physicallyCorrectLights renderer setting. This makes it difficult to combine MeshBasicMaterial with MeshStandardMaterial in the same scene.

[netpro2k]

Note that in lightmap_fragment.glsl.js we have the inverse behavior, multiplying the value by PI if PHYSICALLY_CORRECT_LIGHTS is not defined. This is to cancel out the effect of BRDF_Lambert(diffuseColor) which is applied by default in most other materials (which multiplies the diffuseColro by RECIPROCAL_PI). MeshBasicMaterial does not apply BRDF_Lambert to the diffuseColor so we need to do the inverse for the lightmap to be "physically correct"

We probably do not want to change how a MeshBasicMaterial with no lightmap behaves so we instead apply this here, only to lightmaps, and only when PHYSICALLY_CORRECT_LIGHTS is defined. We could change the surrounding code a bit such that we could #include <lightmpmap_fragment> unchanged but it would slightly complicate the common case of a MeshBasicMaterial with no lightmap.

[mrdoob]

@WestLangley Looks good to you?

[WestLangley]

I'd like to think about this...

I was the one that re-added MeshBasicMaterial support for light maps. #22393 In hindsight, I wish I hadn't.

Subsequent to that PR, so-called "physically-correct" lights were added, which added a further complication.

@netpro2k Just asking, but is AOMap, lightMap support important to you in this case? Does it make sense to consider removing it, or will that cause a backlash?

//

On a related note, I had hoped to get the factor of PI out of the shaders. MeshBasicMaterial is, in fact, the stumbling block. See #22397.

I would like to see this logic moved out of the shaders and into the renderer.

[netpro2k]

is AOMap, lightMap support important to you in this case

In Hubs we "gracefully degrade" all models down to MeshBasicMaterial on mobile clients. Since these materials start out as MeshStandardMaterials they often have lightmaps+AOMaps, so things look much better if we also include those on mobile. We already extend MeshBasicMaterial in Hubs to add support for emissiveMap for similar reasons, so I don't have super strong opinions on if it remains in core (we can easily just add them back to our custom material), but I would imagine others might find it useful for the same reasons we do. Maybe there should be something like a MeshIBLMaterial that has no realtime lighting but has lightmap+ao+emissive+PMREM. (basically what is being discussed in #21578)

Solong as lightMap does exist though I think it should be applied consistently across materials.

[netpro2k]

Also note, as per the discussion in #21912 in Hubs we are likely going to effectively be removing the effect physicallyCorrectLights has on lightmaps (sort of negating this patch, but also applying an inverse patch to MeshStandardMaterial), since what we are getting out of Blender (our primary workflow for lightmpapping) already seems to have PI factored out, so artists end up having to set their lightmapIntensity to PI to compensate.

[WestLangley]

we are likely going to effectively be removing the effect physicallyCorrectLights has on lightmaps (sort of negating this patch...)

That means you are going to negate this PR in your own fork if were to be merged?

[WestLangley]

@mrdoob Just to review...

1. MeshBasicMaterial does not respond to AmbientLight, HemisphereLight , or LightProbe -- all ambient indirect light sources -- but it does respond to LightMap, which represents ... ambient light...

2. MeshBasicMaterial supports AOMap, which occludes ambient light -- the very light the material does not support....

This was all done to be backwards-compatible with the earliest versions of three.js.

MeshBasicMaterial also supports environment maps, but not emissive maps.

There is nothing "physically correct" here.

//

@netpro2k I am open to making a change -- maybe even this PR or something similar.

Unfortunately, AFAIK, no blender dev has articulated what the exported "light maps" represent -- or their units. I doubt the exported maps are compatible with the three.js light map model. I certainly would not recommend a change in three.js to accommodate blender at this point.

[mrdoob]

@WestLangley

I would like to see this logic moved out of the shaders and into the renderer.

That sounds good. Should we create a new Issue to tackle this?

[mrdoob]

@WestLangley

MeshBasicMaterial also supports environment maps, but not emissive maps.

I wouldn't mind adding emissive maps to MeshBasicMaterial actually. I can see them being pretty useful and easy to add/maintain.

[memelotsqui]

Hi!, as a personal opinion, I believe lightmaps should work the same way as they work right now with non physically correct lights.

(Please correct me if Im wrong in something, this is based on personal toughts :) ). Since lightmaps are prebaked, physical correct lighting shouldnt be affecting the lightmap texture, they should be affecting the result of the combined lightmap with diffuse color (that is after multiplying by PI). Not having the lightmap multiplied by PI leads to a darker lightmap (this is happening to me too).

Im using right now a Unity to three js exporter that supports lightmaps, and this is the difference when using physical correct lightmap vs non physical correct lightmap, as you mentioned, everything looks darker, until you multiply it by PI

Non physical correct lighting - standard material

Physical correct lighting - standard material

Unity Scene:

The fix would be to always multiply the lightMapIrradiance by PI regardless of if physicallyCorrectLights is enabled or not, correct?

I would actually prefer this solution from #21912, rather than dividing meshBasicMaterial by PI, but either way it should have consistency to avoid differences between Mesh Standard Material and Mesh Basic Material :)