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

Critical bug with spotLight.target.position #5555

Closed
doexclusive opened this Issue Nov 1, 2014 · 24 comments

Comments

Projects
None yet
8 participants
@doexclusive

doexclusive commented Nov 1, 2014

Bug: The changes to spotLight.target.position are not affecting.

Normal work with r68: http://jsfiddle.net/doexclusive/jhoe4ynr/
Bug with r69 and from dev branch: http://jsfiddle.net/doexclusive/6dhdbpw9/

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 2, 2014

Collaborator

Make sure you use reasonable parameters for your spotlight. The angle should be less than PI / 2.

Spotlight.target is a property of the light, but it is not part of the scene graph. Therefore target.matrix and target.matrixWorld are not being updated.

A work-around is to call target.updateMatrixWorld().

updated fiddle: http://jsfiddle.net/6dhdbpw9/4/

If, instead, you had an object in your scene as the target, there would be no problem.

spotLight.target = myObject;

I agree that this is confusing, and something should be done to fix it.

three.js r.69

Collaborator

WestLangley commented Nov 2, 2014

Make sure you use reasonable parameters for your spotlight. The angle should be less than PI / 2.

Spotlight.target is a property of the light, but it is not part of the scene graph. Therefore target.matrix and target.matrixWorld are not being updated.

A work-around is to call target.updateMatrixWorld().

updated fiddle: http://jsfiddle.net/6dhdbpw9/4/

If, instead, you had an object in your scene as the target, there would be no problem.

spotLight.target = myObject;

I agree that this is confusing, and something should be done to fix it.

three.js r.69

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 2, 2014

Collaborator

I see now that light.target used to be added to the scene in THREE.Scene.prototype.__addObject(). This method has been removed.

Collaborator

WestLangley commented Nov 2, 2014

I see now that light.target used to be added to the scene in THREE.Scene.prototype.__addObject(). This method has been removed.

@mrdoob mrdoob added the Bug label Nov 2, 2014

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 7, 2014

Collaborator

@mrdoob What do you think should be done to remedy this? Something along the lines of the following?

if ( object instanceof THREE.Light ) {
    if ( object.target && object.target.parent === undefined ) {            
       scene.add( object.target );      
    }
}
Collaborator

WestLangley commented Nov 7, 2014

@mrdoob What do you think should be done to remedy this? Something along the lines of the following?

if ( object instanceof THREE.Light ) {
    if ( object.target && object.target.parent === undefined ) {            
       scene.add( object.target );      
    }
}
@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Nov 7, 2014

Owner

I think we should remove target. We should be able to replace it with lookAt and a distance property.

Owner

mrdoob commented Nov 7, 2014

I think we should remove target. We should be able to replace it with lookAt and a distance property.

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 7, 2014

Collaborator

Remember, however, that the original idea was target could be set to an object in the scene. That is why target is an Object3D and not a Vector3.

Collaborator

WestLangley commented Nov 7, 2014

Remember, however, that the original idea was target could be set to an object in the scene. That is why target is an Object3D and not a Vector3.

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Nov 7, 2014

Owner

Yes. But cameras don't have targets and they seem to be doing ok :)

Owner

mrdoob commented Nov 7, 2014

Yes. But cameras don't have targets and they seem to be doing ok :)

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 7, 2014

Collaborator

Yes, yes. I was just thinking outloud... Still am. : - ) I remember that this issue came up several years ago, and I vaguely remember @alteredq had a good reason for why it is the way it is.

Collaborator

WestLangley commented Nov 7, 2014

Yes, yes. I was just thinking outloud... Still am. : - ) I remember that this issue came up several years ago, and I vaguely remember @alteredq had a good reason for why it is the way it is.

@erichlof

This comment has been minimized.

Show comment
Hide comment
@erichlof

erichlof Nov 7, 2014

Contributor

fwiw, I agree with @mrdoob that removing target altogether would be more inline with the rest of Objects in three.js. If we had a simple 'lookAt' function on the spotlight, it would allow users to treat the spotlight as a camera-type object that performs 'lookAt' to an object/character in the scene intuitively (as cameras already do now). Then there's no need to create more data for a target vector and worry about moving and updating that target within the user's code.
Removing target might break some current examples though, I don't know how many would be affected.

Contributor

erichlof commented Nov 7, 2014

fwiw, I agree with @mrdoob that removing target altogether would be more inline with the rest of Objects in three.js. If we had a simple 'lookAt' function on the spotlight, it would allow users to treat the spotlight as a camera-type object that performs 'lookAt' to an object/character in the scene intuitively (as cameras already do now). Then there's no need to create more data for a target vector and worry about moving and updating that target within the user's code.
Removing target might break some current examples though, I don't know how many would be affected.

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Nov 7, 2014

Owner

I remember that this issue came up several years ago, and I vaguely remember @alteredq had a good reason for why it is the way it is.

His argument was because the shadowmap code relied in it.

Owner

mrdoob commented Nov 7, 2014

I remember that this issue came up several years ago, and I vaguely remember @alteredq had a good reason for why it is the way it is.

His argument was because the shadowmap code relied in it.

@erichlof

This comment has been minimized.

Show comment
Hide comment
@erichlof

erichlof Nov 7, 2014

Contributor

In addition to ShadowMapPlugin.js,
https://github.com/mrdoob/three.js/blob/2d59713328c421c3edfc3feda1b116af13140b94/src/renderers/webgl/plugins/ShadowMapPlugin.js
It appears that SpotLightHelper.js is the only other file that uses light.target:
https://github.com/mrdoob/three.js/blob/2d59713328c421c3edfc3feda1b116af13140b94/src/extras/helpers/SpotLightHelper.js

Would it be possible to manually calculate those target vectors using the original spotlight's quaternion and using a quat-to-vector routine? All done internally inside the 2 above files that need some sort of target to function properly:
target = new THREE.Vector3(0,0,-1);
target.applyQuaternion(spotLight.quaternion);
//may need to scale it out to an arbitrarily large distance in worldspace so it never ends up 'behind' the spotlight:
target.multiplyScalar(10000);

Then target could be hidden from the user and instead suggest that the end user uses spotLight.lookAt() whenever they need to focus the spotLight on a moving subject in the scene.

Contributor

erichlof commented Nov 7, 2014

In addition to ShadowMapPlugin.js,
https://github.com/mrdoob/three.js/blob/2d59713328c421c3edfc3feda1b116af13140b94/src/renderers/webgl/plugins/ShadowMapPlugin.js
It appears that SpotLightHelper.js is the only other file that uses light.target:
https://github.com/mrdoob/three.js/blob/2d59713328c421c3edfc3feda1b116af13140b94/src/extras/helpers/SpotLightHelper.js

Would it be possible to manually calculate those target vectors using the original spotlight's quaternion and using a quat-to-vector routine? All done internally inside the 2 above files that need some sort of target to function properly:
target = new THREE.Vector3(0,0,-1);
target.applyQuaternion(spotLight.quaternion);
//may need to scale it out to an arbitrarily large distance in worldspace so it never ends up 'behind' the spotlight:
target.multiplyScalar(10000);

Then target could be hidden from the user and instead suggest that the end user uses spotLight.lookAt() whenever they need to focus the spotLight on a moving subject in the scene.

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 7, 2014

Collaborator

lookAt() sets the quaternion. Spotlights do not use the quaternion.

Collaborator

WestLangley commented Nov 7, 2014

lookAt() sets the quaternion. Spotlights do not use the quaternion.

@erichlof

This comment has been minimized.

Show comment
Hide comment
@erichlof

erichlof Nov 7, 2014

Contributor

Ah, my bad. Since I've just found some more files that use a 'light.target' property, rather that removing target and breaking all that code, could we just add a customized lookAt() to spotlight (and directionalLight for that matter) that manually changes its 'target' property to correctly reflect the new direction?
Then the target property would remain internally in Three.js but the end user gets intuitive and easy-to-use functionality:
spotLight.lookAt( gameCharacter.position );
sunLight = new THREE.DirectionalLight( .. ..);
sunLight.position.set(1000,1000,1000);
sunLight.lookAt( scene.position ); // or getting creative,
sunLight.lookAt( gameCharacter.position );

I know that's not how three.js currently implements the direction of the rays of a DirectionalLight, but it might be worth considering?

Contributor

erichlof commented Nov 7, 2014

Ah, my bad. Since I've just found some more files that use a 'light.target' property, rather that removing target and breaking all that code, could we just add a customized lookAt() to spotlight (and directionalLight for that matter) that manually changes its 'target' property to correctly reflect the new direction?
Then the target property would remain internally in Three.js but the end user gets intuitive and easy-to-use functionality:
spotLight.lookAt( gameCharacter.position );
sunLight = new THREE.DirectionalLight( .. ..);
sunLight.position.set(1000,1000,1000);
sunLight.lookAt( scene.position ); // or getting creative,
sunLight.lookAt( gameCharacter.position );

I know that's not how three.js currently implements the direction of the rays of a DirectionalLight, but it might be worth considering?

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 7, 2014

Collaborator

How about

spotlight.target = gameCharacter;
Collaborator

WestLangley commented Nov 7, 2014

How about

spotlight.target = gameCharacter;
@erichlof

This comment has been minimized.

Show comment
Hide comment
@erichlof

erichlof Nov 7, 2014

Contributor

Yes, I suppose that would be a lot less code to add to three.js (as opposed to the custom lookAt() functions for the directional and spot lights that we were suggesting). I also like the way you implemented the target functionality above. That way, the target is a true part of the scene and can be changed on the fly.
Just so I'm understanding correctly, the new usage would look like:
spotlight.target.copy( gameCharacter.position ); // I hardly use = anymore, rather 'copy'
edit - or since target is going to be an Object3D, should it be
spotlight.target.position.copy( gameCharacter.position );

if it is indeed going to be an Object3D, I could envision doing something creative like:
var myAxis = new THREE.Vector3(1,1,0);
myAxis.normalize();
var mySpeed = 100;
spotlight.target.translateOnAxis( myAxis, mySpeed );

If either is the case, I'm all for it :)

Contributor

erichlof commented Nov 7, 2014

Yes, I suppose that would be a lot less code to add to three.js (as opposed to the custom lookAt() functions for the directional and spot lights that we were suggesting). I also like the way you implemented the target functionality above. That way, the target is a true part of the scene and can be changed on the fly.
Just so I'm understanding correctly, the new usage would look like:
spotlight.target.copy( gameCharacter.position ); // I hardly use = anymore, rather 'copy'
edit - or since target is going to be an Object3D, should it be
spotlight.target.position.copy( gameCharacter.position );

if it is indeed going to be an Object3D, I could envision doing something creative like:
var myAxis = new THREE.Vector3(1,1,0);
myAxis.normalize();
var mySpeed = 100;
spotlight.target.translateOnAxis( myAxis, mySpeed );

If either is the case, I'm all for it :)

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Nov 7, 2014

Collaborator
spotlight.target = gameCharacter;

is the current implementation.

Collaborator

WestLangley commented Nov 7, 2014

spotlight.target = gameCharacter;

is the current implementation.

@erichlof

This comment has been minimized.

Show comment
Hide comment
@erichlof

erichlof Nov 7, 2014

Contributor

Oh ok, I just have trouble following the chains from SpotLight.target through Light.js through Object3D.js to Object3D.position :)

Would any of my above examples conceivably function (using the 'target' just like a regular Object3D in three.js)?

Contributor

erichlof commented Nov 7, 2014

Oh ok, I just have trouble following the chains from SpotLight.target through Light.js through Object3D.js to Object3D.position :)

Would any of my above examples conceivably function (using the 'target' just like a regular Object3D in three.js)?

@RunninglVlan

This comment has been minimized.

Show comment
Hide comment
@RunninglVlan

RunninglVlan May 8, 2015

Contributor

I understand that at some point SpotLight.target is going to be replaced by some other implementation for changing light's direction, but until then it'd be good to add a note in SpotLight's documentation, that its target should be part of the scene to work properly.

Contributor

RunninglVlan commented May 8, 2015

I understand that at some point SpotLight.target is going to be replaced by some other implementation for changing light's direction, but until then it'd be good to add a note in SpotLight's documentation, that its target should be part of the scene to work properly.

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob
Owner

mrdoob commented May 8, 2015

@RunninglVlan Agreed! Feel free to make the change directly :)

@mbrav

This comment has been minimized.

Show comment
Hide comment
@mbrav

mbrav Oct 20, 2016

I'm trying to implement a first person view SpotLight() into my PerspectiveCamera(). That is, direct the SpotLight() towards whatever the PerspectiveCamera() is looking at. I want to allow the user to control the direction of the SpotLight() based on the angle (and position) of the PerspectiveCamera() (first person view).

Here is My Sketch (code) where I have cubes positioned based on some OpenData coordinates.

But here a screenshot of a position that conveys my intention well. The first person view is positioned in such a manner, that it seems like there is spotlight originating from the viewer:

Imgur

Question is, how do I set the direction and the position of the SpotLight() to be the same as the direction and the position of the PerspectiveCamera()? Do I create a 3DObject() that the Spotlight() could be directed towards (and do all the annoying math), or is there a better workaround for this issue?

Three.js r81

P.S. Issue is super annoying, will throw a party when it gets fixed.

Issue reference: #1437, #2251, #3497, #4261, #5079, #5870, #7132, #7536, #7965

mbrav commented Oct 20, 2016

I'm trying to implement a first person view SpotLight() into my PerspectiveCamera(). That is, direct the SpotLight() towards whatever the PerspectiveCamera() is looking at. I want to allow the user to control the direction of the SpotLight() based on the angle (and position) of the PerspectiveCamera() (first person view).

Here is My Sketch (code) where I have cubes positioned based on some OpenData coordinates.

But here a screenshot of a position that conveys my intention well. The first person view is positioned in such a manner, that it seems like there is spotlight originating from the viewer:

Imgur

Question is, how do I set the direction and the position of the SpotLight() to be the same as the direction and the position of the PerspectiveCamera()? Do I create a 3DObject() that the Spotlight() could be directed towards (and do all the annoying math), or is there a better workaround for this issue?

Three.js r81

P.S. Issue is super annoying, will throw a party when it gets fixed.

Issue reference: #1437, #2251, #3497, #4261, #5079, #5870, #7132, #7536, #7965

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Oct 22, 2016

Collaborator

@mbrav This is not a help site. Consider stackoverflow if you still need help.

Collaborator

WestLangley commented Oct 22, 2016

@mbrav This is not a help site. Consider stackoverflow if you still need help.

@looeee

This comment has been minimized.

Show comment
Hide comment
@looeee

looeee Dec 15, 2016

Collaborator

Instructions on how targets work have now been added to the docs :)

Collaborator

looeee commented Dec 15, 2016

Instructions on how targets work have now been added to the docs :)

@mrdoob mrdoob closed this Dec 16, 2016

@schubboy

This comment has been minimized.

Show comment
Hide comment
@schubboy

schubboy Jun 30, 2017

I'm pretty sure this is broken with r86. I tried everything suggested here and I still cannot get the spotlight to target anything but 0,0,0. Are we positive this bug was fixed?

schubboy commented Jun 30, 2017

I'm pretty sure this is broken with r86. I tried everything suggested here and I still cannot get the spotlight to target anything but 0,0,0. Are we positive this bug was fixed?

@schubboy

This comment has been minimized.

Show comment
Hide comment
@schubboy

schubboy Jun 30, 2017

Ok, I figured this out, and you may want to update the help documentation to prevent further confusion.

If the light is in "physical" mode, targeting will not work. For targeting to work, you MUST set the decay parameter to 1. Bug or no bug, thats how this is currently working.

So if I create a spotlight "var l = new THREE.Spotlight(0xffffff, 1);", the target will not work. But I do "var l = new THREE.Spotlight(0xffffff, 1, 0, Math.PI, 1);" targeting will work. If I do "var l = new THREE.Spotlight(0xffffff, 1, 0, Math.PI, 2);" or "var l = new THREE.Spotlight(0xffffff, 1, 100, Math.PI, 2);", targeting will not work.

Fiddle showing targeting working: http://jsfiddle.net/schubboy/rx09v1fy/
fiddle showing targeting NOT working due to decay parameter set to 2: https://jsfiddle.net/schubboy/rx09v1fy/

schubboy commented Jun 30, 2017

Ok, I figured this out, and you may want to update the help documentation to prevent further confusion.

If the light is in "physical" mode, targeting will not work. For targeting to work, you MUST set the decay parameter to 1. Bug or no bug, thats how this is currently working.

So if I create a spotlight "var l = new THREE.Spotlight(0xffffff, 1);", the target will not work. But I do "var l = new THREE.Spotlight(0xffffff, 1, 0, Math.PI, 1);" targeting will work. If I do "var l = new THREE.Spotlight(0xffffff, 1, 0, Math.PI, 2);" or "var l = new THREE.Spotlight(0xffffff, 1, 100, Math.PI, 2);", targeting will not work.

Fiddle showing targeting working: http://jsfiddle.net/schubboy/rx09v1fy/
fiddle showing targeting NOT working due to decay parameter set to 2: https://jsfiddle.net/schubboy/rx09v1fy/

@schubboy

This comment has been minimized.

Show comment
Hide comment
@schubboy

schubboy Jun 30, 2017

schubboy commented Jun 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment