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

SpotLightShadow: Added .focus property #20218

Merged
merged 4 commits into from
Sep 7, 2020

Conversation

WestLangley
Copy link
Collaborator

@WestLangley WestLangley commented Aug 29, 2020

This feature allows the user to set a focused spotlight shadow.

.focus is a value in [ 0, 1 ]. Default is 1.

Everything else just works -- spotlight shadows continue to auto-update, helpers work as expected, and it is backwards-compatible.

I'll update docs, etc., if there is support for this PR.

EDIT: docs updated

@WestLangley
Copy link
Collaborator Author

/ping @RemusMar

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 29, 2020

Nice! So @mrdoob has now the choice between a flag and simple float property :)

One minor disadvantage: Compared to autoFOV (#20147 (comment)), setting shadow.camera.fov would still have no effect.

@WestLangley
Copy link
Collaborator Author

setting shadow.camera.fov would still have no effect

setting shadow.camera.far also has no effect.

setting shadow.camera.near works, because it is not overwritten by the updatMatrices() method.

It is a bit difficult to understand what can be changed -- and what cannot.

@WestLangley
Copy link
Collaborator Author

Consequently, I prefer to consider that the spotlight shadow frustum is auto-set ( that is the intention, after all) , and to provide a focus property that tells the renderer how to auto set it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 29, 2020

It is a bit difficult to understand what can be changed -- and what cannot.

Yep, that's a bit unfortunate. Anyway, your argumentation is definitely valid. TBH, I'm not feeling strong about what solution will be merged. I'm just happy if we are able to avoid the LightShadow export! 😇

@WestLangley
Copy link
Collaborator Author

I'm just happy if we are able to avoid the LightShadow export!

And I'm happy I can focus the shadow. :-)

@RemusMar
Copy link
Contributor

I'm just happy if we are able to avoid the LightShadow export!

Why?

SpotLightShadow: Added .focus property
.focus is a value in [ 0, 1 ]. Default is 1.

👍

@WestLangley
Copy link
Collaborator Author

Updated TS and docs.

Todo: update migration docs, I assume...

@WestLangley WestLangley added this to the r121 milestone Aug 30, 2020
@WestLangley
Copy link
Collaborator Author

Added spotlight shadow .focus to the GUI in the spotlight example.

The demo clearly illustrates how the quality of the shadow is improved by focusing the frustum.

PR requires a rebuild.

@RemusMar
Copy link
Contributor

RemusMar commented Sep 7, 2020

The demo clearly illustrates how the quality of the shadow is improved by focusing the frustum.

There is a significant difference in this example as well:
https://necromanthus.com/Test/html5/testA_disco.html
(spotLight angle at 50, but shadow at 22.5)

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2020

Nice solution!

@mrdoob mrdoob merged commit 0fc4e52 into mrdoob:dev Sep 7, 2020
@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2020

Thanks!

@WestLangley WestLangley deleted the dev_spot_shadow_focus branch September 8, 2020 01:43
@mrdoob
Copy link
Owner

mrdoob commented Sep 8, 2020

Actually, isn't shadow.focus a bit like camera.zoom?
Well, camera.zoom increases to reduce the frustum while shadow.focus decreases... 🤔

@WestLangley
Copy link
Collaborator Author

Yes. Users need to use this pattern, however:

spotLight.shadow.camera.zoom = zoom;
spotLight.shadow.camera.updateProjectionMatrix();

If you prefer, I can update to use .zoom, instead.

@RemusMar
Copy link
Contributor

RemusMar commented Sep 8, 2020

updateProjectionMatrix() is not quite cheap.
I use it here for the right mouse button:
https://necromanthus.com/Test/html5/testA_disco.html
Also, ZOOM for shadows sounds a bit "obfuscated".
I like the FOCUS approach (and I already updated many samples)

@mrdoob
Copy link
Owner

mrdoob commented Sep 8, 2020

Okay, lets go with .focus 👌

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.

None yet

4 participants