Small library for SFML to streamline program flow

Question

For the past week or two I have been working on a small library that aims to ease development of SFML games, and after finishing it I would love to get some feedback on it. I don't have access to any tutors so feedback is very valuable to me when trying to get better at coding. Thanks in advance to anyone who sets some time aside to go through my code.

Link to SFML if you're not familiar with it. It's a library for making simple 2D games (so yes my library is a library for a library).

The reason I started working on this little hobby project is because I was making a few games with SFML, and I noticed that main.cpp just got busier and busier, with more and more calls to handlers to update the logic and draw sprites to the window. I wanted to streamline the program flow so that it could be read almost like those program flow paradigms you see in game development books:

while (gameRunning)
{
 handleInput();
 updateLogic();
 render();
}

What I came up with in the end is two abstract base classes, LogicHandler and GraphicalHandler, that split up all the handlers into logic and graphics. Every handler that handles game logic should be derived from LogicHandler and every handler that handles graphics should be derived from GraphicalHandler.

A handler in my code is an object that keeps track of a certain aspect of the game. Logic handlers keep track of game logic while graphical handlers keep track of graphical objects that can be drawn to the screen. If a handler needs updates from another handler, then it should store a pointer to that handler so it can always update itself using that pointer.

I wanted to be able to update all handlers with just a few method calls in the main game loop. To achieve this, LogicHandler and GraphicalHandler both utilize a technique where they store pointers to every single instance of their class, in a static vector called s_allInstances_(s_ for static). This allows them to, in static methods, loop over every handler that is currently alive and call methods on them. LogicHandler uses this to loop through all logic handlers and deliver events and update them. GraphicalHandler uses this to loop through all graphical handlers and update them and draw their sprites to the screen.

This may sound confusing, so let me briefly explain how these classes use s_allInstances_.

LogicHandler consists of two static methods - handleEvent, updateLogic and two purely virtual methods - receiveEvent and update. Each virtual method corresponds to one of the static methods, you can probably see which belongs to which.

When LogicHandler::handleEvent is called from the game loop when a player input is discovered, it loops through s_allInstances_ and calls receiveEvent (and passes the event) on every single logic handler.

When LogicHandler::updateLogic is called during every iteration of the game loop, it loops through s_allInstances_ and calls update on every single logic handler.

This makes it so that no matter how many logic handlers exist, they can all be simultaneously updated or receive player input with just a single method call.

GraphicalHandler has two static methods - updateGraphicalObjects, render and one purely virtual method - update

All graphical handlers inherit a vector of pointers to drawables (sprites, shapes etc) called graphicalObjects_. Everything that a graphical handler wants displayed on the screen should be pushed onto graphicalObjects_.

When GraphicalHandler::updateGraphicalObjects is called, it loops through all graphical handlers and calls the update method on them, which is when the handlers can update the sprites in their graphicalObjects_ vector, using pointers to logic handlers that they have stored as members.

When GraphicalHandler::render is called, it loops through all graphical handlers and renders the sprites inside their graphicalObjects_ vectors.

Just as with the logic handlers, it doesn't matter how many graphical handlers exist, they can all be updated or rendered from with just one method call.

Combining LogicHandler and GraphicalHandler leaves you with a very straightforward game loop, similar to the paradigm I showed in the beginning:

while(window.isOpen())
{
 sf::Event event;
 while(window.pollEvent(event)
 {
 LogicHandler::handleEvent(event, window);
 }
 LogicHandler::updateLogic();
 GraphicalHandler::updateGraphicalObjects();
 GraphicalHandler::render();
}

With the general idea out of the way, let's get to the actual code.

Besides general review of the code, I would love to know what you think of the approach as a whole. Is this a good way of dealing with program flow, logic handling, and rendering? What are the cons of this system? Don't hold back, my feelings won't be hurt.

OBS: I also added a class and a main.cpp at the end that use the library to make a little program where you control a square, just to show you how I intended for the code base to be used.

Finally, here's the code:

InstanceTracker.h

#pragma once
#include <vector>
template <typename T>
class InstanceTracker
{
public:
 InstanceTracker() noexcept
 {
 s_allInstances_.push_back(static_cast<T*>(this));
 }
 InstanceTracker(const InstanceTracker& source) noexcept
 : InstanceTracker()
 {
 }
 InstanceTracker(const InstanceTracker&& source) noexcept
 : InstanceTracker()
 {
 }
 virtual ~InstanceTracker() noexcept
 {
 auto it = std::find(s_allInstances_.begin(), s_allInstances_.end(), this);
 int index = it - s_allInstances_.begin();
 s_allInstances_.erase(s_allInstances_.begin() + index);
 }
 void moveMyInstanceToLast()
 {
 auto it = std::find(s_allInstances_.begin(), s_allInstances_.end(), this);
 std::rotate(it, it + 1, s_allInstances_.end());
 }
 void moveMyInstanceToFirst()
 {
 auto it = std::find(s_allInstances_.begin(), s_allInstances_.end(), this);
 s_allInstances_.erase(it);
 s_allInstances_.insert(s_allInstances_.begin(), static_cast<T*>(this));
 }
protected:
 static std::vector<T*> s_allInstances_;
};
template<typename T>
std::vector<T*> InstanceTracker<T>::s_allInstances_;

LogicHandler.h

#pragma once
#include "InstanceTracker.h"
#include "SFML/Graphics.hpp"
#include <vector>
class LogicHandler : public InstanceTracker<LogicHandler>
{
public:
 virtual bool receiveEvent(const sf::Event& event, const sf::RenderWindow& window) = 0;
 /* The reason receiveEvent() returns a boolean is to avoid unnecessary looping
 * in handleEvent(). If a handler discovers an event that definitely only applies
 * to that handler, then it should return true from receiveEvent().
 * handleEvent() will then stop looping when it encounters it, thus avoiding a lot of looping.
 * To take advantage of this, handlers should be declared in order of how common the events 
 * they're listening for are. E.g., a handler that listens for the arrow keys being pressed 
 * to move the player should be declared earlier than a handler that listens for a button being 
 * pressed, since the arrow keys input will be much more common.
 */
 virtual void update() = 0;
 static void updateLogic();
 static void handleEvent(const sf::Event& event, const sf::RenderWindow& window);
};

LogicHandler.cpp

#include "LogicHandler.h"
void LogicHandler::updateLogic()
{
 for (LogicHandler* handler : s_allInstances_)
 {
 handler->update();
 }
}
void LogicHandler::handleEvent(const sf::Event& event, const sf::RenderWindow& window)
{
 for (LogicHandler* handler : s_allInstances_)
 {
 if(handler->receiveEvent(event, window))
 {
 break;
 }
 }
}

PositionHandler.h

//Implementation in the header file for such a simple class
#pragma once
#include "LogicHandler.h"
class PositionHandler : public LogicHandler
{
public:
 PositionHandler() { position_ = sf::Vector2f(0, 0); rotation_ = 0; }
 sf::Vector2f getPosition() const { return position_; }
 float getRotation() const { return rotation_; }
protected:
 sf::Vector2f position_;
 float rotation_;
};

Button.h

#pragma once
#include "LogicHandler.h"
class Button : public LogicHandler
{
public:
 Button(sf::Vector2f size, sf::Vector2f position = sf::Vector2f(0.f, 0.f));
 virtual bool receiveEvent(const sf::Event& event, const sf::RenderWindow& window) override;
 virtual void buttonPressed() = 0;
protected:
 sf::Rect<float> hitbox_;
};

Button.cpp

#include "Button.h"
Button::Button(sf::Vector2f size, sf::Vector2f position)
{
 hitbox_.width = size.x;
 hitbox_.height = size.y;
 hitbox_.left = position.x;
 hitbox_.top = position.y;
}
bool Button::receiveEvent(const sf::Event& event, const sf::RenderWindow& window)
{
 if (event.type == sf::Event::MouseButtonPressed && hitbox_.contains(sf::Vector2f(sf::Mouse::getPosition(window))))
 {
 buttonPressed();
 return true;
 }
 return false;
}

PauseButton.h

#pragma once
#include "Button.h"
class PauseButton : public Button
{
 friend class PauseButtonIcon;
 friend class GameEngine;
public:
 PauseButton(sf::Vector2f size, sf::Vector2f position);
 void update() override;
 void buttonPressed() override;
private:
 bool bPaused_;
};

PauseButton.cpp

#include "PauseButton.h"
PauseButton::PauseButton(sf::Vector2f size, sf::Vector2f position)
 : Button(size, position), bPaused_(false)
{
}
void PauseButton::buttonPressed()
{
 bPaused_ = bPaused_ ? false : true;
}
void PauseButton::update()
{
}

GraphicalHandler.h

#pragma once
#include "InstanceTracker.h"
#include "ClonableDrawable.h"
#include "LogicHandler.h"
#include <SFML/Graphics.hpp>
#include <vector>
#include <memory>
class GraphicalHandler : public InstanceTracker<GraphicalHandler>
{
public:
 GraphicalHandler();
 GraphicalHandler(const GraphicalHandler& source);
 
 virtual void update() = 0;
 static void updateGraphicals();
 static void renderObjects(sf::RenderWindow& window);
protected:
 std::vector<std::shared_ptr<ClonableDrawable>> graphicalObjects_;
 bool bVisible_;
};

GraphicalHandler.cpp

#include "GraphicalHandler.h"
GraphicalHandler::GraphicalHandler()
 : bVisible_(true)
{
}
GraphicalHandler::GraphicalHandler(const GraphicalHandler& source)
 : bVisible_(source.bVisible_), InstanceTracker<GraphicalHandler>(source)
{
 for (std::shared_ptr<ClonableDrawable> ptr : source.graphicalObjects_)
 {
 graphicalObjects_.push_back(ptr->clone());
 }
}
void GraphicalHandler::updateGraphicals()
{
 for (GraphicalHandler* handler : s_allInstances_)
 {
 handler->update();
 }
}
void GraphicalHandler::renderObjects(sf::RenderWindow& window)
{
 for (GraphicalHandler* handler : s_allInstances_)
 {
 if (handler->bVisible_)
 {
 for (std::shared_ptr<ClonableDrawable> object : handler->graphicalObjects_)
 {
 window.draw(*object);
 }
 }
 }
}

ClonableDrawable.h

/* The reason ClonableDrawable exists is because graphical handlers cannot be copied if they can't copy 
the contents of their vector of drawables. `sf::Drawable` is abstract
 
so to be able to copy them I added these two classes below. */
#pragma once
#include "SFML/Graphics.hpp"
#include <memory>
class ClonableDrawable
{
public:
 virtual ~ClonableDrawable() = default;
 virtual std::unique_ptr<ClonableDrawable> clone() const = 0;
 virtual operator sf::Drawable& () = 0;
};
template <typename T>
class ClonableDraw : public T, public ClonableDrawable
{
public:
 ClonableDraw() = default;
 template<typename... Args>
 ClonableDraw(Args&... args): T(args...) {}
 template<typename... Args>
 ClonableDraw(Args&&... args): T(args...) {}
 std::unique_ptr<ClonableDrawable> clone() const override
 {
 return std::make_unique<ClonableDraw<T>>(*this);
 }
 operator sf::Drawable& () override { return *this; }
};

PauseButtonIcon.h

#pragma once
#include "GraphicalHandler.h"
#include "PauseButton.h"
#include "SFML/Graphics.hpp"
class PauseButtonIcon : public GraphicalHandler
{
public:
 PauseButtonIcon(std::shared_ptr<PauseButton> handler, sf::Color color);
 void update() override;
private:
 std::shared_ptr<PauseButton> handler_;
 std::shared_ptr<ClonableDraw<sf::VertexArray>> playIcon_;
 std::shared_ptr<ClonableDraw<sf::RectangleShape>> pauseIcon1_;
 std::shared_ptr<ClonableDraw<sf::RectangleShape>> pauseIcon2_;
};

PauseButtonIcon.cpp

#include "PauseButtonIcon.h"
PauseButtonIcon::PauseButtonIcon(std::shared_ptr<PauseButton> handler, sf::Color color)
 : handler_(handler)
{
 playIcon_ = std::make_shared<ClonableDraw<sf::VertexArray>>(sf::Triangles, 3);
 (*playIcon_)[0].position = sf::Vector2f(handler->hitbox_.left, handler->hitbox_.top);
 (*playIcon_)[1].position = sf::Vector2f(handler->hitbox_.left, handler->hitbox_.top+handler_->hitbox_.height);
 (*playIcon_)[2].position = sf::Vector2f(handler->hitbox_.left+handler_->hitbox_.width, handler->hitbox_.top+handler_->hitbox_.width/2);
 (*playIcon_)[0].color = color;
 (*playIcon_)[1].color = color;
 (*playIcon_)[2].color = color;
 pauseIcon1_ = std::make_shared<ClonableDraw<sf::RectangleShape>>(sf::Vector2f(handler_->hitbox_.width / 3, handler_->hitbox_.height));
 pauseIcon1_->setPosition(handler_->hitbox_.left, handler_->hitbox_.top);
 pauseIcon1_->setFillColor(color);
 pauseIcon2_ = std::make_shared<ClonableDraw<sf::RectangleShape>>(sf::Vector2f(handler_->hitbox_.width / 3, handler_->hitbox_.height));
 pauseIcon2_->setFillColor(color);
 pauseIcon2_->setPosition(handler_->hitbox_.left + 2*pauseIcon1_->getSize().x, handler_->hitbox_.top);
}
void PauseButtonIcon::update()
{
 if (handler_->bPaused_ && graphicalObjects_.size() != 1)
 {
 graphicalObjects_.clear();
 graphicalObjects_.push_back(playIcon_);
 }
 else if (!handler_->bPaused_ && graphicalObjects_.size() != 2)
 {
 graphicalObjects_.clear();
 graphicalObjects_.push_back(pauseIcon1_);
 graphicalObjects_.push_back(pauseIcon2_);
 }
}

SimpleGraphical.h

/* SimpleGraphical can be used when you don't need a very complicated graphical handler */
#include "GraphicalHandler.h"
#include "PositionHandler.h"
template<typename T>
class SimpleGraphical : public GraphicalHandler
{
public:
 template<typename... Args>
 SimpleGraphical(Args... args)
 : graphical_(std::make_shared<ClonableDraw<T>>(args...))
 {
 graphicalObjects_.push_back(graphical_);
 }
 
 void setVisible(bool bVisible) { bVisible_ = bVisible; }
 void setHandler(std::shared_ptr<PositionHandler> handler) { handler_ = handler; }
 std::shared_ptr<ClonableDraw<T>> get() { return graphical_; }
 void update() override 
 {
 if (handler_ != nullptr)
 {
 graphical_->setPosition(handler_->getPosition());
 graphical_->setRotation(handler_->getRotation());
 }
 }
private:
 std::shared_ptr<ClonableDraw<T>> graphical_;
 std::shared_ptr<PositionHandler> handler_;
};

GameEngine.h

#pragma once
#include "GraphicalHandler.h"
#include "SimpleGraphical.h"
#include "PauseButton.h"
#include "PauseButtonIcon.h"
#include "SFML/Graphics.hpp"
#include <memory>
class GameEngine
{
public:
 GameEngine
 (
 std::string&& gameTitle = "New Window", 
 int windowWidth = 800, 
 int windowHeight = 800, 
 sf::Color backgroundColor = sf::Color(sf::Color::White)
 );
 void run();
private:
 bool bPaused_;
 std::unique_ptr<sf::RenderWindow> window_;
 std::shared_ptr<PauseButton> pauseButton_;
 std::unique_ptr<PauseButtonIcon> pauseButtonIcon_;
 std::unique_ptr<SimpleGraphical<sf::RectangleShape>> pauseOverlay_;
 sf::Color windowBackgroundColor_;
};

GameEngine.cpp

#include "GameEngine.h"
GameEngine::GameEngine(std::string&& title, int width, int height, sf::Color backgroundColor)
 : bPaused_(false), windowBackgroundColor_(backgroundColor)
{
 window_ = std::make_unique<sf::RenderWindow>(sf::VideoMode(width, height), title);
 pauseButton_ = std::make_shared<PauseButton>(sf::Vector2f(50.f, 50.f), sf::Vector2f(float(width - 60) , 10.f));
 pauseButtonIcon_ = std::make_unique<PauseButtonIcon>(pauseButton_, sf::Color::White);
 pauseOverlay_ = std::make_unique<SimpleGraphical<sf::RectangleShape>>(sf::Vector2f((float)width, (float)height));
 pauseOverlay_->get()->setFillColor(sf::Color(0, 0, 0, 170));
 pauseOverlay_->setVisible(false);
}
void GameEngine::run()
{
 pauseButton_->moveMyInstanceToLast();
 pauseOverlay_->moveMyInstanceToLast();
 pauseButtonIcon_->moveMyInstanceToLast();
 while (window_->isOpen())
 {
 sf::Event event;
 while (window_->pollEvent(event))
 {
 if (event.type == sf::Event::Closed)
 {
 window_->close();
 break;
 }
 if (!bPaused_)
 {
 LogicHandler::handleEvent(event, *window_);
 }
 else
 {
 pauseButton_->receiveEvent(event, *window_);
 pauseButtonIcon_->update();
 }
 }
 if (!bPaused_)
 {
 LogicHandler::updateLogic();
 GraphicalHandler::updateGraphicals();
 }
 bPaused_ = pauseButton_->bPaused_;
 pauseOverlay_->setVisible(bPaused_);
 window_->clear(windowBackgroundColor_);
 GraphicalHandler::renderObjects(*window_);
 window_->display();
 }
}

That's it for the actual library but as stated above here's a simple little program that uses the library to move around a square with the arrow keys or WASD. I used SimpleGraphical for this, since the square doesn't need to do anything more complicated than just moving to a position during every iteration.

ControllablePlayer.h

#pragma once
#include "PositionHandler.h"
class ControllablePlayer : public PositionHandler
{
public:
 bool receiveEvent(const sf::Event& event, const sf::RenderWindow& window) override;
 void update() override;
};

ControllablePlayer.cpp

#include "ControllablePlayer.h"
bool ControllablePlayer::receiveEvent(const sf::Event& event, const sf::RenderWindow& window)
{
 if (event.type == sf::Event::KeyPressed)
 {
 switch (event.key.code)
 {
 case sf::Keyboard::Left:
 case sf::Keyboard::A:
 position_.x -= 20.f;
 return true;
 case sf::Keyboard::Right:
 case sf::Keyboard::D:
 position_.x += 20.f;
 return true;
 case sf::Keyboard::Down:
 case sf::Keyboard::S:
 position_.y += 20.f;
 return true;
 case sf::Keyboard::Up:
 case sf::Keyboard::W:
 position_.y -= 20.f;
 return true;
 }
 }
 return false;
}
void ControllablePlayer::update()
{
}

main.cpp

#include "GameEngine.h"
#include "ControllablePlayer.h"
#include <SFML/Graphics.hpp>
#include <memory>
int main()
{
 GameEngine engine{"New", 800, 800, sf::Color::Red};
 auto rectHandler = std::make_shared<ControllablePlayer>();
 SimpleGraphical<sf::RectangleShape> rect(sf::Vector2f(100.f, 100.f));
 rect.get()->setFillColor(sf::Color::Green);
 rect.setHandler(rectHandler);
 engine.run();
 return 0;
}

• Mr R: Hi @JensB - SFML? perhaps add a link?
• JensB: @MrR Sorry, I thought everyone here had heard of SFML. It's a library for making 2D games. I've added this link to it in the beginning of the question.

Answer 1

Perfect forwarding

template<typename... Args>
ClonableDraw(Args&... args): T(args...) {}
template<typename... Args>
ClonableDraw(Args&&... args): T(args...) {}

We only need a single constructor here, and we should use std::forward:

template<class... Args>
CloneableDraw(Args&&... args): T(std::forward<Args>(args)...) { }

(Similarly with the SimpleGraphical constructor.)

---

Implicit conversions

operator sf::Drawable& () override { return *this; }

We should avoid implicit conversions, and can do so by making conversion operators explicit:

explicit operator sf::Drawable& () override { return *this; }

If T is an sf::Drawable, then we don't need this operator at all though, so we should probably just delete this function!?

---

Unnecessary allocation

There's a lot of unnecessary std::shared_ptr and std::unique_ptr usage.

• shared_ptr gives us shared ownership of an object. If we don't need that (and aren't using weak_ptr), then we don't need shared_ptr.

• unique_ptr gives us single ownership, and a pointer to an object that will be valid for that object's lifetime. However, it also involves heap allocation, which isn't always necessary.

  If we want to ensure a pointer to an object is valid for an object's lifetime, we need to avoid changing the object's location in memory. We can do that by making the object non-copyable and non-moveable.

C++ does not help us enforce correct lifetimes and pointer usage, so it can be a little scary, but there's nothing wrong with storing raw pointers to objects. We just have to manually ensure the pointer is destroyed before the object.

Using std::shared_ptr for everything might seem safer, but it makes object lifetimes a lot more complicated (which in C++ makes things a lot less safe!).

---

Global variables and access

It seems like a bad idea for every object to have access to every other object through s_allInstances_.

Similarly, every graphics object can access and affect every other object through graphicalObjects_.

Even the moveMyInstance* functions are potentially unsafe, and might lead to a lot of "churn" if two objects don't agree on how they should be ordered.

Similarly the pause button decides by itself to clear the entire set of graphicalObjects_.

Perhaps we could delete the InstanceTracker class, and simply have the GameEngine contain two vectors: std::vector<LogicHandler*> logicHandlers_; and std::vector<GrahpicalHandler*> graphicalHandlers_;.

The engine could sort these objects as necessary.

---

Deep inheritance hierarchies

Runtime-polymorphism with inheritance in C++ gives us the ability to refer to objects of different types with the same interface (i.e. store pointers to them in the same container, and call functions on them as if they were the same type).

While it can be used to add state (member variables) or functionality, it's generally better avoid it, and use composition for this purpose instead.

We should usually avoid deep inheritance hierarchies, and use only a single level of inheritance where possible.

Mixing inheritance and static functions (e.g. in GraphicalHandler and its descendents) is very confusing.

---

Suggestions

A more "normal" way to do it would probably be...

Define a base class for each "interface":

class ILogicHandler
{
public:
 
 virtual ~ILogicHandler() { }
 ILogicHandler(ILogicHandler const&) = delete; // no copy
 ILogicHandler& operator=(ILogicHandler const&) = delete; // no copy
 ILogicHandler(ILogicHandler&&) = delete; // no move
 ILogicHandler& operator=(ILogicHandler&&) = delete; // no move
 virtual void update() = 0;
 virtual bool receiveEvent(const sf::Event& event, const sf::RenderWindow& window) = 0;
};
class IGraphicsHandler
{
public:
 
 virtual ~IGraphicsHandler() { }
 IGraphicsHandler(IGraphicsHandler const&) = delete; // no copy
 IGraphicsHandler& operator=(IGraphicsHandler const&) = delete; // no copy
 IGraphicsHandler(IGraphicsHandler&&) = delete; // no move
 IGraphicsHandler& operator=(IGraphicsHandler&&) = delete; // no move
 
 virtual void render(const sf::RenderWindow& window) = 0;
};

Then we can define a System class for each which contains a vector of pointers, and keep those systems in GameEngine:

class LogicSystem
{
public:
 
 void addHandler(ILogicHandler* handler);
 void removeHandler(ILogicHandler* handler);
 
 void update(); // calls update on all handlers
 void receiveEvent(const sf::Event& event, const sf::RenderWindow& window); // calls receive on all handlers
private:
 std::vector<ILogicHandler*> logicHandlers_;
};
class GraphicsSystem
{
public:
 void addHandler(IGraphicsHandler* handler);
 void removeHandler(IGraphicsHandler* handler);
 void render(const sf::RenderWindow& window); // renders all handlers
 
private:
 
 std::vector<IGraphicsHandler*> graphicsHandlers_;
};
class GameEngine
{
public:
 ...
 
 LogicSystem logicSystem_;
 GraphicsSystem graphicsSystem_;
};

Then we can create an object, something like:

class PauseButton : public ILogicHandler, public IGraphicsHandler
{
public:
 
 void update() override;
 bool receiveEvent(const sf::Event& event, const sf::RenderWindow& window) override;
 void render(const sf::RenderWindow& window) override;
 
private:
 
 bool paused_;
 sf::Rect<float> hitbox_;
 
 sf::VertexArray playIcon_;
 sf::RectangleShape pauseIcon1_;
 sf::RectangleShape pauseIcon2_;
};

Of course, we then need to remember to register the object and (more difficultly) unregister the object with the systems:

PauseButton pauseButton_;
logicSystem_.addHandler(&pauseButton_);
graphicsSystem_.addHandler(&pauseButton_);
...
logicSystem.removeHandler(&pauseButton_);
graphicsSystem.removeHandler(&pauseButton_);

Which is awkward. We can make this slightly easier and safer by changing the base classes to do this automatically:

class ILogicHandler
{
public:
 explicit ILogicHandler(LogicSystem& system):
 system_(system)
 {
 system_.addHandler(this);
 }
 ILogicHandler(ILogicHandler const&) = delete; // no copy
 ILogicHandler& operator=(ILogicHandler const&) = delete; // no copy
 ILogicHandler(ILogicHandler&&) = delete; // no move
 ILogicHandler& operator=(ILogicHandler&&) = delete; // no move
 
 virtual ~ILogicHandler()
 {
 system_.removeHandler(this);
 }
 
 virtual void update() = 0;
 virtual bool receiveEvent(const sf::Event& event, const sf::RenderWindow& window) = 0;
 
private:
 
 LogicSystem& system_;
};

Then add a constructor to PauseButton like so:

 PauseButton(LogicSystem& logicSystem, GraphicsSystem& graphicsSystem):
 ILogicHandler(logicSystem),
 IGraphicsHandler(graphicsSystem)
 { }

A simple drawable object might look something like:

template<class T>
SimpleDrawable : public IGraphicsHandler
{
 template<class... Args>
 explicit SimpleDrawable(GraphicsSystem& graphicsSystem, Args&&... args):
 IGraphicsHandler(graphicsSystem),
 visible_(true),
 drawable_(std::forward<Args>(args)...) { }
 
 void render(const sf::RenderWindow& window) override
 {
 if (visible_)
 window.draw(drawable);
 }
 
 bool visible_;
 T drawable_;
};

Although it is a awkward to pass the systems around to every object that needs them, the advantage is that dependencies are explicit and obvious. If we really wanted, we could turn the LogicSystem and GraphicsSystem into Singleton classes, or use the ServiceLocator pattern.

This kind of design should be completely fine for a simple game. In the long-term, it might also be worth looking into ECS systems, e.g. the Entt library.

• JensB: Thanks for the very thorough and insightful code review! I haven't had time to properly go through your entire answer until today, and after going through it I have a few questions. With your suggestion of replacing the InstanceTracker code with two system classes, where would I create and initialize all my handlers? You suggest to keep the two systems in the game engine class but how would I then access the systems when creating my handlers in main.cpp? Should the systems be public members of the game engine so I can access them in main? Or should my handlers be created in the game engine?
• user673679: Yep, I'd suggest making the systems public members of GameEngine. The game objects can then be created wherever they're needed.

Answer 2

I have never written such a system but I worked a bit with Qt framework that does similar things. And does them well enough to suggest studying its source code. I personally learned a lot from it. Below are moments that I notices

Signals and slots

From my experience, they make life easier (I guess it is the reason why Qt even added special syntax and keywords for them). Today that are great stand-alone libraries as SigSlot.

In your case usage of signals would allow to remove virtual void buttonPressed() = 0; and replace it with member variable sigslot::signal<> clickedSignal;. Then anyone interested in button click would just pButton->clickedSignal.connect(&CAnyone::OnClick, &anyoneInstance);. Signals is a powerful tool that allows to decouple stuff and reduces need for inheritance

Combining input handling and graphics

Qt just have QWidget (one of core classes in library) with numerous virtual methods like

virtual void hideEvent(QHideEvent *event)
virtual void keyPressEvent(QKeyEvent *event)
virtual void keyReleaseEvent(QKeyEvent *event)
virtual void leaveEvent(QEvent *event)
virtual void mousePressEvent(QMouseEvent *event)
virtual void moveEvent(QMoveEvent *event)
virtual void paintEvent(QPaintEvent *event)
virtual void resizeEvent(QResizeEvent *event)
virtual void showEvent(QShowEvent *event)

As you may notice input handling and graphics are mixed tougher in this case. I am sure that such approach has its drawbacks but for me, it seems much more convenient. If I need a button I just inherit from QWidget and reimplement mousePressEvent and paintEvent without a need to create 2 classes that are tightly coupled together and even are friends.

Incapsulating drawing logic into Painter class

Qt has QPainter with methods as

void drawArc(const QRect &rectangle, int startAngle, int spanAngle)
void drawEllipse(const QRectF &rectangle)
void drawGlyphRun(const QPointF &position, const QGlyphRun &glyphs)
void drawImage(const QRectF &target, const QImage &image, const QRectF &source, Qt::ImageConversionFlags flags = Qt::AutoColor)
void drawLine(const QLineF &line)
void drawLines(const QVector<QPoint> &pointPairs)
void drawPath(const QPainterPath &path)
void drawPicture(const QPointF &point, const QPicture &picture)
void setBrush(Qt::BrushStyle style)
void setPen(Qt::PenStyle style)

so in the case of a button

void CButton::paintEvent(QPaintEvent *)
{
 QPainter painter(this);
 painter.setPen(Qt::black);
 painter.setFont(QFont("Arial", 30));
 painter.drawText(rect(), Qt::AlignCenter, "click me");
 painter.setBruch(QBrush(Qt::yellow));
 painter.drawRect(rect());
}

I suggest using similar approach because it hides all low-level drowing details and provides clear API

I think at this point it is clear that I would be going suggesting to make stuff as it is made in Qt framework. So I better stop myself because most of it can easily be learned by looking at Qt source and at numerous example project that this library comes with.

Forward declaration

To reduce compile time I would suggest removing as many includes from the header as possible and move them to source files forward declaring stuff. Example

#pragma once
#include "SimpleGraphical.h"
#include "SFML/Color.hpp"
#include <memory>
class PauseButton;
class PauseButtonIcon;
namespace sf
{
 class RenderWindow; //that's how to forward declare stuff in namespaces
 class RectangleShape;
}
class GameEngine
{
public:
 GameEngine
 (
 std::string&& gameTitle = "New Window", 
 int windowWidth = 800, 
 int windowHeight = 800, 
 sf::Color backgroundColor = sf::Color(sf::Color::White)
 );
 ~GameEngine(); //for forward declaration to work for unique pointer you need to have destructor in .cpp file
 void run();
private:
 bool bPaused_;
 std::unique_ptr<sf::RenderWindow> window_;
 std::shared_ptr<PauseButton> pauseButton_;
 std::unique_ptr<PauseButtonIcon> pauseButtonIcon_;
 std::unique_ptr<SimpleGraphical<sf::RectangleShape>> pauseOverlay_;
 sf::Color windowBackgroundColor_;
};

Minor things

• That is controversial but I would suggest using a macro like FORWARD_CLASS. So that

std::weak_ptr<PauseButton> m_wpButton;
std::unique_ptr<PauseButton> m_upButton;
std::shared_ptr<PauseButton> m_spButton;

Becomes

FORWARD_CLASS(PauseButton);
PauseButtonWPtr m_wpButton;
PauseButtonUPtr m_upButton;
PauseButtonSPtr m_spButton;

I hope it is clear how FORWARD_CLASS can be implemented using some macro magic/ugliness. Such an approach makes code a bit cleaner (especially when you pass poiters around) and handles forward declaration.

• Passing smart pointers by const reference