From 29ac7b5a4dd430b8491e48eb5ea49a15630b09bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 5 Mar 2021 14:27:53 +0100 Subject: [PATCH] Trade,ShaderTools: add addFlags() / clearFlags() to all plugin interfaces. 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(). --- doc/changelog.dox | 5 ++++ src/Magnum/MeshTools/sceneconverter.cpp | 4 ++-- src/Magnum/ShaderTools/AbstractConverter.cpp | 8 +++++++ src/Magnum/ShaderTools/AbstractConverter.h | 24 ++++++++++++++++++- .../Test/AbstractConverterTest.cpp | 11 ++++++++- src/Magnum/ShaderTools/shaderconverter.cpp | 2 +- src/Magnum/Trade/AbstractImageConverter.cpp | 8 +++++++ src/Magnum/Trade/AbstractImageConverter.h | 24 ++++++++++++++++++- src/Magnum/Trade/AbstractImporter.cpp | 8 +++++++ src/Magnum/Trade/AbstractImporter.h | 24 ++++++++++++++++++- src/Magnum/Trade/AbstractSceneConverter.cpp | 8 +++++++ src/Magnum/Trade/AbstractSceneConverter.h | 24 ++++++++++++++++++- .../Trade/Test/AbstractImageConverterTest.cpp | 11 ++++++++- .../Trade/Test/AbstractImporterTest.cpp | 19 +++++++++++++-- .../Trade/Test/AbstractSceneConverterTest.cpp | 11 ++++++++- src/Magnum/Trade/imageconverter.cpp | 4 ++-- 16 files changed, 181 insertions(+), 14 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 4b9ac12b66..9c425ef7b8 100644 --- 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 index 8e1676c3b7..0bdbc45cb0 100644 --- 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 index 0443bb7dbb..e43a4ee79f 100644 --- 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>(*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 index a4030950de..61ff94112b 100644 --- a/src/Magnum/ShaderTools/AbstractConverter.h +++ b/src/Magnum/ShaderTools/AbstractConverter.h @@ -471,7 +471,9 @@ 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 @@ -479,6 +481,26 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac */ 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 index 56b75ac645..8d87988a38 100644 --- 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 index ea4098ef91..bca984b013 100644 --- 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 index 3aea1c9ec0..29db56c33e 100644 --- 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 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 index 63ea0f380f..92f29855a7 100644 --- 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 index 947cbd1bb8..7cfc32bb5f 100644 --- 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>(*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 index c031a1c95b..720c40b3ee 100644 --- 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 index 6c1a6c7de8..7807c8d0a2 100644 --- 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 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 index e28d0edf9a..9b2d7aae3d 100644 --- 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 index a2718951e4..d19264257f 100644 --- 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 index e535dc95eb..6013933e70 100644 --- 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 index 3fc4961165..cbf1baecbc 100644 --- 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 index 99cd708b52..27dc0001f2 100644 --- 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 */