Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Matrix4 setXXXX methods blow away Matrix4 state (setRotationX, setRotationY, setRotationZ, setScale, setRotation, setTranslation) #1615

Closed
jicksta opened this Issue · 16 comments

4 participants

@jicksta

This may be the intended behavior but, if so, these methods should perhaps be renamed. Here is some code that caused a bug for me:

var newPlacement = new THREE.Matrix4;
newPlacement.setTranslation(otherPlayerData.x, otherPlayerData.y, otherPlayerData.z);
newPlacement.setRotationY(otherPlayerData.rotation);
mesh.matrix = newPlacement;
mesh.matrixWorldNeedsUpdate = true;

I discovered that I couldn't both setTranslation and setRotationY on the same matrix because they are each completely destructive. In this context though these methods, by their name, sound like they give setter access to a very specific part of the matrix.

This is how setRotationY is implemented:

setRotationY: function( theta ) {
    var c = Math.cos( theta ), s = Math.sin( theta );
    this.set(
         c, 0, s, 0,
         0, 1, 0, 0,
        -s, 0, c, 0,
         0, 0, 0, 1
    );
    return this;
},

So any data in the matrix originally will be destroyed.

Just wanted to let you guys know this was a big gotcha for me. Might trip others up too.

@mrdoob
Owner

I see your point. Maybe setToX would make it clearer?

setToTranslation()
setToRotationX()
setToRotationY()
setToRotationZ()
...
@jicksta

Is it really necessary to have completely destructive setters like this? It seems having them update only the matrix state specific to the method would work for everyone since someone who wanted it reset could do so explicitly.

@WestLangley
Collaborator

I believe the routines you are referring to were intended to be used to construct matrices that implement certain affine transformations, not to manipulate existing matrices--hence their destructive nature.

These constructed matrices could then be applied to an object's matrix like so:

object.matrix.multiplySelf( rotationMatrix );

But as a rule of thumb, I'd use object.position.set(), object.rotation.set(), and object.scale.set(), rather than manipulate the object's matrix directly.

@jicksta

@WestLangley That code I eventually changed to use object.position and object.rotation when it wasn't necessary to handle the matrix explicitly. I think I was managing the matrix manually because that's what jiglibjs2 wanted me to do. I think there are definitely times when it makes sense to set the matrix though so it should be supported.

If the original intent was to offer a nice API for creating certain matrices, the setXXXX naming convention might not be what most users would expect. One possibility is to make those class-level methods, as in:

var matrix = Matrix4.createWithTranslation();
@mrdoob
Owner

That doesn't follow the rest of the API...
How would users expect something that is different from the rest of the API?

@alteredq

make prefix feels somehow more natural:

matrix.makeTranslation();
matrix.makeRotationX();
matrix.makeRotationY();
matrix.makeRotationZ();

Though we already use this convention for methods which create new Matrix4 objects.

@jicksta

@mrdoob I was just meaning to suggest a class-level method if you're willing to consider a class-level solution to this problem. I'm not partial to the createWith prefix.

@alteredq Yeah, I like the make prefix so far the best. I don't think my original issue would have happened if that's what they were called.

@mrdoob
Owner

make prefix feels somehow more natural:

matrix.makeTranslation();
matrix.makeRotationX();
matrix.makeRotationY();
matrix.makeRotationZ();

I'm cool with that too.

Though we already use this convention for methods which create new Matrix4 objects.

Maybe we can move all these THREE.Matrix4.make* static methods to matrix.make* public methods so the API is consistent? (Leaving Matrix4 without any static method.)

@alteredq

Maybe we can move all these THREE.Matrix4.make* static methods to matrix.make* public methods so the API is consistent? (Leaving Matrix4 without any static method.)

So then instead of these factory-type methods the pattern would be to create new Matrix4 objects and then apply these makeXXX methods on them?

this.projectionMatrix = THREE.Matrix4.makePerspective( this.fov, this.aspect, this.near, this.far );
this.projectionMatrix = new THREE.Matrix4();
this.projectionMatrix.makePerspective( this.fov, this.aspect, this.near, this.far );

Sounds good. It would also prevent creation of garbage when changing camera parameters.

@mrdoob
Owner

Yup. Cool, I'll take care of this.

@mrdoob mrdoob closed this issue from a commit
@mrdoob Renamed Matrix4's setTranslation, setRotationX, setRotationY, setRota…
…tionZ, setRotationAxis and setScale to makeTranslation, makeRotationX, makeRotationY, makeRotationZ, makeRotationAxis and makeScale. Fixes #1615.
7f05ba1
@mrdoob mrdoob closed this in 7f05ba1
@alteredq

Nice ;)

@mrdoob
Owner

There is still THREE.Matrix4.makeInvert3x3, but I don't know what to do with it.

@alteredq

Hmmm, this one is strange indeed. It feels like some more complex changes would be needed around there. I'll look into this.

@alteredq alteredq referenced this issue from a commit in alteredq/three.js
@mrdoob Matrix4's static methods makeFrustum, makePerspective, makeOrtho are …
…now normal non-static public methods. Fixes #1615.
c5c89fa
@alteredq alteredq referenced this issue from a commit in alteredq/three.js
@alteredq alteredq Refactored handling of Matrix4 => Matrix3 inversion.
This is now done as method of Matrix3 object which gets as a parameter Matrix4 object to be inverted (to be consistent with how Matrix4 => Matrix4 inversion is done).

As per discussion in #1615.
9ab854d
@alteredq

Ok, done. Should be less weird and more consistent now.

@mrdoob
Owner

Beautiful! :)
Will handle the merge with my branch.

@jicksta

Nice. Thanks guys.

@wvl wvl referenced this issue from a commit in wvl/three.js
@mrdoob (threejs src) Renamed Matrix4's setTranslation, setRotationX, setRota…
…tionY, setRotationZ, setRotationAxis and setScale to makeTranslation, makeRotationX, makeRotationY, makeRotationZ, makeRotationAxis and makeScale. Fixes #1615.
50d5daa
@wvl wvl referenced this issue from a commit in wvl/three.js
@mrdoob (threejs src) Matrix4's static methods makeFrustum, makePerspective, …
…makeOrtho are now normal non-static public methods. Fixes #1615.
6b9be95
@wvl wvl referenced this issue from a commit in wvl/three.js
@alteredq alteredq (threejs src) Refactored handling of Matrix4 => Matrix3 inversion.
This is now done as method of Matrix3 object which gets as a parameter Matrix4 object to be inverted (to be consistent with how Matrix4 => Matrix4 inversion is done).

As per discussion in #1615.
bf6356f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.