Skip to content

Conversation

@bunnybones1
Copy link
Contributor

@bunnybones1 bunnybones1 commented Jan 10, 2019

I changed the indirect lighting on the Lambert material so that shadows aren't pitch black when using hemispherical light. I accomplished this with an extra varying for ambient light, but only if using shadows and a hemisphere light. Supports double-sided materials.
new-lambert

Fixes #15530.

@mrdoob mrdoob added this to the r101 milestone Jan 10, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jan 10, 2019

Nice! Do you mind removing the builds from the PR though?

vLightFront = vec3( 0.0 );
#if defined (USE_SHADOWMAP) && NUM_HEMI_LIGHTS > 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need to condition on the presence of shadows. Keep it simple:

vLightFront = vec3( 0.0 );
vIndirectFront = vec3( 0.0 );

#ifdef DOUBLE_SIDED
	vLightBack = vec3( 0.0 );
	vIndirectBack = vec3( 0.0 );
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like calling the light indirect rather than ambient.
As for the conditions, it's to prevent most uses of Lambert from doing the extra varying color, and also to prevent adding the light colors in the fragment shader. You're sure the simplicity trumps the small performance difference? I'm keen to agree, but just want to make sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can optimize for performance later if there is a performance issue. I doubt there will be.

for ( int i = 0; i < NUM_HEMI_LIGHTS; i ++ ) {
vLightFront += getHemisphereLightIrradiance( hemisphereLights[ i ], geometry );
#ifdef USE_SHADOWMAP
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to condition on shadows

for ( int i = 0; i < NUM_HEMI_LIGHTS; i ++ ) {

	vIndirectFront += getHemisphereLightIrradiance( hemisphereLights[ i ], geometry );

	#ifdef DOUBLE_SIDED

		vIndirectBack += getHemisphereLightIrradiance( hemisphereLights[ i ], backGeometry );

	#endif

}

#define LAMBERT
varying vec3 vLightFront;
#if defined(USE_SHADOWMAP) && NUM_HEMI_LIGHTS > 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the conditional.

// accumulation
reflectedLight.indirectDiffuse = getAmbientLightIrradiance( ambientLightColor );
#if defined(USE_SHADOWMAP) && NUM_HEMI_LIGHTS > 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like this:

// accumulation
reflectedLight.indirectDiffuse = getAmbientLightIrradiance( ambientLightColor );

#include <lightmap_fragment>

#ifdef DOUBLE_SIDED

	reflectedLight.indirectDiffuse += ( gl_FrontFacing ) ? vIndirectFront : vIndirectBack;

	reflectedLight.directDiffuse = ( gl_FrontFacing ) ? vLightFront : vLightBack;

#else

	reflectedLight.indirectDiffuse += vIndirectFront;

	reflectedLight.directDiffuse = vLightFront;

#endif

reflectedLight.indirectDiffuse *= BRDF_Diffuse_Lambert( diffuseColor.rgb );

reflectedLight.directDiffuse *= BRDF_Diffuse_Lambert( diffuseColor.rgb ) * getShadowMask();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just want to clarify that the reason for the shadow and hemisphere light check are because its only when we use these features together that we need to move the final light color calculation to the fragment shader. Otherwise it's fine to precalculate the final light color entirely in the vertex shader. Let me know if you'd prefer the simplicity wins over the small performance save and I'll submit a simplified commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to model the lighting correctly, and keep indirect separate from direct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok cool, I'll have the modified PR ready tomorrow morning. Thanks!

@WestLangley
Copy link
Collaborator

@bunnybones1 Thanks for this. You are correct, this needs to be fixed, but the basic lighting equations should not be changed just because shadows are present. See #15530 (comment).

Please see if my suggestions look correct to you.

@bunnybones1 bunnybones1 force-pushed the features/fix-lambert-hemisphere-shadows branch from 6a01cf2 to bdf5fc2 Compare January 11, 2019 02:14
@bunnybones1
Copy link
Contributor Author

bunnybones1 commented Jan 11, 2019

Nice! Do you mind removing the builds from the PR though?

Done! I'll remove the conditions as @WestLangley suggested and we should be good to go.

@bunnybones1
Copy link
Contributor Author

Should be good to go!

@mrdoob mrdoob merged commit f9c5efd into mrdoob:dev Jan 31, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jan 31, 2019

Thanks!

@bunnybones1 bunnybones1 deleted the features/fix-lambert-hemisphere-shadows branch March 16, 2019 18:09
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