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.

Sign up for GitHub

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 SkeletonModification2DTwoBoneIK with negative scales. #81051

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

Fix SkeletonModification2DTwoBoneIK with negative scales. #81051

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions scene/resources/skeleton_modification_2d_twoboneik.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,17 +138,18 @@ void SkeletonModification2DTwoBoneIK::_execute(float p_delta) {
return;
}

// Adopted from the links below:
// Adapted from the links below:
// http://theorangeduck.com/page/simple-two-joint
// https://www.alanzucconi.com/2018/05/02/ik-2d-2/
// With modifications by TwistedTwigleg
Vector2 target_difference = target->get_global_position() - joint_one_bone->get_global_position();
float joint_one_to_target = target_difference.length();
float angle_atan = target_difference.angle();

float bone_one_length = joint_one_bone->get_length() * MIN(joint_one_bone->get_global_scale().x, joint_one_bone->get_global_scale().y);
float bone_two_length = joint_two_bone->get_length() * MIN(joint_two_bone->get_global_scale().x, joint_two_bone->get_global_scale().y);
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);
Comment on lines +149 to +150
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bool override_angles_due_to_out_of_range = false;
bool same_scale_sign = true;

if (joint_one_to_target < target_minimum_distance) {
joint_one_to_target = target_minimum_distance;
Expand All @@ -161,6 +162,10 @@ void SkeletonModification2DTwoBoneIK::_execute(float p_delta) {
override_angles_due_to_out_of_range = true;
}

if (joint_one_bone->get_global_scale().sign().x != joint_one_bone->get_global_scale().sign().y) {
same_scale_sign = false;
}

if (!override_angles_due_to_out_of_range) {
float angle_0 = Math::acos(((joint_one_to_target * joint_one_to_target) + (bone_one_length * bone_one_length) - (bone_two_length * bone_two_length)) / (2.0 * joint_one_to_target * bone_one_length));
float angle_1 = Math::acos(((bone_two_length * bone_two_length) + (bone_one_length * bone_one_length) - (joint_one_to_target * joint_one_to_target)) / (2.0 * bone_two_length * bone_one_length));
Expand All @@ -173,12 +178,23 @@ void SkeletonModification2DTwoBoneIK::_execute(float p_delta) {
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 {
joint_one_bone->set_global_rotation(angle_atan - angle_0 - joint_one_bone->get_bone_angle());
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@thiagola92 thiagola92 Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

} else {
joint_one_bone->set_global_rotation(angle_atan - joint_one_bone->get_bone_angle());
joint_two_bone->set_global_rotation(angle_atan - joint_two_bone->get_bone_angle());
if (same_scale_sign) {
joint_one_bone->set_global_rotation(angle_atan - joint_one_bone->get_bone_angle());
joint_two_bone->set_global_rotation(angle_atan - joint_two_bone->get_bone_angle());
} else {
joint_one_bone->set_global_rotation(angle_atan + joint_one_bone->get_bone_angle());
joint_two_bone->set_global_rotation(angle_atan + joint_two_bone->get_bone_angle());
}
Copy link
Contributor

@hsandt hsandt Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

stack->skeleton->set_bone_local_pose_override(joint_one_bone_idx, joint_one_bone->get_transform(), stack->strength, true);
Expand Down Expand Up @@ -207,7 +223,8 @@ void SkeletonModification2DTwoBoneIK::_draw_editor_gizmo() {
}
stack->skeleton->draw_set_transform(
stack->skeleton->to_local(operation_bone_one->get_global_position()),
operation_bone_one->get_global_rotation() - stack->skeleton->get_global_rotation());
operation_bone_one->get_global_rotation() - stack->skeleton->get_global_rotation(),
operation_bone_one->get_global_scale());

Color bone_ik_color = Color(1.0, 0.65, 0.0, 0.4);
#ifdef TOOLS_ENABLED
Expand Down