R6025 - pure virtual function call

[DreamCore]

Hi,

I have a very annoying issue. I have made a little Space Invaders clone. I have a vector of enemies:

std::vector<Enemy> enemies_;

When the players "rockets" hit the enemies, I run the following code:

void GameManager::despawnEnemy(Enemy& enemy)
{
   // Unsubscribe the enemy from receving further events
   for (auto &e : enemies_)
   {
      e.removeHandler(&enemy);
   }

   enemies_.erase(std::find(enemies_.begin(), enemies_.end(), enemy));

   score_ += ENEMY_KILL_SCORE;
}

When the enemy has been erased, after a 1-2 sec delay I get the error "R6025 - pure virtual function call". I am not calling any virtual methods from any constructors/destructors so this makes no sense. The Enemy has a class named Entity as its base (abstract base class). For completeness I include their headers plus relevant implementation stuff.

Entity.h:

class Entity
{
private:
	using ENTITY_ID = unsigned;

public:
	Entity(const Entity& e);
	Entity(const ISprite* sprite, Point &position, Point &velocity, double radius);
	virtual ~Entity() { };

	virtual void update(double dt) = 0;
	virtual void render() const;

	void setSprite(const ISprite* sprite);
	void setPosition(Point &position);
	void setPosition(double x, double y);
	void setVelocity(Point &velocity);
	void setVelocity(double x, double y);
	void setRadius(double radius);

	ENTITY_ID getID() const { return id_; };
	const Point& getPosition() const { return position_; };
	const Point& getVelocity() const { return velocity_; };
	double getRadius() const { return radius_; };

	// We must overload the '=' to make sure copied entities gets a new unique ENTITY_ID
	Entity& operator=(const Entity &rhs);

	// Two entities are equal if they share the same ENTITY_ID
	friend bool operator==(const Entity &lhs, const Entity &rhs);
	friend bool operator!=(const Entity &lhs, const Entity &rhs);

private:
	void generateNewID();

	ENTITY_ID id_;
	static ENTITY_ID next_entity_id_;
	
	const ISprite* sprite_;
	Point position_;
	Point velocity_;
	
	double radius_; // Used for circle-based collision detection.
};

bool operator==(const Entity &lhs, const Entity &rhs);
bool operator!=(const Entity &lhs, const Entity &rhs);

Entity.cpp:

Entity::ENTITY_ID Entity::next_entity_id_ = 0;

Entity::Entity(const ISprite* sprite, Point& position, Point& velocity, double radius) :
	position_(position),
	velocity_(velocity),
	id_(next_entity_id_++)
{
	setSprite(sprite);
	setRadius(radius);
}

Entity::Entity(const Entity& e)
{
	operator=(e);
}

void Entity::generateNewID()
{
	id_ = next_entity_id_++;
}

Entity& Entity::operator=(const Entity &rhs)
{
	this->sprite_ = rhs.sprite_;
	this->position_ = rhs.position_;
	this->velocity_ = rhs.velocity_;
	this->radius_ = rhs.radius_;
	this->generateNewID();

	return *this;
}

bool operator==(const Entity &lhs, const Entity &rhs)
{
	return lhs.id_ == rhs.id_;
}

bool operator!=(const Entity &lhs, const Entity &rhs)
{
	return !(lhs == rhs);
}

Enemy.h:

class Enemy : public Entity, public EventHandler, public EventDispatcher
{
public:
	Enemy();
	Enemy(const ISprite* sprite, Point &position, Point &velocity, double radius = ENEMY_RADIUS);

	void update(double dt);

	void handleEvent(const Event &e);

private:
	double last_time_fired_ = 0.0;

	// Has value 1 or -1 and represents the horizontal movement direction
	int direction_multiplier_ = 1;
};

Enemy.cpp:

Enemy::Enemy() :
	Enemy(nullptr, Point(0, 0), Point(0, 0), DEFAULT_RADIUS)
{
}

Enemy::Enemy(const ISprite* sprite, Point &position, Point &velocity, double radius) :
	Entity(sprite, position, velocity, radius)
{
}

All help is greatly appreaciated. Thanks in advance!

/Robin

[Peter87]

Are you storing pointers to the enemy objects somewhere? I mean because when you erase all the element after the erased element will be moved and so pointers to these elements would no longer be valid. Similar problem could occur when adding elements to the vector if the vector need to resize its internal buffer.

[keskiverto]

He also stores objects rather than pointers in a vector. Traditional polymorphism uses pointers. (That avoids slicing too.)

Vector operations, like erase() copy elements. Is it safe to copy Enemies? Are there side effects?

[DreamCore]

Peter87:

Actually the Enemies are EventDispatchers. An event dispatcher looks like this:

EventDispatcher.h:

class EventDispatcher
{
public:
	void addHandler(EventHandler* handler);
	void removeHandler(EventHandler* handler);

protected:
	// Dispatches an event to all subscribed handlers
	void dispatchEvent(const Event& e);

private:
	std::vector<EventHandler*> handlers_;
};

EventDispatcher.cpp:

void EventDispatcher::addHandler(EventHandler* handler)
{
	handlers_.push_back(handler);
}
void EventDispatcher::removeHandler(EventHandler* handler)
{
	handlers_.erase(std::find(handlers_.begin(), handlers_.end(), handler));
}

void EventDispatcher::dispatchEvent(const Event& e)
{
	for (const auto &handler : handlers_)
	{
		handler->handleEvent(e);
	}
}

Since the EventDispatcher store pointers to the enemies, this will be invalid when the enemies are erased and the other enemies change position within the vector? This could be solved by using pointers?

The error still feels kinda weird if this is the case. Wouldn't it just try to access enemies out of range etc.?

keskiverto:

Considering I've overloaded the copy constructor and equality (=) operator in Entity, I think it should be safe to copy considering that Enemy doesn't use any pointers etc. in the class?

Also, before in my GameManager class I had like this:

std::unique_ptr<Player> player_;
std::vector<std::shared_ptr<Enemy>> enemies_;
std::vector<std::unique_ptr<Projectile>> projectiles_;

but after reading that you should use the STACK rather than the HEAP when possible, I changed it to:

Player player_;
std::vector<Enemy> enemies_;
std::vector<std::unique_ptr<Projectile>> projectiles_;

¨

But I guess since both Player, Enemey and Projectile (the Projectile vector stores both Rockets and Bombs so pointers must be used there) uses polymorphism it's perfectly valid (and safer) to use pointers (smart pointers) instead?

[keskiverto]

You have implemented copy constructor and copy assignment for Entity, but not for Enemy. I would not implement copy constructor by using copy assignment.

Besides, each instance of Entity has a unique ID. Therefore,

vector<Enemy> foo;
// assert Entity::next_entity_id_ == x
foo.push_back( Enemy() );
foo.push_back( Enemy() );
// what is Entity::next_entity_id_ now? x+2?  Or more?  Or does the ID even matter?

vector<Enemy*> bar;
// assert Entity::next_entity_id_ == y
bar.push_back( new Enemy );
bar.push_back( new Enemy );
// assert Entity::next_entity_id_ == y+2 

EDIT:

should use the STACK rather than the HEAP when possible

Reference, please.

[DreamCore]

keskiverto:

Regarding the copying I have this:

void Entity::generateNewID()
{
	id_ = next_entity_id_++;
}

Entity& Entity::operator=(const Entity &rhs)
{
	this->sprite_ = rhs.sprite_;
	this->position_ = rhs.position_;
	this->velocity_ = rhs.velocity_;
	this->radius_ = rhs.radius_;
	this->generateNewID();

	return *this;
}

The assignment operator calls generateNewID() so a copied entity never has the same ID, just position, velocity etc. Also I don't think a custom copy constructor/assignment is needed for Enemy since the default one will suffice? The only reason I implemented it on Entity was to handle ENTITY_ID properly. The ID is used for comparing two entities using the == and != operators (I have overloaded them), if more than one had the same ID it would be disastrous.

Regarding stack/heap:

http://stackoverflow.com/questions/599308/proper-stack-and-heap-usage-in-c

The post by MarkR (which got 21 upvotes):

Store it on the stack, if you CAN.
Store it on the heap, if you NEED TO.

[ne555]

> When the enemy has been erased, after a 1-2 sec delay I get the error
backtrace
http://www.cplusplus.com/forum/general/112111/


> We must overload the '=' to make sure copied entities gets a new unique ENTITY_ID
> Two entities are equal if they share the same ENTITY_ID
confusing.

A = B;
A == B; //false 

[keskiverto]

However, then std::vector has the data in the heap (or free store), so the point is moot.

More than one with same ID is clearly unacceptable, but changing the ID of Entities simply because you do something with some other, unrelated Entity, is not intuitive.


Constructors:

Entity::Entity( const Entity &rhs )
 : id_( 0 ), sprite_( rhs.sprite_ ), position_( rhs.position_ ),
   velocity_( rhs.velocity_ ), radius_( rhs.radius_ )
{
  generateNewID();
}

Enemy::Enemy()
 : Entity( nullptr, Point(0, 0), Point(0, 0), DEFAULT_RADIUS )
{
}
// Or, use default parameters so that the four parameter Enemy constructor can be called as Enemy() 

[DreamCore]

For reference, I solved the crash by having:

std::vector<std::shared_ptr<Enemy>> enemies_;

instead of:

std::vector<Enemy> enemies_;

I also rewrote the despawnEnemy function from:

void GameManager::despawnEnemy(Enemy& enemy)
{
   // Unsubscribe the enemy from receving further events
   for (auto &e : enemies_)
   {
      e.removeHandler(&enemy);
   }

   enemies_.erase(std::find(enemies_.begin(), enemies_.end(), enemy));

   score_ += ENEMY_KILL_SCORE;
}

to:

void GameManager::despawnEnemy(shared_ptr<Enemy> enemy)
{
	// Unsubscribe the enemy from receving further events
	for (auto &e : enemies_)
	{
		e->removeHandler(enemy.get());
	}

	enemies_.erase(std::find_if(enemies_.begin(), enemies_.end(), [enemy](shared_ptr<Enemy> e) { return *e == *enemy; }));

	score_ += ENEMY_KILL_SCORE;
}

(It was quite exciting to throw a lambda in there!)

[ne555]

> handlers_.erase(std::find(handlers_.begin(), handlers_.end(), handler));
¿What if 'find()' returns 'end()'?


> For reference, I solved the crash by having:
> std::vector<std::shared_ptr<Enemy>> enemies_;
I'm going to guess that you maintained pointers to enemies, those become invalid when the vector does a reallocation