From 7711ed7dfec3beded9f08ff1a8fb912e597f6dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 7 Oct 2022 16:51:31 +0200 Subject: [PATCH] Don't warn about plugin options added to empty subgroups. This was an annoyance with GltfImporter's customSceneFieldTypes, where it just wasn't possible to set the options through AnySceneImporter or magnum-sceneconverter -i argument without causing a warning to be printed. The behavior is now that if a plugin configuration subgroup (but not the root group) is empty, it's assumed that new values are meant to be added to it, and thus it doesn't warn on them. --- .../Implementation/converterUtilities.h | 23 ++++++++++-- src/Magnum/Test/ConverterUtilitiesTest.cpp | 35 +++++++++++++++++++ .../Test/AnySceneImporterTest.cpp | 28 +++++++++++++++ .../AnySceneImporter/Test/CMakeLists.txt | 1 + .../AnySceneImporter/Test/empty.gltf | 5 +++ .../Implementation/propagateConfiguration.h | 24 ++++++++++--- 6 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 src/MagnumPlugins/AnySceneImporter/Test/empty.gltf diff --git a/src/Magnum/Implementation/converterUtilities.h b/src/Magnum/Implementation/converterUtilities.h index 70aed6747..43408e93b 100644 --- a/src/Magnum/Implementation/converterUtilities.h +++ b/src/Magnum/Implementation/converterUtilities.h @@ -25,6 +25,7 @@ DEALINGS IN THE SOFTWARE. */ +#include #include #include #include @@ -38,6 +39,7 @@ namespace Magnum { namespace Implementation { namespace { void setOptions(PluginManager::AbstractPlugin& plugin, const Containers::StringView anyPluginName, const Containers::StringView options) { + std::unordered_set emptySubgroups; for(const Containers::StringView option: options.splitWithoutEmptyParts(',')) { auto keyValue = option.partition('='); keyValue[0] = keyValue[0].trimmed(); @@ -52,8 +54,19 @@ void setOptions(PluginManager::AbstractPlugin& plugin, const Containers::StringV if(!subgroup) { groupNotRecognized = true; subgroup = group->addGroup(keyParts[i]); + /* For existing subgroups (i.e., not the root configuration) + remember if the group was initially empty (no subgroups, no + values; comments can be there). For those we won't warn about + unrecognized options below as it's a common use case (for + example GltfImporter's customSceneFieldTypes). Has to be done + upfront in case more than one option is added to the same group + -- otherwise adding the second would warn again, as the group + is no longer empty at that point. */ + } else if(!subgroup->hasGroups() && !subgroup->hasValues()) { + emptySubgroups.insert(subgroup); } group = subgroup; + } /* Provide a warning message in case the plugin doesn't define given @@ -62,9 +75,15 @@ void setOptions(PluginManager::AbstractPlugin& plugin, const Containers::StringV not an error. If it's an Any* plugin, then this check is provided by it directly, - and since the Any* plugin obviously don't expose the options of the concrete plugins, this warning would fire for them always, which + and since the Any* plugin obviously don't expose the options of the + concrete plugins, this warning would fire for them always, which wouldn't help anything. */ - if((groupNotRecognized || !group->hasValue(keyParts.back())) && plugin.plugin() != anyPluginName) { + if((groupNotRecognized || (!group->hasValue(keyParts.back()) && + /* The warning isn't printed in case a value is added into an + empty subgroup, see above */ + emptySubgroups.find(group) == emptySubgroups.end() + )) && plugin.plugin() != anyPluginName) + { Warning{} << "Option" << keyValue[0] << "not recognized by" << plugin.plugin(); } diff --git a/src/Magnum/Test/ConverterUtilitiesTest.cpp b/src/Magnum/Test/ConverterUtilitiesTest.cpp index 25cc96dd8..633b2961a 100644 --- a/src/Magnum/Test/ConverterUtilitiesTest.cpp +++ b/src/Magnum/Test/ConverterUtilitiesTest.cpp @@ -150,6 +150,41 @@ option=value )", /* The trailing space is there because the plugin name is empty */ "Option group/notFound/nested/option not recognized by \n"}, + /* This should not warn for emptyGroup, since it's a common use case (for + example GltfImporter's customSceneFields). It should also remember that + the group was initially empty to not warn again when more options are + subsequently added. OTOH, for subgroups added to an empty group it + still warns. */ + {"unrecognized option in empty config subgroup", R"([configuration] +option= +[configuration/emptyGroup] +# No values originally here +[configuration/nonEmptyGroup] +option= +)", + "emptyGroup/notFound=value,nonEmptyGroup/notFound=value,emptyGroup/another,emptyGroup/subgroup/notFound=value", "AnyPlugin", R"([configuration] +option= +[configuration/emptyGroup] +# No values originally here +notFound=value +another=true +[configuration/emptyGroup/subgroup] +notFound=value +[configuration/nonEmptyGroup] +option= +notFound=value +)", + /* The trailing space is there because the plugin name is empty */ + "Option nonEmptyGroup/notFound not recognized by \n" + "Option emptyGroup/subgroup/notFound not recognized by \n"}, + /* OTOH this should warn, as it's an option in the root configuration */ + {"unrecognized option in empty root config", R"([configuration] +)", + "notFound=value", "AnyPlugin", R"([configuration] +notFound=value +)", + /* The trailing space is there because the plugin name is empty */ + "Option notFound not recognized by \n"}, }; ConverterUtilitiesTest::ConverterUtilitiesTest() { diff --git a/src/MagnumPlugins/AnySceneImporter/Test/AnySceneImporterTest.cpp b/src/MagnumPlugins/AnySceneImporter/Test/AnySceneImporterTest.cpp index 56d79fe6a..d0077c77c 100644 --- a/src/MagnumPlugins/AnySceneImporter/Test/AnySceneImporterTest.cpp +++ b/src/MagnumPlugins/AnySceneImporter/Test/AnySceneImporterTest.cpp @@ -63,6 +63,7 @@ struct AnySceneImporterTest: TestSuite::Tester { void propagateFlags(); void propagateConfiguration(); void propagateConfigurationUnknown(); + void propagateConfigurationUnknownInEmptySubgroup(); void propagateFileCallback(); void sceneFieldName(); @@ -115,6 +116,7 @@ AnySceneImporterTest::AnySceneImporterTest() { &AnySceneImporterTest::propagateFlags, &AnySceneImporterTest::propagateConfiguration, &AnySceneImporterTest::propagateConfigurationUnknown, + &AnySceneImporterTest::propagateConfigurationUnknownInEmptySubgroup, &AnySceneImporterTest::propagateFileCallback, &AnySceneImporterTest::sceneFieldName, @@ -293,6 +295,32 @@ void AnySceneImporterTest::propagateConfigurationUnknown() { "Trade::AnySceneImporter::openFile(): option postprocess/feh/noHereNotEither not recognized by AssimpImporter\n"); } +void AnySceneImporterTest::propagateConfigurationUnknownInEmptySubgroup() { + PluginManager::Manager manager{MAGNUM_PLUGINS_IMPORTER_INSTALL_DIR}; + #ifdef ANYSCENEIMPORTER_PLUGIN_FILENAME + CORRADE_VERIFY(manager.load(ANYSCENEIMPORTER_PLUGIN_FILENAME) & PluginManager::LoadState::Loaded); + #endif + + if(manager.load("GltfImporter") < PluginManager::LoadState::Loaded) + CORRADE_SKIP("GltfImporter plugin can't be loaded."); + + Containers::Pointer importer = manager.instantiate("AnySceneImporter"); + importer->configuration().addGroup("customSceneFieldTypes"); + importer->configuration().group("customSceneFieldTypes")->setValue("field", "Float"); + importer->configuration().group("customSceneFieldTypes")->setValue("another", "Int"); + importer->configuration().group("customSceneFieldTypes")->addGroup("notFound")->setValue("noHereNotEither", false); + + std::ostringstream out; + Warning redirectWarning{&out}; + CORRADE_VERIFY(importer->openFile(Utility::Path::join(ANYSCENEIMPORTER_TEST_DIR, "empty.gltf"))); + CORRADE_COMPARE(out.str(), + /* Should not warn for values added to the empty customSceneFieldTypes + group, but should warn if a subgroup is added there. This is + consistent with how the magnum-*converter -i / -c options are + handled in Magnum/Implementation/converterUtilities.h. */ + "Trade::AnySceneImporter::openFile(): option customSceneFieldTypes/notFound/noHereNotEither not recognized by GltfImporter\n"); +} + void AnySceneImporterTest::propagateFileCallback() { if(!(_manager.loadState("ObjImporter") & PluginManager::LoadState::Loaded)) CORRADE_SKIP("ObjImporter plugin not enabled, cannot test"); diff --git a/src/MagnumPlugins/AnySceneImporter/Test/CMakeLists.txt b/src/MagnumPlugins/AnySceneImporter/Test/CMakeLists.txt index 48a5ce9aa..6db15a903 100644 --- a/src/MagnumPlugins/AnySceneImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/AnySceneImporter/Test/CMakeLists.txt @@ -52,6 +52,7 @@ corrade_add_test(AnySceneImporterTest AnySceneImporterTest.cpp LIBRARIES MagnumTrade FILES ${PROJECT_SOURCE_DIR}/src/MagnumPlugins/ObjImporter/Test/mesh-primitive-points.obj + empty.gltf mesh-custom-attribute.gltf scene-custom-field.gltf triangle.ply) diff --git a/src/MagnumPlugins/AnySceneImporter/Test/empty.gltf b/src/MagnumPlugins/AnySceneImporter/Test/empty.gltf new file mode 100644 index 000000000..8b778ccfb --- /dev/null +++ b/src/MagnumPlugins/AnySceneImporter/Test/empty.gltf @@ -0,0 +1,5 @@ +{ + "asset": { + "version": "2.0" + } +} diff --git a/src/MagnumPlugins/Implementation/propagateConfiguration.h b/src/MagnumPlugins/Implementation/propagateConfiguration.h index 201638dd1..a0cbf01b4 100644 --- a/src/MagnumPlugins/Implementation/propagateConfiguration.h +++ b/src/MagnumPlugins/Implementation/propagateConfiguration.h @@ -35,19 +35,21 @@ implementation. Assumes that the Any* plugin itself doesn't have any configuration options and so propagates all groups and values that were set, emitting a warning if the target doesn't have such option in its - default configuration. */ + default configuration. + + Thoroughly tested in AnySceneImporterTest. */ namespace Magnum { namespace Implementation { /* Used only in plugins where we don't want it to be exported */ namespace { -void propagateConfiguration(const char* warningPrefix, const Containers::String& groupPrefix, const Containers::StringView plugin, const Utility::ConfigurationGroup& src, Utility::ConfigurationGroup& dst) { +void propagateConfiguration(const char* warningPrefix, const Containers::String& groupPrefix, const Containers::StringView plugin, const Utility::ConfigurationGroup& src, Utility::ConfigurationGroup& dst, bool warnUnrecognized = true) { using namespace Containers::Literals; /* Propagate values */ for(Containers::Pair value: src.values()) { - if(!dst.hasValue(value.first())) { + if(!dst.hasValue(value.first()) && warnUnrecognized) { Warning{} << warningPrefix << "option" << "/"_s.joinWithoutEmptyParts({groupPrefix, value.first()}) << "not recognized by" << plugin; } @@ -57,8 +59,20 @@ void propagateConfiguration(const char* warningPrefix, const Containers::String& /* Recursively propagate groups */ for(Containers::Pair> group: src.groups()) { Utility::ConfigurationGroup* dstGroup = dst.group(group.first()); - if(!dstGroup) dstGroup = dst.addGroup(group.first()); - propagateConfiguration(warningPrefix, "/"_s.joinWithoutEmptyParts({groupPrefix, group.first()}), plugin, group.second(), *dstGroup); + + /* If the group exists but is empty (such as GltfImporter's + customSceneFieldTypes), don't warn about unrecognized values. This + logic is repeated for nested subgroups instead of being inherited, + meaning that adding a nonexistent subgroup to an empty group will + warn again. */ + bool warnUnrecognizedSubgroup = true; + if(!dstGroup) { + dstGroup = dst.addGroup(group.first()); + } else if(!dstGroup->hasGroups() && !dstGroup->hasValues()) { + warnUnrecognizedSubgroup = false; + } + + propagateConfiguration(warningPrefix, "/"_s.joinWithoutEmptyParts({groupPrefix, group.first()}), plugin, group.second(), *dstGroup, warnUnrecognizedSubgroup); } }