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

Re-add Skeleton3D::animate_physical_bones property #94291

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jul 13, 2024

The bone_pose_updated signal is very dangerous as described in #90575, so it is reimplemented as an alias of skeleton_updated. We cannot re-add a compatible signal that is not hazardous and has no performance problems, so bone_pose_changed signal should be eliminated.

The internal PhysicalBoneSimulator3D is set after enter_tree, so that animate_physical_bones is updated once there.

However, the old configuration where PhysicalBone is directly a child of Skeleton3D without already having PhysicalBoneSimulator3D is deprecated, so it is placed under the group "Deprecated", to avoid confusion if the user follows the recommended new settings and has PhysicalBoneSimulator3D explicitly.

image

scene/3d/skeleton_3d.cpp Outdated Show resolved Hide resolved
scene/3d/skeleton_3d.cpp Show resolved Hide resolved
@TokageItLab TokageItLab requested a review from raulsntos July 13, 2024 23:46
@Mickeon Mickeon self-requested a review July 13, 2024 23:53
@TokageItLab TokageItLab changed the title Re-add Skeleton3D::animate_physical_bones property and Skeleton3D::bone_pose_updated signal for compatibility Re-add Skeleton3D::animate_physical_bones property Jul 14, 2024
@TokageItLab
Copy link
Member Author

#94291 (comment)

As the discussion, we cannot re-add a compatible signal that is not hazardous and has no performance problems, so bone_pose_changed signal should be eliminated.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

The reasoning for not re-adding bone_pose_changed makes sense to me, hopefully it won't be too disruptive. I'll make a note to document the change in godotengine/godot-docs#9250.

@akien-mga akien-mga merged commit ac21501 into godotengine:master Jul 17, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Re-add Skeleton3D::animate_physical_bones property and Skeleton3D::bone_pose_updated signal
3 participants