Fix SkeletonModification2DTwoBoneIK with negative scales. #81051

thiagola92 · 2023-08-27T17:02:44Z

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

Identifies when only one scale axis is negative and change the logic
Take in count scales when drawing gizmo
Fix typo

Note: Last bone will be inverted due to issue #80643 (PR to fix this: #81048)

Bugsquad edit, fixes: Setting scale X or Y (not both) to negative value on Skeleton2D breaks Modifications like SkeletonModification2DLookAt and SkeletonModification2DTwoBoneIK #80252

hsandt · 2023-09-17T10:47:04Z

scene/resources/skeleton_modification_2d_twoboneik.cpp

OK, but it feels like you could keep the general else block from before commit then handle both cases at once:

for the first line, a -1/+1 multiplier would do the trick

the second line is always the same, could be preserved at end of else block

About the first point, you mean creating a variable to hold -1 ou 1, them multiplying angle_0 + joint_one_bone->get_bone_angle() by the variable? Can you show me what you mean?

About the second point, like this?

if (isnan(angle_0) || isnan(angle_1)) { // We cannot solve for this angle! Do nothing to avoid setting the rotation (and scale) to NaN. } else { if (same_scale_sign) { joint_one_bone->set_global_rotation(angle_atan - angle_0 - joint_one_bone->get_bone_angle()); } else { joint_one_bone->set_global_rotation(angle_atan + angle_0 + joint_one_bone->get_bone_angle()); } joint_two_bone->set_rotation(-Math_PI - angle_1 - joint_two_bone->get_bone_angle() + joint_one_bone->get_bone_angle()); }

EDIT: I agree in letting the second line at the end of else block so I'm changing.

Yes, using a float multiplier like this:

float sign_multiplier = same_scale_sign ? -1.0f : 1.0f; joint_one_bone->set_global_rotation(angle_atan + sign_multiplier * (angle_0 + joint_one_bone->get_bone_angle()));

But looking at your comment below, else looks fine too.

hsandt · 2023-09-17T10:49:18Z

scene/resources/skeleton_modification_2d_twoboneik.cpp

Same remark here except you already got a big else block so you'd just have to use the sign multiplier to factorize.

Unless there's a particular policy on this file that developers prefer explicit full line operations with +/- rather than -1/+1 multipliers to make it easier to read? I haven't checked other math code so I don't know what style they picked.

EDIT: I'd say it is indeed very clear on this block because all the lines are uniform and the +/- symbols perfectly aligned; while above, the global_rotation and rotation lines are alternating so it's not that easy to spot what's different.

I'm holding back with this because could impact the readability and I really want to maintain clear which is the main logic and which is the exception (different scales).

I don't think that would be easy to understand from something like this:

angle_atan + joint_one_bone->get_bone_angle() * rotation_dir

Or this:

angle_atan + (angle_0 + joint_one_bone->get_bone_angle()) * rotation_dir

Makes sense, you can keep the else case then. In this case, for consistency you can also keep the else case above and don't use a multiplier. You can add a comment indicating which one is the more rare case if you want.

- Identifies when only one scale axis is negative and change the logic - Take in count scales when drawing gizmo - Fix typo

thecodacus · 2023-10-16T17:22:04Z

can this be pushed as patch update.. the ik bug is making the ik chain unusable

thiagola92 · 2023-10-16T20:55:09Z

I'm not actually happy with this solution (using if to check scales and decide the action).
And I hope someone knows a better way 🤣.

thiagola92 · 2023-12-14T02:09:22Z

scene/resources/skeleton_modification_2d_twoboneik.cpp

+	float bone_one_length = joint_one_bone->get_length() * MIN(joint_one_bone->get_global_scale().abs().x, joint_one_bone->get_global_scale().abs().y);
+	float bone_two_length = joint_two_bone->get_length() * MIN(joint_two_bone->get_global_scale().abs().x, joint_two_bone->get_global_scale().abs().y);


This part is attempting to give scale to bones length and could probably be solving by calculating length with scale: #83397 (comment)

GopherDevGames · 2024-05-28T06:00:30Z

is there anything blocking this PR as of now?

thiagola92 · 2024-05-29T02:04:06Z

is there anything blocking this PR as of now?

Is better to wait someone with knowledge like @TokageItLab to review
Someone that could tell if there is or not a better way to solve this (or tell if this solutions brings any problem)

TokageItLab · 2024-05-29T02:30:46Z

Since TwoBoneIK is separated from any other node in the first place, so the change should be safe, at least from a code standpoint. Also, if this change is mainly dedicated to flips due to scaling, then there seems to be fine.

I am a little concerned about whether it will work well when combined with the #91731 fix. If we can confirm that much, I think it is safe to merge.

BTW, since the ModificationStack2D is experimental to begin with, so I think in the future it will be necessary to migrate into SkeletonModifier2D in the same way as the SkeletonModifier3D.

CsloudX · 2024-07-28T09:53:22Z

Is this plan for 4.3 or 4.4?

fire · 2024-08-17T17:23:36Z

@TokageItLab We can review this for 4.4

fire · 2024-11-17T22:03:40Z

The pull request became stale and requires a rebase.

@SaracenOne, @TokageItLab, @fire and Cthulhu discussed this briefly at the animation meeting.

We discussed the problems with using the scale to flip 2d and 3d animation in general.

We wanted a proposal for modifying the godot human skeleton profile to add a hard-defined mirror boolean for regular and mirrored left hand to right hand.

thiagola92 · 2024-11-30T13:35:05Z

We discussed the problems with using the scale to flip 2d and 3d animation in general.

There is a list?

All I know is that is not possible to differ 2D matrices that had X scaled by -1 from those that had Y scaled by -1 and rotate by 180º. At least not without a variable to store this information.

We wanted a proposal for modifying the godot human skeleton profile to add a hard-defined mirror boolean for regular and mirrored left hand to right hand.

I don't understand how this would work under the hood. There is a change in "modification" logic? Or would be something like:

if (!mirrored) {
  joint_one_bone->set_global_rotation(angle_atan - angle_0 - joint_one_bone->get_bone_angle());
} else {
  joint_one_bone->set_global_rotation(angle_atan + angle_0 + joint_one_bone->get_bone_angle());
}

Instead of my propose:

if (same_scale_sign) {
  joint_one_bone->set_global_rotation(angle_atan - angle_0 - joint_one_bone->get_bone_angle());
} else {
  joint_one_bone->set_global_rotation(angle_atan + angle_0 + joint_one_bone->get_bone_angle());
}

F3der1co · 2025-06-20T08:58:20Z

The pull request became stale and requires a rebase.

@SaracenOne, @TokageItLab, @fire and Cthulhu discussed this briefly at the animation meeting.

We discussed the problems with using the scale to flip 2d and 3d animation in general.

We wanted a proposal for modifying the godot human skeleton profile to add a hard-defined mirror boolean for regular and mirrored left hand to right hand.

This doesn't make sense to me, or are you talking about skeleton2d mirror boolean, not human skeleton profile? Mirroring is required at runtime in almost any 2d skeleton. For left and right facing variants of each animation. Maybe I am not getting what this proposal would do/be about and how it would solve this problem.

Sad to see this being stale

0xcafeb33f · 2025-06-26T02:00:50Z

Can this be merged in 4.5? If not, what specifically is blocking it?

If the concern is only that it needs a rebase, I have already rebased this PR and #83330 locally in my custom build of Godot 4.5, and would be happy to contribute it back if desired.

I don't understand the suggestion for a proposal. This PR does not add a new mirroring feature; it fixes a longstanding bug when Node2D transforms interact with Skeleton2Ds. I have verified that this fixes the issue in my project. Why would a proposal be needed for this bugfix?

rjunkfolder-del · 2025-09-06T18:00:27Z

We really need this to be fixed...

GopherDevGames · 2025-09-06T18:06:04Z

We really do. This PR used to basically fix the issue 99% of the time, but the original code changed too much and I'm too unfamiliar with the engine to rebase it now. At least there's a bunch of GDScript solutions for 2DIK

0xcafeb33f · 2025-09-06T19:10:33Z

I just saw @thiagola92's comment that they no longer plan to work on this PR. I have created a separate PR that rebases this one (#110298) per my previous comment -- please review/test and let me know if there are any issues.

Repiteo · 2025-09-24T15:08:42Z

Superseded by #110298

AThousandShips added bug topic:animation topic:2d labels Aug 27, 2023

AThousandShips added this to the 4.2 milestone Aug 27, 2023

YuriSizov requested a review from a team August 30, 2023 19:39

thiagola92 force-pushed the fix_twoboneik_with_negative_scale branch from b8b6998 to 48ff535 Compare September 11, 2023 11:21

hsandt reviewed Sep 17, 2023

View reviewed changes

thiagola92 force-pushed the fix_twoboneik_with_negative_scale branch from 48ff535 to fc845ce Compare September 19, 2023 19:10

Fix SkeletonModification2DTwoBoneIK with negative scales.

3e02983

- Identifies when only one scale axis is negative and change the logic - Take in count scales when drawing gizmo - Fix typo

thiagola92 force-pushed the fix_twoboneik_with_negative_scale branch from fc845ce to 3e02983 Compare September 19, 2023 19:32

YuriSizov modified the milestones: 4.2, 4.3 Nov 13, 2023

YuriSizov mentioned this pull request Nov 13, 2023

Fix SkeletonModification2DLookAt with negative scales #83330

Closed

YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 13, 2023

YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 4, 2023

YuriSizov modified the milestones: 4.3, 4.x Dec 4, 2023

thiagola92 commented Dec 14, 2023

View reviewed changes

myyk mentioned this pull request Jan 9, 2024

Flip Skeleton2D animation via scale.x, animation with TwoBoneIK #86868

Closed

thiagola92 mentioned this pull request Aug 16, 2025

Setting scale X or Y (not both) to negative value on Skeleton2D breaks Modifications like SkeletonModification2DLookAt and SkeletonModification2DTwoBoneIK #80252

Closed

0xcafeb33f mentioned this pull request Sep 6, 2025

Fix Skeleton2D TwoBoneIK and LookAt mirroring #110298

Merged

Repiteo closed this Sep 24, 2025

github-project-automation bot moved this to Done in Animation Team Issue Triage Sep 24, 2025

Repiteo added the archived label Sep 24, 2025

Repiteo removed this from the 4.x milestone Sep 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix SkeletonModification2DTwoBoneIK with negative scales. #81051

Fix SkeletonModification2DTwoBoneIK with negative scales. #81051

Uh oh!

thiagola92 commented Aug 27, 2023 •

edited by YuriSizov

Loading

hsandt Sep 17, 2023

thiagola92 Sep 17, 2023 •

edited

Loading

hsandt Oct 16, 2023

hsandt Oct 16, 2023

hsandt Sep 17, 2023 •

edited

Loading

thiagola92 Sep 19, 2023

hsandt Oct 16, 2023

thecodacus commented Oct 16, 2023

thiagola92 commented Oct 16, 2023

thiagola92 Dec 14, 2023

GopherDevGames commented May 28, 2024

thiagola92 commented May 29, 2024

TokageItLab commented May 29, 2024

CsloudX commented Jul 28, 2024

fire commented Aug 17, 2024

fire commented Nov 17, 2024 •

edited

Loading

thiagola92 commented Nov 30, 2024

F3der1co commented Jun 20, 2025 •

edited

Loading

0xcafeb33f commented Jun 26, 2025

rjunkfolder-del commented Sep 6, 2025

GopherDevGames commented Sep 6, 2025

0xcafeb33f commented Sep 6, 2025

Repiteo commented Sep 24, 2025

Labels

13 participants

		float bone_one_length = joint_one_bone->get_length() * MIN(joint_one_bone->get_global_scale().abs().x, joint_one_bone->get_global_scale().abs().y);
		float bone_two_length = joint_two_bone->get_length() * MIN(joint_two_bone->get_global_scale().abs().x, joint_two_bone->get_global_scale().abs().y);

Uh oh!

Fix SkeletonModification2DTwoBoneIK with negative scales. #81051

Fix SkeletonModification2DTwoBoneIK with negative scales. #81051

Uh oh!

Conversation

thiagola92 commented Aug 27, 2023 • edited by YuriSizov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

hsandt Sep 17, 2023

Choose a reason for hiding this comment

thiagola92 Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

hsandt Oct 16, 2023

Choose a reason for hiding this comment

hsandt Oct 16, 2023

Choose a reason for hiding this comment

hsandt Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

thiagola92 Sep 19, 2023

Choose a reason for hiding this comment

hsandt Oct 16, 2023

Choose a reason for hiding this comment

thecodacus commented Oct 16, 2023

thiagola92 commented Oct 16, 2023

thiagola92 Dec 14, 2023

Choose a reason for hiding this comment

GopherDevGames commented May 28, 2024

thiagola92 commented May 29, 2024

TokageItLab commented May 29, 2024

CsloudX commented Jul 28, 2024

fire commented Aug 17, 2024

fire commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

thiagola92 commented Nov 30, 2024

F3der1co commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

0xcafeb33f commented Jun 26, 2025

rjunkfolder-del commented Sep 6, 2025

GopherDevGames commented Sep 6, 2025

0xcafeb33f commented Sep 6, 2025

Repiteo commented Sep 24, 2025

Labels

13 participants

thiagola92 commented Aug 27, 2023 •

edited by YuriSizov

Loading

thiagola92 Sep 17, 2023 •

edited

Loading

hsandt Sep 17, 2023 •

edited

Loading

fire commented Nov 17, 2024 •

edited

Loading

F3der1co commented Jun 20, 2025 •

edited

Loading