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.
6 years ago · 0ec8e002fb
10 changed files with 126 additions and 34 deletions
--- 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

--- 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<Trade::AbstractImporter> importer;
-Float shininess;
+Int materialIndex;
 /* [AbstractImporter-usage-cast] */
-Containers::Pointer<Trade::AbstractMaterialData> data = importer->material(12);
-if(data && data->type() == Trade::MaterialType::Phong) {
-    auto& phong = static_cast<Trade::PhongMaterialData&>(*data);
+Containers::Pointer<Trade::ObjectData3D> data = importer->object3D(12);
+if(data && data->instanceType() == Trade::ObjectInstanceType3D::Mesh) {
+    auto& mesh = static_cast<Trade::MeshObjectData3D&>(*data);

-    shininess = phong.shininess();
+    materialIndex = mesh.material();
    // ...
 }
 /* [AbstractImporter-usage-cast] */
-static_cast<void>(shininess);
+static_cast<void>(materialIndex);
 }

 {
--- a/src/Magnum/Trade/AbstractImporter.cpp
+++ b/src/Magnum/Trade/AbstractImporter.cpp
@ -33,12 +33,12 @@
 #include <Corrade/Utility/Directory.h>

 #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<AbstractMaterialData> AbstractImporter::material(const UnsignedInt id) {
+#if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT)
+Containers::Optional<MaterialData>
+#else
+OptionalButAlsoPointer<MaterialData>
+#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<MaterialData> material = doMaterial(id);
+
+    /* GCC 4.8 and clang-cl needs an explicit conversion here */
+    #ifdef MAGNUM_BUILD_DEPRECATED
+    return OptionalButAlsoPointer<MaterialData>{std::move(material)};
+    #else
+    return material;
+    #endif
 }

-Containers::Pointer<AbstractMaterialData> AbstractImporter::doMaterial(UnsignedInt) {
+Containers::Optional<MaterialData> AbstractImporter::doMaterial(UnsignedInt) {
    CORRADE_ASSERT_UNREACHABLE("Trade::AbstractImporter::material(): not implemented", {});
 }

-Containers::Pointer<AbstractMaterialData> AbstractImporter::material(const std::string& name) {
+#if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT)
+Containers::Optional<MaterialData>
+#else
+OptionalButAlsoPointer<MaterialData>
+#endif
+AbstractImporter::material(const std::string& name) {
    CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::material(): no file opened", {});
    const Int id = doMaterialForName(name);
    if(id == -1) {
--- 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<class T> struct OptionalButAlsoPointer: Containers::Optional<T> {
+    /*implicit*/ OptionalButAlsoPointer() = default;
+    /*implicit*/ OptionalButAlsoPointer(Containers::Optional<T>&& optional): Containers::Optional<T>{std::move(optional)} {}
+    CORRADE_DEPRECATED("use Containers::Optional instead of Containers::Pointer for a MaterialData") /*implicit*/ operator Containers::Pointer<T>() && {
+        return Containers::Pointer<T>{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<AbstractMaterialData> material(UnsignedInt id);
+        #if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT)
+        Containers::Optional<MaterialData>
+        #else
+        OptionalButAlsoPointer<MaterialData>
+        #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<AbstractMaterialData> material(const std::string& name);
+        #if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT)
+        Containers::Optional<MaterialData>
+        #else
+        OptionalButAlsoPointer<MaterialData>
+        #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<AbstractMaterialData> doMaterial(UnsignedInt id);
+        virtual Containers::Optional<MaterialData> doMaterial(UnsignedInt id);

        /**
         * @brief Implementation for @ref textureCount()
--- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp
+++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp
@ -27,6 +27,7 @@
 #include <Corrade/Containers/ArrayView.h>
 #include <Corrade/Containers/Optional.h>
 #include <Corrade/TestSuite/Tester.h>
+#include <Corrade/TestSuite/Compare/Numeric.h>
 #include <Corrade/Utility/DebugStl.h>
 #include <Corrade/Utility/Directory.h>

@ -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<AbstractMaterialData> doMaterial(UnsignedInt id) override {
-            if(id == 7) return Containers::pointer(new PhongMaterialData{{},
-                {}, {},
-                {}, {},
-                {}, {}, {}, {},
-                {}, {}, {}, &state});
+        Containers::Optional<MaterialData> doMaterial(UnsignedInt id) override {
+            if(id == 7) return Containers::optional<MaterialData>(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<MaterialData> doMaterial(UnsignedInt) override {
+            CORRADE_IGNORE_DEPRECATED_PUSH
+            return Containers::Optional<MaterialData>{std::move(PhongMaterialData{{},
+                {}, {},
+                {}, {},
+                {}, {},
+                {},
+                {}, {}, {}, {}, &state
+            })};
+            CORRADE_IGNORE_DEPRECATED_POP
+        }
+    } importer;
+
+    {
+        CORRADE_IGNORE_DEPRECATED_PUSH
+        Containers::Pointer<MaterialData> 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<MaterialData> 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 {}; }
--- a/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp
+++ b/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp
@ -239,4 +239,4 @@ Containers::Optional<ImageData2D> 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")
--- a/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.cpp
+++ b/src/MagnumPlugins/AnySceneImporter/AnySceneImporter.cpp
@ -32,11 +32,11 @@
 #include <Corrade/Utility/DebugStl.h>
 #include <Corrade/Utility/String.h>

-#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<AbstractMaterialData> AnySceneImporter::doMaterial(const UnsignedInt id) { return _in->material(id); }
+Containers::Optional<MaterialData> 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<ImageData3D> 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")
--- 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<AbstractMaterialData> doMaterial(UnsignedInt id) override;
+        MAGNUM_ANYSCENEIMPORTER_LOCAL Containers::Optional<MaterialData> doMaterial(UnsignedInt id) override;

        MAGNUM_ANYSCENEIMPORTER_LOCAL UnsignedInt doTextureCount() const override;
        MAGNUM_ANYSCENEIMPORTER_LOCAL Int doTextureForName(const std::string& name) override;
--- a/src/MagnumPlugins/ObjImporter/ObjImporter.cpp
+++ b/src/MagnumPlugins/ObjImporter/ObjImporter.cpp
@ -486,4 +486,4 @@ Containers::Optional<MeshData> 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")
--- a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp
+++ b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp
@ -206,4 +206,4 @@ Containers::Optional<ImageData2D> 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")