godot
godot copied to clipboard
godotengine
Make retarget keep global rest of unmapped bones if there are no mapped bones on the child
Fixes: https://github.com/godotengine/godot/issues/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.
For example such case: [LeftUpperArm] - [elbow_l(unmapped)] - [LeftLowerArm] - [wrist_l(unmapped)] - [LeftHand
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 tail[^1]. And test to create animations with them individually, then if they can share with each other.
[^1]: While this case is probably not a frequent occurrence in blender operations, it may well occur in other software
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.
Okay, now I understand what issue and I could reproduce it. I implemented your request and now this fix is working for both cases.
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.
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 Keep Unmapped Axis default to false (a strange default though) and the check for mapped descendants.
For intermediate bones, Keep Unmapped Axis is strange because it is discarded even if it is axis ummapped, so it would be a very special property like compatibility mode. But I personally do not think this option is necessary. I think it's a pretty rare case where this would cause a problem, also that problem does not occur unless a re-import occurs.
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.
