Smart Pointers or Raw Pointers
I was told by an industry professional that smart pointers (like shared_ptr) are frowned upon in game engines because of instances where reference counting adds an over head.
Yes, shared_ptr has overhead over raw pointers. But shared_ptr also solves problems that raw pointers have. Namely, what happens if you do:
Scene s;
s.AddEntity(e1);
s.AddEntity(e2);
...
{
Scene s2 = s;
// <== s2 destroyed here
}
// <== s destroyed here
You don't provide a copy constructor, so the default just is a simple copy of all your raw pointers. Which will then get deleted twice! Also, what if you never called Post()? Now you're leaking memory.
I don't know if Scene should be copyable or not - but right now it is and that's broken. So at the very least, we can easily fix it by using a smart pointer:
void AddEntity(std::unique_ptr<Core::Entity> );
std::vector<std::unique_ptr<Core::Entity>> entities;
std::vector<std::unique_ptr<Core::Entity>> entitiesToAdd;
This solves all of the problems above. Now, Scene isn't copyable. It is moveable, and that just works correctly. You're also no longer leaking anything from entitiesToAdd. This also obviates the need for a destructor, so you don't have to provide one.
This leaves the camera. You don't delete it, so it's unclear who actually owns it. If the Scene does not own it, then a raw pointer is of course fine.
Although, you have GetEntity(). What do you do with the entities returned from that function? Are they stored somewhere else? If you need shared ownership, then you need to use a shared_ptr (or something similar, like intrusive_ptr). If you need shared ownership, it doesn't really matter how much overhead those smart pointers add...
Bug
In Post(), you will fail to erase all the destroyed entities based on their configuration. Also, you're deleting the wrong ones. Let's say we have two entities, A and B, the first of which needs to be destroyed:
0] A, IsDestroyed() is true
1] B
When i==0, the check passes, then we call erase(), at which point our vector now looks like
0] B
Then, you call delete entities[i];, which deletes B. Now we have a vector of one element, which isn't even a valid element.
The correct way to erase elements from a vector which match a certain condition is the Erase-Remove idiom:
entities.erase(
std::remove_if(entities.begin(), entities.end(), std::mem_fn(&Entity::IsDestroyed)),
entities.end());
And switching to smart pointers takes care of the delete.
While we're in this function, the moving from entitiesToAdd to entities could be done using vector's insert() member function, which will be more efficient than repeated push_back's:
entities.insert(entities.end(),
std::make_move_iterator(entitiesToAdd.begin(),
std::make_move_iterator(entitiesToAdd.end()));
entitiesToAdd.clear();
Excessive Comments
Comments are good, and many of course comments are quite valuable. Some are completely unnecessary though. Clearly the default constructor for Scene should construct a new empty scene, and the destructor destroys it, and so forth. When you're writing comments, ask yourself if somebody reading your code actually needs the comment.
Another example:
// Renders the scene using the given renderer and window.
void Render(std::shared_ptr<Core::Renderer> renderer, std::shared_ptr<Core::Window> window);
Yeah, that's pretty obvious.
Other code comments
UpperCase for member functions is not very common - I would strongly suggest either camcelCase or snake_case.
GetEntity() is searching for a given entity that matches a name. In other words, you're finding an entity if the name matches. Let's just use std::find_if! Also, take your argument by reference to const to avoid an unnecessary copy:
Core::Entity* Scene::getEntity(std::string const& name)
{
auto it = std::find_if(entities.begin(), entities.end(),
[&](std::unique_ptr<Core::Entity>& entity){
return entity->getName() == name; // ==, not compare()
});
return it != entities.end() ? *it : nullptr;
}
In Update(), prefer to use a range-based for expression:
for (auto& entity : entities) {
entity->Update();
}