Unknown problem with vector pushback with class variables

[Jutebox5]

Dear all,

I'm writing a simple 2d sprite game in my spare time. I'm fairly experienced in C++ and I'm running into unknown and unexpected problems with a vector of mine. I have looked on forums and around the web and I cannot find any help towards a solution so I am posting here. This is my first post on this forum so if I do something completely taboo... please forgive me.

So here's the problem: The game contains an std::vector called the "EntityList" This is a vector of pointers to all the entities in the game (enemies, PC's, NPC, Chest... etc) The entity List is where all the collision detection is handled, the calls to render and all that happy fun stuff.)

So I wanted to create a "list" of enemies, that would be dynamic, that is to say it would vary in size based on the map the player was on. (either 1 big boss, or 50 little monsters) and I decided that the best way to do it would be a vector. So...

std::vector<CEnemy> EnemyList;

and then the Enemylist would be "pushed" into the entitylist after it is filled up and then as the enemies die, they are taken out of the EntityList. Then when the player goes to a new Map... Take all the remaining enemies out of the Entity List... then clear the Enemylist, and reload the EnemyList (thats the idea at least.)

NOTE: EntityList = vector of pointers to actual objects in "EnemyList." EnemyList is suppose to contain actual CEnemy's.

Ok so here's the code that's going berserk. It is suppose to (while loading the map to be displayed, also...) load information from the .map file in the format of "x:y " where x is the type of enemy and Y is the number of that enemy on the map... To be read from the Enemy.data file later on... Here it is:

//declared in class declaration
std::vector<CEnemy>	EnemyList;	//Enemy List.
//-------------

//Earlier in Game.OnInitilize()
EnemyList.reserve(50);
//---------------

bool CArea::OnLoad(char* File) {
    MapList.clear();
	 
	for (int i = 0; i < Game.EnemyList.size(); i++){
		Game.EnemyList[i].OnCleanup();
	}
	 
	Game.EnemyList.clear();

	int Area_Enemy;   //The Type of Enemy in the area (range 1-255)
	int En_Num;       //# of those enemies
	char TilesetFile[255]; //File name for loading tileset for map.
    
	FILE* FileHandle = fopen(File, "r");
    
    if(FileHandle == NULL) {
		return false;
    }
     
    fscanf(FileHandle, "%s\n", TilesetFile);
     
    if((Surf_Area_Tileset = CSurface::OnLoad(TilesetFile)) == false) {
		fclose(FileHandle);
		return false;
    }

    fscanf(FileHandle, "%d\n", &AreaSize); //Needed for Map Loading
    fscanf(FileHandle, "%d:%d ", &Area_Enemy, &En_Num); //"x:y " X = types of enemies, y = number of enemies of type X
	 
	//Creates and pushes an new Enemy into Enemy List with with ID = Area_Enemy, En_Num times... 
	while (Area_Enemy != -1) {
		CEnemy Etemp;
		Etemp.ID = Area_Enemy;
			for (int i = 0; i < En_Num; i++) {
                                //THIS IS WHERE IT ALL GOES BAD!!!!-----
				Game.EnemyList.push_back(Etemp);
			}
		fscanf(FileHandle, "%d:%d ", &Area_Enemy, &En_Num);
	 }
	 
	 fscanf(FileHandle, "\n");

// THIS IS ALL MAP LOADING SEQUENCES... THIS ALL WORKS FINE (To my knowledge)
     for(int X = 0; X < AreaSize; X++) {
             for(int Y = 0; Y < AreaSize; Y++) {
                     char MapFile[255];
                     
                     fscanf(FileHandle, "%s ", MapFile);

                     CMap tempMap;
                     if(tempMap.OnLoad(MapFile) == false) {
                          fclose(FileHandle);
                          return false;
                     }
                     
                     tempMap.Surf_Tileset = Surf_Area_Tileset;
                     
                     MapList.push_back(tempMap);
             }
             fscanf(FileHandle, "\n");
     }

     // THIS IS END OF MAP LOADING SEQUENCE ---------------------
     
     fclose(FileHandle);
     AreaControl.LoadEnemyList();  //This is where it goes to the Enemy.data file and loads the information based off of the Enemy's ID
     return true;
}

So after debugging a number of times and stepping through the code I've discovered this is what is happening:

(In this example "x:y " from file is, X = 1, y = 5, so EnemyList.size() is suppose to = 5 (which is does) and Enemylist[all].ID is suppose to = 1)

EnemyList's Capacity is ending up = 52 (not that big a concern... why I don't know though... but not that big of a concern at the moment)
EnemyList[0].ID = 1
EnemyList[1].ID = -865262626
EnemyList[2].ID = 2
EnemyList[3].ID = 0
EnemyList[4].ID = 0 ...etc...

So the first one is correct and each one after that is screwed up.

OnGame.EnemyList.push_back(Etemp); Etemp's ID does = 1, but somewhere in the .push_back() it alters or corrupts.

Another thing to note. CEnemy Class does inherit CEntity Class in its definition. I don't know if that will matter or not but...

Also I've come across an error happening later in my code where on std::string StringName I'm getting a "Corruption of the heap" error. I have a gut feeling that the two are related but for the life of me I can't figure out why or how.

Can anybody take a look at this and let me know?

Note: This is a pretty hefty sized program at the moment and before I implemented this "enemylist" code everything worked fine (no symptoms at least) so I've just included that code. If there is any other code you would like to see please let me know and I will gladly post it. Thank you very much.

- Jutebox5

[MikeyBoy]

Without seeing the definition of CEnemy, I can't know for sure, but could the problem be with the copy constructor for CEnemy? When you store something in a vector, what you're actually storing is a copy of that thing, so maybe something is screwing up in the copy constructor.

That debugging information you posted - is that at the point where you first put the CEnemy objects into EnemyList? Or is that at some later point in the code's execution?

[Jutebox5]

CEnemy currently doesn't have a copy constructor. This is for 2 reasons.

#1) From what I've read I don't know that it specifically needs one, the default copy constructor "should" be fine. (That being said, with this error it probably wouldn't be a bad idea.)

#2) I've never really been explained or read any real way to make one. So I don't really know how.

That being said I don't know if that is the problem. The default copy constructor should be fine. I'm aware that vector.push_back() makes a copy, which is why its set up the way it is, to make one instance of Etemp, and then copy it into the vector x number of times.

Do you think in this situation I would really need it? Something (my gut) tells me that's probably not the problem, but then again I don't know what the problem is so who knows what my gut is talking about. (It could just be hungry...)

Also to answer your next question. This debug code is in the game's initialize sequence so this is the first instance of where any CEnemies are being loaded or put into any list for the first time. It doesn't even get into the game before it errors out.

Thank you for the response. Please let me know if you have any other ideas. I'll take another look into copy constructors. If you know a proper way to make them please let me know and I will put it in a see if that works.

-Jutebox

[MikeyBoy]

Have you put debug code immediately after the point where you put the CEnemy objects in the vector? If you can confirm that the vector's contents are correct:

(a) within the loop, after each push_back() call
(b) immediately after the loop

it would at least confirm whether the problem is happening there.

Also, could you post the header for CEnemy?

[Jutebox5]

The debugging is from immediately inside the loop, directly after the .push_back() call. (I believe its line 44 from above) Directly after the .push_back() function copies the Etemp into the vector...

The CEnemy.h is as follows:

#ifndef _CENEMY_H_
    #define _CENEMY_H_
 
#include "CEntity.h"

class CEnemy : public CEntity {
      public:
             CEnemy();
             
	     int ID;

             std::string Name; //Enemy's Name
             int Max_HP;     //Enemy's Maximum Hit Points
             int Cur_HP;     //Enemy's Current Hit Points
             int Max_MP;     //Enemies Maximum Magic Points
             int Cur_MP;     //Enemies Current Magic Points
             int Max_AP;     //Enemies Maximum Ability Points
             int Cur_AP;     //Enemies Current Ability Points
             
             int ATT;		  //The Enemy's Attack Power
             int ATTPercent;  //Attack Percentage
	     int DEF;		  //The Enemy's Defense Power
             int DEFPercent;  //Dodge/Defense Percentage
	     int ATT_SPEED;   //How fast the enemy attacks (How many ticks before it attacks again.)
             int MOV_SPEED;   //How fast the enemy moves, (how many ticks have to happen before it can move again.) range 333 and up, lower = faster
             
             int EXP;         //How many experience point are awarded when its killed
             int GOLD;        //How much gold are rewarded on its defeat
             int ITEM1;       //Common item reward after its defeat
             int ITEM2;       //Uncommon item reward
             int ITEM3;       //Rare item reward 

	     //Movement Variables
	     int LastMove;    //the ticks count the last time the enemy moved


	     //Image and model files:
	     char *PCIMAGE_FILE;
	     char *EIGHTBITFILE;

             bool OnLoad(char* File, int Width, int Height, bool Animate, bool Transparency, int MaxFrames);
             bool OnLoad();
	     void MoveCheck();
	     void Death();
	     void OnLoop();
             void OnRender(SDL_Surface* Surf_Display);
             void OnCleanup();
             void OnAnimate();
             void OnCollision(CEntity* Entity);
             void HPCheck();
};

#endif 

Looking at this (and re-reading the Cplusplus.com article on copy constructors) there might be a problem with the pointer variables and the default copy constructor... maybe? This is a guess.

But then again I wouldn't understand why that would be screwing up the .ID variable, or any of the other. Since later in the code (In the first posting, at the bottom, there is a call to (LoadEnemyList()) What that function does is read information from a file and plug in all of CEnemy's variables based on their ID... so shortly after this code all of CEnemy's variables are filled in... I don't know if that matters, but I thought I should put that in there.

Thanks so much for the help.

-Jutebox

[Jutebox5]

Oh... One more thing...

Since CEnemy inherits CEntity... I'll include CEntity's class definition too.

class CEntity {
      public:
             static std::vector<CEntity*> EntityList;
             
      protected:
                CAnimation      Anim_Control;
                
                SDL_Surface*    Surf_Entity;
      
      public:
             int                posX;             //The Position of the Character, X coordinate, in tiles.
             int                posY;             //The Position of the Character, Y coordinate, in tiles.
             int                Width;         //The Width of the Character in pixels
             int                Height;        //The Height of the Character in pixels
             int                Type;          //Type of Entity
             bool               Dead;          //If the entity is dead
             int                Flags;         //A number of different flags             
             bool               Anim_Bool;     //A bool if this entity has an animation or not
             bool               Trans_Bool;    //A bool if this entity has transparency or not
      
      public:
             CEntity();                        //Constructor Function
             virtual ~CEntity();               //Destructor Function
             
      public:
             virtual bool OnLoad(char* File, int Width, int Height, bool Animate, bool Transparency, int MaxFrames);
             virtual void OnLoop();
             virtual void OnRender(SDL_Surface* Surf_Display);
             virtual void OnCleanup();          //Basically clears the memory of the surface before you delete it.
             virtual void OnAnimate();
             virtual void OnCollision(CEntity* Entity);
      public:
             void OnMove(float MoveX, float MoveY);
      private:
             bool PosValid(int NewX, int NewY);
             bool PosValidTile(MTile* Tile);
             bool PosValidEntity(CEntity* Entity, int NewX, int NewY);  
};

I hope this isn't too much. Thanks so much for the help. I really do appreciate you taking the time here. Thanks so much.

-Jutebox

[MikeyBoy]

Well, those pointers might well be a problem, but like you, I don't see why it would corrupt the ID member.

So, just to check - the debugging code is right here, right?

fscanf(FileHandle, "%d:%d ", &Area_Enemy, &En_Num); //"x:y " X = types of enemies, y = number of enemies of type X
	 
En_Num times... 
//Creates and pushes an new Enemy into Enemy List with with ID = Area_Enemy, En_Num times... 
while (Area_Enemy != -1) {
  CEnemy Etemp;
  Etemp.ID = Area_Enemy;
  for (int i = 0; i < En_Num; i++) {
    //THIS IS WHERE IT ALL GOES BAD!!!!-----
    Game.EnemyList.push_back(Etemp);
    printf("EnemyList[%d].ID = %d\n", i,  EnemyList[i]);
  }
  fscanf(FileHandle, "%d:%d ", &Area_Enemy, &En_Num);
}

I must admit, I'm a little stumped. You're basically reading one line from the file that says "1:5", then creating Etemp, setting its ID to 1, and then copying Etemp 5 times into the vector, without doing anything to change it. And yet the value of ID is somehow being changed.

Wait... you're initially reserving space for 50 elements in EnemyList, and then using push_back() ? You might want to check the documentation on this, but it might be that push_back is adding to the end of those 50 elements, so that you're not creating EnemyList[0] to EnemyList[4], but EnemyList[50] to EnemyList[54]. Worth checking out!

[Jutebox5]

You are absolutely correct about the debugging code.

I actually built it in MS VC++ and stepped through it too with the same result. It makes no sense to me.

I thought about the reserve comment too. (which is honestly why the reserve() statement is in there. I thought it would fix it, but to no avail.)

But with the reserve() statement in respect to it creating instance 50-54 instead of 0-4, Looking at the debug output the final capacity of EnemyList is in fact 52. Not 50 or 54... I don't know why on that either.

While stepping through it in MS VC++ after the loop that calls the EnemyList.push_back(Etemp); the values in the vector Enemylist are actually EnemyList[0-4] and not EnemyList [50-54]. So that's not the problem. But thanks for the thought.

Yeah I have no idea. I have a number of programmer friends (who don't specialize in C++ but who do this for a living) and none of us can figure out the answer. I will try a copy constructor and see if that works but in all honesty I don't know how to do it correctly so it might take some time and I don't know if it will fix it. If you have any information on the correct way to do a copy constructor so I can see if that makes a difference I would greatly appreciate it.

Thank you again for all your help and time.

[Zaita]

Add a debug line after each scanf to ensure your Area_Enemy and En_Num are being set correct.

You should also check the return value of fscanf to ensure it was correct and there wasn't a problem.

[MikeyBoy]

Writing a copy constructor is pretty straightforward. Think of it like a normal constructor, whose only argument is a constant reference to another object of the same type. So:

class MyClass
{
public:
  MyClass(int i, int j);   // Normal constructor
  MyClass(const MyClass& anotherObject);

private:
  int m_i;
  int m_j;
  char* m_array;
} ;

// Normal constructor
MyClass::MyClass(int i, int j)
  : m_i(i)                  // Using an initialiser list - are you familiar with this?
  , m_j(j)
  , m_array(NULL)
{
}

// Copy constructor
MyClass::MyClass(const MyClass& anotherObject)
{
  // The simple types are easy.  You could use an initialisation list here
  // too, but I figured it might be more obvious what I was doing if I 
  // assigned values in the body of the method.
  m_i = anotherObject.m_i;
  m_j = anotherObject.m_j;

  // The pointers are where you have to figure things out.  Do you want them
  // them to point to the same memory as the pointer in the thing you're 
  // copying?  Or do you want to allocate new memory for a new copy.
  // The choice is yours!
  m_array = ?????? ;
}

Back to the problem. When you've been stepping through the code with the debugger, can you see when the ID values of the members of EnemyList are being corrupted? Is it the instant they are pushed onto the vector? Or is it when a later object is added?

Or, to put it another way, when you add a CEnemy object to the vector, is it the object being added that has its ID corrupted? Or is it a previously-added object already in the vector?

And are the contents of Etemp staying correct? Or is it Etemp itself that is getting corrupted somehow, and the push_back is correctly copying values out of a corrupted Etemp?

[Jutebox5]

I added debug code within the loop to print out "Area_Enemy = " and "En_Num = " and got

Area_Enemy = 1
En_Num = 5
Area_Enemy = 1
En_Num = 5
Area_Enemy = 1
En_Num = 5
Area_Enemy = 1
En_Num = 5
Area_Enemy = 1
En_Num = 5

so... yeah that's working... what the hell is going on? Sorry I'm getting frustrated. Thanks for the ideas though...