Trade,ShaderTools: add addFlags() / clearFlags() to all plugin interf…

…aces. There will be Flag::FlipY for images at some point, enabled by default for compatibility with existing GL code, and so it makes sense to start discouraging setFlags() as early as possible to avoid people resetting the default by accident. Also update the imageconverter, sceneconverter and shaderconverter utils to use these instead of setFlags().
mosra · Mar 5, 2021 · 29ac7b5 · 29ac7b5
1 parent e3c72b3
commit 29ac7b5
Show file tree

Hide file tree

Showing 16 changed files with 181 additions and 14 deletions.
diff --git a/doc/changelog.dox b/doc/changelog.dox
@@ -173,6 +173,11 @@ See also:
     well as support in @ref Trade::AnySceneImporter "AnySceneImporter"
 -   @ref Trade::LightData got extended to support light attenuation and range
     parameters as well and spot light inner and outer angle
+-   @ref Trade::AbstractImporter, @ref Trade::AbstractImageConverter and
+    @ref Trade::AbstractSceneConverter now has @relativeref{Trade::AbstractImporter,addFlags()} and
+    @relativeref{Trade::AbstractImporter,clearFlags()} convenience helpers that
+    are encouraged over @relativeref{Trade::AbstractImporter,setFlags()} as it
+    avoid accidentally clearing default flags potentially added in the future.
 
 @subsubsection changelog-latest-new-vk Vk library
 

diff --git a/src/Magnum/MeshTools/sceneconverter.cpp b/src/Magnum/MeshTools/sceneconverter.cpp
@@ -261,7 +261,7 @@ used.)")
     }
 
     /* Set options, if passed */
-    if(args.isSet("verbose")) importer->setFlags(Trade::ImporterFlag::Verbose);
+    if(args.isSet("verbose")) importer->addFlags(Trade::ImporterFlag::Verbose);
     Implementation::setOptions(*importer, args.value("importer-options"));
 
     std::chrono::high_resolution_clock::duration importTime;
@@ -766,7 +766,7 @@ used.)")
         }
 
         /* Set options, if passed */
-        if(args.isSet("verbose")) converter->setFlags(Trade::SceneConverterFlag::Verbose);
+        if(args.isSet("verbose")) converter->addFlags(Trade::SceneConverterFlag::Verbose);
         if(i < args.arrayValueCount("converter-options"))
             Implementation::setOptions(*converter, args.arrayValue("converter-options", i));
 

diff --git a/src/Magnum/ShaderTools/AbstractConverter.cpp b/src/Magnum/ShaderTools/AbstractConverter.cpp
@@ -95,6 +95,14 @@ void AbstractConverter::setFlags(const ConverterFlags flags) {
 
 void AbstractConverter::doSetFlags(ConverterFlags) {}
 
+void AbstractConverter::addFlags(ConverterFlags flags) {
+    setFlags(_flags|flags);
+}
+
+void AbstractConverter::clearFlags(ConverterFlags flags) {
+    setFlags(_flags & ~flags);
+}
+
 void AbstractConverter::setInputFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*callback)(const std::string&, InputFileCallbackPolicy, void*), void* const userData) {
     /* Clearing the *File bits as those are present in *Data as well and thus
        this would pass even if only file conversion/validation is supported,

diff --git a/src/Magnum/ShaderTools/AbstractConverter.h b/src/Magnum/ShaderTools/AbstractConverter.h
@@ -471,14 +471,36 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
          *
          * Some flags can be set only if the converter supports particular
          * features, see documentation of each @ref ConverterFlag for more
-         * information. By default no flags are set.
+         * information. By default no flags are set. To avoid clearing
+         * potential future default flags by accident, prefer to use
+         * @ref addFlags() and @ref clearFlags() instead.
          *
          * Corresponds to the `-q` / `--quiet`, `-v` / `--verbose`,
          * `--warning-as-error` and `-E` / `--preprocess-only` options
          * in @ref magnum-shaderconverter "magnum-shaderconverter".
          */
         void setFlags(ConverterFlags flags);
 
+        /**
+         * @brief Add converter flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ORed with @p flags.
+         * Useful for preserving the defaults.
+         * @see @ref clearFlags()
+         */
+        void addFlags(ConverterFlags flags);
+
+        /**
+         * @brief Clear converter flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ANDed with inverse of
+         * @p flags. Useful for removing default flags.
+         * @see @ref addFlags()
+         */
+        void clearFlags(ConverterFlags flags);
+
         /**
          * @brief Input file callback function
          *

diff --git a/src/Magnum/ShaderTools/Test/AbstractConverterTest.cpp b/src/Magnum/ShaderTools/Test/AbstractConverterTest.cpp
@@ -359,12 +359,21 @@ void AbstractConverterTest::setFlags() {
 
         ConverterFlags _flags;
     } converter;
-
     CORRADE_COMPARE(converter.flags(), ConverterFlags{});
     CORRADE_COMPARE(converter._flags, ConverterFlags{});
+
     converter.setFlags(ConverterFlag::Verbose);
     CORRADE_COMPARE(converter.flags(), ConverterFlag::Verbose);
     CORRADE_COMPARE(converter._flags, ConverterFlag::Verbose);
+
+    /** @todo use a real flag when we have more than one */
+    converter.addFlags(ConverterFlag(4));
+    CORRADE_COMPARE(converter.flags(), ConverterFlag::Verbose|ConverterFlag(4));
+    CORRADE_COMPARE(converter._flags, ConverterFlag::Verbose|ConverterFlag(4));
+
+    converter.clearFlags(ConverterFlag::Verbose);
+    CORRADE_COMPARE(converter.flags(), ConverterFlag(4));
+    CORRADE_COMPARE(converter._flags, ConverterFlag(4));
 }
 
 void AbstractConverterTest::setFlagsBothQuietAndVerbose() {

diff --git a/src/Magnum/ShaderTools/shaderconverter.cpp b/src/Magnum/ShaderTools/shaderconverter.cpp
@@ -470,7 +470,7 @@ see documentation of a particular converter for more information.)")
             }
         }
 
-        converter->setFlags(flags);
+        converter->addFlags(flags);
 
         /* If we want just SPIR-V info, convert to a SPIR-V and exit */
         if(args.isSet("info")) {

diff --git a/src/Magnum/Trade/AbstractImageConverter.cpp b/src/Magnum/Trade/AbstractImageConverter.cpp
@@ -85,6 +85,14 @@ void AbstractImageConverter::setFlags(ImageConverterFlags flags) {
 
 void AbstractImageConverter::doSetFlags(ImageConverterFlags) {}
 
+void AbstractImageConverter::addFlags(ImageConverterFlags flags) {
+    setFlags(_flags|flags);
+}
+
+void AbstractImageConverter::clearFlags(ImageConverterFlags flags) {
+    setFlags(_flags & ~flags);
+}
+
 Containers::Optional<Image2D> AbstractImageConverter::exportToImage(const ImageView2D& image) {
     CORRADE_ASSERT(features() & ImageConverterFeature::ConvertImage,
         "Trade::AbstractImageConverter::exportToImage(): feature not supported", {});

diff --git a/src/Magnum/Trade/AbstractImageConverter.h b/src/Magnum/Trade/AbstractImageConverter.h
@@ -262,13 +262,35 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract
          *
          * Some flags can be set only if the converter supports particular
          * features, see documentation of each @ref ImageConverterFlag for more
-         * information. By default no flags are set.
+         * information. By default no flags are set. To avoid clearing
+         * potential future default flags by accident, prefer to use
+         * @ref addFlags() and @ref clearFlags() instead.
          *
          * Corresponds to the `-v` / `--verbose` option in
          * @ref magnum-imageconverter "magnum-imageconverter".
          */
         void setFlags(ImageConverterFlags flags);
 
+        /**
+         * @brief Add converter flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ORed with @p flags.
+         * Useful for preserving the defaults.
+         * @see @ref clearFlags()
+         */
+        void addFlags(ImageConverterFlags flags);
+
+        /**
+         * @brief Clear converter flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ANDed with inverse of
+         * @p flags. Useful for removing default flags.
+         * @see @ref addFlags()
+         */
+        void clearFlags(ImageConverterFlags flags);
+
         /**
          * @brief Convert image to different format
          *

diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp
@@ -104,6 +104,14 @@ void AbstractImporter::setFlags(ImporterFlags flags) {
 
 void AbstractImporter::doSetFlags(ImporterFlags) {}
 
+void AbstractImporter::addFlags(ImporterFlags flags) {
+    setFlags(_flags|flags);
+}
+
+void AbstractImporter::clearFlags(ImporterFlags flags) {
+    setFlags(_flags & ~flags);
+}
+
 void AbstractImporter::setFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*callback)(const std::string&, InputFileCallbackPolicy, void*), void* const userData) {
     CORRADE_ASSERT(!isOpened(), "Trade::AbstractImporter::setFileCallback(): can't be set while a file is opened", );
     CORRADE_ASSERT(features() & (ImporterFeature::FileCallback|ImporterFeature::OpenData), "Trade::AbstractImporter::setFileCallback(): importer supports neither loading from data nor via callbacks, callbacks can't be used", );

diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h
@@ -397,7 +397,9 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi
          * It's expected that this function is called *before* a file is
          * opened. Some flags can be set only if the importer supports
          * particular features, see documentation of each @ref ImporterFlag for
-         * more information. By default no flags are set.
+         * more information. By default no flags are set. To avoid clearing
+         * potential future default flags by accident, prefer to use
+         * @ref addFlags() and @ref clearFlags() instead.
          *
          * Corresponds to the `-v` / `--verbose` option in
          * @ref magnum-imageconverter "magnum-imageconverter",
@@ -406,6 +408,26 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi
          */
         void setFlags(ImporterFlags flags);
 
+        /**
+         * @brief Add importer flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ORed with @p flags.
+         * Useful for preserving the defaults.
+         * @see @ref clearFlags()
+         */
+        void addFlags(ImporterFlags flags);
+
+        /**
+         * @brief Clear importer flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ANDed with inverse of
+         * @p flags. Useful for removing default flags.
+         * @see @ref addFlags()
+         */
+        void clearFlags(ImporterFlags flags);
+
         /**
          * @brief File opening callback function
          *

diff --git a/src/Magnum/Trade/AbstractSceneConverter.cpp b/src/Magnum/Trade/AbstractSceneConverter.cpp
@@ -89,6 +89,14 @@ void AbstractSceneConverter::setFlags(SceneConverterFlags flags) {
 
 void AbstractSceneConverter::doSetFlags(SceneConverterFlags) {}
 
+void AbstractSceneConverter::addFlags(SceneConverterFlags flags) {
+    setFlags(_flags|flags);
+}
+
+void AbstractSceneConverter::clearFlags(SceneConverterFlags flags) {
+    setFlags(_flags & ~flags);
+}
+
 Containers::Optional<MeshData> AbstractSceneConverter::convert(const MeshData& mesh) {
     CORRADE_ASSERT(features() & SceneConverterFeature::ConvertMesh,
         "Trade::AbstractSceneConverter::convert(): mesh conversion not supported", {});

diff --git a/src/Magnum/Trade/AbstractSceneConverter.h b/src/Magnum/Trade/AbstractSceneConverter.h
@@ -231,13 +231,35 @@ class MAGNUM_TRADE_EXPORT AbstractSceneConverter: public PluginManager::Abstract
          *
          * Some flags can be set only if the converter supports particular
          * features, see documentation of each @ref SceneConverterFlag for more
-         * information. By default no flags are set.
+         * information. By default no flags are set. To avoid clearing
+         * potential future default flags by accident, prefer to use
+         * @ref addFlags() and @ref clearFlags() instead.
          *
          * Corresponds to the `-v` / `--verbose` option in
          * @ref magnum-sceneconverter "magnum-sceneconverter".
          */
         void setFlags(SceneConverterFlags flags);
 
+        /**
+         * @brief Add converter flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ORed with @p flags.
+         * Useful for preserving the defaults.
+         * @see @ref clearFlags()
+         */
+        void addFlags(SceneConverterFlags flags);
+
+        /**
+         * @brief Clear converter flags
+         * @m_since_latest
+         *
+         * Calls @ref setFlags() with the existing flags ANDed with inverse of
+         * @p flags. Useful for removing default flags.
+         * @see @ref addFlags()
+         */
+        void clearFlags(SceneConverterFlags flags);
+
         /**
          * @brief Convert a mesh
          *

diff --git a/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp b/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp
@@ -170,12 +170,21 @@ void AbstractImageConverterTest::setFlags() {
 
         ImageConverterFlags _flags;
     } converter;
-
     CORRADE_COMPARE(converter.flags(), ImageConverterFlags{});
     CORRADE_COMPARE(converter._flags, ImageConverterFlags{});
+
     converter.setFlags(ImageConverterFlag::Verbose);
     CORRADE_COMPARE(converter.flags(), ImageConverterFlag::Verbose);
     CORRADE_COMPARE(converter._flags, ImageConverterFlag::Verbose);
+
+    /** @todo use a real flag when we have more than one */
+    converter.addFlags(ImageConverterFlag(4));
+    CORRADE_COMPARE(converter.flags(), ImageConverterFlag::Verbose|ImageConverterFlag(4));
+    CORRADE_COMPARE(converter._flags, ImageConverterFlag::Verbose|ImageConverterFlag(4));
+
+    converter.clearFlags(ImageConverterFlag::Verbose);
+    CORRADE_COMPARE(converter.flags(), ImageConverterFlag(4));
+    CORRADE_COMPARE(converter._flags, ImageConverterFlag(4));
 }
 
 void AbstractImageConverterTest::setFlagsNotImplemented() {

diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp
@@ -540,12 +540,21 @@ void AbstractImporterTest::setFlags() {
 
         ImporterFlags _flags;
     } importer;
-
     CORRADE_COMPARE(importer.flags(), ImporterFlags{});
     CORRADE_COMPARE(importer._flags, ImporterFlags{});
+
     importer.setFlags(ImporterFlag::Verbose);
     CORRADE_COMPARE(importer.flags(), ImporterFlag::Verbose);
     CORRADE_COMPARE(importer._flags, ImporterFlag::Verbose);
+
+    /** @todo use a real flag when we have more than one */
+    importer.addFlags(ImporterFlag(4));
+    CORRADE_COMPARE(importer.flags(), ImporterFlag::Verbose|ImporterFlag(4));
+    CORRADE_COMPARE(importer._flags, ImporterFlag::Verbose|ImporterFlag(4));
+
+    importer.clearFlags(ImporterFlag::Verbose);
+    CORRADE_COMPARE(importer.flags(), ImporterFlag(4));
+    CORRADE_COMPARE(importer._flags, ImporterFlag(4));
 }
 
 void AbstractImporterTest::setFlagsNotImplemented() {
@@ -575,7 +584,13 @@ void AbstractImporterTest::setFlagsFileOpened() {
     std::ostringstream out;
     Error redirectError{&out};
     importer.setFlags(ImporterFlag::Verbose);
-    CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::setFlags(): can't be set while a file is opened\n");
+    importer.addFlags(ImporterFlag::Verbose);
+    importer.clearFlags(ImporterFlag::Verbose);
+    CORRADE_COMPARE(out.str(),
+        "Trade::AbstractImporter::setFlags(): can't be set while a file is opened\n"
+        /* These all call into setFlags(), so the same assert is reused */
+        "Trade::AbstractImporter::setFlags(): can't be set while a file is opened\n"
+        "Trade::AbstractImporter::setFlags(): can't be set while a file is opened\n");
 }
 
 void AbstractImporterTest::openData() {

diff --git a/src/Magnum/Trade/Test/AbstractSceneConverterTest.cpp b/src/Magnum/Trade/Test/AbstractSceneConverterTest.cpp
@@ -142,12 +142,21 @@ void AbstractSceneConverterTest::setFlags() {
 
         SceneConverterFlags _flags;
     } converter;
-
     CORRADE_COMPARE(converter.flags(), SceneConverterFlags{});
     CORRADE_COMPARE(converter._flags, SceneConverterFlags{});
+
     converter.setFlags(SceneConverterFlag::Verbose);
     CORRADE_COMPARE(converter.flags(), SceneConverterFlag::Verbose);
     CORRADE_COMPARE(converter._flags, SceneConverterFlag::Verbose);
+
+    /** @todo use a real flag when we have more than one */
+    converter.addFlags(SceneConverterFlag(4));
+    CORRADE_COMPARE(converter.flags(), SceneConverterFlag::Verbose|SceneConverterFlag(4));
+    CORRADE_COMPARE(converter._flags, SceneConverterFlag::Verbose|SceneConverterFlag(4));
+
+    converter.clearFlags(SceneConverterFlag::Verbose);
+    CORRADE_COMPARE(converter.flags(), SceneConverterFlag(4));
+    CORRADE_COMPARE(converter._flags, SceneConverterFlag(4));
 }
 
 void AbstractSceneConverterTest::setFlagsNotImplemented() {

diff --git a/src/Magnum/Trade/imageconverter.cpp b/src/Magnum/Trade/imageconverter.cpp
@@ -244,7 +244,7 @@ key=true; configuration subgroups are delimited with /.)")
         }
 
         /* Set options, if passed */
-        if(args.isSet("verbose")) importer->setFlags(Trade::ImporterFlag::Verbose);
+        if(args.isSet("verbose")) importer->addFlags(Trade::ImporterFlag::Verbose);
         Implementation::setOptions(*importer, args.value("importer-options"));
 
         /* Print image info, if requested */
@@ -328,7 +328,7 @@ key=true; configuration subgroups are delimited with /.)")
     }
 
     /* Set options, if passed */
-    if(args.isSet("verbose")) converter->setFlags(Trade::ImageConverterFlag::Verbose);
+    if(args.isSet("verbose")) converter->addFlags(Trade::ImageConverterFlag::Verbose);
     Implementation::setOptions(*converter, args.value("converter-options"));
 
     /* Save output file */