Skip to content

Commit

Permalink
Propagate configuration options in Any* plugins.
Browse files Browse the repository at this point in the history
Minor but very important convenience feature, especially useful when
dealing with command-line apps. This now works:

   magnum-imageconverter a.png a.jpg -c jpegQuality=0.75

The AnyImageConverter gets the jpegQuality option and then
automatically propagates it to the concrete plugin (which is either
JpegImageConverter or StbImageConverter), possibly warning in case the
target plugin doesn't recognize given option (i.e., doesn't list it in
its default configuration). Previously the user had to always specify a
concrete converter implementation using -C, which was rather annoying
and nonintuitive.
  • Loading branch information
mosra committed Jun 24, 2021
1 parent 4d61cda commit e6d673c
Show file tree
Hide file tree
Showing 34 changed files with 863 additions and 30 deletions.
10 changes: 10 additions & 0 deletions doc/changelog.dox
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,16 @@ See also:
@subsubsection changelog-latest-changes-trade Trade library

- Recognizing TIFF file header magic in @ref Trade::AnyImageImporter "AnyImageImporter"
- @ref Audio::AnyImporter "AnyAudioImporter",
@relativeref{Trade,AnyImageImporter}, @relativeref{Trade,AnyImageConverter},
@relativeref{Trade,AnySceneImporter}, @relativeref{Trade,AnySceneConverter}
and @ref ShaderTools::AnyConverter "AnyShaderConverter" are now capable of
propagating configuration options to the concrete plugin, which is useful
mainly when using @ref magnum-imageconverter and other utilities as you can
now specify just the `-i` / `-c` options without having to specify which
plugin to apply the option to with `-I` / `-C`. For better usability, the
plugins also warn if the user specifies and option that is not present in
the target implementation.
- Added @ref Trade::PhongMaterialData::hasCommonTextureTransformation(),
@ref Trade::PhongMaterialData::ambientTextureMatrix(),
@ref Trade::PhongMaterialData::diffuseTextureMatrix(),
Expand Down
10 changes: 7 additions & 3 deletions src/Magnum/Implementation/converterUtilities.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ namespace Magnum { namespace Implementation {
/* Used only in executables where we don't want it to be exported */
namespace {

void setOptions(PluginManager::AbstractPlugin& plugin, const std::string& options) {
void setOptions(PluginManager::AbstractPlugin& plugin, const std::string& anyPluginName, const std::string& options) {
for(const std::string& option: Utility::String::splitWithoutEmptyParts(options, ',')) {
auto keyValue = Utility::String::partition(option, '=');
Utility::String::trimInPlace(keyValue[0]);
Expand All @@ -60,8 +60,12 @@ void setOptions(PluginManager::AbstractPlugin& plugin, const std::string& option
/* Provide a warning message in case the plugin doesn't define given
option in its default config. The plugin is not *required* to have
those tho (could be backward compatibility entries, for example), so
not an error. */
if(groupNotRecognized || !group->hasValue(keyParts.back())) {
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
wouldn't help anything. */
if((groupNotRecognized || !group->hasValue(keyParts.back())) && plugin.plugin() != anyPluginName) {
Warning{} << "Option" << keyValue[0] << "not recognized by" << plugin.plugin();
}

Expand Down
4 changes: 2 additions & 2 deletions src/Magnum/MeshTools/sceneconverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ used.)")

/* Set options, if passed */
if(args.isSet("verbose")) importer->addFlags(Trade::ImporterFlag::Verbose);
Implementation::setOptions(*importer, args.value("importer-options"));
Implementation::setOptions(*importer, "AnySceneImporter", args.value("importer-options"));

std::chrono::high_resolution_clock::duration importTime;

Expand Down Expand Up @@ -823,7 +823,7 @@ used.)")
/* Set options, if passed */
if(args.isSet("verbose")) converter->addFlags(Trade::SceneConverterFlag::Verbose);
if(i < args.arrayValueCount("converter-options"))
Implementation::setOptions(*converter, args.arrayValue("converter-options", i));
Implementation::setOptions(*converter, "AnySceneConverter", args.arrayValue("converter-options", i));

/* This is the last --converter (or the implicit AnySceneConverter at
the end), output to a file and exit the loop */
Expand Down
2 changes: 1 addition & 1 deletion src/Magnum/ShaderTools/shaderconverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ see documentation of a particular converter for more information.)")

/* Set options if passed */
if(i < args.arrayValueCount("converter-options"))
Implementation::setOptions(*converter, args.arrayValue("converter-options", i));
Implementation::setOptions(*converter, "AnyShaderConverter", args.arrayValue("converter-options", i));

/* Parse format, if passed. If --info is desired, implicitly set the
output format to SPIR-V */
Expand Down
4 changes: 2 additions & 2 deletions src/Magnum/Trade/imageconverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ key=true; configuration subgroups are delimited with /.)")

/* Set options, if passed */
if(args.isSet("verbose")) importer->addFlags(Trade::ImporterFlag::Verbose);
Implementation::setOptions(*importer, args.value("importer-options"));
Implementation::setOptions(*importer, "AnyImageImporter", args.value("importer-options"));

/* Print image info, if requested */
if(args.isSet("info")) {
Expand Down Expand Up @@ -329,7 +329,7 @@ key=true; configuration subgroups are delimited with /.)")

/* Set options, if passed */
if(args.isSet("verbose")) converter->addFlags(Trade::ImageConverterFlag::Verbose);
Implementation::setOptions(*converter, args.value("converter-options"));
Implementation::setOptions(*converter, "AnyImageConverter", args.value("converter-options"));

/* Save output file */
if(!converter->convertToFile(*image, output)) {
Expand Down
12 changes: 11 additions & 1 deletion src/MagnumPlugins/AnyAudioImporter/AnyImporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@

#include <Corrade/Containers/Array.h>
#include <Corrade/PluginManager/Manager.h>
#include <Corrade/PluginManager/PluginMetadata.h>
#include <Corrade/Utility/Assert.h>
#include <Corrade/Utility/DebugStl.h>
#include <Corrade/Utility/String.h>

#include "MagnumPlugins/Implementation/propagateConfiguration.h"

namespace Magnum { namespace Audio {

AnyImporter::AnyImporter(PluginManager::Manager<AbstractImporter>& manager): AbstractImporter{manager} {}
Expand Down Expand Up @@ -74,9 +77,16 @@ void AnyImporter::doOpenFile(const std::string& filename) {
return;
}

/* Instantiate the plugin */
Containers::Pointer<AbstractImporter> importer = static_cast<PluginManager::Manager<AbstractImporter>*>(manager())->instantiate(plugin);

/* Propagate configuration */
const PluginManager::PluginMetadata* const metadata = manager()->metadata(plugin);
CORRADE_INTERNAL_ASSERT(metadata);
Magnum::Implementation::propagateConfiguration("Audio::AnyImporter::openFile():", {}, metadata->name(), configuration(), importer->configuration());

/* Try to open the file (error output should be printed by the plugin
itself) */
Containers::Pointer<AbstractImporter> importer = static_cast<PluginManager::Manager<AbstractImporter>*>(manager())->instantiate(plugin);
if(!importer->openFile(filename)) return;

/* Success, save the instance */
Expand Down
12 changes: 12 additions & 0 deletions src/MagnumPlugins/AnyAudioImporter/AnyImporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,18 @@ target_link_libraries(your-app PRIVATE Magnum::AnyAudioImporter)
@endcode
See @ref building, @ref cmake and @ref plugins for more information.
@section Audio-AnyImporter-proxy Interface proxying and option propagation
On a call to @ref openFile(), a file format is detected from the extension and
a corresponding plugin is loaded. After that, options set through
@ref configuration() are propagated to the concrete implementation, with a
warning emitted in case given option is not present in the default
configuration of the target plugin.
Calls to the @ref format(), @ref frequency() and @ref data() functions are then
proxied to the concrete implementation. The @ref close() function closes and
discards the internally instantiated plugin; @ref isOpened() works as usual.
*/
class MAGNUM_ANYAUDIOIMPORTER_EXPORT AnyImporter: public AbstractImporter {
public:
Expand Down
26 changes: 25 additions & 1 deletion src/MagnumPlugins/AnyAudioImporter/Test/AnyAudioImporterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <Corrade/Containers/Array.h>
#include <Corrade/PluginManager/Manager.h>
#include <Corrade/TestSuite/Tester.h>
#include <Corrade/Utility/ConfigurationGroup.h>
#include <Corrade/Utility/DebugStl.h>
#include <Corrade/Utility/FormatStl.h>

Expand All @@ -44,6 +45,9 @@ struct AnyImporterTest: TestSuite::Tester {

void unknown();

void propagateConfiguration();
void propagateConfigurationUnknown();

/* Explicitly forbid system-wide plugin dependencies */
PluginManager::Manager<AbstractImporter> _manager{"nonexistent"};
};
Expand Down Expand Up @@ -72,7 +76,10 @@ AnyImporterTest::AnyImporterTest() {
addInstancedTests({&AnyImporterTest::detect},
Containers::arraySize(DetectData));

addTests({&AnyImporterTest::unknown});
addTests({&AnyImporterTest::unknown,

&AnyImporterTest::propagateConfiguration,
&AnyImporterTest::propagateConfigurationUnknown});

/* Load the plugin directly from the build tree. Otherwise it's static and
already loaded. */
Expand Down Expand Up @@ -133,6 +140,23 @@ void AnyImporterTest::unknown() {
CORRADE_COMPARE(output.str(), "Audio::AnyImporter::openFile(): cannot determine the format of sound.mid\n");
}

void AnyImporterTest::propagateConfiguration() {
CORRADE_SKIP("No importer has any configuration options to test.");
}

void AnyImporterTest::propagateConfigurationUnknown() {
if(!(_manager.loadState("WavAudioImporter") & PluginManager::LoadState::Loaded))
CORRADE_SKIP("WavAudioImporter plugin not enabled, cannot test");

Containers::Pointer<AbstractImporter> importer = _manager.instantiate("AnyAudioImporter");
importer->configuration().setValue("noSuchOption", "isHere");

std::ostringstream out;
Warning redirectWarning{&out};
CORRADE_VERIFY(importer->openFile(WAV_FILE));
CORRADE_COMPARE(out.str(), "Audio::AnyImporter::openFile(): option noSuchOption not recognized by WavAudioImporter\n");
}

}}}}

CORRADE_TEST_MAIN(Magnum::Audio::Test::AnyImporterTest)
9 changes: 7 additions & 2 deletions src/MagnumPlugins/AnyImageConverter/AnyImageConverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include <Corrade/Utility/String.h>

#include "Magnum/Trade/ImageData.h"
#include "MagnumPlugins/Implementation/propagateConfiguration.h"

namespace Magnum { namespace Trade {

Expand Down Expand Up @@ -84,11 +85,12 @@ bool AnyImageConverter::doConvertToFile(const ImageView2D& image, const Containe
Error{} << "Trade::AnyImageConverter::convertToFile(): cannot load the" << plugin << "plugin";
return false;
}

const PluginManager::PluginMetadata* const metadata = manager()->metadata(plugin);
CORRADE_INTERNAL_ASSERT(metadata);
if(flags() & ImageConverterFlag::Verbose) {
Debug d;
d << "Trade::AnyImageConverter::convertToFile(): using" << plugin;
PluginManager::PluginMetadata* metadata = manager()->metadata(plugin);
CORRADE_INTERNAL_ASSERT(metadata);
if(plugin != metadata->name())
d << "(provided by" << metadata->name() << Debug::nospace << ")";
}
Expand All @@ -97,6 +99,9 @@ bool AnyImageConverter::doConvertToFile(const ImageView2D& image, const Containe
Containers::Pointer<AbstractImageConverter> converter = static_cast<PluginManager::Manager<AbstractImageConverter>*>(manager())->instantiate(plugin);
converter->setFlags(flags());

/* Propagate configuration */
Magnum::Implementation::propagateConfiguration("Trade::AnyImageConverter::convertToFile():", {}, metadata->name(), configuration(), converter->configuration());

/* Try to convert the file (error output should be printed by the plugin
itself) */
return converter->convertToFile(image, filename);
Expand Down
11 changes: 11 additions & 0 deletions src/MagnumPlugins/AnyImageConverter/AnyImageConverter.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,17 @@ target_link_libraries(your-app PRIVATE Magnum::AnyImageConverter)
See @ref building, @ref cmake, @ref plugins and @ref file-formats for more
information.
@section Trade-AnyImageConverter-proxy Interface proxying and option propagation
On a call to @ref convertToFile(), a target file format is detected from the
extension and a corresponding plugin is loaded. After that, flags set via
@ref setFlags() and options set through @ref configuration() are propagated to
the concrete implementation, with a warning emitted in case given option is not
present in the default configuration of the target plugin.
The output of the @ref convertToFile() function called on the concrete
implementation is then proxied back.
*/
class MAGNUM_ANYIMAGECONVERTER_EXPORT AnyImageConverter: public AbstractImageConverter {
public:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include <Corrade/Containers/StringStl.h>
#include <Corrade/PluginManager/Manager.h>
#include <Corrade/TestSuite/Tester.h>
#include <Corrade/TestSuite/Compare/File.h>
#include <Corrade/Utility/ConfigurationGroup.h>
#include <Corrade/Utility/Directory.h>
#include <Corrade/Utility/DebugStl.h>
#include <Corrade/Utility/FormatStl.h>
Expand All @@ -53,6 +55,12 @@ struct AnyImageConverterTest: TestSuite::Tester {

void propagateFlags2D();
void propagateFlagsCompressed2D();
void propagateConfiguration2D();
void propagateConfigurationUnknown2D();
void propagateConfigurationCompressed2D();
void propagateConfigurationCompressedUnknown2D();
/* configuration propagation fully tested in AnySceneImporter, as there the
plugins have configuration subgroups as well */

/* Explicitly forbid system-wide plugin dependencies */
PluginManager::Manager<AbstractImageConverter> _manager{"nonexistent"};
Expand Down Expand Up @@ -94,7 +102,11 @@ AnyImageConverterTest::AnyImageConverterTest() {
&AnyImageConverterTest::unknownCompressed2D,

&AnyImageConverterTest::propagateFlags2D,
&AnyImageConverterTest::propagateFlagsCompressed2D});
&AnyImageConverterTest::propagateFlagsCompressed2D,
&AnyImageConverterTest::propagateConfiguration2D,
&AnyImageConverterTest::propagateConfigurationUnknown2D,
&AnyImageConverterTest::propagateConfigurationCompressed2D,
&AnyImageConverterTest::propagateConfigurationCompressedUnknown2D});

/* Load the plugin directly from the build tree. Otherwise it's static and
already loaded. */
Expand Down Expand Up @@ -204,6 +216,58 @@ void AnyImageConverterTest::propagateFlagsCompressed2D() {
CORRADE_SKIP("No file formats to store compressed data yet.");
}

void AnyImageConverterTest::propagateConfiguration2D() {
PluginManager::Manager<AbstractImageConverter> manager{MAGNUM_PLUGINS_IMAGECONVERTER_INSTALL_DIR};
#ifdef ANYIMAGECONVERTER_PLUGIN_FILENAME
CORRADE_VERIFY(manager.load(ANYIMAGECONVERTER_PLUGIN_FILENAME) & PluginManager::LoadState::Loaded);
#endif

if(manager.loadState("OpenExrImageConverter") < PluginManager::LoadState::Loaded)
CORRADE_SKIP("OpenExrImageConverter plugin can't be loaded.");

const std::string filename = Utility::Directory::join(ANYIMAGECONVERTER_TEST_OUTPUT_DIR, "depth32f-custom-channels.exr");

if(Utility::Directory::exists(filename))
CORRADE_VERIFY(Utility::Directory::rm(filename));

const Float Depth32fData[] = {
0.125f, 0.250f, 0.375f,
0.500f, 0.625f, 0.750f
};

const ImageView2D Depth32f{PixelFormat::Depth32F, {3, 2}, Depth32fData};

Containers::Pointer<AbstractImageConverter> converter = manager.instantiate("AnyImageConverter");
converter->configuration().setValue("layer", "left");
converter->configuration().setValue("depth", "height");
CORRADE_VERIFY(converter->convertToFile(Depth32f, filename));
/* Compare to an expected output to ensure the custom channels names were
used */
CORRADE_COMPARE_AS(filename, EXR_FILE, TestSuite::Compare::File);
}

void AnyImageConverterTest::propagateConfigurationUnknown2D() {
if(!(_manager.loadState("TgaImageConverter") & PluginManager::LoadState::Loaded))
CORRADE_SKIP("TgaImageConverter plugin not enabled, cannot test");

/* Just test that the exported file exists */
Containers::Pointer<AbstractImageConverter> converter = _manager.instantiate("AnyImageConverter");
converter->configuration().setValue("noSuchOption", "isHere");

std::ostringstream out;
Warning redirectWarning{&out};
CORRADE_VERIFY(converter->convertToFile(Image, Utility::Directory::join(ANYIMAGECONVERTER_TEST_OUTPUT_DIR, "output.tga")));
CORRADE_COMPARE(out.str(), "Trade::AnyImageConverter::convertToFile(): option noSuchOption not recognized by TgaImageConverter\n");
}

void AnyImageConverterTest::propagateConfigurationCompressed2D() {
CORRADE_SKIP("No file formats to store compressed data yet.");
}

void AnyImageConverterTest::propagateConfigurationCompressedUnknown2D() {
CORRADE_SKIP("No file formats to store compressed data yet.");
}

}}}}

CORRADE_TEST_MAIN(Magnum::Trade::Test::AnyImageConverterTest)
6 changes: 5 additions & 1 deletion src/MagnumPlugins/AnyImageConverter/Test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@

if(CORRADE_TARGET_EMSCRIPTEN OR CORRADE_TARGET_ANDROID)
set(ANYIMAGECONVERTER_TEST_OUTPUT_DIR "write")
set(EXR_FILE depth32f-custom-channels.exr)
else()
set(ANYIMAGECONVERTER_TEST_OUTPUT_DIR ${CMAKE_CURRENT_BINARY_DIR})
set(EXR_FILE ${PROJECT_SOURCE_DIR}/src/MagnumPlugins/AnyImageImporter/Test/depth32f-custom-channels.exr)
endif()

# CMake before 3.8 has broken $<TARGET_FILE*> expressions for iOS (see
Expand All @@ -47,7 +49,9 @@ file(GENERATE OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/configure.h
INPUT ${CMAKE_CURRENT_BINARY_DIR}/configure.h.in)

corrade_add_test(AnyImageConverterTest AnyImageConverterTest.cpp
LIBRARIES MagnumTrade)
LIBRARIES MagnumTrade
FILES
../../AnyImageImporter/Test/depth32f-custom-channels.exr)
target_include_directories(AnyImageConverterTest PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>)
if(MAGNUM_ANYIMAGECONVERTER_BUILD_STATIC)
target_link_libraries(AnyImageConverterTest PRIVATE AnyImageConverter)
Expand Down
15 changes: 15 additions & 0 deletions src/MagnumPlugins/AnyImageConverter/Test/configure.h.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,18 @@
#cmakedefine ANYIMAGECONVERTER_PLUGIN_FILENAME "${ANYIMAGECONVERTER_PLUGIN_FILENAME}"
#cmakedefine TGAIMAGECONVERTER_PLUGIN_FILENAME "${TGAIMAGECONVERTER_PLUGIN_FILENAME}"
#define ANYIMAGECONVERTER_TEST_OUTPUT_DIR "${ANYIMAGECONVERTER_TEST_OUTPUT_DIR}"
#define EXR_FILE "${EXR_FILE}"

#ifdef CORRADE_TARGET_WINDOWS
#ifdef CORRADE_IS_DEBUG_BUILD
#define MAGNUM_PLUGINS_IMAGECONVERTER_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/${MAGNUM_PLUGINS_IMAGECONVERTER_DEBUG_BINARY_INSTALL_DIR}"
#else
#define MAGNUM_PLUGINS_IMAGECONVERTER_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/${MAGNUM_PLUGINS_IMAGECONVERTER_RELEASE_BINARY_INSTALL_DIR}"
#endif
#else
#ifdef CORRADE_IS_DEBUG_BUILD
#define MAGNUM_PLUGINS_IMAGECONVERTER_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/${MAGNUM_PLUGINS_IMAGECONVERTER_DEBUG_LIBRARY_INSTALL_DIR}"
#else
#define MAGNUM_PLUGINS_IMAGECONVERTER_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/${MAGNUM_PLUGINS_IMAGECONVERTER_RELEASE_LIBRARY_INSTALL_DIR}"
#endif
#endif
Loading

0 comments on commit e6d673c

Please sign in to comment.