diff --git a/src/Magnum/SceneTools/Test/CMakeLists.txt b/src/Magnum/SceneTools/Test/CMakeLists.txt index e55538ff3..d146389fc 100644 --- a/src/Magnum/SceneTools/Test/CMakeLists.txt +++ b/src/Magnum/SceneTools/Test/CMakeLists.txt @@ -79,6 +79,10 @@ corrade_add_test(SceneToolsSceneConverterTest SceneConverterTest.cpp SceneConverterTestFiles/quad-duplicates-fuzzy.obj SceneConverterTestFiles/quad-duplicates.obj SceneConverterTestFiles/quad-duplicates.ply + SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin + SceneConverterTestFiles/quad-name-custom-attributes-duplicates.gltf + SceneConverterTestFiles/quad-name-custom-attributes.bin + SceneConverterTestFiles/quad-name-custom-attributes.gltf SceneConverterTestFiles/quad-normals-texcoords.obj SceneConverterTestFiles/quad-strip.bin SceneConverterTestFiles/quad-strip.gltf diff --git a/src/Magnum/SceneTools/Test/SceneConverterTest.cpp b/src/Magnum/SceneTools/Test/SceneConverterTest.cpp index f6a990388..d1ab0aa1f 100644 --- a/src/Magnum/SceneTools/Test/SceneConverterTest.cpp +++ b/src/Magnum/SceneTools/Test/SceneConverterTest.cpp @@ -390,6 +390,30 @@ const struct { " 65536 -> 65536 covered pixels\n" " overdraw 1 -> 1\n" "Trade::AnySceneConverter::beginFile(): using StanfordSceneConverter\n"}, + {"implicit custom-processed mesh with a name and custom attributes", Containers::array({ + /* Removing the generator identifier to have the file closer to the + original */ + "--remove-duplicate-vertices", "-c", "generator=", + Utility::Path::join(SCENETOOLS_TEST_DIR, "SceneConverterTestFiles/quad-name-custom-attributes-duplicates.gltf"), Utility::Path::join(SCENETOOLS_TEST_OUTPUT_DIR, "SceneConverterTestFiles/quad-name-custom-attributes.gltf")}), + "GltfImporter", "GltfSceneConverter", + /* The output should be mostly the same, except that there's now only 4 + vertices instead of 6. The code that adds meshes manually instead of + using addSupportedImporterContents() should take care of propagating + mesh names and custom attributes as well. */ + "quad-name-custom-attributes.gltf", "quad-name-custom-attributes.bin", + {}}, + {"selected custom-processed mesh with a name and custom attributes", Containers::array({ + /* Removing the generator identifier to have the file closer to the + original */ + "--mesh", "0", "--remove-duplicate-vertices", "-c", "generator=", + Utility::Path::join(SCENETOOLS_TEST_DIR, "SceneConverterTestFiles/quad-name-custom-attributes-duplicates.gltf"), Utility::Path::join(SCENETOOLS_TEST_OUTPUT_DIR, "SceneConverterTestFiles/quad-name-custom-attributes.gltf")}), + "GltfImporter", "GltfSceneConverter", + /* The output should be mostly the same, except that there's now only 4 + vertices instead of 6. The code that adds meshes manually instead of + using addSupportedImporterContents() should take care of propagating + mesh names and custom attributes as well. */ + "quad-name-custom-attributes.gltf", "quad-name-custom-attributes.bin", + {}}, }; const struct { diff --git a/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin new file mode 100644 index 000000000..2928d6caf Binary files /dev/null and b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin differ diff --git a/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin.in b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin.in new file mode 100644 index 000000000..ae2d60e37 --- /dev/null +++ b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin.in @@ -0,0 +1,19 @@ +type = "6H 3f3f3f 3f3f3f" +input = [ + # 2 3--5 + # |\ \ | + # | \ \| + # 0--1 4 + + 0, 1, 2, 3, 4, 5, + + -1, -1, 0, + 1, -1, 0, + -1, 1, 0, + + -1, 1, 0, + 1, -1, 0, + 1, 1, 0, +] + +# kate: hl python diff --git a/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.gltf b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.gltf new file mode 100644 index 000000000..d7283187c --- /dev/null +++ b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.gltf @@ -0,0 +1,50 @@ +{ + "asset": { + "version": "2.0" + }, + "buffers": [ + { + "uri": "quad-name-custom-attributes-duplicates.bin", + "byteLength": 72 + } + ], + "bufferViews": [ + { + "buffer": 0, + "byteOffset": 0, + "byteLength": 12 + }, + { + "buffer": 0, + "byteOffset": 12, + "byteLength": 72 + } + ], + "accessors": [ + { + "bufferView": 0, + "componentType": 5123, + "count": 6, + "type": "SCALAR" + }, + { + "bufferView": 1, + "componentType": 5126, + "count": 6, + "type": "VEC3" + } + ], + "meshes": [ + { + "primitives": [ + { + "indices": 0, + "attributes": { + "_CUSTOM_ATTRIBUTE": 1 + } + } + ], + "name": "A mesh where POSITION is _CUSTOM_ATTRIBUTE for some reason" + } + ] +} diff --git a/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.bin b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.bin new file mode 100644 index 000000000..280f198f4 Binary files /dev/null and b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.bin differ diff --git a/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.gltf b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.gltf new file mode 100644 index 000000000..d25e01a0d --- /dev/null +++ b/src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.gltf @@ -0,0 +1,50 @@ +{ + "asset": { + "version": "2.0" + }, + "buffers": [ + { + "uri": "quad-name-custom-attributes.bin", + "byteLength": 60 + } + ], + "bufferViews": [ + { + "buffer": 0, + "byteOffset": 0, + "byteLength": 12 + }, + { + "buffer": 0, + "byteOffset": 12, + "byteLength": 48 + } + ], + "accessors": [ + { + "bufferView": 0, + "componentType": 5123, + "count": 6, + "type": "SCALAR" + }, + { + "bufferView": 1, + "componentType": 5126, + "count": 4, + "type": "VEC3" + } + ], + "meshes": [ + { + "primitives": [ + { + "indices": 0, + "attributes": { + "_CUSTOM_ATTRIBUTE": 1 + } + } + ], + "name": "A mesh where POSITION is _CUSTOM_ATTRIBUTE for some reason" + } + ] +} diff --git a/src/Magnum/SceneTools/sceneconverter.cpp b/src/Magnum/SceneTools/sceneconverter.cpp index d9bca5572..195af75c2 100644 --- a/src/Magnum/SceneTools/sceneconverter.cpp +++ b/src/Magnum/SceneTools/sceneconverter.cpp @@ -513,25 +513,59 @@ the first mesh.)") vertexCount}; } - /* Create an importer instance that contains just the single mesh for - further step, without any other data. Simpler than having to - special-case the single-mesh case in all following steps. */ + /* Create an importer instance that contains just the single mesh and + related metadata for further steps, without anything else */ + /** @todo might be useful to have this split out of the file and tested + directly if the complexity grows even further */ struct SingleMeshImporter: Trade::AbstractImporter { - explicit SingleMeshImporter(Trade::MeshData&& mesh): mesh{std::move(mesh)} {} + explicit SingleMeshImporter(Trade::MeshData&& mesh_, Containers::String&& name, Trade::AbstractImporter& original): mesh{std::move(mesh_)}, name{std::move(name)} { + for(UnsignedInt i = 0; i != mesh.attributeCount(); ++i) { + const Trade::MeshAttribute name = mesh.attributeName(i); + if(!isMeshAttributeCustom(name)) continue; + /* Appending even empty ones so we don't have to + special-case "not found" in doMeshAttributeName() */ + arrayAppend(attributeNames, InPlaceInit, meshAttributeCustom(name), original.meshAttributeName(name)); + } + } Trade::ImporterFeatures doFeatures() const override { return {}; } /* LCOV_EXCL_LINE */ bool doIsOpened() const override { return true; } void doClose() override {} /* LCOV_EXCL_LINE */ UnsignedInt doMeshCount() const override { return 1; } + Containers::String doMeshName(UnsignedInt) override { + return name; + } + Containers::String doMeshAttributeName(UnsignedShort name) override { + for(const Containers::Pair& i: attributeNames) + if(i.first() == name) return i.second(); + /* All custom attributes, including the unnamed, are in the + attributeNames array and both our attribute name propagation + here and addSupportedImporterContents() call + meshAttributeName() only with attributes present in the + actual mesh, so this should never be reached */ + CORRADE_INTERNAL_ASSERT_UNREACHABLE(); + } Containers::Optional doMesh(UnsignedInt, UnsignedInt) override { return MeshTools::reference(mesh); } Trade::MeshData mesh; + Containers::String name; + Containers::Array> attributeNames; }; - importer.emplace(*std::move(mesh)); + /* Save the previous importer so we can pass it to the constructor in + emplace(), otherwise the constructor would access a deleted pointer */ + /** @todo would it make sense for emplace() to first construct and only + then delete so I wouldn't need to juggle the previous value + manually? */ + Containers::Pointer previousImporter = std::move(importer); + importer.emplace(*std::move(mesh), + /* Propagate the name only in case of a single mesh, for + concatenation it wouldn't make sense */ + args.value("mesh") ? previousImporter->meshName(args.value("mesh")) : Containers::String{}, + *previousImporter); } /* Operations to perform on all meshes in the importer. If there are any, @@ -658,7 +692,28 @@ the first mesh.)") Warning{} << "Ignoring" << meshes.size() << "meshes not supported by the converter"; } else for(UnsignedInt i = 0; i != meshes.size(); ++i) { Trade::Implementation::Duration d{conversionTime}; - if(!converter->add(meshes[i])) { + + const Trade::MeshData& mesh = meshes[i]; + + /* Propagate custom attribute names, skip ones that are empty. + Compared to data names this is done always to avoid + information loss. */ + for(UnsignedInt j = 0; j != mesh.attributeCount(); ++j) { + /** @todo have some kind of a map to not have to query the + same custom attribute again for each mesh */ + const Trade::MeshAttribute name = mesh.attributeName(j); + if(!isMeshAttributeCustom(name)) continue; + /* The expectation here is that the meshes are coming from + the importer instance. If --mesh or --concatenate-meshes + was used, the original importer is replaced a new one + containing just one mesh, so in that case it works + too. */ + if(const Containers::String nameString = importer->meshAttributeName(name)) { + converter->setMeshAttributeName(name, nameString); + } + } + + if(!converter->add(mesh, contents & Trade::SceneContent::Names ? importer->meshName(i) : Containers::String{})) { Error{} << "Cannot add mesh" << i; return 1; }