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

Directional light target position cannot be updated #10220

Closed
looeee opened this issue Nov 24, 2016 · 16 comments
Closed

Directional light target position cannot be updated #10220

looeee opened this issue Nov 24, 2016 · 16 comments

Comments

@looeee
Copy link
Collaborator

looeee commented Nov 24, 2016

The directional light calculates its direction based on its position and the position of its target, which is an empty Object3D set by default at the origin.

It uses the matrixWorld property of its target for calculation, so if the position of the target is updated the target.matrixWorld needs to be updated too, and this doesn't happen automatically unless it is added to the scene. See #10218

So if I do this:

var light = new THREE.DirectionalLight( 0xfff000, 2 );
scene.add(light);

light.position.set(0, 1, 0);  // default; light from top shining down

//light.target.position is currently (0,0,0)

Then do this:

light.target.position.set( 0, 2, 0 );

I would expect the light to switch to shining from the bottom up. However nothing happens because the light.target.matrixWorld doesn't get updated.

One possible fix is to add the target to the scene alongside the light:

scene.add(light, light.target);

Now things work as expected. I can update the docs to reflect this requirement if people think it's the best approach.

But it would be great if it was automatic. Perhaps the Object3d.add() function could be extended to check if an object has a .target and if so add that too?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 24, 2016

But it would be great if it was automatic. Perhaps the Object3d.add() function could be extended to check if an object has a .target and if so add that too?

I don't think thats a good idea. The scene object represents a so called scene graph. The renderer should only process and update entities that are part of this graph. So just creating an Object3d is not enough. It must be a child of the scene object to get properly updated.

More about this concept: Scene Graph

Maybe we could enhance the scene docs with some of these information.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 24, 2016

#10218 refers to the same topic...

@looeee
Copy link
Collaborator Author

looeee commented Nov 24, 2016

@Mugen87 the issue is that the DirectionalLight is instantiated with a target, which is used in calculating the direction, but which is not automatically updated unless it's added to the scene graph along with the light. Most users would reasonably expect otherwise, so this behaviour is quite unintuitive.

A better solution would be for the default DirectionalLight to be untargeted and have its direction set by its rotation.

@mrdumb
Copy link

mrdumb commented Nov 24, 2016

It uses the matrixWorld property of its target for calculation, so if the position of the target is updated the target.matrixWorld needs to be updated too, and this doesn't happen automatically unless it is added to the scene.
See #10218

If something is not added to the scene, it doesn't exist in that scene (obviously ... )
If it doesn't exist, doesn't have a matrix in that world.
If it doesn't have a matrix, you cannot update the matrixWorld.

@looeee
Copy link
Collaborator Author

looeee commented Nov 24, 2016

@mrdumb that's not correct, all Object3D's and derived classes have a .matrixWorld whether or not they are in the scene. It's used to show the transform of the object relative to its parent, and if it doesn't have a parent will be identical to its local .matrix

You can manually call DirectionalLight.target.updateMatrixWorld() and it updates the matrixWorld fine even though it's not in the scene.

The issue here is that an object that is added to the scene (and thus has its matrixWorld automatically updated) relies in its calculations on an object that is not by default added to the scene (and thus doesn't have its matrixWorld automatically updated), leading to unintuitive behaviour.

I'm just wondering what the best way to deal with this is - whether to just document and forget about it, or if we can make the process automatic so that people using the library are not faced with this issue.

@mrdumb
Copy link

mrdumb commented Nov 24, 2016

all Object3D's and derived classes have a .matrixWorld whether or not they are in the scene.

Can you estimate the height of someone if you don't see him?

@looeee
Copy link
Collaborator Author

looeee commented Nov 24, 2016

@mrdumb this is my point exactly - the DirectionalLight.target.matrixWorld is being used to calculate the direction of the DirectionalLight without the DirectionalLight.target being part of the scene...

@WestLangley
Copy link
Collaborator

Related thread: #5555

@aardgoose
Copy link
Contributor

Could you add an updateMatrixWorld() method to the light, which calls the base object3d updateMatrixWorld and updates the light target? Transparent to user, downside is the target gets updated twice if part of scene.

@Usnul
Copy link
Contributor

Usnul commented Nov 25, 2016

personally I found "target" to be more of a distraction that something useful on "Directional" light. I have a wrapper around it that only reasons in terms of "direction" and never "target". Even "position" of a directed light is not a meaningful concept as there is no source in the strictest sense, if you wanted to model one - it would be a plane negative infinite distance along the direction vector.

@WestLangley
Copy link
Collaborator

Even "position" of a directed light is not a meaningful concept.

Lights require a position for shadow mapping. The position is where the shadow camera is located.

I found "target" to be more of a distraction than something useful on "Directional" light.

The target is an Object3D, which can be tracked by the light. The direction is inferred from the world position of the target and the world position of the light.

Could you add an updateMatrixWorld() method to the light

The target is a property of the light, not a child of the light. Also, updateMatrixWorld currently does not properly update the world matrix of the object because it traverses the children instead of the parents.

I think the current best solution is to continue to do what we have been recommending for a long time: make a note to the user to add the target to the scene graph -- at least until target is removed or updateMatrixWorld() is fixed.

@looeee
Copy link
Collaborator Author

looeee commented Nov 25, 2016

@WestLangley what do you think of the idea of adding a lookAt function to the light as discussed in #5555? It seems like this would be relatively straightforward, it would just have to set the target's position and call target.updateMatrixWorld() ( plus a few checks for the situation where the target has been changed to a different object).

@aardgoose
Copy link
Contributor

@WestLangley I was suggesting that updateMatrixWorld() for the light basically did:

Object3D.prototype.updateMatrixWorld.call (this);
this.target.updateMatrixWorld().

I wasn't thinking it was a child, Do the same issues effect SpotLight?

@WestLangley
Copy link
Collaborator

@looeee lookAt() does not work correctly if the object has a rotated/translated parent, so it is restricted. See the inline comment. (The basic problem is getting it to work while still keeping the object "right-side-up" after lookAt() is called. A rotated parent can tilt the object.)

@aardgoose Yes, that seems to be a reasonable approach to consider, but updateMatrixWorld() would need to be fixed.

@mrdoob has been advocating removing target completely for some time. I am keeping an open-mind on that approach. Forcing the renderer to auto-update the target is also reasonable.

Until then, some comments in the docs advising the user to add the target to the scene graph is appropriate.

@Usnul
Copy link
Contributor

Usnul commented Nov 25, 2016

@WestLangley
All valid points, which is why I had a kludge written on top of DirectionLight and shadowmaps to make it "just work". There is no user value in having access to "position" of a directional light (not so far as I can see), it is something to tweak until your shadowcamera's frontal plane no longer clips into the scene, and then if you scene is dynamic - you have to keep updating that. I like it with my kludge, but I would prefer not having to make one in the first place :)

@looeee
Copy link
Collaborator Author

looeee commented Dec 15, 2016

Added information on light targets the docs, and there are many similar open issues to this one, closing.

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

No branches or pull requests

6 participants