Fix GitHub Issue #86: Deliver MCSurface as a shared_ptr

- Also modernizes renderers

Author: juzzlin

src/game/MiniCore/src/Asset/mcmeshconfigloader.cc

@@ -51,7 +51,7 @@ bool MCMeshConfigLoader::load(const std::string & filePath)
         auto && node = root.firstChild();
         while (!node.isNull() && node.nodeName() == "mesh")
         {
-            MeshDataPtr newData(new MCMeshMetaData);
+            const auto newData = std::make_shared<MCMeshMetaData>();
             const auto && element = node.toElement();
             if (!element.isNull())
             {
@@ -68,7 +68,7 @@ bool MCMeshConfigLoader::load(const std::string & filePath)
     return true;
 }
 
-void MCMeshConfigLoader::parseAttributes(const QDomElement & element, MeshDataPtr newData, const std::string & baseModelPath)
+void MCMeshConfigLoader::parseAttributes(const QDomElement & element, MCMeshDataPtr newData, const std::string & baseModelPath)
 {
     const std::string model = element.attribute("model", "").toStdString();
 
@@ -84,7 +84,7 @@ void MCMeshConfigLoader::parseAttributes(const QDomElement & element, MeshDataPt
     }
 }
 
-void MCMeshConfigLoader::parseChildNodes(const QDomNode & node, MeshDataPtr newData)
+void MCMeshConfigLoader::parseChildNodes(const QDomNode & node, MCMeshDataPtr newData)
 {
     auto && childNode = node.firstChild();
     while (!childNode.isNull())
@@ -120,14 +120,12 @@ void MCMeshConfigLoader::parseChildNodes(const QDomNode & node, MeshDataPtr newD
     }
 }
 
-unsigned int MCMeshConfigLoader::meshCount() const
+size_t MCMeshConfigLoader::meshCount() const
 {
-    return static_cast<unsigned int>(m_meshes.size());
+    return m_meshes.size();
 }
 
-const MCMeshMetaData & MCMeshConfigLoader::mesh(unsigned int index) const
+MCMeshDataPtr MCMeshConfigLoader::mesh(size_t index) const
 {
-    assert(index < static_cast<unsigned int>(m_meshes.size()));
-    assert(m_meshes.at(index));
-    return *m_meshes.at(index);
+    return m_meshes.at(index);
 }

src/game/MiniCore/src/Asset/mcmeshconfigloader.hh

@@ -37,19 +37,17 @@ public:
     bool load(const std::string & filePath);
 
     //! Get mesh count.
-    unsigned int meshCount() const;
+    size_t meshCount() const;
 
     //! Get mesh data of given index.
-    const MCMeshMetaData & mesh(unsigned int index) const;
+    MCMeshDataPtr mesh(size_t index) const;
 
 private:
-    typedef std::shared_ptr<MCMeshMetaData> MeshDataPtr;
+    void parseAttributes(const QDomElement & element, MCMeshDataPtr newData, const std::string & baseModelPath);
 
-    void parseAttributes(const QDomElement & element, MeshDataPtr newData, const std::string & baseModelPath);
+    void parseChildNodes(const QDomNode & node, MCMeshDataPtr newData);
 
-    void parseChildNodes(const QDomNode & node, MeshDataPtr newData);
-
-    std::vector<MeshDataPtr> m_meshes;
+    std::vector<MCMeshDataPtr> m_meshes;
 };
 
 #endif // MCMESHCONFIGLOADER_HH

src/game/MiniCore/src/Asset/mcmeshmanager.cc

@@ -35,22 +35,21 @@ MCMeshManager::MCMeshManager()
 {
 }
 
-MCMesh & MCMeshManager::createMesh(
-  const MCMeshMetaData & data, const MCMesh::FaceVector & faces)
+MCMeshPtr MCMeshManager::createMesh(const MCMeshMetaData & data, const MCMesh::FaceVector & faces)
 {
     // Create material
-    MCGLMaterialPtr material(new MCGLMaterial);
+    const auto material = std::make_shared<MCGLMaterial>();
 
     material->setTexture(
-      data.texture1 != "" ? MCAssetManager::surfaceManager().surface(data.texture1).material()->texture(0) : 0,
+      data.texture1 != "" ? MCAssetManager::surfaceManager().surface(data.texture1)->material()->texture(0) : 0,
       0);
 
     material->setTexture(
-      data.texture2 != "" ? MCAssetManager::surfaceManager().surface(data.texture2).material()->texture(0) : 0,
+      data.texture2 != "" ? MCAssetManager::surfaceManager().surface(data.texture2)->material()->texture(0) : 0,
       1);
 
     // Create a new MCMesh object
-    MeshPtr mesh(new MCMesh(data.handle, faces, material));
+    const auto mesh = std::make_shared<MCMesh>(data.handle, faces, material);
 
     // Maybe better place for this could be in the material?
     mesh->setColor(data.color);
@@ -62,29 +61,28 @@ MCMesh & MCMeshManager::createMesh(
 
     m_meshMap[data.handle] = mesh;
 
-    return *mesh;
+    return mesh;
 }
 
-void MCMeshManager::load(
-  const std::string & configFilePath, const std::string & baseDataPath)
+void MCMeshManager::load(const std::string & configFilePath, const std::string & baseDataPath)
 {
     MCMeshConfigLoader configLoader;
     MCMeshLoader modelLoader;
 
     if (configLoader.load(configFilePath))
     {
-        for (unsigned int i = 0; i < configLoader.meshCount(); i++)
+        for (size_t i = 0; i < configLoader.meshCount(); i++)
         {
-            const MCMeshMetaData & metaData = configLoader.mesh(i);
+            auto && metaData = configLoader.mesh(i);
 
             QString modelPath =
-              QString(baseDataPath.c_str()) + QDir::separator().toLatin1() + metaData.modelPath.c_str();
+              QString(baseDataPath.c_str()) + QDir::separator().toLatin1() + metaData->modelPath.c_str();
             modelPath.replace("./", "");
             modelPath.replace("//", "/");
 
             if (modelLoader.load(modelPath))
             {
-                createMesh(metaData, modelLoader.faces());
+                createMesh(*metaData, modelLoader.faces());
             }
             else
             {
@@ -98,7 +96,7 @@ void MCMeshManager::load(
     }
 }
 
-MCMesh & MCMeshManager::mesh(const std::string & handle) const
+MCMeshPtr MCMeshManager::mesh(const std::string & handle) const
 {
     // Try to find existing mesh for the handle
     if (m_meshMap.count(handle) == 0)
@@ -107,7 +105,5 @@ MCMesh & MCMeshManager::mesh(const std::string & handle) const
     }
 
     // Yes: return handle for the mesh
-    MeshPtr mesh = m_meshMap.find(handle)->second;
-    assert(mesh);
-    return *mesh;
+    return m_meshMap.find(handle)->second;
 }

src/game/MiniCore/src/Asset/mcmeshmanager.hh

@@ -61,36 +61,34 @@ public:
     MCMeshManager();
 
     //! Destructor
-    virtual ~MCMeshManager() {};
+    virtual ~MCMeshManager()
+    {
+    }
 
     /*! Loads mesh config from strBasePath using the given mapping file strFile.
      *  \param configFilePath Path to the XML-based input file.
      *  \param baseDataPath The absolute search path for an mesh is
      *  baseDataPath + baseModelPath + fileName. baseModelPath and the fileName are
      *  defined in the input file. */
-    virtual void load(
-      const std::string & configFilePath, const std::string & baseDataPath);
+    virtual void load(const std::string & configFilePath, const std::string & baseDataPath);
 
     /*! Returns a mesh object associated with given strId.
      *  MCMeshManager will keep the ownership.
      *  \param handle Handle defined in the mesh config file.
      *  \return Reference to the corresponding MCMesh.
      *  \throws std::runtime_error on failure. */
-    MCMesh & mesh(
-      const std::string & handle) const;
+    MCMeshPtr mesh(const std::string & handle) const;
 
     //! Create a mesh from given meta data and face vector.
-    MCMesh & createMesh(
-      const MCMeshMetaData & data, const MCMesh::FaceVector & faces);
+    MCMeshPtr createMesh(const MCMeshMetaData & data, const MCMesh::FaceVector & faces);
 
 private:
-    //! Map for resulting mesh objects
-    typedef std::shared_ptr<MCMesh> MeshPtr;
-    typedef std::unordered_map<std::string, MeshPtr> MeshHash;
+    typedef std::unordered_map<std::string, MCMeshPtr> MeshHash;
     MeshHash m_meshMap;
 
     DISABLE_COPY(MCMeshManager);
     DISABLE_ASSI(MCMeshManager);
+    DISABLE_MOVE(MCMeshManager);
 };
 
 #endif // MCMESHMANAGER_HH

src/game/MiniCore/src/Asset/mcmeshmetadata.hh

@@ -24,16 +24,13 @@
 #include <MCVector3d>
 
 #include <map>
+#include <memory>
 #include <string>
 
 /*! Mesh metadata structure returned by MCMeshConfigLoader.
  *  MCMeshManager can create MCMesh objects based on this data. */
 struct MCMeshMetaData
 {
-    MCMeshMetaData()
-    {
-    }
-
     //! Handle of the mesh.
     std::string handle;
 
@@ -53,4 +50,6 @@ struct MCMeshMetaData
     std::pair<MCVector3dF, bool> scale;
 };
 
+using MCMeshDataPtr = std::shared_ptr<MCMeshMetaData>;
+
 #endif // MCMESHMETADATA_HH

src/game/MiniCore/src/Asset/mcsurfacemanager.cc

@@ -195,7 +195,7 @@ MCSurfaceManager::MCSurfaceManager()
 {
 }
 
-MCSurface & MCSurfaceManager::createSurfaceFromImage(const MCSurfaceMetaData & data, QImage image)
+std::shared_ptr<MCSurface> MCSurfaceManager::createSurfaceFromImage(const MCSurfaceMetaData & data, QImage image)
 {
     if (data.handle.size() == 0)
     {
@@ -212,35 +212,32 @@ MCSurface & MCSurfaceManager::createSurfaceFromImage(const MCSurfaceMetaData & d
     // that are initialized before this surface.
     MCGLMaterialPtr material(new MCGLMaterial);
     material->setTexture(create2DTextureFromImage(data, image), 0);
-    material->setTexture(data.handle2.length() ? surface(data.handle2).material()->texture(0) : 0, 1);
-    material->setTexture(data.handle3.length() ? surface(data.handle3).material()->texture(0) : 0, 2);
+    material->setTexture(data.handle2.length() ? surface(data.handle2)->material()->texture(0) : 0, 1);
+    material->setTexture(data.handle3.length() ? surface(data.handle3)->material()->texture(0) : 0, 2);
 
     if (data.specularCoeff.second)
     {
         material->setSpecularCoeff(data.specularCoeff.first);
     }
 
     // Create a new MCSurface object
-    MCSurface * surface =
-      new MCSurface(data.handle, material, origW, origH, data.z0, data.z1, data.z2, data.z3);
+    auto surface = std::make_shared<MCSurface>(data.handle, material, origW, origH, data.z0, data.z1, data.z2, data.z3);
 
     // Maybe better place for this could be in the material?
     surface->setColor(data.color);
 
-    assert(surface);
-    createSurfaceCommon(*surface, data);
+    createSurfaceCommon(surface, data);
 
-    return *surface;
+    return surface;
 }
 
-void MCSurfaceManager::createSurfaceCommon(MCSurface & surface, const MCSurfaceMetaData & data)
+void MCSurfaceManager::createSurfaceCommon(std::shared_ptr<MCSurface> surface, const MCSurfaceMetaData & data)
 {
     // Enable alpha blend, if set
-    surface.material()->setAlphaBlend(
-      data.alphaBlend.second, data.alphaBlend.first.m_src, data.alphaBlend.first.m_dst);
+    surface->material()->setAlphaBlend(data.alphaBlend.second, data.alphaBlend.first.m_src, data.alphaBlend.first.m_dst);
 
     // Store MCSurface to map
-    m_surfaceMap[data.handle] = &surface;
+    m_surfaceMap[data.handle] = surface;
 }
 #ifdef __MC_GLES__
 static bool isPowerOfTwo(unsigned int x)
@@ -425,25 +422,21 @@ void MCSurfaceManager::applyAlphaClamp(QImage & textureImage, unsigned int a) co
 MCSurfaceManager::~MCSurfaceManager()
 {
     // Delete OpenGL textures and Textures
-    auto iter(m_surfaceMap.begin());
-    while (iter != m_surfaceMap.end())
+    for (auto && iter : m_surfaceMap)
     {
-        if (iter->second)
+        if (iter.second)
         {
-            MCSurface * p = iter->second;
+            const auto surface = iter.second;
             for (unsigned int i = 0; i < MCGLMaterial::MAX_TEXTURES; i++)
             {
-                GLuint dummyHandle1 = p->material()->texture(i);
+                GLuint dummyHandle1 = surface->material()->texture(i);
                 glDeleteTextures(1, &dummyHandle1);
             }
-            delete p;
         }
-        iter++;
     }
 }
 
-void MCSurfaceManager::load(
-  const std::string & configFilePath, const std::string & baseDataPath)
+void MCSurfaceManager::load(const std::string & configFilePath, const std::string & baseDataPath)
 {
     MCSurfaceConfigLoader loader;
 
@@ -479,7 +472,7 @@ void MCSurfaceManager::load(
     }
 }
 
-MCSurface & MCSurfaceManager::surface(const std::string & id) const
+std::shared_ptr<MCSurface> MCSurfaceManager::surface(const std::string & id) const
 {
     // Try to find existing texture for the surface
     if (m_surfaceMap.count(id) == 0)
@@ -488,7 +481,5 @@ MCSurface & MCSurfaceManager::surface(const std::string & id) const
     }
 
     // Yes: return handle for the texture
-    MCSurface * pSurface = m_surfaceMap.find(id)->second;
-    assert(pSurface);
-    return *pSurface;
+    return m_surfaceMap.find(id)->second;
 }

src/game/MiniCore/src/Asset/mcsurfacemanager.hh

@@ -20,6 +20,7 @@
 #ifndef MCSURFACEMANAGER_HH
 #define MCSURFACEMANAGER_HH
 
+#include <memory>
 #include <string>
 #include <unordered_map>
 
@@ -88,16 +89,14 @@ public:
     /*! Returns a surface object associated with given strId.
      *  Corresponding OpenGL texture handle can be obtained
      *  by calling handle() of the resulting MCSurface.
-     *  MCSurfaceManager will keep the ownership.
      *  \param handle Handle defined in the textures XML file.
      *  \return Reference to the corresponding MCSurface.
      *  \throws std::runtime_error on failure. */
-    MCSurface & surface(
-      const std::string & handle) const;
+    std::shared_ptr<MCSurface> surface(const std::string & handle) const;
 
     /*! Creates an MCSurface containing an OpenGL texture from a QImage + texture meta data.
      *  MCSurfaceManager keeps the ownership. */
-    MCSurface & createSurfaceFromImage(const MCSurfaceMetaData & data, QImage image);
+    std::shared_ptr<MCSurface> createSurfaceFromImage(const MCSurfaceMetaData & data, QImage image);
 
 private:
     //! Apply alpha clamp (set alpha values off based on the given limit).
@@ -110,10 +109,10 @@ private:
     GLuint create2DTextureFromImage(const MCSurfaceMetaData & data, const QImage & image);
 
     //! Helper to set surface meta data.
-    void createSurfaceCommon(MCSurface & surface, const MCSurfaceMetaData & data);
+    void createSurfaceCommon(std::shared_ptr<MCSurface> surface, const MCSurfaceMetaData & data);
 
     //! Map for resulting surface objects
-    typedef std::unordered_map<std::string, MCSurface *> SurfaceHash;
+    typedef std::unordered_map<std::string, std::shared_ptr<MCSurface>> SurfaceHash;
     SurfaceHash m_surfaceMap;
 
     DISABLE_COPY(MCSurfaceManager);

src/game/MiniCore/src/Asset/mcsurfaceobjectdata.cc

@@ -41,7 +41,9 @@ void MCSurfaceObjectData::setDefaultCircleShape(bool state)
     m_defaultCircleShape = state;
 }
 
-bool MCSurfaceObjectData::defaultCirleShape() const
+bool MCSurfaceObjectData::defaultCircleShape() const
 {
     return m_defaultCircleShape;
 }
+
+MCSurfaceObjectData::~MCSurfaceObjectData() = default;

src/game/MiniCore/src/Asset/mcsurfaceobjectdata.hh

@@ -33,7 +33,7 @@ public:
     explicit MCSurfaceObjectData(const std::string & typeId);
 
     //! Destructor.
-    virtual ~MCSurfaceObjectData() {};
+    virtual ~MCSurfaceObjectData();
 
     //! Set the id of the already created MCSurface.
     void setSurfaceId(const std::string & id);
@@ -45,7 +45,7 @@ public:
     void setDefaultCircleShape(bool state);
 
     //! Returns true if default circle shape is wanted.
-    bool defaultCirleShape() const;
+    bool defaultCircleShape() const;
 
 private:
     DISABLE_COPY(MCSurfaceObjectData);