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 SkeletonIK3D interpolation and bone roll #77469

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented May 25, 2023

Two changes:
Fix bug in internal Basis::rotate_to_align function (also used with identity Basis in scene/resources/curve.cpp)
Use ChainItem children rather than local bone rest to determine IK bone roll to match Godot 3.x behavior

Fixes #77271 by solving some other mistakes that were introduced in 4.0 and not in the equivalent code from 3.x

SkeletonIK3D should now behave identically to SkeletonIK in Godot 3.x
Does not address the root cause of #54891 (wow if only I had read the comments on that issue: they had so much diagnosis, and almost every function mentioned in the comments of that issue was the cause of a bug in it in 4.0)

cc @TwistedTwigleg @fire

Correct IK on example project

@wagnerfs
Copy link

wagnerfs commented May 25, 2023

I gave it a try, it does fix the unintentional rotation but it retains the odd length change, seems to happen with any humanoid armature I use.
It's as if the bone itself is bending instead of rotating on its base.

godot.windows.editor.x86_64_2023-05-25_17-45-55.mp4

I'm not all smart with IK and bone transforms but I'll give it a check next week to help tracking where it goes wrong.

@lyuma
Copy link
Contributor Author

lyuma commented May 25, 2023

@wagnerfs Can you import the same armature on Godot 3 (such as 3.5.2) and test there to see if the exact same thing happens? It's helpful to know which behavior is a regression (they have all been core engine bugs so far) and which is just an issue with how SkeletonIK was originally written.

If it's possible, feel free to share which test project / scene / model you're using there and I can investigate if there's something obvious.

Another question: the bone IK solver may be assuming that the Y direction points forward on the armature. it's possible your armature is not Y-forward. Have you tried the same example after enabling retargeting by adding a BoneMap with a SkeletonProfileHumanoid inside during import?

@wagnerfs
Copy link

@lyuma I already tested the same project in 3.x, works fine, I'll drop the project here asap but the issue only happens on godot 4.
I tested it with 3 different models, one without a humanoid profile, one with and to make sure I downloaded a mixamo character and the issue was present on all 3 (I haven't tested the mixamo one -now- after this PR though).

@fracteed
Copy link

@lyuma thanks for looking into these issues! Alas, this PR seems to not be working well. I have attached a clip using the project file that I had previously posted in the linked issue. The skinning seems to go in the opposite direction of the bone, and the bone itself can be seen to be distorting.

ik_roll_bug.2.mp4

@lyuma
Copy link
Contributor Author

lyuma commented May 26, 2023

@wagnerfs , if you continue to have bugs with skeleton IK , please attach a minimal reproduction project .zip and it would help if you state whether it happens on 3.x but honestly the project zip is the most important.

I see the project from fracteed and I will do more testing on it. This PR is currently fixing specific regressions that make it act different from godot 3

I won't be able to know if I am solving your specific issues without the project .zip

@wagnerfs
Copy link

@lyuma yeah I wasn't around that's why I took a bit, the project is the same attached here:
#77194 (comment) but that version is just the armature without the mesh, I can create a quick one with a mixamo mesh if you need me to.

This is the exact same mesh and armature on the exact same circumstances on Godot 3, no bone bending or weird rotations at all.

Godot_v3.5.2-stable_win64_2023-05-26_09-11-36.mp4

Fix bug in internal Basis::rotate_to_align function (also used with identity Basis in scene/resources/curve.cpp)
Use ChainItem children rather than local bone rest to determine IK bone roll to match Godot 3.x behavior
@lyuma
Copy link
Contributor Author

lyuma commented Jun 6, 2023

Hi, @wagnerfs and @fracteed , I have corrected the PR. I made a mistake when comparing the code against 3.x and I accidentally turned in a left-multiply into a right-multiply in the Basis::rotate_to_align function which is new to 4.0

That seems to have resolved the difference in the "issue77271_ik_roll_bug" example project. I have verified that the IK results are now numerically identical to 3.x

Correct IK on example project

@fracteed
Copy link

fracteed commented Jun 6, 2023

@lyuma thanks for fixing this! I just did some extensive tests, including my in dev game which has several 2, 3 and 6 legged ik creatures. It all works perfectly from what I can tell.

The only issue I can see is probably not related to this PR and is a separate issue. When using a tool script to solve the ik in viewport, there are many times when the ik stops working or crashes. If you look at the simple leg scene that I had posted, which does not have the magnet option on. If you turn magnet on, then try and move the ik target it does nothing. But if you save and reload the scene it does work, until you change another parameter on the skeltonik node and it either crashes or stops working. This seems purely a viewport issue though, and I don't see these problems at runtime.

In general, using a tool script in 3.x and 4.0 for ik has always been flaky and often crashes. I guess a better long term solution would be to have some sort of inbuilt realtime solve like the "play ik" button that works as you move ik targets around.

Is this fix going to be added to the 4.0.x branch or just to 4.1?

@lyuma
Copy link
Contributor Author

lyuma commented Jun 6, 2023

Is this fix going to be added to the 4.0.x branch or just to 4.1?

Yes, Akien added the cherrypick:4.0 tag, so it should be backported to 4.0

The Play IK button is much harder to work with in 4.0. I think the issue is the inspector gets unloaded when selecting a different node, and the inspector/toolbar is what manages the "Play IK" state.

Perhaps rather than remembering the state and unsetting it, the Play IK button should toggle set_process(true) and let everything else be handled in the IK solver itself.

@fracteed
Copy link

fracteed commented Jun 6, 2023

@lyuma I also had a go with the scorpion test scene in #74753

If you move the ik target, there is no feedback in the viewport with the tool script, but it does solve correctly when the "play ik" button is pressed. I did notice that this same issue persists regardless of it having the magnet option on or off. So, if use that scene, turn off magnet. Save and reload, the ik target does nothing interactive in the viewport.

@Zireael07
Copy link
Contributor

Play IK button has had issues/crashes before that PR (I'm not sure if there's a dedicated issue, but I've certainly seen comment smentioning this)

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and user tests seem to confirm it works.

@akien-mga akien-mga merged commit 2a1bc05 into godotengine:master Jun 6, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.4.

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.

SkeletonIK3D bone roll axis is not working correctly
6 participants