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

Retargeting option to use a template for silhouette. #88824

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Feb 25, 2024

Option to create and use SkeletonProfile as silhouette template.

Adds a few Skeleton3D options:

  • For a reference model, adds export_skeleton_profile_to_file/enabled and /path to export the Skeleton3D node as a SkeletonProfile before retargeting.
  • For retargeting, retarget/rest_fixer/silhouette_template option to replace bone rest with an exported SkeletonProfile before retargeting.

With the latest version of the PR, I now use animations to transfer the pose from one model to another.
I tested the workflow. Here are some ways the new Rest Pose feature can be activated:
image

And for an example of it in action, here is of jog animation from issue #89244, showing the problem:

model in a wacky pose with limbs flailing

From this model, I now open up the good t-pose version and export the t-pose animation to a file:
image
This is best done using the "Export Skeleton Rest Pose" field on the skeleton, which works in case the model does not contain an explicit t-pose animation and avoids several pitfalls when referencing an AnimationLibrary or other extracted animation.

After reimporting the t-pose model, I can now use the extracted rest-pose animation in another model:
image

And tada! We now have the correct jogging animation, in Godot's humanoid format:
image

I will re-post this in the PR as well to show the workflow.

Fixes #65656
Fixes #89244

@fire
Copy link
Member

fire commented Feb 25, 2024

Thoughts on a boolean/enumeration that hides the bonemap. Like a switch between bone map or retarget/rest_fixer/silhouette_template where we control visibility.

@fire
Copy link
Member

fire commented Feb 25, 2024

We discussed a read-only multiline string property for displaying correction instructions.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 25, 2024

Wasn't the real problem with this that the other animation might have T or A pose instead of the animation of top of the list?
I don't think this workflow requiring a separate model is a good idea. So the right way is to add a GUI to select the reference animation such as adding processing steps in the importer IMO.

@lyuma
Copy link
Contributor Author

lyuma commented Feb 25, 2024

Original version of PR description, with screenshots:


Example model showing this workflow before and after:
Here is a Maya FBX animation with a simple BoneMap applied but with silhouette fixer disabled.
retargeted skeleton not in t-pose
I can use silhouette fixer, but it was not designed to recenter the hips position (I may attempt to fix this, since the lack of recentering is a bug IMHO), but even if the center were fixed, you can see here in the profile view that silhouette fixer just isn't able to correct the entire skeleton when the pose is too off-center (I think the hips is not being axis-aligned correctly: this might be another bug that I will solve separately):
result of silhouette fixer
Anyway, the fact is that the silhouette fixer is guesswork and will not be perfect in 100% of cases. That's where this PR comes in. I can pass in a profile exported from a model in t-pose, and voila! The model is now imported correctly:
correct t-pose

Together this allows a purely importer based workflow to transfer a known good pose from one FBX to another.

Screenshots of preferred workflow.
First, how to export a known good reference pose. This can be T-Pose or anything reliably fixable by Silhouette Fixer (such as A-pose)
image

This exported profile goes into a new slot for "Silhouette Template" in the Rest Fixer
using_silhouette_template
The importer performs validation of the silhouette to ensure it comes from a compatible rig:
warning messages in the importer
This allows manually fine-tuning the silhouette pose which can be used to mitigate cases where Silhouette fixer cannot fix the initial pose.
Fixes #65656

(Note that the preview is invisible because #78188 was not yet merged)

Here is an archive of screenshots from a previous version of this PR:

Old version of the warning using_silhouette_template_warning
Older / Manual workflow for creating a SkeletonProfile BoneMap must not be used for the reference model. This is likely going to be the most confusing part of the process. import_silhouette_template The "Export Skeleton Profile" feature in the Skeleton3D menu already existed, but was previously only used for constructing new types of rigs. If you adjust the rest pose, make sure to use "Apply all Poses to Rest" also in the Skeleton3D menu before exporting the profile. export_skeleton_profile

There is no list of animations on Maya exports. It is very common for an animation export from Maya to be a single animation named "Take 001", which is why animation authors in Unity typically include a separate fbx file in the reference pose.

I do not know if there are different workflows common in other engines such as Unreal, but the suggestion to add additional animations in each file for t-pose or a-pose is impractical at least for FBX and Maya exports.

Can you explain why it is not a good idea?

@AThousandShips AThousandShips added this to the 4.x milestone Feb 25, 2024
@TokageItLab
Copy link
Member

TokageItLab commented Feb 25, 2024

There are several problems:

Importing another model as an actual object increases the size of the project folder by the amount of the imported model.

It is difficult to detect the possibility that a profile has incorrect rests. This may have to be left to the user's file management practices, but at least it is a hassle to output a profile from the model each time and save it as a separate resource.

If managed as another model, it would be better to use a name suffix or special extension so that the collider uses _col and the external fbx can be specified directly from the importer as a file to be used only at import time.

@fire fire requested a review from a team February 25, 2024 18:16
@lyuma
Copy link
Contributor Author

lyuma commented Feb 25, 2024

Importing another model as an actual object increases the size of the project folder by the amount of the imported model.
Usually these models don't need the mesh, so they are just a skeleton posed in t-pose. So it may take on the order of one extra megabyte. For character models with separate animations, usually the character itself is the reference pose, so there is often no need to add additional files.

This is a critique of the workflow, but not a reason why it is wrong to do this, just that it is not as efficient. Also, there is no requirement to retain the original t-posed model in the project after extracting a skeleton profile. This is one of the reasons I decided to take this approach, since it creates an asset that can be reused for subsequent imports of a particular character.

It is difficult to detect the possibility that a profile has incorrect rests. This may have to be left to the user's file management practices, but at least it is a hassle to output a profile from the model each time and save it as a separate resource.

I warn if a SkeletonProfile is used which does not match up with the chosen BoneMap. If two models have the same sets of bones, my assumption is that they likely have the same rests, but of course it is no guarantee. This feature is designed to work when two files refer to precisely the same skeleton, but the burden is on the user to make sure not to mix up files. This is indeed a workflow challenge but not a reason why this cannot be used as a workflow.

If managed as another model, it would be better to use a name suffix or special extension so that the collider uses _col

I strongly dislike the collider suffix convention, and I would be unhappy creating a new workflow which depends on name suffixes, as other parts of the system may need to become aware of such rules..

and the external fbx can be specified directly from the importer as a file to be used only at import time.

This certainly could be done as an alternative to specifying a SkeletonProfile. It adds complexity because the external model would have to be loaded which creates an import dependency. If you insist that we implement the feature this way, I can try to work on it.

I have some concerns about referencing another scene file from the import. Godot might not have a guarantee that models are imported in a particular order, or it could create a cyclic import if the user by mistake sets two gltf files as each other's silhouette.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 25, 2024

Usually these models don't need the mesh, so they are just a skeleton posed in t-pose. So it may take on the order of one extra megabyte. For character models with separate animations, usually the character itself is the reference pose, so there is often no need to add additional files.

This is a critique of the workflow, but not a reason why it is wrong to do this, just that it is not as efficient. Also, there is no requirement to retain the original t-posed model in the project after extracting a skeleton profile. This is one of the reasons I decided to take this approach, since it creates an asset that can be reused for subsequent imports of a particular character.

The most problem with this is that after fbx changes the rest, this steps is required again. Also users will not know if the workflow needs to be re-done until they execute to re-import the model.

I strongly dislike the collider suffix convention, and I would be unhappy creating a new workflow which depends on name suffixes, as other parts of the system may need to become aware of such rules..

This certainly could be done as an alternative to specifying a SkeletonProfile. It adds complexity because the external model would have to be loaded which creates an import dependency. If you insist that we implement the feature this way, I can try to work on it.

Well, yes, suffixes and extensions are not required. So for now, the most optimal compromise would be to allow external fbxes to be specified IMO. As long as the external fbx exporter outputs the actual and reference (T/A-pose) models at the same time, the consistency is guaranteed.

@lyuma
Copy link
Contributor Author

lyuma commented Feb 25, 2024

after fbx changes the rest, this steps is required again

It should not be required, because the model uses the same skeleton either way (For example, if I have Alicia in t-pose and Alicia doing a walk animation, the skeleton is the same, so it does not matter which version of Alicia's bone rest I use.

Overwriting the bone rest is merely adjusting the model into a known pose, Also, this known pose can be combined with silhouette fixer to account for any offset between the reference t-pose and the desired t-pose.

allow external fbxes to be specified IMO.

I'll have to figure out how to do this, but I assume it would be a scene file. We just need to make sure that the imports happen in the correct order if I lose my .godot directory. Perhaps it would be sufficient to call reimport_append if the dependency scene cannot be loaded.

We'd still have the problem that the target scene is not allowed to use BoneMap (since we lose the original bone rolls if the user has set a BoneMap, but it would be easy to detect this because the node names will not match (GeneralSkeleton vs Skeleton3D) as well as the bone names.

Anyway, I will work on this version of the change. From the user's perspective it should not be much different.

Technically I fear we are opening a can of worms that has not been tested. Do I use a PackedScene resource picker? a little scared to serialize a .glb or .fbx reference from a .import file
... or a PROPERTY_HINT_FILE file picker with *.fbx,*.glb,*.gltf,*.scn,*.tscn,*.dae ... but it can't target a .vrm if I do that: the user would have to inherit scene and reference the inherited .scn

@Illauriel
Copy link
Contributor

Importing another model as an actual object increases the size of the project folder by the amount of the imported model.

But if you export the Skeleton Profile, and remove the model afterwards, it shouldn't be that big of an impact, right? And this approach would fit with a kind of workflow where source model files themselves are not version-controlled and are split into their respective resources. So the FBX (or GLTF or whatever) from which the correct pose was borrowed only bloats the project folder of the team member responsible for setting the models up.

Overall, I see merit in both approaches, and if it would be possible to reference either a scene file or a Skeleton Profile in the Silhouette Template field, that might be the best way to do it. Exporting a Skeleton Profile would be an extra step you have to do for each model, but it looks like a 'cleaner' workflow. Correct me if I'm wrong.^^'

If managed as another model, it would be better to use a name suffix or special extension so that the collider uses _col and the external fbx can be specified directly from the importer as a file to be used only at import time.

It might be an option, but I don't use suffixes in my workflow so I don't think it would necessarily be 'better' to do it this way.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 25, 2024

It is true that SkeletonProfile is not referenced with the source model. It is focused to work in cases where changes to the source model are not important.

For example, SkeletonProfile is designed to be independently sharable for sharing among workers to allow retargeting of monster models with specific Rest, etc.

However, this is not the case in this PR case. If only one silhouette of one specific model is to be targeted, the Profile must have an reference with the source model. This is the same as the AnimationLibrary continuing to have an reference with the source file. In other words, if the referenced Profile is a tres, it must be generated with the .import through ImportedAs in Godot. Also, many fields are unnecessary for SkeletonProfile for this purpose.

I think it would be suitable to have something like SilhouetteData, a list consisting purely of pose, rest, name and parent (or a dictionary with nest instead of parent) that can be generated from fbx/gltf with ImportedAs, but I am not sure if it is worth adding that as a new class.

Well, in actuality, pose and name may be all that is needed. Does it work to import as AnimationLibrary and select Animation from it on importer? It is sufficient if a single animation can be generated in the following case as you said:

There is no list of animations on Maya exports. It is very common for an animation export from Maya to be a single animation named "Take 001", which is why animation authors in Unity typically include a separate fbx file in the reference pose.

@Illauriel
Copy link
Contributor

This is the same as the AnimationLibrary continuing to have an reference with the source file.

I think I get your point, but we have also independent Animation Libraries that are not "models imported as AnimationLibrary", but "A grouping of externally stored animations" so this analogy is not entirely correct.

In other words, if the referenced Profile is a tres, it must be generated with the .import through ImportedAs in Godot.

But wat if I don't want to import the file as this kind of resource, because it also holds a ton of other useful data, like meshes, materials, etc. Say we have one big blend file with character, animations and all the stuff. It's like 150Mb. I never ever plan to edit it under any circumstances. (because I'm not an artist) I want to export all of this useful data as separate resources and discard the source .blend file for good. Should I instead create multiple copies of this .blend file to import one as an AnimationLibrary, another as SkeletonProfileForBetterSilhouetteFix and save meshes and materials from the third one?

I think it would be suitable to have something like SilhouetteData

If it's a standalone file and it can safely lose reference to the imported model file that it got created from, it might be okay, but also as you've said, making a separate class just for this specific purpose seems wasteful.

Here's how I see my specific workflow with this in my imagination:

  • I bought a pack of animations from some store and want to retarget them to my character that has vastly different proportions to Unreal Mannequin or whatever the animation creators used as their base model.
  • I drop the folder with animation FBX files in the project hierarchy, open the file that says T-pose, set BoneMapHumanoid, reimport open it as a scene and export a SkeletonProfile or whatever it will be called from it.
  • I open each of the other animation files' advanced import dialogues, set BoneMapHumanoid, set the exported SkeletonProfile, set a destination to export the animations, reimport.
  • Then I delete the FBX source files and forget about their existence. I don't include them in the version control, I don't bloat my project folder, and feel happy about it.

TL;DR: all of the "ImportAs" and resource inheritance discussed previously is very good when you have a project where assets are in constant state of iteration and need updating, but it's not the only use case.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 26, 2024

I think I get your point, but we have also independent Animation Libraries that are not "models imported as AnimationLibrary", but "A grouping of externally stored animations" so this analogy is not entirely correct.

Conversely, we can say that if the SkeletonProfile can also have a reference to the original model, it should do so.


Finally, it would be best if the split fbx could be imported as an AnimationLibrary and specified as my comment above.

Well, in actuality, pose and name may be all that is needed. Does it work to import as AnimationLibrary and select Animation from it on importer? It is sufficient if a single animation can be generated in the following case as you said:

There is no list of animations on Maya exports. It is very common for an animation export from Maya to be a single animation named "Take 001", which is why animation authors in Unity typically include a separate fbx file in the reference pose.

If that is possible, then we can share that method to solve two problem cases:

  1. If the reference pose is in a separate file, we can load it As AnimationLibrary, then specify an external animation library and the reference animation from it
  2. If the reference pose is in the same file, but not at the beginning of the animation list, we can specify reference animation from model's own AnimationLibrary (Although it may needs to have some internal exception handling such as temporary duplicating original animation to account for the effects of retargeting)

@lyuma lyuma force-pushed the retarget_silhouette_template branch 4 times, most recently from 442bbce to 2ef6b13 Compare February 26, 2024 13:51
@lyuma lyuma requested a review from a team as a code owner February 26, 2024 13:51
@lyuma lyuma force-pushed the retarget_silhouette_template branch from 2ef6b13 to 6c66917 Compare February 26, 2024 13:55
@lyuma
Copy link
Contributor Author

lyuma commented Feb 26, 2024

Revision 442bbce adds a better warning to the SkeletonProfile template.
warning text

I have spent several hours prototyping a workflow for animations or AnimationLibrary or PackedScene references, but they all seemed so clunky, and animations in particular are highly error prone either due to mixing up post-retargeting animations from pre-retargeting animations, as well as the default setting of "Remove Immutable Tracks" being on (BoneMap forces immutable tracks to be written, but we can't use a BoneMap'ed animation)

If only one silhouette of one specific model is to be targeted, the Profile must have an reference with the source model. This is the same as the AnimationLibrary continuing to have an reference with the source file. In other words, if the referenced Profile is a tres, it must be generated with the .import through ImportedAs in Godot. Also, many fields are unnecessary for SkeletonProfile for this purpose.

Anyway, I was chatting with @Illauriel and thinking about your comment, and it occurred to me: I can make the SkeletonProfile generated by the .import.
image
This also adds a new workflow to easily create new workflow-specific profiles without having to open the scene and manually select "Export Skeleton Profile"

Further, since it is generated before the BoneMap is applied, it no longer requires an additional file for "retargeted" and "not retargeted" versions: you can enable the new Export Skeleton Profile to file and still perform the retarget, and the exported profile will be pre-retarget and hence suitable for use as a "Silhouette Template":
image

@lyuma
Copy link
Contributor Author

lyuma commented Mar 8, 2024

Updated the PR to accept animation libraries, either externally or animations from the same file's AnimationPlayer.

There are now a few different cases here, and I have some warnings to detect the common pitfalls, (these pitfalls are why I originally shied away from using animations). For example, using a BoneMap converted animation; or using an animation with (almost) no tracks -- both of these are easy to create by mistake due the remove immutable tracks.
image

@lyuma lyuma force-pushed the retarget_silhouette_template branch from 4719940 to a7b748d Compare March 8, 2024 10:55
@lyuma lyuma force-pushed the retarget_silhouette_template branch from a7b748d to bea0306 Compare March 9, 2024 22:29
@Invertex
Copy link
Contributor

Invertex commented Mar 10, 2024

Oh, glad to see this PR haha. I actually implemented this a couple months ago:
https://github.com/Invertex/godot/tree/fix-silhouette-using-other-source

But I never submitted a PR because I assumed it probably wouldn't get approved... Was waiting to see if I could think of some other solutions. This has been a pretty big issue for me as it has severely restricted what third party animations we can use as a smaller studio. Many of the animation packs people may buy will come in this "bad workflow" format.

I took a similar approach as here, but instead of storing the Resource as an externally referenced file, I embed it in the .import of the model it belongs to. My thought here is that it ensures there's no mistake of ownership or possibility of another asset overwriting the Resource.

Other models can choose the target source model and read the BindPose resource from its .import if it exists.

I also update the pose at the very start before any of the regular importing fixes happen, so that the file is basically utilized by the import system as though it actually contained the proper rest pose. Don't need to mess with any of the actual import code.
BoneMap can do its thing, working with a proper matching Bindpose then.

In this sense I disagree with referencing animations (at least for external targeting, it's a good option for models that have a Bindpose animation in them but no stored Bindpose, though that's a lot more rare).
Animations represent a post-import pose, instead of the true bind pose of the actual asset. Having this system reference the target asset and use its actual Bindpose and then just let the import system do its work like normal lowers the tech-debt in the codebase. Assets can already share the BoneMap settings resource; so if you change BoneMap values on the source model, the other models can reimport with those same settings, giving you a consistent, proper result without modifying the animation import pipeline.


A possibility for triggering dependency could be having a list in the source file of other assets that rely on this profile. When this source model reimports, it could trigger those list of files for reimport after the source file is done (and any paths that don't exist anymore can be culled and warned about).

@lyuma lyuma force-pushed the retarget_silhouette_template branch from bea0306 to 000f7e3 Compare March 10, 2024 01:54
@lyuma
Copy link
Contributor Author

lyuma commented Mar 10, 2024

Hi, @Invertex thanks for your interest and feedback!

Here are some comments I am making on the points you raise.

use its actual Bindpose

I believe the FBX importer has code to locate and use the mesh bindposes if the model has meshes (Nuance: bindpose a mesh property, not a skeleton3D so we shouldn't call it that. In particular, many animations do not contain any meshes and hence a bindpose can not exist in this case). It might be interesting to make this an option, at least in FBX or glTF formats but that should be a separate system and not what I am solving here.

(An aside: I have seen models whose bindpose is in A-Pose, or even contain multiple meshes with a mixture of A-Pose and T-Pose bind poses, due to joining outfits or objects made by another artist).

instead of storing the Resource as an externally referenced file, I embed it in the .import of the model it belongs to. My thought here is that it ensures there's no mistake of ownership or possibility of another asset overwriting the Resource.
// Store Skeleton's bind-pose in this file's .import data, and potentially load a Bindpose to use from another file.

May I ask how you implemented "embed it in the .import" at a technical level? I couldn't figure out which lines of code force a loaded resource to be embedded in the .import file (the user can do this manually using "Make Unique"). I was considering forcing it by setting "Resource Local To Scene", but BoneMap (as well as its referenced SkeletonProfile) are actually subject to the same dangers and I want to figure out a solution that works for all of them.

This problem already existed for a while: From what I recall, deleting a BoneMap resource referenced in a .import file may cause Godot to segfault. Furthermore, .import files do not store file UIDs, so renaming a resource breaks all .import files referencing it. I fully agree this is a problem but we should solve it in a separate PR.

Here is an example from the test project in the issue:

"nodes": {
"PATH:Skeleton3D": {
"rest_pose/external_animation_library": Resource("res://TPose-MotusMan.anim"),
"rest_pose/load_pose": 2,
"rest_pose/selected_animation": "MOB1_Walk_F_IPC",
"retarget/bone_map": Resource("res://BoneMap_MotusMan.tres"),
"retarget/rest_fixer/fix_silhouette/enable": true
}
}

Notice how both bone_map and the new external_animation_library use Resource(...) references.

I also update the pose at the very start before any of the regular importing fixes happen, so that the file is basically utilized by the import system as though it actually contained the proper rest pose.

My PR also does this early on, so it should work the same as yours. I ended up doing it directly in resource_importer_scene.cpp, since I wanted this feature to be separate from rest fixer (You may wish to change the bone rest without using a bone map)

(at least for external targeting, it's a good option for models that have a Bindpose animation in them but no stored Bindpose, though that's a lot more rare).
One of the maintainers brought up an example of this. I personally find it to be more rare, but I can't deny that some files exist which work this way, hence the "Internal Animation" option. I think in particular, teams which use Blender NLA animations in a glTF-native workflow are likely to encounter this, so I think it is worth solving.

In this sense I disagree with referencing animations ... Animations represent a post-import pose, instead of the true bind pose of the actual asset.

I agree with you. In fact, the original version of this PR used a SkeletonProfile resource as a "serialized skeleton". I felt SkeletonProfile was a good fit, since to me, a reference pose is the same type of thing as a profile.

I raised my concerns at the last official meeting to other people on the animation team: Arguments against existing types such as SkeletonProfile or PackedScene were that they had too many unused fields in this application. There were maintainability arguments against introducing yet another bespoke resource type for this usecase. Most in the meeting agreed that AnimationLibrary was sufficient to solve this problem. I raised concerns about usability that Animation import has caveats such as "Remove Immutable Tracks", or being saved after BoneMap.

By the end of the meeting, I think I was satisfied with the compromise to use Animation rather than introducing a new bespoke type. We agreed that with a good UI it would work. Directly or indirectly, @fire gave me the idea of modifying SceneImportSettingsData::_get_property_list to implement an animation picker dropdown and some validation logic.

To this end, I tried my best to carefully build the workflow and add some warnings for those usability traps (The import dialog shows a warning either when "Remove Immutable Tracks" is left enabled, as well as BoneMap'ed animations). Additionally, I added an easy way to export the rest pose as an Animation from the advanced import dialog.

without modifying the animation import pipeline.

I didn't modify the animation import pipeline directly. Same as your workflow, this is a feature in the scene importer which runs before rest fixer. Naturally, changing the scene import flow will affect imported animations, but it should not introduce tech debt here other than the new fields.

A possibility for triggering dependency could be having a list in the source file of other assets that rely on this profile. When this source model reimports, it could trigger those list of files for reimport after the source file is done (and any paths that don't exist anymore can be culled and warned about).

Yeah, I agree that there are some unsettled issues with dependencies of .import files. But I can't see a way around this other than to force resources local, which carries its own risks, since there will be stale copies of t-pose animations scattered throughout several .import files.

TL;DR: This design is a result of what I think is the best compromise we can achieve for now.

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.

The UI seems much improved. However, I feel that the Export Skeleton Rest Pose features need more discussion.

I understand where you are coming from with the desire to extract individual Animations, but from a consistency standpoint, an option to 'include rests as "REST" animation in the AnimationPlayer of imported tres', and separate the animation from the imported scene and save it as new animation would seem more appropriate, IMO. Then, other models should be able to refer that AnimationPlayer to specify the "REST" animation for Rest Pose.

Even if it is extracted separately, at least it should be a folded option like Use External, similar to Mesh and Material since this option should be not necessary in many cases.

image

@lyuma So, I believe that the Export Skeleton Rest Pose functionality can be separated from this PR for now, can you remove/separate that option once from this PR? Other than that, it seems to make sense and we can be Approved.

@lyuma
Copy link
Contributor Author

lyuma commented Mar 16, 2024

Before I update this PR, I would like to first understand the intended direction so that I can prepare both PRs: there is no point merging one feature without the other feature: a user cannot select a rest pose animation when there is no way to create a rest pose animation.

Sure, I would be fine without "Export Skeleton Rest Pose" since I am using Unidot to import fbx files, and Unidot now creates the rest pose animation automatically on 4.3. But losing this feature would interfere with ordinary users who are not using a third party tool script to create this animation automatically. While it does not need to be implemented in exactly this way, a way to create a "rest pose animation" does need to exist in some form in 4.3 for FBX animation import to be functional.

Therefore, I would like to understand the intended user-facing workflow / steps for FBX.

To be clear, here is the problem that is being solved with "Export Skeleton Rest Pose":
The user has two FBX files. One is in t-pose, call it "TPose.fbx". In particular, "TPose.fbx" may have no animations: it is just a skeleton and possibly a mesh. The other is an animation FBX, maybe "Walking.fbx" posed on the first frame (incorrect rest pose) and no mesh, so no bind pose.

Here is my understanding of the workflow you are proposing:

  1. Open the advanced importer of TPose.fbx
  2. Select "Include rests as "RESET" animation" checkbox on the Skeleton3D
  3. The advanced importer will refresh and the RESET animation is now listed as an animation
  4. The user selects to extract animations to a file or import as library
  5. Open "Walking.fbx" in the advanced importer
  6. Now select the extracted RESET animation in "Load pose"

Clearly, this "RESET" animation would always include all keys (ignore the immutable track policy), since all tracks are immutable when only one keyframe exists.

(also, is "REST" this the same thing as "RESET"? I think the similar names in all caps may be confusing me.)

@TokageItLab
Copy link
Member

TokageItLab commented Mar 16, 2024

I recognize that REST animation and RESET animation are different things.

RESET was supposed to define a pose at load time, but as we discussed on RocketChat, if that is not currently working and AutoPlay is working as an alternative, then I think it is acceptable to include it in RESET.

Also, it would be ideal to include that setting in the normal importer settings rather than the advanced importer.

image

Because the rest extraction itself is done outside of the Retartget process and has no direct impact on Retartget.

@lyuma
Copy link
Contributor Author

lyuma commented Mar 16, 2024

I think I understand: if it's in the normal import settings, then it can be generated before _pre_import completes, so it will enforce the creation of an AnimationPlayer and a rest animation, which can then be extracted using Godot's standard animation flow (AnimationLibrary or extract animations)

Ok, so adjusting my above workflow, it would be:

  1. Select TPose.fbx in the Import Dock
  2. (Optional) Enable "Import as skeleton bones"
  3. Enable "Generate Rest Pose Animation"
  4. Do not enable humanoid import. Instead:
    1. either: Switch "Import As" to AnimationLibrary and Reimport
    2. or: Open advanced importer and selects Extract Animations and Reimport.
  5. Select the other FBX (e.g. Walking.FBX)
  6. Enable "Import as skeleton bones"
  7. Open advanced importer:
  8. Enable BoneMap
  9. Set "Rest Pose" to "External Animation"
  10. Select the animation exported in step 4.
  11. Click Reimport

Does this flow look correct?

@TokageItLab
Copy link
Member

TokageItLab commented Mar 16, 2024

Enable "Import as skeleton bones"

Does this mean that you implement something new? Does this mean that it uses the Skeleton as its root, unlike the import as Scene which uses the Node3D as its root?

I think it is sufficient to add the option "Import Rest as RESET" under "Remove Immutable Tracks". Is the problem when there are more than one Skeletons?

@lyuma
Copy link
Contributor Author

lyuma commented Mar 16, 2024

Does this mean that you implement something new

No, since "Import as skeleton bones" is the feature implemented in #88819 and was already merged,

I am merely recommending to use this feature in conjunction: It should be documented as part of the recommended flow for animation import, since it will guarantee that the animation FBX contains a skeleton (otherwise, a FBX with no meshes might only contain nodes and no bones)

Does this mean that it uses the Skeleton as its root

No, that feature does not change the root of the scene to skeleton: this created too many compatibility problems, so the root is Node3D but the children are always Skeleton3D and AnimationPlayer if animations are present.

"Import Rest as RESET"

Sounds like a good name. I would be happy to implement this and name the resulting animation "RESET" by default.

Is the problem when there are more than one Skeletons?

It could be confusing when there are multiple skeletons.

However, the "Import as skeleton bones" feature above will guarantee that exactly one skeleton is present, since all nodes in the scene are now bones in that single skeleton.

Generally speaking, our workflow is not well suited to scenes with multiple skeletons, so this should be discussed separately with a specific example file (I have a few but they are not designed to be retargeted). The limitation of one logical skeleton per animation file seems fairly common across several engines / file formats, so I don't think we should try to solve this right now.

@TokageItLab
Copy link
Member

No, since "Import as skeleton bones" is the feature implemented in #88819 and was already merged,

Okay, so it is no problem. However, please keep in mind that ""Import Rest as RESET" option may be enabled even for Import as Scene. I think we now agree.

@lyuma
Copy link
Contributor Author

lyuma commented Mar 18, 2024

Updated this PR to remove the "Export Skeleton Rest Pose" option.

Opened #89629 to add a "Import Rest as Reset" option in the top-level import dock, which can be used to generate a compatible rest pose (t-pose) animation from fbx or similar files which did not come with one.

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.

LGTM

@TokageItLab TokageItLab modified the milestones: 4.x, 4.3 Mar 18, 2024
editor/import/3d/scene_import_settings.cpp Outdated Show resolved Hide resolved
editor/import/3d/scene_import_settings.cpp Outdated Show resolved Hide resolved
editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved
editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved
editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved
@lyuma lyuma force-pushed the retarget_silhouette_template branch from ce01f58 to 0b363e3 Compare March 19, 2024 09:20
@akien-mga
Copy link
Member

There seems to be a build issue for the editor:

Error: editor/import/3d/scene_import_settings.cpp:116:7: error: 'option' was not declared in this scope; did you mean 'options'?
  116 |   if (option.name == "rest_pose/load_pose") {
      |       ^~~~~~
      |       options
Error: editor/import/3d/scene_import_settings.cpp:121:7: error: 'option' was not declared in this scope; did you mean 'options'?
  121 |   if (option.name == "rest_pose/selected_animation") {
      |       ^~~~~~
      |       options

Adds `rest_pose/external_animation_library` advanced option to replace bone rest with an exported Animation before retargeting.
Together this allows a purely importer based workflow to transfer a known good pose from one FBX to another.
@fire fire force-pushed the retarget_silhouette_template branch from 0b363e3 to 9db0860 Compare March 24, 2024 00:32
@akien-mga akien-mga merged commit 9a8fb26 into godotengine:master Mar 24, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
7 participants