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.

Can you please add more of the relevant code? For example, what type is _data? Also, in the top code, is data the same as _data in the later code, or is there an assignment somewhere that we're not seeing? We're being asked to make too many assumptions about your code, in my opinion. — KevenK
– KevenK, Commented Jun 15, 2010 at 20:33
Oh right, I tried to remove as much irrelevant code as I could. The top snippet is in my main rendering loop, and just before that, there's a call to LoadObj (which is the lower 2 snippets) where the returned value is assigned to data as follows: data = LoaderClass.LoadObj("cube.obj"); Both _data and data are of the type ObjData, which stores the loaded .obj. — Ahsan Fayaz
– Ahsan Fayaz, Commented Jun 15, 2010 at 20:44
Though the question was (somewhat) different, you might want to look at the code in an answer I previously posted about reading obj files: stackoverflow.com/questions/2908854/… — Jerry Coffin
– Jerry Coffin, Commented Jun 15, 2010 at 20:44
@usm: I wrote an answer before I saw your comment, but you seem to confirm what I expected with your assignment. I fully admit I am not an expert in this exact topic, but I do believe my answer should help get you where you need to go. As a quick test of this, you could write output with your pointer addresses both within the LoadObj method and then again on the data object after you've assigned it. I suspect you'll find they are not in sync. — KevenK
– KevenK, Commented Jun 15, 2010 at 21:04

KevenK · Accepted Answer · 2010-06-15 20:58:31Z

2

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.

answered Jun 15, 2010 at 20:58

KevenK

3,0414 gold badges28 silver badges34 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Ahsan Fayaz Over a year ago

I think you're right. I don't have a copy constructor or an assignment operator for the ObjData class. Doing a bit of reading tells me they are required for objects that have pointer attributes. This also explains why everything works in the LoadObj function but not outside of it. Outside of LoadObj, the destructor for the original data is called and the copy still points to the same portion of memory, which is now invalid. I will write a copy constructor & assignment operator tomorrow morning and report back. Thanks!

Ahsan Fayaz Over a year ago

I wrote a copy constructor and overloaded the assignment operator, and the issue is gone. Thanks a lot!

John Dibling · Accepted Answer · 2010-06-15 20:55:03Z

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 );

answered Jun 15, 2010 at 20:55

John Dibling

102k34 gold badges192 silver badges335 bronze badges

2 Comments

Ahsan Fayaz Over a year ago

Thanks, I suppose I'll use vectors for most things now. I initially avoided them because I thought they were based on linked lists (I'm sure they are in Java..) but that doesn't seem to be the case. I like the structure of the loop in ObjFace, too. Are there any good resources on how to structure code well/properly? Of course, before all of that I'm going to see if what I have now works after writing the copy constructor and assignment operator as suggested by KevenK. Otherwise I think I will constantly wonder what went wrong.

PhysicalEd Over a year ago

Because you say you are new to C++ I'd strongly recommend reading the Effective C++ books by Scott Meyers. He discusses a lot of the gotcha's in C++ and gives lots of advice on how to program C++ safely, clearly and efficiently. Plus he is a fun author to read. As a matter of fact, in "Effective C++" 2nd Edition, Item 11 says "Declare a copy constructor and an assignment operator for classes with dynamically allocated memory". So there you see a tip about your exact issue. Good luck!

Collectives™ on Stack Overflow

Access Violation reading elements of an array

2 Answers 2

2 Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related