   Using vectors in C++
  Hi everyone, newbie game programmer here. As many people before me, I'm starting from the bottom, I already finished the obligatory Tetris(tm) and just moved to the Arkanoid clone, but I'm puzzled about something. Thought you might be able to help me ;)OK, first, I'm using SDL with C++. That being said, in my screen there are a ship, a ball, and a group of blocks. The blocks are stored like this:std::vector<Block> blocks;Block *temp;for (int i = 0; i < LINES_PER_LEVEL; i++){  for (int j = 0; j < BLOCKS_PER_LINE;  j++)  {    temp = new Block(...);    blocks.push_back(*temp);  }}Each block has a draw() method which uses standard SDL functions to blit an image on screen. It works perfectly... until I destroy a block.int Game::logic(){  (...) // This code moves the ball and the ship  static int blockToDelete = -1;  for (int i = 0; i < blocks.size(); i++)  {    // getCollBox returns an SDLRect with the collsion box dimensions,    // and checkCollision returns true if the boxes overlap    if (checkCollision(ball->getCollBox(), blocks.at(i).getCollBox()))    { blockToDelete = i;    }  }  if (blockToDelete != -1)  {    blocks.erase(blocks.begin() + blockToDelete);    blockToDelete = -1;  }}I know, there are better ways to do this, but I prefer to do the optimising once everything works to an acceptable degree. The thing is, every frame, I do the previous logic, then draw the screen like this:for (int i = 0; i < blocks.size(); i++){  blocks.at(i).draw();}(...) // Draw ship and ballbut, for some reason, as soon as a block is destroyed, the program exits. I ran some checks, and the program ends when trying to draw the last block of the vector. Say I have 10 blocks, numbered from 0 to 9, and that I destroy block 5. The program draws blocks 0, 1, 2, 3, 4, 6, 7, 8, and exits when drawing block 9. If I change the code to never draw the last block, it doesn't exit, but starts behaving erratically after the first collision (music stops, doesn't exit, feels similar to entering an infinite loop, but the ship still moves).The draw() method just calls SDL_BlitSurface() with appropiate arguments, that's all.I know I'm being kind of vague here, but after a weekend of thinking this code over and over, I just don't know what could be wrong. Maybe I missed something completely obvious, maybe not. If you guys could help me with this, I'd really appreciate it. Also, if you need any other part of the code, just ask.Thanks in advance, everyone.   
  The STL uses non-pointer types; if you want a container of pointers use boost::ptr_vector<Block> or std::vector<boost::shared_ptr<Block>>.  You can have a std::vector<Block*>, but it won't delete them for you.A few notes:std::vector<Block> blocks;for (int i = 0; i < LINES_PER_LEVEL; i++){  for (int j = 0; j < BLOCKS_PER_LINE;  j++)  {    // temp = new Block(...); // Allocate a Block    // blocks.push_back(*temp); // Push a copy of the Block pointed by temp onto "blocks"    // The location of the allocated memory for temp will be overwritten (forgotten) on the next iteration of this loop    blocks.push_back( Block( ... ) ); // Push a copy of a temporary object onto "blocks"  }}Then to iterate through a vector in looking to delete elements:for ( std::vector<Block>::iterator it = blocks.begin(), end = blocks.end(); it != end; /* no inc */ )  if ( /* collision */ )    it = blocks.erase( it );  else    ++it;Quote:If I change the code to never draw the last block, it doesn't exit, but starts behaving erratically after the first collision (music stops, doesn't exit, feels similar to entering an infinite loop, but the ship still moves).It sounds like you've got some memory leaks, memory buffer overruns, and/or other miscellaneous BadThings(TM).  Could you post some more information?What compiler are you using?;  Well, thanks for the fast reply! And thanks for pointing out that using "new" when adding objects to a vector is a memory leak, I thought vectors called destructors when erasing objects.I just tried your points. When doing the "blocks.push_back( Block( ... ) )" thing, something really weird happened: suddenly, all the blocks were exact copies of the first object I add to the vector! The coordinates were correct, but the collision box coords and the colors were all the same as the first block I created...That led me to believe that my problem was inheritance related. You see, my blocks inherit from a custom Sprite class, which holds basic sprite data. I took all the data from the Sprite class, added it into the Block class and removed all the inheritance. Now everythings works perfectly! Apparently my problem was even more basic that I thought. That's what you get when going to C++ from Java: you make lots of assumptions ;)Well, _fastcall, lots and lots of thanks. Problem solved, to an extent. Now all that remains is figuring out why I was doing the inheritance incorrectly. Let's see...// In sprite.hclass Sprite{  (...)}// In sprite.cppSprite::Sprite(int x, int y){  (...)}// In block.hclass Block : public Sprite{  (...)}// In block.cppBlock::Block(int x, int y, int type) : Sprite(x, y){  (...)}I didn't post this code before because, well, it is EXTREMELY basic, even though I was obviously doing something wrong here. Any ideas?As for the compiler, I'm using MinGW under Windows (I know, I know...). Anything else you need, just ask. And thanks. Really :)PS: What tag do you use for that nifty code embedding effect? Looks great.   ;  Here's another thing you can try (with inheritance):// typedefs make typing easier (among other things...)typedef std::vector<Block*> BlockContainerType; BlockContainerTypeblocks;for (int i = 0; i < LINES_PER_LEVEL; i++)  for (int j = 0; j < BLOCKS_PER_LINE;  j++)    blocks.push_back( new Block(...) ); // Push back a *copy of the pointer* to the Block// When removing a block:for ( BlockContainerType::iterator it = blocks.begin(), end = blocks.end(); it != end; ++it ){  if ( /* collision */ )  { delete *it; // Delete the pointer it = blocks.erase( it ); // Erase the node  }}// On cleanup:for ( BlockContainerType::iterator it = blocks.begin(), end = blocks.end(); it != end; ++it )  delete *it; // Delete the pointerQuote:Original post by ArreisI thought vectors called destructors when erasing objects.They do; it calls Block::~Block() for each element.  It won't, however, delete dynamically allocated objects (i.e. std::vector<Block*>)Quote:Original post by Arreissuddenly, all the blocks were exact copies of the first object I add to the vector! The coordinates were correct, but the collision box coords and the colors were all the same as the first block I created...That led me to believe that my problem was inheritance related. You see, my blocks inherit from a custom Sprite class, which holds basic sprite data. I took all the data from the Sprite class, added it into the Block class and removed all the inheritance. Now everythings works perfectly!There's definitely something beefy with those two classes; I'm not sure it's even possible to have an inheritance related problem since you're storing the most derived class in a vector.  Could you post the source?Quote:Original post by ArreisI didn't post this code before because, well, it is EXTREMELY basic, even though I was obviously doing something wrong here.The code itself fine, but somethings wrong with the code you put in (...)  It could be that you're freeing a SDL_Surface* in a destructor of your sprite during a copy constructor or assignment operation.Quote:Original post by ArreisAs for the compiler, I'm using MinGW under Windows (I know, I know...). Anything else you need, just ask. And thanks. Really :)I suggest you try out Microsoft Visual C++ 2008 Express Edition.  It's got an awesome debugger to help you solve these kinds of problems.Quote:Original post by ArreisPS: What tag do you use for that nifty code embedding effect? Looks great.From the FAQ:  [source][/source][Edited by - _fastcall on May 31, 2009 10:08:21 PM];Quote:PS: What tag do you use for that nifty code embedding effect?That would be [source]...[/source].   ;  All right, here's my source.struct CollisionBox{int x;int y;int w;int h;bool isCircle;};class Sprite{public:Sprite(int x, int y);virtual ~Sprite();virtual void draw();virtual struct CollisionBox* getCollBox();virtual void updateCollBox();protected:SDL_Surface *image;double x, y;struct CollisionBox *collBox;};Sprite::Sprite(int x, int y){image = NULL;this->x = x;this->y = y;collBox = new CollisionBox();}Sprite::~Sprite(){delete(image);delete(collBox);}class Block : public Sprite{public:Block(int x, int y, SDL_Surface *tileset, int type);virtual ~Block();private:int type;void selectColor(SDL_Surface *tileset);};Block::Block(int x, int y, SDL_Surface *tileset, int type) : Sprite(x, y){this->type = type;selectColor(tileset);collBox->x = x + 1;collBox->y = y + 1;collBox->w = 29;collBox->h = 15;collBox->isCircle = false;}Block::~Block(){}That's all the code I can think of that can be related to the problem. I removed declaration and initialization of ints, bools and the like. As for the "selectColor" method, all it does is load an image and assign it to the "image" variable.Any obvious mistake in sight?   ;Quote:Original post by ArreisAny obvious mistake in sight?Here's a small recreation of the problem:#include <iostream>#include <vector>using namespace std;class Foobar{public:int*mPointer;Foobar(){this->mPointer = new int;}~Foobar(){delete this->mPointer;}};int main(){std::vector<Foobar> foos;// ConstructFoobar a;cout << "Foobar " << &a << " has mPointer " << a.mPointer << '\n';// Copy constructFoobar b( a ); cout << "Foobar " << &b << " has mPointer " << b.mPointer << '\n';// std::vector creates a Foobar and copy-constructs it from bfoos.push_back( b ); cout << "Foobar " << &foos.at(0) << " has mPointer " << foos.at(0).mPointer << '\n';} // Oops, a.mPointer gets deleted 3 times!Sample output:Foobar 0022F9A8 has mPointer 000C25C8Foobar 0022F99C has mPointer 000C25C8Foobar 000C26A0 has mPointer 000C25C8This would explain why all of the Blocks had the same CollisionBox.  The simple solution is to provide a copy constructor for your Block, allocating its own CollisionBox (instead of stealing the copy's):class Foobar{public:// ...Foobar( const Foobar& copy ){// Create a new integer and assign it to the value of what copy.mPointer points tothis->mPointer = new int( *copy.mPointer );}};Sample output:Foobar 001DF840 has mPointer 00A425C8Foobar 001DF834 has mPointer 00A426A0Foobar 00A426E0 has mPointer 00A42720The other downside is that you have to do this with your image too, and that's a little tricky.  So far you have an image allocated and loaded for every Block you create.  Not only does this consume more memory than is needed, it also slows down your program as you're loading the images.  There are several things you could try:1)  You could use SDL_CreateRGBSurfaceFrom to create a sub-surface of tileset; it won't copy any pixel data, but be sure to call SDL_FreeSurface when you're done using the sub-surface.  2)  You could store a reference to the tileset surface in your Block, and blit just a portion of it (just don't delete tileset when a Block is done with it [smile])3)  You could have an ImageManager class keep track of the Blocks' images, and giving each Block a weak pointer (i.e. a pointer that doesn't have sole ownership) during its existence.Another note:Use SDL_FreeSurface instead of delete on your SDL surfaces.[Edited by - _fastcall on June 1, 2009 2:00:41 AM]; static int blockToDelete = -1;  for (int i = 0; i &lt; blocks.size(); i++)  {    // getCollBox returns an SDLRect with the collsion box dimensions,    // and checkCollision returns true if the boxes overlap    if (checkCollision(ball-&gt;getCollBox(), blocks.at(i).getCollBox()))    { blockToDelete = i;    }  }  if (blockToDelete != -1)  {    blocks.erase(blocks.begin() + blockToDelete);    blockToDelete = -1;  }Just pointing this out.  In this code, where you have blockToDelete = i:If it's possible to hit 2 blocks at 1 time, you'll probably want to store multiple numbers and then delete them all that way. ORIf it's not possible to hit 2 blocks at 1 time, just break right after blockToDelete = i.  There's no point in going any deeper into the list if you've already found you match. :)   ;  _fastcall: OK, first of all, thanks for replying. I'll start from the end of your post.Of course, I know using a separate image for each and every block is a waste of memory, and I intend to solve that, but since the code should work nonetheless, I prefer to leave that aside until the rest of the logic works.Now for the main topic. If I understand correctly, whatblocks.push_back( Block(...) );does is as follows:1- Calls the normal constructor of the Block class to create a new object.2- Calls the copy constructor on that object, and obtain a duplicate.3- Adds that copy to the end of the vector.So I create new objects, and the push_back() function creates copies of them, which should be ok as long as the destructor for the original object is not called. However, since all the objects are created on local context, they are deleted sooner or later and the copy is left with several inconsistent pointers. Is that correct? Is that what was happening?Holland: I'll probably do the first of your solutions as soon as I get some part of the collision detection to work as I want. Thanks for the tip, anyway :)   ;Quote:Original post by ArreisWell, thanks for the fast reply! And thanks for pointing out that using "new" when adding objects to a vector is a memory leak, I thought vectors called destructors when erasing objects.As already said - they do, however the issue actually comes from the fact that that delete is paired with the placement new to allocate it, all inside the vector. Your new is unmatched.Every new of yours needs a corresponding delete of yours.I think it's time to introduce you to the rule of three."Sprite" should either implement a copy-constructor and assignment operator, or those should both be declared private and left unimplemented. In any code I review, that "should" becomes a "must", otherwise it's a bug IMO. Lately I'm also trying to stress that all three should be removed if they aren't needed.   