godotengine
Implement a base class `SkeletonModifier3D` as refactoring for nodes that may modify `Skeleton3D`
- Closes https://github.com/godotengine/godot-proposals/issues/9015
- Closes https://github.com/godotengine/godot-proposals/issues/7820
- Closes https://github.com/godotengine/godot-proposals/issues/4664
Add SkeletonModifier3D as base class for some Nodes
The base class for the following Nodes will be SkeletonModifier3D:
- SkeletonIK3D
- BoneAttachment3D
- OpenXRHand
What is SkeletonModifier3D
SkeletonModifier3D retrieves a Skeleton by either having a Skeleton parent or by specifying an external Skeleton. This is almost the same way as the current BoneAttachment.
SkeletonModifier3D, like AnimationMixer, allows the user to select a process as CallbackModeProcess. Also, by specifying a target AnimationMixer, it is possible to process the animation in the same process as the AnimationMixer. At this time, SkeletonModifier3D always performs modification after the AnimationMixer.
In other words, the actual SkeletonModifier3D process only registers its own ObjectID with AnimationMixer. AnimationMixer performs modification after the registered SkeletonModifier3D playback process. If AnimationMixer has already processed the animations at the time of registration, SkeletonModifier3D will not register it and will perform modification on the fly.
Q. Why instead of connecting the signal like AnimationMixer.mixer_applied to each modifier, the order is managed by registering object IDs?
A. Because the signal does not care / cannot handle about the order of processing when multiple objects are connected.
This allows the processing order of SkeletonModifier3D to depend on the scene tree order and ProcessPriority, and prevents modifications from overwriting animations.
BTW, for performance reasons, both NodePath object pointers are cached.
Separate the methods related to PhysicalBone from Skeleton3D as PhysicalBoneSimulator3D
PhysicalBoneSimulator3D has SkeletonModifier3D as its base class.
PhysicalBoneSimulator3D has the following roles:
- Stores the simulated pose separately from the Skeleton3D.
- Apply the results of the simulation to Skeleton3D
Physical simulation process is performed on the PhysicalBone side of the child node. PhysicalBoneSimulator3D's CallbackModeProcess only affects when the simulation results are applied to Skeleton3D. So it does not affect the child nodes or the physical simulation.
Finally, for compatibility, Skeleton3D has PhysicalBoneSimulator3D as a virtual child. This ensures compatibility without changing the tree structure of old projects.
Deprecate pose override
Considering why bone_pose_override exists, there are two main reasons:
- to create a pose that has priority over animation playback
- to manage Transform separately from pose
This PR makes pose override internally unused. For about 1, it is solved by the property target_animation_mixer. For about 2, it should not kept the simulation data on the skeleton side in the first place, and is solved by having the PhysicalBone stored in the PhysicalBoneSimulator.
This allows multiple IK to be applied to a single Skeleton and blended with the results of physics simulations.
https://github.com/godotengine/godot/assets/61938263/13006b41-c5f8-4624-9d97-cb6576882067
https://github.com/godotengine/godot/assets/61938263/e388beb8-1946-4cb8-ac12-9121c661a7b3
However, pose override is left as a deprecated method because it would be completely incompatible with old projects if it were eliminated.
- Supersedes/Closes https://github.com/godotengine/godot/pull/82192
Breaking Compatibility
This PR may change the update timing and processing order of Nodes with SkeletonModifier3D as their base class, which may change the behavior of the program. You will need to set the target AnimationMixer and Process Priority as necessary. However, if Process Priority is already set in the old project, there should be no problem.
To take it to the extreme, if you set the ProcessPriority of IK and PhysicalBoneSimulator to INFINITY - 1 and the ProcessPriority of BoneAttachment to INFINITY, you should be compatible with previous projects as long as you have not set ProcessPriority = INFINITY to the Node in the Scene in most of the case.
The following properties will change:
- OpenXRHand.hand_skeleton
- Skeleton3D.animate_physical_bones
However, the setter and getter will remain, as compatibility methods exist.
- Closes https://github.com/godotengine/godot/issues/76502
- Closes https://github.com/godotengine/godot/issues/88181
Other concerns
- We may need a function like reset_on_save() in SkeletonModifire. Would this be sufficient to reset Pose back to Rest?
- It is possible to apply another IK to a pose after the IK has been applied, but if you want those IKs to be based on the same pose -- in other words, they cannot be blended in "parallel". For that, perhaps something like a SkeletonModifierTree like AnimationTree would be needed, but I don't think it is necessary at this time. Because for gaming applications, most use cases should be "serial" blending, such as applying IKs to poses after an animation clip is played.
Production edit: closes godotengine/godot-roadmap#81
You have a typo. It should be “StatusModifier3D”.
@CrayolaEater I don't understand what you mean. It would be inappropriate since this Node focus on Skeleton3D modifications.
About deprecating Pose Override:
As of now I am using the bones overridden transform data (the state after the IK is applied) for a raycast check to know if I should lock my character's IK foot to the ground.
I don't understand if I can still do this with this PR or not ?
Can I still have a bone attachment for the overridden state ?
Also can I choose that the bone attachment instead uses the Animation Transform without any IK or physical modification ? This is also useful for raycast checking to unlock the foot from the ground.
@ywmaa
If you are using "only" get_bone_global_pose_override() without set_bone_global_pose_override(), you may need to rewrite it to get_bone_global_pose(). Since BoneAttachment3D no longer uses override.
I am wondering if it is necessary to make get_bone_global_pose_override() as a wrapper function for get_bone_global_pose() for compatibility, since it should not be a wrapper function for applications that use set_bone_global_pose_override().
Also can I choose that the bone attachment instead uses the Animation Transform without any IK or physical modification ? This is also useful for raycast checking to unlock the foot from the ground.
This should not be a problem. It should work if you get transform of the BoneAttachment before the IK application process: Probably by creating a class that extends SkeletonModificaton3D and registering it to AnimationMixer before IK it is possible.
However it might be fine to add some signals between the AnimationMixer processes for usability. We can follow up later if that is needed.
Signals AnimationMixer.mixer_applied and SkeletonModifire3D.modification_processed are added. It should be useful to get the current skeleton pose at each step of the modification process, like get_bone_pose_no_override() in the past.
Also, the application process of PhysicalBoneSimulator3D has been revised. Since set_bone_global_pose() executes update process by the dirty flag, so if multiple global poses are to be applied, it can prevent to reduce the performance by pre-calculating them as local poses.
However, for BoneAttachments, it is inevitable to use set_bone_global_pose() as it is now because it cannot be known in advance how many BoneAttachments will be added. If you are using a large number of BoneAttachments with override mode and performance is an issue, perhaps we should consider adding a Node like MultipleBoneAttachment which has several Nodes as children similar to PhysicalBoneSimulator3D.
Hey General question about this. Will this be useful for Full body IK in VR Games?
Yes, I believe this should be the base for implementing them without conflicting with existing modifiers.
Due to a concerns raised by @lyuma (and I had the same concerns while working on it), so now the design of the BoneAttachment is changing a bit.
The old BoneAttachment constantly observed bone pose transform with connecting signal, which could cause a significant performance hit each time a pose update was triggered causing the process to run by another SkeletonModifier.
With the change, BoneAttachment will only perform updates in a set process.
This means that in past projects that used AnimationMixer and BoneAttachment at the same time, unless you set target AnimationMixer on BoneAttachment or set process priority BoneAttachment deformation updates may not occur unless you set a target AnimationMixer on the BoneAttachment or set a process priority.
Simply put, the old BoneAttachment exclusivity control is too strong, so this PR will change it has a flexible update process by SkeletonModifier inheritance.
A question was raised above by @ywmaa:
Also can I choose that the bone attachment instead uses the Animation Transform without any IK or physical modification ? This is also useful for raycast checking to unlock the foot from the ground.
For example, this change allows, if the BoneAttachment put before the IK in the SceneTree, so that the BoneAttachment only reflects the results of the AnimationMixer and ignores the modification of the IK. And if you want to reflect IK modification, just change the order.
https://github.com/godotengine/godot/assets/61938263/330154e2-27f2-42c7-ab0b-f4406893fd13
I've been developing a set of tracked-body provider scripts which can drive skeletons from tracking data coming in from MoCap suits or camera-trackers. These are 100% gdscript using UDPServer and some packet-decoding logic. I feel they would want to be changed to extend from SkeletonModifier3D, however that class is marked as Abstract. I guess I could reimplement them in C++ via GDExtension; but it might be valuable to have some sort of script-extensible node somewhere under the SkeletonModifier3D tree for scriptable driving.
UPDATE: On further consideration I'm hoping to convert the provider-scripts into separate addons, and possibly have the companies take over support at a commercial level. As such it may be better to have them migrate to GDExtension anyway - as this would allow them to also switch to using their commercial C/C++ MoCap libraries if they choose to.
The virtual method _process_modification() is exposed, so I assume this is actually usable enough without the need to abstract. But I'm not sure what should be marked as abstract in terms of Godot's policy (what do I do if nothing works visually but the virtual method works), so I'm just marking it as abstract for now. I want opinions from some reviewers.
just marking it as abstract for now
The reason is because the node does nothing by itself. However, abstract classes cannot be implemented in script (I just confirmed this is still an issue in godot 4.2), so it would not be useful as an abstract node. Unfortunately we have to remove abstract so that users can implement their own modifiers.
I wish there would be a way to indicate in the UI that a particular node should always have a script attached. but I think currently there is no way to indicate this
Comments from the production team:
- if we have a game that works on 4.2 that use the deprecated features, will the game still work on 4.3? It seems so if we check the changes, but could you make it very clear about this subject?
- if the deprecated features still work and the users want to upgrade their stack, documentation will be needed to explain how to upgrade from the past flow to the new one.
- the animation team needs a consensus on this (cc. @lyuma and @SaracenOne). If more time is needed, there's no need to rush this PR for 4.3.
For users using "bone_global_pose_override":
If you are using "only" get_bone_global_pose_override() without set_bone_global_pose_override(), you may need to rewrite it to get_bone_global_pose(). Since BoneAttachment3D no longer uses override.
If the user has set their own values with set_bone_global_pose_override() and retrieved them with get_bone_global_pose_override(), the old methods will remain to preserve project compatibility. The program will not be broken in this case.
If the user is using a SkeletonModifier and is retrieving the transform with get_bone_global_pose_override(), the program needs to rewrite get_bone_global_pose_override() to get_bone_global_pose().
For users using SkeletonModifier with BoneAttachment or AnimationPlayer:
If SkeletonModifier is used with BoneAttachment or AnimationPlayer at the same time, the process order or target animation player needs to be set.
To take it to the extreme, if you set the ProcessPriority of IK and PhysicalBoneSimulator to INFINITY - 1 and the ProcessPriority of BoneAttachment to INFINITY, you should be compatible with previous projects as long as you have not set ProcessPriority = INFINITY to the Node in the Scene in most of the case.
This implementation blocks any other changes to Skeleton, such as IK improvements, so efforts should be made to add it to 4.3 as much as possible.
Since @reduz seemed to have some concerns about the deprecation of override and the need to specify an AnimationMixer at the Contributors meeting, so I explain it in the following:
TL;DR
The bone_global_pose_override is a hack to make the result of PhysicsBone or BoneAttachment modification always override the AnimationMixer result.
The bone_global_pose_override hack is not sufficient for modifiers such as IK, Constraint and etc. which need to take into account AnimationMixer result.
For IK and Constraint and etc., we need a way to properly manage ProcessPriority with AnimationMixer, which can replace the bone_global_pose_override hack.
Finally, bone_global_pose_override is deprecated because the two management methods are conflicted and should not be mixed up in there.
Why sould we deprecate bone_global_pose_override
We can think up three different use cases for the current bone_global_pose_override:
- To set global_pose
- To store one more pose in addition to the original pose
- To override a pose prior to any other transform
Then, case 1 and case 2 are not the real reasons for the necessity.
For the case 1, it is sufficient if there is a method to convert between local pose and global pose.
For the case 2, it should be stored on the modifier side, not on the Skeleton side. If "only one" modifier uses override, it is effective to obtained the result without the modifier's influence with get_bone_global_pose_no_override(), but if multiple modifiers use override, the assumption breaks down completely; This means that when Modifier A and Modifier B are using override, if you only want to remove the effect of Modifier A, it will also remove the effect of Modifier B. All modifiers could be chained, so override should not be used in the first place.
So the only reason why bone_global_pose_override is needed is to apply the pose overriding the AnimationMixer result, as the case 3.
BoneAttachment and PhysicsBone completely override the AnimationMixer result, so the order of processing with the AnimationMixer during the frame does not need to be considered. In this case, they can get the benefit of bone_global_pose_override.
On the other hand, bone_global_pose_override has a negative effect: Modifiers (such as IK, Constraint and etc.) do not need to use override at all actually, but if they need to take account of the bone_global_pose_override results, they will have to use bone_global_pose_override as well. In other words, as long as PhysicsBone and others use bone_global_pose_override, BoneAttachment will be forced to use bone_global_pose_override to take into account those results. Even if you use clear_bones_global_pose_override(), it would be a nightmare of code that would confuse where and what pose is being processed right now. Moreover, additional workarounds such as #82192 which are not essentially necessary is required.
Why do the such modifiers (IK, Constraint and etc.) cannot receive the benefit of overriding the AnimationMixer result? Because the modifiers must consider the order of processing with the AnimationMixer in the frame, and must process calculation after the AnimationMixer.
So, what the modifiers need is "the way to properly manage the Process Priority between the Modifier and AnimationMixer", not "the way to ignore and override the result of AnimationMixer".
As a cheap workaround, the old SkeletonIK3D attempted to solve that problem by "setting the ProcessPriority default to 1", but this is obviously not enough and hacky.
This PR ensures consistency in their ordering by specifying the Target Animation Mixer.
By the way, BoneAttachment and PhysicsBone also should not need bone_global_pose_override if they are processed later than AnimationMixer in the frame.
In other words, if there is a way to properly manage ProcessPriority between SkeletonModifier and AnimationMixer, bone_global_pose_override becomes unnecessary and should not be used because of conflicting uses.
Another idea to manage ProcessPriority between SkeletonModifier and AnimationMixer
Actually, I have another idea to solve those problems without specifying Target Animation Mixer.
It is to add _post_process() and _physics_post_process() to SceneTree, but I avoided it because it would be exaggerated implementation just to solve the problem between SkeletonModifier and AnimationMixer.
However, that solution may be better for compatibility.
In any case, override must be made deprecated, but it makes the step of specifying an AnimationMixer is no longer needed to make the Modifiers work.
(Although, CallbackModeProcess is needed to set correctly to match AnimationMixers. Also, if the SkeletonModifier needs to handle RootMotion, it may be more convenient to specify an AnimationMixer to add it to the AnimationMixer...)
I can change this PR to do this (add _post_process()) if it is preferable for the core.
More compatibility
For compatibility, I believe PhysicalBone compatibility mostly covered since when they are direct children of Skeleton without PhysicalBoneSimulator, they are in compatibility mode.
However, the behavior of BoneAttachment changes quite significantly, so if that is a concern for you, we can leave the old BoneAttachment as a Deprecated class and add the new BoneAttachment as like "BoneManipulator3D".
Why sould we deprecate bone_global_pose_override
Addons like jigglebones use set_bone_global_pose_override()
https://github.com/TokisanGames/godot-jigglebones/blob/master/addons/jigglebones/jigglebone.gd
@TokisanGames As I explained above, programs using both set_bone_global_pose_override() and get_bone_global_pose() should keep to work after this PR, since the methods themselves remain. Deprecated does not mean remove it from the core, only makes the warning. When it actually occurs that they are removed is in Godot5.
Also we are planning to re-add something like jiggle bone to core in the future (maybe 4.4 or 4.5).
I tested this PR with an existing project that uses XRHandModifier3D and, unfortunately, it doesn't work. For some reason XRHandModifier3D::_update_skeleton() never gets called.
Looking at the code, I'm a little confused as to how this is supposed to work, so I'm not really sure how to fix it... It seems like _update_process_mode() never gets called, because it's the setters that call it in the first place (set_active(), set_callback_process_mode(), etc) but they never run their logic because I haven't changed those properties from their default values.
If I mess with the values of one of those properties while the app is running (via "Deploy with Remote Debug") then the hand tracking will kick in. But if I save the scene with the values modified (for example, switching to a "Callback Mode Process" of "Physics"), it still doesn't work, possibly because of the order that the setters run in? I'm not sure.
@dsnopek If an AnimationPlayer is present in the scene, you must specify target AnimationPlayer if the XRHandModifier is before the AnimationPlayer in the SceneTree (if ProcessPriority is lower). But if not so and it doesn't work, then something is odd since other modifiers work. Also, I just added an XRHandModifier to the scene without a tracker, but _update_skeleton() itself is called correctly.
I would like to look into that later, but I don't have a debugging environment for XR controller. Is there a test project that works without a tracker? Or maybe @fire , @lyuma , @SaracenOne who have XR controller, can help me test it if there is a test project.
Oh indeed, the behavior of Activation when there is no AnimationPlayer seems a bit odd. Perhaps this problem is not just XRModifier, I'll try to fix it tomorrow.
@dsnopek Alright, I think that's fixed. There was some confusion between the old SkeletonIK and BoneAttachment-based code and the AnimationMixer-based code regarding the registration of processes in CallbackProcessMode and active on _notification(). Now it has been unified with the AnimationMixer-based code and organized.
Okay, I am not going to review the source code in detail yet because I think I have some fundamental differences on how this should work, so here is a list of bullet points of what changes I would make to this:
Skeleton should track and process the modifiers
The skeleton should track the modifiers as children nodes and have an array of child node modifiers (pointers to) that is ordered by child order. You can do this by overriding the following virtual functions from Node in Skeleton and implement something like this:
void Skeleton3D::add_child_notify(Node *p_child) {
if (Object::cast_to<SkeletonModifier3D)>(p_child)) {
modifier_list_dirty=true;
}
// to be used when not wanted
}
void Skeleton3D::remove_child_notify(Node *p_child) {
if (Object::cast_to<SkeletonModifier3D)>(p_child)) {
modifier_list_dirty=true;
}
// to be used when not wanted
}
void Skeleton3D::move_child_notify(Node *p_child) {
if (Object::cast_to<SkeletonModifier3D)>(p_child)) {
modifier_list_dirty=true;
}
// to be used when not wanted
}
likewise, it should be skeleton implementing the modifier process mode (as process or physics). Make it use its own enum, not the animationmixer one. Eventually, this should use the NOTIFICATION_PROCESS_INTERNAL (or PHYSICS_PROCESS) depending on what you choose.
In that notification, check if the list of child modifiers is dirty, then clear your cache (likely something like a LocalVector<SkeletonModifier3D*> as a property of Skeleton3D), go through all your childs of Skeleton and create the cache again. This warrants that modifiers are always processed in scene order, so if the users want to change the order or processing, they just change the order of nodes.
Again, you need to only turn on processing in the skeleton if any modifier exists and is active, otherwise turn it off.
Modifiers should have a blend amount
I think doing this is a much simpler design than what is done in this PR. When processing the list of modifiers, you always clear the base transform for each bone pose override at the beginning (this can be an internal one, not necessarily the current one?) so each modifier you process blends the previous transform to the new one based on the blend amount for the modifier.
This lets you control how much a modifier is affecting via a regular animation property (that you can animate), so no need to do anything more complex than this and you can keep AnimationMixer as is.
Other feedback
I don´t understand why BoneAttachment needs to be a SkeletonModifier.
@reduz
Skeleton should track and process the modifiers
If so, does SkeletonModifire always need to be after AnimationMixer?
The main problem that SkeletonModifire wanted to solve is to do modifire without changing the order of the AnimationMixer in the scene tree. This is especially necessary for imported scenes. How do we solve this?
If you agree to do the following, then I believe that change is acceptable.
Another idea to manage ProcessPriority between SkeletonModifier and AnimationMixer Actually, I have another idea to solve those problems without specifying Target Animation Mixer.
It is to add _post_process() and _physics_post_process() to SceneTree, but I avoided it because it would be exaggerated implementation just to solve the problem between SkeletonModifier and AnimationMixer.
However, that solution may be better for compatibility.
In any case, override must be made deprecated, but it makes the step of specifying an AnimationMixer is no longer needed to make the Modifiers work.
(Although, CallbackModeProcess is needed to set correctly to match AnimationMixers. Also, if the SkeletonModifier needs to handle RootMotion, it may be more convenient to specify an AnimationMixer to add it to the AnimationMixer...)
I can change this PR to do this (add _post_process()) if it is preferable for the core.
https://github.com/godotengine/godot/pull/87888#issuecomment-1945334496
I don´t understand why BoneAttachment needs to be a SkeletonModifier.
Because there are cases where we want to get the pose after a specific modifire. Since it could be a process between several modifiers, BoneAttachment needs a way to listen to it in some way. See also https://github.com/godotengine/godot/pull/87888#issuecomment-1925119036 https://github.com/godotengine/godot/pull/82192
Here is some pseudocode of how I would implement the logic if its useful, feel free to use if you need:
void Skeleton3D::add_child_notify(Node *p_child) {
if (Object::cast_to<SkeletonModifier3D)>(p_child)) {
modifier_list_dirty=true;
}
// to be used when not wanted
}
void Skeleton3D::remove_child_notify(Node *p_child) {
if (Object::cast_to<SkeletonModifier3D)>(p_child)) {
modifier_list_dirty=true;
}
// to be used when not wanted
}
void Skeleton3D::move_child_notify(Node *p_child) {
if (Object::cast_to<SkeletonModifier3D)>(p_child)) {
modifier_list_dirty=true;
}
// to be used when not wanted
}
void Skeleton3D::_notification(int p_what) {
///...//
case NOTIFICATION_INTERNAL_PROCESS:
case NOTIFICATION_INTERNAL_PHYSICS_PROCESS: {
if (modifier_list_dirty) {
modifier_list_cache.clear();
for(int i=0;i<get_child_count();i++) {
SkeletonModifier3D *c = Object::cast_to<SkeletonModifier3D>(get_child(i));
if (c) {
modifier_list_cache.append(c);
}
}
modifier_list_dirty=false;
}
for(int i in all bones) {
// The modifiers will set these when processed, so clear them before processing them
bone[i].modifier_xform = Transform(); //clear
bone[i].modifier_xform_blend = 0;
}
for(int i=0;i<modifier_list_cache.size();i++) {
modifier_list_cache[i]->modifier_process(delta); // this function processes the modifier and likely calls an internal Skeleton3D function that sets bone[x].modifier_xform and xform_blend. Something like (private but friend for SkeletonModifier3D) Skeleton3D::_blend_modifier(int p_bone, const Transform3D& p_transform, float p_blend);
}
} break;
///...//
}
//how to do the blending for each modifier?
Skeleton3D::_blend_modifier(int p_bone, const Transform3D& p_modifier_transform, floatp_modifier_blendp_blend);
// should work kind of like:
if (p_modifier_blend > 0.999) {
bone[p_bone].modifier_xform = p_modifier_transform;
bone[p_bone].modifier_xform blend = 1.0;
} else {
bone[p_bone].modifier_xform = bone[i].modifier_xform.interpolate_with(p_modifier_transform, p_modifier_blend);
bone[p_bone].modifier_xform blend = bone[i].modifier_xform blend * (1.0 - p_modifier_blend) + p_modifier_blend;
}
From discussion in ContributorsChat:
Skeleton should track and process the modifiers
We agreed, we should register the modification in the skeleton so that it is always multiplied when calculating the bone pose. I'll never know until I try it, but I think it would work.
I don´t understand why BoneAttachment needs to be a SkeletonModifier.
For the BoneAttachment, we need to at least avoid using pose override, but basically keep the previous functionality since after the above changes, it makes no sense to make it a SkeletonModifier.
To get only the modifications of a specific Modifier, you will need to either use a signal from the script to the SkeletonModifier or create a custom SkeletonModifier. AbstractClass is not available in GDScript, so it will be inconvenient for a while but will be improved later by some means.
@reduz I was working with the above recommendations, but some problems came up, so I write down them below.
The skeleton should track the modifiers as children nodes and have an array of child node modifiers (pointers to) that is ordered by child order. You can do this by overriding the following virtual functions from Node in Skeleton and implement something like this:
- SkeletonModifier is forced to be a child of Skeleton. This means that External Skeletons such as XRBody and XRHand's target property are discontinued. Is it okay?
- Is the process_priority of the SkeletonModifier ignored?
If the above is acceptable, I don't think this is much of a blocker.
This lets you control how much a modifier is affecting via a regular animation property (that you can animate), so no need to do anything more complex than this and you can keep AnimationMixer as is.
- Modifiers that animate themselves, such as SpringBone, must be processed once per frame in InternalProcess in Skeleton, so that they must be processed even if set_bone() is not called. At this point, there is no way to resolve the order of processing with the AnimationMixer, and if the Skeleton's InternalProcess is before/after the AnimationMixer's InternalProcess, the SpringBone may be processed twice per frame. However, this is problematic because SpringBone must use the difference from the previous calculation. How should this be resolved?
This is quite problematic since the modifier should calculate it only once per frame.
Also, the modifier results from a calculation based on the pose after the animation is applied are different from the results based on the pose before the animation is applied. So, it makes no sense to calculate the Transform and then change the timing of its application. The result would depend on the ProcessPriority of the Skeleton and AnimationMixer, so it is impossible to do the calculation only once without resolving the processing order between the AnimationMixer and Skeleton. It might be possible to solve this problem by allowing AnimationMixer to be set to Skeleton, but I think this is not good for the architectural design.
From that, I still suspect that we need to choose between two ways: adding something like _post_process() to the SceneTree or registering SkeletonModifiers to the AnimationMixer as this PR originally. (I recall that Skin deformation was a performance bottleneck during pose updating, so SceneTree ::_post_process() may help solve that as well)
From discussions on RocketChat:
It looks like some changes are needed for APIs that use NOTIFICATION_UPDATE_SKELETON. It should only be called once per frame and should not be executed on the fly.
SkeletonModifier is forced to be a child of Skeleton.
Currently the XRBodyModifier3D node positions itself at the "Root" tracking location (on the ground under the hips). We then usually have the avatar childed under this node so the avatar moves and rotates as the player walks around their tracking-space. Having XRBodyModifier3D acting as a guaranteed "Root" location (even if the skeleton doesn't have a root bone) is useful for things beyond the avatar such as positioning floating displays around the user.
If XRBodyModifier3D is a SkeletonModifier3D and SkeletonModifier3D nodes must be direct children of the Skeleton3D then would we lose this positioning capability? One alternative might be to have XRBodyModifier3D move the entire Skeleton3D node to the "Root" tracking location.
My understanding is the parent child relationship is only a recommendation to ensure consistent update order, but not required. Would it be possible to adjust process priority instead, or manually tick updates?
EDIT: I see there is some new change which somehow tied the skeleton to its modifiers, but it should still be possible to manually tick, no?
I do have a question about what happens about in the case of (indirect) descendants, in particular if there are multiple nested skeletons.
My understanding is that in order for a Skeleton to create a list of Modifiers it must be a child of the Skeleton. It may be possible to create the list by iterating over the SceneTree, but it is not a good idea to iterate every time the SceneTree is changed.