← Back to team overview

kicad-developers team mailing list archive

[PATCH] S3D Mesh Smart Pointer

 

Hi Guys,

Could someone who has access to both vrml1 and vrml2 models please
test the attached patch which uses a smart pointer rather than
S3D_MESH* to get rid of some coverity issues and feedback whether it
makes your computer explode, or carry on working normally?

Alternatively, could someone email a couple of vrml files that would
exercise it and I'll put it through it's paces...

Sorry, there's white space deletions going on in the patch so it's a bit noisy.

Thanks,

Brian.
=== modified file '3d-viewer/3d_mesh_model.cpp'
--- 3d-viewer/3d_mesh_model.cpp	2015-03-29 16:13:58 +0000
+++ 3d-viewer/3d_mesh_model.cpp	2015-04-01 21:58:33 +0000
@@ -68,13 +68,6 @@
 
 S3D_MESH::~S3D_MESH()
 {
-    for( unsigned int idx = 0; idx < childs.size(); idx++ )
-    {
-        if( childs[idx] )
-        {
-            delete childs[idx];
-        }
-    }
 }
 
 
@@ -82,7 +75,7 @@
 {
     if( !m_BBox.IsInitialized() )
         calcBBoxAllChilds();
-    
+
     return m_BBox;
 }
 
@@ -100,7 +93,7 @@
     // Calc transformation matrix
     glm::mat4 fullTransformMatrix;
     glm::mat4   translationMatrix       = glm::translate( glm::mat4(),       m_translation );
-    
+
     if( m_rotation[3] != 0.0f )
     {
         glm::mat4   rotationMatrix      = glm::rotate(    translationMatrix, glm::radians( m_rotation[3] ),
@@ -110,7 +103,7 @@
     else
         fullTransformMatrix = glm::scale(     translationMatrix, m_scale );
 
-    
+
     // Apply transformation
     m_BBox.Set( S3D_VERTEX( fullTransformMatrix * glm::vec4( tmpBBox.Min(), 1.0f ) ),
                 S3D_VERTEX( fullTransformMatrix * glm::vec4( tmpBBox.Max(), 1.0f ) ) );
@@ -243,7 +236,7 @@
             if ( m_MaterialIndex.size() > idx )
             {
                 bool isTransparent = m_Materials->SetOpenGLMaterial( m_MaterialIndex[idx], useMaterial );
-                
+
                 if( isTransparent && aIsRenderingJustNonTransparentObjects )
                     continue;
 
@@ -652,7 +645,7 @@
                             l,
                             (unsigned int)m_CoordIndex[idx].size()) );
                     */
-    
+
                     if( ( cross_prod.x > cross_prod.y ) && ( cross_prod.x > cross_prod.z ) )
                     {
                         cross_prod.x = 0.0f;
@@ -713,7 +706,7 @@
     for( unsigned int each_face_A_idx = 0; each_face_A_idx < m_CoordIndex.size(); each_face_A_idx++ )
     {
         glm::vec3 initVertexFaceNormal = m_PerFaceNormalsRaw_X_PerFaceSquaredArea[each_face_A_idx];
-        
+
         std::vector< glm::vec3 >& face_A_normals = m_PerFaceVertexNormals[each_face_A_idx];
 
         for( unsigned int each_vert_A_idx = 0; each_vert_A_idx < m_CoordIndex[each_face_A_idx].size(); each_vert_A_idx++ )
@@ -722,7 +715,7 @@
         }
     }
 
-    
+
     #ifdef USE_OPENMP
     #pragma omp parallel for
     #endif /* USE_OPENMP */
@@ -732,7 +725,7 @@
     {
         // n = face A facet normal
         std::vector< glm::vec3 >& face_A_normals = m_PerFaceVertexNormals[each_face_A_idx];
- 
+
         // loop through all vertices
         // for each vert in face A
         for( unsigned int each_vert_A_idx = 0; each_vert_A_idx < m_CoordIndex[each_face_A_idx].size(); each_vert_A_idx++ )

=== modified file '3d-viewer/3d_mesh_model.h'
--- 3d-viewer/3d_mesh_model.h	2015-03-29 10:59:53 +0000
+++ 3d-viewer/3d_mesh_model.h	2015-04-10 22:50:03 +0000
@@ -24,12 +24,14 @@
 
 /**
  * @file 3d_mesh_model.h
- * @brief 
+ * @brief
  */
 
 #ifndef __3D_MESH_MODEL_H__
 #define __3D_MESH_MODEL_H__
 
+#include <memory>
+#include <boost/shared_ptr.hpp>
 #include <vector>
 #define GLM_FORCE_RADIANS
 #include <gal/opengl/glm/glm.hpp>
@@ -37,9 +39,14 @@
 #include "3d_material.h"
 #include "CBBox.h"
 
-
 class S3D_MESH;
 
+/** A smart pointer to an S3D_MESH object */
+typedef boost::shared_ptr<S3D_MESH> S3D_MESH_PTR;
+
+/** A container of smar S3D_MESH object pointers */
+typedef std::vector<S3D_MESH_PTR> S3D_MESH_PTRS;
+
 class S3D_MESH
 {
 public:
@@ -60,7 +67,7 @@
     std::vector< S3D_VERTEX >       m_PerFaceNormalsNormalized;
     std::vector< S3D_VERTEX >       m_PerVertexNormalsNormalized;
     std::vector< int >              m_MaterialIndex;
-    std::vector< S3D_MESH * >       childs;
+    S3D_MESH_PTRS                   childs;
 
     S3D_VERTEX  m_translation;
     glm::vec4   m_rotation;

=== modified file '3d-viewer/modelparsers.h'
--- 3d-viewer/modelparsers.h	2015-03-29 16:13:58 +0000
+++ 3d-viewer/modelparsers.h	2015-04-01 23:08:49 +0000
@@ -30,6 +30,7 @@
 #define MODELPARSERS_H
 
 #include <map>
+#include <memory>
 #include <vector>
 #include <wx/string.h>
 #include <3d_mesh_model.h>
@@ -71,11 +72,11 @@
      * @param aFilename = the full file name of the file to load
      * @return true if as succeeded
      */
-    virtual bool Load( const wxString& aFilename ) { 
+    virtual bool Load( const wxString& aFilename ) {
         return false;
     };
 
-    std::vector< S3D_MESH* > childs;
+    S3D_MESH_PTRS childs;
 
 private:
     S3D_MASTER* master;
@@ -127,7 +128,7 @@
 
 private:
     wxString                 m_Filename;
-    S3D_MESH*                m_model;
+    S3D_MESH_PTR             m_model;
 
     std::vector< wxString > vrml_materials;
     std::vector< wxString > vrml_points;
@@ -143,7 +144,7 @@
 
 
 typedef std::map< std::string, std::vector< glm::vec3 > > VRML2_COORDINATE_MAP;
-typedef std::map< std::string, S3D_MESH* > VRML2_DEF_GROUP_MAP;
+typedef std::map< std::string, S3D_MESH_PTR > VRML2_DEF_GROUP_MAP;
 
 /**
  * class VRML2_MODEL_PARSER
@@ -164,7 +165,7 @@
      * @param aTransformationModel a model with translation, rotation and scale to apply to default root
      * @return bool - true if finnished with success
      */
-    bool Load( const wxString& aFilename, S3D_MESH *aTransformationModel );
+    bool Load( const wxString& aFilename, S3D_MESH_PTR aTransformationModel );
 
     /**
      * Return string representing VRML2 file in vrml2 format
@@ -174,7 +175,7 @@
     wxString VRML2_representation();
 
 private:
-    int loadFileModel( S3D_MESH *transformationModel );
+    int loadFileModel( S3D_MESH_PTR transformationModel );
     int read_Transform();
     int read_DEF();
     int read_DEF_Coordinate();
@@ -211,7 +212,7 @@
 
     bool                      m_normalPerVertex;
     bool                      colorPerVertex;
-    S3D_MESH*                 m_model;                  ///< It stores the current model that the parsing is adding data
+    S3D_MESH_PTR              m_model;                  ///< It stores the current model that the parsing is adding data
     FILE*                     m_file;
     wxFileName                m_Filename;
     VRML2_COORDINATE_MAP      m_defCoordinateMap;
@@ -266,7 +267,7 @@
 
     bool                     m_normalPerVertex;
     bool                     colorPerVertex;
-    S3D_MESH*                m_model;
+    S3D_MESH_PTR             m_model;
     FILE*                    m_file;
     wxString                 m_Filename;
     S3D_MODEL_PARSER*        m_ModelParser;

=== modified file '3d-viewer/vrml_v1_modelparser.cpp'
--- 3d-viewer/vrml_v1_modelparser.cpp	2015-03-28 11:33:56 +0000
+++ 3d-viewer/vrml_v1_modelparser.cpp	2015-04-01 22:31:20 +0000
@@ -51,7 +51,7 @@
 {
     m_ModelParser = aModelParser;
     m_Master = m_ModelParser->GetMaster();
-    m_model = NULL;
+    m_model.reset();
     m_file  = NULL;
     m_normalPerVertex = true;
     colorPerVertex = true;
@@ -88,7 +88,7 @@
 
         if( strcmp( text, "Separator" ) == 0 )
         {
-            m_model = new S3D_MESH();
+            m_model.reset( new S3D_MESH() );
             m_ModelParser->childs.push_back( m_model );
             read_separator();
         }
@@ -122,18 +122,18 @@
         }
         else if( strcmp( text, "Separator" ) == 0 )
         {
-            S3D_MESH* parent = m_model;
+            S3D_MESH_PTR parent( m_model.get() );
 
-            S3D_MESH* new_mesh_model = new S3D_MESH();
+            S3D_MESH_PTR new_mesh_model( new S3D_MESH() );
 
             m_model->childs.push_back( new_mesh_model );
 
-            m_model = new_mesh_model;
+            m_model.reset( new_mesh_model.get() );
 
             // recursive
             read_separator();
 
-            m_model = parent;
+            m_model.reset( parent.get() );
         }
         else if( ( *text != '}' ) )
         {

=== modified file '3d-viewer/vrml_v2_modelparser.cpp'
--- 3d-viewer/vrml_v2_modelparser.cpp	2015-04-02 11:18:19 +0000
+++ 3d-viewer/vrml_v2_modelparser.cpp	2015-04-09 21:16:53 +0000
@@ -54,7 +54,7 @@
 {
     m_ModelParser = aModelParser;
     m_Master = m_ModelParser->GetMaster();
-    m_model = NULL;
+    m_model.reset();
     m_file = NULL;
     m_normalPerVertex = true;
     colorPerVertex = true;
@@ -98,7 +98,7 @@
     // Switch the locale to standard C (needed to print floating point numbers)
     LOCALE_IO toggle;
 
-    loadFileModel( NULL );
+    loadFileModel( S3D_MESH_PTR() );
 
     fclose( m_file );
 
@@ -107,7 +107,7 @@
 }
 
 
-bool VRML2_MODEL_PARSER::Load( const wxString& aFilename, S3D_MESH *aTransformationModel )
+bool VRML2_MODEL_PARSER::Load( const wxString& aFilename, S3D_MESH_PTR aTransformationModel )
 {
     if( aTransformationModel )
     {
@@ -141,7 +141,7 @@
 }
 
 
-int VRML2_MODEL_PARSER::loadFileModel( S3D_MESH *aTransformationModel )
+int VRML2_MODEL_PARSER::loadFileModel( S3D_MESH_PTR aTransformationModel )
 {
     char text[BUFLINE_SIZE];
 
@@ -156,9 +156,9 @@
 
         if( strcmp( text, "Transform" ) == 0 )
         {
-            m_model = new S3D_MESH();
+            m_model.reset( new S3D_MESH() );
 
-            S3D_MESH* save_ptr = m_model;
+            S3D_MESH_PTR save_ptr( m_model );
 
             if( read_Transform() == 0 )
             {
@@ -167,12 +167,12 @@
                 if( ((m_model->m_Point.size() == 0) || (m_model->m_CoordIndex.size() == 0)) &&
                      (m_model->childs.size() == 0) )
                 {
-                    delete m_model;
+                    m_model.reset();
                     wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "loadFileModel: skipping model with no points or childs" ) );
                 }
                 else
                 {
-                    if( aTransformationModel )
+                    if( aTransformationModel.get() != NULL )
                     {
                         m_model->m_translation   = aTransformationModel->m_translation;
                         m_model->m_rotation      = aTransformationModel->m_rotation;
@@ -187,14 +187,14 @@
             }
             else
             {
-                delete m_model;
+                m_model.reset();
             }
         }
         else if( strcmp( text, "DEF" ) == 0 )
         {
-            m_model = new S3D_MESH();
+            m_model.reset( new S3D_MESH() );
 
-            S3D_MESH* save_ptr = m_model;
+            S3D_MESH_PTR save_ptr( m_model );
 
             if( read_DEF() == 0 )
             {
@@ -203,12 +203,12 @@
                 if( ((m_model->m_Point.size() == 0) || (m_model->m_CoordIndex.size() == 0)) &&
                      (m_model->childs.size() == 0) )
                 {
-                    delete m_model;
+                    m_model.reset();
                     wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "loadFileModel: skipping model with no points or childs" ) );
                 }
                 else
                 {
-                    if( aTransformationModel )
+                    if( aTransformationModel.get() != NULL )
                     {
                         m_model->m_translation   = aTransformationModel->m_translation;
                         m_model->m_rotation      = aTransformationModel->m_rotation;
@@ -223,14 +223,14 @@
             }
             else
             {
-                delete m_model;
+                m_model.reset();
             }
         }
         else if( strcmp( text, "Shape" ) == 0 )
         {
-            m_model = new S3D_MESH();
+            m_model.reset( new S3D_MESH() );
 
-            S3D_MESH* save_ptr = m_model;
+            S3D_MESH_PTR save_ptr = m_model;
 
             if( read_Shape() == 0 )
             {
@@ -239,12 +239,12 @@
                 if( ((m_model->m_Point.size() == 0) || (m_model->m_CoordIndex.size() == 0)) &&
                      (m_model->childs.size() == 0) )
                 {
-                    delete m_model;
+                    m_model.reset();
                     wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "loadFileModel: skipping model with no points or childs" ) );
                 }
                 else
                 {
-                    if( aTransformationModel )
+                    if( aTransformationModel.get() != NULL )
                     {
                         m_model->m_translation   = aTransformationModel->m_translation;
                         m_model->m_rotation      = aTransformationModel->m_rotation;
@@ -261,7 +261,7 @@
             }
             else
             {
-                delete m_model;
+                m_model.reset();
             }
         }
     }
@@ -279,7 +279,7 @@
             for( VRML2_DEF_GROUP_MAP::iterator groupIt = m_defGroupMap.begin(); groupIt!=m_defGroupMap.end(); ++groupIt )
             {
                 wxString groupName = groupIt->first;
-                S3D_MESH* ptrModel = groupIt->second;
+                S3D_MESH_PTR ptrModel( groupIt->second );
 
 
                 if( ((ptrModel->m_Point.size() == 0) || (ptrModel->m_CoordIndex.size() == 0)) &&
@@ -326,9 +326,9 @@
 
         if( strcmp( text, "Transform" ) == 0 )
         {
-            m_model = new S3D_MESH();
+            m_model.reset( new S3D_MESH() );
 
-            S3D_MESH* save_ptr = m_model;
+            S3D_MESH_PTR save_ptr( m_model );
 
             if( read_Transform() == 0 )
             {
@@ -337,7 +337,7 @@
                 if( ((m_model->m_Point.size() == 0) || (m_model->m_CoordIndex.size() == 0)) &&
                     (m_model->childs.size() == 0) )
                 {
-                    delete m_model;
+                    m_model.reset();
                     wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "read_Transform: skipping model with no points or childs" ) );
                 }
                 else
@@ -352,7 +352,7 @@
             }
             else
             {
-                delete m_model;
+                m_model.reset();
             }
         }
         else if( strcmp( text, "translation" ) == 0 )
@@ -463,14 +463,14 @@
         else if( strcmp( text, "Shape" ) == 0 )
         {
             // Save the pointer
-            S3D_MESH* parent = m_model;
+            S3D_MESH_PTR parent( m_model );
 
-            S3D_MESH* new_mesh_model = new S3D_MESH();
+            S3D_MESH_PTR new_mesh_model( new S3D_MESH() );
 
             // Assign the current pointer
             m_model = new_mesh_model;
 
-            S3D_MESH* save_ptr = m_model;
+            S3D_MESH_PTR save_ptr = m_model;
 
             if( read_Shape() == 0 )
             {
@@ -481,7 +481,7 @@
                     if( ((m_model->m_Point.size() == 0) || (m_model->m_CoordIndex.size() == 0)) &&
                          (m_model->childs.size() == 0) )
                     {
-                        delete m_model;
+                        m_model.reset();
                         wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "read_Transform: Shape, skipping model with no points or childs" ) );
                     }
                     else
@@ -496,14 +496,14 @@
                 }
                 else
                 {
-                    delete m_model;
+                    m_model.reset();
 
                     wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "read_Transform: Shape, discard child." ) );
                 }
             }
             else
             {
-                delete m_model;
+                m_model.reset();
             }
 
             m_model = parent;
@@ -540,7 +540,7 @@
                     return -1;
                 }
 
-                S3D_MESH* ptrModel = groupIt->second;
+                S3D_MESH_PTR ptrModel( groupIt->second );
 
                 if( ((ptrModel->m_Point.size() == 0) || (ptrModel->m_CoordIndex.size() == 0)) &&
                      (ptrModel->childs.size() == 0) )
@@ -778,14 +778,14 @@
             wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "Shape" ) );
 
             // Save the pointer
-            S3D_MESH* parent = m_model;
+            S3D_MESH_PTR parent = m_model;
 
-            S3D_MESH* new_mesh_model = new S3D_MESH();
+            S3D_MESH_PTR new_mesh_model( new S3D_MESH() );
 
             // Assign the current pointer
             m_model = new_mesh_model;
 
-            S3D_MESH* save_ptr = m_model;
+            S3D_MESH_PTR save_ptr = m_model;
 
             if( read_Shape() == 0 )
             {
@@ -794,7 +794,7 @@
                 if( ((m_model->m_Point.size() == 0) || (m_model->m_CoordIndex.size() == 0)) &&
                      (m_model->childs.size() == 0) )
                 {
-                    delete m_model;
+                    m_model.reset();
                     wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "read_DEF: Shape, skipping model with no points or childs" ) );
                 }
                 else
@@ -809,7 +809,7 @@
             }
             else
             {
-                delete m_model;
+                m_model.reset();
             }
 
             m_model = parent;
@@ -824,9 +824,9 @@
             wxLogTrace( traceVrmlV2Parser, m_debugSpacer + wxT( "Group %s" ), tagName );
 
             // Save the pointer
-            S3D_MESH* parent = m_model;
+            S3D_MESH_PTR parent = m_model;
 
-            S3D_MESH* new_mesh_model = new S3D_MESH();
+            S3D_MESH_PTR new_mesh_model( new S3D_MESH() );
 
             // Assign the current pointer
             m_model = new_mesh_model;
@@ -848,7 +848,7 @@
             }
             else
             {
-                delete m_model;
+                m_model.reset();
             }
 
             // Restore current model pointer

=== modified file '3d-viewer/vrmlmodelparser.cpp'
--- 3d-viewer/vrmlmodelparser.cpp	2015-03-28 11:33:56 +0000
+++ 3d-viewer/vrmlmodelparser.cpp	2015-04-01 21:55:31 +0000
@@ -45,14 +45,6 @@
 
 VRML_MODEL_PARSER::~VRML_MODEL_PARSER()
 {
-    for( unsigned int idx = 0; idx < childs.size(); idx++ )
-    {
-        if( childs[idx] )
-        {
-            delete childs[idx];
-            childs[idx] = 0;
-        }
-    }
 }
 
 bool VRML_MODEL_PARSER::Load( const wxString& aFilename )

=== modified file '3d-viewer/x3dmodelparser.cpp'
--- 3d-viewer/x3dmodelparser.cpp	2015-03-28 11:33:56 +0000
+++ 3d-viewer/x3dmodelparser.cpp	2015-04-01 23:12:18 +0000
@@ -52,20 +52,12 @@
 X3D_MODEL_PARSER::X3D_MODEL_PARSER( S3D_MASTER* aMaster ) :
     S3D_MODEL_PARSER( aMaster )
 {
-    m_model = NULL;
+    m_model.reset();
 }
 
 
 X3D_MODEL_PARSER::~X3D_MODEL_PARSER()
 {
-    for( unsigned int idx = 0; idx < childs.size(); idx++ )
-    {
-        if( childs[idx] )
-        {
-            delete childs[idx];
-            childs[idx] = 0;
-        }
-    }
 }
 
 
@@ -102,7 +94,7 @@
          node_it != transforms.end();
          node_it++ )
     {
-        m_model = new S3D_MESH();
+        m_model.reset( new S3D_MESH() );
         childs.push_back( m_model );
 
         readTransform( *node_it );


Follow ups