Note: I first looked at modernizing your code, after that, I've resolved your questions in the assumption you applied my suggestions.
Looking at your code, I'm a bit worried that you are trying to reeinvent some weels. First of all: SID
and HashString
.
I'm really worried about this, as it ain't as readable, predictable and performant as it could be.
Let's start with readable: Why would you redefine HashString
to SID
? This introduces an extra indirection that doesn't add any value. I can see some arguments of making an alias, however, as you are using C++17, just make it an inline function.
Secondly: Predictable. HashString returns a StringId
. All std::hash
return std::size_t
. I suspect it's the same type, however, all your calculations use unsigned int
iso StringId
. So any hash you create will have several zeros.
Finally: Performance. Your function accepts const char *
. Why don't you use std::string_view
instead? If you would have a std::string
, it already knows the size, so you shouldn't recalculate it. It can still be called with a zero-terminated char*, in which case strlen will be called in the Ctor of the view.
As already said, it looks like a reimplementation of std::hash<std::string_view>
. However, I see an argument in having your own hash function.
Still looking at the same function: fnv_prime
is a constant. Why don't you use constexpr
for it iso const
?
I also see a for-loop. Whenever, I see for (i = 0
, I immediately worry about the scope of the variable, do we need it after the loop? Having to check this increases the complexity for me. How about for (unsigned int i = 0; i < len; ++i)
? However, as you will be using std::string_view
, it can become: for (auto c : str)
, even easier to read/understand;
Moving on: the Component class. Again, you have a constant that could be constexpr. However, I'm worried much more about mID
. This ID is free to access for everyone and free to update. Make it private and provide a getter/setter for it.
Your constructor/dtor are implemented as {}
, while this could be = default;
and the move/copy ctor/assignment are missing. Best to check on the rule of 5
.
Going forward: TransformComponent. Are you compiling with compiler warnings (-Weverything -Werror
in Clang, /WX /W4
in MSVC)? You have a nice example of what is called shadowing. The member mPosition
will never be initialized as you create a variable with the same name in a different scope. One could even wonder why you pass x
and y
separately, I would expect a single argument of type Vec2
.
The struct Components
creeps me out. Looking at it, its a really bad implementation of std::vector
. Get rid of it! (And prereserve the vector if relevant).
AddComponents
also looks pre-C++17. An alternative:
template <typename Arg, typename ... Args>
inline void AddComponents(StringID gameObjectID, Arg first, Args ... args) {
// Do the work
if constexpr (sizeof...(args))
AddComponents(gameObjectID, args...);
}
Moving to CreateGameObject
why do a c-style cast to char*
when not needed?
Up to the Application
class. This looks like an attempt for a singleton pattern. I would at least use std::cerr
iso std::cout
for reporting failures. However, I'd even recommend assert
. Your destructor also never resets the static to nullptr
.
And a final remark for main
: Why would you even allocate memory here. Try writing it as:
TestGame testGame{};
testGame.Run();
return 0;
Looking at your questions:
Templates ain't slow, please compile with optimizations: -O3
in clang, /O2
in MSVC. It might hurt you for compile time, however, it hurts less as having to write everything manually.
I agree, typeid
is bad. You don't need it. Having the overload will work good enough without the runtime overhead. However, I wouldn't overload AddComponents
on the type. I would have an overloaded function that returns you the correct std::vector
. Much less code to duplicate, much easier to reuse at other places.