-1

I have an Entity class that all objects (Missiles, Spacesship, Stars, Explosions) inherit from and whenever instances of these classes are created, Entity's constructor adds 'this' to its vector list of all instances, just like this:

std::vector<Entity*> Entity::instances; Entity::Entity() { instances.push_back(this); }

This worked fine, because I could just loop through every instance and execute its method via e->Render() or e->Update() etc.

What is weird is, it works everywhere else but in one specific place. It works here: Core.cpp

for (int i = 0; i < 128; i++) { stars[i] = new Star(); stars[i]->position.x = rand() % (screenWidth + 1); stars[i]->position.y = rand() % (screenHeight + 1); stars[i]->angle = rand() % 360; stars[i]->boundingbox.w = 2 + rand() % 7; stars[i]->boundingbox.h = stars[i]->boundingbox.w; } for (int i = 0; i < 16; i++) { meteorites[i] = new Meteorite(); meteorites[i]->Spawn(); }

BUT here .. it pops 'vector iterators incompatible'

Meteorite.cpp

void Meteorite::TakeDamage() { health -= 5; if (health <= 0) { Missile* explo = new Missile(); explo->position.x = position.x; explo->position.y = position.y; Spawn(); } }

I'm completely clueless - it's not as if I was creating these elements in a different than the usual way.

#EDIT I think this is also important, Meteorite::Update() that detects collision and runs TakeDamage() where the instance is created:

for each (Entity* e in Entity::instances) { if (e == this) { continue; } if (e->entClass == "ENTITY_MISSILE") { Missile* m = (Missile*)e; if (abs(position.x - m->position.x) * 2 < (boundingbox.w + m- >boundingbox.w)) { if (abs(position.y - m->position.y) * 2 < (boundingbox.h + m- >boundingbox.h)) { TakeDamage(); break; } } } }
Improve this question
15
  • 1
    @Netheous -- No, it is not C++. It is some weird syntax that possibly a particular compiler supports. Commented Nov 24, 2017 at 3:40
  • 1
    But it works everywhere else, it worked for creation of objects. -- We have no context whatsoever in where, when, and how these functions are called. That's what's missing in your post, a minimal reproducible example. Commented Nov 24, 2017 at 3:42
  • 1
    Also, this looks like the classic mistake of iterating over a container while changing the container you're iterating over. That is a recipe for disaster. Commented Nov 24, 2017 at 3:49
  • 1
    @Netheous -- What is Spawn? And also, how do we know that your type is Missile, thus doing the unsafe cast here Missile* m = (Missile*)e;? If anything, you should be issuing a dynamic_cast<> to ensure you haven't made an error. Commented Nov 24, 2017 at 4:02
  • 1
    @PasserBy -- Yes, macros are evil. Commented Nov 24, 2017 at 4:07

2 Answers 2

Reset to default
2

Note that when you call push_back on a std::vector, then iterators get invalidated (emphasis mine):

If the new size() is greater than capacity() then all iterators and references (including the past-the-end iterator) are invalidated. Otherwise only the past-the-end iterator is invalidated.

So in your for loop when TakeDamage gets called, and you construct a Missile (which subsequently calls Entity::Entity() and instances.push_back(this)), you are invalidating your iterators.

You either need to loop via indices instead of iterators, or keep track of which objects need TakeDamage() called on them and call them after you're finished iterating, or create a copy of instances that you iterate on.

Note that I've never seen the syntax for each (... in ...) but presumably it's using iterators.

You mention that the code in the question is ran within Meteorite::Update, and you also mention that

This worked fine, because I could just loop through every instance and execute its method via e->Render() or e->Update() etc.

It's VERY likely you're iterating over all instances, calling Update, and within that calling TakeDamage which is invalidating your instances. Calling push_back within TakeDamage while iterating over instances is definitely your issue.

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

8 Comments

Replacing it with what NL628 said below still pops same error.
@Netheous -- If what you're doing is what this answer suggests, then it is the problem. You are altering the container that you're relying on being "stable" in that for syntax. That will not work correctly. As a matter of fact, you cannot alter the container that is specified in the range-based for loop when iterating, but you're doing so.
@PaulMcKenzie I've also followed the suggestion that Tas gave. I've created a bool collided = false; Then inside the for loop, I'm checking whether or not collision happened - if so, set collided to true and break; After for loop - run TakeDamage(); if collided. Same error.
If just calling TakeDamage triggers the error, then consider updating your question to JUST do that, rather than including what is superfluous surrounding code. I stand by my answer that this is your problem, whether or not it's for the reasons I mention. Note in your question you mention that Meteorite::Update calls this code, but by chance is Meteroite::Update being called in a for loop on instances? You are definitely attempting to call push_back while iterating through instances.
No worries. I edited my answer to include this information as well, since these comments may get cleaned up.
|
0

try: for(Entity* e : Entity::instances) instead of your for each thing

Improve this answer

6 Comments

Same. I truly think it's not the issue with object itself, I can create it elsewhere and it works perfectly.
As in what? The error is completely vague and points to no line except code of vector itself.
As in printing out a different number before and after each line of vector manipulation, and seeing the last number which prints out. Then, finding the exact line where the error happens.
try: 'for(Entity* e : Entity::instances)'
I don't think you can change while iterating...try this: for(auto it = Entity::instances.begin(); it!= Entity::instances.end(); it++)
|