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 SkeletonModification2DLookAt with negative scales #83330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiagola92
Copy link
Contributor

@thiagola92 thiagola92 commented Oct 14, 2023

Fixes #80252.

This is an attempt to fix SkeletonModification2DLookAt when only one scale is negative.


Instead of getting the transform and operate over it, changes will happen through Node2D methods. I'm doing this because we can't decompose X negative scale from transforms, which means that you would have to replicate what Node2D already does (recovering the correct scale).

I'm operating over the angle and only at the end call methods like set_rotation() (i'm just reducing method calls).

Adding X to local rotation would also means that the global will increase by X, right? That's why I'm just adding to global rotation without any conversion.

@AThousandShips AThousandShips added this to the 4.2 milestone Oct 14, 2023
@AThousandShips AThousandShips requested a review from a team October 14, 2023 13:38
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 13, 2023
@YuriSizov YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 13, 2023
@YuriSizov
Copy link
Contributor

You have two PRs, this and #81051, that refer to the same two issues. Are they both required to fix both of the issues, or how does this work?

@thiagola92
Copy link
Contributor Author

You have two PRs, this and #81051, that refer to the same two issues. Are they both required to fix both of the issues, or how does this work?

Sorry, saying that fixes #75224 is wrong! The issue is saying that all IK modifications fail scaling X negatively and I'm just fixing for two modifications (SkeletonModification2DLookAt SkeletonModification2DTwoBoneIK).

Having said that... I'm still unsure about the solution, after reading @lyuma old proposal I started questioning if It could be solved other way.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 4, 2023
@YuriSizov YuriSizov modified the milestones: 4.3, 4.x Dec 4, 2023
@thiagola92
Copy link
Contributor Author

@YuriSizov I started a personal project to better understand the subject and I believe that this part is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants