jsantell / THREE.IK

inverse kinematics for three.js
https://jsantell.github.io/THREE.IK
MIT License
455 stars 54 forks source link

Fix for Skinned Mesh #9

Closed sneha-belkhale closed 5 years ago

sneha-belkhale commented 5 years ago

Hey @jsantell ,

Firstly, thanks for making this repo ~~

Secondly, I looked into integrating the three.ik system with the three.js skinned meshes / skeletons. Things looked really disastrous on the first try, the vertex points were completely out of place causing the bones to look dislocated.

It was pretty clear that there was a discrepancy between the direction of the IK.Joints and the direction of the three.js bones. It may not have been apparent to you if you were testing using the helpers, because the helpers are designed relative to the direction between the IK.Joints anyways.

I've added a fix to the src files in this PR that addresses this fix by using the correct direction when applying the transformation to the three.js bone -- along with a small example for testing.

Let me know what you think.

If it looks alright, I can make a better example and we can add it to the page.

sneha-belkhale commented 5 years ago

Hey, thanks for checking out the fix. So I looked into it again and yes the other examples seem to act strangely, with the constraint of 90. However when you increase the constraint to 360, the examples are identical to the original.

Ideally this constraint should be 180 degrees.. is there a reason why in the constraint code you use this.angle / 2 instead of the raw angle?

https://github.com/jsantell/THREE.IK/blob/d502845fda818e8787f8b7f09208804449149b65/src/IKBallConstraint.js#L38

sneha-belkhale commented 5 years ago

I also just pushed a commit that makes the skinned-mesh example smoother. Basically increased the constraint to 360, which as I mentioned above actually functions as a 180 degree constraint.

jsantell commented 5 years ago

Ideally this constraint should be 180 degrees.. is there a reason why in the constraint code you use this.angle / 2 instead of the raw angle?

THREE.IK/src/IKBallConstraint.js

Line 38 in d502845 if ((this.angle / 2) < currentAngle) {

For a ball joint, 90 degrees of rotation is 45 degrees from the normal in any direction

jsantell commented 5 years ago

The skinned mesh example looks like it's working now, thanks! The previous examples still are broken though, resulting in bones crossing back on each other:

screenshot from 2019-02-25 21-42-42

screenshot from 2019-02-25 21-46-52

Changing the constraint to 360 allows full rotation of the joints, as expected, but then the constraint isn't doing anything

sneha-belkhale commented 5 years ago

For a ball joint, 90 degrees of rotation is 45 degrees from the normal in any direction

Hm, but I think the angle you are calculating is the full angle between the direction and parent direction, not the angle from the normal

sneha-belkhale commented 5 years ago

In the images you just posted it looks to me like there is 45 degrees between parent/child directions, which would correspond to a 90 degree constraint parameter in the current implementation -- although I think it would make more sense to have this correspond to a 45 degree constraint parameter

jsantell commented 5 years ago

In the images you just posted it looks to me like there is 45 degrees between parent/child directions, which would correspond to a 45 degree constraint?

In the images, the blue lines indicate forward/+Z -- along the plane of a child bone, the angles are > 90 degrees. This patch also changes the axis of the child bone (https://github.com/jsantell/THREE.IK/issues/1) to -Z. Is the issue your facing from FBX loader using a different forward axis for the bones?

If this were a hinge constraint, it looks like it'd be around 45 degrees in those examples -- a ball/socket constraint (AFAIK) however is total range of motion, similar to field of view (e.g. a VR headset with 120 degrees of horizontal FOV is 60 degrees to the left and 60 degrees to the right). Our shoulders, for example, have a range of movement of around 180 degrees, able to be rotated around a hemisphere.

sneha-belkhale commented 5 years ago

Ahh I see makes sense, yeah I was thinking of it more like a hinge constraint.

So the main issue is that the skinned mesh seems to bind to the mesh using -Z forward. This is what the mesh binding looks like without the z flipping.

screen shot 2019-02-26 at 11 19 32 pm
sneha-belkhale commented 5 years ago

The previous examples still are broken though, resulting in bones crossing back on each other:

So it seems like the constraint angle calculation is messed up by the z flipping, because after flipping the parent bone, it's flipped direction is used to see if a constraint should be applied to its child.

I can think of some ways to fix this, but want to check with you first if there is an obvious solution :)

sneha-belkhale commented 5 years ago

@jsantell , So I went ahead and just flipped the z axis permanently to be consistent with the skinned mesh. All examples seem to be restored.

Let me know if changing the schema to -Z is okay. I think it should be fine for now since there are no other examples that have +Z skinning. In the future maybe this could be parameterized.

arpu commented 5 years ago

i try this for some time without this patch from this PR https://uncovered-produce.glitch.me/

using a skinnedMesh gltf model

bildschirmfoto von 2019-02-27 01-17-34

the source is here https://glitch.com/edit/#!/uncovered-produce

would be realy cool some one could look at this

sneha-belkhale commented 5 years ago

hi @arpu,

I checked it out. There are a few things I noticed - Firstly, the joints were being iterated through incorrectly. You have to start from each limb origin, say the hip bone, and iterate down it's children until the toe, and then add the target to the toe.

I added that here: https://glitch.com/edit/#!/quiver-tune

now it seems like the IK is working when you move the target around. However the mesh does not look like its binded correctly. Have you used this model before?

arpu commented 5 years ago

hi @sneha-belkhale

wau thx a lot now this all makes much more sense for me, thx a lot for the hint

for now for me it looks like the bone IK scaling is wrong https://glitch.com/edit/#!/ubiquitous-target?path=cool-file.js

sneha-belkhale commented 5 years ago

for now for me it looks like the bone IK scaling is wrong

add this line~ this.model.updateMatrixWorld(true)

https://glitch.com/edit/#!/quiver-tune

still not sure what's going on with the mesh bindings, my guess is that the skeleton bind matrices were not imported correctly..

arpu commented 5 years ago

awesome! but yeahh looks not complete right :> ihave done some cleanup https://hilarious-meat.glitch.me/

the strange is i cannot see the IKHelper Bones anymore

jsantell commented 5 years ago

I'm not sure I follow, but is the issue the FBX model uses -Z as it's forward direction? Trying a handful of skeletons, there were no consistent patterns I've seen (others noted in #1), and it doesn't appear to be specific to SkinnedMesh (correct me if I'm wrong!)

If that's the case, then maybe a better solution could be a utility to convert the axes of the bones to a consistent direction. Thoughts?

sneha-belkhale commented 5 years ago

@jsantell , So I just looked into the FBXloader / SkinnedMesh code and it seems like neither of these are responsible for bone orientation. Which means that it must be set by the convention of the modeling software.

The model I am testing with right now is exported from HoudiniFx, but as you mentioned, it's not a consistent pattern with other softwares.

I'm not sure if a utility to convert the axes of the bones would work, because if you flip the bones you would also need to flip the skinned weight coefficients, right?

I feel like a less messy option would be to just add a parameter to the IK constructor.

arpu commented 5 years ago

sorry for the noise in this thread, will open a new Issue

jsantell commented 5 years ago

I'm not sure if a utility to convert the axes of the bones would work, because if you flip the bones you would also need to flip the skinned weight coefficients, right?

Weights are mapping from vertices to bones, so those shouldn't change I think. Changing coordinate systems/orientations is a surprisingly difficult task AFAIK in the general sense, although some are easier than others (e.g. -Z -> +Z). While not a coordinate system, the same idea applies; could the IK system get away with not having an opinion about this, and use whatever is provided? (I haven't looked at the code in a bit, but have learned a lot of things over the last few months :smile: )

I feel like a less messy option would be to just add a parameter to the IK constructor.

I think this would still require a coordinate mapping conversion, no?

jsantell commented 5 years ago

Note: this is a good resource on coordinate system remapping although unsure if it's applicable for rotations, and the closest thing I've ever seen for a generalizable formula.

Looking at more rigs and the code, an idea: what if the joints use their children's position to determine what the forward is by storing the joint's initial rotation? Maya has +X, this FBX here has -Z, openGL/three use +Z, etc., I don't think a solution that requires any specific direction (even +Z) will work without some sort of coordinate system casting, which should already be handled at the model level.

That might work for the generalized case? There are a lot of implicit assumptions about forward in the codebase which would have to be changed. What do you think? @arpu @sneha-belkhale

sneha-belkhale commented 5 years ago

@jsantell Ah yes you are right about the weight mapping -- i'm pretty new to skinning so still trying to wrap my head around some of these concepts :)

I feel like a less messy option would be to just add a parameter to the IK constructor.

I think this would still require a coordinate mapping conversion, no?

What I meant by this was a parameter to notify that the bones were modeled in a -Z forward coordinate system. If so, we could just apply the fixes that I've made in this PR. However, this obviously would not generalize to any coordinate system so probably not a good solution.

I think the approach you have described sounds reasonable, should keep things consistent. I just did a test where I go through the bone hierarchy and align the parent quaternion so that it's forward is pointing towards the child position.

screen shot 2019-03-03 at 9 36 52 pm

In the screenshot you can see that the blue lines of the bones are facing the next bone, so it seems to be implemented correctly. However there is clearly still a binding issue. I think because the bind matrices given with the model are no longer accurate.

Oh and this image is after I tried skinnedMesh.calculateInverses(), to attempt to update the bind matrices.

sneha-belkhale commented 5 years ago

@jsantell

Hey, so spent some time debugging the issue above and turns out there was a bug in my conversion code. I fixed it and seems like the binding matrices have updated correctly!

screen shot 2019-03-04 at 1 44 12 pm

I'll push the conversion utility soon, currently only works on a single chain skeleton, but should be good enough to start a review.

sneha-belkhale commented 5 years ago

Hey, just for an update on this, the utility has worked for the example, my project, and issue #10, which are from HoudiniFX and Blender. Probably should test a model from Maya as well.. I just don't have one.

So for this pull request, we no longer need to change any of the existing code. Instead, the utility and the example of how to use it should do. @jsantell , let me know what you think.

jsantell commented 5 years ago

Glad to see the Z flip is working well! Until landing some generalized way of handling the orientations in the core, having this utility available to fix -Z models will be valuable. Thank you!

I added the dependencies to be managed by npm and updated three.js to latest r102 and made the appropriate changes https://github.com/jsantell/THREE.IK/pull/11 (and committed for gh-pages); could you squash and rebase your commits then with only the new example/assets?

sneha-belkhale commented 5 years ago

@jsantell awesome, the PR now has one commit with the asset, example, and utility. Thanks a lot for your suggestions on this!

jsantell commented 5 years ago

Thanks @sneha-belkhale! Looking great, let's get this merged in -- thanks for your patience, this solution looks great for folks wanting to use this with practical, real models!