Make retarget keep global rest of unmapped bones if there are no mapped bones on the child #91560
Conversation
There was a problem hiding this comment.
I don't know why this was commented out, as it worked in every case I tested.
No. I remember I commented out them because the bones between the mapped bones have rest rotation on sharing animation in Godot 4 caused corrupting the animation sharing even if the those unmapped bones are not animating.
For example such case:
[LeftUpperArm] - [elbow_l(unmapped)] - [LeftLowerArm] - [wrist_l(unmapped)] - [LeftHand]
I think it's fine to keep their original rest if "there are no mapped bones in their children", so at least that check needs to be made.
I did tests for cases like this and didn't find issues with keeping the axis. In fact, resetting it like before or implementing your suggestion breaks the animation when there are unmapped bones between mapped too. I couldn't find any but isn't having issues due to irregular bone hierarchy when sharing animations the expected behavior? The user should fix the source or maybe they designed the rig like this on purpose. Handling it on the engine side seems to break everything else. |
|
In Godot 4 animation, the child mapped rests must match the relative rotation from the parent mapped rest on retargeting. You should test this in the case where the intermediate bone global rotation is different from the parent's global rotation (means that set rest other than Quat(0, 0, 0, 1)). To explain this in terms of Blender manipulation, you need to not only add a bone extended from its parent, but also add a bone in between that has been extended and has a modified roll or tail1. And test to create animations with them individually, then if they can share with each other. So if you only apply keep global rest when there is no child mapped bone, this change is acceptable, but otherwise we need a workaround with another module such as real time retarget to keep global rest. Footnotes
|
ydeltastar
force-pushed
the
retarget-axis-fix
branch
from
ffbc8f4
to
9d09f1c
Compare
|
Okay, now I understand what issue and I could reproduce it. I implemented your request and now this fix is working for both cases. |
ydeltastar
force-pushed
the
retarget-axis-fix
branch
from
9d09f1c
to
9cb0eed
Compare
TokageItLab
changed the title
There was a problem hiding this comment.
In terms of code, it's ok. This is haphazardly changing the rest rotation for some non-human bones, and I think the logic seems a little non-obvious. For example, it checks if a child is mapped, but what about grandchildren.
Will a user understand why a "elbow twist" bone has rotation changed while a "elbow sleeve" bone keeps the original rotation?
Furthermore, there may be users who have created BoneAttachments using the identity Basis and adjusted for it either in code or by manually positioning attached objects. Therefore, this risks compatibility break with any existing scenes / bone attachments. which were made assuming the previous design (which has stayed the same since Godot 4.0 was released)
With Godot 4.3 about to hit beta, this is quite late in the release cycle to be changing a fundamental part of the retargeting design.
At the very least, if we decide to go forward with this change, we need to include a compatibility setting on the retarget settings (Skeleton3D) which defaults to the old behavior for existing files and a different setting for new files.
There was a problem hiding this comment.
Ah yes, sorry I missed that it only looks at the children and not the grandchildren.
As long as we don't reimport them, there should be no compatibility issues, but I don't know how to handle compatibility as I don't have a strong opinion for that.
At least if you change it to check its grandchildren, I still can approve this, IMO. For performance, it would better to scan from parentress_bone to pre-create a whitelist with no mapped descendants and check if the rest rewriting bones are included in it.
AThousandShips
changed the title
ydeltastar
force-pushed
the
retarget-axis-fix
branch
3 times, most recently
from
1cfbae3
to
d6d9a4c
Compare
It was my first intention to make it an option when I went on this but I thought this was a bug. Since it is a wanted behavior for the importer, I added a |
|
For intermediate bones, |
|
Wouldn't this affect every bone attachment on retargeted skeletons in every existing godot project? I'm not convinced that this is rare. Also, about reimport, we can't assume that they don't happen. In fact, the whole .godot directory is excluded from git and users are told to delete that directory when there is a problem or if reporting a bug, so it is almost guaranteed that any issues which occur on reimport will eventually occur for a user. |
|
The values of the bone attachments may be changed, but I expect that in cases where the original rest is discarded due to override rest, the user will notice that something is strange, like the author of this PR, and therefore it is unusual to create a project that highly depending on that rest. |
ydeltastar
force-pushed
the
retarget-axis-fix
branch
2 times, most recently
from
9392808
to
fe12553
Compare
ydeltastar
force-pushed
the
retarget-axis-fix
branch
from
fe12553
to
32f8266
Compare
ydeltastar
force-pushed
the
retarget-axis-fix
branch
2 times, most recently
from
4d606a7
to
b400b44
Compare
| Variant PostImportPluginSkeletonRestFixer::get_internal_option_visibility(InternalImportCategory p_category, bool p_for_animation, const String &p_option, const HashMap<StringName, Variant> &p_options) const { | ||
| if (p_category == INTERNAL_IMPORT_CATEGORY_SKELETON_3D_NODE) { | ||
| if (p_option == "retarget/rest_fixer/keep_global_rest_on_leftovers") { | ||
| return bool(p_options["retarget/rest_fixer/overwrite_axis"]); | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
It looks a bit inconsistent with other similar parts in terms of the order of checks, but since the result is not different, it should not be a problem.
ydeltastar
force-pushed
the
retarget-axis-fix
branch
from
b400b44
to
ca58715
Compare
|
Rebased with master. |
|
Thanks! |
Fixes: #91558
I don't know why this was commented out, as it worked in every case I tested. Unmapped bones are unusable without it since we don't want retargeting to change their rests.