HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Entity management design & smart pointers

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
designsmartpointersmanagemententity

Problem

I'd like to get some guidance on whether or not I'm approaching my design the correct way, especially with regards to usage of smart pointers and references.

I've started developing a small game engine which among other things consists of Entities, an EntityManager to 'own' the entities, a Quadtree for 2d entity position lookups which will be regenerated each update and a World to encapsulate manager and tree. Here are the relevant bits:

World:

EntityManager mEntityManager;
Quadtree mTree;

void World::Init() {
    // Add some initial entities
    for( int i=0 ; i  entity( new Entity(*this) );
        mEntityManager.AddEntity(&entity);
    }
}

void World::Update() {
    // Rebuild quadtree and update entities
    mTree.clear();
    for( const auto &i : mEntityManager.getEntities() ) {
        mTree.insert(*i.get(), i->position().x, i->position().z ); 
    }
    mEntityManager.update();
}


EntityManager:

std::vector> mEntities;

const std::vector> &getEntities(){ return mEntities; } 

void EntityManager::AddEntity(std::unique_ptr* entity) {
    mEntities.push_back(std::move(*entity));
}


Entity:

World&      mWorld;

Entity(World& world) : mWorld( world ) { };

void Entity::update() {
    // Call world to retrieve a list of
    for( const auto &i : world.getNearbyEntities( this->position, radius ) ) {
        // etc...
    }
}


All currently seems to works fine, but I'd just like to know if I'm going about this the right way or if I'm lining myself up for problems in the future based on how I'm defining/passing data.

EDIT
Okay, big thanks @Morwenn . How does this look, assuming I want to create a new instance and then perhaps perform some operations on the newly created entity. Anything I've misunderstood?

EntityManager:

```
template
Derived& EntityManager::EmplaceEntity(Args&&... args) {
std::unique_ptr ptr( new Derived{std::forward(args)...} );
mEntities.push_back(std::move(ptr));
return dynamic_cast(*(mEntities.back()));

Solution

The fact that AddEntity leaves behind a unique_ptr to a possibly destroyed entity bothers me (right now, moving your entity does not "destroy" it, but you might add destructible components later). In my opinion, AddEntity should not take its argument by pointer but by reference:

void EntityManager::AddEntity(const Entity& entity) {
    mEntities.push_back(entity);
}


You can provide an overload for rvalue-references to move entities that are meant to disappear anyway though:

void EntityManager::AddEntity(Entity&& entity) {
    mEntities.push_back(std::move(entity));
}


You probably didn't care because in your use case, you passed temporary variables that are destroyed right after having been created. But what you might actually want to do is to create the entities directly in the vector insteas of creating them, then adding them to the vector. Therefore, what you really need is an emplace_back function:

template
void EntityManager::EmplaceEntity(Args&&... args) {
    mEntities.emplace_back(std::forward(args)...);
}


With such a function, you can init your world like this:

void World::Init() {
    // Add some initial entities
    for( int i=0 ; i < 200; i++ ) {
        mEntityManager.EmplaceEntity(*this);
    }
}


EDIT: Answering your comment. If Entity is an abstract class and you only add instances of derived classes, then I think that push_back still works fine since polymorphism will be achieved via reference semantics. On the other hand, EmplaceEntity could take another argument if you want to emplace instances of classes derived from Entity:

template
void EntityManager::EmplaceEntity(Args&&... args) {
    std::unique_ptr ptr( new Derived{std::forward(args)...} );
    mEntities.push_back(std::move(ptr));
}


I did not test the code above, but theorically it should. That allows you to create instances of classes derived from Entity directly without having to bother with the pointers. Here is how you could use it:

mEntityManager.EmplaceEntity(*this);


Anyway, whenever possible, try to use reference smantics and to hide pointer semantics inside the classes so that the users of the classes don't have to bother with the pointers.

Code Snippets

void EntityManager::AddEntity(const Entity& entity) {
    mEntities.push_back(entity);
}
void EntityManager::AddEntity(Entity&& entity) {
    mEntities.push_back(std::move(entity));
}
template<typename... Args>
void EntityManager::EmplaceEntity(Args&&... args) {
    mEntities.emplace_back(std::forward<Args>(args)...);
}
void World::Init() {
    // Add some initial entities
    for( int i=0 ; i < 200; i++ ) {
        mEntityManager.EmplaceEntity(*this);
    }
}
template<typename Derived, typename... Args>
void EntityManager::EmplaceEntity(Args&&... args) {
    std::unique_ptr<Entity> ptr( new Derived{std::forward<Args>(args)...} );
    mEntities.push_back(std::move(ptr));
}

Context

StackExchange Code Review Q#60495, answer score: 5

Revisions (0)

No revisions yet.