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

Skeleton add the option to Lock Bone Scaling #4190

Open
mrjustaguy opened this issue Mar 9, 2022 · 9 comments
Open

Skeleton add the option to Lock Bone Scaling #4190

mrjustaguy opened this issue Mar 9, 2022 · 9 comments

Comments

@mrjustaguy
Copy link

Describe the project you are working on

Character Creator

Describe the problem or limitation you are having in your project

Scaling a given bone will also apply it's scaling to it's Direct children bones (indirectly also the bones of those child bones) and the Direct children Bones have to get the exact opposite scaling to undo this (for example scale a parent bone by 2, the children of the parent bone have to be scaled by 0.5 for them to keep their scale)

This behavior is counter-intuitive, and while it makes sense when you think about the inner workings, it doesn't provide the most common use case in it's area which most users that do this will use and expect.

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

Allowing Scale to not be inherited from parent bones would allow for easy individual bone editing often used in Character Creators, both in apps and games (most often RPGs)

Locking Translation/Rotation like this afaik doesn't make any sense for anything.

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

Add a bool to bones to ignore parent bone scaling, and use by default, as this is generally the desired outcome (I haven't seen many use the current behavior where parent bones also scale it's child bones, albeit there are some applications of this too so keeping the option available makes sense)

This part is self explanatory, if a Bone is ignoring it's parent bone scaling, it doesn't scale itself (and it's children too as a result) when the parent bone changes it's scaling. This is just a tiny bit of simple math.

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

It can be.

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

This is an intuition issue in Core, and while it can be fixed with an addon, one shouldn't have to use addons to fix the editor's lack of intuitiveness.

@TokageItLab
Copy link
Member

TokageItLab commented Mar 9, 2022

That behavior certainly makes sense for character creation, but not for animation.

Also, when doing character creation, it is common to apply the final scale to the rest, not the pose. Think of Blender's EditBone edit; At least, I think that rests should be transformed independently of the parent-child relationship.

In my opinion, the proper way to go is to implement a RestEdit mode that allows such rests to be edited independently, in addition to the PoseEdit mode that currently already exists in SkeletonEditor.

However, if you want to use it externally, you need to use tools.

So, obviously, that is information that should be held on the Editor side, and I think it can't get consensuses to have it in the Skeleton::Bone. Wouldn't a function like set_bone_pose_independently() or set_bone_rest_independently() be sufficient?

@mrjustaguy
Copy link
Author

mrjustaguy commented Mar 9, 2022

Probably, however I don't see any use of setting translation/rotation of bones independently.. but yeah, Don't see why it wouldn't be sufficient.

It would also indeed be very helpful for editor UX in general to have the ability to specifically edit rest, regardless of the individual bone setting asked for in this proposal, and then have it set bones independently, while keeping the pose editor the same.

Also, while looking around the Skeleton3D Documentation, why does Transform3D have methods such as scaled, rotated translated with the d at the end? It makes it seem like the methods are bools and not math functions.. this also exists with Basis and Transform2D, while the same functions don't have the d at the end for nodes. For godotengine/godot#54161 maybe?

@ElfWitch
Copy link

Is this something that should be implemented as a single bool on a Skeleton3D that affects all bones, or should it be per bone?

@mrjustaguy
Copy link
Author

Per bone is more flexible, but typically you want it on the whole skeleton so IDK.

@ElfWitch
Copy link

I'll be finishing this up soon. Do we think per-bone inherit_scale should be on or off by default? If bones don't inherit their parent's scale, it might be more intuitive, but people's animations that already have bone scale keyframes might not work as expected when using this change. Having inherit_scale on by default would match the current behaviour, but would be slightly counter-intuitive as mentioned in the original issue.

I'd also like to add that it's not possible to do this from gdscript, because there's sheering artifacts when bones rotate with non-uniform scale, even if you correct the scale. The only way to solve this is by changing Skeleton3D::force_update_bone_children_transforms().

@mrjustaguy
Copy link
Author

It's a really good question..

One part of me says default to on as it's more intuitive, but the other doesn't want to screw over existing animations..

I think keeping it off is probably the way to go, it's not like scaling is a super common thing in animation and just having the option between the two behaviors should help significantly for those using it, so doesn't really matter a ton if the default option is the less intuitive one, and I'd say compatibility takes priority here..

@ElfWitch
Copy link

ElfWitch commented Oct 24, 2023

I agree about compatibility. I'm pretty sure it's done. This is my first time programming c++ and my first PR ever so hopefully I did everything right. 🤞

@TokageItLab
Copy link
Member

TokageItLab commented Oct 24, 2023

There was a previous proposal almost identical to this one. As I added there, this should be an EDITOR ONLY feature. Since this proposal was recently dug up, I had forgotten that this was a proposal I had already seen. So I say again but as I commented above (#4190 (comment)), 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.

The most problem at this time is that Pose in Godot 4.x is no longer relative to Rest. And, "Pose Scale is propagated to the children" is never a problem.

I've argued this issue forcefully in godotengine/godot#56902 and published a module in https://github.com/TokageItLab/realtime_retarget, but maybe we should re-arrange to bring that implement it into core once again now that the demand has been visualized.

Performance is lower than godotengine/godot#56902, but if the bones have the option of applying animation (Absolute, Relative, Global) method as Enum, the implementation is simpler than godotengine/godot#56902 and it will be a similar interface to the PR in godotengine/godot#83903. Although the original BoneRest transforms which is used to make the animation must be stored in the AnimationLibrary or Animation to extract Relative/Global transform.

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.

@fire
Copy link
Member

fire commented Oct 25, 2023

Inverse Kinematics is a method of animating objects using bones chained into a skeleton. Instead of manually setting the rotation of individual bones in the skeleton, IK solves the rotations for us based on the end target position.

I thought that to lock a bone's position or rotation, you would typically use an Inverse Kinematics (IK) constraint system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants