Game engine Scene class using raw pointers

Question

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 <memory>
#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 
      // MissingComponentException will be thrown.
      void SetCameraEntity(Core::Entity* newCameraEntity);

      // Updates the scene, should be called every frame. Calls update on 
      // all the entities that the scene contains.
      void Update();

      // Renders the scene using the given renderer and window. 
      void Render(std::shared_ptr<Core::Renderer> renderer, std::shared_ptr<Core::Window> window);

      // Post updates the scene, removing any entities marked for deletion and
      // adding any entities in the entitiesToAdd list. 
      void Post();

   protected:
      // Use of raw pointers chosen here specifically for efficiency,
      // i.e. a reduce overhead when passing objects around over 
      // std::shared_ptr.
      Components::Camera* camera;

      // Use of raw pointers chosen here specifically for efficiency,
      // i.e. a reduce overhead when passing objects around over 
      // std::shared_ptr.
      // Use of std::vector here as the number of entities is likely
      // to vary a lot during the scenes life time.
      std::vector<Core::Entity*> entities; 

      // Entities that will  be added in the Post phase of the scene
      // update.
      std::vector<Core::Entity*> entitiesToAdd;

      // The projection transform used to define perspective, it requires:
      // - The y field of view
      // - The aspect ratio
      // - The near plane distance
      // - The far plane distance
      // When a camera component is suppled it is retrieved from this
      // the component during render.
      glm::mat4 projectionTransform;

      // The view transform used to define where the view is looking from
      // and to, it requires: 
      // - An eye position
      // - A focus position
      // - An up vector
      // When a camera component is suppled it is retrieved from this
      // the component during render.
      glm::mat4 viewTransform;
   };
}

Scene.cpp

#include "Scene.h"

#include <iostream>
#include "glm\glm.hpp"
#include "glm\gtc\matrix_transform.hpp"
#include "Components\Model.h"
#include "Components\Transform.h"
#include "Components\Camera.h"
#include "Structures\Mesh.h"
#include "Utility\Maths.h"


namespace Core
{
   Scene::Scene() :
      camera(nullptr),
      projectionTransform(glm::perspective(Utility::Maths::PI / 4.0f, 1.0f, 0.1f, 100.0f)), // A default projection transform (should never be used)
      viewTransform(glm::lookAt(glm::vec3(1,0,0),glm::vec3(0,0,0), glm::vec3(0,1,0))) // A default view transform (should never be used)
   {}

   Scene::~Scene()
   {
      for(unsigned int i = 0; i < entities.size(); i++)
      {
         delete entities[i];
      }
      entities.clear();
   }

   void Scene::AddEntity(Core::Entity* entity)
   {
      entity->SetScene(this);

      // This entity will be added in the Post phase of the update cycle
      entitiesToAdd.push_back(entity);
   }

   Core::Entity* Scene::GetEntity(std::string name)
   {
      Entity* matchingEntity = nullptr;
      for(unsigned int i = 0; i < entities.size() && matchingEntity == nullptr; i++)
      {
         if(entities[i]->GetName().compare(name) == 0)
         {
            matchingEntity = entities[i];
         }
      }
      return matchingEntity;
   }

   void Scene::SetCameraEntity(Core::Entity* cameraEntity)
   {
      if(cameraEntity->GetComponent<Components::Camera>() == nullptr)
      {
         throw MissingComponentException("Camera", "Provided camera entity not does not have a camera component.");
      }

      camera = cameraEntity->GetComponent<Components::Camera>();;
   }

   void Scene::Update()
   {
      for(unsigned int i = 0; i < entities.size(); i++)
      {
         entities[i]->Update();
      }
   }

   void Scene::Render(std::shared_ptr<Core::Renderer> renderer, std::shared_ptr<Core::Window> window)
   { 
      if(camera == nullptr)
      {
         // Attempting to render without a camera... give up.
         return;
      }

      projectionTransform = camera->GetProjectionMatrix(window);
      viewTransform = camera->GetViewMatrix();

      for(unsigned int i = 0; i < entities.size(); i++)
      {
         if(entities[i]->HasComponent<Components::Model>())
         {
            Components::Model* model = entities[i]->GetComponent<Components::Model>();

            glm::mat4 modelTransform = glm::mat4(1.0f);
            if(entities[i]->HasComponent<Components::Transform>())
            {
               Components::Transform* transform = entities[i]->GetComponent<Components::Transform>();
               modelTransform = transform->GetMatrix();
            }

            renderer->RenderMesh(
               model->GetMesh(), 
               model->GetTexture(),
               model->GetShader(),
               projectionTransform,
               viewTransform,
               modelTransform
               );
         }
      }
   }

   void Scene::Post()
   {
      for(unsigned int i = 0; i < entities.size(); i++)
      {
         if(entities[i]->IsDestroyed())
         {
            entities.erase(entities.begin() + i);
            delete entities[i];
         }
      }

      for(unsigned int i = 0; i < entitiesToAdd.size(); i++)
      {
         entities.push_back(entitiesToAdd[i]);
      }
      entitiesToAdd.clear();
   }
}

Answer 1

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, 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<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();
}

Answer 2

I think you did a good job here and there's little to change. Now about the pointers and memory management...

I was told by an industry professional that smart pointers (like shared_ptr) are frowned upon in game engines

It is true. The game development industry is known for its fear of the Standard C++ Library. From my own experience, for the most part it is just cargo cult from the early days of embedded development for Consoles that had very steep hardware limitations, and from when the Standard Library was still very new and not well implemented.

This is not to say that there aren't a few instances where this fear is justifiable, but in general I think it is very exaggerated. I suggest taking a look at this CppCon talk from last year's conference where some points about "C++ for game development" are discussed.

---

Now let's get to the point, when should you use a shared pointer, a unique pointer, a raw pointer or a reference?

• shared_ptr is meant for when the lifetime of an object needs to be shared by distinct owners. The last owner holding a reference to the object will destroy it when it goes away. In your code specifically, who owns the Entities? If they are owned by the Scene, then you certainly don't need a shared pointer, because there's no shared ownership going on!

• So how about unique_ptr? This one meant for when an object has a single owner during its lifetime. When the owner goes away, it deletes the object. Ownership can be transferred with std::move, but no two object can own the same unique_ptr. Should Scene be using unique_ptr? It could. Your vectors of entities could be storing then by unique_ptr. It would make your code a lot simpler by removing all manual cleanup that you are currently doing. It would also make sure your code never leaks memory in the event of an exception being thrown.

• And raw pointers and references, where do they fit in this? At the compiler level, a reference and a pointer to an object are the same thing, but it makes a huge difference for the programmer. A pointer has the potential of being nullptr, you can assign null to it explicitly, so we use pointers when we know the object might be null and we are going to check for that. A reference, on the other hand, cannot have null assigned to it and cannot be rebound once initialized, so when we pass a parameter by reference, for instance, we are making it cristal clear to the reader that the object will not be null. NOTE that both a reference and a raw pointer convey no ownership semantics to the programmer whatsoever, so they should only be used for temporary references that no one will hold on to. If you need to pass or share ownership, that's when our friendly smart pointers come into play. In your code, you should now take this information into consideration and replace your raw pointers with references where a null is not expected and where you are not transferring or sharing ownership. One place where I'm pretty sure you've misused the shared pointer is in the render function:

  void Render(std::shared_ptr<Core::Renderer> renderer, std::shared_ptr<Core::Window> window)
  

  I see no reason for the Scene render to take shared ownership of those objects just for the drawing. Unless you have some serious threading going on, there's no way the objects can be destroyed while the scene is rendered. You should be using references in there.