Deleting from vector with iterator (99% sure this is the way to go)

It does look in order. Are you sure that the last element in the vector is a valid pointer in the first place? It could be the pointer that is incorrect and not the way you're removing it. But I would consider using the standard library algorithms to handle correct removal of objects:
[source]
auto predicate = [](Enemy const *e)->bool {
if(e->getState()) {
delete e;
return true;
} else {
return false;
}
};

v.erase(std::remove_if(v.begin(), v.end(), predicate), v.end());
[/source]
Now you have separated the logic of determining what is being removed (the predicate) and the way elements are removed from the vector (using working standard library algorithms). The predicate, which is your own code, has nothing to do with removing elements. Likewise, the standard algorithms knows how to remove elements correctly but does not need to know how to determine what elements to remove.

If you're not into C++11 then you can provide a regular function or a function object instead of a lambda object for the predicate instead.

Furthermore, I would suggest pointer-containers instead of value-containers: boost::ptr_vector<Enemy> or std::vector<scoped_ptr<Enemy> will automatically release the pointer so you don't have to do it yourself.

Thanks for the reply.

This is how i create my enemies atm

/***In .h File***/
#include "enemy.h"
vector<Enemy*> enemyVector;

/***In .cpp File***/
//create a enemy (int hpnumb, int x, int y, int w, int h, int sheetnumb, int tilenumb)
enemyVector.push_back(new Enemy(40, 384, 64, 64, 64, 1, 0));
enemyVector.push_back(new Enemy(40, 384, 128, 64, 64, 1, 1));


But what does the vector contain when you try to remove the last element? That's where it's crashing. Use the debugger to inspect the vector when it crashes and see what it contains and why it crashes on the last one and not any of the other elements in the vector.

By changing some code in my imageloader class it's not crashing anymore... Weird, i'm guessing i screwed up somewhere, will probably pick it up sometime x)

But what does the vector contain when you try to remove the last element? That's where it's crashing. Use the debugger to inspect the vector when it crashes and see what it contains and why it crashes on the last one and not any of the other elements in the vector.

I've actually never debugged in codeblocks and i have no idea how it works. Do you know any tutorials i can learn from? Since if i knew how to debug properly a lot of these small mistakes could be easily solved.

I just searched for codeblocks and debugging and got quite a few results. Looks like it works pretty much like the Visual Studio debugger, and I would assume like most other visual debuggers as well.

This is not the canonical method of erasing elements from a vector. Take a look at remove_if in algorithms. The typical problem is that the iterator is invalid after the erase.

iterator it = remove_if(enemyVector.begin(), enemyVector.end(), YourPredicate);

Then use this new iterator to visit the removed elements to delete them. I would then revisit to erase all in one block.

for_each(it, enemyVector.end(), DeleteFunction);
enemyVector.erase((it, enemyVector.end());

Three functions calls, two simple (inline) functions.

Bill Door, that is a similar but more elongated version of what Brother Bob was talking about.
Why make a special delete function and use a for_each? that seems a waste of time, since you can just do the delete in the predicate function.

this is what i did on a homework assignment where i had to delete dead game actors

del_func (takes object type or type pointer) { if (dead) delete}

iterator newEnd = remove_if (vec.begin(), vec.end(), del_func);

vec.erase(newEnd, vec.end());



again this is all similar to what Brother Bob already mentioned

I just searched for codeblocks and debugging and got quite a few results. Looks like it works pretty much like the Visual Studio debugger, and I would assume like most other visual debuggers as well.

I've actually never debugged before and when i read a tutorial earlier today i didn't not get all of the words used. Guess i'll have to find one that expects no previous knowledge of the subject.

Above solutions should all work.. but if order doesn't matter and extra speed is warranted:

// If order doesn't matter, swap to back and and pop. This wont move the
// elements after current iterator during erase, but the order of elements
// will change.
for (vector<Enemy*>::iterator it = enemyVector.begin(); it != enemyVector.end(); )
{
if ((*it)->getState())
{
delete *it;
// Swap to back and pop, and delete it from there, but don't swap the last element.
if (enemyVector.size() > 1)
std::swap(*it, enemyVector.back());
enemyVector.pop_back();
}
else
{
++it;
}
}



Edit: Disregard that, I'll edit another solution (different from above)
Edit2: there.

Since all you want to do is delete each elements and clear the array, split it to two:

for (auto it = enemyVector.begin(), it_end = enemyVector.end(); it != it_end; ++it)
{
delete *it;
}
enemyVector.clear();


Edit: Disregard that, I'll edit another solution (different from above)

I don't want to clear the vector, only kill of those enemies which are flagged as dead, but i think the reason it crashed before is because of sloppy code. This is my first ever game and i am trying to make it as much as a engine as possible (gotten pretty far as well), but there is bound to be some problems here and there.

I am by no means an expert on debugging, nor am I saying "How do you not know debugging!!!!" as, trust me, I too was once in your shoes.

Once you understand that debugging shows you what your variables ACTUALLY contain, or which memory address is ACTUALLY being pointed too, as opposed to what you think is happening, you will learn more about coding than hacking away at previous functions that may, or may not, work as to how you expect.

The main thing I can say is that it is great at helping you identify a point in your code where you may have forgotten to initialise an attribute/variable or a pointer with a default value. This may sound like a silly thing in a program slightly bigger than your basic "Hello World" but when it come to multiple classes throwing data at each other, you might have a check in one class for an integer value of 0 - 5, but the class that supplies this data hasn't been initialised at all, so it will never compare and execute when you want.

E.g.


class A{
void method_A(int paramA)
{
if(paramA >= 0 && paramA <= 5)
{
//execute one
}
else if(paramA > 5)
{
//execute another
}
}
}
main(){
int test;
A();
A.method_A(test);


This may lead to undesired results. This would be easy to spot for such a short program, but imagine having lines and lines of code where you might be unable to spot such a silly error, using the Debugger will allow you to put a break point in the code. This will halt execution at the point of break. If you break when the Integer 'test' is first declared, and step over it, you will see in your locals window that it will represent the value -854637382 (I cannot remember the number exactly, but its the maximum negative value for said data type). From that, you will know that 'test' was never initialised, and the method of Class A will never run as expected.

This is the simplest way I can show you the benefits of using a debugger. If anyone with more experience thinks this incorrect, then I do apologise if I have given an incorrect explanation, or my example isn't up to scratch.

With regards to your original question, I had a similar problem, and I will look for the program that I personally overcame it in. When I do find it, I will post here with an explanation of where I went wrong, and how it can be applied to your issue.

Regards,

Stitchs.

Also, quick question, what Graphics library are you using, because when you delete your enemy you never seem to free the memory associated with it's image. I know this may not be causing the issue with you crash, but it could lead to a number of memory leaks if not cleaned up properly. I'll make use of Allegro for this example as it is the one I'm most familiar with:


Delete_Function(Enemy*& enemy_to_be_deleted){
destroy_bitmap(enemy_to_be_deleted->image);
delete enemy_to_be_deleted;
enemy_to_be_deleted = NULL;
}


This is the order in which the image attribute of said enemy should be deleted. The pointer should then be set to Null so that no undesired results occur.

Also, quick question, what Graphics library are you using, because when you delete your enemy you never seem to free the memory associated with it's image. I know this may not be causing the issue with you crash, but it could lead to a number of memory leaks if not cleaned up properly. I'll make use of Allegro for this example as it is the one I'm most familiar with:


Delete_Function(Enemy*& enemy_to_be_deleted){
destroy_bitmap(enemy_to_be_deleted->image);
delete enemy_to_be_deleted;
enemy_to_be_deleted = NULL;
}


This is the order in which the image attribute of said enemy should be deleted. The pointer should then be set to Null so that no undesired results occur.

I am using SDL. And i have a imageloader class which is very basic at the moment (going to make it more dynamic when needed). At the moment it loads in a sheet with multiple enemydesigns on it and then i cut out which enemy to be shown. When i add animations i'm sure to change all of this. But at the moment there shouldn't be any memory leak.

I've actually never debugged before and when i read a tutorial earlier today i didn't not get all of the words used. Guess i'll have to find one that expects no previous knowledge of the subject.

I dont know anything about codeblocks. I'll give explain roughly couple most used words in debuggers.

Breakpoint is some line in source code where you want to break in debugger. You can set them to anywhere in IDE. You can also put "assert(false);" in the sourcecode to trigger assertion failure, then clicking Retry in the dialog should break the debugger at that line.

Stepping in code means you can step line by line, and observe how every variable changes. You can also step in and out of functions.

Callstack shows you what function called what function, and you can "jump back in time" to inspect all variables from the callee.

Almost always develope with debug builds, make sure you're not compiling release.

I did notice one thing, which I don't think has been picked up on here yet (apologies if it has). The below portion of your code:

if((*it)->getState())
{
delete *it;
it = enemyVector.erase(it);
}

What does getState() return, I am assuming a boolean for the moment as you say it cycles through and deletes everything until it errors on the last element. What are you checking getState() against because, if it is a boolean, then it will erase every time that getState() returns true, which may be what you want if it only represents ALIVE as being true or false, which in any case should be checked against a second boolean attribute, for clarity. Also, the naming of the method seems ambiguous, and may lose meaning when you go away and come back to the code at a later date.

If it is a numerical data type, like an Integer, then it will delete an enemy every time the loop is called, as once the 'State' is initialised, it will always evaluate to true. The only time, to my knowledge, that this isn't the case is when the 'State' is either not initialised (this will lead to a runtime check failure), or initialised to NULL or 0 (this will prevent the conditional check from evaluating true). In every other case, whether 'State' is 1 or -9999, this will always lead to your delete code being run, which could lead to some if not all of your enemies 'dying' prematurely.

I hope this is of help,

Regards,

Stitchs.

I did notice one thing, which I don't think has been picked up on here yet (apologies if it has). The below portion of your code:

[quote name='Tallkotten' timestamp='1342792260' post='4961322']
if((*it)->getState())
{
delete *it;
it = enemyVector.erase(it);
}

What does getState() return, I am assuming a boolean for the moment as you say it cycles through and deletes everything until it errors on the last element. What are you checking getState() against because, if it is a boolean, then it will erase every time that getState() returns true, which may be what you want if it only represents ALIVE as being true or false, which in any case should be checked against a second boolean attribute, for clarity. Also, the naming of the method seems ambiguous, and may lose meaning when you go away and come back to the code at a later date.

If it is a numerical data type, like an Integer, then it will delete an enemy every time the loop is called, as once the 'State' is initialised, it will always evaluate to true. The only time, to my knowledge, that this isn't the case is when the 'State' is either not initialised (this will lead to a runtime check failure), or initialised to NULL or 0 (this will prevent the conditional check from evaluating true). In every other case, whether 'State' is 1 or -9999, this will always lead to your delete code being run, which could lead to some if not all of your enemies 'dying' prematurely.

I hope this is of help,

Regards,

Stitchs.
[/quote]

getState() returns the boolean dead from the enemy. Which is false as long as it's health is above 0, so i doubt the problem lies here

Okay, that's what I thought. I wasn't suggesting it was related to the current problem at hand, what I was saying is that, if it were representing the 'boolean dead', then maybe it should be renamed to reflect that: maybe getDead() or getAlive() or isDead(), something along these lines.

I only say this as, when I read something like getState, it would refer to more than just an entity being alive, or dead in this case. getState sounds more likely to refer to a number of states, depending on the program of course: alive, patrol, attack, defend, flee, dead... n. The list could go on. Of course, it is down to personal preference of the developer, I am only suggesting this as a way of assisting your code to have a better sense of clarity/be more manageable in the long run.

Regards,

Stitchs.

Yeah, currently i'm only focusing on making the functions work. May have to rename that function later on though since you are totally right

Wierd no one thought to check if the vector wasnt empty before running that delete state...

a simple

if(!vector.empty())
{
do delete code stuff
}

It sounds like its trying to access a element that doesnt exist, and it never hurts ( at least I dont think it does ) to check if a container is empty before doing stuff.