Add fbx2gltf support for importing .fbx files by fire · Pull Request #59653 · godotengine/godot (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Lets you drag or place .fbx files in the project folder and it will import the files.

An editor setting sets the location of the fbx2gltf binary.

Pending a proposal. Need to discuss with reduz the details.

Bugsquad edit: A 3.x backport will be considered, as it seems to really work much better than the current importer in 3.x.

Diddykonga, akien-mga, Calinou, barbaros83, lufog, ChuddyzZ, SimplyPhy, RevoluPowered, and realkotob reacted with thumbs up emoji Zireael07, akien-mga, SimplyPhy, and realkotob reacted with hooray emoji

fire marked this pull request as ready for review

March 29, 2022 01:52

Choose a reason for hiding this comment

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

I would also move this to modules/gltf/editor/ like the Blend importer. I can take care of these tweaks if you want and force push updates to both PRs.

I was unable to do this properly. I reverted to the last commit for now.

@fire I've fixed errors and cleaned things up locally, I can take it from there.

Here's a cleaned up version that matches the cleaned up Blend importer:
https://github.com/akien-mga/godot/tree/fbx-import

Some remaining issues I noted with both importers:

• The source and sink JSON stuff isn't exactly the same in both importers. Blend one globalizes the source but not the sink, and runs c_escape() on both. FBX one globalizes both and doesn't escape. Needs to be harmonized.
• I actually don't understand the point of this JSON stuff? It's not saved anywhere and it only contains the source and sink which are anyway passed as strings to OS.execute(), so... it could all be removed?
• Blend imports as .gltf + .bin while FBX imports as .glb. Is there a reason for the difference?
• Nitpick, but the Blend to glTF importer is called ...ImporterBlend while the FBX to glTF importer is called ...ImporterGLTFFromFBX. Maybe this can be harmonized? E.g. rename GLTFFromFBX to just FBX after the old importer has been removed, or also include GLTFFrom in the Blend one.

This was referenced

Mar 30, 2022

The source and sink JSON stuff isn't exactly the same in both importers. Blend one globalizes the source but not the sink, and runs c_escape() on both. FBX one globalizes both and doesn't escape. Needs to be harmonized.
I actually don't understand the point of this JSON stuff? It's not saved anywhere and it only contains the source and sink which are anyway passed as strings to OS.execute(), so... it could all be removed?

I think it was an attempt not allow the user generated path to change the output via adding new options to the execution. It probably can be removed.

Blend imports as .gltf + .bin while FBX imports as .glb. Is there a reason for the difference?

Handling of extracted textures is problematic. Recall in the voice call about the blender import keeping the original images. This feature isn't currently in the fbx2gltf importer. The feature of keeping the original image path can be added to fbx2gltf.

Nitpick, but the Blend to glTF importer is called ...ImporterBlend while the FBX to glTF importer is called ...ImporterGLTFFromFBX. Maybe this can be harmonized? E.g. rename GLTFFromFBX to just FBX after the old importer has been removed, or also include GLTFFrom in the Blend one.

Let's go with FBX and Blend unless you want GLTFFromFBX and BlendFromFBX. Abstraction!

Nitpick, but the Blend to glTF importer is called ...ImporterBlend while the FBX to glTF importer is called ...ImporterGLTFFromFBX. Maybe this can be harmonized? E.g. rename GLTFFromFBX to just FBX after the old importer has been removed, or also include GLTFFrom in the Blend one.

The problem was overlapping names. I couldn't call the GLTFFromFBX until the FBX importer is removed.

I am pulling https://github.com/akien-mga/godot/tree/fbx-import as-is because fighting the CICD pipeline is fun but exhausting.

Pending work.

1. Remove the json struct abstraction because the usecase for 5-10 user created configuration entries wasn't implemented
2. Rename GLTFFromFBX to FBX

Note that I just pushed an update because you changed the the Blend importer to enabled by default but this hadn't been reflected in the class reference (now fixed).

Pending work.

1. Remove the json struct abstraction because the usecase for 5-10 user created configuration entries wasn't implemented
2. Rename GLTFFromFBX to FBX

I can do that quickly, give me a minute.

It's worth nothing that since both the FBX and Blend importer are now enabled by default as of this PR, users will see warnings on editor start until they configure paths both to Blender and to FBX2glTF.

It looks like this:

WARNING: FBX file import is enabled, but no FBX2glTF path is configured. FBX files will not be imported.
     at: _editor_init (modules/gltf/register_types.cpp:95)

If you see this warning and it's a bother, you can either configure FBX2glTF using the latest @V-Sekai fork build for your platform: https://github.com/V-Sekai/FBX2glTF/releases
Or disable FBX importer in the Project Settings (avanced setting).

This will be improved soon to be more user-friendly, and we'll also document things properly eventually.

Lets you drag or place .fbx files in the project folder and it will import the files.

An editor setting sets the location of the fbx2gltf binary.

Enables .fbx and .blend by default.

Co-authored-by: Rémi Verschelde rverschelde@gmail.com

Choose a reason for hiding this comment

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

Tested with a MRP from one of the many outstanding FBX import bug reports, and it seems to work great :)

simplescreenrecorder-2022-03-30_15.27.23.mp4

fire deleted the fbx-import branch

March 30, 2022 14:50

gaudecker pushed a commit to gaudecker/godot that referenced this pull request

Apr 3, 2022

This importer was the fruit of a lot of amazing reverse engineering work by RevoluPowered, based on the original Assimp importer that was introduced by fire.

While promising and well tuned for a specific type of FBX scenes, it was found to have many flaws to support the many FBX exporters and legacy models that Godot users want to use. As we currently lack a maintainer to improve it, those issues are left unresolved and FBX import is still sub-par in the current Godot releases.

After some experimentation, we're instead adding a new importer that relies on Facebook's fbx2gltf command line tool to convert FBX to glTF, so that we can then use our well-maintained glTF importer. See godotengine#59653 and https://github.com/facebookincubator/FBX2glTF for details.

Does support for this tool replace the built-in fbx importer? Because, for some models I have, Godot actually does a better job than fbx2gltf (which does not even produce output). The commit does not remove anything, it seems, but maybe it was done earlier.

Are you able to provide the test files for review?

@fire Sure, I can send a few FBX, I'd just prefer direct contact to not attach them here; would you kindly provide an email? Github does not seem to provide a PM functionality.