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.cameraAutoUpdate / Light.shadowAutoUpdate #14663

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

mbredif
Copy link
Contributor

@mbredif mbredif commented Aug 8, 2018

Preventing the Autoupdating of the SpotLight.shadow.camera was previously performed through the hack of overwriting the SpotLightShadow with a LightShadow.

Instead, this PR proposes a new property SpotLightShadow.cameraAutoUpdateLight.shadowAutoUpdate that makes optional the fitting of the shadow camera frustum to the square cone defined by light.angle/light.distance.

(this PR is the item 2 of #14658, extracted here for an easier review)

== Edit ==
Instead of overwriting the shadow, an alternative hack to get the same result would be to set this.shadow.isSpotLightShadow = false. As that was the sole use of isSpotLightShadow, this PR is essentially moving and renaming Light.shadow.isSpotLightShadow to Light.shadowAutoUpdate.

@mbredif mbredif force-pushed the SpotLightCameraUpdateOptOut branch from 9476109 to 2cbd289 Compare August 8, 2018 10:37
@@ -20,6 +21,8 @@ SpotLightShadow.prototype = Object.assign( Object.create( LightShadow.prototype

update: function ( light ) {

if ( ! this.cameraAutoUpdate ) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead, I would do the following in the calling method:

if ( shadow.isSpotLightShadow && shadow.cameraAutoUpdate ) {

	shadow.update( light );

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@WestLangley
Copy link
Collaborator

I definitely support this.

I also support removal of the need for this pattern:

light.shadow = new THREE.LightShadow( ... );

Forcing the shadow frustum to auto-size to the cone can have the side-effect of causing pixelated shadows.

@mbredif
Copy link
Contributor Author

mbredif commented Aug 8, 2018

2 more commits :

  • I found 2 more uses of the light.shadow = new THREE.LightShadow( ... ); pattern in the examples.
  • I moved the cameraAutoUpdate check to the calling site, as suggested in the review.

@mbredif
Copy link
Contributor Author

mbredif commented Aug 8, 2018

I am not sure about 2da8234 though, at it breaks http://localhost:8000/examples/webgl_lights_spotlight.html due to

spotLight.shadow.update(spotLight);
which is now ungarded (it is required to have the spotlight and camera helpers in sync)

@mbredif
Copy link
Contributor Author

mbredif commented Aug 9, 2018

@WestLangley what do you think of this new commit 64b191c ?
I moved SpotLightShadow.cameraAutoUpdate to Light.shadowAutoUpdate, updated the docs, and added the (conditional) light shadow update to the SpotLight helper. I also remove the now unused isSpotLightShadow.

@mbredif mbredif force-pushed the SpotLightCameraUpdateOptOut branch 2 times, most recently from 7f3ce22 to 9a706ef Compare August 9, 2018 15:40
<h3>[property:Boolean shadowAutoUpdate]</h3>
<p>
If set to *true*, the shadow camera will be automatically updated each frame to match the
geometry of the spotLight (a square frustum tightly fitting the cone of the spot light defined
Copy link
Collaborator

Choose a reason for hiding this comment

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

If set to *true*, the shadow camera frustum will be automatically sized to match the cone of the spotLight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -175,6 +175,13 @@ <h3>[property:Object3D target]</h3>
The spotlight will now track the target object.
</p>

<h3>[property:Boolean shadowAutoUpdate]</h3>
Copy link
Collaborator

Choose a reason for hiding this comment

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

[property:Boolean autoSizeShadowFrustum] ?

@@ -134,6 +134,11 @@ <h3>[method:Object toJSON]()</h3>
Serialize this LightShadow.
</p>

<h3>[method:void update]( [param:Light light] )</h3>
<p>
Updates this LightShadow (e.g. [page:SpotLightShadow.update SpotLightShadow.update]).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method is akin to a virtual base class and does not do anything. Define what it does where it is overloaded.

@WestLangley
Copy link
Collaborator

Actually, I think some others need to chime in regarding both the feature and the implementation details.

You are not "auto-updating" the shadow camera frustum, you are "auto-sizing" it. At least that is how I see it.

@mbredif
Copy link
Contributor Author

mbredif commented Aug 10, 2018

The naming shadowAutoUpdate refers to what it does for all lights (it guards a call to shadow.update, similarly to how matrixAutoUpdate works for Object3D.matrix) rather than the previous naming cameraAutoUpdate or autoSizeShadowFrustum which refer to what it does for Spotlights only.

It is thus more generic and makes SpotLightShadow.isSpotLightShadow unnecessary. Furthermore, it can be used to add custom shadow update behavior for any Light.

@mbredif mbredif changed the title SpotLightShadow.cameraAutoUpdate SpotLightShadow.cameraAutoUpdate / Light.shadowAutoUpdate Aug 10, 2018
@mbredif
Copy link
Contributor Author

mbredif commented Oct 4, 2018

Up !

There is no new functionality here. The goal is to provide a better API for the hack of overwriting the SpotLightShadow with a LightShadow to prevent the lightshadow update.

The alternatives are :
A- renaming LightShadow.isSpotLightShadow to LightShadow.autoUpdate
B- moving LightShadow.isSpotLightShadow to Light.shadowAutoUpdate
C- use a more specific naming like autoSizeShadowFrustum or cameraAutoUpdate, but that opens the door for users to use the LightShadow.update function as a generic custom callback, guarded by a flag with a specific naming.

(Complementarily the autoUpdate approach, a needsUpdate approach could be implemented as well for a 1-time updating dirty flag.)

This PR implements B without the needsUpdate approach. What API would you prefer @WestLangley @mrdoob ?

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.

2 participants