From 0ec8e002fb7509cc9738fd8b88fca799eb02fe10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 26 Jul 2020 01:25:40 +0200 Subject: [PATCH] Trade: return Optional instead of Pointer from *Importer::material(). The plugin interface version got bumped to avoid ABI issues when loading plugins that weren't updated for the change, but apart from that this shouldn't be a breaking change, as the API returns a type that can be both an Optional and a Pointer. --- doc/changelog.dox | 6 ++ doc/snippets/MagnumTrade.cpp | 14 ++--- src/Magnum/Trade/AbstractImporter.cpp | 30 ++++++++-- src/Magnum/Trade/AbstractImporter.h | 38 +++++++++--- .../Trade/Test/AbstractImporterTest.cpp | 58 +++++++++++++++++-- .../AnyImageImporter/AnyImageImporter.cpp | 2 +- .../AnySceneImporter/AnySceneImporter.cpp | 6 +- .../AnySceneImporter/AnySceneImporter.h | 2 +- src/MagnumPlugins/ObjImporter/ObjImporter.cpp | 2 +- src/MagnumPlugins/TgaImporter/TgaImporter.cpp | 2 +- 10 files changed, 126 insertions(+), 34 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 2004f6f15..fd0811f87 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -150,6 +150,12 @@ See also: directly but rather generated on-the-fly from attribute data, which makes them less efficient than calling @ref Trade::MaterialData::hasAttribute() etc. +- @ref Trade::AbstractImporter::material() now returns + @ref Corrade::Containers::Optional instead of a @ref Corrade::Containers::Pointer, + as the new @ref Trade::MaterialData class isn't polymorphic anymore. If + @ref MAGNUM_BUILD_DEPRECATED is enabled, the returned type acts as a + @ref Corrade::Containers::Optional but has (deprecated) implicit conversion + to a @ref Corrade::Containers::Pointer to avoid breaking user code. @subsection changelog-latest-compatibility Potential compatibility breakages, removed APIs diff --git a/doc/snippets/MagnumTrade.cpp b/doc/snippets/MagnumTrade.cpp index d94679d9f..2e524f8de 100644 --- a/doc/snippets/MagnumTrade.cpp +++ b/doc/snippets/MagnumTrade.cpp @@ -42,7 +42,7 @@ #include "Magnum/Trade/ImageData.h" #include "Magnum/Trade/MeshData.h" #include "Magnum/Trade/ObjectData2D.h" -#include "Magnum/Trade/ObjectData3D.h" +#include "Magnum/Trade/MeshObjectData3D.h" #include "Magnum/Trade/PhongMaterialData.h" #ifdef MAGNUM_TARGET_GL #include "Magnum/GL/Texture.h" @@ -116,17 +116,17 @@ importer->openFile("scene.gltf"); // memory-maps all files { Containers::Pointer importer; -Float shininess; +Int materialIndex; /* [AbstractImporter-usage-cast] */ -Containers::Pointer data = importer->material(12); -if(data && data->type() == Trade::MaterialType::Phong) { - auto& phong = static_cast(*data); +Containers::Pointer data = importer->object3D(12); +if(data && data->instanceType() == Trade::ObjectInstanceType3D::Mesh) { + auto& mesh = static_cast(*data); - shininess = phong.shininess(); + materialIndex = mesh.material(); // ... } /* [AbstractImporter-usage-cast] */ -static_cast(shininess); +static_cast(materialIndex); } { diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index 1425b2377..404c275b9 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -33,12 +33,12 @@ #include #include "Magnum/FileCallback.h" -#include "Magnum/Trade/AbstractMaterialData.h" #include "Magnum/Trade/AnimationData.h" #include "Magnum/Trade/ArrayAllocator.h" #include "Magnum/Trade/CameraData.h" #include "Magnum/Trade/ImageData.h" #include "Magnum/Trade/LightData.h" +#include "Magnum/Trade/MaterialData.h" #include "Magnum/Trade/MeshData.h" #include "Magnum/Trade/ObjectData2D.h" #include "Magnum/Trade/ObjectData3D.h" @@ -59,7 +59,7 @@ namespace Magnum { namespace Trade { std::string AbstractImporter::pluginInterface() { - return "cz.mosra.magnum.Trade.AbstractImporter/0.3.1"; + return "cz.mosra.magnum.Trade.AbstractImporter/0.3.2"; } #ifndef CORRADE_PLUGINMANAGER_NO_DYNAMIC_PLUGIN_SUPPORT @@ -671,17 +671,35 @@ std::string AbstractImporter::materialName(const UnsignedInt id) { std::string AbstractImporter::doMaterialName(UnsignedInt) { return {}; } -Containers::Pointer AbstractImporter::material(const UnsignedInt id) { +#if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT) +Containers::Optional +#else +OptionalButAlsoPointer +#endif +AbstractImporter::material(const UnsignedInt id) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::material(): no file opened", {}); CORRADE_ASSERT(id < doMaterialCount(), "Trade::AbstractImporter::material(): index" << id << "out of range for" << doMaterialCount() << "entries", {}); - return doMaterial(id); + + Containers::Optional material = doMaterial(id); + + /* GCC 4.8 and clang-cl needs an explicit conversion here */ + #ifdef MAGNUM_BUILD_DEPRECATED + return OptionalButAlsoPointer{std::move(material)}; + #else + return material; + #endif } -Containers::Pointer AbstractImporter::doMaterial(UnsignedInt) { +Containers::Optional AbstractImporter::doMaterial(UnsignedInt) { CORRADE_ASSERT_UNREACHABLE("Trade::AbstractImporter::material(): not implemented", {}); } -Containers::Pointer AbstractImporter::material(const std::string& name) { +#if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT) +Containers::Optional +#else +OptionalButAlsoPointer +#endif +AbstractImporter::material(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::material(): no file opened", {}); const Int id = doMaterialForName(name); if(id == -1) { diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h index 30e5adee4..794503df9 100644 --- a/src/Magnum/Trade/AbstractImporter.h +++ b/src/Magnum/Trade/AbstractImporter.h @@ -129,6 +129,18 @@ MAGNUM_TRADE_EXPORT Debug& operator<<(Debug& debug, ImporterFlag value); */ MAGNUM_TRADE_EXPORT Debug& operator<<(Debug& debug, ImporterFlags value); +#if defined(MAGNUM_BUILD_DEPRECATED) && !defined(DOXYGEN_GENERATING_OUTPUT) +/* Could be a concrete type as only MaterialData need this, but that would + mean I'd need to include MaterialData here */ +template struct OptionalButAlsoPointer: Containers::Optional { + /*implicit*/ OptionalButAlsoPointer() = default; + /*implicit*/ OptionalButAlsoPointer(Containers::Optional&& optional): Containers::Optional{std::move(optional)} {} + CORRADE_DEPRECATED("use Containers::Optional instead of Containers::Pointer for a MaterialData") /*implicit*/ operator Containers::Pointer() && { + return Containers::Pointer{new T{std::move(**this)}}; + } +}; +#endif + /** @brief Base for importer plugins @@ -186,8 +198,8 @@ expose internal state through various accessors: - @ref importerState() can expose a pointer to the global importer state for currently opened file -- @ref AbstractMaterialData::importerState() can expose importer state for - given material imported by @ref material() +- @ref MaterialData::importerState() can expose importer state for given + material imported by @ref material() - @ref AnimationData::importerState() can expose importer state for given animation imported by @ref animation() - @ref CameraData::importerState() can expose importer state for a camera @@ -325,7 +337,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @brief Plugin interface * * @code{.cpp} - * "cz.mosra.magnum.Trade.AbstractImporter/0.3.1" + * "cz.mosra.magnum.Trade.AbstractImporter/0.3.2" * @endcode */ static std::string pluginInterface(); @@ -1020,11 +1032,16 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @brief Material * @param id Material ID, from range [0, @ref materialCount()). * - * Returns given material or @cpp nullptr @ce if importing failed. - * Expects that a file is opened. + * Returns given material or @ref Containers::NullOpt if importing + * failed. Expects that a file is opened. * @see @ref material(const std::string&) */ - Containers::Pointer material(UnsignedInt id); + #if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT) + Containers::Optional + #else + OptionalButAlsoPointer + #endif + material(UnsignedInt id); /** * @brief Material for given name @@ -1036,7 +1053,12 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @ref Containers::NullOpt, otherwise propagates the result from * @ref material(UnsignedInt). Expects that a file is opened. */ - Containers::Pointer material(const std::string& name); + #if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT) + Containers::Optional + #else + OptionalButAlsoPointer + #endif + material(const std::string& name); /** * @brief Texture count @@ -1695,7 +1717,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi virtual std::string doMaterialName(UnsignedInt id); /** @brief Implementation for @ref material() */ - virtual Containers::Pointer doMaterial(UnsignedInt id); + virtual Containers::Optional doMaterial(UnsignedInt id); /** * @brief Implementation for @ref textureCount() diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index d71d54210..d9f0f1009 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -189,6 +190,9 @@ struct AbstractImporterTest: TestSuite::Tester { #endif void material(); + #ifdef MAGNUM_BUILD_DEPRECATED + void materialDeprecatedFallback(); + #endif void materialNameNotImplemented(); void materialNameOutOfRange(); void materialNotImplemented(); @@ -391,6 +395,9 @@ AbstractImporterTest::AbstractImporterTest() { #endif &AbstractImporterTest::material, + #ifdef MAGNUM_BUILD_DEPRECATED + &AbstractImporterTest::materialDeprecatedFallback, + #endif &AbstractImporterTest::materialNameNotImplemented, &AbstractImporterTest::materialNameOutOfRange, &AbstractImporterTest::materialNotImplemented, @@ -2989,12 +2996,8 @@ void AbstractImporterTest::material() { if(id == 7) return "eighth"; else return {}; } - Containers::Pointer doMaterial(UnsignedInt id) override { - if(id == 7) return Containers::pointer(new PhongMaterialData{{}, - {}, {}, - {}, {}, - {}, {}, {}, {}, - {}, {}, {}, &state}); + Containers::Optional doMaterial(UnsignedInt id) override { + if(id == 7) return Containers::optional(MaterialTypes{}, nullptr, &state); else return {}; } } importer; @@ -3013,6 +3016,49 @@ void AbstractImporterTest::material() { CORRADE_COMPARE(data->importerState(), &state); } } + +#ifdef MAGNUM_BUILD_DEPRECATED +void AbstractImporterTest::materialDeprecatedFallback() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doMaterialCount() const override { return 8; } + Int doMaterialForName(const std::string&) override { return 0; } + /* Using a deprecated PhongMaterialData constructor to verify that + propagating such instance works as well (array deleters etc.) */ + Containers::Optional doMaterial(UnsignedInt) override { + CORRADE_IGNORE_DEPRECATED_PUSH + return Containers::Optional{std::move(PhongMaterialData{{}, + {}, {}, + {}, {}, + {}, {}, + {}, + {}, {}, {}, {}, &state + })}; + CORRADE_IGNORE_DEPRECATED_POP + } + } importer; + + { + CORRADE_IGNORE_DEPRECATED_PUSH + Containers::Pointer data = importer.material(0); + CORRADE_IGNORE_DEPRECATED_POP + CORRADE_VERIFY(data); + CORRADE_COMPARE_AS(data->attributeCount(), 0, TestSuite::Compare::Greater); + CORRADE_COMPARE(data->importerState(), &state); + } { + CORRADE_IGNORE_DEPRECATED_PUSH + Containers::Pointer data = importer.material(""); + CORRADE_IGNORE_DEPRECATED_POP + CORRADE_VERIFY(data); + CORRADE_COMPARE_AS(data->attributeCount(), 0, TestSuite::Compare::Greater); + CORRADE_COMPARE(data->importerState(), &state); + } +} +#endif + void AbstractImporterTest::materialNameNotImplemented() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; } diff --git a/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp b/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp index 635cc9923..b2a789627 100644 --- a/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp +++ b/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp @@ -239,4 +239,4 @@ Containers::Optional AnyImageImporter::doImage2D(const UnsignedInt }} CORRADE_PLUGIN_REGISTER(AnyImageImporter, Magnum::Trade::AnyImageImporter, - "cz.mosra.magnum.Trade.AbstractImporter/0.3.1") + "cz.mosra.magnum.Trade.AbstractImporter/0.3.2") diff --git a/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.cpp b/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.cpp index 64043c433..25048581b 100644 --- a/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.cpp +++ b/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.cpp @@ -32,11 +32,11 @@ #include #include -#include "Magnum/Trade/AbstractMaterialData.h" #include "Magnum/Trade/AnimationData.h" #include "Magnum/Trade/CameraData.h" #include "Magnum/Trade/ImageData.h" #include "Magnum/Trade/LightData.h" +#include "Magnum/Trade/MaterialData.h" #include "Magnum/Trade/MeshData.h" #include "Magnum/Trade/ObjectData2D.h" #include "Magnum/Trade/ObjectData3D.h" @@ -217,7 +217,7 @@ CORRADE_IGNORE_DEPRECATED_POP UnsignedInt AnySceneImporter::doMaterialCount() const { return _in->materialCount(); } Int AnySceneImporter::doMaterialForName(const std::string& name) { return _in->materialForName(name); } std::string AnySceneImporter::doMaterialName(const UnsignedInt id) { return _in->materialName(id); } -Containers::Pointer AnySceneImporter::doMaterial(const UnsignedInt id) { return _in->material(id); } +Containers::Optional AnySceneImporter::doMaterial(const UnsignedInt id) { return _in->material(id); } UnsignedInt AnySceneImporter::doTextureCount() const { return _in->textureCount(); } Int AnySceneImporter::doTextureForName(const std::string& name) { return _in->textureForName(name); } @@ -245,4 +245,4 @@ Containers::Optional AnySceneImporter::doImage3D(const UnsignedInt }} CORRADE_PLUGIN_REGISTER(AnySceneImporter, Magnum::Trade::AnySceneImporter, - "cz.mosra.magnum.Trade.AbstractImporter/0.3.1") + "cz.mosra.magnum.Trade.AbstractImporter/0.3.2") diff --git a/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.h b/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.h index d26744289..6a68182a6 100644 --- a/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.h +++ b/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.h @@ -203,7 +203,7 @@ class MAGNUM_ANYSCENEIMPORTER_EXPORT AnySceneImporter: public AbstractImporter { MAGNUM_ANYSCENEIMPORTER_LOCAL UnsignedInt doMaterialCount() const override; MAGNUM_ANYSCENEIMPORTER_LOCAL Int doMaterialForName(const std::string& name) override; MAGNUM_ANYSCENEIMPORTER_LOCAL std::string doMaterialName(UnsignedInt id) override; - MAGNUM_ANYSCENEIMPORTER_LOCAL Containers::Pointer doMaterial(UnsignedInt id) override; + MAGNUM_ANYSCENEIMPORTER_LOCAL Containers::Optional doMaterial(UnsignedInt id) override; MAGNUM_ANYSCENEIMPORTER_LOCAL UnsignedInt doTextureCount() const override; MAGNUM_ANYSCENEIMPORTER_LOCAL Int doTextureForName(const std::string& name) override; diff --git a/src/MagnumPlugins/ObjImporter/ObjImporter.cpp b/src/MagnumPlugins/ObjImporter/ObjImporter.cpp index 1a609a223..f6ceb3b35 100644 --- a/src/MagnumPlugins/ObjImporter/ObjImporter.cpp +++ b/src/MagnumPlugins/ObjImporter/ObjImporter.cpp @@ -486,4 +486,4 @@ Containers::Optional ObjImporter::doMesh(UnsignedInt id, UnsignedInt) }} CORRADE_PLUGIN_REGISTER(ObjImporter, Magnum::Trade::ObjImporter, - "cz.mosra.magnum.Trade.AbstractImporter/0.3.1") + "cz.mosra.magnum.Trade.AbstractImporter/0.3.2") diff --git a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp index 2f9e90a86..f37a236de 100644 --- a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp +++ b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp @@ -206,4 +206,4 @@ Containers::Optional TgaImporter::doImage2D(UnsignedInt, UnsignedIn }} CORRADE_PLUGIN_REGISTER(TgaImporter, Magnum::Trade::TgaImporter, - "cz.mosra.magnum.Trade.AbstractImporter/0.3.1") + "cz.mosra.magnum.Trade.AbstractImporter/0.3.2")