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 and Skeleton3D::bone_pose_updated signal #90880

Closed
raulsntos opened this issue Apr 19, 2024 · 3 comments · Fixed by #94291
Closed

Comments

@raulsntos
Copy link
Member

Tested versions

  • Reproducible in v4.3.dev.mono.custom_build [2efbc6b]

System information

Pop OS 21.04

Issue description

As I was looking into the compat break in 4.3 for the migration guide (godotengine/godot-docs#9250) I found these changes in Skeleton3D that look like they could be avoided.

The property animate_physical_bones:

The signal bone_pose_updated:

Steps to reproduce

Try to use any of the mentioned members in GDScript or C#.

Minimal reproduction project (MRP)

N/A

@TokageItLab
Copy link
Member

TokageItLab commented Jul 9, 2024

bone_pose_updated is quite dangerous and cannot be re-added in the same way as in the past, as it cannot be guaranteed to work in the future. For example, this could be called a lot in the Deffered process, but it could become an obstacle in the future when we support multi-threading, etc. So I think it could be added as an alias for skeleton_updated.

animate_physical_bones is based on the state of Skeleton3D's internal objects (SkeletonSimulator3D) and needs to sync, so this cannot be added in the same way as before. I think it is possible to bind it in a state that is not exposed to the editor and is not serialized. (If there are no problems testing for regression of #93504, we may be possible to serialize it.)

@raulsntos
Copy link
Member Author

Sounds good. As long as the methods exist to prevent breaking binary compatibility, and the behavioral changes are kept to a minimum and are well documented I think that's fine.

@dsnopek
Copy link
Contributor

dsnopek commented Jul 10, 2024

I think it is possible to bind it in a state that is not exposed to the editor and is not serialized.

Yes, this would be PROPERTY_USAGE_NONE passed to PropertyInfo() when registering the property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Release Blocker
Development

Successfully merging a pull request may close this issue.

4 participants