Remove SkeletonModificationStack and convert IK solvers to nodes.

[lyuma]

Describe the project you are working on

RenIK, a skeleton full body IK implementation.

Describe the problem or limitation you are having in your project

SkeletonModification3D and SkeletonModificationStack3D are doubly-nested (two layers deep) which makes it hard to work with. And analogously for Skeleton2D

Further, because they are resources and were holding runtime state, it led to unusual glitches when duplicating skeletons and in some cases required restarting the editor when testing.

The fact is that the current implementation has bugs, and some change is needed. The open question is what type of change.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Skeleton Modifications will be replaced with Nodes, and the SkeletonModificationStack resources will be removed as well as the property in Skeleton3D / Skeleton2D.

Additionally, IK solvers will be changed to operate on the bone_pose instead of using overrides.

Nodes can be added anywhere in the tree, so they allow making use of the tree order for processing. For example, so some nodes could be added before an animation player and some after.

((Note: There is an alternative approach to nodes, which is to keep the Resources as they are, but use a RefCounted "Playback" object, like AnimationTree has. I think this would be more work, but it is the competing proposal.))

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Iteration order is not a problem because nodes update in tree order, so IK solvers can be composed simply by re-ordering children.

The SkeletonModification3DStackHolder could be implemented by making a child node with more children of its own.

If this enhancement will not be used often, can it be worked around with a few lines of script?

You could write IK implementations as nodes.

Is there a reason why this should be core and not an add-on in the asset library?

Skeleton3D and Skeleton2D are core.

[TokageItLab]

Does it contain refactoring around the bone override? They were implemented for the ModificationStack and seem to complicate the whole skeleton..

[lyuma]

As far as I know, the ModificationStack has no effect on how bone override works.

That said, the proposal to use bone_pose where possible is just a proposed simplification of the existing IK implementations... it's not proposing to remove or change how they works. But as far as I can see, there is no need to use override in most cases: setting bone pose should work just as well.

[lyuma]

I wrote this proposal because the SkeletonModificationStack and SkeletonModification resources were added in Godot 4, to handle processing. There were parts of the original GSOC 2020 change godotengine/godot#51368 which were not properly reviewed or showed a lack of understanding of Godot's architecture.

As a result, SkeletonModificationStack has bugs, most importantly that some of the implementations hold references to runtime state, such as node references (through a complicated cache system) and physics properties which cause duplicate nodes to misbehave in some cases.

This proposal is trying to discover the best way to address a bug introduced as part of SkeletonModificationStack. I really could have filed it as a bug, but I put it in as a proposal.

As far as I can see, there are there are two reasonable ways to move forward:

1. Make an object analogous to AnimationNodeStateMachinePlayback which is created fresh each time and holds runtime state. Additionally, improve the inspector to make editing double-nested resources easier, or remove the outer layer of nesting since it seems to serve no purpose at all.

2. (This proposal) Since each modification is an object which has a single processing function, it serves the same purpose as a node, so use Nodes. This is more in-line with how Godot is designed to work. Nodes are instantiated so can safely contain runtime state, are designed to process, have a well-defined tree execution order.

[TokageItLab]

I think that a lot of properties and functions have been added to the skeleton that should not be there, just for the IK, and they are contaminating the skeleton class.

For example, rest_bone_forward_axis/vector and local_pose_override should be on the IK side instead skeleton, so they and their related functions need to be removed from the skeleton.

[lyuma]

Yes, I agree with all of that. There are also some quirks with the fact that get_bone_children returns a reference to an internal Vector which could be manipulated (or call set_bone_children) so as to cause an inconsistent data structure.

[TokageItLab]

Closed by godotengine/godot#71137