From 4f6bb9316916edc395b437934d4e28ccd625dcf8 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:22:24 +0200 Subject: [PATCH 01/23] TinyGltfImporter: keep original buffer length for .glb conversion Needed for test files that have smaller/larger sizes --- .../TinyGltfImporter/Test/buffer-wrong-size.glb | Bin 348 -> 348 bytes .../TinyGltfImporter/Test/gltf2glb.py | 6 +++++- .../Test/image-buffer-embedded.glb | Bin 408 -> 408 bytes .../TinyGltfImporter/Test/image-buffer.glb | Bin 408 -> 408 bytes 4 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/buffer-wrong-size.glb b/src/MagnumPlugins/TinyGltfImporter/Test/buffer-wrong-size.glb index 163296563d11b7b92b5437dd530b1b7d927b779b..6fb6d2ece0c71d626b2c1a62618a25f7348ed93b 100644 GIT binary patch delta 31 icmcb^bcbm|`oxR}q6$0=3=B@5en7Ip9!NL Date: Fri, 8 Oct 2021 13:23:48 +0200 Subject: [PATCH 02/23] TinyGltfImporter: doc++ --- .../TinyGltfImporter/TinyGltfImporter.h | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h index 64221ea11..09a6620a0 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h @@ -122,7 +122,7 @@ See @ref building-plugins, @ref cmake-plugins, @ref plugins and @section Trade-TinyGltfImporter-behavior Behavior and limitations The plugin supports @ref ImporterFeature::OpenData and -@ref ImporterFeature::FileCallback features. The `tiny_gltf` library loads +@ref ImporterFeature::FileCallback features. The `TinyGLTF` library loads everything during initial import, meaning all external file loading callbacks are called with @ref InputFileCallbackPolicy::LoadTemporary and the resources can be safely freed right after the @ref openData() / @ref openFile() function @@ -170,7 +170,7 @@ Import of morph data is not supported at the moment. - If no @cb{.json} "scene" @ce property is present and the file contains at least one scene, @ref defaultScene() returns @cpp 0 @ce instead of - @cpp -1 @ce. According to the [glTF 2.0 specification](https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#scenes) + @cpp -1 @ce. According to the [glTF 2.0 specification](https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html#scenes) the importer is free to not render anything, but the suggested behavior would break even some official sample models. - In case object transformation is set via separate @@ -189,7 +189,7 @@ Import of morph data is not supported at the moment. @subsection Trade-TinyGltfImporter-behavior-lights Light import -- The importer supports the [KHR_lights_punctual](https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_lights_punctual) +- The importer supports the [KHR_lights_punctual](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_lights_punctual/README.md) extension @subsection Trade-TinyGltfImporter-behavior-meshes Mesh import @@ -203,7 +203,7 @@ Import of morph data is not supported at the moment. @ref VertexFormat::Vector3bNormalized, @ref VertexFormat::Vector3usNormalized or @ref VertexFormat::Vector3sNormalized (which includes the additional types - specified by [KHR_mesh_quantization](https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_mesh_quantization/README.md)) + specified by [KHR_mesh_quantization](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_mesh_quantization/README.md)) - Normals, if any, are imported as @ref VertexFormat::Vector3, @ref VertexFormat::Vector3bNormalized or @ref VertexFormat::Vector3sNormalized @@ -214,7 +214,7 @@ Import of morph data is not supported at the moment. @ref VertexFormat::Vector3bNormalized, @ref VertexFormat::Vector3usNormalized or @ref VertexFormat::Vector3sNormalized (which includes the additional types - specified by [KHR_mesh_quantization](https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_mesh_quantization/README.md)). The + specified by [KHR_mesh_quantization](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_mesh_quantization/README.md)). The data are by default Y-flipped on import unless @cb{.conf} textureCoordinateYFlipInMaterial @ce is either explicitly enabled, or if the file contains non-normalized integer or normalized @@ -268,24 +268,24 @@ fail. @subsection Trade-TinyGltfImporter-behavior-materials Material import -- Builtin [metallic/roughness](https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#metallic-roughness-material) material is imported always, +- Builtin [metallic/roughness](https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html#metallic-roughness-material) material is imported always, setting @ref MaterialType::PbrMetallicRoughness on the @ref MaterialData. Unfortunately TinyGLTF doesn't provide a way to detect if metallic/roughness properties are actually present, so this type is set always. -- If the [KHR_materials_pbrSpecularGlossiness](https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_materials_pbrSpecularGlossiness) +- If the [KHR_materials_pbrSpecularGlossiness](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_materials_pbrSpecularGlossiness/README.md) extension is present, its properties are imported with @ref MaterialType::PbrSpecularGlossiness present in material types. - Additional normal, occlusion and emissive maps are imported, together with related properties -- If the [KHR_materials_unlit](https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_materials_unlit/README.md) +- If the [KHR_materials_unlit](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_materials_unlit/README.md) extension is present, @ref MaterialType::Flat is set in material types, replacing @ref MaterialType::PbrMetallicRoughness or @ref MaterialType::PbrSpecularGlossiness. -- If the [KHR_materials_clearcoat](https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_materials_clearcoat) +- If the [KHR_materials_clearcoat](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_materials_clearcoat/README.md) extension is present, @ref MaterialType::PbrClearCoat is set in material types, and a new layer with clearcoat properties is added -- Custom texture coordinate sets as well as [KHR_texture_transform](https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_texture_transform/README.md) +- Custom texture coordinate sets as well as [KHR_texture_transform](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_texture_transform/README.md) properties are imported on all textures. - If the on-by-default @cb{.ini} phongMaterialFallback @ce @ref Trade-TinyGltfImporter-configuration "configuration option" is From fba97006092ada199bc07fbdddda5ed3b5d0c129 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:24:47 +0200 Subject: [PATCH 03/23] TinyGltfImporter: document missing URI decoding --- src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h index 09a6620a0..d3065a471 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h @@ -131,6 +131,10 @@ calls with @ref InputFileCallbackPolicy::LoadTemporary and @ref InputFileCallbackPolicy::Close is emitted right after the file is fully read. +[Percent-encoded](https://datatracker.ietf.org/doc/html/rfc3986#section-2.1) +external file paths are not decoded before loading files. If you need to +support those paths, you can intercept and decode them with a file callback. + Import of morph data is not supported at the moment. @subsection Trade-TinyGltfImporter-behavior-animation Animation and skin import From e85f528cd5cde2c2be0c68241e8eba5805ad522d Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:25:33 +0200 Subject: [PATCH 04/23] TinyGltfImporter: document handling (or not) of extensionsRequired --- src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h index d3065a471..b175df474 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h @@ -135,6 +135,11 @@ read. external file paths are not decoded before loading files. If you need to support those paths, you can intercept and decode them with a file callback. +The content of the global [`extensionsRequired`](https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html#specifying-extensions) +array is ignored. If a glTF file requires an unknown extension, the import will +most likely succeed, but some things might be missing or not get imported +correctly. + Import of morph data is not supported at the moment. @subsection Trade-TinyGltfImporter-behavior-animation Animation and skin import From 210e0b1debe084a2d911277411e3bb5873089436 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:28:14 +0200 Subject: [PATCH 05/23] TinyGltfImporter: test asset version --- .../TinyGltfImporter/Test/CMakeLists.txt | 6 ++- .../Test/TinyGltfImporterTest.cpp | 39 ++++++++++++++++++- .../TinyGltfImporter/Test/version-legacy.gltf | 5 +++ .../Test/version-supported.gltf | 6 +++ .../Test/version-unsupported-min.gltf | 6 +++ .../Test/version-unsupported.gltf | 5 +++ .../TinyGltfImporter/TinyGltfImporter.cpp | 15 +++++++ 7 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/version-legacy.gltf create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/version-supported.gltf create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported-min.gltf create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt index a92b5f361..a8bfe6397 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt @@ -192,7 +192,11 @@ corrade_add_test(TinyGltfImporterTest texture-empty-sampler.gltf texture-empty-sampler.glb texture-invalid.gltf - texture-missing-source.gltf) + texture-missing-source.gltf + version-legacy.gltf + version-supported.gltf + version-unsupported.gltf + version-unsupported-min.gltf) target_include_directories(TinyGltfImporterTest PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/$) if(MAGNUM_TINYGLTFIMPORTER_BUILD_STATIC) target_link_libraries(TinyGltfImporterTest PRIVATE TinyGltfImporter) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 808fd7c10..2024b321b 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -168,6 +168,9 @@ struct TinyGltfImporterTest: TestSuite::Tester { void escapedStrings(); void encodedUris(); + void versionSupported(); + void versionUnsupported(); + /* Needs to load AnyImageImporter from system-wide location */ PluginManager::Manager _manager; }; @@ -467,6 +470,16 @@ constexpr struct { {"embedded binary", "-embedded.glb"}, }; +constexpr struct { + const char* name; + const char* file; + const char* message; +} UnsupportedVersionData[]{ + {"legacy major version", "version-legacy.gltf", "unsupported version 1.0, expected 2.x"}, + {"unknown major version", "version-unsupported.gltf", "unsupported version 3.0, expected 2.x"}, + {"unknown minor version", "version-unsupported-min.gltf", "unsupported minVersion 2.1, expected 2.0"} +}; + using namespace Magnum::Math::Literals; TinyGltfImporterTest::TinyGltfImporterTest() { @@ -619,7 +632,12 @@ TinyGltfImporterTest::TinyGltfImporterTest() { addTests({&TinyGltfImporterTest::utf8filenames, &TinyGltfImporterTest::escapedStrings, - &TinyGltfImporterTest::encodedUris}); + &TinyGltfImporterTest::encodedUris, + + &TinyGltfImporterTest::versionSupported}); + + addInstancedTests({&TinyGltfImporterTest::versionUnsupported}, + Containers::arraySize(UnsupportedVersionData)); /* Load the plugin directly from the build tree. Otherwise it's static and already loaded. It also pulls in the AnyImageImporter dependency. Reset @@ -3965,6 +3983,25 @@ void TinyGltfImporterTest::encodedUris() { CORRADE_COMPARE(strings[5], "image-escaped%2Fříční%20člun.png"); } +void TinyGltfImporterTest::versionSupported() { + Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); + + CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, + "version-supported.gltf"))); +} + +void TinyGltfImporterTest::versionUnsupported() { + auto&& data = UnsupportedVersionData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); + + std::ostringstream out; + Error redirectError{&out}; + CORRADE_VERIFY(!importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, data.file))); + CORRADE_COMPARE(out.str(), Utility::formatString("Trade::CgltfImporter::openData(): {}\n", data.message)); +} + }}}} CORRADE_TEST_MAIN(Magnum::Trade::Test::TinyGltfImporterTest) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/version-legacy.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/version-legacy.gltf new file mode 100644 index 000000000..6d9cab8b6 --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/version-legacy.gltf @@ -0,0 +1,5 @@ +{ + "asset": { + "version": "1.0" + } +} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/version-supported.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/version-supported.gltf new file mode 100644 index 000000000..c8bbce873 --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/version-supported.gltf @@ -0,0 +1,6 @@ +{ + "asset": { + "version": "2.1", + "minVersion": "2.0" + } +} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported-min.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported-min.gltf new file mode 100644 index 000000000..fd7acf05a --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported-min.gltf @@ -0,0 +1,6 @@ +{ + "asset": { + "version": "2.1", + "minVersion": "2.1" + } +} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported.gltf new file mode 100644 index 000000000..208cc089f --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/version-unsupported.gltf @@ -0,0 +1,5 @@ +{ + "asset": { + "version": "3.0" + } +} diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 4afa8daca..185c06094 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -340,6 +340,21 @@ void TinyGltfImporter::doOpenData(const Containers::ArrayView data) return; } + /* Major versions are forward- and backward-compatible, but minVersion can + be used to require support for features added in new minor versions. + So far there's only 2.0 so we can use an exact comparison. */ + const tinygltf::Asset& asset = _d->model.asset; + if(!asset.minVersion.empty() && asset.minVersion != "2.0") { + Error{} << "Trade::CgltfImporter::openData(): unsupported minVersion" << asset.minVersion << Debug::nospace << ", expected 2.0"; + doClose(); + return; + } + if(!asset.version.empty() && asset.version.find("2.") != 0) { + Error{} << "Trade::CgltfImporter::openData(): unsupported version" << asset.version << Debug::nospace << ", expected 2.x"; + doClose(); + return; + } + /* Bounds checks that can't be deferred to later. No, tinygltf doesn't check for this. */ if(_d->model.defaultScene != -1 && UnsignedInt(_d->model.defaultScene) >= _d->model.scenes.size()) { From 0cc06616256327d0c759520981540f4826551e62 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:32:10 +0200 Subject: [PATCH 06/23] TinyGltfImporter: check for invalid scene hierarchies - scene node list must only contain root nodes - nodes must not have multiple parents - cycle detection --- .../TinyGltfImporter/Test/CMakeLists.txt | 2 + .../Test/TinyGltfImporterTest.cpp | 26 +++++----- .../Test/scene-invalid-child-not-root.gltf | 19 ++++++++ .../Test/scene-invalid-multiple-parents.gltf | 16 +++++++ .../TinyGltfImporter/TinyGltfImporter.cpp | 48 +++++++++++++++++++ 5 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-child-not-root.gltf create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-multiple-parents.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt index a8bfe6397..658f9880b 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt @@ -162,12 +162,14 @@ corrade_add_test(TinyGltfImporterTest scene-nodefault.gltf scene-nodefault.glb scene-invalid-camera-oob.gltf + scene-invalid-child-not-root.gltf scene-invalid-child-oob.gltf scene-invalid-default-oob.gltf scene-invalid-light-oob.gltf scene-invalid-material-oob-multi-primitive.gltf scene-invalid-material-oob.gltf scene-invalid-mesh-oob.gltf + scene-invalid-multiple-parents.gltf scene-invalid-node-oob.gltf scene-invalid-skin-oob-multi-primitive.gltf scene-invalid-skin-oob.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 2024b321b..4a1c81be6 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -108,7 +108,7 @@ struct TinyGltfImporterTest: TestSuite::Tester { void sceneInvalidMesh(); void sceneInvalidScene(); void sceneInvalidDefaultScene(); - void sceneCycle(); + void sceneInvalidHierarchy(); void objectTransformation(); void objectTransformationQuaternionNormalizationEnabled(); @@ -384,9 +384,12 @@ constexpr struct { constexpr struct { const char* name; const char* file; -} SceneCycleData[]{ - {"child is self", "scene-cycle.gltf"}, - {"great-grandchild is self", "scene-cycle-deep.gltf"} + const char* message; +} SceneInvalidHierarchyData[]{ + {"scene node has parent", "scene-invalid-child-not-root.gltf", "node 1 in scene 0 is not a root node"}, + {"node has multiple parents", "scene-invalid-multiple-parents.gltf", "node 2 has multiple parents"}, + {"child is self", "scene-cycle.gltf", "node tree contains cycle starting at node 0"}, + {"great-grandchild is self", "scene-cycle-deep.gltf", "node tree contains cycle starting at node 0"} }; constexpr struct { @@ -547,8 +550,8 @@ TinyGltfImporterTest::TinyGltfImporterTest() { addTests({&TinyGltfImporterTest::sceneInvalidScene, &TinyGltfImporterTest::sceneInvalidDefaultScene}); - addInstancedTests({&TinyGltfImporterTest::sceneCycle}, - Containers::arraySize(SceneCycleData)); + addInstancedTests({&TinyGltfImporterTest::sceneInvalidHierarchy}, + Containers::arraySize(SceneInvalidHierarchyData)); addInstancedTests({&TinyGltfImporterTest::objectTransformation}, Containers::arraySize(SingleFileData)); @@ -1735,15 +1738,16 @@ void TinyGltfImporterTest::sceneInvalidDefaultScene() { CORRADE_COMPARE(out.str(), "Trade::TinyGltfImporter::openData(): scene index 0 out of bounds for 0 scenes\n"); } -void TinyGltfImporterTest::sceneCycle() { - auto&& data = SceneCycleData[testCaseInstanceId()]; +void TinyGltfImporterTest::sceneInvalidHierarchy() { + auto&& data = SceneInvalidHierarchyData[testCaseInstanceId()]; setTestCaseDescription(data.name); Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); - CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, data.file))); - CORRADE_EXPECT_FAIL("Cyclic node hierarchy is not checked."); - CORRADE_VERIFY(!importer->object3D(data.name)); + std::ostringstream out; + Error redirectError{&out}; + CORRADE_VERIFY(!importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, data.file))); + CORRADE_COMPARE(out.str(), Utility::formatString("Trade::TinyGltfImporter::openData(): {}\n", data.message)); } void TinyGltfImporterTest::objectTransformation() { diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-child-not-root.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-child-not-root.gltf new file mode 100644 index 000000000..ab348195e --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-child-not-root.gltf @@ -0,0 +1,19 @@ +{ + "asset": { + "version": "2.0" + }, + "scenes": [ + { + "name": "scene node has parent", + "nodes": [0, 1] + } + ], + "nodes": [ + { + "children": [1] + }, + { + + } + ] +} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-multiple-parents.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-multiple-parents.gltf new file mode 100644 index 000000000..39a9a15ff --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/scene-invalid-multiple-parents.gltf @@ -0,0 +1,16 @@ +{ + "asset": { + "version": "2.0" + }, + "nodes": [ + { + "children": [2] + }, + { + "children": [2] + }, + { + "name": "node has multiple parents" + } + ] +} diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 185c06094..accd8f597 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -363,6 +363,54 @@ void TinyGltfImporter::doOpenData(const Containers::ArrayView data) return; } + /* Check node hierarchy for forbidden parent-child relationships and + non-root scene nodes */ + + std::unordered_map parentFor; + for(std::size_t i = 0; i != _d->model.nodes.size(); ++i) { + for(Int child: _d->model.nodes[i].children) { + const auto inserted = parentFor.emplace(child, i); + if(!inserted.second) { + Error{} << "Trade::TinyGltfImporter::openData(): node" << child << "has multiple parents"; + doClose(); + return; + } + } + } + + for(std::size_t i = 0; i != _d->model.scenes.size(); ++i) { + for(Int node: _d->model.scenes[i].nodes) { + if(parentFor.count(node)) { + Error{} << "Trade::TinyGltfImporter::openData(): node" << node << "in scene" << i << "is not a root node"; + doClose(); + return; + } + } + } + + for(std::size_t i = 0; i != _d->model.nodes.size(); ++i) { + auto getParent = [&](Int node) -> Int { + const auto it = parentFor.find(node); + if(it == parentFor.end()) + return -1; + return it->second; + }; + + Int p1 = getParent(i); + Int p2 = getParent(p1); + + while(p1 != -1 && p2 != -1) { + if(p1 == p2) { + Error{} << "Trade::TinyGltfImporter::openData(): node tree contains cycle starting at node" << i; + doClose(); + return; + } + + p1 = getParent(p1); + p2 = getParent(getParent(p2)); + } + } + /* Treat meshes with multiple primitives as separate meshes. Each mesh gets duplicated as many times as is the size of the primitives array. */ _d->meshSizeOffsets.emplace_back(0); From e14551f5004c433e78aff4d13812a78945b0352f Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:34:42 +0200 Subject: [PATCH 07/23] TinyGltfImporter: mesh spanning multiple buffers is now an error Used to be an assert and consequently untested --- .../Test/TinyGltfImporterTest.cpp | 3 ++- .../TinyGltfImporter/Test/mesh-invalid.gltf | 27 +++++++++++++++++++ .../TinyGltfImporter/TinyGltfImporter.cpp | 6 +++-- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 4a1c81be6..a1d15d869 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -343,7 +343,8 @@ constexpr struct { {"buffer view range out of bounds", "mesh-invalid-bufferview-short.gltf", "bufferView 0 needs 72 bytes but buffer 0 has only 68"}, {"buffer index out of bounds", "mesh-invalid-buffer-oob.gltf", "buffer 1 out of bounds for 1 buffers"}, {"buffer view index out of bounds", "mesh-invalid-bufferview-oob.gltf", "bufferView 4 out of bounds for 1 views"}, - {"accessor index out of bounds", "mesh-invalid-accessor-oob.gltf", "accessor 2 out of bounds for 2 accessors"} + {"accessor index out of bounds", "mesh-invalid-accessor-oob.gltf", "accessor 2 out of bounds for 2 accessors"}, + {"multiple buffers", "mesh-invalid.gltf", "meshes spanning multiple buffers are not supported"} }; constexpr struct { diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf index 16a8d5f08..21ae032ab 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf @@ -227,6 +227,17 @@ } } ] + }, + { + "name": "multiple buffers", + "primitives": [ + { + "attributes": { + "POSITION": 0, + "TEXCOORD_0": 16 + } + } + ] } ], "accessors": [ @@ -356,6 +367,13 @@ "componentType": 5126, "count": 1, "type": "VEC3" + }, + { + "name": "16", + "bufferView": 2, + "componentType": 5126, + "count": 3, + "type": "VEC2" } ], "bufferViews": [ @@ -369,12 +387,21 @@ "buffer": 0, "byteOffset": 36, "byteLength": 32 + }, + { + "buffer": 1, + "byteOffset": 124, + "byteLength": 24 } ], "buffers": [ { "byteLength": 68, "uri": "mesh-invalid.bin" + }, + { + "byteLength": 160, + "uri": "mesh.bin" } ] } diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index accd8f597..5f41247ae 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -1484,8 +1484,10 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un vertexCount = accessor.count; } else { /* ... and probably never will be */ - CORRADE_ASSERT(std::size_t(bufferView.buffer) == bufferId, - "Trade::TinyGltfImporter::mesh(): meshes spanning multiple buffers are not supported, sorry", {}); + if(std::size_t(bufferView.buffer) != bufferId) { + Error{} << "Trade::TinyGltfImporter::mesh(): meshes spanning multiple buffers are not supported"; + return Containers::NullOpt; + } bufferRange = Math::join(bufferRange, Math::Range1D::fromSize(bufferView.byteOffset, bufferView.byteLength)); From fb14494239a2250e4d1aab25aa3e7cbe84ab3765 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:40:36 +0200 Subject: [PATCH 08/23] TinyGltfImporter: support KHR_texture_basisu The MSFT_texture_dds in the test files is there so other importers (CgltfImporter, really) can share test files. Also for CgltfImporter, the invalid basisu index test needs to be separate because that's checked by cgltf at import. --- .../TinyGltfImporter/Test/CMakeLists.txt | 3 + .../Test/TinyGltfImporterTest.cpp | 67 +++++++++++ ...texture-extensions-invalid-basisu-oob.gltf | 21 ++++ .../Test/texture-extensions-invalid.gltf | 42 +++++++ .../Test/texture-extensions.gltf | 107 ++++++++++++++++++ .../TinyGltfImporter/TinyGltfImporter.cpp | 73 +++++++----- .../TinyGltfImporter/TinyGltfImporter.h | 23 ++-- 7 files changed, 300 insertions(+), 36 deletions(-) create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid-basisu-oob.gltf create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid.gltf create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt index 658f9880b..24f2e443d 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt @@ -193,6 +193,9 @@ corrade_add_test(TinyGltfImporterTest texture-default-sampler.glb texture-empty-sampler.gltf texture-empty-sampler.glb + texture-extensions.gltf + texture-extensions-invalid.gltf + texture-extensions-invalid-basisu-oob.gltf texture-invalid.gltf texture-missing-source.gltf version-legacy.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index a1d15d869..92f81dcd7 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -150,6 +150,8 @@ struct TinyGltfImporterTest: TestSuite::Tester { void textureDefaultSampler(); void textureEmptySampler(); void textureMissingSource(); + void textureExtensions(); + void textureExtensionsInvalid(); void imageEmbedded(); void imageExternal(); @@ -444,6 +446,31 @@ constexpr struct { {"invalid sampler wrapT", "invalid wrap mode 4"} }; +constexpr struct { + const char* name; + const UnsignedInt id; +} TextureExtensionsData[]{ + {"GOOGLE_texture_basis", 1}, + {"KHR_texture_basisu", 2}, + /* unknown extension, falls back to default source */ + {"MSFT_texture_dds", 0}, + {"MSFT_texture_dds and GOOGLE_texture_basis", 1}, + /* KHR_texture_basisu has preference */ + {"GOOGLE_texture_basis and KHR_texture_basisu", 2}, + {"unknown extension", 0}, + {"GOOGLE_texture_basis and unknown", 1} +}; + +constexpr struct { + const char* name; + const char* file; + const char* message; +} TextureExtensionsInvalidData[]{ + {"out of bounds GOOGLE_texture_basis", "texture-extensions-invalid.gltf", "GOOGLE_texture_basis image 3 out of bounds for 3 images"}, + {"out of bounds KHR_texture_basisu", "texture-extensions-invalid-basisu-oob.gltf", "KHR_texture_basisu image 0 out of bounds for 0 images"}, + {"unknown extension, no fallback", "texture-extensions-invalid.gltf", "no image source found"} +}; + constexpr struct { const char* name; const char* suffix; @@ -614,6 +641,12 @@ TinyGltfImporterTest::TinyGltfImporterTest() { addTests({&TinyGltfImporterTest::textureMissingSource}); + addInstancedTests({&TinyGltfImporterTest::textureExtensions}, + Containers::arraySize(TextureExtensionsData)); + + addInstancedTests({&TinyGltfImporterTest::textureExtensionsInvalid}, + Containers::arraySize(TextureExtensionsInvalidData)); + addInstancedTests({&TinyGltfImporterTest::imageEmbedded}, Containers::arraySize(ImageEmbeddedData)); @@ -3599,6 +3632,40 @@ void TinyGltfImporterTest::textureMissingSource() { CORRADE_COMPARE(out.str(), "Trade::TinyGltfImporter::texture(): no image source found\n"); } +void TinyGltfImporterTest::textureExtensions() { + auto&& data = TextureExtensionsData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); + + CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, + "texture-extensions.gltf"))); + + /* Check we didn't forget to test anything */ + CORRADE_COMPARE(importer->textureCount(), Containers::arraySize(TextureExtensionsData)); + + auto texture = importer->texture(data.name); + CORRADE_VERIFY(texture); + CORRADE_COMPARE(texture->image(), data.id); +} + +void TinyGltfImporterTest::textureExtensionsInvalid() { + auto&& data = TextureExtensionsInvalidData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); + + CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, data.file))); + + /* Check we didn't forget to test anything */ + CORRADE_VERIFY(Containers::arraySize(TextureExtensionsInvalidData) >= importer->textureCount()); + + std::ostringstream out; + Error redirectError{&out}; + CORRADE_VERIFY(!importer->texture(data.name)); + CORRADE_COMPARE(out.str(), Utility::formatString("Trade::TinyGltfImporter::texture(): {}\n", data.message)); +} + constexpr char ExpectedImageData[] = "\xa8\xa7\xac\xff\x9d\x9e\xa0\xff\xad\xad\xac\xff\xbb\xbb\xba\xff\xb3\xb4\xb6\xff" "\xb0\xb1\xb6\xff\xa0\xa0\xa1\xff\x9f\x9f\xa0\xff\xbc\xbc\xba\xff\xcc\xcc\xcc\xff" diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid-basisu-oob.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid-basisu-oob.gltf new file mode 100644 index 000000000..e6994ea5e --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid-basisu-oob.gltf @@ -0,0 +1,21 @@ +{ + "asset" : { + "version" : "2.0" + }, + "textures" : [ + { + "name": "out of bounds KHR_texture_basisu", + "extensions": { + "KHR_texture_basisu": { + "source": 0 + } + } + } + ], + "extensionsUsed": [ + "KHR_texture_basisu" + ], + "extensionsRequired": [ + "KHR_texture_basisu" + ] +} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid.gltf new file mode 100644 index 000000000..fd96dfdf5 --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions-invalid.gltf @@ -0,0 +1,42 @@ +{ + "asset" : { + "version" : "2.0" + }, + "images" : [ + { + "uri": "data:application/octet-stream;base64,AAA=", + "mimeType": "image/x-basis" + }, + { + "uri" : "data:application/octet-stream;base64,AAA=", + "mimeType": "image/jpeg" + }, + { + "uri" : "data:application/octet-stream;base64,AAA=", + "mimeType": "image/mgn" + } + ], + "textures" : [ + { + "name": "out of bounds GOOGLE_texture_basis", + "source" : 1, + "extensions": { + "GOOGLE_texture_basis": { + "source": 3 + } + } + }, + { + "name": "unknown extension, no fallback", + "extensions": { + "MGNM_fake_extension": { + "source": 2 + } + } + } + ], + "extensionsUsed": [ + "MGNM_fake_extension", + "GOOGLE_texture_basis" + ] +} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions.gltf new file mode 100644 index 000000000..e98dd81ac --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/texture-extensions.gltf @@ -0,0 +1,107 @@ +{ + "asset" : { + "version" : "2.0" + }, + "images" : [ + { + "uri": "data:application/octet-stream;base64,AAA=", + "mimeType": "image/png" + }, + { + "uri" : "data:application/octet-stream;base64,AAA=", + "mimeType": "image/x-basis" + }, + { + "uri" : "data:application/octet-stream;base64,AAA=", + "mimeType": "image/ktx2" + }, + { + "uri" : "data:application/octet-stream;base64,AAA=", + "mimeType": "image/vnd-ms.dds" + }, + { + "uri" : "data:application/octet-stream;base64,AAA=", + "mimeType": "image/mgn" + } + ], + "textures" : [ + { + "name": "GOOGLE_texture_basis", + "source": 0, + "extensions": { + "GOOGLE_texture_basis": { + "source": 1 + } + } + }, + { + "name": "KHR_texture_basisu", + "source": 0, + "extensions": { + "KHR_texture_basisu": { + "source": 2 + } + } + }, + { + "name": "MSFT_texture_dds", + "source": 0, + "extensions": { + "MSFT_texture_dds": { + "source": 3 + } + } + }, + { + "name": "MSFT_texture_dds and GOOGLE_texture_basis", + "source": 0, + "extensions": { + "MSFT_texture_dds": { + "source": 3 + }, + "GOOGLE_texture_basis": { + "source": 1 + } + } + }, + { + "name": "GOOGLE_texture_basis and KHR_texture_basisu", + "source": 0, + "extensions": { + "GOOGLE_texture_basis": { + "source": 1 + }, + "KHR_texture_basisu": { + "source": 2 + } + } + }, + { + "name": "unknown extension", + "source": 0, + "extensions": { + "MGNM_fake_extension": { + "source": 4 + } + } + }, + { + "name": "GOOGLE_texture_basis and unknown", + "source": 0, + "extensions": { + "MGNM_fake_extension": { + "source": 4 + }, + "GOOGLE_texture_basis": { + "source": 1 + } + } + } + ], + "extensionsUsed": [ + "GOOGLE_texture_basis", + "KHR_texture_basisu", + "MSFT_texture_dds", + "MGNM_fake_extension" + ] +} diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 5f41247ae..8b81f64e6 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -2133,35 +2133,54 @@ std::string TinyGltfImporter::doTextureName(const UnsignedInt id) { Containers::Optional TinyGltfImporter::doTexture(const UnsignedInt id) { const tinygltf::Texture& tex = _d->model.textures[id]; - /* Image ID. Try various extensions first. */ - UnsignedInt imageId; - - /* Basis textures. This extension is nonstandard and in case of embedded - images there's no standardized MIME type either. Fortunately we - don't care as we detect the file type based on magic, unfortunately we - *have to* use data:application/octet-stream there because TinyGLTF has a - whitelist for MIME types: - https://github.com/syoyo/tinygltf/blob/7e009041e35b999fd1e47c0f0e42cadcf8f5c31c/tiny_gltf.h#L2706 - This will all get solved once KTX2 materializes (but then it becomes - more complex as well). For reference: - https://github.com/BabylonJS/Babylon.js/issues/6636 - https://github.com/BinomialLLC/basis_universal/issues/52 */ - if(tex.extensions.find("GOOGLE_texture_basis") != tex.extensions.end()) { - /** @todo check for "extensionsRequired" as well? currently not doing - that, because I don't see why */ - tinygltf::Value basis = tex.extensions.at("GOOGLE_texture_basis"); - imageId = basis.Get("source").Get(); - - /* Image source */ - } else if(tex.source != -1) { - imageId = UnsignedInt(tex.source); - - /* Well. */ - } else { - Error{} << "Trade::TinyGltfImporter::texture(): no image source found"; - return Containers::NullOpt; + UnsignedInt imageId = ~0u; + + using namespace Containers::Literals; + + /* Various extensions, they override the standard image */ + constexpr Containers::StringView extensions[]{ + /* Allows the usage of mimeType image/ktx2 but only explicitly talks + about KTX2 with Basis compression. We don't care since we delegate + to AnyImageImporter and let it figure out the file type based on + magic. However, in case of embedded images we *have to* use + data:application/octet-stream there because TinyGLTF has a whitelist + for MIME types and the bundled version doesn't know image/ktx2: + https://github.com/syoyo/tinygltf/blob/7e009041e35b999fd1e47c0f0e42cadcf8f5c31c/tiny_gltf.h#L2706 */ + "KHR_texture_basisu"_s, + /* This is not a registered extension but can be found in some of the + early Basis Universal examples. Basis files don't have a registered + mimetype either, but as explained above we don't care about mimetype + at all. Same issue with embedded images applies because even if + there was a MIME type, TinyGLTF wouldn't know it. */ + "GOOGLE_texture_basis"_s + }; + + for(const auto& ext: extensions) { + const auto found = tex.extensions.find(ext); + if(found != tex.extensions.end()) { + int source = found->second.Get("source").Get(); + if(source < 0 || UnsignedInt(source) >= _d->model.images.size()) { + Error{} << "Trade::TinyGltfImporter::texture():" << ext << "image" << source << "out of bounds for" << _d->model.images.size() << "images"; + return Containers::NullOpt; + } + imageId = source; + break; + } } + if(imageId == ~0u) { + /* If not overwritten by an extension, use the standard 'source' + attribute. It's not mandatory, so this can still fail. */ + if(tex.source != -1) + imageId = UnsignedInt(tex.source); + else { + Error{} << "Trade::TinyGltfImporter::texture(): no image source found"; + return Containers::NullOpt; + } + } + + CORRADE_INTERNAL_ASSERT(imageId < _d->model.images.size()); + /* Sampler */ if(tex.sampler < 0) { /* The specification instructs to use "auto sampling", i.e. it is left diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h index b175df474..e1ed816ea 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h @@ -325,10 +325,14 @@ following defaults have been chosen for this importer: - Wrapping (all axes): @ref SamplerWrapping::Repeat
  • - The importer supports the non-standard `GOOGLE_texture_basis` extension - for referencing [Basis Universal](https://github.com/binomialLLC/basis_universal) - files, which then get loaded using @ref BasisImporter (or an equivalent - alias). The use is like this, [equivalently to Basis own glTF example](https://github.com/BinomialLLC/basis_universal/blob/1cae1d57266e2c95bc011b0bf1ccb9940988c184/webgl/gltf/assets/AgiHqSmall.gltf#L230-L240): + The importer supports the following extensions for image types not defined + in the [core glTF 2.0 specification](https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html#gltf-basics): + [KHR_texture_basisu](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_texture_basisu/README.md) + for Khronos Texture 2.0 images (`*.ktx2`) with [Basis Universal](https://github.com/binomialLLC/basis_universal) + supercompression and the original provisional `GOOGLE_texture_basis` + extension for referencing plain Basis Universal files (`*.basis`). There was + no formal specification of the extension but the use is like below, + [equivalently to Basis own glTF example](https://github.com/BinomialLLC/basis_universal/blob/1cae1d57266e2c95bc011b0bf1ccb9940988c184/webgl/gltf/assets/AgiHqSmall.gltf#L230-L240): @code{.json} { @@ -357,17 +361,18 @@ following defaults have been chosen for this importer: } @endcode - The MIME type is not standard either and the importer doesn't check its - value. However, in case of embedded data URIs, the prefix *has to* be set - to `data:application/octet-stream` as TinyGLTF has a whitelist for data URI - detection and would treat the URI as a filename otherwise: + While the `mimeType` field isn't checked by the importer, embedded data + URIs for both extensions *need to have* their prefix set to + `data:application/octet-stream`. TinyGLTF has a whitelist for data URI + detection that doesn't know `image/ktx2` or `image/x-basis` and would treat + the URI as a filename otherwise: @code{.json} { ... "images": [ { - "mimeType": "image/x-basis", + "mimeType": "image/ktx2", "uri": "data:application/octet-stream;base64,..." } ] From 26d50e5a45df7851ccc02632f8383ad6adc8a8c0 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:45:23 +0200 Subject: [PATCH 09/23] TinyGltfImporter: warn about non-contiguous numbered mesh attributes --- .../Test/TinyGltfImporterTest.cpp | 9 ++++ .../TinyGltfImporter/TinyGltfImporter.cpp | 46 ++++++++++++++++--- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 92f81dcd7..39e811af9 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -2331,10 +2331,19 @@ void TinyGltfImporterTest::meshUnorderedAttributes() { /* Custom attributes are sorted in alphabetical order */ CORRADE_VERIFY(customAttribute1 < customAttribute4); + std::ostringstream out; + Warning redirectWarning{&out}; + auto mesh = importer->mesh(0); CORRADE_VERIFY(mesh); CORRADE_COMPARE(mesh->attributeCount(), 7); + /* No warning about _CUSTOM_4 and _CUSTOM_1 */ + CORRADE_COMPARE(out.str(), + "Trade::TinyGltfImporter::mesh(): found attribute COLOR_3 but expected COLOR_0\n" + "Trade::TinyGltfImporter::mesh(): found attribute COLOR_9 but expected COLOR_4\n" + ); + /* Sets of the same attribute are imported in ascending set order. Checking the formats should be enough to test the import order. */ CORRADE_VERIFY(mesh->hasAttribute(MeshAttribute::TextureCoordinates)); diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 8b81f64e6..b95537082 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -174,6 +175,17 @@ Containers::StridedArrayView2D bufferView(const tinygltf::Model& mod {std::ptrdiff_t(stride), 1}}; } +Containers::StringView attributeSemantic(Containers::StringView attribute) { + /* Get the semantic base name ([semantic]_[set_index]) */ + const Containers::Array3 parts = attribute.partition('_'); + const bool isNumbered = !parts.back().isEmpty() && + std::all_of(parts.back().begin(), parts.back().end(), [](unsigned char c) { return std::isdigit(c); }); + /* Return empty semantic for invalid or non-numbered attribute names. This + ensures they don't get recognized as one of the numbered attributes like + TEXCOORD, etc. just because they share the prefix. */ + return isNumbered ? parts.front() : Containers::StringView{}; +} + } struct TinyGltfImporter::Document { @@ -456,9 +468,10 @@ void TinyGltfImporter::doOpenData(const Containers::ArrayView data) for(const tinygltf::Mesh& mesh: _d->model.meshes) { for(const tinygltf::Primitive& primitive: mesh.primitives) { for(const std::pair& attribute: primitive.attributes) { - if(Utility::String::beginsWith(attribute.first, "TEXCOORD_")) { + const Containers::StringView semantic = attributeSemantic(attribute.first); + if(semantic == "TEXCOORD") { if(!_d->textureCoordinateYFlipInMaterial) { - /* Ignore aaccessor is out of bounds, this will fail + /* Ignore accessor is out of bounds, this will fail later during mesh import */ if(std::size_t(attribute.second) >= _d->model.accessors.size()) continue; @@ -474,11 +487,11 @@ void TinyGltfImporter::doOpenData(const Containers::ArrayView data) } } - /* If the name isn't recognized, add the attribute to custom if - not there already */ + /* If the name isn't recognized or not in MeshAttribute, add + the attribute to custom if not there already */ } else if(attribute.first != "POSITION" && attribute.first != "NORMAL" && - !Utility::String::beginsWith(attribute.first, "COLOR_")) + semantic != "COLOR") { if(_d->meshAttributesForName.emplace(attribute.first, meshAttributeCustom(_d->meshAttributeNames.size())).second) @@ -1288,6 +1301,8 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un std::size_t bufferId; UnsignedInt vertexCount = 0; std::size_t attributeId = 0; + Containers::StringView lastAttributeSemantic; + Int lastAttributeIndex = -1; Math::Range1D bufferRange; Containers::Array attributeData{primitive.attributes.size()}; for(auto& attribute: primitive.attributes) { @@ -1295,6 +1310,23 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un if(!acessorPointer) return Containers::NullOpt; const tinygltf::Accessor& accessor = *acessorPointer; + const Containers::StringView semantic = attributeSemantic(attribute.first); + + /* Numbered attributes are expected to be contiguous (COLORS_0, + COLORS_1...). If not, print a warning, because in the MeshData they + will appear as contiguous. */ + if(!semantic.isEmpty()) { + if(semantic != lastAttributeSemantic) + lastAttributeIndex = -1; + + const Int index = std::atoi(attribute.first.c_str() + semantic.size() + 1); + if(index != lastAttributeIndex + 1) + Warning{} << "Trade::TinyGltfImporter::mesh(): found attribute" << attribute.first << "but expected" << semantic << Debug::nospace << "_" << Debug::nospace << lastAttributeIndex + 1; + + lastAttributeSemantic = semantic; + lastAttributeIndex = index; + } + /* Whitelist supported name and type combinations */ MeshAttribute name; if(attribute.first == "POSITION") { @@ -1352,7 +1384,7 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un } /* Texture coordinate attribute ends with _0, _1 ... */ - } else if(Utility::String::beginsWith(attribute.first, "TEXCOORD")) { + } else if(semantic == "TEXCOORD") { name = MeshAttribute::TextureCoordinates; if(accessor.type != TINYGLTF_TYPE_VEC2) { @@ -1373,7 +1405,7 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un } /* Color attribute ends with _0, _1 ... */ - } else if(Utility::String::beginsWith(attribute.first, "COLOR")) { + } else if(semantic == "COLOR") { name = MeshAttribute::Color; if(accessor.type != TINYGLTF_TYPE_VEC4 && accessor.type != TINYGLTF_TYPE_VEC3) { From 53c7fd23427a84f402b09479145d796780d9f122 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:46:46 +0200 Subject: [PATCH 10/23] TinyGltfImporter: check skin attribute types and mention their existence in the docs --- .../Test/TinyGltfImporterTest.cpp | 4 ++ .../TinyGltfImporter/Test/mesh-invalid.gltf | 40 +++++++++++++++++++ .../TinyGltfImporter/TinyGltfImporter.cpp | 35 ++++++++++++++++ .../TinyGltfImporter/TinyGltfImporter.h | 8 ++++ 4 files changed, 87 insertions(+) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 39e811af9..8b5643e0d 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -329,6 +329,10 @@ constexpr struct { {"unsupported texcoord component type", "mesh-invalid.gltf", "unsupported TEXCOORD component type normalized 5125"}, {"unexpected color type", "mesh-invalid.gltf", "unexpected COLOR type 2"}, {"unsupported color component type", "mesh-invalid.gltf", "unsupported COLOR component type unnormalized 5120"}, + {"unexpected joints type", "mesh-invalid.gltf", "unexpected JOINTS type 3"}, + {"unsupported joints component type", "mesh-invalid.gltf", "unsupported JOINTS component type unnormalized 5120"}, + {"unexpected weights type", "mesh-invalid.gltf", "unexpected WEIGHTS type 65"}, + {"unsupported weights component type", "mesh-invalid.gltf", "unsupported WEIGHTS component type unnormalized 5120"}, {"unexpected object id type", "mesh-invalid.gltf", "unexpected object ID type 2"}, {"unsupported object id component type", "mesh-invalid.gltf", "unsupported object ID component type unnormalized 5122"}, {"unexpected index type", "mesh-invalid.gltf", "unexpected index type 2"}, diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf index 21ae032ab..de8114b4c 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf @@ -112,6 +112,46 @@ } ] }, + { + "name": "unexpected joints type", + "primitives": [ + { + "attributes": { + "JOINTS_0": 0 + } + } + ] + }, + { + "name": "unsupported joints component type", + "primitives": [ + { + "attributes": { + "JOINTS_0": 5 + } + } + ] + }, + { + "name": "unexpected weights type", + "primitives": [ + { + "attributes": { + "WEIGHTS_0": 7 + } + } + ] + }, + { + "name": "unsupported weights component type", + "primitives": [ + { + "attributes": { + "WEIGHTS_0": 5 + } + } + ] + }, { "name": "unexpected object id type", "primitives": [ diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index b95537082..c3f57c059 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -1422,6 +1422,41 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un return Containers::NullOpt; } + /* Joint IDs attribute ends with _0, _1 ... */ + } else if(semantic == "JOINTS") { + name = _d->meshAttributesForName.at(attribute.first); + + if(accessor.type != TINYGLTF_TYPE_VEC4) { + Error{} << "Trade::TinyGltfImporter::mesh(): unexpected JOINTS type" << accessor.type; + return Containers::NullOpt; + } + + if(!(accessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_BYTE && !accessor.normalized) && + !(accessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_SHORT && !accessor.normalized)) { + Error{} << "Trade::TinyGltfImporter::mesh(): unsupported JOINTS component type" + << (accessor.normalized ? "normalized" : "unnormalized") + << accessor.componentType; + return Containers::NullOpt; + } + + /* Joint weights attribute ends with _0, _1 ... */ + } else if(semantic == "WEIGHTS") { + name = _d->meshAttributesForName.at(attribute.first); + + if(accessor.type != TINYGLTF_TYPE_VEC4) { + Error{} << "Trade::TinyGltfImporter::mesh(): unexpected WEIGHTS type" << accessor.type; + return Containers::NullOpt; + } + + if(!(accessor.componentType == TINYGLTF_COMPONENT_TYPE_FLOAT && !accessor.normalized) && + !(accessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_BYTE && accessor.normalized) && + !(accessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_SHORT && accessor.normalized)) { + Error{} << "Trade::TinyGltfImporter::mesh(): unsupported WEIGHTS component type" + << (accessor.normalized ? "normalized" : "unnormalized") + << accessor.componentType; + return Containers::NullOpt; + } + /* Object ID, name user-configurable */ } else if(attribute.first == configuration().value("objectIdAttribute")) { name = MeshAttribute::ObjectId; diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h index e1ed816ea..52a868092 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h @@ -236,6 +236,14 @@ Import of morph data is not supported at the moment. @ref VertexFormat::Vector4ubNormalized, @ref VertexFormat::Vector3usNormalized or @ref VertexFormat::Vector4usNormalized +- Joint IDs and weights for skinning are imported as custom vertex attributes + named "JOINTS_0", "JOINTS_1", etc. and "WEIGHTS_0", "WEIGHTS_1", etc. + Their mapping to/from a string can be queried using + @ref meshAttributeName() and @ref meshAttributeForName(). Joint IDs are + imported as @ref VertexFormat::Vector4ub or @ref VertexFormat::Vector4us. + Joint weights are imported as @ref VertexFormat::Vector4, + @ref VertexFormat::Vector4ubNormalized or + @ref VertexFormat::Vector4usNormalized. - Per-vertex object ID attribute is imported as either @ref VertexFormat::UnsignedInt, @ref VertexFormat::UnsignedShort or @ref VertexFormat::UnsignedByte. By default `_OBJECT_ID` is the recognized From 4bf726788886edcbd1b7944f4c82e4286640af4c Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:48:26 +0200 Subject: [PATCH 11/23] TinyGltfImporter: test skin attribute import --- .../TinyGltfImporter/Test/CMakeLists.txt | 2 + .../Test/TinyGltfImporterTest.cpp | 70 ++++++++++++++++++ .../Test/mesh-skin-attributes.bin | Bin 0 -> 144 bytes .../Test/mesh-skin-attributes.bin.in | 9 +++ .../Test/mesh-skin-attributes.gltf | 70 ++++++++++++++++++ 5 files changed, 151 insertions(+) create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.bin create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.bin.in create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt index 24f2e443d..0b6ec1529 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt @@ -149,6 +149,8 @@ corrade_add_test(TinyGltfImporterTest mesh-multiple-primitives.gltf mesh-primitives-types.gltf mesh-primitives-types.bin + mesh-skin-attributes.gltf + mesh-skin-attributes.bin mesh-unordered-attributes.gltf přívodní-šňůra.gltf přívodní-šňůra.bin diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 8b5643e0d..d620fc7ab 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -123,6 +123,7 @@ struct TinyGltfImporterTest: TestSuite::Tester { void meshIndexed(); void meshIndexedAttributeless(); void meshColors(); + void meshSkinAttributes(); void meshCustomAttributes(); void meshCustomAttributesNoFileOpened(); void meshDuplicateAttributes(); @@ -606,6 +607,7 @@ TinyGltfImporterTest::TinyGltfImporterTest() { &TinyGltfImporterTest::meshIndexed, &TinyGltfImporterTest::meshIndexedAttributeless, &TinyGltfImporterTest::meshColors, + &TinyGltfImporterTest::meshSkinAttributes, &TinyGltfImporterTest::meshCustomAttributes, &TinyGltfImporterTest::meshCustomAttributesNoFileOpened, &TinyGltfImporterTest::meshDuplicateAttributes, @@ -2188,6 +2190,74 @@ void TinyGltfImporterTest::meshColors() { }), TestSuite::Compare::Container); } +void TinyGltfImporterTest::meshSkinAttributes() { + Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); + CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, + "mesh-skin-attributes.gltf"))); + + /* The mapping should be available even before the mesh is imported */ + const MeshAttribute joints0Attribute = importer->meshAttributeForName("JOINTS_0"); + CORRADE_COMPARE(joints0Attribute, meshAttributeCustom(0)); + const MeshAttribute joints1Attribute = importer->meshAttributeForName("JOINTS_1"); + CORRADE_COMPARE(joints1Attribute, meshAttributeCustom(1)); + const MeshAttribute weights0Attribute = importer->meshAttributeForName("WEIGHTS_0"); + CORRADE_COMPARE(weights0Attribute, meshAttributeCustom(2)); + const MeshAttribute weights1Attribute = importer->meshAttributeForName("WEIGHTS_1"); + CORRADE_COMPARE(weights1Attribute, meshAttributeCustom(3)); + + /* One attribute for each set, not one for all sets */ + CORRADE_COMPARE(importer->meshAttributeForName("JOINTS"), MeshAttribute{}); + CORRADE_COMPARE(importer->meshAttributeForName("WEIGHTS"), MeshAttribute{}); + + CORRADE_COMPARE(importer->meshCount(), 1); + + auto mesh = importer->mesh(0); + CORRADE_VERIFY(mesh); + CORRADE_VERIFY(!mesh->isIndexed()); + + CORRADE_COMPARE(mesh->attributeCount(), 5); + CORRADE_COMPARE(mesh->attributeFormat(MeshAttribute::Position), VertexFormat::Vector3); + CORRADE_COMPARE_AS(mesh->attribute(MeshAttribute::Position), + Containers::arrayView({ + {1.5f, -1.0f, -0.5f}, + {-0.5f, 2.5f, 0.75f}, + {-2.0f, 1.0f, 0.3f} + }), TestSuite::Compare::Container); + + CORRADE_COMPARE(mesh->attributeCount(joints0Attribute), 1); + CORRADE_COMPARE(mesh->attributeFormat(joints0Attribute), VertexFormat::Vector4ub); + CORRADE_COMPARE_AS(mesh->attribute(joints0Attribute), + Containers::arrayView({ + {1, 2, 3, 4}, + {5, 6, 7, 8}, + {9, 10, 11, 12} + }), TestSuite::Compare::Container); + CORRADE_COMPARE(mesh->attributeCount(joints1Attribute), 1); + CORRADE_COMPARE(mesh->attributeFormat(joints1Attribute), VertexFormat::Vector4us); + CORRADE_COMPARE_AS(mesh->attribute(joints1Attribute), + Containers::arrayView({ + {13, 14, 15, 16}, + {17, 18, 19, 20}, + {21, 22, 23, 24} + }), TestSuite::Compare::Container); + CORRADE_COMPARE(mesh->attributeCount(weights0Attribute), 1); + CORRADE_COMPARE(mesh->attributeFormat(weights0Attribute), VertexFormat::Vector4); + CORRADE_COMPARE_AS(mesh->attribute(weights0Attribute), + Containers::arrayView({ + {0.125f, 0.25f, 0.375f, 0.0f}, + {0.1f, 0.05f, 0.05f, 0.05f}, + {0.2f, 0.0f, 0.3f, 0.0f} + }), TestSuite::Compare::Container); + CORRADE_COMPARE(mesh->attributeCount(weights1Attribute), 1); + CORRADE_COMPARE(mesh->attributeFormat(weights1Attribute), VertexFormat::Vector4usNormalized); + CORRADE_COMPARE_AS(mesh->attribute(weights1Attribute), + Containers::arrayView({ + { 0, 0xffff/8, 0, 0xffff/8}, + {0xffff/2, 0xffff/8, 0xffff/16, 0xffff/16}, + { 0, 0xffff/4, 0xffff/4, 0} + }), TestSuite::Compare::Container); +} + void TinyGltfImporterTest::meshCustomAttributes() { Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.bin b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.bin new file mode 100644 index 0000000000000000000000000000000000000000..327a6ba3e849fe5f3f72ea10398a961e8b4139ec GIT binary patch literal 144 zcmX|*+YN$16hx1LiuhPTLI1E3Sqh*hZ3CNO3s`Ep4w^X0`M8t0lY!vjH3H*enWP7f zI8Hig<%=L@S@Q#B?;Ui0wcca}C1);7F+Qf)|FNev=FI6asK>vDq21=YeMQZcoAUj) MclMQ$%$t^c0B!go@Bjb+ literal 0 HcmV?d00001 diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.bin.in b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.bin.in new file mode 100644 index 000000000..7b86e8b5b --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.bin.in @@ -0,0 +1,9 @@ +type = '<3f4B4H4f4H 3f4B4H4f4H 3f4B4H4f4H' +input = [ + # positions, # joints 0, # joints 1 # weights 0 # weights 1 (normalized ushort) + 1.5, -1.0, -0.5, 1, 2, 3, 4, 13, 14, 15, 16, 0.125, 0.25, 0.375, 0.0, 0, 0xffff//8, 0, 0xffff//8, + -0.5, 2.5, 0.75, 5, 6, 7, 8, 17, 18, 19, 20, 0.1, 0.05, 0.05, 0.05, 0xffff//2, 0xffff//8, 0xffff//16, 0xffff//16, + -2.0, 1.0, 0.3, 9, 10, 11, 12, 21, 22, 23, 24, 0.2, 0.0, 0.3, 0.0, 0, 0xffff//4, 0xffff//4, 0 +] + +# kate: hl python diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.gltf new file mode 100644 index 000000000..078a86fca --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-skin-attributes.gltf @@ -0,0 +1,70 @@ +{ + "asset": { + "version": "2.0" + }, + "accessors": [ + { + "bufferView": 0, + "componentType": 5126, + "count": 3, + "type": "VEC3" + }, + { + "bufferView": 0, + "byteOffset": 12, + "componentType": 5121, + "count": 3, + "type": "VEC4" + }, + { + "bufferView": 0, + "byteOffset": 16, + "componentType": 5123, + "count": 3, + "type": "VEC4" + }, + { + "bufferView": 0, + "byteOffset": 24, + "componentType": 5126, + "count": 3, + "type": "VEC4" + }, + { + "bufferView": 0, + "byteOffset": 40, + "componentType": 5123, + "normalized": true, + "count": 3, + "type": "VEC4" + } + ], + "bufferViews": [ + { + "buffer": 0, + "byteLength": 144, + "byteStride": 48 + } + ], + "buffers": [ + { + "byteLength": 144, + "uri": "mesh-skin-attributes.bin" + } + ], + "meshes": [ + { + "primitives": [ + { + "attributes": { + "JOINTS_0": 1, + "JOINTS_1": 2, + "WEIGHTS_0": 3, + "WEIGHTS_1": 4, + "POSITION": 0 + } + } + ] + } + ] +} From 18ab4dc7f092916cc69a1d0b2b42ddc9d92e2b99 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:49:27 +0200 Subject: [PATCH 12/23] TinyGltfImporter: don't import tangents as custom attributes --- .../TinyGltfImporter/Test/TinyGltfImporterTest.cpp | 5 +++++ src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp | 1 + 2 files changed, 6 insertions(+) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index d620fc7ab..509fb3e5d 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -2039,6 +2039,10 @@ void TinyGltfImporterTest::mesh() { CORRADE_COMPARE(importer->meshName(0), "Non-indexed mesh"); CORRADE_COMPARE(importer->meshForName("Non-indexed mesh"), 0); + /* _OBJECT_ID is the only custom attribute */ + CORRADE_COMPARE(importer->meshAttributeName(meshAttributeCustom(0)), "_OBJECT_ID"); + CORRADE_COMPARE(importer->meshAttributeName(meshAttributeCustom(1)), ""); + auto mesh = importer->mesh(0); CORRADE_VERIFY(mesh); CORRADE_VERIFY(mesh->importerState()); @@ -2160,6 +2164,7 @@ void TinyGltfImporterTest::meshColors() { "mesh-colors.gltf"))); CORRADE_COMPARE(importer->meshCount(), 1); + CORRADE_COMPARE(importer->meshAttributeName(meshAttributeCustom(0)), ""); auto mesh = importer->mesh(0); CORRADE_VERIFY(mesh); diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index c3f57c059..88145065f 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -491,6 +491,7 @@ void TinyGltfImporter::doOpenData(const Containers::ArrayView data) the attribute to custom if not there already */ } else if(attribute.first != "POSITION" && attribute.first != "NORMAL" && + attribute.first != "TANGENT" && semantic != "COLOR") { if(_d->meshAttributesForName.emplace(attribute.first, From 12ec21ae4826f00caa113ec21a7e637bfad2f6ec Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:50:05 +0200 Subject: [PATCH 13/23] TinyGltfImporter: add missing asset object to glTF test files --- .../Test/image-basis-embedded.glb | Bin 820 -> 844 bytes .../Test/image-basis-embedded.gltf | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/image-basis-embedded.glb b/src/MagnumPlugins/TinyGltfImporter/Test/image-basis-embedded.glb index e4c5965517d717bfc2f9e2a4eaa22394fa30f22f..98fb53a4886bae54434e9758897a2248c8e44549 100644 GIT binary patch delta 56 zcmdnOc7{zqJtxGCiGhK^hna!F0*JkW{r#$y5{rvdOO&jtmC90!iZk=`l&qAD^bC}0 KbvBC1G6MjOkq&AA delta 33 ncmX@ZwuMbJJtxGCiGhK^gqeXs0f@bV{r##p>dP`SDJTE{cw+`) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/image-basis-embedded.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/image-basis-embedded.gltf index e8488471b..8bdc6b89b 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/image-basis-embedded.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/image-basis-embedded.gltf @@ -1 +1 @@ -{"textures":[{"extensions":{"GOOGLE_texture_basis":{"source":1}}}],"images":[{"mimeType":"image/png","uri":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAADCAYAAABbNsX4AAAACXBIWXMAAAsTAAALEwEAmpwYAAAASElEQVQIHQ3JOwqAMBBAwWfwgyIpJJWQ+/fBxhO5iHUQ42Z12mnStlvfOcZpoDzKW5S2qiKXYGbEuBLCgrtzRqvh/fwHHHLyAZ75HS8LboO+AAAAAElFTkSuQmCC"},{"mimeType":"image/x-basis","uri":"data:application/octet-stream;base64,c0ITAE0AtAyTAAAA0MECAAABAAAAAwAAAAAAAAAAAAAAAAAAAAAAAwB7AAAAKwAAAwCmAAAADQAAswAAACsAAABNAAAAAAAAAAAAAAAAAAAAAAUAAwACAAEA3gAAAAEAAABbegAAAAEAAgABAAEAAQDfAAAAAQAAAOCuAcAEAAAAAAAAwgCIABAAAAAAOQeACQABAAAAAIQIwBQAAQAAAACShR+4AFT9V1WNFvb3p6KiogIAwUQAAAAAAADyX20AmAAAAAAAAEFGAEwAEAAAAIBAcQAwAQAAAAAAgAACagw="}],"extensionsUsed":["GOOGLE_texture_basis"],"extensionsRequired":["GOOGLE_texture_basis"]} \ No newline at end of file +{"asset":{"version":"2.0"},"textures":[{"extensions":{"GOOGLE_texture_basis":{"source":1}}}],"images":[{"mimeType":"image/png","uri":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAADCAYAAABbNsX4AAAACXBIWXMAAAsTAAALEwEAmpwYAAAASElEQVQIHQ3JOwqAMBBAwWfwgyIpJJWQ+/fBxhO5iHUQ42Z12mnStlvfOcZpoDzKW5S2qiKXYGbEuBLCgrtzRqvh/fwHHHLyAZ75HS8LboO+AAAAAElFTkSuQmCC"},{"mimeType":"image/x-basis","uri":"data:application/octet-stream;base64,c0ITAE0AtAyTAAAA0MECAAABAAAAAwAAAAAAAAAAAAAAAAAAAAAAAwB7AAAAKwAAAwCmAAAADQAAswAAACsAAABNAAAAAAAAAAAAAAAAAAAAAAUAAwACAAEA3gAAAAEAAABbegAAAAEAAgABAAEAAQDfAAAAAQAAAOCuAcAEAAAAAAAAwgCIABAAAAAAOQeACQABAAAAAIQIwBQAAQAAAACShR+4AFT9V1WNFvb3p6KiogIAwUQAAAAAAADyX20AmAAAAAAAAEFGAEwAEAAAAIBAcQAwAQAAAAAAgAACagw="}],"extensionsUsed":["GOOGLE_texture_basis"],"extensionsRequired":["GOOGLE_texture_basis"]} \ No newline at end of file From b95e7a2112389422871d6aedc7ae4a27259d6296 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 13:50:36 +0200 Subject: [PATCH 14/23] TinyGltfImporter: cleanup --- .../TinyGltfImporter/Test/TinyGltfImporterTest.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 509fb3e5d..0ffb126a5 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -183,12 +183,11 @@ struct TinyGltfImporterTest: TestSuite::Tester { constexpr struct { const char* name; - const char* suffix; Containers::ArrayView shortData; const char* shortDataError; } OpenErrorData[]{ - {"ascii", ".gltf", {"?", 1}, "JSON string too short.\n"}, - {"binary", ".glb", {"glTF?", 5}, "Too short data size for glTF Binary.\n"} + {"ascii", {"?", 1}, "JSON string too short."}, + {"binary", {"glTF?", 5}, "Too short data size for glTF Binary."} }; constexpr struct { @@ -726,7 +725,7 @@ void TinyGltfImporterTest::openError() { Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); CORRADE_VERIFY(!importer->openData(data.shortData)); - CORRADE_COMPARE(out.str(), "Trade::TinyGltfImporter::openData(): error opening file: " + std::string{data.shortDataError}); + CORRADE_COMPARE(out.str(), Utility::formatString("Trade::TinyGltfImporter::openData(): error opening file: {}\n", data.shortDataError)); } void TinyGltfImporterTest::openExternalDataNotFound() { From c7e5375314f2b091d23129d18a29a869513d6fc3 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 14:38:40 +0200 Subject: [PATCH 15/23] TinyGltfImporter: test more invalid accessors The accessor checks for these were previously untested --- .../Test/TinyGltfImporterTest.cpp | 8 ++- .../Test/animation-invalid.gltf | 52 +++++++++++++++++++ .../TinyGltfImporter/Test/mesh-invalid.gltf | 16 ++++++ .../TinyGltfImporter/Test/skin-invalid.gltf | 12 +++++ 4 files changed, 86 insertions(+), 2 deletions(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 0ffb126a5..471f39d4b 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -219,6 +219,8 @@ constexpr struct { {"unexpected rotation type", "animation-invalid.gltf", "rotation track has unexpected type 65/5126"}, {"unexpected scaling type", "animation-invalid.gltf", "scaling track has unexpected type 4/5126"}, {"unsupported path", "animation-invalid.gltf", "unsupported track target color"}, + {"invalid input accessor", "animation-invalid.gltf", "accessor 3 needs 40 bytes but bufferView 0 has only 0"}, + {"invalid output accessor", "animation-invalid.gltf", "accessor 4 needs 120 bytes but bufferView 0 has only 0"}, {"sampler index out of bounds", "animation-invalid-sampler-oob.gltf", "sampler 1 out of bounds for 1 samplers"}, {"node index out of bounds", "animation-invalid-node-oob.gltf", "target node 2 out of bounds for 2 nodes"}, {"sampler input accessor index out of bounds", "animation-invalid-input-accessor-oob.gltf", "accessor 2 out of bounds for 2 accessors"}, @@ -249,7 +251,8 @@ constexpr struct { {"accessor out of bounds", "skin-invalid-accessor-oob.gltf", "accessor 1 out of bounds for 1 accessors"}, {"wrong accessor type", "skin-invalid.gltf", "inverse bind matrices have unexpected type 35/5126"}, {"wrong accessor component type", "skin-invalid.gltf", "inverse bind matrices have unexpected type 36/5123"}, - {"wrong accessor count", "skin-invalid.gltf", "invalid inverse bind matrix count, expected 2 but got 3"} + {"wrong accessor count", "skin-invalid.gltf", "invalid inverse bind matrix count, expected 2 but got 3"}, + {"invalid accessor", "skin-invalid.gltf", "accessor 3 needs 196 bytes but bufferView 0 has only 192"} }; constexpr struct { @@ -350,7 +353,8 @@ constexpr struct { {"buffer index out of bounds", "mesh-invalid-buffer-oob.gltf", "buffer 1 out of bounds for 1 buffers"}, {"buffer view index out of bounds", "mesh-invalid-bufferview-oob.gltf", "bufferView 4 out of bounds for 1 views"}, {"accessor index out of bounds", "mesh-invalid-accessor-oob.gltf", "accessor 2 out of bounds for 2 accessors"}, - {"multiple buffers", "mesh-invalid.gltf", "meshes spanning multiple buffers are not supported"} + {"multiple buffers", "mesh-invalid.gltf", "meshes spanning multiple buffers are not supported"}, + {"invalid index accessor", "mesh-invalid.gltf", "accessor 17 needs 40 bytes but bufferView 0 has only 36"}, }; constexpr struct { diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/animation-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/animation-invalid.gltf index 6f4efecbe..67c45be9e 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/animation-invalid.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/animation-invalid.gltf @@ -119,6 +119,44 @@ "output": 0 } ] + }, + { + "name": "invalid input accessor", + "channels": [ + { + "sampler": 0, + "target": { + "node": 0, + "path": "translation" + } + } + ], + "samplers": [ + { + "input": 3, + "interpolation": "STEP", + "output": 2 + } + ] + }, + { + "name": "invalid output accessor", + "channels": [ + { + "sampler": 0, + "target": { + "node": 0, + "path": "translation" + } + } + ], + "samplers": [ + { + "input": 1, + "interpolation": "STEP", + "output": 4 + } + ] } ], "note": "additional accessor bounds checks are tested inside mesh-invalid.gltf", @@ -143,6 +181,20 @@ "componentType": 5126, "count": 0, "type": "VEC3" + }, + { + "bufferView": 0, + "byteOffset": 0, + "componentType": 5126, + "count": 10, + "type": "SCALAR" + }, + { + "bufferView": 0, + "byteOffset": 0, + "componentType": 5126, + "count": 10, + "type": "VEC3" } ], "bufferViews": [ diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf index de8114b4c..49d67dff8 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf @@ -278,6 +278,15 @@ } } ] + }, + { + "name": "invalid index accessor", + "primitives": [ + { + "attributes": {}, + "indices": 17 + } + ] } ], "accessors": [ @@ -414,6 +423,13 @@ "componentType": 5126, "count": 3, "type": "VEC2" + }, + { + "name": "17", + "bufferView": 0, + "componentType": 5125, + "count": 4, + "type": "SCALAR" } ], "bufferViews": [ diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/skin-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/skin-invalid.gltf index 5c0847a4b..66b1ade96 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/skin-invalid.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/skin-invalid.gltf @@ -26,6 +26,11 @@ "name": "wrong accessor count", "inverseBindMatrices": 2, "joints": [0, 1] + }, + { + "name": "invalid accessor", + "inverseBindMatrices": 3, + "joints": [0, 1] } ], "accessors": [ @@ -49,6 +54,13 @@ "componentType": 5126, "count": 3, "type": "MAT4" + }, + { + "bufferView": 0, + "byteOffset": 68, + "componentType": 5126, + "count": 2, + "type": "MAT4" } ], "bufferViews": [ From d86ee94d2ce22d269977aed18ccbc3179705e89a Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 14:59:56 +0200 Subject: [PATCH 16/23] TinyGltfImporter: move non-OOB tests back to mesh-invalid.gltf Originally moved out for CgltfImporter, but it doesn't perform these tests in openData() anymore, so they don't need to be in separate files --- .../TinyGltfImporter/Test/CMakeLists.txt | 3 -- .../Test/TinyGltfImporterTest.cpp | 6 +-- .../Test/mesh-invalid-accessor-short.gltf | 38 ------------- .../Test/mesh-invalid-bufferview-short.gltf | 37 ------------- ...h-invalid-mismatching-attribute-count.gltf | 53 ------------------- .../TinyGltfImporter/Test/mesh-invalid.gltf | 49 +++++++++++++++++ 6 files changed, 52 insertions(+), 134 deletions(-) delete mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-accessor-short.gltf delete mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-bufferview-short.gltf delete mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-mismatching-attribute-count.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt index 0b6ec1529..63e0eda14 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt @@ -141,11 +141,8 @@ corrade_add_test(TinyGltfImporterTest mesh-invalid.bin mesh-invalid.gltf mesh-invalid-accessor-oob.gltf - mesh-invalid-accessor-short.gltf mesh-invalid-buffer-oob.gltf mesh-invalid-bufferview-oob.gltf - mesh-invalid-bufferview-short.gltf - mesh-invalid-mismatching-attribute-count.gltf mesh-multiple-primitives.gltf mesh-primitives-types.gltf mesh-primitives-types.bin diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 471f39d4b..33326b3d3 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -321,7 +321,7 @@ constexpr struct { const char* message; } MeshInvalidData[]{ {"invalid primitive", "mesh-invalid.gltf", "unrecognized primitive 666"}, - {"different vertex count for each accessor", "mesh-invalid-mismatching-attribute-count.gltf", "mismatched vertex count for attribute TEXCOORD_0, expected 3 but got 4"}, + {"different vertex count for each accessor", "mesh-invalid.gltf", "mismatched vertex count for attribute TEXCOORD_0, expected 3 but got 4"}, {"unexpected position type", "mesh-invalid.gltf", "unexpected POSITION type 2"}, {"unsupported position component type", "mesh-invalid.gltf", "unsupported POSITION component type unnormalized 5125"}, {"unexpected normal type", "mesh-invalid.gltf", "unexpected NORMAL type 2"}, @@ -348,8 +348,8 @@ constexpr struct { {"non-normalized byte matrix", "mesh-invalid.gltf", "unsupported matrix component type unnormalized 5120"}, {"sparse accessor", "mesh-invalid.gltf", "accessor 14 is using sparse storage, which is unsupported"}, {"no bufferview", "mesh-invalid.gltf", "accessor 15 has no bufferView"}, - {"accessor count larger than buffer size", "mesh-invalid-accessor-short.gltf", "accessor 0 needs 33 bytes but bufferView 0 has only 32"}, - {"buffer view range out of bounds", "mesh-invalid-bufferview-short.gltf", "bufferView 0 needs 72 bytes but buffer 0 has only 68"}, + {"accessor range out of bounds", "mesh-invalid.gltf", "accessor 18 needs 48 bytes but bufferView 0 has only 36"}, + {"buffer view range out of bounds", "mesh-invalid.gltf", "bufferView 3 needs 164 bytes but buffer 1 has only 160"}, {"buffer index out of bounds", "mesh-invalid-buffer-oob.gltf", "buffer 1 out of bounds for 1 buffers"}, {"buffer view index out of bounds", "mesh-invalid-bufferview-oob.gltf", "bufferView 4 out of bounds for 1 views"}, {"accessor index out of bounds", "mesh-invalid-accessor-oob.gltf", "accessor 2 out of bounds for 2 accessors"}, diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-accessor-short.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-accessor-short.gltf deleted file mode 100644 index 78d14d145..000000000 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-accessor-short.gltf +++ /dev/null @@ -1,38 +0,0 @@ -{ - "asset": { - "version": "2.0" - }, - "meshes": [ - { - "name": "accessor count larger than buffer size", - "primitives": [ - { - "attributes": {}, - "indices": 0 - } - ] - } - ], - "accessors": [ - { - "bufferView": 0, - "byteOffset": 1, - "componentType": 5125, - "count": 8, - "type": "SCALAR" - } - ], - "bufferViews": [ - { - "buffer": 0, - "byteOffset": 36, - "byteLength": 32 - } - ], - "buffers": [ - { - "byteLength": 68, - "uri": "mesh-invalid.bin" - } - ] -} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-bufferview-short.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-bufferview-short.gltf deleted file mode 100644 index 76afff8f8..000000000 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-bufferview-short.gltf +++ /dev/null @@ -1,37 +0,0 @@ -{ - "asset": { - "version": "2.0" - }, - "meshes": [ - { - "name": "buffer view range out of bounds", - "primitives": [ - { - "attributes": {}, - "indices": 0 - } - ] - } - ], - "accessors": [ - { - "bufferView": 0, - "componentType": 5125, - "count": 3, - "type": "SCALAR" - } - ], - "bufferViews": [ - { - "buffer": 0, - "byteOffset": 60, - "byteLength": 12 - } - ], - "buffers": [ - { - "byteLength": 68, - "uri": "mesh-invalid.bin" - } - ] -} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-mismatching-attribute-count.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-mismatching-attribute-count.gltf deleted file mode 100644 index 2db9841b3..000000000 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid-mismatching-attribute-count.gltf +++ /dev/null @@ -1,53 +0,0 @@ -{ - "asset": { - "version": "2.0" - }, - "meshes": [ - { - "name": "different vertex count for each accessor", - "primitives": [ - { - "attributes": { - "POSITION": 0, - "TEXCOORD_0": 1 - } - } - ] - } - ], - "accessors": [ - { - "name": "0", - "bufferView": 0, - "componentType": 5126, - "count": 3, - "type": "VEC3" - }, - { - "name": "1", - "bufferView": 1, - "componentType": 5126, - "count": 4, - "type": "VEC2" - } - ], - "bufferViews": [ - { - "buffer": 0, - "byteOffset": 0, - "byteLength": 36, - "byteStride": 12 - }, - { - "buffer": 0, - "byteOffset": 36, - "byteLength": 32 - } - ], - "buffers": [ - { - "byteLength": 68, - "uri": "mesh-invalid.bin" - } - ] -} diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf index 49d67dff8..75e003dda 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf @@ -12,6 +12,17 @@ } ] }, + { + "name": "different vertex count for each accessor", + "primitives": [ + { + "attributes": { + "POSITION": 0, + "TEXCOORD_0": 1 + } + } + ] + }, { "name": "unexpected position type", "primitives": [ @@ -287,6 +298,25 @@ "indices": 17 } ] + }, + { + "name": "accessor range out of bounds", + "primitives": [ + { + "attributes": { + "POSITION": 18 + } + } + ] + }, + { + "name": "buffer view range out of bounds", + "primitives": [ + { + "attributes": {}, + "indices": 19 + } + ] } ], "accessors": [ @@ -430,6 +460,20 @@ "componentType": 5125, "count": 4, "type": "SCALAR" + }, + { + "name": "18", + "bufferView": 0, + "componentType": 5126, + "count": 4, + "type": "VEC3" + }, + { + "name": "19", + "bufferView": 3, + "componentType": 5126, + "count": 3, + "type": "VEC3" } ], "bufferViews": [ @@ -448,6 +492,11 @@ "buffer": 1, "byteOffset": 124, "byteLength": 24 + }, + { + "buffer": 1, + "byteOffset": 124, + "byteLength": 40 } ], "buffers": [ From 9c4711f3f5bdd2388ce9de70d2cd9d6809be3526 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 16:45:40 +0200 Subject: [PATCH 17/23] TinyGltfImporter: fix compilation with MSVC 2015 and 2017 std::isdigit requires , apparently MSVC 2019 transitively included it somewhere already --- src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 88145065f..9a062886a 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -28,6 +28,7 @@ #include "TinyGltfImporter.h" #include +#include #include #include #include From ea6db96350a91ed995468530552cc0c6349ae462 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 18:20:07 +0200 Subject: [PATCH 18/23] TinyGltfImporter: check for out-of-bounds image in texture() --- .../TinyGltfImporter/Test/CMakeLists.txt | 1 + .../Test/TinyGltfImporterTest.cpp | 15 +++++++------- .../Test/texture-invalid-image-oob.gltf | 20 +++++++++++++++++++ .../TinyGltfImporter/TinyGltfImporter.cpp | 13 ++++++++---- 4 files changed, 38 insertions(+), 11 deletions(-) create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-image-oob.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt index 63e0eda14..0a18e1f14 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt @@ -196,6 +196,7 @@ corrade_add_test(TinyGltfImporterTest texture-extensions-invalid.gltf texture-extensions-invalid-basisu-oob.gltf texture-invalid.gltf + texture-invalid-image-oob.gltf texture-missing-source.gltf version-legacy.gltf version-supported.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 33326b3d3..1048ea52c 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -446,12 +446,14 @@ constexpr struct { constexpr struct { const char* name; + const char* file; const char* message; } TextureInvalidData[]{ - {"invalid sampler minFilter", "invalid minFilter 1"}, - {"invalid sampler magFilter", "invalid magFilter 2"}, - {"invalid sampler wrapS", "invalid wrap mode 3"}, - {"invalid sampler wrapT", "invalid wrap mode 4"} + {"invalid sampler minFilter", "texture-invalid.gltf", "invalid minFilter 1"}, + {"invalid sampler magFilter", "texture-invalid.gltf", "invalid magFilter 2"}, + {"invalid sampler wrapS", "texture-invalid.gltf", "invalid wrap mode 3"}, + {"invalid sampler wrapT", "texture-invalid.gltf", "invalid wrap mode 4"}, + {"image out of bounds", "texture-invalid-image-oob.gltf", "image 3 out of bounds for 1 images"} }; constexpr struct { @@ -3658,11 +3660,10 @@ void TinyGltfImporterTest::textureInvalid() { setTestCaseDescription(data.name); Containers::Pointer importer = _manager.instantiate("TinyGltfImporter"); - CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, - "texture-invalid.gltf"))); + CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TINYGLTFIMPORTER_TEST_DIR, data.file))); /* Check we didn't forget to test anything */ - CORRADE_COMPARE(importer->textureCount(), Containers::arraySize(TextureInvalidData)); + CORRADE_VERIFY(Containers::arraySize(TextureInvalidData) >= importer->textureCount()); std::ostringstream out; Error redirectError{&out}; diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-image-oob.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-image-oob.gltf new file mode 100644 index 000000000..e80071584 --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-image-oob.gltf @@ -0,0 +1,20 @@ +{ + "asset" : { + "version" : "2.0" + }, + "images" : [ + { + "uri" : "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP4z/D/PwAG/gL+0yubzgAAAABJRU5ErkJggg==" + } + ], + "samplers" : [ + {} + ], + "textures" : [ + { + "name": "image out of bounds", + "sampler" : 0, + "source" : 3 + } + ] +} diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 9a062886a..438b749fb 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -2227,8 +2227,8 @@ Containers::Optional TinyGltfImporter::doTexture(const UnsignedInt for(const auto& ext: extensions) { const auto found = tex.extensions.find(ext); if(found != tex.extensions.end()) { - int source = found->second.Get("source").Get(); - if(source < 0 || UnsignedInt(source) >= _d->model.images.size()) { + const int source = found->second.Get("source").Get(); + if(UnsignedInt(source) >= _d->model.images.size()) { Error{} << "Trade::TinyGltfImporter::texture():" << ext << "image" << source << "out of bounds for" << _d->model.images.size() << "images"; return Containers::NullOpt; } @@ -2240,9 +2240,14 @@ Containers::Optional TinyGltfImporter::doTexture(const UnsignedInt if(imageId == ~0u) { /* If not overwritten by an extension, use the standard 'source' attribute. It's not mandatory, so this can still fail. */ - if(tex.source != -1) + if(tex.source != -1) { + if(UnsignedInt(tex.source) >= _d->model.images.size()) { + Error{} << "Trade::TinyGltfImporter::texture(): image" << tex.source << "out of bounds for" << _d->model.images.size() << "images"; + return Containers::NullOpt; + } + imageId = UnsignedInt(tex.source); - else { + } else { Error{} << "Trade::TinyGltfImporter::texture(): no image source found"; return Containers::NullOpt; } From 25b809d37b581209c83f167558320ebbff396505 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 20:00:29 +0200 Subject: [PATCH 19/23] TinyGltfImporter: check for out-of-bounds sampler in texture() --- .../TinyGltfImporter/Test/CMakeLists.txt | 1 + .../Test/TinyGltfImporterTest.cpp | 3 ++- .../Test/texture-invalid-sampler-oob.gltf | 26 +++++++++++++++++++ .../TinyGltfImporter/TinyGltfImporter.cpp | 8 +++++- 4 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-sampler-oob.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt index 0a18e1f14..ed176a707 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/TinyGltfImporter/Test/CMakeLists.txt @@ -197,6 +197,7 @@ corrade_add_test(TinyGltfImporterTest texture-extensions-invalid-basisu-oob.gltf texture-invalid.gltf texture-invalid-image-oob.gltf + texture-invalid-sampler-oob.gltf texture-missing-source.gltf version-legacy.gltf version-supported.gltf diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 1048ea52c..2c3bf66fa 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -453,7 +453,8 @@ constexpr struct { {"invalid sampler magFilter", "texture-invalid.gltf", "invalid magFilter 2"}, {"invalid sampler wrapS", "texture-invalid.gltf", "invalid wrap mode 3"}, {"invalid sampler wrapT", "texture-invalid.gltf", "invalid wrap mode 4"}, - {"image out of bounds", "texture-invalid-image-oob.gltf", "image 3 out of bounds for 1 images"} + {"sampler out of bounds", "texture-invalid-sampler-oob.gltf", "sampler 1 out of bounds for 1 samplers"}, + {"image out of bounds", "texture-invalid-image-oob.gltf", "image 3 out of bounds for 1 images"}, }; constexpr struct { diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-sampler-oob.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-sampler-oob.gltf new file mode 100644 index 000000000..8a0279a39 --- /dev/null +++ b/src/MagnumPlugins/TinyGltfImporter/Test/texture-invalid-sampler-oob.gltf @@ -0,0 +1,26 @@ +{ + "asset" : { + "generator" : "Khronos Blender glTF 2.0 exporter", + "version" : "2.0" + }, + "images" : [ + { + "uri" : "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP4z/D/PwAG/gL+0yubzgAAAABJRU5ErkJggg==" + } + ], + "samplers" : [ + { + "wrapS": 33648, + "wrapT": 33071, + "minFilter": 9984, + "magFilter": 9728 + } + ], + "textures" : [ + { + "name": "sampler out of bounds", + "sampler": 1, + "source": 0 + } + ] +} diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 438b749fb..bfd996b12 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -2256,12 +2256,18 @@ Containers::Optional TinyGltfImporter::doTexture(const UnsignedInt CORRADE_INTERNAL_ASSERT(imageId < _d->model.images.size()); /* Sampler */ - if(tex.sampler < 0) { + if(tex.sampler == -1) { /* The specification instructs to use "auto sampling", i.e. it is left to the implementor to decide on the default values... */ return TextureData{TextureType::Texture2D, SamplerFilter::Linear, SamplerFilter::Linear, SamplerMipmap::Linear, {SamplerWrapping::Repeat, SamplerWrapping::Repeat, SamplerWrapping::Repeat}, imageId, &tex}; } + + if(UnsignedInt(tex.sampler) >= _d->model.samplers.size()) { + Error{} << "Trade::TinyGltfImporter::texture(): sampler" << tex.sampler << "out of bounds for" << _d->model.samplers.size() << "samplers"; + return Containers::NullOpt; + } + const tinygltf::Sampler& s = _d->model.samplers[tex.sampler]; SamplerFilter minFilter; From f74b5c783a5ce53956325c52006cb86fa486e260 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Fri, 8 Oct 2021 21:52:16 +0200 Subject: [PATCH 20/23] TinyGltfImporter: check for normalized int mesh attributes Not allowed, and used to lead to an assert in vertexFormat() --- .../Test/TinyGltfImporterTest.cpp | 7 ++++--- .../TinyGltfImporter/Test/mesh-invalid.gltf | 19 ++++++++++++++++++- .../TinyGltfImporter/TinyGltfImporter.cpp | 10 +++++++--- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp index 2c3bf66fa..0918b1941 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/Test/TinyGltfImporterTest.cpp @@ -329,7 +329,7 @@ constexpr struct { {"unexpected tangent type", "mesh-invalid.gltf", "unexpected TANGENT type 3"}, {"unsupported tangent component type", "mesh-invalid.gltf", "unsupported TANGENT component type unnormalized 5120"}, {"unexpected texcoord type", "mesh-invalid.gltf", "unexpected TEXCOORD type 3"}, - {"unsupported texcoord component type", "mesh-invalid.gltf", "unsupported TEXCOORD component type normalized 5125"}, + {"unsupported texcoord component type", "mesh-invalid.gltf", "unsupported TEXCOORD component type unnormalized 5125"}, {"unexpected color type", "mesh-invalid.gltf", "unexpected COLOR type 2"}, {"unsupported color component type", "mesh-invalid.gltf", "unsupported COLOR component type unnormalized 5120"}, {"unexpected joints type", "mesh-invalid.gltf", "unexpected JOINTS type 3"}, @@ -343,8 +343,9 @@ constexpr struct { {"normalized index type", "mesh-invalid.gltf", "index type can't be normalized"}, {"strided index view", "mesh-invalid.gltf", "index bufferView is not contiguous"}, {"accessor type size larger than buffer stride", "mesh-invalid.gltf", "16-byte type defined by accessor 10 can't fit into bufferView 0 stride of 12"}, - {"normalized float", "mesh-invalid.gltf", "floating-point component types can't be normalized"}, - {"normalized double", "mesh-invalid.gltf", "floating-point component types can't be normalized"}, + {"normalized float", "mesh-invalid.gltf", "component type 5126 can't be normalized"}, + {"normalized double", "mesh-invalid.gltf", "component type 5130 can't be normalized"}, + {"normalized int", "mesh-invalid.gltf", "component type 5125 can't be normalized"}, {"non-normalized byte matrix", "mesh-invalid.gltf", "unsupported matrix component type unnormalized 5120"}, {"sparse accessor", "mesh-invalid.gltf", "accessor 14 is using sparse storage, which is unsupported"}, {"no bufferview", "mesh-invalid.gltf", "accessor 15 has no bufferView"}, diff --git a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf index 75e003dda..445763374 100644 --- a/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf +++ b/src/MagnumPlugins/TinyGltfImporter/Test/mesh-invalid.gltf @@ -317,6 +317,16 @@ "indices": 19 } ] + }, + { + "name": "normalized int", + "primitives": [ + { + "attributes": { + "_THING": 20 + } + } + ] } ], "accessors": [ @@ -352,7 +362,6 @@ "name": "4", "bufferView": 0, "componentType": 5125, - "normalized": true, "count": 3, "type": "VEC2" }, @@ -474,6 +483,14 @@ "componentType": 5126, "count": 3, "type": "VEC3" + }, + { + "name": "20", + "bufferView": 0, + "componentType": 5125, + "normalized": true, + "count": 1, + "type": "SCALAR" } ], "bufferViews": [ diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index bfd996b12..2c1ece0d5 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -1522,9 +1522,13 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un vectorCount = 4; } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ - /* Floats should not be normalized */ - if(accessor.normalized && (componentFormat == VertexFormat::Float || componentFormat == VertexFormat::Double)) { - Error{} << "Trade::TinyGltfImporter::mesh(): floating-point component types can't be normalized"; + /* Check for illegal normalized types */ + if(accessor.normalized && + componentFormat != VertexFormat::Byte && + componentFormat != VertexFormat::UnsignedByte && + componentFormat != VertexFormat::Short && + componentFormat != VertexFormat::UnsignedShort) { + Error{} << "Trade::TinyGltfImporter::mesh(): component type" << accessor.componentType << "can't be normalized"; return Containers::NullOpt; } From 3aac997b3fe4956f9ba1219ff7e0b845fbb5c0f1 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Mon, 11 Oct 2021 15:06:31 +0200 Subject: [PATCH 21/23] TinyGltfImporter: doc++ --- src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h index 52a868092..745d31e41 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h @@ -122,7 +122,7 @@ See @ref building-plugins, @ref cmake-plugins, @ref plugins and @section Trade-TinyGltfImporter-behavior Behavior and limitations The plugin supports @ref ImporterFeature::OpenData and -@ref ImporterFeature::FileCallback features. The `TinyGLTF` library loads +@ref ImporterFeature::FileCallback features. The TinyGLTF library loads everything during initial import, meaning all external file loading callbacks are called with @ref InputFileCallbackPolicy::LoadTemporary and the resources can be safely freed right after the @ref openData() / @ref openFile() function From deffad9154605b0c9bc41a17c8c661fd66b159e5 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Mon, 11 Oct 2021 15:12:09 +0200 Subject: [PATCH 22/23] TinyGltfImporter: prefer strtol over atoi It's used in other places already, be consistent and allow easier search for integer parsing --- src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index 2c1ece0d5..c7ed14939 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -1321,7 +1321,7 @@ Containers::Optional TinyGltfImporter::doMesh(const UnsignedInt id, Un if(semantic != lastAttributeSemantic) lastAttributeIndex = -1; - const Int index = std::atoi(attribute.first.c_str() + semantic.size() + 1); + const Int index = std::strtol(attribute.first.c_str() + semantic.size() + 1, nullptr, 10); if(index != lastAttributeIndex + 1) Warning{} << "Trade::TinyGltfImporter::mesh(): found attribute" << attribute.first << "but expected" << semantic << Debug::nospace << "_" << Debug::nospace << lastAttributeIndex + 1; From 2bdc698217a05e0f15124b9aeda957fb97e70c21 Mon Sep 17 00:00:00 2001 From: Pablo Escobar Date: Mon, 11 Oct 2021 17:04:17 +0200 Subject: [PATCH 23/23] TinyGltfImporter: we don't need unordered_map here --- .../TinyGltfImporter/TinyGltfImporter.cpp | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp index c7ed14939..57633a039 100644 --- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp +++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp @@ -379,21 +379,31 @@ void TinyGltfImporter::doOpenData(const Containers::ArrayView data) /* Check node hierarchy for forbidden parent-child relationships and non-root scene nodes */ - std::unordered_map parentFor; + Containers::Array parentFor{DirectInit, _d->model.nodes.size(), -1}; for(std::size_t i = 0; i != _d->model.nodes.size(); ++i) { for(Int child: _d->model.nodes[i].children) { - const auto inserted = parentFor.emplace(child, i); - if(!inserted.second) { + /* Ignore out-of-bounds child nodes, those produce an error later + in doObject3D() */ + if(UnsignedInt(child) >= _d->model.nodes.size()) + continue; + + if(parentFor[child] != -1) { Error{} << "Trade::TinyGltfImporter::openData(): node" << child << "has multiple parents"; doClose(); return; } + parentFor[child] = i; } } for(std::size_t i = 0; i != _d->model.scenes.size(); ++i) { for(Int node: _d->model.scenes[i].nodes) { - if(parentFor.count(node)) { + /* Ignore out-of-bounds scene nodes, those produce an error later + in doScene() */ + if(UnsignedInt(node) >= _d->model.nodes.size()) + continue; + + if(parentFor[node] != -1) { Error{} << "Trade::TinyGltfImporter::openData(): node" << node << "in scene" << i << "is not a root node"; doClose(); return; @@ -403,10 +413,7 @@ void TinyGltfImporter::doOpenData(const Containers::ArrayView data) for(std::size_t i = 0; i != _d->model.nodes.size(); ++i) { auto getParent = [&](Int node) -> Int { - const auto it = parentFor.find(node); - if(it == parentFor.end()) - return -1; - return it->second; + return node != -1 ? parentFor[node] : -1; }; Int p1 = getParent(i);