Fix console errors and crash in cleanup code for PhysicalBoneSimulator3D

[mikest]

Skeleton3D/PhysicalBoneSimulator does not clean up internal state when removing bones

• Reproduced in 4.3, 4.4b3, 4.4
• Windows 11, Godot-4.4beta3, Godot-4.4

Description

There is a bug which prevents dynamically removing and then adding bones to a skeleton.

The use case for dynamically constructing skeletons is in creating generative creatures, mechanism, and so on.
For my example, I use a ball and chain simulation with dynamic number of links. The scene componentry builds a chain link up from a link template and a ball template. This repo is a test case for demonstrating the behavior.

The Chain.gd file creates a ball-and-chain simulation with a dynamic number of links that can be changed directly within the editor. It is the only script file of interest for the project.

Project for reproducing:

https://github.com/mikest/godot-chain/

Steps to reproduce crash

1. Open project.
2. Open Scene "Chain.tscn". You should see a monkey head on a chain.
3. Select the root node.
4. In the properties panel there is a counter for the number of chain links.
5. Increase the counter by one.

Results:

1. Editor segfaults and exits.

Investigation

Skeleton3D, PhysicalBoneSimulator3D, and PhysicalBone3D all maintain a set of internal caching structures that appear to be there as speed optimizations. These caching structures are not properly kept in sync when removing physical bones or when calling Skeleton3D::clear_bones() to remove logical bones. In most cases, this results in numerous errors logged in the console, but on occasion, it can also result in crashes.

This PR is an attempt to reduce those errors and to reduce the conditions which trigger spurious console logging from cache size misses due to stale cache data.

To address this, I included more cache invalidation in Skeleton3D::clear_bones().

I also fixed cache sync problems with the bone_global_pose_dirty and nested_set_offset_to_bone_index in Skeleton3D::add_bone(const String &p_name).

Many of the logging errors that arrise after removing and adding a bone are due to caches being rebuilt before the PhysicalBone3D has been added to the tree. I have moved the cache rebuild to NOTIFICATION_POST_ENTER_TREE. This fixes the errors from _reload_joints.

The last change I made was to the PhysicalBoneSimulator3D::_pose_updated() call, which when detecting a cache size mismatch between the skeleton bone count and the internal bones cache will rebuild it. This happens when a physical bone is still attached and the Skeleton clears the bones and then rebuilds them to a different size. The simulator will make pose updates and cause cache misses.

Related Issues

Fix error spam at start with Skeleton3D modifiers #102030
#102030

NOTE: I'm not terribly familiar with the godot internals and this is my first PR, so extra attention is probably warranted.

[mikest]

Apologies. I deleted my local branch and that invalidated the PR. This is the resubmit, I'll leave this one open.

ref:
#102939 (comment)

[SaracenOne]

Hmm...this seems fine in so far as a way to make sure the simulator is able to keep track of bone count changes. After looking over it a bit, I can't see anything obviously problematic with this approach and it does appear to address the problem.

Would recommend cleaning up the redundant switch cases, but other than that, I think this will be fine to approve.

[SaracenOne]

LGTM 👍

[akien-mga]

Looks good! Could you squash the commits? See PR workflow for instructions.

[mikest]

Looks good! Could you squash the commits? See PR workflow for instructions.

@akien-mga I think I did the squash correctly as my branch only shows one commit, but I'm still seeing 7 here and "processing updates" has been running for some time? I'm not sure if it is slow or I broke something.

Apologies if I missed something obvious. This is my first time using git's PR system 😅

[akien-mga]

It's the first time I see a PR stuck like this with "Processing updates". It might have been a GitHub fluke.

You could try to do an amend (with no edit) and force push again to get GitHub to hopefully wake up.

[akien-mga]

You could try to do an amend (with no edit) and force push again to get GitHub to hopefully wake up.

I did that myself (with a commit message tweak) and that seemed to do the trick this time.

[Repiteo]

Thanks! Congratulations on your first merged contribution! 🎉

[mikest]

@Repiteo thank you! i've been using this engine for a few years, feels really good to make a tiny contribution back ^__^

[akien-mga]

Cherry-picked for 4.4.1.