Fix SkeletonModification2DTwoBoneIK with negative scales.

[thiagola92]

Issues: #80252

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]

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

[thiagola92]

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.

[hsandt]

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()));

[hsandt]

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

[hsandt]

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.

[thiagola92]

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

[hsandt]

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.

[thecodacus]

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

[thiagola92]

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]

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

[GopherDevGames]

is there anything blocking this PR as of now?

[thiagola92]

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]

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]

Is this plan for 4.3 or 4.4?

[fire]

@TokageItLab We can review this for 4.4