toji / gl-matrix

Javascript Matrix and Vector library for High Performance WebGL apps
glmatrix.net
MIT License
5.4k stars 725 forks source link

combined TRS decompose for mat4 #401

Closed randName closed 4 years ago

randName commented 4 years ago

(unrelated to #305)

mat4.fromRotationTranslationScale exists but not the other way round (i.e. Matrix4.decompose from Three.js)

The getX functions from #204 are excellent, but if both rotation and scale are desired there is a wasted call to getScaling (position is independent)

I can do a PR but the only thing I am unsure of is the function signature, since I can't find an example of multiple outs on a function (besides quat.getAxisAngle which sets the vec3 and returns the angle). I am also not sure if there was a concious decison to exclude it because of this.

stefnotch commented 4 years ago

I think the combined getter methods were excluded, because they weren't absolutely necessary. Still, I do think that combined getter methods are quite useful and would be quite happy about a PR.

Regarding the name, either getRotationTranslationScale or decompose would be fine.

randName commented 4 years ago

I was concerned about the return value too. Here is the proposed signature

/**
 * Decomposes a transformation matrix into its rotation, translation and scale components. ...
 * @param  {quat} out_r Quaternion to receive the rotation component
 * @param  {vec3} out_t Vector to receive the translation vector
 * @param  {vec3} out_s Vector to receive the scaling factor
 * @param  {ReadonlyMat4} mat Matrix to be decomposed (input)
 * @returns ??? <=
 */
export function decompose(out_r, out_t, out_s, mat) { // ...

Technically I don't even need the position to be lumped together for my own use (I am referencing the underlying ArrayBuffer directly i.e. new Float32Array(mat.buffer, 48, 3)), but I understand that is a little egregious to include another getRotationScale just for this narrow use case.

Alternatively, I propose adding a 3rd optional argument to getRotation that will allow an already-computed scaling vector to be used

/**
 * Returns a quaternion representing the rotational component ...
 * @param {quat} out Quaternion to receive the rotation component
 * @param {ReadonlyMat4} mat Matrix to be decomposed (input)
 * @param {ReadonlyVec3} scaling Scaling vector (optional)
 * @return {quat} out
 */
export function getRotation(out, mat, scaling) {
  if (scaling === undefined) {
    scaling = new glMatrix.ARRAY_TYPE(3);
    getScaling(scaling, mat);
  }
  // ...
stefnotch commented 4 years ago

I like the first option.