Fix hips world position update #1057
Conversation
| @@ -130,7 +131,8 @@ export class VRMHumanoidRig extends VRMRig { | |||
|
|
|||
| // Move the mass center of the VRM | |||
| if (boneName === 'hips') { | |||
| const boneWorldPosition = rigBoneNode.getWorldPosition(new THREE.Vector3()); | |||
| const boneWorldPosition = rigBoneNode.getWorldPosition(_boneWorldPos); | |||
0b5vr
requested review from
0b5vr and
ke456-png
and removed request for
0b5vr
| @@ -130,7 +131,8 @@ export class VRMHumanoidRig extends VRMRig { | |||
|
|
|||
| // Move the mass center of the VRM | |||
| if (boneName === 'hips') { | |||
| const boneWorldPosition = rigBoneNode.getWorldPosition(new THREE.Vector3()); | |||
| const boneWorldPosition = rigBoneNode.getWorldPosition(_boneWorldPos); | |||
| boneNode.parent!.matrixWorld.multiplyMatrices(boneNode.parent!.parent!.matrixWorld, boneNode.parent!.matrix); | |||
There was a problem hiding this comment.
I believe updateWorldMatrix(false, false) says the almost same thing.
also, why we don't need to do updateWorldMatrix(true, false) here?
There was a problem hiding this comment.
Yes updateWorldMatrix(false,false) does almost the same thing but it add an extra updateMatrix that isn't necessary here.
I don't remember exactly why but it would say that the parent matrixWorld is already up to date and correct.
calling the method would trigger some unnecessary matrix updates and impact the perf.
There was a problem hiding this comment.
calling the method would trigger some unnecessary matrix updates and impact the perf.
yes indeed but we want to make sure that we update all the necessary matrices here.
I just have built a breadboard of the specific case and now I'm 95% sure it has to be updateWorldMatrix(true, false):
There was a problem hiding this comment.
Mmmmm something feels wrong for me in this example.
I'll check again by the end of the day if you don't mind waiting.
I'll explain fully why I think it's not necessary to use updateWorldMatrix(true, false) in a normal way of updating a VRM and then you will be able to tell me if you agree or not. 🙇
There was a problem hiding this comment.
take your time, I'll wait for you 👍
In my understanding, world matrices of objects in a scene usually update upon renderer.render, but we need the world matrix before the render call, so we need to update the ancestors of the boneNode.
There was a problem hiding this comment.
Ok I think you're right!
In my test case I was moving vrm.scene, I thought it was the only "correct" way to move a skinnedMesh
I tried your example using vrm.scene.position.x = 1.0; instead of moving the hips and my solution works.
But I didn't consider that hips could move too and in that scenario my solution breaks.
I'm also still a bit confused about the difference between hipsNormalized and hipsRaw and some other aspects of VRM so I won't dig deeper for now.
I updated the PR accordingly 👍
This PR is sponsored by Sougen
Thanks for this great repo! :)
I noticed an issue in the VRMHumanoidRig update method causing some tiny flicker in the VRM position.
Check this video to see the before/after
https://user-images.githubusercontent.com/7174039/192105285-aa55e9ec-3124-436e-8ef9-9d972bff54e1.mp4
Top VRM use my fix, bottom VRM is the current version.
Both VRM are synchronised with the purple line position but as you can see, the bottom VRM is not at the correct position.
Updating the matrixWorld before using it fix this issue.
I also made a modification to avoid a needless instantiation.
Let me know if you need more info/edits.