[3.x] Fixed issue in SkeletonIK leading to some root bones being twisted incorrectly

[TwistedTwigleg]

I think I have finally fixed the issue in the SkeletonIK node leading to the root bone being twisted when rotated. This PR also combines the fix in #48166 - where the for checking the bone forward direction was not working correctly.
(I will port the fix in this PR to #48166)

I tested the minimum replication projects in #48078 and #47803, and both projects seem to be fixed in my testing. However, additional testing is appreciated to make sure the fix in this PR truly does solve the issue.

Closes #48078
Closes #47803

[akien-mga]

CC @macryc @yaelatletl @Naurk @wojtekpil New try :)

You can download artifacts from the CI build of this 3.x PR to test it:

• Linux: https://github.com/godotengine/godot/runs/2452423028
• Windows: https://github.com/godotengine/godot/runs/2452422871

Specifically with the "Artifacts" menu here:

[macryc]

CC @macryc @yaelatletl @Naurk @wojtekpil New try :)

You can download artifacts from the CI build of this 3.x PR to test it:

• Linux: https://github.com/godotengine/godot/runs/2452423028
• Windows: https://github.com/godotengine/godot/runs/2452422871

Specifically with the "Artifacts" menu here:

Thanks @akien-mga. It does seem to fix the issue partially. The bone isn't twisted by 180 degrees any more but it's still twisted to the left by about 20-30 degrees or so. See attached screen shot:

[Naurk]

Thanks for your work @akien-mga : like macryc it seems to work better, but there's some issue yet and I don't understand completely the behaviour, but it is still different from 3.2.3 and previous.

[yaelatletl]

Artifact
It untwisted the bones that were twisted but seemed to change how bones behave, those shoulders have tracking behaviour

RC 4

[TwistedTwigleg]

Okay, I think I found a new, better solution. It has a few unique pros and cons, but ultimately I think its the best fix so far.
@macryc @Naurk @yaelatletl - can you all try the latest build artifact to see if it works for you.

Below is an explanation of the change, but the TLDR is that I made a minor adjustment to the Skeleton node and now it can use the previous rotation code, which should fix the regression while still working with BoneAttachment nodes.

---

With the change, I reverted the code back to how it was prior to fix, where it wasn't working with BoneAttachment nodes but it was otherwise working okay. Instead of trying to rotate the root bone or apply a patch that way, I decided that perhaps the best way to work around this issue is to instead change the code in SkeletonIK so it no longer resets the global pose override multiple times to get the global pose without any overrides, which was the issue causing it to not work with BoneAttachment nodes.

To do this, I added a new transform to the bone struct in the Skeleton node called global_pose_no_override, which is simply the global pose of the bone without any bone overrides applied. This does have the minor con of meaning that the code can no longer take a short cut when the bone global pose override is over 0.999, as we need to calculate the global pose without overrides. So, this may lead to slightly lower performance for projects that have lots of bone global pose overrides with interpolations over 0.999, but I do not expect it to be noticeable.
I was going to add this anyway in the 3D IK PR, so this PR will just be adding it a touch earlier anyway. Once that PR is merged in, the performance loss for calculating the bone global pose without overrides should be even lower, as only the bones that actually changed will be calculated.

For SkeletonIK itself, it actually should be a little faster! Now the code doesn't force the Skeleton to reset all its transforms, recalculate all the transforms, cache the global poses without overrides, and then recalculate the poses again with overrides. Now it just gets the global poses without overrides, and then calculates the poses with overrides, skipping an entire recalculation cycle.
Additionally, because it is using the previous SkeletonIK code for rotation, all of the regressions and differences in how it calculates the bone angles and rotation should be fixed as well!

Ultimately, I think its a positive change with very few cons. Hopefully this will be the final change needed to fully fix the SkeletonIK regressions found.

[macryc]

Okay, I think I found a new, better solution. It has a few unique pros and cons, but ultimately I think its the best fix so far.
@macryc @Naurk @yaelatletl - can you all try the latest build artifact to see if it works for you.

Hi @TwistedTwigleg , thanks for this and the explanation, I'm keen to try this new build but the only one I can find is the previous one from 5 days ago. Could you post a link to the new one?

[akien-mga]

Click on the green tick next to the last commit to get a list of CI runs, and then pick the one that matches your platform to access artifacts:

[macryc]

@TwistedTwigleg , just tested the fix and the root bone seems to be working correctly, however somehow the fix has had an effect on the interpolation property. Basically, interpolation has now become all-or-nothing. The property has no effect between 0-0.01, and 100% from 0.01 - 1.

[TwistedTwigleg]

Okay, awesome! I'm glad the root bone issue is fixed. I have a hunch that the interpolation issue is just somewhere the code is using the global pose with overrides rather than without, so fingers crossed its just that and easy to fix. I'll take a look once I can and see if I can fix the issue.

Thanks for testing, I really appreciate it!

[Naurk]

I've done a quick test, but it seems like macryc said: the root bone issue seems fixed and the interpolation is now just 0 or 1, but this is the way! I'll do some extensive comparative tests nex days. Ty! :)

[TwistedTwigleg]

I think I fixed the interpolation bug, at least its fixed on my end. 👍

I think this PR should be good to go now. If you want, I can squash the previous commit so this is just one (since the stuff in the first commit didn't work), just let me know and I'll tackle it this weekend. I'll also port the code to the master branch this weekend as well.

[akien-mga]

Awesome work! If that works for everyone we should be nearly ready for 3.3.1 RC 1 :)

To pass CI you'll have to add the documentation for the new method, we recently added a check that makes it a requirement:

diff --git a/doc/classes/Skeleton.xml b/doc/classes/Skeleton.xml
index 244cb29..fe4e909 100644
--- a/doc/classes/Skeleton.xml
+++ b/doc/classes/Skeleton.xml
@@ -80,6 +80,14 @@
 				Returns the overall transform of the specified bone, with respect to the skeleton. Being relative to the skeleton frame, this is not the actual "global" transform of the bone.
 			</description>
 		</method>
+		<method name="get_bone_global_pose_no_override" qualifiers="const">
+			<return type="Transform">
+			</return>
+			<argument index="0" name="bone_idx" type="int">
+			</argument>
+			<description>
+			</description>
+		</method>
 		<method name="get_bone_name" qualifiers="const">
 			<return type="String">
 			</return>

(Ideally with the description completed :))

[TwistedTwigleg]

Awesome, will add documentation later tonight 🙂

[TwistedTwigleg]

Documentation is now added and I squashed it all down into a single commit 👍

[akien-mga]

@macryc @yaelatletl @Naurk Could you test the latest version of this PR (commit c1bc87e) with your projects to confirm that the interpolation issue is fixed (and the original problem still is)?

Artifacts can be downloaded here (click green tick next to commit hash):

[macryc]

@macryc @yaelatletl @Naurk Could you test the latest version of this PR (commit c1bc87e) with your projects to confirm that the interpolation issue is fixed (and the original problem still is)?

Will do this evening, you bet

[wojtekpil]

I tested all of my broken projects and commit c1bc87e seems to be fixing all of the issues. Thanks for your work @TwistedTwigleg, @akien-mga!

[TwistedTwigleg]

Awesome! Thanks for testing!

[macryc]

I think I fixed the interpolation bug, at least its fixed on my end. 👍

Dude, you're awesome! Purrs like a kitten now. Really appreciate your help!

[TwistedTwigleg]

Dude, you're awesome! Purrs like a kitten now. Really appreciate your help!

Awesome! I'm glad its working.
Huge thanks to you and all the others who have helped get the PR to this point

[Naurk]

@TwistedTwigleg, @akien-mga I can confirm, the issue is fixed! Thank you guys!

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 3.3.1.