Access Violation reading elements of an array

Question

I've written my own code to parse an .obj model file - essentially just ASCII text. The file gets parsed and stored in the class correctly according to my tests. I can read back the values (from data members) just fine in the loading function.

The problem occurs when I try to read back the values in my main rendering loop. There is an access violation error on the line beginning "int v":

 for(int i = 0; i<data.numFaces; i++){
  for(int j = 0; j<3; j++){ //Assuming triangles for now.

   int v = data.faceList[i].vertex[j]; // Access violation here.
   double vX = data.vertexList[v].x;
   double vY = data.vertexList[v].y;
   double vZ = data.vertexList[v].z;
   glVertex3d(vX, vY, vZ);
  }
 }

I'm not exactly sure why this happens, and I've checked everything I could possibly think of. I'm not very experienced in C++. Most of my programming experience is in Java, Python and PHP although I have previously written a medium sized project in C++.

I'm sure the problem is something basic related to memory allocation or pointers used for the dynamic arrays.

Here are the relevant parts of code in the obj loading class:

ObjData ObjLoader::LoadObj(std::string filename){

    //... Initalization ...

 // 1st pass: find number of elements so array sizes can be defined.
 while(!file.eof()){
  //...
 }

 //...close file...

 _data.faceList = new ObjFace[_data.numFaces];
 _data.vertexList = new ObjVert[_data.numVertices];
 _data.uvList = new ObjUV[_data.numUVcoords];
 _data.normalList = new ObjNormal[_data.numNormals];

    //TODO: Make size dynamic according to each face. Just use the first 3 points for now.
 for (int i = 0; i < _data.numFaces; i++){
  _data.faceList[i].vertex = new int[3];
  _data.faceList[i].normal = new int[3];
  _data.faceList[i].uv = new int[3];
 }

 //... file stuff ...

 // 2nd pass: read values into arrays.
 while(!file.eof()){
  //...

  if(type=="v"){
   _data.vertexList[currentVertex].x = atof(param1.c_str());
   _data.vertexList[currentVertex].y = atof(param2.c_str());
   _data.vertexList[currentVertex].z = atof(param3.c_str());
   currentVertex++;
  }else if(type=="vt"){
   _data.uvList[currentUV].u = atof(param1.c_str());
   _data.uvList[currentUV].v = atof(param2.c_str());
   currentUV++;
  }else if(type=="vn"){
   _data.normalList[currentNormal].x = atof(param1.c_str());
   _data.normalList[currentNormal].y = atof(param2.c_str());
   _data.normalList[currentNormal].z = atof(param3.c_str());
   currentNormal++;
  }else if(type=="f"){
  //...Within loop over each vertex in a single face ...

        if(endPos != string::npos){
        // Value before 1st "/" (Vertex index).
        // ...find value in string...
        _data.faceList[currentFace].vertex[i] = atoi(token.c_str()) -1; // File format begins indices from 1.

        // Value between slashes (UV index).
        // ...find value in string...
        _data.faceList[currentFace].uv[i] = atoi(token.c_str()) -1;

        // Value after 2nd "/" (Normal index).
        // ...find value in string...
        _data.faceList[currentFace].normal[i] = atoi(token.c_str()) -1;
   }
//...End of loop over every vertex in a single face...
currentFace++;
}

}

 return _data;

    }

And the structs ObjFace, ObjVert, ObjUV and ObjNormal are defined as:

    struct ObjVert{
       float x, y, z;
    };

    struct ObjUV{
      float u, v;
    };

    struct ObjNormal{
       float x, y, z;
    };

    // Contains indexes.
       struct ObjFace{
       int* vertex;
       int* uv;
       int* normal;
    };

Thanks for any help. Also, any good sources on avoiding these types of errors in future would be appreciated.

Answer 1

I had typed up some silly response which I realized wasn't right...but I had to keep thinking on it, and I came up with another idea.

Where is this object being assigned?

Your code isn't clear if data and _data are the same object or not, but I noticed that your method there seems to return _data as an object. I'm led to believe that perhaps you are using an assignment like ObjData data = LoadObj("myfilename"); somewhere?

If this is the case, I believe your problem may come from a lack of a copy constructor or overloaded assignment operator for your ObjData class. (I'm not a C++ guru, so I don't recall exactly which one this would fall under. Hopefully someone else can confirm if we're on the right track).

If your pointers are not being copied correctly during the copy and assignment (returning from LoadObj calls a copy constructor iirc, and then the obvious assignment to data), then even though you intended to already have an array of int at that location, you may in fact be accessing uninitialized memory, thus causing your Access Violation.

I'm not an expert with either copy constructors or overloaded assignment operators, but a quick way around this would be to return a pointer to an ObjData rather than to return an object itself.

Answer 2

At first glance I didn't see anything blatantly wrong. But if as you say it is exploding at int v = data.faceList[i].vertex[j]; then it seems very likely that the problem is either i or j or both are too big or too small.

Aside from the obvious approach of getting cozy with your debugger and flattening this bug out, the best approach to solving problems like these is probably to avoid them altogether. Certain things programmers do are more prone to errors than others. The list is long, but you do at least two of them, in spades, in your code.

1) You use dynamically-allocated arrays 2) You use hand-crafted loops

Try to avoid doing these things by using the tools that C++ gives you. Start with #1, and get rid of the dynamically-allocated arrays.

You have a struct:

struct ObjFace{
  int* vertex;
  int* uv;
  int* normal;
};

...with 3 pointers-to-arrays-of-int. Instead of doing that, use a vector:

struct ObjFace{
  vector<int> vertex;
  vector<int> uv;
  vector<int> normal;
};

...and then a whole lot of code you had to write before becomes much simpler now, and much less prone to mistakes:

// all this goes away
//_data.faceList = new ObjFace[_data.numFaces];
//_data.vertexList = new ObjVert[_data.numVertices];
//_data.uvList = new ObjUV[_data.numUVcoords];
//_data.normalList = new ObjNormal[_data.numNormals];

...and:

// now you ask the vector how many elements it really has
for(int i = 0; i<data.faceList.size(); i++){
  for(int j = 0; j<data.faceList.size(); j++){ //Ask the vector instead of assuming triangles

   int v = data.faceList[i].vertex[j]; // Access violation here.
   double vX = data.vertexList[v].x;
   double vY = data.vertexList[v].y;
   double vZ = data.vertexList[v].z;
   glVertex3d(vX, vY, vZ);
  }
 }

Now, look at that loop. Loops are a very common source of errors. The best loop is the loop you never have to write. So use the STL's algorithms instead. Add a function to ObjFace to execute glVertex3d on each of it's elements:

struct ObjFace{
//... 
  void do_vertex() const 
  {
    typedef vector<int> ints;
    for( ints::iterator it = vertex.begin(); it != vertex.end(); ++it )
      glVertex3d(it->x, it->y, it->z);
  }
};

...then go back and whittle down that original loop: (psudocode, actual syntax is more complex)

typedef vector<ObjFace> ObjFaces;
for( ObjFaces::iterator it = data.faceList.begin(); it != data.faceList.end(); ++it )
  it->do_vertex();

...or, with a little more effort:

  for_each( data.faceList.begin(), data.faceList.end(), &ObjFace::do_vertex );