Some assumptions/definitions:
objects
a member variable, something like vector<Object*> objects;
p
is also a member variable, something like vector<Object*>::iterator p;
So p
is an iterator, *p
is an Object pointer, and **p
is an Object.
The problem is that this method:
void Engine::killUpdate(std::string command) {
if ((*p)->getIsDead() == true) {
delete *p;
}
}
deallocates the Object pointed to by *p
, the pointer in the vector at the position referenced by the p
iterator. However the pointer *p
itself is still in the vector, now it just points to memory that is no longer allocated. Next time you try to use this pointer, you will cause undefined behavior and very likely crash.
So you need to remove this pointer from your vector once you have deleted the object that it points to. This could be as simple as:
void Engine::killUpdate(std::string command) {
if ((*p)->getIsDead() == true) {
delete *p;
objects.erase(p);
}
}
However, you are calling killUpdate
from update
in a loop that iterates over the objects
vector. If you use the code above, you will have another problem: once you erase p
from the objects
vector, it is no longer safe to execute p++
in your for-loop statement, because p
is no longer a valid iterator.
Fortunately, STL provides a very nice way around this. vector::erase
returns the next valid iterator after the one you erased! So you can have the killUpdate
method update p
instead of your for-loop statement, e.g.
void Engine::update(string command) {
if (getGameOver() == false) {
for (p = objects.begin(); p != objects.end(); /* NOTHING HERE */) {
// ...
killUpdate(command);
}
}
}
void Engine::killUpdate(std::string command) {
if ((*p)->getIsDead() == true) {
delete *p;
p = objects.erase(p);
} else {
p++;
}
}
This is of course assuming that you always call killUpdate
in the loop, but I'm sure you can see the way around this if you don't -- just execute p++
at the end of the for-loop body in the case that you haven't called killUpdate
.
Also note that this is not particularly efficient, since every time you erase an element of the vector, the elements that follow it have to be shifted back to fill in the empty space. So this will be slow if your objects
vector is large. If you used a std::list
instead (or if you are already using that), then this is not a problem, but lists have other drawbacks.
A secondary approach is to overwrite each pointer to a deleted object with nullptr
and then use std::remove_if
to remove them all in one go at the end of the loop. E.g.:
void Engine::update(string command) {
if (getGameOver() == false) {
for (p = objects.begin(); p != objects.end(); p++) {
// ...
killUpdate(command);
}
}
std::erase(std::remove_if(objects.begin(), objects.end(),
[](const Object* o) { return o == nullptr; }),
objects.end());
}
void Engine::killUpdate(std::string command) {
if ((*p)->getIsDead() == true) {
delete *p;
*p = nullptr;
}
}
The assumption this time is that you will never have a nullptr
element of objects
that you want to keep for some reason.
Since you seem to be a beginner, I should note that this:
std::erase(std::remove_if(objects.begin(), objects.end(),
[](const Object* o) { return o == nullptr; }),
objects.end());
is the erase-remove idiom, which is explained well on Wikipedia. It erases elements from the vector if they return true when a given function object is called on them. In this case, the function object is:
[](const Object* o) { return o == nullptr; }
Which is a lambda expression and is essentially shorthand for an instance of an object with this type:
class IsNull {
public:
bool operator() (const Object* o) const {
return o == nullptr;
}
};
One last caveat to the second approach, I just noticed that you have another loop over objects
in scrollUpdate
. If you choose the second approach, be sure to update this loop to check for nullptr
s in objects
and skip them.