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

combined TRS decompose for mat4 #401

Closed
randName opened this issue Aug 26, 2020 · 3 comments · Fixed by #402
Closed

combined TRS decompose for mat4 #401

randName opened this issue Aug 26, 2020 · 3 comments · Fixed by #402

Comments

@randName
Copy link
Contributor

randName commented Aug 26, 2020

(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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

I like the first option.

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

Successfully merging a pull request may close this issue.

2 participants