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 scaling last bone length with negative scales #81048

Closed

Conversation

thiagola92
Copy link
Contributor

@thiagola92 thiagola92 commented Aug 27, 2023

Fixes: #80643

Change to use absolute value.

EDIT:
I was trying to maintain the logic "use the lower value to define the length of the last bone", but I don't think this the right way.
The user always want to see how much the bone is scaled in any direction, so why use the lower value?

No change to scale:
Bones with all scales 1

Current behaviour when scaling X to 3:
Bones with X scaled to 3, current behavior

Propose change:
Bones with X scaled to 3, propose behavior

This also helps with #76087, in this issue we can see that saving calls this functions to redraw bones. Current behavior:

  • Before the user save, they are scaled using both X and Y axis.
  • After the user save, it redraw only using the lower value from scale.

Note: Changing this would only help with the last bone.

@fire
Copy link
Member

fire commented Sep 5, 2023

@TokageItLab does this look ok?

@thiagola92 thiagola92 force-pushed the fix_last_bone2d_scale branch from 9c6b2ec to d3ae624 Compare September 11, 2023 11:23
@akien-mga akien-mga requested a review from TokageItLab October 26, 2023 12:16
@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 26, 2023
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Oct 26, 2023
@@ -325,7 +325,7 @@ bool Bone2D::_editor_get_bone_shape(Vector<Vector2> *p_shape, Vector<Vector2> *p
rel = rel.rotated(-get_global_rotation()); // Undo Bone2D node's rotation so its drawn correctly regardless of the node's rotation
} else {
real_t angle_to_use = get_rotation() + bone_angle;
rel = Vector2(cos(angle_to_use), sin(angle_to_use)) * (length * MIN(get_global_scale().x, get_global_scale().y));
rel = Vector2(cos(angle_to_use), sin(angle_to_use)) * length * get_global_scale().abs();
Copy link
Contributor Author

@thiagola92 thiagola92 Dec 14, 2023

Choose a reason for hiding this comment

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

This PR is attempting to give scale to length and could probably be solved by changing how length is calculated: #83397 (comment)

(just an observation, no change should be made in this PR)

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I also found a more fundamental problem with the bone direction calculation. When the parent has Skew, the bone direction does not take it into account, so everything gets messed up, but this has nothing to do with this PR. Although I think it should be fixed somewhere else.

scene/2d/skeleton_2d.cpp Show resolved Hide resolved
@akien-mga
Copy link
Member

Superseded by #91731. Thanks for the contribution!

@akien-mga akien-mga closed this Jul 31, 2024
@akien-mga akien-mga added archived and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jul 31, 2024
@akien-mga akien-mga removed this from the 4.3 milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The last Bone2D in the chain will scale incorrectly
5 participants