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

setFromMatrixDirection #14688

Closed
wants to merge 2 commits into from
Closed

Conversation

mbredif
Copy link
Contributor

@mbredif mbredif commented Aug 10, 2018

With #14677, getWorldPosition and getWorldDirection have the same logic (except for the normalization of the direction) :

  • call updateMatrixWorld( true )
  • read a Vector3 from the matrixWorld

As a follow-up to #14677 (comment), this PR introduces setFromMatrixDirection as the counterpart of Vector3.setFromMatrixPosition, to be used directly when the matrixWorld is known to be up to date.

For instance, it would be useful in #14658 where light.matrixWorld is already synced :

light.getWorldDirection( uniforms.direction ).negate();

Note: setFromMatrixDirection is equivalent to setFromMatrixColumn( m, 2 ).normalize() but I propose it anyway as it saves a few function calls and multiplications and is more explicit.

@mbredif mbredif force-pushed the setFromMatrixDirection branch from cc4dadb to d88888c Compare August 10, 2018 12:43
@WestLangley
Copy link
Collaborator

I don't think a matrix commonly has a 'direction'.

BTW, what you are doing is also equivalent to:

vector.set( 0, 0, 1 ).transformDirection( matrix );

which, for clarity I much prefer to:

var e = matrix.elements;
vector.set( e[ 8 ], e[ 9 ], e[ 10 ] ).normalize();

even though it is marginally slower.

And, as you said, it is also equivalent:

vector.setFromMatrixColumn( matrix, 2 ).normalize();

@mbredif
Copy link
Contributor Author

mbredif commented Aug 10, 2018

Ok, I will then use the explicit vector.set( 0, 0, 1 ).transformDirection( matrix ); in my code and future PR whenever I know that the matrixWorld is in sync and getWorldDirection otherwise.
Thanks for the review.

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.

2 participants