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

mat4 decomposition #305

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

mat4 decomposition #305

wants to merge 6 commits into from

Conversation

igualeonte
Copy link

these new removal/decomposition functions allow to trasform some objects with their own translation/scaling/rotation while they are involved in a scene graph

these new removal/decomposition functions allow to trasform some objects with their own translation/scaling/rotation while they are involved in a scene graph
these new removal/decomposition functions allow to trasform some objects with their own translation/scaling/rotation while they are involved in a scene graph
@igualeonte igualeonte changed the title Patch 1 mat4 decomposition May 15, 2018
@@ -1240,6 +1240,138 @@ export function fromQuat(out, q) {
return out;
}

/**
* Removes the transation component from the transformation matrix.
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be translation?

Copy link
Author

Choose a reason for hiding this comment

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

yes thanks

* Removes the scaling component from the transformation matrix.
* This is equivalent to (but much faster than):
*
* mat4.getTransation(t, mat);
Copy link
Contributor

Choose a reason for hiding this comment

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

translation

Copy link
Author

Choose a reason for hiding this comment

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

yes thanks


/**
* Removes the scaling component from the transformation matrix.
* This is equivalent to (but much faster than):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have tests to support this claim? would be nice to have a look at them too

Copy link
Author

Choose a reason for hiding this comment

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

you are right ... but the errors you have report are comments ... all unit tests work ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe they do, but regarding performance, how can I know that this option is faster than the alternative written on the comment?

* Removes the rotation component from the transformation matrix.
* This is equivalent to (but much faster than):
*
* mat4.getTransation(t, mat);
Copy link
Contributor

Choose a reason for hiding this comment

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

translation :)

Copy link
Author

Choose a reason for hiding this comment

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

yes thanks

mat4.fromRotationTranslationScale(matA, q, t, s);
mat4.removeRotation(matB, matA);
result = quat.setAxisAngle(q, [0, 1, 0], 0.7);
console.log( result );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind removing this? I suppose it's just some debugging code.

result = quat.setAxisAngle(q, [0, 1, 0], 0.7);
console.log( result );
quat.normalize(result, mat4.getRotation(result, matB) );
console.log( result );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same goes here.

Copy link
Author

Choose a reason for hiding this comment

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

sorry thanks

let s = vec3.fromValues(5, 6, 7);
mat4.fromRotationTranslationScale(matA, q, t, s);
mat4.removeScaling(matB, matA);
result = vec3.fromValues(5, 6, 7);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the point of this?

let t = vec3.fromValues(1, 2, 3);
let s = vec3.fromValues(5, 6, 7);
mat4.fromRotationTranslationScale(matA, q, t, s);
mat4.removeScaling(matB, matA);
Copy link
Collaborator

Choose a reason for hiding this comment

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

result = mat4.removeScaling(out, matA); would be better, since that allows you to check more things.

  • You can check if it returns the result
  • If it sets out to the correct value
  • If matA is untouched

@stefnotch
Copy link
Collaborator

stefnotch commented May 18, 2018

while they are involved in a scene graph

That sounds like a good use case. However, I'm not entirely sure how they would be used. Would you mind typing up some pseudo code/an explanation?

A link is also fine.

@igualeonte
Copy link
Author

OK ... later ...

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.

3 participants