Skip to content

Fix rest update process by dirty flag to not take into account pose dirty flag in Skeleton3D#105920

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
TokageItLab:fix-rest-update
May 19, 2025
Merged

Fix rest update process by dirty flag to not take into account pose dirty flag in Skeleton3D#105920
Repiteo merged 1 commit into
godotengine:masterfrom
TokageItLab:fix-rest-update

Conversation

@TokageItLab
Copy link
Copy Markdown
Member

There is a problem where the flags have the wrong priority and when rest is changed, rest is not updated if there is no change in the pose, fixed.

@TokageItLab TokageItLab added bug topic:animation topic:3d cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release labels Apr 29, 2025
@TokageItLab TokageItLab added this to the 4.5 milestone Apr 29, 2025
@TokageItLab TokageItLab requested a review from a team April 29, 2025 18:47
@TokageItLab TokageItLab requested a review from a team as a code owner April 29, 2025 18:47
@TokageItLab TokageItLab changed the title Fix rest update process by dirty flag to not take into account pose dirty flag Fix rest update process by dirty flag to not take into account pose dirty flag in Skeleton3D Apr 29, 2025
Comment thread scene/3d/skeleton_3d.cpp
Comment on lines +1119 to +1124
if (rest_dirty) {
int current_bone_idx = nested_set_offset_to_bone_index[offset];
Bone &b = bonesptr[current_bone_idx];
b.global_rest = b.parent >= 0 ? bonesptr[b.parent].global_rest * b.rest : b.rest; // Rest needs update apert from pose.
}

Copy link
Copy Markdown
Member Author

@TokageItLab TokageItLab Apr 29, 2025

Choose a reason for hiding this comment

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

Suggested change
if (rest_dirty) {
int current_bone_idx = nested_set_offset_to_bone_index[offset];
Bone &b = bonesptr[current_bone_idx];
b.global_rest = b.parent >= 0 ? bonesptr[b.parent].global_rest * b.rest : b.rest; // Rest needs update apert from pose.
}
int current_bone_idx = nested_set_offset_to_bone_index[offset];
Bone &b = bonesptr[current_bone_idx];
bool bone_enabled = b.enabled && !show_rest_only;
if (rest_dirty) {
b.global_rest = b.parent >= 0 ? bonesptr[b.parent].global_rest * b.rest : b.rest; // Rest needs update apert from pose.
}

current_bone_idx and &b are defined again immediately below.

I think we wanted to skip access to the arrays for improved performance if there is no pose change, so I put them inside the if block to account for the frequency of rest dirty (since it should be few), but the code would look cleaner if we put them outside. Which would be better?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so I put them inside the if block to account for the frequency of rest dirty (since it should be few)

I wonder if we can see the performance on a graph.

@TokageItLab TokageItLab moved this to Ready for review in Animation Team Issue Triage Apr 29, 2025
Copy link
Copy Markdown
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

Approved in meeting 👍

@TokageItLab TokageItLab moved this from Ready for review to Approved, Waiting for Production in Animation Team Issue Triage May 16, 2025
@Repiteo Repiteo merged commit 4af69c9 into godotengine:master May 19, 2025
20 checks passed
@github-project-automation github-project-automation Bot moved this from Approved, Waiting for Production to Done in Animation Team Issue Triage May 19, 2025
@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented May 19, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release topic:animation topic:3d

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants