Skip to content
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

Support COLLADA2GLTF for better imports. #63072

Closed
wants to merge 1 commit into from

Conversation

fire
Copy link
Member

@fire fire commented Jul 16, 2022

Fixes: godotengine/godot-proposals#4885
Fixes: #51148
See godotengine/godot-proposals#2130

This also removes the Godot collada importer.

editor_screenshot_2022-07-16T055120

@YuriSizov YuriSizov added this to the 4.0 milestone Jul 16, 2022
@fire fire marked this pull request as ready for review July 16, 2022 14:45
@fire fire requested review from a team as code owners July 16, 2022 14:45
@akien-mga
Copy link
Member

akien-mga commented Jul 16, 2022

We'll need heavy testing on many types of Collada exports to see how this converter fares and make sure it's better than our ad hoc importer. But on the principle I'm all for it.

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.

Code looks great, just a few nitpicks.

Comment on lines 11 to 14
"EditorSceneFormatImporterBlend",
"EditorSceneFormatImporterFBX",
"EditorSceneFormatImporterDAE",
"EditorSceneFormatImporterGLTF",
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetical ordering :)

Comment on lines +3 to +6
<brief_description>
</brief_description>
<description>
</description>
Copy link
Member

Choose a reason for hiding this comment

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

You can adapt description from the FBX one.

@@ -98,6 +99,26 @@ static void _editor_init() {
ResourceImporterScene::add_importer(importer);
}
}

// Collada importer.
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
// Collada importer.
// Collada to glTF importer.

@akien-mga
Copy link
Member

You can add this too to fix capitalization in the editor settings:

diff --git a/editor/editor_property_name_processor.cpp b/editor/editor_property_name_processor.cpp
index 09d2992e07..9f4ab6f1be 100644
--- a/editor/editor_property_name_processor.cpp
+++ b/editor/editor_property_name_processor.cpp
@@ -118,6 +118,7 @@ EditorPropertyNameProcessor::EditorPropertyNameProcessor() {
 	capitalize_string_remaps["bvh"] = "BVH";
 	capitalize_string_remaps["ca"] = "CA";
 	capitalize_string_remaps["cd"] = "CD";
+	capitalize_string_remaps["collada2gltf"] = "COLLADA2GLTF";
 	capitalize_string_remaps["cpu"] = "CPU";
 	capitalize_string_remaps["csg"] = "CSG";
 	capitalize_string_remaps["db"] = "dB";

@akien-mga akien-mga changed the title Support collada2gltf for better imports. Support COLLADA2GLTF for better imports. Jul 17, 2022
@akien-mga
Copy link
Member

akien-mga commented Jul 17, 2022

Tested a few scenes with COLLADA2GLTF 2.1.5 on Linux:

Screenshot_20220717_165409

image

So it's not a frank success so far, but to be fair there seems to be a lot of issues with importing animations and skeletons in general in master right now.


Tested with a build of COLLADA2GLTF from their master branch, not better.

For the record, diff to get it to compile with current GCC 12:

diff --git a/src/draco/core/hash_utils.h b/src/draco/core/hash_utils.h
index 0e8da60..8fa8574 100644
--- a/src/draco/core/hash_utils.h
+++ b/src/draco/core/hash_utils.h
@@ -15,6 +15,7 @@
 #ifndef DRACO_CORE_HASH_UTILS_H_
 #define DRACO_CORE_HASH_UTILS_H_
 
+#include <cstddef>
 #include <stdint.h>
 #include <functional>
 
diff --git a/src/draco/io/parser_utils.cc b/src/draco/io/parser_utils.cc
index 1aa52cc..85aed24 100644
--- a/src/draco/io/parser_utils.cc
+++ b/src/draco/io/parser_utils.cc
@@ -18,6 +18,7 @@
 #include <cctype>
 #include <cmath>
 #include <iterator>
+#include <limits>
 
 namespace draco {
 namespace parser {

to apply in the GLTF/dependencies/draco submodule.

Overall that library hasn't seen much activity since 2018 and seems more or less abandoned now. Might not be the best option.

We should see if we find another better converter, or get in touch with the lib maintainers and see what their plan is.

@fire
Copy link
Member Author

fire commented Aug 1, 2022

I don't think this was a success. The work is salvageable. The importer has a self contained system that can import using anything that can take the input, output and binary flags.

In the proposal thread sketchup mentioned gltf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants