(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)

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.

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.

@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.

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 !

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.

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.

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

See #14677 for the getWorldDirection optimization.

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

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

This PR is now ready for testing/review !

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

The DirectionalLight points from its [page:.position position] to target.position. If target is undefined, it points to to the origin (0, 0, 0) of its parent frame.

light.lookAt( lookAtPosition ); Note: the LookAt position must be expressed in the parent frame of the light, which is typically the world frame if the light has been added to the scene with no rotation/translation.

Can you please provide (several) code snippets illustrating what you mean by each of these statements?

updated docs : SpotLight and DirectionalLight. (I also removed the obsolete Note about Position, Target and rotation from DirectionalLight.)

Is that closer to what you had in mind ?

1. I see you removed all references to the parent's frame, so my previous question is obsolete.

2. I guess you should remove normalize() from the following pattern in the examples:

light.position.set( 1, 1, 1 ).normalize();

3. Also, this change in the light constructors will be a breaking change for all existing users, right?

this.target = undefined;

4. I would be inclined to think, then, that if the goal is to remove target, you should remove it completely.

5. Also, if you want the direction of the light to be defined by its quaternion, then if the light is a child of a rotated parent, (a) the light's frustum may be 'tilted', and (2) as a child of a rotated parent, lookAt() will not work. Correct?

1. ok, great
2. why not, but that would require to check the near/far values to check that the shadow camera still bounds the scene...
3. It is indeed a breaking change : adding light.target = new THREE.Object3D() is required to get the previous behavior in all cases. It is needed to get the tracking behavior back. Alternatively, the direction of the light may also be set by altering the world quaternion (eg a lookAt or by rotating the light or one or more ancestors). If the light direction is constant along negative Z (the default direction), no change is necessary in user code.
4. I do not need the target tracking feature for now so I am ok to remove it, but I thought that keeping it around as an opt-in feature would be less painful for current users.
5. a) I do not get what you mean by tilted. what do you mean ? Relevant examples include webgl_shadowmap_viewer, webgl_lights_spotlight and webgl_lights_spotlights
6. b) that is correct, lookAt is not valid for objects with rotated/translated ancestors (I can add a comment in the lookat example in the docs for that)

2. Why normalize the position of the light? The position is used to set the location of the shadow camera.

3. The default direction is positive-z. Sorry, I do not understand your comment at all.

4. a. lookAt() orients the local z-direction and keeps the local x-direction parallel to the plane orthogonal to the up-direction. Applying an arbitrary quaternion to an object does not do that.

5. b. IMO, proper support for lookAt() is required if you want to implement this PR. See #14517.

2- These calls to normalize were already there. Removing the normalization will move the cameras, which has a major effect for spotlights (modified direct light) and a minor effect for spot and directional lights (shadow depth translation and thus possibly near/far clipping). I understand that these may convey the false impression that positions should be normalized, but isn't it orthogonal to this PR? if not could you point me to a specific example please !

3- If you look at the docs , the default light direction is top down (position 0,0,1 looking at 0,0,0)

3- Let me try to rephrase : this PR introduces a breaking change by initializing the spotlight and directionallight targets as undefined. To migrate previous code, adding light.target = new THREE.Object3D()  right after the light creation will always work. This PR introduces the new feature of making targets optional and handling translated/rotated lights. This change could be made not breaking by keeping the target initialization to a new Object3d in the constructors, would you prefer that option ? Alternatively the target tracking feature may be removed entirely, but that may break many code bases including existing examples and loaders. It is easier for me to keep the feature than to provide a patch for target tracking examples !

5.a) I agree that modifying quaternions will give an extra degree of freedom (the rotation around the light direction) which you can also get if you modify the up property of Object3D that does the lookAt. That is actually a feature that will be used when projecting textured lights #14621 (see https://github.com/mrdoob/three.js/pull/13057#discussion_r161080235 as well)

5.b) This PR is functionnally correct by working on light and target world positions on an orphaned shadow camera (even with a moving light looking at a moving target, as in the webgl_shadowmap_viewer example). I agree however that a more general lookAt function could be useful to the user for him to setup the light rotation. Thanks for the pointer !

2. OK. I expect the lights that normalize the position do not cast shadows. It's legacy code, anyway. You can leave it if you want, but it should be removed at some point because it is confusing.

3. a. OK.

4. b. "This change could be made not breaking by keeping the target initialization". Good idea. I am inclined to suggest that you make that change, but others should participate in this discussion.

2. I gave it a try in #14708.

Ok, I have rebased with a first series of commits with the opt-out target (no breaking change) followed by 3 commits for the breaking change (opt-in target) to ease the discussion.

@mrdoob ?

Users will need to remember to set

light.target = null;

to be able to set the light's orientation via light.rotation or light.quaternion or to use light.lookAt().

Failure to do so will result in bizarre behavior.

---

@mbredif I recommend that you set the default rotation to zero (as it is with all other objects), with the light shining up the positive-z axis. Otherwise, unexpected results occur if the user does something like this:

light.rotation.y += 0.01;

Also, in the docs you write:

// set the quaternion directly
light.quaternion.set( 1, 0, 0, 0 );
light.matrixWorldNeedsUpdate = true;

I think we want to say to set the rotation property, instead. Users do not set the quaternion. I do not think light.matrixWorldNeedsUpdate should be mentioned, either.

Failure to do so will result in bizarre behavior.

Modified rotation or quaternion will simply be ignored when a target is defined. If it is not clear in the docs, could you make a proposal ?

I recommend that you set the default rotation to zero

I am fine with that, but that is a breaking change : the default rotation for lights was not the identity: it was to look down at 0,0,0 from 0,0,1. If you insist, my proposal would be to have lights share the camera convention that they are pointing down, so that an initial identity rotation will keep the current behavior. That would mean having an isCamera || isLight test in the lookAt function and may allow me to get rid of cameraConvention. WDYT ?

light.matrixWorldNeedsUpdate = true;

Ok, but if the light was already rendered, then failing to set matrixWorldNeedsUpdate to true may prevent the light.matrixWorld from updating.

light.quaternion.set( 1, 0, 0, 0 );

OK, I can use something like light.rotation.y = 0.2;

Failure to do so will result in bizarre behavior.

The only other option I see here would be to add a callback to rotation and quaternion that sets the target to undefined. But that looks pretty hacky to me...

the default rotation for lights was not the identity

The current default rotation for lights is ( 0, 0, 0 ). I think you should keep it that way. For lookAt() to work correctly, lights should point up the positive z-axis.

OK, I can use something like light.rotation.y = 0.2;

light.rotation.y = Math.PI / 2;

The only other option I see here would be to add a callback ...

Not that. You could remove light.target altogether, which has been a goal of @mrdoob for a long time, anyway... But it is not for me to decide.

The current default rotation for lights is ( 0, 0, 0 ). I think you should keep it that way. For lookAt() to work correctly, lights should point up the positive z-axis.

AFAIK, lights (spotlights and directionallights at least) did not care about their local and world rotation(which was superseeded by the lookAt to the target). The lookAt was performed by the shadow camera, thus with a negative Z convention for the direction. I am just saying that to have non-rotated lights shine downwards, they should adopt the Camera convention for their lookAt, which will have the advantage of having identical world matrices for lights and their shadow cameras.

But it is not for me to decide.

OK, so let s wait for some decision between opt-out, opt-in and no support for light targets.

The current default rotation for lights is ( 0, 0, 0 ). I think you should keep it that way. For lookAt() to work correctly, lights should point up the positive z-axis.

This is how area lights work.

which will have the advantage of having identical world matrices for lights and their shadow cameras.

It seems to me the rotation of the shadow camera is irrelevant; users do not set that.

Large PRs are difficult to get merged, but I think this PR is worthy of consideration, as it allows users to set a light's orientation via its rotation, while still being backwards-compatible with existing code.

Using light.lookAt() will still be limited to lights without rotated parents unless lookAt() is fixed.

I just want to chime in and point out that we have an opportunity with this change to make a better API for a part of three.js that is currently confusing to users - however at the moment this PR is just going to replace one source of confusion with another, even though the functionality is improved.

The problem is that you are trying to implement two different light types (targeted and untargeted) using a single light.

Users will need to remember to set

light.target = null;

to be able to set the light's orientation via light.rotation or light.quaternion or to use light.lookAt().

Failure to do so will result in bizarre behavior.

Users will not remember this - in fact, they will just not know this to begin with, and it will lead to lots of confusion and endless posts here and on the forum that we will have to deal with, no matter how well we document it.

A better solution would be to add a targeted flag to the light:

const light = new THREE.DirectionLight();

light.targeted = false; // now you can set the rotation, use lookAt, etc, but any target will be ignored

light.targeted = true; // now you can use the target, but rotation, lookAt will have no effect

I have a minor preference that targeted = false should be the default, although obviously this would be a breaking change.

Thanks for the suggestion @looeee . I tried to implement it in the last commit. Is that what you had in mind ?

I have a minor preference that targeted = false should be the default, although obviously this would be a breaking change.

It's probably not advisable, then. If you are willing to have a breaking change, then you could remove target altogether.

It's probably not advisable, then.

I disagree, since it will overcome a common point of confusion and bring us in line with the standard used by most other apps, in this case it is advisable. Since we do already have targets and they are useful, I don't see any reason to remove the functionality completely. Let's just bring ourselves into line with everyone else by having a non-targeted light as the default.