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

Fixed issue in SkeletonIK leading to some root bones being twisted incorrectly #48166

Conversation

TwistedTwigleg
Copy link
Contributor

@TwistedTwigleg TwistedTwigleg commented Apr 24, 2021

Should close #48078 and close #47803 - at least I think it should.

I still have the issue where in Godot 4.0 I am unable to see meshes, but the bone gizmo seems to no longer be twisted. I tested the minimum replication projects in both of the linked issues, as well as some other projects, and the gizmos seem to be in the correct rotation.

The issue seems to be two fold: the first problem was that I was incorrectly making both of the Vector3s I was using for directions absolute, making it where bones with -Z as the forward vector would be incorrectly parsed as +Z as the forward direction. The other issue was that LookAt was using a up direction of +Y, so for bones with a forward bone direction on the Z axis, up is now +X.

That said, this needs testing to make sure the visuals are correct! It could be the rotation is off by an increment of 90 degrees, in which case I would be unable to see it in the bone gizmo. Help testing this PR to make sure it actually fixes the issues linked would be greatly appreciated!

@TwistedTwigleg TwistedTwigleg requested a review from a team as a code owner April 24, 2021 17:13
@Calinou Calinou added bug cherrypick:3.3 topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Apr 24, 2021
@Calinou Calinou added this to the 4.0 milestone Apr 24, 2021
@akien-mga
Copy link
Member

I made builds of the PR for Windows and Linux, cherry-picked on the 3.x branch (3.4-beta, but should still be nearly identical to 3.3-stable):

CC @macryc @yaelatletl @Naurk @wojtekpil Please confirm that it fixes your issue(s).

@TwistedTwigleg
Copy link
Contributor Author

Awesome, thanks @akien-mga!

@macryc
Copy link

macryc commented Apr 26, 2021

Cool! I'll check this afternoon and report. Thanks.

@Naurk
Copy link

Naurk commented Apr 26, 2021

@akien-mga Unfortunately this don't fix the twisted root bone for me. You can check it yourself: https://github.com/Naurk/TP-Minimal-Controller

Anyway thank you for your work.

@macryc
Copy link

macryc commented Apr 26, 2021

I made builds of the PR for Windows and Linux, cherry-picked on the 3.x branch (3.4-beta, but should still be nearly identical to 3.3-stable):

CC @macryc @yaelatletl @Naurk @wojtekpil Please confirm that it fixes your issue(s).

No, it didn't fix the issue I'm afraid. Is it possible that the code didn't get pulled into this build? Just thinking - if something got changed we'd see a different behavior but the issue looks exactly like it does in 3.3 stable.

@wojtekpil
Copy link
Contributor

I made builds of the PR for Windows and Linux, cherry-picked on the 3.x branch (3.4-beta, but should still be nearly identical to 3.3-stable):

CC @macryc @yaelatletl @Naurk @wojtekpil Please confirm that it fixes your issue(s).

Unfortunately, also in my case the error has not been fixed.

@akien-mga
Copy link
Member

Is it possible that the code didn't bet pulled into this version? Just thinking - if something got changed we'd see a different behavior but the issue looks exactly like it does in 3.3 stable.

It's possible yes as it's mostly a manual process when I do a custom build like this. But I just checked and I do have the relevant changes committed so they should be part of that build normally.

I pushed the manual cherry-pick to this branch: https://github.com/akien-mga/godot/commits/3.x-pr48166
(It's the same as this PR but I had to add the up vector in the first two looking_at calls as there's no default value in 3.x.)

@TwistedTwigleg
Copy link
Contributor Author

I just tested the Godot 3.x build linked and unfortunately it doesn't work for me either. I also just looked at the commit Akien linked and can confirm it is the same code change. It seems this PR doesn't fix it after all. This PR does fix an issue where the wrong condition is used if the bone axis is negative, so it's not totally useless, but alas it doesn't fix the bone twist issue it seems.

I'll take another swing at trying to fix it once I can. It looks like I'll need to work in the Godot 3.x branch to make the fix and then port it to master, as it seems I cannot go by the skeleton gizmo as an indicator of whether its working or not.

@TwistedTwigleg
Copy link
Contributor Author

Took the additional changes from #48251 and put them into this PR.

@akien-mga akien-mga removed cherrypick:3.3 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Apr 29, 2021
@TwistedTwigleg TwistedTwigleg force-pushed the skeletonik_changes_and_bug_fixes_regressionfix3 branch from 61810eb to 446460e Compare May 8, 2021 20:36
@TwistedTwigleg TwistedTwigleg requested a review from a team as a code owner May 8, 2021 20:36
@TwistedTwigleg
Copy link
Contributor Author

The code in this PR is now a direct port of #48251 but for master instead of Godot 3.x

@akien-mga akien-mga merged commit 7050e4d into godotengine:master May 8, 2021
@akien-mga
Copy link
Member

Thanks!

@TwistedTwigleg TwistedTwigleg deleted the skeletonik_changes_and_bug_fixes_regressionfix3 branch May 24, 2021 14:23
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.

SkeletonIK works incorrectly in Godot 3.3 IK is not deforming properly with some positions of target node
6 participants