From 1925da0a7397581049b1b57e22f07162bd26b4a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 20 Sep 2022 18:50:54 +0200 Subject: [PATCH] sceneconverter: propagate names and custom attributes of processed meshes. It's done by addSupportedImporterContents() only if they aren't processed explicitly and passed directly. --- src/Magnum/SceneTools/Test/CMakeLists.txt | 4 ++ .../SceneTools/Test/SceneConverterTest.cpp | 24 +++++++ ...quad-name-custom-attributes-duplicates.bin | Bin 0 -> 84 bytes ...d-name-custom-attributes-duplicates.bin.in | 19 +++++ ...uad-name-custom-attributes-duplicates.gltf | 50 +++++++++++++ .../quad-name-custom-attributes.bin | Bin 0 -> 60 bytes .../quad-name-custom-attributes.gltf | 50 +++++++++++++ src/Magnum/SceneTools/sceneconverter.cpp | 67 ++++++++++++++++-- 8 files changed, 208 insertions(+), 6 deletions(-) create mode 100644 src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin create mode 100644 src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.bin.in create mode 100644 src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes-duplicates.gltf create mode 100644 src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.bin create mode 100644 src/Magnum/SceneTools/Test/SceneConverterTestFiles/quad-name-custom-attributes.gltf 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 0000000000000000000000000000000000000000..2928d6caf07b4efae8b0e7dcfaf237d3bc311b2f GIT binary patch literal 84 scmZQzU}RuoU}j)pU}a!nXxI& 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; }