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 SkeletonModification3Ds to work with the new bone pose changes. #53821

Conversation

TwistedTwigleg
Copy link
Contributor

@TwistedTwigleg TwistedTwigleg commented Oct 14, 2021

Closes #53794
Edit: Closes #53895 as well, see comments below

Most of the fixes were pretty easy and the majority of the IK modifications work now without any issues. However, FABRIK is providing difficult due to changes involving bone positions, which is causing it not to work.

It seems that the previous method of setting the local_pose_override position to Vector3(0, 0, 0) no longer works for setting the bone at its default local position, which was causing it not work. Using the global pose without overrides as the position sort of works, but it's not ideal nor fully working right now.
I have some ideas on what might work to solve it, but haven't had the time to investigate yet. I'll try to look at this further soon though.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 14, 2021

@TwistedTwigleg The default transform of the pose has been changed to "rest" instead of Transform(). For example, in the case of default position, it is rest.origin instead of Vector3(0, 0, 0). One problem is that we decided on the concept that to set up a pose, it needs to go through the TRS(position, rotation, scale) channel, so instead of applying the rest or any Transform3D value directly to the pose, it needs to be decomposed into the TRS channel as needed.

@TwistedTwigleg
Copy link
Contributor Author

TwistedTwigleg commented Oct 15, 2021

@TwistedTwigleg The default transform of the pose has been changed to "rest" instead of Transform(). For example, in the case of default position, it is rest.origin instead of Vector3(0, 0, 0). One problem is that we decided on the concept that to set up a pose, it needs to go through the TRS(position, rotation, scale) channel, so instead of applying the rest or any Transform3D value directly to the pose, it needs to be decomposed into the TRS channel as needed.

Oh okay! I'll quickly give rest.origin a try and mess around with that and see if that fixes it. Thanks for letting me know how it works now, I appreciate it 👍

Edit: Using rest.origin didn't fix it with the quick testing I did, but I will look into it further once I can.

@fire
Copy link
Member

fire commented Oct 15, 2021

Another way of saying what @TokageItLab said was:

If you start from a Godot 3.x pose in this system, we can just multiply by rest.affine_inverse().

Like new_pose = rest.affine_inverse() * old pose.

old_pose = rest * new_pose.

@TwistedTwigleg TwistedTwigleg force-pushed the Godot_Master_SkeletonModificationIK_FixPoseChange branch from 7130b29 to d40a0d5 Compare October 15, 2021 22:21
@TwistedTwigleg
Copy link
Contributor Author

Okay, FABRIK is fixed now! I had to change how it works so that it uses global poses instead of local ones, as well as making it save the transforms internally. This does have some benefits though, as now the code is much more compact, less conversions are needed (more performance), and it's easier to understand.
For reasons mentioned in more detail below, I also added some code to the modification stack so the global pose override is reset when the modification is disabled.

With these changes done, I'm confident that the IK modifications in 3D should be back to fully working and have removed the draft status on this PR.


However, while working on this I found what I believe was the actual culprit: the global_pose_to_local_pose function is broken! The changes in #53765 seems to have altered the function where it no longer works, at least with positions.

For example, the following line of code is how this PR applies the global pose to the skeleton (line 288):

stack->skeleton->set_bone_global_pose_override(current_bone_idx, current_trans, stack->strength, true);

The transform passed, current_trans, is a valid global pose transform and it works completely as expected when passed to set_bone_global_pose_override.

However, the follow line of code doesn't work, even though it would have worked prior to the changes in #53765

stack->skeleton->set_bone_local_pose_override(current_bone_idx, stack->skeleton->global_pose_to_local_pose(current_bone_idx, current_trans), stack->strength, true);

Which is a problem, because it means that valid global poses are not being correctly converted to local poses, meaning the global_pose_to_local_pose function is broken. I took a look at the function in Skeleton.cpp to try and fix it, but my experiments didn't yield proper behavior.

While it's not a blocking issue for this PR and the skeleton modification system currently, I just changed the FABRIK code to work entirely in global pose space and to cache the transforms it needs, it needs to be fixed. I can make a new issue for this if needed though.

@TwistedTwigleg TwistedTwigleg marked this pull request as ready for review October 15, 2021 22:33
@fire
Copy link
Member

fire commented Oct 15, 2021

I can make a new issue for this if needed though.

Please do!

@TwistedTwigleg
Copy link
Contributor Author

Will do! I’ll get a quick replication project and issue made this weekend 👍

@TwistedTwigleg TwistedTwigleg force-pushed the Godot_Master_SkeletonModificationIK_FixPoseChange branch from d40a0d5 to 0cedc04 Compare October 16, 2021 20:35
@TwistedTwigleg TwistedTwigleg requested a review from a team as a code owner October 16, 2021 20:35
@TwistedTwigleg
Copy link
Contributor Author

I fixed the issue with the global_pose_to_local_pose function, so now it works as expected again! I also reset the stack modification disable code, so it only disables the local poses. Should fully close #53895 and everything IK related should be working as expected again!

@akien-mga akien-mga merged commit c2a616f into godotengine:master Oct 16, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

global_pose_to_local_pose function in Skeleton3D node not working properly Regression on master on Godot ik.
5 participants