-
Notifications
You must be signed in to change notification settings - Fork 476
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
Skinned animation and node animation coordinate space bugfixes #146
Conversation
Doesn't compile quite yet Porting from this PR: https://github.com/KhronosGroup/UnityGLTF/pull/43/files
Coordinate spaces are a mess though
This reverts commit 4ad9af2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great change John! Just left a couple of comments.
return "WEIGHTS_" + index; | ||
} | ||
|
||
public static string Joint(int index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
string primitiveObjPath = relativePath + "/Primitive" + primitiveIndex; | ||
for (int targetIndex = 0; targetIndex < targetCount; targetIndex++) | ||
{ | ||
//clip.SetCurve(primitiveObjPath, typeof(SkinnedMeshRenderer), "blendShape." + targetIndex, curves[targetIndex]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove, leave comment, or reactivate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a TODO to add blend shapes/morph targets. I 'm going to do that in a separate PR since blendshapes have more than 4 curves and will need some re-jiggering of the animation construction code
animation.AddClip(clip, clip.name); | ||
if (i == 0) | ||
{ | ||
animation.clip = clip; | ||
} | ||
//animation.Play(); | ||
|
||
animation.Play(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return primitive.Targets != null; | ||
} | ||
|
||
void SetupBones(Skin skin, MeshPrimitive primitive, SkinnedMeshRenderer renderer, GameObject primitiveObj, UnityEngine.Mesh curMesh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
access modifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the above as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (for all methods, they are private)
|
||
GLTFHelpers.BuildBindPoseSamplers(ref attributeAccessor); | ||
|
||
GLTF.Math.Matrix4x4[] gltfBindPoses = attributeAccessor.AccessorContent.AsMatrix4x4s;//.ToUnityMatrix4x4sConvert(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is commented out at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops - I moved that to the for loop below it and forgot to remove it. It's gone now
return boneWeights; | ||
} | ||
|
||
void MakeSureWeightsAddToOne(Vector4[] weights) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better name? WeightNormalize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some comments here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the normalize name you suggested is better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
var skinnedMeshRenderer = primitiveObj.AddComponent<SkinnedMeshRenderer>(); | ||
skinnedMeshRenderer.material = material; | ||
skinnedMeshRenderer.quality = SkinQuality.Bone4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be a modifiable property on the script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that adding the skinnedmeshrenderer should be modifiable? I think it's correct to only add it if the mesh has a skin. We don't have switches for other things like whether we add a material or not, so I think this is a similar situation
I did realize after you pointed out this code that the mesh filter is only added to the static objects, so I've fixed that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No meant whether the skinquality should be
skinnedMeshRenderer.material = material; | ||
skinnedMeshRenderer.quality = SkinQuality.Bone4; | ||
/* | ||
if (HasBlendShapes(primitive)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment or remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a TODO to add blend shapes/morph targets. I 'm going to do that in a separate PR since blendshapes have more than 4 curves and will need some re-jiggering of the animation construction code
SetupBlendShapes(primitive); | ||
*/ | ||
if (HasBones(skin)) | ||
SetupBones(skin, primitive, skinnedMeshRenderer, primitiveObj, curMesh); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yield after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to yield to the call and yield properly in SetupBones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to coroutine as suggested
Alright! All PR feedback addressed |
…osGroup#146) * First attempt at getting animation working Doesn't compile quite yet Porting from this PR: https://github.com/KhronosGroup/UnityGLTF/pull/43/files * Little more progress * Bind pose access is functional! * First end-to-end loading of animations and bindpose Coordinate spaces are a mess though * ConstructUnityMesh isn't populating mesh.boneWeights yet... * Still not working... * Trying some ideas from a different PR * Revert "Trying some ideas from a different PR" This reverts commit 4ad9af2. * First working skinning! * Fixed inverted transforms in animation * Fixing issue where too many bones were named GLTFNode and caused naming conflicts * Code cleanup * More code cleanup * Defaulting animation to play * Clips loop by default * Reverting files before checkin * Reverting files before checkin * Addressing PR feedback
Adding support for skinned mesh animation, as well as bugfixing some issues in the animation construction code around coordinate space conversions