Fix get_bone_global_pose_no_override() returning incorrect values

[lyuma]

A change introduced in

This is the cause of the bug in SkeletonIK3D

Fixes #60180
Fixes #65167
Fixes #74753
Fixes #77138
and probably more

The cause of the bug is get_bone_pose_global_no_override was inadvertently broken in 2021 due to a typo in the forward port of 3.x #48251 to master #48166 , and then the typo was concealed later in #51368 due to removing what then appeared to be duplicate code.

I double checked against the corresponding if statements from scene/3d/skeleton.cpp in 3.x, with the exception of ignoring the extra b.rest * multiply which is no longer used in 4.0

[fire]

I tested #74753 and it works.

Recording.2023-05-18.085512.mp4

[YuriSizov]

Great find, thanks!

[wagnerfs]

I'm not sure it quite fixes the IK though, still getting undesired rotations when acquiring the rest pose transform apparently

godot.windows.editor.x86_64_2023-05-19_21-11-23.mp4

[fire]

What git commit is that and can you provide a test project?

[wagnerfs]

I'll drop the project tomorrow, and I cloned from master since it's already been merged, double checked the changed files to make sure I got the right thing

[wagnerfs]

Godot4-test2.zip
Took a bit to do an empty project with just the basics, lots of crashing related to tool mode and skeleton.
This project should be enough, it's just the bones, just hit PlayIK and you're gonna see the shoulder deform going nuts.

[lyuma]

@wagnerfs I would encourage you to file a new issues.

This issue has already been closed and your problem is going to get lost and forgotten if it's left here only in a comment

I believe it looks like a separate bug from this issue. The IK would have been far more broken if you had not used latest master.

If you want to verify, can you test the same setup in Godot 3.5 or 3.6 and see if it acts the same? it should

[wagnerfs]

@lyuma Nah, tested on Godot 3 way before, but did it again just now to make sure, works perfectly on 3, same GLB file.
I do assume it's something to do with either the data retrieved by bone_pose methods or how the initial bone transform is stored, regardless I'll file a new issue when I get the chance, just wanted to point this didn't solve the issue because I was using @fire many bone IK solution and noticed it was deleted prior to this commit.

[YuriSizov]

Cherry-picked for 4.0.4.

[YuriSizov]

The cherry-pick is going to be reverted for 4.0.4 stable in the end. Since this changes the behavior, even if it fixes a bug it's not fitting for a patch release and I made a mistake picking it in the first place. For a proper fix consider updating to 4.1, if this is important for your project.

[lyuma]

Thank you for doing the revert. I think it's the right call. While this API now acts the same way it did in 3.x (which is why I originally felt a backport made sense), there are already a number of people who expect the 4.0 behavior, since it turns out to be a little more useful for some applications.

(I'm having growing doubts that the approach we picked is the right thing in the long term... while it fixes the single IK case, it makes it impossible to daisy chain IK solvers, since each one will solve based on the whole skeleton's rest pose. This whole discussion points to doing more work to design a proper IK solver system in future 4.x releases. )