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

Remove bone_pose_updated signal and replace it with the skeleton_updated signal #90575

Merged

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Apr 12, 2024

As has always been the case, the bone_pose_updated signal is fired a crazy many times in a single frame and can cause unusual updates in BoneAttachment. It should never be used, but used only by BoneAttachment.

For safety and performance, make skeleton fire the skeleton_updated signal only once per frame at the moment all modifiers processed. Then, the BoneAttachment uses it.

If the user has been using the bone_pose_updated signal, replace it with the pose_updated signal which is used by some Editors. This signal may also be fired multiple times per frame, but is relatively safe to use since it is not fired during the update process although it does not detect changes made by the Modifier.

If you need a post modification signal for a specific modifier, use the already existing SkeletonModifier3d.modifier_processed signal.

For getting the final pose which will be reflected in the skin, use the skeleton_updated signal that this PR adds.

@TokageItLab TokageItLab added this to the 4.3 milestone Apr 12, 2024
@TokageItLab TokageItLab requested review from a team as code owners April 12, 2024 11:41
@TokageItLab TokageItLab changed the title Remove bone_pose_updated signal and replace it with the skeleton_updated signal Remove bone_pose_updated signal and replace it with the skeleton_updated signal Apr 12, 2024
@TokageItLab TokageItLab force-pushed the boneattachment-performance branch 2 times, most recently from e53b353 to ca40af3 Compare April 12, 2024 11:49
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

Approve in a technical sense. It's going to be a really big performance improvement for rigs with lots of bones and lots of bone attachments.

I think we need to understand the overall design regarding the update order. Also, is it okay from the user perspective for bone attachment to only update once per frame?

I think it might be good to first figure out if the skeleton modifier design changes before merging this.

doc/classes/Skeleton3D.xml Outdated Show resolved Hide resolved
doc/classes/Skeleton3D.xml Outdated Show resolved Hide resolved
@TokageItLab
Copy link
Member Author

I think we need to understand the overall design regarding the update order. Also, is it okay from the user perspective for bone attachment to only update once per frame?

I have already agreed with @reduz in the past that if poses during modification by the Modifier are needed, the user must either use the modification_processed signal or create their own SkeletonModifier3D.

From the use case of the BoneAttachment, I conclude for now that it is only important to be consistent with the final visual pose.

@TokageItLab TokageItLab force-pushed the boneattachment-performance branch 3 times, most recently from 4bb420b to 136ebf6 Compare April 12, 2024 12:44
@TokageItLab TokageItLab requested review from a team as code owners April 12, 2024 12:44
@TokageItLab TokageItLab force-pushed the boneattachment-performance branch from 136ebf6 to 8869141 Compare April 12, 2024 12:58
@TokageItLab TokageItLab force-pushed the boneattachment-performance branch from 8869141 to 78a5ef4 Compare April 12, 2024 20:48
@akien-mga akien-mga merged commit 673e770 into godotengine:master Apr 15, 2024
16 of 17 checks passed
@akien-mga
Copy link
Member

Thanks!

@GeorgeS2019

This comment was marked as off-topic.

@lyuma

This comment was marked as off-topic.

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.

4 participants