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 Bone2D scaling #91731

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

Fix Bone2D scaling #91731

wants to merge 1 commit into from

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented May 8, 2024

Co-authored-by: Thiago Lages de Alencar thiagola92@gmail.com @thiagola92

To minimize the effect of the fix; Context: #81048 (comment) Revised overall behavior.

@TokageItLab TokageItLab added bug topic:animation topic:2d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels May 8, 2024
@TokageItLab TokageItLab added this to the 4.3 milestone May 8, 2024
@TokageItLab TokageItLab requested a review from a team May 8, 2024 18:30
@TokageItLab TokageItLab requested a review from a team as a code owner May 8, 2024 18:30
@TokageItLab
Copy link
Member Author

I think the problem was that the gizmo scaling was scaling after the rotation. So, perhaps the scaling of Node is done before the rotation? I think that is where the direction mismatch was occurring.

Also, the abs() was needed in #81048 because there was a reverse rotation there to cancel first rotation (it was done rotation->scaling->rotation.inv). So now I make to simplify the calculations for the deformation so that they match the Node scaling.

@thiagola92 Does this change make sense to you?

Co-authored-by: Thiago Lages de Alencar <thiagola92@gmail.com>
@thiagola92
Copy link
Contributor

I think the problem was that the gizmo scaling was scaling after the rotation. So, perhaps the scaling of Node is done before the rotation? I think that is where the direction mismatch was occurring.

Also, the abs() was needed in #81048 because there was a reverse rotation there to cancel first rotation (it was done rotation->scaling->rotation.inv). So now I make to simplify the calculations for the deformation so that they match the Node scaling.

@thiagola92 Does this change make sense to you?

I tested and it seems to be working 👍

I'm still confuse about how the rotation was affecting everything.
In my head get_rotation() was useless in this lines because would cancel itself:

real_t angle_to_use = get_rotation() + bone_angle;
//
rel = rel.rotated(-get_rotation());

But if I'm understanding correctly, there were some scale operation during this step... I'm understanding correctly?

@thiagola92
Copy link
Contributor

Note: This would fix Bone2D length when scaling

To fix height too would probably need something like #81544 (which I used abs() probably without needing)

@TokageItLab
Copy link
Member Author

I'm still confuse about how the rotation was affecting everything.
In my head get_rotation() was useless in this lines because would cancel itself:

In the old code, the vector after rotation (Bone Angle + rotation) was globally scaled.

This code globally scales the vector after rotation by Bone Angle only.

Evenually, Bone Angle is a Bone2D property, so it should be handled by Bone2D, but rotation is a Node2D property, so transformation/scaling for rotation should be handled by Node2D.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:animation topic:2d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The last Bone2D in the chain will scale incorrectly
3 participants