-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Rewrite FBX Importer to support Maya #39418
Rewrite FBX Importer to support Maya #39418
Conversation
f9e03af
to
f09949e
Compare
8f32e91
to
7928bd9
Compare
81e4d5c
to
8e4a96d
Compare
All cases found for the DOM have been initialized @fire let me know if you can see anymore. I might actually split up the 'assimp' code into our single folder under modules since we are maintaining this ourselves in our codebase with our own types, so we don't have a third-party anymore |
c127185
to
843af0c
Compare
More instances of uninitialized variables are in ASSIMP::FBX:: on DOM elements themselves. :D Will fix this when I get some time after blend shapes. |
I have rewritten render code to make it more reliable as it's now abstract enough to hanlde lines, points, quads and triangles :) |
In the process of rewriting the MeshGeometry:: to make it only read the data rather than also manipulate it. This patch disables all mesh rendering as we are gonna rewrite the mesh_data cpp in godot to handle the FBX polygon type properly, so we can also directly use indices. This will resolve other issues too and make it testable. NormalsW is not handled at the moment but this opens the opportunity to support it. :) |
c0a1fd4
to
1033ce4
Compare
Importing empty fbx file exported from blender cause leak Leak:
|
Thanks, just to confirm what options do you use to detect the leak? |
|
Thanks! yeah just wanted to set up the same testing environment thank you so much for reporting this issue :) |
I believe the problem is the Assimp::FBX::Document is not deleted. I will make a patch now and see if this is resolved |
I have resolved the leak on my side, could you please confirm it's gone? |
I'm still having leak |
Sorry, indeed in latest commit there isn't any leak. You can add |
No need to apologize it happens and I have done this before myself too, I am just happy it's fixed, I didn't know that I needed DRI_PRIME what is that? |
This command switches management on/off(in case =0 - off) of hybrid GPU - https://wiki.archlinux.org/index.php/PRIME |
@RevoluPowered Here's the html log.
|
cb808ab
to
a6ff4bf
Compare
There are link errors if you try to compile with
You'll need to conditionally compile tests by defining |
Co-authored-by: Gordon MacPherson <[email protected]> Co-authored-by: Andrea Catania <[email protected]> Co-authored-by: K. S. Ernest (iFire) Lee <[email protected]> This is a complete rewrite of the importer. It will give more deterministic behaviour and has been sponsored by IMVU inc, over 1 year has gone into the development of this importer to remove the burden of the FBX SDK. This was my project for 1 entire year and I really enjoyed the opportunity to add to Godot. Along the road of implementing fixes we implemented fbx pivots, animations and inheritance type handling, which in most cases works properly. We have implemented animation and mesh skinning too this should work out of the box, if there are issues let us know. It's designed so that you can expand this with ease, and fix bugs easily too. It can import from Autodesk Maya and import into Godot, with pivots. There are bits we could polish but for now this is good enough. This was sponsored by IMVU, and a special thanks to everyone who supported this project. Signed-off-by: Gordon MacPherson <[email protected]> Additional fixes made before upstreaming: - fixed memory leak - ensure consistent ordering on mac linux and windows for fbx tree. (very important for material import to be deterministic) - disabled incorrect warnings for fbx_material - added compatibility code for /RootNode/ so compat is not broken - Optimise FBX - directly import triangles - remove debug messages - add messages for mesh id, mesh re-import is sometimes slow and we need to know what mesh is being worked on - correct culling settings on materials
a6ff4bf
to
9947061
Compare
Fixed :) We removed the old tests for now, no point in modifying them right now, more info is here: They're backed up and saved to be ported into 4.0 rather than added to 3.2 since doctest is not backported. |
also fixed some cleanup notes
- Document no longer uses unordered maps - Removed some usages of &GetRequiredToken replaced with safe *GetRequiredToken() function - Added parser debugging - Added ERR_FAIL_CONDS for unsupported mesh formats (we can add these later super easy to do now) - Add memory debugging for the Tokens and the TokenParser to make it safe - Add memory initialisation to mesh.cpp surface_tool.h and mesh.h - Initialize boolean flags properly - Refactored to correct naming for the fbx_mesh_data.h so you know what data you are working on - Disabled corruption caused by the FIXME: - Fixed document reading indexes and index_to_direct vs indexes mode
a0536a6
to
330e156
Compare
Superseded by #41979. |
Adds the ability to load Autodesk FBX files, without requiring the SDK or thirdparty dependencies. We don't support 3DS max as of yet, due to a limitation of geometric transform. We will add support for this in the future, if required.
Previously due to FBX Pivot Transforms this was not possible, also inherit type (nodes which do not inherit parent scale, or do different multiplication orders) was previously complex to support and is required to support Maya fully.
We abstract most of Ref away and keep this simple for anyone who needs to make some edit to the importer. Pivot transform handles pivot's and also inherit type (scaling ignored, parent before scaling, etc)
Previously we had four layers, one for assimp one for godot, and one for the importer itself converting the data to godot, this removes assimp from the middle simplifiying the method to
FBX Document -> Godot Importer (this is a direct importer now, and should be much more efficient in some cases over the previous importer)
Another advantage is we also only have a single set of core math types between FBX and Godot (Vector3, Vector2 and Transform), less conversions, less can go wrong.
This can be extended with as many features as we want now and there is now room to grow it into all the specific tools for Maya etc, or even possibly 3ds max.
Why the decision to drop assimp?
FBX Importer with viable support for commercial FBX files
This work is sponsored by IMVU.