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

spotLightHelper: fix duplicate rotation #14738

Closed
wants to merge 4 commits into from

Conversation

but0n
Copy link
Contributor

@but0n but0n commented Aug 18, 2018

When I rotate the parent of spot light, the cone of light helper will rotate twice.

image

Scene tree

  • scene
    • spotLight group (transform control attach here)
      • spot light
      • target of spot light

Example

this.matrix = light.matrixWorld;

  • The only thing this object need is the world position of the spot light, but this matrix here include world position and world rotation (not direction) of the light, so the helper will apply the world rotation matrix of the light.

this.cone.lookAt( vector2.sub( vector ) );

  • this will also locally rotate the cone to the correct direction.

@@ -83,6 +80,7 @@ SpotLightHelper.prototype.update = function () {
vector.setFromMatrixPosition( this.light.matrixWorld );
vector2.setFromMatrixPosition( this.light.target.matrixWorld );

this.position.setFromMatrixPosition( this.light.matrixWorld );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do this, instead:

this.position.copy( vector );

@WestLangley
Copy link
Collaborator

SpotLightHelper is limited in that it only supports lights without rotated parents.

An advantage of this change is it eliminates this pattern:

this.matrix = light.matrixWorld;
this.matrixAutoUpdate = false;

If #14658 is merged, then this PR would have to be revisited.

/ping @mbredif

@mbredif
Copy link
Contributor

mbredif commented Oct 5, 2018

/ping @mbredif

This is a quick and simple fix, I think it can be merged before the discussion on #14658 settles.

As a side note you could save a temporary Vector3 variable and a Vector3.copy call by replacing

var vector = new Vector3();
var vector2 = new Vector3();
[...]
vector.setFromMatrixPosition( this.light.matrixWorld );
vector2.setFromMatrixPosition( this.light.target.matrixWorld );
this.position.copy( vector );
this.cone.lookAt( vector2.sub( vector ) );

with

var target = new Vector3();
[...]
this.position.setFromMatrixPosition( this.light.matrixWorld );
target.setFromMatrixPosition( this.light.target.matrixWorld );
this.cone.lookAt( target.sub( this.position ) );

@WestLangley
Copy link
Collaborator

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Nov 27, 2018

Do you still have the issue after #15320?

@but0n
Copy link
Contributor Author

but0n commented Nov 27, 2018

Nope, thanks!

@but0n but0n closed this Nov 27, 2018
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.

4 participants