Hi all, 

New to GameDev.net in terms of posting, but I've often visited this website for articles in the past. I've recently taken up getting into C++ Game Development and have decided to take my hand at writing my first game in OpenGL from scratch. I've made some good progress in terms of the architecture thus far. I've gone for a Component/Entity system, that I think is an excellent concept for structuring and building up a game, as it allows for good code reuse and ease of creating/adding new and unique objects to the world. 

Naturally, I am only in the early stages and just have a Primitive Component that is basically rendering a red triangle to the screen at this time.

What I am looking for at this stage, is some feedback for a newbie like myself. What parts of my code could use improvement? Or are there any pitfalls I might be heading into it? I am open for any feedback, so please don't hold back.

See my code on Github here: 

https://github.com/robplsykes/nihilus

P.S I apologies if this is the wrong forum to post this. 

I just looked at one file pair (entity.h/cc). Here are a few comments to maybe get things started:

- I'm wondering about how unique_ptr is being used to store components. As you have things now, it seems like you could pass a pointer to an existing Component instance (rather than an instance created using 'new') to addComponent(), resulting in a unique_ptr instance with the default deleter tracking an arbitrary component instance (e.g. an instance on the stack, an instance already tracked by another unique_ptr, etc.). Also, you return raw pointers to components managed by unique_ptr's, which could be fine in some circumstances, but could cause problems if you were to e.g. cache those pointers for some reason or other.

- Another thing that immediately jumped out was lack of use of 'const'. Although opinions may differ on this, I think many would consider 'const correctness' to be a best practice in C++. Candidates for 'const' in your code would be member fields that are never modified after initialization (as appears to be the case with m_entityId and m_name), functions that don't modify the instance (such as has()), local variables that aren't modified after initialization (e.g. 'transform' in getTransformComponent()), and arguments that are never modified (probably most if not all of the arguments in your code).

- It's common to pass non-trivial types like std::string by reference to avoid unnecessary copying (this would apply, for example, to your constructor).

- I doubt reinterpret_cast is really what you want in getTransformComponent(). You might review the differences between reinterpret_cast, static_cast, and dynamic_cast with that in mind.

1 minute ago, Zakwayda said:

I just looked at one file pair (entity.h/cc). Here are a few comments to maybe get things started:

- I'm wondering about how unique_ptr is being used to store components. As you have things now, it seems like you could pass a pointer to an existing Component instance (rather than an instance created using 'new') to addComponent(), resulting in a unique_ptr instance with the default deleter tracking an arbitrary component instance (e.g. an instance on the stack, an instance already tracked by another unique_ptr, etc.). Also, you return raw pointers to components managed by unique_ptr's, which could be fine in some circumstances, but could cause problems if you were to e.g. cache those pointers for some reason or other.

- Another thing that immediately jumped out was lack of use of 'const'. Although opinions may differ on this, I think many would consider 'const correctness' to be a best practice in C++. Candidates for 'const' in your code would be member fields that are never modified after initialization (as appears to be the case with m_entityId and m_name), functions that don't modify the instance (such as has()), local variables that aren't modified after initialization (e.g. 'transform' in getTransformComponent()), and arguments that are never modified (probably most if not all of the arguments in your code).

- It's common to pass non-trivial types like std::string by reference to avoid unnecessary copying (this would apply, for example, to your constructor).

- I doubt reinterpret_cast is really what you want in getTransformComponent(). You might review the differences between reinterpret_cast, static_cast, and dynamic_cast with that in mind.

Hi, 

Brilliant. Thanks for taking the time to look at this for me. I'll take what you have said onboard and apply the appropriate changes. 

Regards

One thing I immediately noticed: In a "real" Entity Component system you do not want to store components inside the entities. Rather what you do is you store an array of arrays of components, with each "sub-array" storing all components of a certain component type. If you want to do this (I recommend it) look into type erasure.

22 minutes ago, NotAPenguin said:

One thing I immediately noticed: In a "real" Entity Component system you do not want to store components inside the entities. Rather what you do is you store an array of arrays of components, with each "sub-array" storing all components of a certain component type. If you want to do this (I recommend it) look into type erasure.

Thanks!

This feels like it would decouple the code further, which is brilliant. I assume not having entities "own" the components would actually be cleaner.

This also gives me an idea to just let components hold data rather than any logic and let subsystems handle it using that data to render/run the appropriate code. 

I have a rudimentary idea of how to implement what you said above, i'm sure once I start planning it out and coding it will come together. Do you have any articles or sample code I could look at for reference?

Thanks for your feedback! I'll definitely take a look at type erasure, also.

Regards

 

 

1

6 minutes ago, robplsykes said:

This also gives me an idea to just let components hold data rather than any logic and let subsystems handle it using that data to render/run the appropriate code. 

I have a rudimentary idea of how to implement what you said above, i'm sure once I start planning it out and coding it will come together. Do you have any articles or sample code I could look at for reference?

1. That's exactly how you should do it. Components should not store any logic. 

2. I have this implemented in my own engine (which is still a heavy work in progress, but the ECS is pretty much complete right now).

Include files for ECS

Source files for ECS

Brillaint! 

Thanks a lot, @NotAPenguin.

Regards