Game engine Scene class using raw pointers

I'm writing a C++ game engine and I have a scene class that I'd like to present as a code example to a team that is interested in my skills.

The scene class holds games entities, updating and rendering them using a camera.

I'm interested in the stylistic or functional improvements that could be made to improve this class, as well as any possible optimisations. In particular, 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 overhead. Is there truth in this?

Scene.h

```
#pragma once

#include
#include "Entity.h"
#include "Renderer.h"
#include "Window.h"
#include "Components\Camera.h"

namespace Core
{
// A class that represents a level or screen within the game. It maintains
// a collection of entities which make up the objects in the scene. These are
// updated and rendered each frame. The scene must be given a specified camera
// entity to view the world.
class Scene
{
public:
// An exception used by a scene to declare that an entity is missing
// a required component.
class MissingComponentException : public std::runtime_error
{
public:
MissingComponentException(const std::string& component, const std::string& message)
: std::runtime_error(component + " component missing: " + message){}
};

// Construct a new empty scene
Scene();

// Deconstructs the scene and any entities that it contains
~Scene();

// Adds an entity to the current scene. The scene will delete this
// entity when it is deleted.
void AddEntity(Core::Entity* entity);

// Gets an entity from the scene by name and returns it by pointer.
Core::Entity* GetEntity(std::string name);

// Sets the entity which will be used as the camera for rendering the
// scene. This entity must have a camera component otherwise
// MissingComponent

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 );

std::vector> entities; 
std::vector> 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, and we will have leaked A. If B also needed to be erased, at this point i becomes 1 so we decide we're done, and don't erase it.

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 renderer, std::shared_ptr 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& 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();
}

StackExchange Code Review Q#116138, answer score: 8