Refactor function `calculate_length_and_rotation()` from Skeleton2D #83397

thiagola92 · 2023-10-15T14:37:34Z

Based on: godotengine/godot-proposals#7472

Correct me if I'm wrong:

No need to break out of the loop to return right after. Just return.
~~No need to convert your child global position to local position, right?~~
- ~~to_local(child->get_global_position()) and child->get_position() give the same value, right?~~

EDIT:

Remove first if condition because loop will check this condition.

kleonc · 2023-10-15T15:14:47Z

Correct me if I'm wrong:

No need to break out of the loop to return right after. Just return.

That's correct. A matter of preference / code style, not sure if one is preferred over the other in the Godot codebase.

No need to convert your child global position to local position, right?

to_local(child->get_global_position()) and child->get_position() give the same value, right?

Only under assumption that child->is_set_as_top_level() == false. Not sure if nesting Bone2Ds where some are set as top level makes any sense though. Also no idea if such setup currently works (if that's the case then the change would break it).

kleonc · 2023-10-15T15:16:31Z

scene/2d/skeleton_2d.cpp

 	// the length and direction. We will always just use the first Bone2D for this.
-	bool calculated = false;
 	int child_count = get_child_count();
 	if (child_count > 0) {


This if is also not needed.

Oooh, true! Will change it.

thiagola92 · 2023-10-15T16:45:46Z

Only under assumption that child->is_set_as_top_level() == false. Not sure if nesting Bone2Ds where some are set as top level makes any sense though. Also no idea if such setup currently works (if that's the case then the change would break it).

Hmmm, I'm reading about top_level right now...

So node position would work as global position when this is true? This would totally change the final values... Okay, I'm reverting this change.

thiagola92 · 2023-12-14T01:52:47Z

scene/2d/skeleton_2d.cpp

+			Vector2 child_local_pos = to_local(child->get_global_position());
+			length = child_local_pos.length();
+			bone_angle = child_local_pos.normalized().angle();


It's probably better to do:

length = (child->get_global_position() - get_global_position()).length(); bone_angle = to_local(child->get_global_position()).angle();

This will give us length scaled and probably help with scaling problems from others issues.

Bone shapes would probably need to convert to absolute, because it uses RenderingServer::get_singleton()->canvas_item_add_polygon() to draw it and seems to take scale sign in count somehow.

It's probably better to do:

length = (child->get_global_position() - get_global_position()).length(); bone_angle = to_local(child->get_global_position()).angle();

This will give us length scaled and probably help with scaling problems from others issues.

I'd say let's leave the behavior as is. AFAICT your original intent with this PR was to just improve readability.

To make Skeleton2D etc. work with negative scales (or in general with any transforms) probably all related calculations need a general revamp/rewrite to account for this. No point in making a small change only here which "maybe will help with other issues". Maybe it would help, maybe it would break things even more. 🙃 I think better not touch it without taking a look at the whole picture.

If changed, it should be properly justified why the given change is needed.

Bone shapes would probably need to convert to absolute, because it uses RenderingServer::get_singleton()->canvas_item_add_polygon() to draw it and seems to take scale sign in count somehow.

Canvas item rendering commands draw accoring to such canvas item's (final/combined) transform, yes. That's normal/expected.

True, no intention of doing changes about this here, just registering thoughts (would have to review many Skeleton2DModification to actually make this change).

If changed, it should be properly justified why the given change is needed.

Bone shapes would probably need to convert to absolute, because it uses RenderingServer::get_singleton()->canvas_item_add_polygon() to draw it and seems to take scale sign in count somehow.

I need to check if this works as I think before justifying anything.

kleonc

Added suggestion for some changes.

In general I would advise against opening such PRs/proposals though - changes are minimal and do not improve much. It makes more sense to do this kind of code cleanup alongside actual important changes, e.g. when fixing some bug in this file etc.

Of course thanks for the contribution / good intents! 🙂 All I'm saying is if you want to contribute it's better to focus on trying to fix even the smallest/simplest bugs rather than making small code-style type changes which don't change much in the end.

Nevertheless, with the suggested changes applied I can approve this.

kleonc · 2024-01-10T21:41:20Z

scene/2d/skeleton_2d.cpp

+			Vector2 child_local_pos = to_local(child->get_global_position());
+			length = child_local_pos.length();
+			bone_angle = child_local_pos.normalized().angle();


It's probably better to do:

length = (child->get_global_position() - get_global_position()).length(); bone_angle = to_local(child->get_global_position()).angle();

This will give us length scaled and probably help with scaling problems from others issues.

I'd say let's leave the behavior as is. AFAICT your original intent with this PR was to just improve readability.

To make Skeleton2D etc. work with negative scales (or in general with any transforms) probably all related calculations need a general revamp/rewrite to account for this. No point in making a small change only here which "maybe will help with other issues". Maybe it would help, maybe it would break things even more. 🙃 I think better not touch it without taking a look at the whole picture.

If changed, it should be properly justified why the given change is needed.

Bone shapes would probably need to convert to absolute, because it uses RenderingServer::get_singleton()->canvas_item_add_polygon() to draw it and seems to take scale sign in count somehow.

Canvas item rendering commands draw accoring to such canvas item's (final/combined) transform, yes. That's normal/expected.

kleonc · 2024-01-10T21:50:14Z

scene/2d/skeleton_2d.cpp

+	for (int i = 0; i < child_count; i++) {
+		Bone2D *child = Object::cast_to<Bone2D>(get_child(i));
+		if (child) {
+			Vector2 child_local_pos = to_local(child->get_global_position());
+			length = child_local_pos.length();
+			bone_angle = child_local_pos.normalized().angle();


If you want to just simplify the code:

to_local would calculate affine inverse every time it's called, so it makes sense to calculate it once outside the loop,

normalization is not needed for obtaining the angle.

Suggested change

for (int i = 0; i < child_count; i++) {

Bone2D *child = Object::cast_to<Bone2D>(get_child(i));

if (child) {

Vector2 child_local_pos = to_local(child->get_global_position());

length = child_local_pos.length();

bone_angle = child_local_pos.normalized().angle();

Transform2D global_inv = get_global_transform().affine_inverse();

for (int i = 0; i < child_count; i++) {

Bone2D *child = Object::cast_to<Bone2D>(get_child(i));

if (child) {

Vector2 child_local_pos = global_inv * child->get_global_position();

length = child_local_pos.length();

bone_angle = child_local_pos.angle();

Did you mean global_inv.xform(child->get_global_position())?

Is okay doing that? I don't know nothing about ERR_READ_THREAD_GUARD_V(Point2()); that is in Node2D::to_local.

Did you mean global_inv.xform(child->get_global_position())?

Oh! Yes, indeed. 👍

Is okay doing that? I don't know nothing about ERR_READ_THREAD_GUARD_V(Point2()); that is in Node2D::to_local.

It's also in CanvasItem::get_global_transform and Node2D::get_global_position, and in case these guards would fail the resulting child_local_pos will end up being the same (result of Transform2D().xform(Vector2()) so Vector2()). So yes, this seems okay / shouldn't change the behavior anyhow.

(Point2/Size2 are just aliases for Vector2, they're all the same thing)

Make sense.

Updated!

thiagola92 · 2024-01-11T01:57:55Z

In general I would advise against opening such PRs/proposals though - changes are minimal and do not improve much. It makes more sense to do this kind of code cleanup alongside actual important changes, e.g. when fixing some bug in this file etc.

Make sense, It cost's a lot of maintainer's time to little gain.

scene/2d/skeleton_2d.cpp

akien-mga · 2024-01-11T19:57:30Z

Thanks!

thiagola92 requested a review from a team as a code owner October 15, 2023 14:37

AThousandShips added enhancement topic:codestyle topic:animation labels Oct 15, 2023

AThousandShips added this to the 4.x milestone Oct 15, 2023

kleonc reviewed Oct 15, 2023

View reviewed changes

thiagola92 force-pushed the readability_skeleton_2d_calculate_length_and_rotation branch from 701310c to fa9b338 Compare October 15, 2023 16:44

thiagola92 commented Dec 14, 2023

View reviewed changes

This was referenced Dec 14, 2023

Fix scaling last bone length with negative scales #81048

Closed

Fix SkeletonModification2DTwoBoneIK with negative scales. #81051

Open

kleonc reviewed Jan 10, 2024

View reviewed changes

thiagola92 force-pushed the readability_skeleton_2d_calculate_length_and_rotation branch 2 times, most recently from dcb3b56 to 4852a1e Compare January 11, 2024 01:48

kleonc approved these changes Jan 11, 2024

View reviewed changes

AThousandShips reviewed Jan 11, 2024

View reviewed changes

scene/2d/skeleton_2d.cpp Outdated Show resolved Hide resolved

Refactor function calculate_length_and_rotation() from Skeleton2D

6a05825

thiagola92 force-pushed the readability_skeleton_2d_calculate_length_and_rotation branch from 4852a1e to 6a05825 Compare January 11, 2024 16:35

akien-mga merged commit 1b25b4c into godotengine:master Jan 11, 2024
15 checks passed

akien-mga mentioned this pull request Jan 11, 2024

Readability from Skeleton2D calculate_length_and_rotation() function godotengine/godot-proposals#7472

Closed

AThousandShips modified the milestones: 4.x, 4.3 Jan 11, 2024

thiagola92 deleted the readability_skeleton_2d_calculate_length_and_rotation branch January 12, 2024 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor function `calculate_length_and_rotation()` from Skeleton2D #83397

Refactor function `calculate_length_and_rotation()` from Skeleton2D #83397

thiagola92 commented Oct 15, 2023 •

edited

Loading

kleonc commented Oct 15, 2023

kleonc Oct 15, 2023

thiagola92 Oct 15, 2023

thiagola92 commented Oct 15, 2023

thiagola92 Dec 14, 2023

thiagola92 Jan 10, 2024

kleonc Jan 10, 2024

thiagola92 Jan 10, 2024

kleonc left a comment

kleonc Jan 10, 2024

kleonc Jan 10, 2024

thiagola92 Jan 11, 2024

kleonc Jan 11, 2024 •

edited

Loading

thiagola92 Jan 11, 2024

thiagola92 commented Jan 11, 2024

akien-mga commented Jan 11, 2024

Refactor function calculate_length_and_rotation() from Skeleton2D #83397

Refactor function calculate_length_and_rotation() from Skeleton2D #83397

Conversation

thiagola92 commented Oct 15, 2023 • edited Loading

kleonc commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagola92 commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleonc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleonc Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagola92 commented Jan 11, 2024

akien-mga commented Jan 11, 2024

Refactor function `calculate_length_and_rotation()` from Skeleton2D #83397

Refactor function `calculate_length_and_rotation()` from Skeleton2D #83397

thiagola92 commented Oct 15, 2023 •

edited

Loading

kleonc Jan 11, 2024 •

edited

Loading