Skip to content

Conversation

fire
Copy link
Member

@fire fire commented Mar 29, 2022

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.

@fire fire force-pushed the fbx-import branch 3 times, most recently from 054fff8 to 030554f Compare March 29, 2022 01:43
@fire fire marked this pull request as ready for review March 29, 2022 01:52
@fire fire requested review from a team as code owners March 29, 2022 01:52
@Chaosus Chaosus added this to the 4.0 milestone Mar 29, 2022
Comment on lines 8 to 9
The location of the FBX2GLTF binary is set via editor settings "filesystem/gltf_from_fbx/fbx2gltf_path".
This importer is only used if project settings "filesystem/gltf_from_fbx/enabled" is enabled, otherwise [code].fbx[/code] present in the project folder are not imported.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The location of the FBX2GLTF binary is set via editor settings "filesystem/gltf_from_fbx/fbx2gltf_path".
This importer is only used if project settings "filesystem/gltf_from_fbx/enabled" is enabled, otherwise [code].fbx[/code] present in the project folder are not imported.
The location of the FBX2GLTF binary is set via [member EditorSettings.filesystem/gltf_from_fbx/fbx2gltf_path].
This importer is only used if [member ProjectSettings.filesystem/gltf_from_fbx/enabled] is enabled, otherwise [code].fbx[/code] present in the project folder are not imported.

@akien-mga
Copy link
Member

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.

@fire fire force-pushed the fbx-import branch 3 times, most recently from b9b0ef8 to 030554f Compare March 29, 2022 17:49
@fire
Copy link
Member Author

fire commented Mar 29, 2022

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

@fire fire force-pushed the fbx-import branch 4 times, most recently from 5ecbd4b to b761349 Compare March 30, 2022 05:40
@fire fire requested a review from a team as a code owner March 30, 2022 05:40
@fire fire force-pushed the fbx-import branch 3 times, most recently from 2f89ae8 to cff5aeb Compare March 30, 2022 10:28
@akien-mga
Copy link
Member

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

@akien-mga
Copy link
Member

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.

@fire
Copy link
Member Author

fire commented 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!

@fire
Copy link
Member Author

fire commented Mar 30, 2022

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.

@fire
Copy link
Member Author

fire commented Mar 30, 2022

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

@akien-mga
Copy link
Member

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).

@akien-mga
Copy link
Member

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.

@akien-mga
Copy link
Member

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>
Copy link
Member

@akien-mga akien-mga left a comment

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

@akien-mga akien-mga merged commit bfd26b3 into godotengine:master Mar 30, 2022
@akien-mga
Copy link
Member

Thanks! Amazing work :)

@fire 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.
@dioptryk
Copy link
Contributor

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.

@fire
Copy link
Member Author

fire commented May 26, 2022

Are you able to provide the test files for review?

@dioptryk
Copy link
Contributor

@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.

@fire
Copy link
Member Author

fire commented May 27, 2022

Email me on the email at https://chibifire.com

@dioptryk
Copy link
Contributor

Sent!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants