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

Remove node.matrix #745

Closed
mre4ce opened this issue Oct 12, 2016 · 5 comments
Closed

Remove node.matrix #745

mre4ce opened this issue Oct 12, 2016 · 5 comments

Comments

@mre4ce
Copy link

mre4ce commented Oct 12, 2016

A node can currently store a separate translation, rotation and scale or it can store a matrix. I vote for removing the matrix because it forces an implementation to implement code to decompose the matrix into a separate translation, rotation and scale in case an animation only changes some but not all three. Afaik this is the only reason to even have code to decompose a matrix. If for some reason source data stores matrices then it'd be better to do the decomposition in the converter as opposed to adding complexity to the glTF format.

@lexaknyazev
Copy link
Member

Can node.matrix contain something besides TRS (such as skew)?

@pjcozzi
Copy link
Member

pjcozzi commented Oct 12, 2016

Engines should not need to decompose the matrix for animation; the spec requires that when a node is targeted for animation that it uses TRS since animations can only target those properties.

@mre4ce would you be OK with just a spec update here that make this more explicit?

@mre4ce
Copy link
Author

mre4ce commented Oct 12, 2016

Making this more explicit in the spec would be good thanks. I would still prefer to get rid of node.matrix just to reduce complexity but keeping it is not the end of the world :)

@pjcozzi
Copy link
Member

pjcozzi commented Oct 17, 2016

I added a spec clarification in #750.

@donmccurdy
Copy link
Contributor

Addressed in glTF 2.0.

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

No branches or pull requests

4 participants