Skip to content

Lights: Add shadowLimit property to PointLightShadow and SpotLightShadow #27345

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

kalegd
Copy link
Contributor

@kalegd kalegd commented Dec 9, 2023

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

Copy link

github-actions bot commented Dec 9, 2023

📦 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

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 11, 2023

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.

@kalegd
Copy link
Contributor Author

kalegd commented Dec 11, 2023

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

@Mugen87 Mugen87 added this to the r162 milestone Feb 20, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 20, 2024

see #27722 (comment)

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 20, 2024

I can document the new property when we agree on this design. After trying out different approaches, it's the best solution for #27290 , imo.

@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2024

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

@WestLangley
Copy link
Collaborator

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

See the discussion here.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 20, 2024

@WestLangley What alternative approach would you suggest? Something like #27290 (comment)?

@mrdoob mrdoob modified the milestones: r162, r163 Feb 29, 2024
@mrdoob mrdoob modified the milestones: r163, r164 Mar 29, 2024
@mrdoob mrdoob modified the milestones: r164, r165 Apr 25, 2024
@mrdoob mrdoob modified the milestones: r165, r166 May 31, 2024
@mrdoob mrdoob modified the milestones: r166, r167 Jun 28, 2024
@mrdoob mrdoob modified the milestones: r167, r168 Jul 25, 2024
@mrdoob mrdoob modified the milestones: r168, r169 Aug 30, 2024
@mrdoob mrdoob modified the milestones: r169, r170 Sep 26, 2024
@mrdoob mrdoob modified the milestones: r170, r171 Oct 31, 2024
@mrdoob mrdoob modified the milestones: r171, r172 Nov 29, 2024
@mrdoob mrdoob modified the milestones: r172, r173 Dec 31, 2024
@mrdoob mrdoob modified the milestones: r173, r174 Jan 31, 2025
@mrdoob mrdoob modified the milestones: r174, r175 Feb 27, 2025
@mrdoob mrdoob modified the milestones: r175, r176 Mar 28, 2025
@mrdoob mrdoob modified the milestones: r176, r177 Apr 24, 2025
@mrdoob mrdoob modified the milestones: r177, r178 May 30, 2025
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.

SpotLight map affected by distance updates
4 participants