Fix FBX runtime import

[Naming-things-is-hard-btw]

trafficstars

fixes #96029 fixes #96043

ufbx can import fbx files in the editor but not at runtime

edit: now it fully supports loading at runtime and in editor it fully supports document extensions it loads textures perfectly knowing when to load with a resourceloader and when to load with a buffer all tested animations, bones, textures, models, surfaces they all work now

[Naming-things-is-hard-btw]

btw im open to any improvements on this since this isn't a real solution but it works

[fire]

In gltf document we have a gltf extension system that converts this. But I was waiting for demand for this feature to restore it.

I’ll post a gdscript version momentarily and lets figure out how to add this properly

[fire]

https://github.com/V-Sekai/TOOL_model_explorer/blob/cb9d112f051ba5426cd04b6e95228f03c30c744f/vsk_model_explorer/core/UIController.gd#L85

Note that this has a bug where I free empty nested importer mesh instances in error

[Naming-things-is-hard-btw]

In gltf document we have a gltf extension system that converts this. But I was waiting for demand for this feature to restore it.

I’ll post a gdscript version momentarily and lets figure out how to add this properly

interesting.. does that mean that we will use gltf extension for fbx or we will make a separate extension system for fbx

[Naming-things-is-hard-btw]

https://github.com/V-Sekai/TOOL_model_explorer/blob/cb9d112f051ba5426cd04b6e95228f03c30c744f/vsk_model_explorer/core/UIController.gd#L85

Note that this has a bug where I free empty nested importer mesh instances in error

we can add a checker that will skip if the children array size is zero

[Naming-things-is-hard-btw]

https://github.com/V-Sekai/TOOL_model_explorer/blob/cb9d112f051ba5426cd04b6e95228f03c30c744f/vsk_model_explorer/core/UIController.gd#L85

Note that this has a bug where I free empty nested importer mesh instances in error

btw are you the one who worked on the importer? i was trying to talk to the one who implemented this for a very long time

[Naming-things-is-hard-btw]

In gltf document we have a gltf extension system that converts this. But I was waiting for demand for this feature to restore it.

I’ll post a gdscript version momentarily and lets figure out how to add this properly

should i convert this into draft so we can work on it together??

[Naming-things-is-hard-btw]

https://github.com/V-Sekai/TOOL_model_explorer/blob/cb9d112f051ba5426cd04b6e95228f03c30c744f/vsk_model_explorer/core/UIController.gd#L85

Note that this has a bug where I free empty nested importer mesh instances in error

i tried to build my version and i tried empty nested importer mesh instances and it didn't give an error since it checks for children size first in the for loop

//##################VVVVVVVVVVVVVV
for (int i = 0; i < children.size(); i++) {

[fire]

My original plan was to add a FBXDocument extension system and then use FBX runtime import as the conversion of ImporterMesh3D to MeshInstance3D as a runtime plugin.

I think that would be the proper way extending the fbx importer. What do you think?

Edited:

GLTFDocument Methods

Documentation Link

void register_gltf_document_extension(extension: GLTFDocumentExtension, first_priority: bool = false) static
void unregister_gltf_document_extension(extension: GLTFDocumentExtension) static

void register_fbx_document_extension(extension: FBXDocumentExtension, first_priority: bool = false) static
void unregister_fbx_document_extension(extension: FBXDocumentExtension) static

GLTFDocument Class Documentation

Edited:

https://github.com/godotengine/godot/blob/28a72fa4344befeae251d8e9b11b84acd601a244/modules/gltf/extensions/gltf_document_extension_convert_importer_mesh.cpp#L46-L87

[Naming-things-is-hard-btw]

My original plan was to add a FBXDocument extension system and then use FBX runtime import as the conversion of ImporterMesh3D to MeshInstance3D as a runtime plugin.

I think that would be the proper way extending the fbx importer. What do you think?

Edited:

GLTFDocument Methods

Documentation Link

void register_gltf_document_extension(extension: GLTFDocumentExtension, first_priority: bool = false) static
void unregister_gltf_document_extension(extension: GLTFDocumentExtension) static

void register_fbx_document_extension(extension: FBXDocumentExtension, first_priority: bool = false) static
void unregister_fbx_document_extension(extension: FBXDocumentExtension) static

GLTFDocument Class Documentation

Edited:

https://github.com/godotengine/godot/blob/28a72fa4344befeae251d8e9b11b84acd601a244/modules/gltf/extensions/gltf_document_extension_convert_importer_mesh.cpp#L46-L87

sounds cool but there's something i don't understand exactly why is fbx document inherited from gltf? wouldn't it be more convenient to make an abstract class that has the basic shared functions of the tow? and then inheriting both the gltf document and fbx document from it? that can make things 100x easier if we ever wanted to add more importer types support

because this feels like those situations where i make a grenade in a game then i inherit all bullets from it yea it works but that's confusing and might create alot of bugs in the future is it too much work to make this cleaner? or is it not worth it?

[fire]

That was the original design, but when we tried that the gltfdocument and fbxdocument didn't gain much from separation. We also had to duplicate the 10-20 auxiliary classes like Ref<GLTFMesh>.

[Naming-things-is-hard-btw]

That was the original design, but when we tried that the gltfdocument and fbxdocument didn't gain much from separation. We also had to duplicate the 10-20 auxiliary classes like Ref<GLTFMesh>.

oh i see then adding register_fbx_document_extension would be best right even tho we would have both register fbx and register gltf in fbx document class that is still far cleaner than duplicating most of the code another approach can be renaming register_gltf_document_extension to just register_document_extension and overriding it? but i dont think godots development guidelines allows that since it can break compatibility for cleaner code

anyways when can we work on implementing this? the important part is getting fbx runtime import support, code beauty is not that important also how does this work in the editor side? does it use extensions in editor but not ingame (that part was driving me insane hehe)

[Naming-things-is-hard-btw]

That was the original design, but when we tried that the gltfdocument and fbxdocument didn't gain much from separation. We also had to duplicate the 10-20 auxiliary classes like Ref<GLTFMesh>.

after more thinking about this i guess it would be better to just reuse gltf register funtion in fbx since fbx already uses alot of what gltf uses already (gltfmesh, gltfcamera, etc)

[Naming-things-is-hard-btw]

That was the original design, but when we tried that the gltfdocument and fbxdocument didn't gain much from separation. We also had to duplicate the 10-20 auxiliary classes like Ref<GLTFMesh>.

okay i have implemented it finally... can you verify it please

okay now it can import fbx almost perfectly it imports fbx but not the images saying there are no importers for that resource type

[fire]

I'm here. Let me review.

Edited:

Was away over the weekend.

[fire]

Can you run pre-commit run -a? https://docs.godotengine.org/en/stable/contributing/workflow/pr_review_guidelines.html

https://docs.godotengine.org/en/stable/contributing/development/code_style_guidelines.html#doc-code-style-guidelines

[lyuma]

@Naming-things-is-hard-btw

helo where did u disappear? that aint fun :<

Please try to treat contributors with respect and refrain from posting comments which do not contribute new information (especially including unrelated quote reply in each comment). It makes it hard to follow the conversation.

Things don't always get reviewed or merged immediately, so please be patient. Reviewers have to sleep, too.

[Naming-things-is-hard-btw]

@Naming-things-is-hard-btw

helo where did u disappear? that aint fun :<

Please try to treat contributors with respect and refrain from posting comments which do not contribute new information (especially including unrelated quote reply in each comment). It makes it hard to follow the conversation.

Things don't always get reviewed or merged immediately, so please be patient. Reviewers have to sleep, too.

sorry but i didnt know what to implement thats why he told me where and what to look for i completely understand if he doesn't have time for this ill be waiting

[Naming-things-is-hard-btw]

I'm here. Let me review.

Edited:

Was away over the weekend.

thanks for your time bro

btw i thought that it was complete i tried some tests and i cant import the images if the fbx model had external images for example it tries to import png files using resource loader is importing images also an extension or is it another thing

also i noticed that my coding style is a bit different than what godot coding style is so it refuses to accept the code how can i fix this?

[fire]

Try the earlier instructions about installing pre-commit to adjust style

[fire]

Import the images if the fbx model had external images. for example it tries to import png files using resource loader.

Is importing images also an extension or is it another thing?

We should be using the low level image importers at runtime I think, need to review the code. It's been a while since I looked at the gltf document (fbx document) image import.

[Naming-things-is-hard-btw]

Import the images if the fbx model had external images. for example it tries to import png files using resource loader. Is importing images also an extension or is it another thing?

We should be using the low level image importers at runtime I think, need to review the code. It's been a while since I looked at the gltf document (fbx document) image import.

im searching in gltf too i think theres an extension that does this currently i found ktx and webp importers as extensions but not png

heres the exact error [it thinks that the textures are resource or imported image type]

E 0:00:00:0782   Viewer.gd:28 @ _load_gltf_or_fbx(): No loader found for resource: /home/ahmad/Desktop/github_ufbx_bug/models/fbx/knight_texture.png (expected type: Texture2D)
  <C++ Error>    Method/function failed. Returning: Ref<Resource>()
  <C++ Source>   core/io/resource_loader.cpp:295 @ _load()
  <Stack Trace>  Viewer.gd:28 @ _load_gltf_or_fbx()
                 Viewer.gd:19 @ load_fbx()
                 Viewer.gd:14 @ _ready()

the wierd part is that it imports everything perfectly see

[Naming-things-is-hard-btw]

Can you run pre-commit run -a? https://docs.godotengine.org/en/stable/contributing/workflow/pr_review_guidelines.html

https://docs.godotengine.org/en/stable/contributing/development/code_style_guidelines.html#doc-code-style-guidelines

i did it and now its formatted but it looks like i accidentally broke the .net version of godot silly me

[Naming-things-is-hard-btw]

okai i fixed the error now it imports fbx with the images without showing any errors [finally!!!!!!!!!!!!!!!] but still i want it to be perfect now it has full extension support and it can load at runtime without any errors but the root node has no name which bugs me alot also there are some warnings about inconsistent owners i want to fix that too

[AThousandShips]

Please set up the pre-commit hook to make sure your code style is proper before you push

[Naming-things-is-hard-btw]

Please set up the pre-commit hook to make sure your code style is proper before you push

yea i did that but i forgot to use that this time sorry [the older commit was working prefectly]

[AThousandShips]

You need to install the hook and it'll do its work without you manually activating it, that way you won't forget

[Naming-things-is-hard-btw]

You need to install the hook and it'll do its work without you manually activating it, that way you won't forget

ok i installed it and i committed with it did that work

[AThousandShips]

It didn't because the changes you have that are using the wrong style are in earlier commits, you added commits after, you will need to apply the style fixes manually and after that things will work correctly if you have them installed

[Naming-things-is-hard-btw]

It didn't because the changes you have that are using the wrong style are in earlier commits, you added commits after, you will need to apply the style fixes manually and after that things will work correctly if you have them installed

wierd i tried using clang format on the older files i edited one by one to fix this btw how do you know that my coding style is what breaks it is it because the github tests fail? if so i tried to read it and it fails bacause there isnt a c# doc for the new class i added (how can i fix that)