three.js icon indicating copy to clipboard operation
three.js copied to clipboard

Lights: Add shadowLimit property to PointLightShadow and SpotLightShadow

Open kalegd opened this issue 2 years ago • 8 comments

Fixed #27290

Description

Setting the distance of PointLight/SpotLight affect the PointLightShadow/SpotLightShadow's camera.far when the distance is set to 0. If I set the distance to 5 and then later to 0, the camera.far of the Shadow is still set to 5. This change introduces a shadowLimit property to the Shadow class that will be used to set the value of camera.far when the corresponding light object is 0

kalegd avatar Dec 09 '23 00:12 kalegd

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669 kB (166.1 kB) 669.1 kB (166.1 kB) +64 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
449.9 kB (109 kB) 449.9 kB (109 kB) +0 B

github-actions[bot] avatar Dec 09 '23 00:12 github-actions[bot]

I hesitated a bit with approving the PR since we have always stated users can directly modify the shadow camera's frustum. However, when we started to evaluate the distance (a light property) in the light shadow classes, things started to get confusing. Given the current state, I think this solution is okay.

The alternative would be to completely remove the distance evaluation from the light shadow classes. So devs always have to configure the far property of the shadow camera and distance only affects the light, not shadows anymore. I like the resulting simplicity of this approach but I'm unsure about the consequences. Using point and spot lights with their (physically correct) default distance value won't be affected by this change.

Mugen87 avatar Dec 11 '23 10:12 Mugen87

Understandable. I'll leave that decision to you and the rest of the maintainers of this project. Totally fine with scrapping this PR if we end up going down another route

kalegd avatar Dec 11 '23 16:12 kalegd

see https://github.com/mrdoob/three.js/pull/27722#issuecomment-1953915716

Mugen87 avatar Feb 20 '24 10:02 Mugen87

I can document the new property when we agree on this design. After trying out different approaches, it's the best solution for https://github.com/mrdoob/three.js/issues/27290 , imo.

Mugen87 avatar Feb 20 '24 10:02 Mugen87

Hmm... I'll look into this one tomorrow.

mrdoob avatar Feb 20 '24 13:02 mrdoob

I am inclined to think this approach gets away from the original intent of making the lights "just work".

See the discussion here.

WestLangley avatar Feb 20 '24 19:02 WestLangley

@WestLangley What alternative approach would you suggest? Something like https://github.com/mrdoob/three.js/issues/27290#issuecomment-1839697678?

Mugen87 avatar Feb 20 '24 20:02 Mugen87