2
\$\begingroup\$

My game uses an entity manager and entities to represent everything including the player, enemes, items, ..everything in the game. In my engine it's the responsibility of every entity to update itself.

Sometimes an entity finds out it has expired somehow, and thus needs to remove itself from the entity manager. My question is: Is it safe to have an object delete itself? I will illustrate what I mean.

void EntityManager::remove_entity(Entity* entity) {
    m_entities.erase(std::remove_if( //...
}

void Entity::update() {
    if (has_expired)
        m_manager.remove_entity(this); // <- safe?
}

I can't concretely prove a bug by using this, but it kind of looks and feels like it should at least be undefined behaviour. So is it OK? If not, what is a common practice for dealing with this situation?

\$\endgroup\$
  • \$\begingroup\$ Yes, I believe it's safe. Usually in this situation you'd leave freed pointers somewhere, though. \$\endgroup\$ – Ben Jan 16 '15 at 1:36
  • \$\begingroup\$ @Ben What do you mean exactly? \$\endgroup\$ – Anonymous Entity Jan 16 '15 at 1:44
  • \$\begingroup\$ What type is m_entities? Is Entity::update called while iterating over m_entities? \$\endgroup\$ – user253751 Jan 16 '15 at 3:20
  • \$\begingroup\$ It's a vector of std::shared_ptr<Entity>. Yes, it is call when iterating m_entities. \$\endgroup\$ – Anonymous Entity Jan 16 '15 at 11:06
1
\$\begingroup\$

You could create a pool at the manager which will clean up all after or before the EntityManager::update. This way it is save. using 'delete this' will raise big issues amongst a lot of developers.

std::vector m_garbage;
void EntityManager::collect(Entity* entity) {
    entity->collected = true;
    garbage.push_back(entity);
}
void EntityManager::update(..) {
    // erase all in m_garbage
}
\$\endgroup\$
0
\$\begingroup\$

It depends on what remove_entity does. If you're freeing some memory then you shouldn't access the this pointer after calling remove_entity. If you're just marking the entity as "to be deleted later", then you can do whatever you want with it.

Usually what happens is entities add themselves to a buffer, and at the end of the game's update loop the manager loops over the buffer and deletes all the entities. This would be called delayed destruction, and is usually the appropriate way to do this sort of thing.

\$\endgroup\$
  • 1
    \$\begingroup\$ remove_entity compares the argument to a vector of smart pointers and if a match is found it erases it, freeing the memory. I do not try to use the this pointer after remove entity, but I still just want to know whether the operation is 100% safe, and that I haven't just been getting lucky with undefined behaviour. \$\endgroup\$ – Anonymous Entity Jan 16 '15 at 2:08
  • \$\begingroup\$ @user11177 Yeah of course it's safe, so long as you don't do anything with that freed memory. You can think of the class method as a normal function with an implicitly defined parameter for the this pointer. When you free the entity the this pointer will be pointing to freed memory. If you don't access the this pointer after a free, you're good to go :). But of course, just because it's "safe" may not mean it's a good idea. Whether or not it's a good idea is a different question. \$\endgroup\$ – RandyGaul Jan 16 '15 at 2:51
  • \$\begingroup\$ "It depends on what remove_entity does." - this is stated in the question. \$\endgroup\$ – user253751 Jan 16 '15 at 3:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.