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

Fix for Skinned Mesh #9

Merged
merged 1 commit into from
Mar 10, 2019
Merged

Fix for Skinned Mesh #9

merged 1 commit into from
Mar 10, 2019

Conversation

sneha-belkhale
Copy link
Contributor

@sneha-belkhale sneha-belkhale commented Feb 16, 2019

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.

Copy link
Owner

@jsantell jsantell left a comment

Choose a reason for hiding this comment

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

Great find & thanks for filing! Does this affect all FBX files/skinned meshes? The skinned-mesh example doesn't seem to work well for me; the last bone seems to just twitch -- some comments inline that I suspect could be the culprit. The current examples also act strangely (the bones begin to fold in on each other), which are only regular mesh/geometry -- what direction do the bones point for SkinnedMesh/Skeleton? It's been a while since I've dug into this code so I'm a bit rusty, but interested in learning more

const chain = new IK.IKChain();
var currentBone = boneGroup
for (let i = 0; i < 4; i++) {
currentBone = currentBone.children[0]
Copy link
Owner

Choose a reason for hiding this comment

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

currentBone doesn't appear to change over the loop, setting multiple joints to the same bone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, if you print out the joint array at the end, each joint as a bone with a unique UUID. I've noticed this kind of bug with console.logging before, I think it's a real issue :0

https://code-maven.com/logging-javascript-objects


update() {
if(this.target){
this.target.position.z = 1+Math.sin(0.01*Date.now())
Copy link
Owner

Choose a reason for hiding this comment

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

performance.now() is probably more appropriate here

@sneha-belkhale
Copy link
Contributor Author

sneha-belkhale commented Feb 26, 2019

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?

if ((this.angle / 2) < currentAngle) {

@sneha-belkhale
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

sneha-belkhale commented Feb 26, 2019

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
Copy link
Owner

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 (#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
Copy link
Contributor Author

sneha-belkhale commented Feb 26, 2019

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
Copy link
Contributor Author

sneha-belkhale commented Feb 26, 2019

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
Copy link
Contributor Author

sneha-belkhale commented Feb 26, 2019

@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
Copy link

arpu commented Feb 27, 2019

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
Copy link
Contributor Author

sneha-belkhale commented Feb 27, 2019

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
Copy link

arpu commented Feb 28, 2019

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
Copy link
Contributor Author

sneha-belkhale commented Feb 28, 2019

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
Copy link

arpu commented Feb 28, 2019

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
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link

arpu commented Mar 2, 2019

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

@jsantell
Copy link
Owner

jsantell commented Mar 2, 2019

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 😄 )

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
Copy link
Owner

jsantell commented Mar 2, 2019

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.

  • For joints with multiple children, the forward can be the average of all the children.
  • (still non-existent) Hinge constraints can be defined in terms of three.js axes; no magic happening on the IK end.
  • Ball constraints, currently defined relative to the parent's +Z/forward vector, can continue being defined relative to the parent's forward vector, which is now the same as its initial orientation
  • For scenarios where joints aren't lined up on an axis, that's a model/exporting/rigging problem, although should still potentially work, although defining a useful constraint will probably be challenging.

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Owner

jsantell commented Mar 9, 2019

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 #11 (and committed for gh-pages); could you squash and rebase your commits then with only the new example/assets?

… axis conversion utility to convert models to +Z Forward coordinate system
@sneha-belkhale
Copy link
Contributor Author

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

@jsantell
Copy link
Owner

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!

@jsantell jsantell merged commit b0df479 into jsantell:master Mar 10, 2019
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