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 SkeletonIK not working correctly with 0 interpolation and incorrectly rotating with animation #47441

Conversation

TwistedTwigleg
Copy link
Contributor

Should close #47381

I know for sure this PR closes the issue with 0 interpolation not working correctly. I'm fairly confident that it will also fix the animation issue, as I removed a function that I thought was unnecessary but I think its removal was causing the issue.

@TwistedTwigleg TwistedTwigleg requested a review from a team as a code owner March 28, 2021 16:02
@TwistedTwigleg
Copy link
Contributor Author

Hmm, now there's an issue with Bone attachment nodes not being in sync, the same issue as #39893.

I will investigate, but I'm wondering if the issue is more on the BoneAttachment side than the SkeletonIK side...

@akien-mga akien-mga added this to the 4.0 milestone Mar 28, 2021
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 28, 2021
@TwistedTwigleg
Copy link
Contributor Author

I haven't found the issue, but I have narrowed it down a bit. The issue seems to be in lines 273 to 285 in skeleton_ik_3d.cpp. It seems that either removing the lines that reset the global pose overrides (273 to 281) fixes the issue or removing _update_chain on line 285, but then the rotation doesn't work.
In other words, in my testing and debugging I have found a way to fix either the BoneAttachment issue (#39893) or the rotation issue (#47381), but not both.

From my testing, the BoneAttachment issue does not appear to be specific to the BoneAttachment node itself. Changing how the BoneAttachment node to get the pose itself (instead of having the Skeleton update its pose) didn't fix the issue in my testing. The global pose does truly seem to be incorrectly reset by the SkeletonIK node.

@AndreaCatania - do you maybe know what would be causing the issue? Looking at the code, I do not see anything in the lines mentioned above that would necessarily cause it.

I don't have anymore time to look this issue, but I will try to look at it further next weekend.

@AndreaCatania
Copy link
Contributor

Hmmm looking at the code everything seems fine, and there is not an obvious explanation. The best to do in these cases is to re-create the issue with an extremely simple skeleton (3, max 4 bones, or really the least amount) and use the C++ debugger to walk the entire logic. While you do this, it's necessary check if the output transformations are as expected. Usually this is the most effective way that allow me to spot issues, because you truly see what's going on and find what's broken.

Let me know, or if I can help you.

@AndreaCatania
Copy link
Contributor

If you manage to create the sample project, please update it here.

@yanorax
Copy link
Contributor

yanorax commented Mar 30, 2021

I hope this helps. I have combined both phenomenon into this one test project.
Skeleton Ik Test.zip

Godot 3.3 RC6 produces this result. Left side is correct.

3 3rc6

Godot 3.2.4 RC4 produces this result. Right side is correct.
3 2 4rc4

@TwistedTwigleg
Copy link
Contributor Author

Thanks @AndreaCatania and @yanorax - I'll look further into this issue as soon as I can! I'm hoping that I can find a fix by the end of this upcoming weekend.

…orrectly rotating with animation. Now the root bone rotates differently to ensure it always rotates correctly and works with BoneAttachment3D nodes.
@TwistedTwigleg TwistedTwigleg force-pushed the skeletonik_changes_and_bug_fixes_regressionfix2 branch from d85ec31 to 318a81f Compare April 2, 2021 17:47
@TwistedTwigleg
Copy link
Contributor Author

It should probably be tested to confirm, but I believe both issues are fixed now! I couldn't check using the example project @yanorax made because its for Godot 3.x, but I tested using some Godot 4 projects and everything seems to work now.

@TwistedTwigleg
Copy link
Contributor Author

@akien-mga - Can this be merged soon so it makes it into Godot 3.3? In my testing it fixes the SkeletonIK issues mentioned above.

@akien-mga
Copy link
Member

Ah yes, sorry I missed than in the previous build.

@akien-mga akien-mga merged commit 47aef8e into godotengine:master Apr 7, 2021
@akien-mga
Copy link
Member

Thanks! Could you make a backport for 3.x?
There are a few conflicts which don't seem too hard to solve, but I'd prefer if it's done and tested by someone who understands this code.

@TwistedTwigleg
Copy link
Contributor Author

No worries! I just figure since the release of Godot 3.3 is soon, it couldn't hurt to ask so hopefully it's fixed moving forward.

Thanks for merging and the quick reply! :)

@TwistedTwigleg
Copy link
Contributor Author

Thanks! Could you make a backport for 3.x?
There are a few conflicts which don't seem too hard to solve, but I'd prefer if it's done and tested by someone who understands this code.

Sure! I'll try to backport it soon, likely this weekend at the latest. 👍

@Janders1800
Copy link

I can test it on 3.3 once the backport its done

@TwistedTwigleg
Copy link
Contributor Author

@akien-mga - I ported the code to Godot 3.x, checked the code to make sure it works, and made a pull request: #47738.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 9, 2021
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.

Skeleton IK breaks root bone position in rc5 and rc6
5 participants