patterncppMinor
Entity Component System in C++
Viewed 0 times
componentsystementity
Problem
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
```
void World::update()
{
for( auto& ecsystem : m_systems )
{
if( ecsystem.second->enabled() ) //if system is enabled
{
std::bitset& lockbits = ecsystem.second->getLockBits();
for( auto& entity : m_entities )
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& lockbits = ecsystem.second->getLockBits();
for( auto& entity : m_entities )
Solution
With regards to returning
You might consider writing a member function for
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
I'm tempted to suggest moving your for loops into member functions in
Edit
You can probably replace some of
This of course assumes that provider.m_relevantComponentMap is the same type as m_componentMap.
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& and performs the check:bool Validate(const std::bitset& 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_ifComponentProvider Entity::getRelevantComponents( std::bitset& lockBits )
{
ComponentProvider provider;
std::copy_if(
std::begin(m_componentMap),
std::end(m_componentMap),
std::begin(provider.m_relevantComponentMap),
[&lockBits](std::pair & mapItem)
{
return lockBits.test(mapItem.first);
});
return provider;
}This of course assumes that provider.m_relevantComponentMap is the same type as m_componentMap.
Code Snippets
bool Validate(const std::bitset<64>& lockbits) const
{
return ( entity->getKeyBits() & lockbits ) == lockbits;
}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;
}Context
StackExchange Code Review Q#30170, answer score: 3
Revisions (0)
No revisions yet.