Entity Component System in C++

Question

I've written an Entity Component System using C++ for my game engine. I'm still inexperienced so I've probably made a lot of mistakes. Thus I've decided to ask here for an honest review.

The complete code is fairly large (although not too large IMHO) so it wouldn't quite fit here. Still if anyone here wishes to browse through it and give me some advice you can find the code on GitHub.

However the point of this topic was to present a few dubious design choices and code parts which I'm not completely sure about.

DESIGN:

There are many ECS designs but I've chosen the one which was the easiest to grasp logically. Current design is based on Entities being Component containers, and Systems being Component processors.

Basically Entity contains a unique id, a bitset which represents a "key" for systems (with each bit representing a component type) and a map of components.

Components on the other hand contain only data, no methods besides constructors.

And finally Systems contain a bitset which represents a "lock" where each bit represents a component which are registered(subscribed might be a better word) to the system.

Now while I really like this design because it's easy to understand, it does contain a double nested loop, and if a component contains a collection then it leads to a triple nested loop. I'm worried that for a game which requires fast updates this might cause performance issues. Should I redesign the system? What would be a better design?

CODE:

There are many things I'm worried about in regards to code.

First is that I've got a nested for -> if -> for -> if loop. Which, in my opinion, is an essence of bad code. And also the fact that it's also copied code (used twice).

void World::update()
{
    for( auto& ecsystem : m_systems )
    {
        if( ecsystem.second->enabled() ) //if system is enabled
        {
            std::bitset< 64 >& lockbits = ecsystem.second->getLockBits();
            for( auto& entity : m_entities )
            {
                if( ( entity->getKeyBits() & lockbits ) == lockbits )
                {
                    ecsystem.second->update( entity->getRelevantComponents( lockbits ) );
                }
            }
        }
    }
}

void World::draw()
{
    for( auto& ecsystem : m_systems )
    {
        if( ecsystem.second->enabled() )
        {
            std::bitset< 64 >& lockbits = ecsystem.second->getLockBits();
            for( auto& entity : m_entities )
            {
                if( (entity->getKeyBits() & lockbits) == lockbits )
                {
                    ecsystem.second->draw( entity->getRelevantComponents( lockbits ) );
                }
            }
        }
    }
}

Both methods (update() and draw()) must have 0 parameters (though I'm planning on adding a time step parameter to update method later on) and I thought that I might somehow create a single method which iterates over all systems and entities. But since the end call to each systems update or draw method is made from within the system itself, I can't think of a way to make the iteration more ambiguous.

Second is that getRelevantComponents call. I've got a separate class called ComponentProvider which provides relevant components to users of the system. Basically to make sure that whoever is creating new systems can't use components which aren't registered to that system. The way getRelevantComponents( bitset<64> lockbits ) works is that it creates an instance of ComponentProvider class, then adds the pointers of entity's relevant components and then returns that ComponentProvider instance.

ComponentProvider Entity::getRelevantComponents( std::bitset< 64 >& lockBits )
{
    ComponentProvider provider;

    for( auto& mapItem : m_componentMap )
    {
        auto key = mapItem.first;
        if( lockBits.test( key ) )
        {
            provider.m_relevantComponentMap.insert( std::make_pair( key, mapItem.second) );
        }
    }
    return provider;
}

Now what worries me the most in this code is that I'm returning an object instanced in the method itself. The result of the method call is thrown as a parameter to another method. I thought that I should make an rvalue assignment operator overload so that the values won't be copied but instead swapped, however I've been told that I should use the copy-swap idiom instead and leave it to RVO to handle the optimizations. Is that really a good idea?

Third is the fact that I'm using shared pointers a lot. I'm afraid of shared pointer overhead as well as slowdowns since new shared pointers are being created and destroyed every update cycle. Should I use raw pointers instead and just be very careful of memory leaks?

Fourth is that I'm using a map to contain all systems since I need to be able to remove specific systems and not allow adding more systems of the same type, but I also need to iterate over all systems each update cycle so I'm worried if it was really a good idea to use a map( instead of an unordered map since as far as I know normal map is contiguous ) instead of a vector. I've also read that unordered_map is faster than normal map in every case so I'm not sure at what I should use really.

Those are my largest worries right now. Please let me know if my post is hard to understand and I'll try to explain things better.

Answer 1

With regards to returning ComponentProvider by value from getRelevantComponents if you have a move constructor defined (or the default move constructor is generated) the return is likely to be moved instead of copied and you won't have to worry about it at all.

You might consider writing a member function for entity called Validate or similar which takes std::bitset<64>& and performs the check:

bool Validate(const std::bitset<64>& lockbits) const
{
    return ( entity->getKeyBits() & lockbits ) == lockbits;
}

This will reduce code duplication ever so slightly and if your validation rules ever change you only have to change it once in the member function and not both in update and draw and anywhere else.

I'm tempted to suggest moving your for loops into member functions in ecsystem, but that would require introducing a dependency between ecsystem and entity so probably isn't worth doing.

Edit You can probably replace some of getRelevantComponents with std::copy_if

ComponentProvider Entity::getRelevantComponents( std::bitset< 64 >& lockBits )
{
    ComponentProvider provider;

    std::copy_if(
        std::begin(m_componentMap),
        std::end(m_componentMap),
        std::begin(provider.m_relevantComponentMap),
        [&lockBits](std::pair<keyType, valueType> & mapItem)
        {
           return lockBits.test(mapItem.first);
        });

    return provider;
}

This of course assumes that provider.m_relevantComponentMap is the same type as m_componentMap.