8
\$\begingroup\$

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<Entity&> mTree;

void World::Init() {
    // Add some initial entities
    for( int i=0 ; i < 200; i++ ) {
        std::unique_ptr<Entity> 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<std::unique_ptr<Entity>> mEntities;

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

void EntityManager::AddEntity(std::unique_ptr<Entity>* 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<typename Derived, typename... Args>
Derived& EntityManager::EmplaceEntity(Args&&... args) {
    std::unique_ptr<Entity> ptr( new Derived{std::forward<Args>(args)...} );
    mEntities.push_back(std::move(ptr));
    return dynamic_cast<Derived&>(*(mEntities.back()));
}

World:

for( int i=0 ; i < 200; i++ ) {
    BlueEntity& entity = mEntityManager.EmplaceEntity<BlueEntity>(*this); 
    entity.setPosition(x, y);       
}
\$\endgroup\$
5
\$\begingroup\$

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<typename... Args>
void EntityManager::EmplaceEntity(Args&&... args) {
    mEntities.emplace_back(std::forward<Args>(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<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));
}

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<BlueEntity>(*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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback @Morwenn, very useful. Though I didn't make it obvious in my example, Entity will in fact be an abstract class from which other Entity types would inherit from. That being the case I guess the Entity would need to be instantiated outside of the EntityManager, but ultimately I'd like the EntityManager to own it. Would your first two options be most suitable for that? \$\endgroup\$ – user3700278 Aug 19 '14 at 21:42
2
\$\begingroup\$

I suppose these are member variables of the class World:

EntityManager mEntityManager;
Quadtree<Entity&> mTree;

what is the purpose of EntityManager? As long as it is nothing more than a container, you should use a container. Keep it simple! You can define the container with a typedef so that if later some more functionality is needed you can extend the functionality with a real custom class.

Here I see a problem of ownership because mTree has pointers which are owned by mEntityManager. If you plan to regenerate the whole mTree in each update, then mTree should be a local variable in the update function. If you plan to update mTree using its previous state, you must be very careful to handle deleted and added entities. This should be handled by the World object since it owns coordinate both mEntityManager and mTree. Hence the conclusion that World is the actual EntityManager (which, after all, sounds as a trivial truth).

void World::Init() {

Why the method's name is capitalized? I think init would be better.

    std::unique_ptr<Entity> entity( new Entity(*this) );
    mEntityManager.AddEntity(&entity);

even if you plan to keep your EntityManager as a custom class, it would be a good idea to use the same interface as any standard container class. So EntityManager::AddEntity should be push_back and you should pass entity to it instead of &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 ); 
    }

As said before: mTree should be a local variable in World::update so you make it clear that there is no need to take care of its content while you create and delete entities.

This is at present the only functionality of EntityManager:

    mEntityManager.update();
}

you can easily replace this line with a for_each or an iteration. This would result to be more clear, since you are not really hiding anything in this method.

EntityManager: as already said, I see nothing useful in this class. I read somewhere that whenever you name a class XxxxManager you should ask yourself if such class is really needed. And if it is needed you should be able to find a more explicit name which explain the purpose of the class.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback @emanuele-paolini ! While I feel like the Entity Manager class will still be useful, your comments have certainly made me rethink some of the other 'Manager' classes I'm currently using. Regarding the scope of the tree, the reason it's not currently local to 'Update' is that I'm using it in a 'Render' method to draw the partitions for debug purposes. I guess I could just generate/store the polygons and destroy the QuadTree instance itself. \$\endgroup\$ – user3700278 Aug 20 '14 at 11:32

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.