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

Modification of LightMap and addition of AOMap to Materials. #6263

Merged
merged 18 commits into from
Mar 26, 2015

Conversation

bhouston
Copy link
Contributor

This PR requires some discussion I believe.

Previously we were treating lightMap as something that modulates the outgoingLight from a surface. This is not the normal definition of a lightMap as treated in other game engines. Usually a lightMap contributes outgoing light from a surface via being modulated with the surface color. Thus I have made this change.

But to accommodate the previous use case of lightMap (where it modulated the outgoingLight), I've added ambientOcclusionMap which does directly modulate the outgoingLight from a surface. I have added an ambientOcclusionScale as well that varies the degree to which the light is modulated by the ambientOcclusionMap - where 0 is no darkening, while 1 is full darkening based on the map. Values greater than 1 are allowed but could lead to non-physical results.

To be specific, the way that ambientOcclusionMap has been implemented is that it modulates the outgoingLight after the main lighting equation, thus it modulates both diffuse and specular and emissive contributions. It is arguable whether this is fully correct, but it does minimic exactly the previous implementation for the outgoingLight modulating lightMap.

With this change ThreeJS treats both lightMaps and ambientOcclusionMaps in a similar fashion to other game engines.

While doing this change I have pulled apart the handling of both UV and UV2 varyings in the fragment and vertex shaders. They are no longer part of map and the lightmap glsl shaders pieces, but rather are the own pieces. This is clearer but these two variables (UV and UV2) are really shared varyings, not tied to a specific map.

/ping @WestLangley

@WestLangley
Copy link
Collaborator

  1. As discussed previously ambient occlusion maps only modulate ambient light. We should not reproduce what we had before, as it was incorrect. This is an easy fix to this PR.
  2. Also, I would suggest naming it aoMap and aoScale rather than the more verbose ambientOcclusionMap and ambientOcclusionScale. Devs will appreciate it.
  3. three.js has treated light maps as a form of "baked shadows" -- and as multiplicative to outgoing light. You appear to think of them as "baked lights" -- another source of lights -- and as additive to incoming light. I will defer to the experts on this, but we need to come to some consensus ASAP.

I'll have a detailed look at this PR as soon as I can.

@bhouston
Copy link
Contributor Author

three.js has treated light maps as a form of "baked shadows" -- and as multiplicative to outgoing light.

This is not what light maps are intended to be used for. Light maps are not baked shadow maps, rather they represent by definition incoming light to a surface. They can represent shadows by the absence of light though.

Here is what the lightmap example looks like with this change:

image

You can see there are still shadows, but these shadows are represented by the absence of light in the light map, rather than the modulation of existing light.

The reason why the modulation of outgoingLight is problematic is that it violates the idea of the separability of light sources. Lighting for the most part has an "associative property", where each light is independent of the others. Light maps are just precomputed lights and they should exhibit the same "associative property." And you apply them such that you can have a real-time light or a baked light and they generally are the same thing (you only loose directionality effects with baked lights.)

IBL is different than light maps. Light maps are baked onto objects with a specific UV mapping. A light map generally is not reused on multiple parts of an object, it is unique per object surface.

An IBL setup is generally an environment, usually spherical that is is queried from the surface normal of an object for its lighting -- thus it is accessed via the surface normal and not a specific UV mapping on the object. If you have multiple IBL light probes, then they are queried based on surface position and surface normal.

You have have LDR and HDR versions of both IBL and lightMaps. IBL is additive as well as it follows the "associative property", but IBL is accessed very differently.

IBL, LightMaps, and AmbientOcclusion are used in UE4 and Unity5.

@bhouston
Copy link
Contributor Author

As discussed previously ambient occlusion maps only modulate ambient light. We should not reproduce what we had before, as it was incorrect. This is an easy fix to this PR.

That is a good idea.

I will use the ambientOcclusion to modify all " indirect diffuse
lighting", thus both ambient and diffuse lighting contributions as described in the FB4 paper here on page 74/75:

http://www.frostbite.com/wp-content/uploads/2014/11/course_notes_moving_frostbite_to_pbr.pdf

Is that okay?

@bhouston bhouston changed the title Modification of LightMap and addition of AmbientOcclusionMap to Materials. Modification of LightMap and addition of AOMap to Materials. Mar 20, 2015
@WestLangley
Copy link
Collaborator

@bhouston

This is excellent. Thank you for the clarification regarding light maps.

Here is how I would implement the aoMap calculation:

totalAmbientLight = ambientLightColor * ( 1.0 + aoScale * ( aoMap - 1.0 ) );

outgoingLight += diffuseColor.rgb * ( totalDiffuseLight + totalAmbientLight ) + totalSpecularLight + emissive;

I would also highly recommend changing the variable names to match terminology that is common in the literature:

directDiffuseLight ( I actually prefer the simpler term directDiffuse )
indirectDiffuseLight ( or indirectDiffuse )

directSpecularLight ( indirectSpecular )
indirectSpecularLight ( not implemented yet )

outgoingLight += diffuseColor.rgb * ( directDiffuseLight + indirectDiffuseLight ) + directSpecularLight + emissive;

Also, in my view, a hemi-light is a model of indirect light -- I know one could argue otherwise. If you agree, then the hemi-light should be included in the indirect lighting calculation, and be modulated by the aoMap. Furthermore, since it has a specular term, that term would be part of indirect specular. This can be handled in a separate PR.

You will have to remove AO from MeshBasicMaterial ( and maybe MeshLambertMaterial -- I need to think about that one... ).

@bhouston
Copy link
Contributor Author

These suggestions, while useful, will make this PR significantly more complex and harder to review.

Why don't we split this into two parts. I'd suggest sticking with this current PR (with any little polish like naming) that has the old-style AmbientOcclusion that affects everything like the old lightMap implementation.

Remember the goal fo this PR was primarily to make lightMaps correct, and I only added ambientOcclusion so that those that wanted the old lightMap mode, could use that instead (even though I know it is still wrong theoretically.)

We can then review the lightMap and AmbinetOcclusion setup. And then we can do another PR that improves AmbientOcclusion to affect only indirect lighting.

Is that okay?

@WestLangley
Copy link
Collaborator

Absolutely!

@bhouston
Copy link
Contributor Author

Here is an issue where we can discuss the separate from indirect diffuse from indirect/direct specular:

#6271

@bhouston
Copy link
Contributor Author

@WestLangley:

You will have to remove AO from MeshBasicMaterial ( and maybe MeshLambertMaterial -- I need to think about that one... ).

I added aoMap to every material that already had lightMap. We can remove them from these two materials or leave them -- I think the overhead is almost nothing to keeping them around when they are not used. I think the oaMap makes as much sense as a lightMap -- so we would suggest either keeping both or removing both on a per material basis.

@WestLangley
Copy link
Collaborator

MeshBasicMaterial does not respond to ambient light, so it does not really make sense to support ambient occlusion maps in that case -- unless you want to hack it in as a special case. That would be OK, I guess. This is the pseudo code for how you would do it:

outgoingLight = diffuseColor.rgb * ( 1.0 + aoScale * ( aoMap - 1.0 ) );

As far as MeshLambertMaterial goes, ambient lighting is calculated in the vertex shader, so there would have to be some code changes.

I would keep Basic and Lambert simple, and not support ambient occlusion maps for these materials -- although it means removing a "feature".

@bhouston
Copy link
Contributor Author

Just to be clear, lightMaps are generally considered to be non-directional light -- you generally bake the global illumination solution into lightMaps because you can not get it from local lighting calculations. And both MeshBasicMaterial and MeshLambertMaterial have light maps and thus that would be included in the oa calculation.

/have a good weekend!

@bhouston
Copy link
Contributor Author

Actually I suspect it is a rare case that lightMaps and oaMaps are used together, even if possible. One will likely just bake oa into the lightMap instead of having two. Thus I will remove oaMap from Lambert (which should be Gouroud) and Basic.

@WestLangley
Copy link
Collaborator

Thus I will remove oaMap from Lambert (which should be Gouroud) and Basic.

Good. It is settled! : - )

@WestLangley
Copy link
Collaborator

Just to be clear, lightMaps are generally considered to be non-directional light -- you generally bake the global illumination solution into lightMaps because you can not get it from local lighting calculations.

That is a great way of thinking about it: "Light maps are baked GI".

@bhouston
Copy link
Contributor Author

I've just removed aoMap/Scale from Basic and Lambert materials. I also added a lightScale parameter to Phong so that one can adjust the lightMap intensity -- I think the name is a little confusing (it sounds like it could scale all lights, but it follows the naming pattern already established for normalMap/normalScale and bumpMap/bumpScale) but the parameter is really useful.

@WestLangley
Copy link
Collaborator

I think the name is a little confusing

That it is. A compromise would be to break the pattern and call it lightMapScale. It is up to you.

Otherwise, this is good-to-go.

@bhouston
Copy link
Contributor Author

I've renamed the variable. :)

if ( data.aoMap !== undefined ) {

material.aoMap = getTexture( data.aoMap );
if ( data.aoScale ) {
Copy link
Owner

Choose a reason for hiding this comment

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

For clarity and robustness I would change this if ( data. aoScale !== undefined ).

"lightMapScale" : { type: "f", value: 1 },

"aoMap" : { type: "t", value: null },
"aoScale" : { type: "f", value: 1 },
Copy link
Owner

Choose a reason for hiding this comment

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

This should be aoMapScale.

@mrdoob
Copy link
Owner

mrdoob commented Mar 25, 2015

I'm not sure about lightMapScale and aoMapScale... I think normalScale and bumpScale are not the same. I think I would go with lightMapOpacity and aoMapOpacity instead.

@WestLangley
Copy link
Collaborator

I think I would go with lightMapOpacity and aoMapOpacity instead.

aoMap modulates ambientLightColor. aoScale is an extra degree of freedom to dial the AO effect.

totalAmbientLight = ambientLightColor * ( 1.0 + aoScale * ( aoMap - 1.0 ) );

It is not an opacity. I think aoScale is an appropriate term.

So is lightMapScale.

@mrdoob
Copy link
Owner

mrdoob commented Mar 25, 2015

aoScale and lightMapScale sounds like it's scaling the dimensions of the map. Maybe *Intensity?

@WestLangley
Copy link
Collaborator

normalScale and bumpScale scale the "intensity" of the normal/bump maps. Likewise, aoScale and lightMapScale scale the intensity of the ao/light maps.

I think we are going to have to pick the better of some not-so-good alternatives...

@bhouston
Copy link
Contributor Author

I addressed @mrdoob's suggestions. I choose aoMapIntensity and lightMapIntensity for no particular reason. I am agnostic on the specific naming of these two variables.

@@ -65,6 +69,10 @@ THREE.MeshPhongMaterial = function ( parameters ) {
this.map = null;

this.lightMap = null;
this.lightMapIntensity = null;
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be 1.0?

@bhouston
Copy link
Contributor Author

BTW I also sorted the shaders alphabetically in utils/build/includes/common.json. Just in case you are wondering why every shader line is modified:

https://github.com/bhouston/three.js/blob/ambientOcclusionMap/utils/build/includes/common.json#L87

mrdoob added a commit that referenced this pull request Mar 26, 2015
Modification of LightMap and addition of AOMap to Materials.
@mrdoob mrdoob merged commit 3d85d2c into mrdoob:dev Mar 26, 2015
@mrdoob
Copy link
Owner

mrdoob commented Mar 26, 2015

Nice! Thanks!

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.

3 participants