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

Refactor flexible skinning #4812

Merged
merged 11 commits into from Jul 11, 2014
Merged

Conversation

ikerr
Copy link
Contributor

@ikerr ikerr commented May 13, 2014

This commit improves the flexibility of the animation system by allowing any Object3D instance to be used for skinning--not just Bone instances. Furthermore, this commit removes the restriction that a SkinnedMesh can only be skinned by child objects/bones. Now, a SkinnedMesh can reference arbitrary objects for the
purpose of skinning. One implication of this is that Bone.skinMatrix has been removed as it is no longer necessary.

For prior discussion, see issue #4782:

#4782

Ian Kerr added 2 commits May 13, 2014 09:32
…ing any

Object3D instace to be used for skinning--not just Bone instances.  Furthermore,
this commit removes the restriction that a SkinnedMesh can only be skinned by
child objects/bones.  Now, a SkinnedMesh can reference arbitrary objects for the
purpose of skinning.  One implication of this is that Bone.skinMatrix has been
removed as it is no longer necessary.

Here is a breakdown of the notable changes:

src/extras/animation/Animation.js
* Fixed animation of non-bones
* Removed references to skinMatrix

src/extras/animation/AnimationHandler.js
* Removed special handling of SkinnedMatrix instances

src/extras/animation/KeyFrameAnimation.js
* Removed special handling of Bone instances
* Removed references to skinMatrix

src/extras/helpers/SkeletonHelper.js
* Removed references to skinMatrix and Skeleton

src/objects/Bone.js
* Removed references to skinMatrix

src/objects/Skeleton.js
* Removed entirely, refactoring functionality back into SkinnedMesh

src/objects/SkinnedMesh.js
* Re-added code to construct the skeleton hierarchy when it is present in the geometry object that is passed to the constructor.  This is here for backwards compatibility and could be refactored at a later time.
* Added bind() function that takes an array of bones (Object3D instances) that are used to skin the mesh.  This function establishes the initial bind transforms of the mesh and bones.
* Added initBoneInverses() function that is used internally by bind() to calculate the bind matrices.
* Added initBoneMatrices(), which contains refactored code that previously existed in the constructor (prior to the THREE.Skeleton refactor).  This code sets up storage for the bone matrices and/or texture.  It is used internally by bind().
* Added updateBoneMatrices(), which is called by the renderer after the scene's world matrices have been updated.  This is done in a separate pass because SkinnedMesh can depend on objects that are stored anywhere in the scene hierarchy, not necessarily child objects.

src/renderers/WebGLRenderer.js
* Call updateBoneMatrices() on SkinnedMesh instances after world matrices have been updated
* Removed references to Skeleton
Conflicts:
	build/three.js
	build/three.min.js
@ikerr ikerr mentioned this pull request May 13, 2014
@mrdoob
Copy link
Owner

mrdoob commented May 14, 2014

Why was Skeleton removed?

I mean, I did read your reasoning in the other thread but I agree with @crobi on thinking that is good to have an object that tracks all the bone hierarchy and that can be reused in different SkinnedMesh.

@mrdoob
Copy link
Owner

mrdoob commented May 14, 2014

Also, try not to include builds in PRs.

@ikerr
Copy link
Contributor Author

ikerr commented May 14, 2014

Hi Mr. Doob,

First, sorry for committing the builds—I’ll be sure not to do that next time.

I like the idea of being able to animate multiple meshes with a single Skeleton. I also like the idea of having a single place to reference all of the objects/bones that make up a skeleton. However, we'll need some additional changes to make that work...

Firstly, with my changes, SkinnedMesh's boneMatrices/boneTexture depend on the transform of the SkinnedMesh, so if we were to put those transforms in Skeleton, the Skeleton would still be tied to a specific SkinnedMesh. This doesn't really help us, and is the reason that I removed Skeleton.

Why do SkinnedMesh's boneMatrices now depend on the SkinnedMesh transform? This is new with my changes. It is necessary because SkinnedMesh can now reference objects/bones that are not its descendants... To perform skinning, we need a common space (for the objects/bones and the mesh) in which to perform the skinning. Previously, we performed skinning in the local space of the SkinnedMesh. But now, since the objects/bones are not necessarily descendants, it is more natural to perform skinning in world space…

For example, previously boneMatrices were calculated like this:

bones[b].skinMatrix * boneInverses[b]

where boneInverses[b] was set to inverse(bones[b].skinMatrix) at bind time. With my changes, we calculate boneMatrices like this:

inverse(skinnedMesh.matrixWorld) * bones[b].matrixWorld * boneInverses[b]

where boneInverses[b] is set to inverse(bones[b].matrixWorld) * skinnedMesh.matrixWorld at bind time.

In either case, the end result is the same: boneMatrices maps vertices from the local space of SkinnedMesh to their new, animated positions in the same local space. The difference is that, in the second case, more general skeletons (that include objects/bones that are not descendants of the mesh) can be used. The drawback is that boneMatrices is now dependent on the SkinnedMesh transform, which prevents us from decoupling boneMatrices and SkinnedMesh.

How can we solve this? How can we decouple boneMatrices and SkinnedMesh with this new update? To do this, we would need to modify the shaders slightly. Right now, the shaders expect boneMatX, boneMatY, etc., to map vertices from/to the local space of SkinnedMesh. Then the shaders apply the model-view transform of the SkinnedMesh:

vec4 skinVertex = vec4( position, 1.0 ); // skinVertex is in the local space of the mesh
vec4 skinned = vec4( 0.0 );
skinned += boneMatX * skinVertex * skinWeight.x;
skinned += boneMatY * skinVertex * skinWeight.y;
mvPosition = modelViewMatrix * skinned;

Instead, if the shader expects boneMatX, boneMatY, etc., to map vertices from/to world space, we can do something like this:

vec4 skinVertex = skinnedMeshBindMat * vec4( position, 1.0 ); // skinVertex is in world space
vec4 skinned = vec4( 0.0 );
skinned += boneMatX * skinVertex * skinWeight.x;
skinned += boneMatY * skinVertex * skinWeight.y;
mvPosition = viewMatrix * skinned;

In this case, we have decoupled the SkinnedMesh transform (skinnedMeshBindMat) from boneMatX, boneMatY, etc. In turn, this would allow us to store boneMatrices in Skeleton, because they would no longer depend on the SkinnedMesh transform. We would calculate boneMatrices as follows:

bones[b].matrixWorld * boneInverses[b]

where boneInverses[b] is set to inverse(bones[b].matrixWorld) at bind time.

With this approach, we could then have multiple SkinnedMesh instances animated by a single Skeleton, which would be great. If you think this is a good idea, I’d be happy to amend the code in this pull request to do just that.

This post ended up being longer than I planned, but hopefully it makes sense. Please let me know if you have questions or suggestions.

@crobi
Copy link
Contributor

crobi commented May 15, 2014

Your suggestion with doing skinning in world space costs one more matrix multiplication per vertex and gives you the flexibility to reuse the bone matrices for different meshes, I like that.
It is also the same as the approach in COLLADA files. Their equation is (summed over i):

weight[i] * bone[i].world_matrix * bone[i].inverse_bind_matrix * bind_shape_matrix * vertex

I assume you can also set your boneInverses matrices manually (without calling bind()), in case you are loading a skinned mesh from a file (COLLADA files explicitely store the bone inverse matrices)?

Another thing, apart from storing the bone matrices, a Skeleton class should also just store a list of all bones used in the skeleton. Or maybe even a map that maps bone names to bone objects.
Imagine an application (e.g., a web-based game) where you interactively instantiate skinned meshes (e.g. spawn monster characters). For this, you need a geometry with a skeleton "template", which stores the hierarchy of bones, detached from the scene graph. When instantiating a skinned mesh, you create a Skeleton object from the template and insert the created bones into the scene graph. When playing back an animation, you use the Skeleton object to find out which objects the animation should target (for this, bone names could be useful). When deleting the mesh instance (e.g., after the monster has been vaporized), you use the Skeleton object to remove all bone objects from the scene graph.
This convenience functionality is independent from storing the matrices in the Skeleton class and could be implemented without changing the skinning equation.

Ian Kerr added 4 commits May 21, 2014 09:45
…sformations

from the transforms that bind a THREE.Skeleton to a THREE.SkinnedMesh.  This
allows us to animate multiple THREE.SkinnedMesh instances with a single
THREE.Skeleton.

Here is a summary of the major changes:

extras/helpers/SkeletonHelper.js
* Fixed code mistakes (missing 'THREE').

objects/Bone.js
* Removed duplicate code that is already in THREE.Object3D.

objects/Skeleton.js
* Not a descendant of THREE.Object3D (previous version was).
* Stores boneMatrices and boneTexture.
* Independent of THREE.SkinnedMesh.

objects/SkinnedMesh.js
* Refactored boneMatrices and boneTexture related code into THREE.Skeleton.
* Added two bind matrix modes: "attached" and "detached".

renderers/WebGLRenderer.js
renderers/webgl/WebGLProgram.js
* Attach bindMatrix and bindMatrixInverse to shaders.

renderers/shaders/ShaderChunk.js
renderers/shaders/ShaderLib.js
* Use bind matrices during skinning.
@ikerr
Copy link
Contributor Author

ikerr commented May 21, 2014

Hi @crobi, @mrdoob,

Please see the most recent code. I have re-introduced THREE.Skeleton and decoupled it from THREE.SkinnedMesh. This will allow us to animate multiple meshes with a single skeleton. The mesh-specific bind matrices are stored in THREE.SkinnedMesh and used by the vertex shader to transform to/from "bind space" (world space in the current implementation).

The previous version of THREE.Skeleton created the skeleton hierarchy, which I have moved back to THREE.SkinnedMesh. I see this as application-specific (or at least high-level functionality) and would rather not have it in THREE.Skeleton. For example, the hierarchies that I deal with are often mixes of THREE.Bone, THREE.Mesh, THREE.Light, etc. I would like to remove this behaviour from THREE.SkinnedMesh as well, but I have left it there for backwards compatibility.

In the future, it might be nice to have a THREE.Model class that would provide convenience functions for loading files (FBX, Collada), attaching skeletons and animations as well as providing a logical container for node hierarchies. I think this was partly the intent of THREE.SkinnedMesh; however, the THREE.SkinnedMesh approach imposed the limitations that led to this pull request.

Anyways, let me know what you think and if you have any suggestions, questions or comments.

@ikerr
Copy link
Contributor Author

ikerr commented May 29, 2014

Thoughts?

Ian Kerr added 3 commits June 18, 2014 06:48
@ikerr
Copy link
Contributor Author

ikerr commented Jun 18, 2014

I just updated the branch, merging it with the current version of "dev" and removing the built files (three.js and three.min.js).

I'd love to get this pulled into dev, if possible. Please let me know if there's anything I can do to help, if any changes are required, or if you have any questions. Thanks.

@mrdoob
Copy link
Owner

mrdoob commented Jun 24, 2014

I'll have a proper read on this one later this week. Sorry for the delay!

@mrdoob
Copy link
Owner

mrdoob commented Jul 7, 2014

Sorry for this, but could you try merging with the latest dev?

Ian Kerr added 2 commits July 7, 2014 08:42
Conflicts:
	src/objects/Skeleton.js
	src/renderers/shaders/ShaderChunk.js
@ikerr
Copy link
Contributor Author

ikerr commented Jul 7, 2014

Yup, should be merged now.

mrdoob added a commit that referenced this pull request Jul 11, 2014
@mrdoob mrdoob merged commit f0a2a84 into mrdoob:dev Jul 11, 2014
@mrdoob
Copy link
Owner

mrdoob commented Jul 11, 2014

Thanks for your patience! ^^

@EskelCz
Copy link

EskelCz commented Jul 11, 2014

Just for clarification, is this a potentially breaking change? Or will the current skinned meshes work the same with current parameters?
If not, it would be nice to have a short post how to work with it. Thank you ! :)

@ikerr
Copy link
Contributor Author

ikerr commented Jul 11, 2014

@mrdoob - Great, thanks!

@EskelCz - I did my best to maintain backwards compatibility with the previous usage of SkinnedMesh. All of the example applications ran as-is, for example. It's possible that missed something though, so please let me know if you run into any differences and I'll let you know how to update your code.

@mrdoob
Copy link
Owner

mrdoob commented Jul 15, 2014

@ikerr I think this change broke glTFLoader:
https://github.com/mrdoob/three.js/blob/dev/examples/webgl_loader_gltf.html

Do you mind taking a look?

@ikerr
Copy link
Contributor Author

ikerr commented Jul 16, 2014

@mrdoob Thanks, taking a look now. I'll let you know when I have a fix.

ikerr pushed a commit to ikerr/three.js that referenced this pull request Jul 16, 2014
@ikerr
Copy link
Contributor Author

ikerr commented Jul 16, 2014

@mrdoob Here is a new pull-request with the fix:

#5075

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.

None yet

4 participants