Make Light targets optional

[mbredif]

(This PR is the geometric counterpart of #14621)

This enables the use of custom user-provided OrthographicCameras for DirectionalLights and PerspectiveCameras for SpotLights, by making optional the 2 hardwired updates that were updating the cameras at each frame :

1. Light.target was unconditionnaly tracking an Object3D using its matrixWorld
2. SpotLightShadow.update was modifying the shadowCamera to have (most notably) its fov fit its cone angle.

Possible use-cases are, combined with #14621, to do image-based rendering by backprojecting images as textured lights, with photogrammetrically-estimated camera parameters, or to more simply implement a slide or video projector with user-provided intrinsics/extrinsics (focal, aspect, position, rotation...).

Proposals :

1. make target tracking opt-in : the default is undefined (no tracking). examples have been modified to either make a light.lookAt or assign a target object
2. make cone fitting opt-out : SpotLightShadow.cameraAutoUpdate is introduced (default: true) to disable the cone fitting of the camera.

WDYT?

--
Edit : item 2 moved to its own PR (#14663)

[looeee]

In simpler terms, after this PR there would be two ways to change the direction of these lights:

• for once off redirection, use light.lookAt( vector3 )

• for per frame redirection use light.target = object3D. In this case the target needs to be added the scene graph, just like the current situation.

I think that this would be an improvement over the current situation which causes a lot of confusion. On the other hand, at least for DirectionalLight I prefer #5079 (remove target completely and use rotation to set light orientation).

In the case of #5079 a lookAt method would still be desirable though.

[mbredif]

PR updated : item 2 moved to its own PR #14663.

Thanks @looeee for the pointers, I was not aware that the removal of the target feature had been debated ! So my proposal is a smoother breaking change than complete removal : instead of removing the feature, it is only disabled by default (the user has to create the target object to enable it).

for once off redirection, use light.lookAt( vector3 )

If I got it right, it is more than that : the shadow camera will get exactly the matrixWorld of the light object and the light direction is extracted from this matrixWorld (as it is already done currently for the position). Thus setting it with lookAt is only one option, the user may manipulate the light object as any object3D (position, rotation...), similar to how a regular camera may be moved.

[looeee]

@mbredif see also #13373 and my comment there. One compelling reason for removing the target completely is that three.js is the odd one out - nearly every other app doesn't have targets for their directional lights. I'm not sure what the situation is for spot lights.

Your solution look like it may give us the best of both worlds though, we can set the light's rotation directly as other apps do and have a target if we want it.

[mbredif]

src

I have reworked the approach and I am now quite satisfied with having light.matrixWorld as the single point of contact for the light geometry :

• shadow cameras are no longer added to the scene, they just copy light.matrixWorld and apply further transforms (inversion, camera convention, projectionMatrix...) to get the shadow matrix
• the world light position should be accessed everywhere using position.setFromMatrixPosition( light.matrixWorld );
• the world light direction should be accessed everywhere using light.getWorldDirection( direction ).negate();
• SpotLight and DirectionnalLight are now rotated, so that there is no need to rotate their helpers.

examples

I have tried to update all examples, by :

• creating the target : light.target = new THREE.Object3D();
• or doing a lookAt : light.lookAt( 0, 0, 0 ), which is only necessary if the light.position is modified.

Results look good and I see no errors left, but I find it hard to verify all examples by hand. Is there an automated way of doing it ?

test

npm run test fails on JSON serialization/deserialization (mismatching quaternion/positions).
I need help here !

[WestLangley]

the world light direction should be accessed everywhere using light.getWorldDirection( direction ).negate();

Unfortunately, that is a pretty inefficient method. I would avoid using it.

Maybe try something like this:

spotlight._target = new THREE.Object3D();
spotlight._target.position.set( 0, 0, 1 );
spotlight.add( _target );

You can then get the world direction of the spotlight from the world matrices of spotlight._target and spotlight.

[mbredif]

Good point, but IMHO that only means that the current getWorldDirection implementation is suboptimal...

Provided matrixWorld is in sync, isn't it just a matter of reading the 3rd column ?

new THREE.Vector3(spotLight.matrixWorld.elements[8], spotLight.matrixWorld.elements[9], spotLight.matrixWorld.elements[10])

I can do another PR for that.

[WestLangley]

Yes, that is much better. Normalize it, though, because the matrix may be scaled.

[mbredif]

See #14677 for the getWorldDirection optimization.

Any clue on the test correction and on the example verification ?

[mbredif]

Ok I figured out to fix the test (this.matrix was out of sync).

This PR is now ready for testing/review !

[mbredif]

As discussed in #14688, use transformDirection instead of getWorldDirection when matrixWorld is already up to date.

[looeee]

Well, if we are ok with a breaking change here in order to be in line with other apps, that seems like the better option rather than removing the target completely.

That way when people have to update their code that uses a light target, they just have to add 'light.targeted = true'. If we remove the target completely then they may have to do considerable refactoring.

[mbredif]

Up !
@WestLangley @mrdoob @looeee , any decision on the possible deprecation of the target functionality ?
I can remove it, make it opt-out or make it opt-in, just tell me.

[WestLangley]

@mrdoob is going to have to decide what features he wants, then we can debate the implementation details.

Also, any changes made in this PR should be consistent with #14935.

[WestLangley]

This was a feature that was suggested by @mrdoob years ago -- removing targets from lights.

@mbredif has invested a lot of effort in this PR. If the feature is still desired, we should push to integrate it.

[WestLangley]

As @WestLangley said:

Removing targets from lights was suggested by @mrdoob years ago.

@mbredif has invested a lot of effort implementing @mrdoob's suggestion.

I think @mbredif is entitled to a response to #14658 (comment).

@Mugen87 Would you also be willing to provide your feedback? (I expect you will have to read the entire thread carefully because a lot of issues were discussed.)

I see @donmccurdy may also support this. In light of the discussion in #14935, perhaps you have a preferred implementation.

//

Please also see #14621.

[mrdoob]

One approach to maintain backwards compatibility would be by doing something like this:

Light -> FreeSpotLight -> SpotLight

FreeSpotLight extends from Light and has no target.
SpotLight extends from FreeSpotLight and adds a target.

I took the "Free Spot Light" name from 3D Studio Max but open to better names.

[donmccurdy]

What would you think of reversed naming? SpotLight would have no target, but be extended by a subclass that adds the target?

• TargetedSpotLight
• TrackingSpotLight
• LegacySpotLight
• ...

The latter could be moved to examples/jsm, eventually. I realize that's a breaking change, but at least we can simplify the migration with warnings when .target is accessed on a plain SpotLight. If we want the end state to be untargeted by default, it seems like that should be associated with the more intuitive name.

[LeviPesin]

@mbredif Can you please update the PR?

[hybridherbst]

Still an issue! I think the target properties are about the soon-be-deprecated for at least 5 years now 🥲

[hybridherbst]

@mrdoob @donmccurdy do you think you can agree on a naming for new light types? If so, which?

[donmccurdy]

Personally I'm fine with any of these proposals, but perhaps happiest if we have a solution compatible with moving the targeting logic (maybe, eventually) move into addons/jsm/*.

One more idea to throw into the hat, add a LightTarget class, which can be bound to any compatible light source: DirectionalLight, SpotLight, IESpotLight, or RectAreaLight.

import { SpotLight } from 'three';
import { LightTarget } from 'three/addons/lights/LightTarget.js';

const light = new SpotLight();
light.position.set( 2, 1.5, 3 );
scene.add( light );

const target = new LightTarget().bind( light );
target.position.set( 0, 0, 0 );
scene.add( target );

[Mugen87]

Multiple times I've played around with the idea of changing the current way target works for directional and spot lights and I'm not really happy with the alternatives. I think what we have now is already good.

My latest branch where I have tested removing targets from light is here: Mugen87@2361111

There are two options:

a) Removing targets from lights completely.
b) Making them just optional but the default is "untargeted".

Both of them result in more code in three.js apps since you end up with additional lookAt() calls to get directional and spot lights right. This is noticeable when a light's position is animated like in webgl_lights_spotlights(see Mugen87@2361111#diff-66cb30d39214e3d336c0a3211271b06b5ba91b5b4ead298eef319e99af5d479b).

In the meanwhile, I have the feeling targeted lights actually make it easier to setup typical lit scenes (especially for beginners). I would not vote to completely remove targeted lights from three.js anymore. And I'm not sure it would be a good decision to make it optional and change the default from "targeted" to "untargeted".

In fact, directional and spot lights can already be "untargeted" by using the following setup:

light.target.position.set( 0, 0, - 1 );
light.add( light.target );

Now you can rotate the light by changing its rotation or quaternion property (directly or via methods like lookAt() or rotateTowards()) and get the expected result. This approach is already used by GLTFLoader.

For testing: https://jsfiddle.net/ouqwsp64/2/

Maybe we can put the above code in new convenient methods for DirectionalLight and SpotLight. Something like disableTargeting(). enableTargeting() would just undo this operation. Alternatively, a new property called targeted could be implemented as a getter/setter.

[hybridherbst]

Does that "untargeting" approach work with three's scene serialization? Last time I checked the three.js editor always breaks when saving and opening a scene that has lights loaded from glTF because then they are targeted again and look in the wrong ways.

(for the record, I'm still voting for untargeting by default as it is handled in all other 3D apps that I'm aware of)

[Mugen87]

Does that "untargeting" approach work with three's scene serialization?

Not yet, but #28696 should add support for it.