Skip to content
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

Add inherit_scale to bones #83903

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ElfWitch
Copy link

Adds inherit_scale property to bones in Skeleton3D. Setting this to false will allow the bone to be scaled independently of its parent.
This is useful for character customization that requires proportional editing. This is also useful for some animation techniques that were not possible before.

Addresses godotengine/godot-proposals#4190

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in godotengine/godot-proposals#4190 (comment), this should be Editor Only feature and enable only for BoneRest. "Pose Scale is propagated to the children" is never a problem, so this fix is not a proper solution.

@ElfWitch
Copy link
Author

replying to godotengine/godot-proposals#4190 (comment) and #83903 (review)

this should be an EDITOR ONLY feature.

The use of the Bone Scale in the Character Creator assumes a Rest transform. At this point, it makes sense to make it an option in the editor whether or not to propagate the parent's transforms. It would work like Blender's bone Edit Mode.

If this was editor only, it would make certain runtime character proportion editing and procedural animation impossible. By making it an option that can be applied per bone at runtime, we open the possibilities for non-authored procedural animations that weren't possible before. I'd also like to mention that even though blender does use rest transforms for its bone Edit Mode, even it has an option to control how transforms are propagated during animation per bone:
image-1
This PR introduces modes that would be equivalent to Blender's functionality as far as:
inherit_scale=true = Blender's Full
inherit_scale=false = Blender's None -which uses similar sheer correcting orthonormalization under the hood.
These are the modes used most often in blender animations, and blender is the animation authoring tool of choice for most Godot users, so it makes sense to support a one-to-one feature like this, regardless of potential future work with rest transforms.

"Pose Scale is propagated to the children" is never a problem.

This can be a problem if you are trying to make character customization that relies on proportional editing, which is the primary use case for needing to change bone scale inheritance. #83903 addresses two problems. Firstly, it adds a feature that allows users to choose which bones inherit their parent's full transform, or only inherit rotation/translation. In the current version of Godot, you can manually change a child bone's transform to account for the scale propagation, but the problem is that even with manual scale correction, there are major sheering and rotation artifacts which get worse the further the child scale is from 1.0. The second problem this PR addresses is the aforementioned sheering and rotation artifacts, when using inherit_scale=false.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 25, 2023

Read that comment godotengine/godot-proposals#4190 (comment) right. This is all I have to say.

In conclusion, the option that should be added to the bone is not "whether to propagate the scale" but "how to apply the pose animation". And the Lock feature should be implemented as a usability option when in Rest edit mode; It should be a kind of toggle option at the top of the viewport, rather than an addition to the bone.

Then relative animation/local transform (means same behavior with 3.x) should complement the case which you are described - character animation with scaled bone in runtime as long as you only allow the BoneRest to have a scale, not BonePose... but there is no use case there for BonePose i.e. to make the animation scale at runtime so that it does not propagate to the children. not accepted by Godot core if there is no need for it. If you are thinking of using Pose to change the shape of your character in the first place, it is completely wrong.


These are the modes used most often in blender animations, and blender is the animation authoring tool of choice for most Godot users, so it makes sense to support a one-to-one feature like this, regardless of potential future work with rest transforms.

Also, Blender animation system is a bit unique as well as the bone structure. They will be discarded when export it as glTF, so it makes no sense to make them compatible there.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 25, 2023

However, if we make the Editor allows independent bone manipulation, I think it would be possible to implement a usability function in Skeleton3D for independent bone manipulation. This should make it possible to create addons that do not propagate specific elements by overriding AnimationMixer::_post_process_key_value().

@AThousandShips AThousandShips added this to the 4.x milestone Oct 25, 2023
@ElfWitch
Copy link
Author

ElfWitch commented Oct 25, 2023

Then relative animation/local transform (means same behavior with 3.x) should complement the case which you are described - character animation with scaled bone in runtime as long as you only allow the BoneRest to have a scale, not BonePose...

Even in 3.x, the rest transform is propagated to children. This is a problem because as stated in my last comment, even if you correct scaling yourself, there are sheering artifacts which cannot be prevented from outside of the actual propagation function. One way to solve this would be to allow users to define their own transform propagation function similar to Array.sort_custom(), but that would be far too complicated for most users. The other way to solve this is by adding functionality for the most common use cases people would have for this feature, which is what this PR does. This very same optional transformation correction can be expanded to rest transforms when/if they get the old 3.x functionality back in 4.x.

but there is no use case there for BonePose i.e. to make the animation scale at runtime so that it does not propagate to the children. not accepted by Godot core if there is no need for it.

There absolutely are use cases for changing transform propagation at runtime. For games, procedural animation and character creation with proportional editing like ARK. Some games actually scale bones locally on the x/y axes when the mesh is under armor to reduce clipping. For robotics, inverse kinematics with telescopic/linear actuators. Keep in mind, it's an optional feature so people who don't need it aren't affected by it. It can also be useful to toggle this on and off as the given animation requires, especially for procedural animation.

If you are thinking of using Pose to change the shape of your character in the first place, it is completely wrong.

All components of a pose, namely the location, rotation, and scale, are intended to change the shape, as it represents a transformation. That's the point. If you mean explicitly the customizable proportions, that is absolutely something that belongs in pose, and would come down to the needs of the individual project and its animation team.

Also, Blender animation system is a bit unique as well as the bone structure. They will be discarded when export it as glTF, so it makes no sense to make them compatible there.

This isn't necessarily something that ever needs to be added to the importer, because it's probably not possible given the glTF spec. It's just there for the cases (usually for advanced IK armatures) that need analogous functionality. Users can toggle this feature as it is needed on a per-bone basis.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 26, 2023

Even in 3.x, the rest transform is propagated to children. This is a problem because as stated in my last comment, even if you correct scaling yourself, there are sheering artifacts which cannot be prevented from outside of the actual propagation function.

Even so, it would need to be an option like "Pose ignores Rest's scale", not "whether to propagate the scale".

However, regarding Rest deformations affecting it, I believe it is simplest to implement a function that "rebinds the skins and initializes the scales". This will lose the scale information, but if you need to use them again in the character creator, you can make them into a dictionary to store it.

There absolutely are use cases for changing transform propagation at runtime. For games, procedural animation and character creation with proportional editing like ARK. Some games actually scale bones locally on the x/y axes when the mesh is under armor to reduce clipping.

As I have said many times, body shape adjustments should be done in Rest.

For robotics, inverse kinematics with telescopic/linear actuators. Keep in mind, it's an optional feature so people who don't need it aren't affected by it. It can also be useful to toggle this on and off as the given animation requires, especially for procedural animation.

This is more of a bone structure issue. They should not have a parent-child relationship to begin with. If you want to make animation on Godot, Constraint/IK should be implemented or a feature such as BoneAttachment should be used. Additionally, it is not a valid theory because sharing animations of robotics is very rare.

As shown above, godotengine/godot-proposals#4190 issue can be solved by adding some editor options and functions. So there is no point that really needs to implement this property in your described use cases.


To begin with, Bone and Node3D have almost the same concept of parent-child relationship behavior for deformations, and implementing the "do not propagate parent scale" option only for Bone is not good from an architectural design point of view and would require adding it to Node3D as well. Moreover, such options can break animation sharing in retargeting, so basically, options for bones should be implemented in the SkeletonProfile. However, these would be too extensive and would not be allowed.

In summary, we believe that the correct architectural design to solve those problems is to divide them into the following two implementations.

1. implement an Independent mode in BoneEditor (or Node3DEditor if needed) that automatically adds cancel deformations to children of deformed bones

  • Implement set_bone_pose(rest)_independently() for making easy body shape adjustment on Godot and for runtime

2. implement functions like rebind_skin(bool p_init_scale) to rebind/init scale to skin

  • Remove scale from Rest to eliminates the effect of the bone scale (which is used in the Rest for body shape adjusting) statically for Pose

@ElfWitch
Copy link
Author

Even so, it would need to be an option like "Pose ignores Rest's scale", not "whether to propagate the scale".

Ignoring Rest's scale wouldn't be enough, because of sheering. Please note that this doesn't just prevent scale propagation, but performs other transformation correction during propagation. These corrections cannot be applied after the fact, and must be applied during propagation.

As I have said many times, body shape adjustments should be done in Rest.

I agree that pose should be relative to rest, but that is just not how things are right now. Having inherit_scale will be useful now, and it is forward compatible for if relative poses get added back in. Even if pose was relative to rest as it is in 3.x, the parent scale gets propagated to children bone transforms. This leads to sheering artifacts when bones are rotated because the transforms are simply multiplied. By using the optional transform correction introduced in this PR, those sheering and rotation artifacts are entirely mitigated. "Pose ignores Rest's scale" would simply not achieve the results intended because the point is having the ability to scale bones independently of their parent, while still keeping the correct rotation/translation. Mathematically, this is something that can only be fixed in propagation.

To begin with, Bone and Node3D have almost the same concept of parent-child relationship behavior for deformations, and implementing the "do not propagate parent scale" option only for Bone is not good from an architectural design point of view and would require adding it to Node3D as well.

That is irrelevant because while Bone and Node3D are very similar, they in no way affect each other or should have feature parity because of this similarity. I would also like to point out that Node3D already has similar functionality to this, however this PR only discludes scale inheritance, and does all necessary rotation and translation correction, as users expect. See https://docs.godotengine.org/en/stable/classes/class_node3d.html#class-node3d-property-top-level

Moreover, such options can break animation sharing in retargeting, so basically, options for bones should be implemented in the SkeletonProfile. However, these would be too extensive and would not be allowed.

However, regarding Rest deformations affecting it, I believe it is simplest to implement a function that "rebinds the skins and initializes the scales". This will lose the scale information, but if you need to use them again in the character creator, you can make them into a dictionary to store it.

These solutions are needlessly complex, and retargeting/animation sharing would not be affected in any way, as this is intended to be used after a skeleton has already been retargeted, during live animation.

I'd like to demonstrate what this PR does visually.
The following gif is from 3.5.2. The upper arm's rest pose scale is being set to (1.0, 4.0, 1.0), the lower arm's rest pose is being set to (1.0, 0.25, 1.0) to account for the scale which propagates to it:
3 x upper arm scale
As you can see, there are sheering artifacts, even with scale correction.

The next gif is from 4.x. As we know, pose is not relative to rest in Godot 4, but the same transformation as the first gif is being applied to the pose transform. The animation does not contain scale keyframes, so it is valid to change scale in pose.
4 x upper arm scale
The same artifacts as 3.x can be observed.

This final gif is in my fork of 4.x master with this PR applied. The upper arm has a pose scale of (1.0, 4.0, 1.0), and the lower arm has a scale of (1.0, 1.0, 1.0) and its inherit_scale is set to false.
4 x inherit_scale false
A rather extreme example, but you can see that the lower arm has no sheering or rotation artifacts, and its transformation is entirely predictable.

I say again: Adding optional scale propagation and sheering correction is the only way to achieve these results, even with pose relative to rest.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 26, 2023

I have understood what it is supposed to do. But as I said above, it is absolutely sufficient to statically remove scale by rebind. If we are concerned about processing at runtime, the best we can approve is to implement some of the functions (such as set_bone_rest(pose)_independently() and rebind_skin()) in Skeleton3D.

retargeting/animation sharing would not be affected in any way, as this is intended to be used after a skeleton has already been retargeted, during live animation.

The problem is that when you share an animation created on a model that uses that property with a model that does not have that property set, you have to rewrite the properties of the target model.

These solutions are needlessly complex

Not make sense, the main problem with your PR implementation is that you are confusing the animation with the adjustment of the body shape. When we split the implementation into two, we have more steps, but the workflow is simpler; no one thinks applying scale on Blender is complicated.

@ElfWitch
Copy link
Author

ElfWitch commented Oct 26, 2023

I have understood what it is supposed to do. But as I said above, it is absolutely sufficient to statically remove scale by rebind. If we are concerned about processing at runtime, the best we can approve is to implement some of the functions (such as set_bone_rest(pose)_independently() and rebind_skin()) in Skeleton3D.

Simply rebinding the skin won't affect BoneAttachment3D, IK, or anything else that relies on the direct bone transform. This isn't intended for preprocessing animations or skeletons like retargeting. This is intended as an additive effect after the skeleton is already imported, retargeted, etc.

The problem is that when you share an animation created on a model that uses that property with a model that does not have that property set, you have to rewrite the properties of the target model.

This isn't true. This isn't a property on models. It is a property on Bone in Skeleton3D. It is skeleton/model agnostic and non-destructive. You only disable inherit_scale when you want to take manual control over scale editing. Typically this should be done via script anyway, like in character creators for example.

Not make sense, the main problem with your PR implementation is that you are confusing the animation with the adjustment of the body shape.

One of the most important parts of this PR is that it does translation correction when a parent bone is scaled. Simply not inheriting parent scale is not enough for the intended effect. When you scale a parent bone, you expect the child bone to still inherit its position as if it was scaled from its parent, then it adjusts scale as needed.

When we split the implementation into two, we have more steps, but the workflow is simpler; no one thinks applying scale on Blender is complicated.

I don't understand what you mean by "no one thinks applying scale on Blender is complicated". Some games have character creators that allow proportional editing, which by its very nature cannot be pre-authored, or applied in Blender. Such games are not currently possible in Godot without major animation artifacting. Yes, rebinding the skin can change the scale of bones, but not the correct proportional offsets if you change the length of a bone. It has to be done on the skeleton, and as demonstrated earlier, even pose relative to rest will result in artifacts when scaled non-uniformly.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 26, 2023

Simply rebinding the skin won't affect BoneAttachment3D, IK, or anything else that relies on the direct bone transform. This isn't intended for preprocessing animations or skeletons like retargeting. This is intended as an additive effect after the skeleton is already imported, retargeted, etc.

I already answered this above. If the current scaling method has problems with IK, it is a bone structure issue. It should not have a parent-child relationship. Besides, it would be more ideal if all the bone scales were removed for IK and PhysicalBone in the first place.

This isn't true. This isn't a property on models. It is a property on Bone in Skeleton3D. It is skeleton/model agnostic and non-destructive. You only disable inherit_scale when you want to take manual control over scale editing. Typically this should be done via script anyway, like in character creators for example.

No, bones are tied to the model. For example, SkeletonProfile may change the Rest of the bone by importing. And, I am pointing out that if you have two models, there is no way to synchronize their properties since a model may have a large number of bones, the bone options involved in such deformations should be presettable in a SkeletonProfile or copied by the utility function. Don't distract from the point.

And the rest, I can only say is that you are confusing animation with body shape adjustment.

You may be overzealous with your first Pull Request, but you need to get calm down. Your implementation confuses multiple issues, and so problematic from an architectural design, that we cannot approve it.

@ElfWitch
Copy link
Author

it would be more ideal if all the bone scales were removed for IK and PhysicalBone in the first place.

Perhaps for physical bones because scaling collision shapes/rigid bodies is problematic, but sometimes there are uses for scaling in BoneAttachment3D, and IK. Examples: parenting armor to a BoneAttachment3D when the character's proportions are edited, procedural/IK squash and stretch rigs, rendering linear actuators/pistons for robotics development.

No, bones are tied to the model. For example, SkeletonProfile may change the Rest of the bone by importing. And, I am pointing out that if you have two models, there is no way to synchronize their properties since a model may have a large number of bones, the bone options involved in such deformations should be presettable in a SkeletonProfile or copied by the utility function.

This change does not affect import, glTF models, or SkeletonProfile. It is for use after skeletons are already imported in the engine. Its primary use is procedural animation, and character scaling. I believe the problem you are describing is if someone imports two separate models that have different skeleton proportions and wants to unify animations between them using SkeletonProfile. This PR is still compatible with that, as it only adds optional transform correction during bone transform propagation. inherit_scale should not exist from the perspective of SkeletonProfile, because it is explicitly for runtime bone scaling.

Your implementation confuses multiple issues, and so problematic from an architectural design

My implementation is almost a direct correlation to how Blender and Maya solve it. It doesn't complicate any existing transform logic. It is intended to expand on what is already possible with scale animation in the least intrusive way possible.

@TokageItLab
Copy link
Member

Your repeating things I have negated many times above does not make them correct.

My implementation is almost a direct correlation to how Blender and Maya solve it.

Blender's bone structure is quite special, but Maya also has a special bone system. Maya's storing and controlling local axes way is special. Furthermore, the glTF skins output from Maya are bound in a special way. I remember I had to implement some complex calculations for a glTF output from Maya when I worked on the importer.

Godot is Blender friendly, but it is not Blender, and the existence of a feature in Blender is not a reason to implement that feature in Godot. The only case in which Godot will follow the addition of an external feature is when glTF is extended.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 3, 2024

Memo:

If the purpose of the bone transformation is to adjust the body shape, then the method I commented on above should work just fine.

However, in some cases where the purpose of the animation is not to adjust the body shape, it may be useful to prevent certain properties from being inherited.

In such cases, it is common in other game engines to use Bone Constraint to dynamically constrain certain properties. It seems completely make sense since it does not change the properties of existing Skeleton in Godot.

Bone Constraint is not yet implemented in Godot, but we do have plans to work on it at the same time we implement IK, etc.

In conclusion, the correct way is to implement a method like Bone Constraint that controls the bone from the outside, not having the bone itself have the properties of the constraint as in this PR.

Add inherit_scale to bones

Adds inherit_scale property to bones in Skeleton3D. Setting this to false
will allow the bone to be scaled independently of its parent.
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