Skip to content

PhysicalBone3D: expose sleeping state #99975

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

Closed

Conversation

paul-marechal
Copy link

@paul-marechal paul-marechal commented Dec 3, 2024

It seems like PhysicalBone3D nodes support sleeping, but it wasn't possible to access this state unless going through the PhysicsServer3D directly, and even then it would miss the convenient "sleeping_state_changed" signal.

This commit exposes the sleeping state and adds the "sleeping_state_changed" by copying what's done for RigidBody3D.

Example project using the new APIs: ragdoll-sleep.zip (hit spacebar to reset the bones)

It seems like PhysicalBone3D nodes support sleeping, but it wasn't
possible to access this state unless going through the PhysicsServer3D
directly, and even then it would miss the convenient
"sleeping_state_changed" signal.

This commit exposes the sleeping state and adds the
"sleeping_state_changed" by copying what's done for RigidBody3D.
@TokageItLab
Copy link
Member

TokageItLab commented Dec 5, 2024

It is indeed strange that PhysicalBone3D can_sleep is true but there is no sleeping property. However, I am not sure if we should add a sleeping property for each classes without extending.

Is there a reason not to add a sleeping property to PhysicsBody3D? Or maybe we should add an intermediate class like RigidableBody3D between RigidBody3D and PhysicsBody3D and add the sleeping property there.

So the decision requires the opinion of the physics team. cc @rburing

@paul-marechal
Copy link
Author

paul-marechal commented Dec 6, 2024

@TokageItLab I hear you as I also wondered if a refactoring was due.

But here's why I think this PR is fine as is:

  • This is my very first contribution to Godot's code base, but also my first contribution to a C++ project at all
  • I am not confident enough to carry on a refactor of Node classes
  • I submitted this contribution as it is something I need for my own projects
  • The refactoring can be deferred for when someone with more experience wants to tackle it

This change doesn't add much more debt for when the refactoring will happen: currently is just means there's a little code duplication, and it may actually help direct the effort when refactoring by showing where things need to be shared.

@TokageItLab
Copy link
Member

Yes, refactoring can be done later. However, I cannot judge the reasonableness of can_sleep being true and the safety of implementing sleeping, so the decision must be made by the physics team.

@paul-marechal
Copy link
Author

paul-marechal commented Dec 6, 2024

I'd be happy to hear from the physics team to see if my understanding is correct:

PhysicalBone3D nodes are regular PhysicsBody3D that are either static or kinematic while following animations, and become rigid when simulating. Something I haven't considered is what happens to the sleeping state when in either static or kinematic mode? I can look into this.

However, when in rigid mode there's most definitely value in having access to the sleeping state: the simulation will eventually settle and we don't want to waste physics resources on non-moving bodies (the physical bones). Once the bones are sleeping I can stop doing whatever processing I was doing on them and only resume when woken up.

@paul-marechal
Copy link
Author

Haven't been able to spend more time on this yet but I'll eventually reach the point where this becomes relevant again for me in the future. I'll close this PR to reduce clutter for now.

@AThousandShips AThousandShips removed this from the 4.x milestone Apr 13, 2025
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