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

CSM: Directional light without a shadow breaks shaders #23631

Merged
merged 2 commits into from
Mar 12, 2022

Conversation

OndrejSpanel
Copy link
Contributor

When you add another directional light without a shadow into the CSM example. it breaks, shaders no longer compile and example does not display any objects.

Solution being prepared, stay tuned.

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented Mar 2, 2022

Solution: directional lights without shadows need a dedicated loop, the shadow casting loop traverses only until NUM_DIR_LIGHT_SHADOWS, not until NUM_DIR_LIGHTS.

@OndrejSpanel OndrejSpanel marked this pull request as ready for review March 2, 2022 15:05
@@ -97,6 +97,10 @@
const ambientLight = new THREE.AmbientLight( 0xffffff, 0.5 );
scene.add( ambientLight );

const additionalDirectionalLight = new THREE.DirectionalLight( 0x000020, 0.5 );
additionalDirectionalLight.position.set( params.lightX, params.lightY, params.lightZ ).normalize().multiplyScalar( - 200 );
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: Indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed.

@mrdoob mrdoob added this to the r139 milestone Mar 2, 2022
@OndrejSpanel OndrejSpanel force-pushed the csm-additional-light branch from 3dd4d64 to 2fd0a03 Compare March 3, 2022 13:30
@mrdoob
Copy link
Owner

mrdoob commented Mar 8, 2022

@gkjohnson Do you mind double checking this PR?

@gkjohnson
Copy link
Collaborator

I'll take a deeper look in a bit but in the meantime here's a link for testing:

https://raw.githack.com/OndrejSpanel/three.js/csm-additional-light/examples/webgl_shadowmap_csm.html

Comment on lines 152 to 165
#if ( NUM_DIR_LIGHTS > NUM_DIR_LIGHT_SHADOWS)
// compute the lights not casting shadows (if any)

#pragma unroll_loop_start
for ( int i = NUM_DIR_LIGHT_SHADOWS; i < NUM_DIR_LIGHTS; i ++ ) {

directionalLight = directionalLights[ i ];

getDirectionalLightInfo( directionalLight, geometry, directLight );

RE_Direct( directLight, geometry, material, reflectedLight );

}
#pragma unroll_loop_end
Copy link
Collaborator

Choose a reason for hiding this comment

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

The indentation looks a little wonky here but other than that it looks good.

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 can fix that. Normally automated check catches this (caused by different Tab / Space settings in my editor), but not inside of string literals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now.

@OndrejSpanel OndrejSpanel force-pushed the csm-additional-light branch from 2fd0a03 to 2180434 Compare March 10, 2022 18:01
@OndrejSpanel OndrejSpanel force-pushed the csm-additional-light branch from 2180434 to 16f0f71 Compare March 10, 2022 18:08
@mrdoob mrdoob merged commit 4082a14 into mrdoob:dev Mar 12, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 12, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Demonstrate issue - directional light without a shadow breaks shaders

* Fix: light not casting shadows need to be processed separately in the CSM shader
@yfunk
Copy link
Contributor

yfunk commented Nov 2, 2023

I'm a bit late to this, but it seems that after the changes in this PR, the CSM directional lights are no longer calculated at all when shadow maps are disabled (renderer.shadowMap.enabled = false). Is this intentional?

I've modified the CSM example to add a toggle for shadow maps, so you can compare the result before this PR and after. Previously, turning shadow maps on or off didn't affect the lighting in the scene, only the shadows themselves. Now the whole scene becomes noticeably darker when shadow maps are turned off.

(@OndrejSpanel, @gkjohnson)

float dist = min( linearDepth - csmx, csmy - linearDepth );
float ratio = clamp( dist / margin, 0.0, 1.0 );
if( UNROLLED_LOOP_INDEX < NUM_DIR_LIGHT_SHADOWS ) {
#if defined( USE_SHADOWMAP ) && ( UNROLLED_LOOP_INDEX < NUM_DIR_LIGHT_SHADOWS )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Evaluating USE_SHADOWMAP at this place is redundant since there is a #if defined( USE_SHADOWMAP ) in line 79 that already checks it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be fair this one is easy to miss since the code isn't properly formatted. There should be an indentation right after the check in line 79.

Sidenote: In recent version of CSMShader the line numbers have changed a bit. The issue is still present though.

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.

6 participants