Remove scene/3d/model_*.h from FBX pull request
#88680
Closed
Conversation
This file contains 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
fire
force-pushed
the
vsk-ufbx-untangle
branch
4 times, most recently
from
e41fee1 to
b1fb7c4
Compare
aaronfranke
reviewed
Feb 22, 2024
fire
force-pushed
the
vsk-ufbx-untangle
branch
from
b1fb7c4 to
0c01996
Compare
aaronfranke
reviewed
Feb 22, 2024
fire
force-pushed
the
vsk-ufbx-untangle
branch
2 times, most recently
from
c8991bf to
d0e5cf3
Compare
lyuma
reviewed
Feb 23, 2024
There was a problem hiding this comment.
Mostly approving topmost commit (Remove scene/3d/model_*.h from FBX commit.)
consolidating ModelDocument back into GLTFDocument (and same for ModelState) drastically simplifies the code and avoids any compatibility breakage.
The only change I would like to request is to have FBXDocument inherit from Resource (really it should be RefCounted but it doesn't matter) instead of GLTFDocument, and re-declare the three public methods rather than relying on inheritance.
lyuma
approved these changes
Feb 23, 2024
This update introduces a new import method for FBX files using ufbx. If the fbx2gltf import fails, it will use the most recently cached scene from the ufbx import. The process is sped up by introducing threads to load the ufbx portion. Key changes include: - Support for importing geometry helper nodes in FBX files. - Addition of cameras and lights with updated names. - Removal of the fbx importer manager. - Introduction of ModelDocument3D and updates to its methods. - Changes to FBX import options and visibility. - Updating the documentation and handling some errors. - Store the original non-unique node, mesh and animation names in FBX and glTF. Co-Authored-By: bqqbarbhg <bqqbarbhg@gmail.com>
- ModelDocument3D is completely dependent on gltf, so we can't have it in
`scene`, it breaks the build with gltf disabled, and is super hacky.
It was even depending on the FBX module, so I copied a helper method to
work it around.
* So I moved it back to the gltf module. Needs further work to actually
make it a proper abstraction, but currently this is not it.
- The gltf module should NOT depend on fbx. Modules are meant to be MODULAR,
they can't have circular dependencies like this. In theory, the fbx module
should also not depend on gltf, but I see we're far from that goal, so
I made the dependency explicit (fbx can't be enabled if gltf is disabled).
- Thus, I renamed and moved the FBX2glTF importer to the fbx module. The
hack used to find which importer to use, where each importer refers to the
other one, is really hacky... This also will need to be fixed. But I'm
aware it's dependent on a bug report, which I linked in a comment.
- Renamed `filesystem/import/fbx/enabled` to `fbx2gltf`, since it doesn't
control ufbx.
- Please please pay attention to includes, I'm tired of having to clean
them up. I didn't do a full review but I finally noticed that the forward
declares of core types in `gltf_defines.h`, combined with your IWYU-style
IDE plugin that always re-adds all includes, is a very cursed combo.
So I nuked the forward declares, if you want to always include everything,
let's not do it twice...
fire
force-pushed
the
vsk-ufbx-untangle
branch
from
d0e5cf3 to
6bd1a34
Compare
|
Updated to fix merge conflict in |
Mickeon
reviewed
Feb 23, 2024
Update GLTFDocument inheritance, method names, and parameters. Refactor FBXDocument methods for consistency with GLTFDocument. Update FBX and GLTF importers to use boolean flag for helper nodes.
fire
force-pushed
the
vsk-ufbx-untangle
branch
from
6bd1a34 to
e376762
Compare
Mickeon
approved these changes
Feb 23, 2024
Co-authored-by: Micky <66727710+Mickeon@users.noreply.github.com>
akien-mga
changed the title
scene/3d/model_*.h from FBX pull request
akien-mga
approved these changes
Feb 23, 2024
| ClassDB::bind_method(D_METHOD("write_to_filesystem_from_data", "state", "path"), | ||
| &ModelDocument3D::write_to_filesystem_from_data); | ||
| } | ||
| } |
| }; | ||
|
|
||
| #endif // MODEL_DOCUMENT_3D_H | ||
| #endif // SKIN_TOOL_H |
Comment on lines
+36
to
+38
| #include "modules/gltf/structures/gltf_node.h" | ||
| #include "modules/gltf/structures/gltf_skeleton.h" | ||
| #include "modules/gltf/structures/gltf_skin.h" |
There was a problem hiding this comment.
The module's own includes should be relative paths.
Suggested change
| #include "modules/gltf/structures/gltf_node.h" | |
| #include "modules/gltf/structures/gltf_skeleton.h" | |
| #include "modules/gltf/structures/gltf_skin.h" | |
| #include "structures/gltf_node.h" | |
| #include "structures/gltf_skeleton.h" | |
| #include "structures/gltf_skin.h" |
Comment on lines
34
to
+37
| #include "core/io/resource.h" | ||
|
|
||
| #include "../gltf_defines.h" | ||
|
|
There was a problem hiding this comment.
This should be changed back, the convention for includes in modules is:
- Module-scoped includes first
- Rest of the engine (core, scene, etc.)
- Thirdparty
27 tasks
|
Merged into #81746 with the edits I listed above. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As mentioned in #81746 (comment) here's the stacked commits.