Refactored bone_pose_override in Skeleton3D

[TokageItLab]

Currently, the bone stores three pose values: bone_pose, bone_global_pose_override and bone_local_pose_override. However, it is not possible to get which of those values is used and how much. In other words we can't get the correct pose that the Skeleton is finally displaying. Moreover, global_pose_override and local_pose_override are not tied together so it looks mess.

I'm working on implementing RetargetNode (godotengine/godot-proposals#3379) but due to these factors, it is difficult to get the final bone pose, and we can't retarget the modifier transforms.

This will do some refactoring.

• We should have two readable pose values, bone_pose and bone_pose_override
• Remove bool president and float amount, then implement bool is_override_enabled instead (if amount is needed, use interpolate_with() before passing the pose to set_bone_pose_override(); president should be managed in modifier's priority)
• Caching the value of global_pose is fine for performance reasons, but it is a value used for conversion between local <-> global and we should not apply it directly as a pose
• get_bone_pose() returns the final displayed pose, get_bone_pose_no_override() returns the value without the override for the bone

If this is approved, the same changes may need to be made to Skeleton2D for consistency.

[realkotob]

Code changes look good to me 👍

I didn't test it though, @TokageItLab if you can provide a sample project I can test it, although that might not be necessary if someone more familiar with the code takes a look, maybe @fire

[TokageItLab]

I organized it a little more. I've tested at least SkeletonIK node, but I'm missing some testing on ModifierStack. @fire @TwistedTwigleg Can you help me with the test and review?

[TwistedTwigleg]

Sure, I'll take a look at the modifications soon in the test project I have and will let you know the results!

[TwistedTwigleg]

I just tested and everything seems to be working fine! I had a slight issue with one of the character setups, but I think it's part of the model import (I probably needed to re-import) than the changes in this PR as all of the test meshes worked fine.

The only thing that didn't work was that when you disable the modification stack, it doesn't reset the bone poses back to how it was prior to the override, but that is literally the only thing that I could spot and I'm not sure if it was that way prior to this PR or not.

[TokageItLab]

@TwistedTwigleg Thank you a lot! Probably the reason why the poses are not reset when modifier is disabled is that I forgot to enable the pose_cache_dirty flag in clear_bones_pose_override(). So I fixed.

[fire]

I did some style review of the code. @TwistedTwigleg reviewed this work in a practical demo.

Looks good to me.

I added some c++ comment changes.

Normally I'd ask Reduz, but he's currently busy, and @TwistedTwigleg was in this area of the code.

[TokageItLab]

Superseded by #56902.

[TokageItLab]

Request of reopen from @fire, for review separated by #56902 with @reduz.

[fire]

Commentary:

I was looking at godotengine/godot-proposals#3379 and it was hard to review, so I wanted to break the pr into pieces now that the retarget node is more complete.

[reduz]

Some notes here based on my review:

• I think local bone override does not make much sense, or at least I am not finding what it is useful for. I have seen that it was added for configuring a Local mode for BoneAttachment3D, which also I have no idea what it is for. I suggest this is removed and only keep global pose.
• For global pose, the blend amount ensures that the process order for IK and skeleton can be kept independent so I would not remove this.

[TokageItLab]

@reduz The previous implementation can only have one blend amount per bone, so I think it doesn't make sense to have it in the skeleton; once you have two blends in one bone, you don't know how much of each blend you have. So the blend amount should be controlled externally.

Remove bool president and float amount, then implement bool is_override_enabled instead (if amount is needed, use interpolate_with() before passing the pose to set_bone_pose_override(); president should be managed in modifier's priority)

[TokageItLab]

Memo - I and @lyuma's discussion log from Rocket Chat:

• pose_override should be local
• In set_bone_pose_override(), implement the amount to lerp as an argument (the amount will be used temporally for lerp, not be stored) omit argument
• pose_override doesn't have to be an array and doesn't store blend amount. Blending should be done externally in IK, PhysicalBone, etc. and only the final result should be stored in pose_override
• Replace set_bone_pose_override_enabled() with clear_bone_pose_override()
• Rename get_bone_pose_no_override() to get_bone_pose_base()
• Rename get_bone_pose() to get_bone_pose_overridden()

But renaming could be done with a different PR, I think it's better to get a consensus on the renaming-proposal first.

cc: @reduz

[TokageItLab]

I reconsidered the implementation of the argument amount in set_bone_pose_override(), I decided to remove it.

The reason is that it would cause confusion as to whether the amount should be blended with bone_pose_override or bone_pose_no_override.

SkeletonModification blended with bone_pose_no_override, but since that is implemented on the SkeletonModification side as a parameter called strength, it seems a bit different in nuance from amount.

Also, in PhysicsBody and BoneAttachment, those values are only used as 0 or 1. Now that I have implemented clear_bone_pose_override(), the parameter amount is no longer needed.

cc @lyuma @reduz

[TokageItLab]

Reimplemented p_amount into the set_bone_pose_override() and set_bone_global_pose_override().

Still @reduz claims that pose_override should be in global space, but I don't understand the reasoning behind this.

In my opinion, pose_override is there to avoid breaking the pose set by the animation when controlling bones from scripts. This could be the case where multiple scripts reference the pose set by the animation, or it could be the case where disabling pose_override only immediately restores the pose set by the animation.

Also, set_bone_global_pose_override() is still provided as an api. In it, using cached global_pose, the transformations passed as arguments are correctly converted from Global space to Local space and stored in pose_override.

Although reduz mentions an independent process, get_bone_global_pose() is eventually called in that api. This means that if the dirty flag is true, cache and skin updates will occur regardless of Skeleton's Process. I guess the update method is the same as in past implementations, and it should remain the same whether pose_override is in global or local space. (The skin update is a performance bottleneck that should be solved separately from this PR...)

[TokageItLab]

@reduz If you mean by "process independence" that you want "to avoid updating by the dirty flag", then I think the correct way to implement it is to have a dedicated option for that.

[TokageItLab]

Even if pose_override is in global space to avoid updating the dirty flag, if you want to put local coordinates in global_pose_override for a particular IK or something, get_bone_global_pose() will be called for the conversion, causing a bottleneck.

In other words, if pose_override has only one of the coordinate spaces, one of them will have a bottleneck due to updating the dirty flag.

The immediate solution is to add some kind of mode to pose_override to switch between local and global, or to implement an option to ignore the dirty flag as an argument, but we may need to look for a better way.

For now, I think the performance thing can be improved later; there are many features around skeleton and skeleton itself that need more refactoring to improve performance, not just pose_override.

The main purpose of this PR is to fix the major problem of not being able to final bone pose and to remove some redundant functions.

[TokageItLab]

Probably superseded by godotengine/godot-proposals#4863.