First game in C++ Snake2d

Question

I decided to dig into game programming, and here is my first C++ project snake2d game. Please review it. I've used X11 for Linux and Win32 API for Windows build. For graphics and rendering part I've asked ChatGPT to assist me, so not much review is gonna help there. The reason I want the community to review is the overall architecture and other game logic part implementations. Any advice for the next steps will be appreciated.

Vec2.hpp

#ifndef BEBOP_SNAKE_2D_GAME_VEC2_HPP
#define BEBOP_SNAKE_2D_GAME_VEC2_HPP
namespace bebop::snake2d::game
{
/**
 * Simple struct representing a vector in a 2D dimension
 */
struct Vec2
{
 int x, y;
 bool operator==(const Vec2& other) const;
 Vec2& operator+=(const Vec2& other);
};
} // namespace bebop::snake2d::game
#endif // BEBOP_SNAKE_2D_GAME_VEC2_HPP

Vec2.cpp

#include "Vec2.hpp"
bool bebop::snake2d::game::Vec2::operator==(const Vec2& other) const
{
 return x == other.x && y == other.y;
}
bebop::snake2d::game::Vec2& bebop::snake2d::game::Vec2::operator+=(const Vec2& other)
{
 x += other.x;
 y += other.y;
 return *this;
}

Snake.hpp

#ifndef BEBOP_SNAKE_2D_GAME_SNAKE_HPP
#define BEBOP_SNAKE_2D_GAME_SNAKE_HPP
#include <algorithm>
#include <vector>
#include "../render/IWindow.hpp"
#include "Vec2.hpp"
namespace bebop::snake2d::game
{
/**
 * Struct representing a snake direction and body segments
 */
struct Snake
{
 Snake(const Vec2& position, const Vec2& direction, int segmentsCount);
 /**
 * Get the head of the snake
 * @returns The head of the snake
 */
 const Vec2& head() const;
 /**
 * Set direction of the snake, no opposite direction is allowed
 * @param newDirection The new direction to be set
 */
 void setDirection(const Vec2& newDirection);
 /**
 * Moves the snake based on frame delta passed
 * @param dt The delta time between the frames
 */
 void move(float dt);
 /**
 * Checks whether a snake eats itself
 */
 bool eatsItself() const;
 /**
 * Grows the snake by one segment
 */
 void grow();
 float moveTimer = 0.0f;
 static constexpr auto MOVE_DELAY = 0.1f;
 Vec2 direction;
 std::vector<Vec2> segments;
};
/**
 * Utility class to render a snake
 */
struct RenderSnake
{
 /**
 * Renders the snake on the window
 * @param snake The snake going to be rendered
 * @param window The window on which the snake is going to be rendered
 */
 static void render(const Snake& snake, render::IWindow& window);
 private:
 static constexpr auto CIRCLE_RADIUS = 10;
 static constexpr auto EYE_RADIUS = CIRCLE_RADIUS / 4;
 static constexpr auto EYE_OFFSET_X = CIRCLE_RADIUS / 2;
 static constexpr auto EYE_OFFSET_Y = CIRCLE_RADIUS / 3;
 static constexpr auto HEAD_COLOR = 0xFF00FF00;
 static constexpr auto BODY_COLOR = 0xFF007700;
 static constexpr auto EYE_COLOR = 0xFF000000;
};
} // namespace bebop::snake2d::game
#endif // BEBOP_SNAKE_2D_GAME_SNAKE_HPP

Snake.cpp

#include "Snake.hpp"
bebop::snake2d::game::Snake::Snake(const Vec2& position, const Vec2& direction, int segmentsCount)
 : direction(direction)
{
 segments.reserve(segmentsCount);
 for (int i = 0; i < segmentsCount; ++i)
 {
 Vec2 segmentPos{position.x - direction.x * i, position.y - direction.y * i};
 segments.emplace_back(segmentPos);
 }
}
const bebop::snake2d::game::Vec2& bebop::snake2d::game::Snake::head() const
{
 return segments.front();
}
void bebop::snake2d::game::Snake::setDirection(const Vec2& newDirection)
{
 if (newDirection.x != 0 && newDirection.x == -direction.x)
 return;
 if (newDirection.y != 0 && newDirection.y == -direction.y)
 return;
 direction = newDirection;
}
void bebop::snake2d::game::Snake::move(float dt)
{
 moveTimer += dt;
 if (moveTimer < MOVE_DELAY)
 return;
 moveTimer = 0.0f;
 if (direction.x == 0 && direction.y == 0)
 return;
 // Move body from tail to front
 for (int i = segments.size() - 1; i > 0; --i)
 {
 segments[i] = segments[i - 1];
 }
 // Move head based on direction
 segments[0] += direction;
}
bool bebop::snake2d::game::Snake::eatsItself() const
{
 return std::find(segments.begin() + 1, segments.end(), head()) != segments.end();
}
void bebop::snake2d::game::Snake::grow()
{
 Vec2 tail = segments.back();
 segments.emplace_back(tail);
}
void bebop::snake2d::game::RenderSnake::render(const Snake& snake, render::IWindow& window)
{
 // Render the head
 Vec2 head{snake.head().x * CIRCLE_RADIUS * 2, snake.head().y * CIRCLE_RADIUS * 2};
 window.drawCircle(head.x, head.y, CIRCLE_RADIUS, RenderSnake::HEAD_COLOR);
 // Render the eyes
 Vec2 leftEye{head.x - EYE_OFFSET_X, head.y};
 Vec2 rightEye{head.x + EYE_OFFSET_X, head.y};
 window.drawCircle(leftEye.x, leftEye.y, EYE_RADIUS, RenderSnake::EYE_COLOR);
 window.drawCircle(rightEye.x, rightEye.y, EYE_RADIUS, RenderSnake::EYE_COLOR);
 // Render the body
 for (int i = 1; i < snake.segments.size(); ++i)
 {
 Vec2 body{snake.segments[i].x * CIRCLE_RADIUS * 2, snake.segments[i].y * CIRCLE_RADIUS * 2};
 window.drawCircle(body.x, body.y, CIRCLE_RADIUS, RenderSnake::BODY_COLOR);
 }
}

Fruit.hpp

#ifndef BEBOP_SNAKE_2D_GAME_FRUIT_HPP
#define BEBOP_SNAKE_2D_GAME_FRUIT_HPP
#include "../render/IWindow.hpp"
#include "Vec2.hpp"
namespace bebop::snake2d::game
{
/**
 * Fruit is just a Vec2, nothing more
 */
using Fruit = Vec2;
struct RenderFruit
{
 static constexpr auto CIRCLE_RADIUS = 10;
 static constexpr auto FRUIT_COLOR = 0xFFFFFF00;
 /**
 * Renders the fruit on the window
 * @param fruit The fruit going to be rendered
 * @param window The window on which the fruit is going to be rendered
 */
 static void render(const Fruit& fruit, render::IWindow& window);
};
} // namespace bebop::snake2d::game
#endif // BEBOP_SNAKE_2D_GAME_FRUIT_HPP

Fruit.cpp

#include "Fruit.hpp"
void bebop::snake2d::game::RenderFruit::render(const Fruit& fruit, render::IWindow& window)
{
 window.drawCircle(fruit.x * CIRCLE_RADIUS * 2, fruit.y * CIRCLE_RADIUS * 2, CIRCLE_RADIUS, FRUIT_COLOR);
}

Game.hpp

#ifndef BEBOP_SNAKE_2D_GAME_GAME_HPP
#define BEBOP_SNAKE_2D_GAME_GAME_HPP
#include <chrono>
#include <random>
#include <thread>
#include <memory>
#include <array>
#include "Fruit.hpp"
#include "Snake.hpp"
#include "Vec2.hpp"
#ifdef PLATFORM_WINDOWS
 #include "../render/Win32Window.hpp"
#elif defined(PLATFORM_LINUX)
 #include "../render/XWindow.hpp"
#endif
namespace bebop::snake2d::game
{
/**
 * The game struct holding all game objects and the logic
 */
struct Game
{
 enum class GameState
 {
 PLAYING,
 PAUSED,
 GAME_OVER
 };
 Game(int width, int height);
 /**
 * Run the game
 */
 void run();
 private:
 int width;
 int height;
 static constexpr auto FRUIT_COUNT = 5;
 static constexpr auto CIRCLE_RADIUS = 10;
 static constexpr auto WINDOW_BACKGROUND_COLOR = 0xFF222222;
 static constexpr auto GAME_OVER_BACKGROUND_COLOR = 0xFF111111;
 static constexpr auto TEXT_COLOR = 0xFFFFFFFF;
 /**
 * Checks whether the snake hits the walls(the edge of the screen)
 */
 bool checkSnakeCollisionWithWalls();
 /**
 * Check whether the snake eats a fruit
 * @returns The index of the fruit its going to eat, and -1 if no fruit is eaten
 */
 int snakeEatsFruit();
 /**
 * Spawn a fruit in a random free grid and return it
 */
 Fruit spawnFruit();
 /**
 * Shows the score on the top right corner of the screen
 */
 void showScore();
 /**
 * Function that ends the game and shows the game over screen
 */
 void gameOver();
 /**
 * Restart the game
 */
 void restart();
 Snake snake;
 std::array<Fruit, FRUIT_COUNT> fruits;
 GameState state = GameState::PLAYING;
 int score = 0;
 std::unique_ptr<render::IWindow> window;
};
} // namespace bebop::snake2d::game
#endif // BEBOP_SNAKE_2D_GAME_GAME_HPP

Game.cpp

#include "Game.hpp"
bebop::snake2d::game::Game::Game(int width, int height)
 : width(width), height(height), snake({10, 10}, {1, 0}, 5)
{
#ifdef PLATFORM_WINDOWS
 window = std::make_unique<render::Win32Window>(width, height, "Snake2D");
#elif defined(PLATFORM_LINUX)
 window = std::make_unique<render::XWindow>(width, height, "Snake2D");
#endif
 int spawned = 0;
 while (spawned < FRUIT_COUNT)
 {
 fruits[spawned] = spawnFruit();
 ++spawned;
 }
}
void bebop::snake2d::game::Game::run()
{
 auto lastTime = std::chrono::steady_clock::now();
 while (window->isOpen())
 {
 auto now = std::chrono::steady_clock::now();
 float dt = std::chrono::duration<float>(now - lastTime).count();
 lastTime = now;
 window->processEvents(
 [&](render::Key key)
 {
 switch (key)
 {
 case render::Key::UP:
 snake.setDirection({0, -1});
 break;
 case render::Key::DOWN:
 snake.setDirection({0, 1});
 break;
 case render::Key::LEFT:
 snake.setDirection({-1, 0});
 break;
 case render::Key::RIGHT:
 snake.setDirection({1, 0});
 break;
 case render::Key::p:
 state =
 state == GameState::PLAYING ? GameState::PAUSED : GameState::PLAYING;
 break;
 case render::Key::r:
 if (state != GameState::GAME_OVER)
 break;
 restart();
 break;
 case render::Key::ESCAPE:
 window->close();
 }
 });
 if (state == GameState::GAME_OVER)
 {
 gameOver();
 window->update();
 std::this_thread::sleep_for(std::chrono::milliseconds(16));
 continue;
 }
 if (snake.eatsItself() || checkSnakeCollisionWithWalls())
 {
 state = GameState::GAME_OVER;
 continue;
 }
 int fruitIndex = snakeEatsFruit();
 if (fruitIndex != -1)
 {
 ++score;
 fruits[fruitIndex] = spawnFruit();
 snake.grow();
 }
 if (state != GameState::PAUSED)
 snake.move(dt);
 window->clear(WINDOW_BACKGROUND_COLOR);
 RenderSnake::render(snake, *window);
 std::for_each(fruits.begin(), fruits.end(),
 [&](const auto& fruit) { RenderFruit::render(fruit, *window); });
 showScore();
 window->update();
 std::this_thread::sleep_for(std::chrono::milliseconds(16)); // ~60 FPS
 }
}
bool bebop::snake2d::game::Game::checkSnakeCollisionWithWalls()
{
 const int GRID_WIDTH = width / (CIRCLE_RADIUS * 2);
 const int GRID_HEIGHT = height / (CIRCLE_RADIUS * 2);
 const auto& head = snake.head();
 return head.x < 0 || head.x >= GRID_WIDTH || head.y < 0 || head.y >= GRID_HEIGHT;
}
int bebop::snake2d::game::Game::snakeEatsFruit()
{
 auto it = std::find(fruits.begin(), fruits.end(), snake.segments.front());
 return it == fruits.end() ? -1 : std::distance(fruits.begin(), it);
}
bebop::snake2d::game::Fruit bebop::snake2d::game::Game::spawnFruit()
{
 std::mt19937 rng(std::random_device{}());
 std::uniform_int_distribution<int> xdist(0, width / (CIRCLE_RADIUS * 2) - 1);
 std::uniform_int_distribution<int> ydist(0, height / (CIRCLE_RADIUS * 2) - 1);
 Fruit pos{xdist(rng), ydist(rng)};
 auto segmentsFound = std::find(snake.segments.begin(), snake.segments.end(), pos);
 auto spawnedFound = std::find(fruits.begin(), fruits.end(), pos);
 bool collides = segmentsFound != snake.segments.end() || spawnedFound != fruits.end();
 while (collides)
 {
 pos = Vec2{xdist(rng), ydist(rng)};
 auto segmentsFound = std::find(snake.segments.begin(), snake.segments.end(), pos);
 auto spawnedFound = std::find(fruits.begin(), fruits.end(), pos);
 collides = segmentsFound != snake.segments.end() || spawnedFound != fruits.end();
 }
 return pos;
}
void bebop::snake2d::game::Game::showScore()
{
 std::string scoreText = "Score: " + std::to_string(score);
 int textX = width - 200;
 int textY = 10;
 window->drawText(textX, textY, scoreText, TEXT_COLOR);
}
void bebop::snake2d::game::Game::gameOver()
{
 window->clear(GAME_OVER_BACKGROUND_COLOR);
 constexpr const char* text = "Game Over! Press R to Restart";
 int textX = width / 2 - 200;
 int textY = height / 2;
 window->drawText(textX, textY, text, TEXT_COLOR);
}
void bebop::snake2d::game::Game::restart()
{
 snake = Snake({5, 10}, {1, 0}, 5);
 std::fill(fruits.begin(), fruits.end(), Fruit{0, 0});
 int spawned = 0;
 while (spawned < FRUIT_COUNT)
 {
 fruits[spawned] = spawnFruit();
 ++spawned;
 }
 state = GameState::PLAYING;
 score = 0;
}

IWindow.hpp

#ifndef BEBOP_SNAKE_2D_RENDER_IWINDOW_HPP
#define BEBOP_SNAKE_2D_RENDER_IWINDOW_HPP
#include <cstdint>
#include <functional>
#include <string>
namespace bebop::snake2d::render
{
enum class Key
{
 UNKNOWN,
 UP,
 DOWN,
 LEFT,
 RIGHT,
 ESCAPE,
 p,
 r
};
using KeyCallback = std::function<void(Key)>;
struct IWindow
{
 /**
 * Clear the window with the given color
 * @param color The color to be cleared with
 */
 virtual void clear(std::uint32_t) = 0;
 /**
 * Draws a circle on the window
 * @param cx The x coordinate of the center
 * @param cy The y coordinate of the center
 * @param radius The radius of the circle
 * @param color The color of the circle
 */
 virtual void drawCircle(int x, int y, int radius, std::uint32_t color) = 0;
 /**
 * Draws a text on the window
 * @param x The x coordinate of the text
 * @param y The y coordinate of the text
 * @param text The text
 * @param color The color
 */
 virtual void drawText(int x, int y, const std::string& text, std::uint32_t color) = 0;
 /**
 * Update is called to flush the display
 */
 virtual void update() = 0;
 /**
 * Process window events
 * @param onKeyPress The listener possibly nullptr, passed to this function.
 */
 virtual void processEvents(const KeyCallback& callback = nullptr) = 0;
 /**
 * Closes the window
 */
 virtual void close() = 0;
 /**
 * Boolean reporting whether the window is still open... used in loops
 */
 virtual bool isOpen() const = 0;
};
} // namespace bebop::snake2d::render
#endif // BEBOP_SNAKE_2D_RENDER_IWINDOW_HPP

The X11Window class and Win32Window class use X11 api and Win32 API respectively to implement the IWindow interface.

Answer 1

Let me start the review by saying this is one of the best-written game examples I think I’ve ever seen. Not just as a first attempt... but one of the best period. The code is all very clean, well-formatted, and well-commented. And the overall structure is excellent.

There are a few small bugs and other issues here and there, but, most of my suggestions will be more-or-less nitpicks.

Use the build system to handle different platforms

Let me pull out a small bit of the code:

// render/iwindow.hpp //////////////////////////
namespace bebop::snake2d::render {
struct IWindow
{
 // pure abstract interface...
};
} // namespace bebop::snake2d::render
// game.hpp ////////////////////////////////////
#ifdef PLATFORM_WINDOWS
 #include "../render/Win32Window.hpp"
#elif defined(PLATFORM_LINUX)
 #include "../render/XWindow.hpp"
#endif
namespace bebop::snake2d::game {
struct Game
{
 // ... [snip] ...
 std::unique_ptr<render::IWindow> window;
};
} // namespace bebop::snake2d::game
// game.cpp ////////////////////////////////////
#include "Game.hpp"
bebop::snake2d::game::Game::Game(int width, int height)
 : width(width), height(height), snake({10, 10}, {1, 0}, 5)
{
#ifdef PLATFORM_WINDOWS
 window = std::make_unique<render::Win32Window>(width, height, "Snake2D");
#elif defined(PLATFORM_LINUX)
 window = std::make_unique<render::XWindow>(width, height, "Snake2D");
#endif
 // ... [snip] ...
}

I know that it is widespread practice to use the preprocessor to handle multiple platforms... but it is widespread bad practice. The correct tool for handling different platforms is not the preprocessor, it is the build system.

First, let’s pull the platform-specific includes out of the game.hpp header, because they don’t need to be there. All you’re using in Game is the abstract base IWindow.

This alone will already have an enormous effect, because now every file that includes Game.hpp will not be subject to the vagaries of different platforms.

Now all the platform-selection stuff is localized in Game.cpp, and everything else is platform-agnostic:

// game.hpp ////////////////////////////////////
#include "../render/IWindow.hpp"
namespace bebop::snake2d::game {
struct Game
{
 // ... [snip] ...
 std::unique_ptr<render::IWindow> window;
};
} // namespace bebop::snake2d::game
// game.cpp ////////////////////////////////////
#ifdef PLATFORM_WINDOWS
 #include "../render/Win32Window.hpp"
#elif defined(PLATFORM_LINUX)
 #include "../render/XWindow.hpp"
#endif
#include "Game.hpp"
bebop::snake2d::game::Game::Game(int width, int height)
 : width(width), height(height), snake({10, 10}, {1, 0}, 5)
{
#ifdef PLATFORM_WINDOWS
 window = std::make_unique<render::Win32Window>(width, height, "Snake2D");
#elif defined(PLATFORM_LINUX)
 window = std::make_unique<render::XWindow>(width, height, "Snake2D");
#endif
 // ... [snip] ...
}

Now let’s add a new function to the renderer interface:

// render/iwindow.hpp //////////////////////////
namespace bebop::snake2d::render {
struct IWindow
{
 // pure abstract interface...
};
// Could also be a static member function of IWindow:
auto make_window(int, int, std::string_view) -> std::unique_ptr<IWindow>;
} // namespace bebop::snake2d::render

And now let’s go back to the Game implementation file, where we no longer need to care about platform-specific shenanigans:

// game.cpp ////////////////////////////////////
#include "Game.hpp"
bebop::snake2d::game::Game::Game(int width, int height)
 : width(width), height(height), snake({10, 10}, {1, 0}, 5)
{
 window = make_window(width, height, "Snake2D");
 // ... [snip] ...
}

But where do we define the make_window() function? We do it in the implementation file for each platform:

// render/Win32Window.cpp //////////////////////
#include "IWindow.hpp"
class Win32Window : public IWindow
{
 // ... implement window interface ...
};
auto make_window(int w, int h, std::string_view title) -> std::unique_ptr<IWindow>
{
 return std::make_unique<Win32Window>(w, h, title);
}
// render/XWindow.cpp //////////////////////////
#include "IWindow.hpp"
class XWindow : public IWindow
{
 // ... implement window interface ...
};
auto make_window(int w, int h, std::string_view title) -> std::unique_ptr<IWindow>
{
 return std::make_unique<XWindow>(w, h, title);
}

And then, using the build system, compile and link Win32Window.cpp if and only if the platform is Windows, and compile and link XWindow.cpp if and only if the platform is Linux (or anything else running X11).

See? Other than the includes (which are unavoidable until you adopt modules), there is not a single use of the preprocessor.

What do you gain from this? Quite a bit actually.

Now all of the Windows-specific stuff is localized in Win32Window.cpp, and all Linux-specific stuff is localized in XWindow.cpp. If you have a team of coders, you can restrict the Windows developers to only touch Win32Window.cpp, and the Linux developers to only touch XWindow.cpp. This reduces the chance of bugs or nefarious code being slipped into the project, because the people who are working on a specific platform will be more focused.

But even if you’re just a single developer and don’t have to worry about managing a large group of contributors, this structure is still beneficial. It makes it trivial to add a new platform. None of the rest of the game’s code need ever be touched, so if the game works perfectly on Windows and Linux, you can add MacOS support without any worries about breakage. Just make a new platform-specific source file (or directory, if there will be multiple files), and point your build system to use it instead of the other platform stuff.

This also makes it especially easy for someone else to fork your project to add support for a new platform.

And there are other benefits, too, like improved build times.

This is the way to build a multi-platform application. I know, I know, pretty much everywhere else you look, people are doing it with conditional compilation. But that is just not the right tool for this job.

You need a "renderable" interface

You currently have two objects in the game: the snake, and some fruits. For each of those, you have a type. And for each of those, you have a Render(?) type: so Snake and RenderSnake, and Fruit and RenderFruit. And in Game.run(), you have to handle rendering those separately:

void bebop::snake2d::game::Game::run()
{
 // ... [snip] ...
 while (window->isOpen())
 {
 // ... [snip] ...
 window->clear(WINDOW_BACKGROUND_COLOR);
 RenderSnake::render(snake, *window);
 std::for_each(fruits.begin(), fruits.end(),
 [&](const auto& fruit) { RenderFruit::render(fruit, *window); });
 // ... [snip] ...
 }
}

This "works", but is clunky, and would be difficult to expand. What if you wanted to add something new, like, say, "bombs", that, unlike the fruits, the snake has to avoid lest it gets blown up.

What if, instead, you make an IRenderable interface, like so:

struct IRenderable
{
 virtual ~IRenderable() = default;
 virtual auto render(IWindow& canvas) -> void = 0;
};

Now, throw away the SnakeRender and FruitRender classes, and just make both Snake and Fruit use the IRenderable interface:

struct Snake : public IRenderable
{
 // ... [snip] ...
 auto render(IWindow& canvas) -> void override;
};
auto Snake::render(IWindow& canvas) -> void
{
 // draw the snake
}
struct Fruit : public IRenderable
{
 // ... [snip] ...
 auto render(IWindow& canvas) -> void override;
};
auto Fruit::render(IWindow& canvas) -> void
{
 canvas.drawCircle(x * CIRCLE_RADIUS * 2, y * CIRCLE_RADIUS * 2, CIRCLE_RADIUS, FRUIT_COLOR);
}

Now in your game class, you could have a simple vector of pointers to all renderable types like so:

struct Game
{
 // ... [snip] ...
 std::vector<std::unique_ptr<IRenderable>> objects;
};
Game::::run()
{
 // ... [snip] ...
 while (window->isOpen())
 {
 // ... [snip] ...
 window->clear(WINDOW_BACKGROUND_COLOR);
 // Draw *EVERYTHING*: the snake, the fruits, and anything else
 // you haven't thought of yet.
 std::ranges::for_each(objects, [&window] (auto&& object) { object->render(window); });
 // ... [snip] ...
 }
}

Or you could keep the snake and fruits separately, if you prefer, and just use a view of all renderables:

struct Game
{
 // ... [snip] ...
 Snake snake;
 std::array<Fruit, FRUIT_COUNT> fruits;
};
Game::::run()
{
 // ... [snip] ...
 auto renderables = std::vector<IRenderable*>{};
 renderables.push_back(&snake);
 std::ranges::transform(fruits, std::back_inserter(renderables), [](auto&& fruit) { return &fruit; });
 // ... add any other renderables (possibly dynamically as the
 // game plays out)...
 while (window->isOpen())
 {
 // ... [snip] ...
 window->clear(WINDOW_BACKGROUND_COLOR);
 std::ranges::for_each(renderables, [&window] (auto&& object) { object->render(window); });
 // ... [snip] ...
 }
}

The point is that each game object should know how to render itself, so that as you add new objects, they will automatically be added to the render with no extra work.

Refactor the run function

Most of the functions in the project are short and focused. The one exception is Game::run(), which is rather large and contains all of the input handling logic, quite a bit of the simulation logic, and the rendering logic.

The classic (single-threaded!) game loop structure is:

while (not done)
{
 input();
 update();
 render();
}

Your game loop has this basic structure, more or less. It’s just hard to see. I would suggest refactoring the content of Game::run() out into those functions.

But I would actually suggest going a step further.

Right now you use an enumeration to keep track of the game state. I propose to create actual state classes instead, and use a state machine to control the game state. For example:

// State interface
struct IState
{
 virtual ~IState() = default;
 virtual auto input() -> void = 0;
 virtual auto update() -> std::unique_ptr<IState> = 0;
 virtual auto render() -> void = 0;
};
// Concrete states
struct PlayingState : public IState
{
 auto input() -> void override;
 auto update() -> std::unique_ptr<IState> override;
 auto render() -> void override;
};
struct PausedState : public IState
{
 auto input() -> void override;
 auto update() -> std::unique_ptr<IState> override;
 auto render() -> void override;
};
struct GameOverState : public IState
{
 auto input() -> void override;
 auto update() -> std::unique_ptr<IState> override;
 auto render() -> void override;
};
auto Game::run() -> void
{
 auto state = std::unique_ptr<IState>{};
 // Start in the playing state
 state = std::make_unique<PlayingState>();
 while (state)
 {
 state->input();
 auto next_state = state->update();
 state->render();
 state = std::move(next_state);
 }
}

Basically, after processing the input, the current state does all of its logic and decides whether to continue in the current state, or to change to a new state. For example, while in the playing state, if the snake hits its tail, the update() function will return a new GameOverState. Otherwise, the state will return the current state, and continue. When the game is finally and truly done, the update function can return nullptr.

The above is a very rough description of the idea, and you could go a lot further with it. For example, you could make a push-down automata. Instead of a single state, you would have a stack of states. When you are in the "playing state" and the user enters the pause command, you could push a new "pause state" on top of the "playing state". Then when the users ends the pause, you simply pop the pause state, and fall back to the currently-in-progress playing state. Basically, you want a system where a state can either push a new state on top of itself (useful for pauses or submenus and so on), or replace itself with a new state (for when the playing state needs to end and be replaced with the game over state, for example). The entire game ends when the state stack has been emptied.

But even just a basic state machine would be a massive improvement. Separating each game state’s logic into its own state class will clean up the code considerably. Only the "game over" state needs to know how to render the game over screen, how long to display it for, and then what to do next (start a new "playing" state). Only the "playing" state needs to keep track of the snake and fruits, and all the simulation logic. Everything gets cleanly isolated, and it becomes pretty trivial to add new states. For example, maybe a startup screen. Maybe a "goodbye" credits screen shown for a second or two when shutting down the game. And so on.

Code review

struct Vec2
{
 int x, y;
 bool operator==(const Vec2& other) const;
 Vec2& operator+=(const Vec2& other);
};

One declaration per line, please.

There is no good reason to put the implementations of the functions in a separate implementation file. Doing so prevents inlining, and these operations are perfect for inlining.

If you’re going to have the plus-assignment operator, you might as well have the plus operator as well. It’s practically free.

Also, you can default the comparison, and there’s really no good reason not to:

struct Vec2
{
 int x;
 int y;
 constexpr auto operator==(Vec2 const&) const noexcept -> bool = default;
 constexpr auto operator+=(Vec2 const& v) -> Vec2&
 {
 x += v.x;
 y += v.y;
 return *this;
 }
}
constexpr auto operator+(Vec2 const& a, Vec2 const& b)
{
 auto r = a;
 a += b;
 return a;
}

This class is serviceable enough as a start, but if you are going to be making more games, and more complex games, you will find you want to expand its capabilities. You should plan from the start to allow for more power.

For example, you might want to allow floating point vectors, which would be useful for linear interpolation. And then you may want 3- or even 4-D vectors.

Given those possibilities, you might want to make Vec2 a template right from the start:

template<typename T, std::size_t N>
struct Vec;
template<typename T>
struct Vec<T, 2>
{
 T x;
 T y;
 constexpr auto operator==(Vec const&) const noexcept -> bool = default;
 constexpr auto operator+=(Vec const& v) -> Vec2&
 {
 x += v.x;
 y += v.y;
 return *this;
 }
};
template<typename T, std::size_t N>
constexpr auto operator+(Vec<T, N> const& a, Vec<T, N> const& b)
{
 auto r = a;
 a += b;
 return r;
}
// At some point in the future...
template<typename T>
struct Vec<T, 3>
{
 T x;
 T y;
 T z;
 constexpr auto operator==(Vec const&) const noexcept -> bool = default;
 constexpr auto operator+=(Vec const& v) -> Vec2&
 {
 x += v.x;
 y += v.y;
 z += v.z;
 return *this;
 }
};
// And maybe even a 4-D vector with x, y, z, and w.

You can even alias with using Vec2 = Vec<int, 2>;, if you like.

To be clear, I am not saying there is anything wrong with your existing vector class. I’m just saying that if you want to do more game programming, you will inevitably want a more powerful library of geometric types like points, vectors, and so on, in two and three dimensions. Might as well start coding with that goal in sight now.

On to the Snake class.

You do well to make certain operations const when they shouldn’t logically change the state of the snake. But you should also considering making operations noexcept when they can’t possibly fail. For example, there is no conceivable situation where a call to .eatsItself() can fail; either the snake is eating itself or it is not, and it will always be possible to determine that. So .eatsItself() could be marked noexcept, to signal that it is no-fail.

Now you have several functions—including the constructor—in Snake that have preconditions, but you never check for them. Some of them you mention in comments, like you can’t turn the snake back onto itself in .setDirection(). But others go unmentioned. For example, the constructor is Snake(const Vec2& position, const Vec2& direction, int segmentsCount), and it is assumed that segmentsCount is at least 1... because if it is not, the segments vector will be empty, and the call to segments.front() in .head() will trigger UB.

You need to be more explicit about the preconditions and postconditions of each function, and the invariants of the class itself. One of the invariants of Snake appears to be that there will always be at least one segment. You should make sure that invariant is set up correctly in every constructor, and maintained in every member function. Once you start being explicit about preconditions, postconditions, and invariants, you will be able to spot potential bugs, and guard against them, before they can even happen.

Now, onto RenderSnake... I’ve already suggested that this should not be a separate class, but rather Snake should model an IRenderable interface, and the .render() function should be in Snake. But there is another issue I’d like to focus on:

struct RenderSnake
{
 /**
 * Renders the snake on the window
 * @param snake The snake going to be rendered
 * @param window The window on which the snake is going to be rendered
 */
 static void render(const Snake& snake, render::IWindow& window);
 private:
 static constexpr auto CIRCLE_RADIUS = 10;
 static constexpr auto EYE_RADIUS = CIRCLE_RADIUS / 4;
 static constexpr auto EYE_OFFSET_X = CIRCLE_RADIUS / 2;
 static constexpr auto EYE_OFFSET_Y = CIRCLE_RADIUS / 3;
 static constexpr auto HEAD_COLOR = 0xFF00FF00;
 static constexpr auto BODY_COLOR = 0xFF007700;
 static constexpr auto EYE_COLOR = 0xFF000000;
};

You should not name variables—or constants—using SCREAMING_SNAKE_CASE.

The reason is that SCREAMING_SNAKE_CASE is conventionally reserved for preprocessor macros. If you name your variables in that style, you risk them being usurped by macros in third-party libraries, and then all hell can break loose.

By the same token, do not use SCREAMING_SNAKE_CASE for enumerators. In other words, this is a no-no:

struct Game
{
 enum class GameState
 {
 PLAYING,
 PAUSED,
 GAME_OVER
 };

Now, in all your source files, you do not use a namespace scope (though, in your headers you do). Why? In what way is this:

const bebop::snake2d::game::Vec2& bebop::snake2d::game::Snake::head() const
{
 return segments.front();
}

... better than this:

namespace bebop::snake2d::game {
// ...
const Vec2& Snake::head() const
{
 return segments.front();
}
// ...
} // namespace bebop::snake2d::game

Mind you, this particular function should probably be inlined in the header. It is just a basic accessor, and there doesn’t seem to be any conceivable reason to do any other work than just returning the snake’s head location.

bool bebop::snake2d::game::Snake::eatsItself() const
{
 return std::find(segments.begin() + 1, segments.end(), head()) != segments.end();
}

This could be simplified with contains():

bool Snake::eatsItself() const noexcept
{
 return std::ranges::contains(++segments.begin(), segments.end(), head());
}

On to Fruit:

/**
 * Fruit is just a Vec2, nothing more
 */
using Fruit = Vec2;

That is not correct. A fruit is not just a vector. A fruit is much, much more than just a vector... and much less.

If a fruit were just a vector, then you could create a snake out of two fruits and a segment count:

auto fruit_1 = Fruit{};
auto fruit_2 = Fruit{};
auto snake = Snake{fruit_1, fruit_2, 3};

If fruit were just a vector, then you could add two fruits:

auto fruit_1 = Fruit{};
auto fruit_2 = Fruit{};
auto what_the_hell_is_this = fruit_1 + fruit_2;

All of that is nonsense. A fruit has a vector (technically, it has a position, which is not a vector, but can be inferred from one, but that’s really getting into the weeds of what a good geometry library would need to consider). It may also have more—you may have fruits with different colours, or maybe you want some variation in fruit appearance: some are rendered as cherries, others as bananas, and so on. It may even just have some animation state: like, if the fruits are rendered in such a way that they pulse or wiggle or something, to make them stand out more, you might want to keep track of the current pulse intensity or wiggle angle for rendering purposes.

Even if it doesn’t have any other properties, a fruit doesn’t behave like a vector. You can’t do math on a fruit. And you can’t eat a vector. They are clearly two different things.

So, at a minimum, a fruit should be defined as something like:

struct Fruit
{
 Vec2 position;
};

But just as with Snake, there is the separate "render" class. And just as with Snake, this should really be folded into the real class:

struct Fruit
{
 auto render(IWindow&) -> void;
 Vec2 position;
};

As mentioned in the design section, you probably want a renderable interface, too.

At the top of Game.hpp, there is this:

#include <chrono>
#include <random>
#include <thread>
#include <memory>
#include <array>
#include "Fruit.hpp"
#include "Snake.hpp"
#include "Vec2.hpp"
#ifdef PLATFORM_WINDOWS
 #include "../render/Win32Window.hpp"
#elif defined(PLATFORM_LINUX)
 #include "../render/XWindow.hpp"
#endif

Now, most of these includes are not necessary. There is nothing from <chrono>, <random>, or <thread> in the header. Nor is there any appearance of Vec2 (though that will be naturally transitively included via Snake and Fruit anyway.

The other problem is the order of includes. Because the effect of includes can change based on their order (one of the many reasons includes are so terrible), it is good practice to order them consistently. Usually, alphabetically is the easiest way to have a consistent order.

So what you should really have is more like:

#include <array>
#include <memory>
#include "Fruit.hpp"
#include "IWindow.hpp"
#include "Snake.hpp"

The same problem exists elsewhere in the code, too. Snake.hpp has no need of <algorithm>.

bebop::snake2d::game::Game::Game(int width, int height)
 : width(width), height(height), snake({10, 10}, {1, 0}, 5)
{
#ifdef PLATFORM_WINDOWS
 window = std::make_unique<render::Win32Window>(width, height, "Snake2D");
#elif defined(PLATFORM_LINUX)
 window = std::make_unique<render::XWindow>(width, height, "Snake2D");
#endif
 int spawned = 0;
 while (spawned < FRUIT_COUNT)
 {
 fruits[spawned] = spawnFruit();
 ++spawned;
 }
}

This code seems more or less duplicated with .restart(). It seems to me that the snake and the fruits should not be created in the constructor, because they are not needed or relevant until a game actually starts.

A more logical design would probably look like:

Game::Game(int width, int height)
{
 window = make_window(width, height, "Snake2D");
}
auto Game::restart() -> void
{
 snake = Snake({5, 10}, {1, 0}, 5);
 std::ranges::generate(fruits, [this] { return spawnFruit(); });
 state = GameState::playing;
 score = 0;
}
auto Game::run() -> void
{
 while (window->isOpen())
 {
 restart();
 do
 {
 // actually play the game
 } while (/* ... */);
 }
}

Or with a proper state machine, setting up the game start would be done in the constructor of the playing state:

class playing_state : public state
{
 snake _snake;
 std::array<fruit, fruit_count> _fruits;
 int _score;
public:
 playing_state()
 {
 _snake = snake{{5, 10}, {1, 0}, 5};
 std::ranges::generate(_fruits, [this] { return spawn_fruit(); });
 _score = 0;
 }
 auto process() -> std::tuple<state_transition_type, std::unique_ptr<state>> override
 {
 // ...
 if (game_is_lost)
 return {state_transition::replace, std::make_unique<game_over_state>()};
 return {state_transition::none, nullptr};
 }
};
class game_over_state : public state
{
public:
 auto process() -> std::tuple<state_transition_type, std::unique_ptr<state>> override
 {
 // ...
 if (restart_was_chosen)
 return {state_transition::replace, std::make_unique<playing_state>()};
 if (quit_was_chosen)
 return {state_transition::end, nullptr};
 return {state_transition::none, nullptr};
 }
};
auto Game::run() -> void
{
 auto states = std::vector<std::unique_ptr<state>>{};
 states.emplace_back(std::make_unique<playing_state>());
 while (not states.empty())
 {
 auto&& current_state = *states.back();
 current_state.input();
 auto [transition, next_state] = current_state.process();
 current_state.render();
 switch (transition)
 {
 using state_transition;
 case push:
 states.emplace_back(std::move(next_state));
 break;
 case replace:
 states.back() = std::move(next_state);
 break;
 case end:
 states.pop_back();
 break;
 case none:
 break;
 }
 }
}

Or something like that, probably with a dedicated state machine class to handle the management and transition of states.

Now I mentioned the .run() function really needs to be properly refactored and cleaned up, but I didn’t mention the problems that arise from it being so hefty and disorganized.

For example, on each run through the loop, the function first checks the input, then does the processing. That sounds right, and it even matches the classic game loop structure... but the classic game loop structure assumes you have already split the actual game logic into distinct states. It doesn’t work when your game has multiple states. In your case, just consider the difference between the playing state and the paused state. Because they are not handled separately, you end up with a tricky way to cheat the game.

You see, because the input checking is done without consideration for the state, pressing one of the arrow keys changes the snake’s direction... even when the game is paused. So if I wanted to make the game really easy, I could just pause the game... then take my time and plan my next move, and press a new direction or not at my leisure... then rapidly press the pause key twice to quickly un-pause then re-pause... then once again leisurely consider my next move.

What you should have is two separate and distinct states for "playing" and "paused". When paused, you should not accept new direction input for the snake. (You might also want to not render the snake or fruits, and instead just render a "PAUSED" message.) At the very least, this would now require me to very quickly try to change the direction between the un-pause and re-pause... which is a lot harder. (But blanking the playing field while paused would make it even harder still.)

You could "fix" this by doing a state check in the key processing lambda, and only accepting a direction change command when in the playing state. But that would be a hack. Your input processing operation is already brittle (as I just demonstrated), and there may be other sneaky tricks possible... or bugs. Adding more complex logic will not make problems less likely.

There is another issue: timing. If I strip the function down, it looks like this:

void bebop::snake2d::game::Game::run()
{
 // ...
 while (window->isOpen())
 {
 // Do all processing and rendering.
 std::this_thread::sleep_for(std::chrono::milliseconds(16)); // ~60 FPS
 }
}

Now, you hope that the "do all processing and rendering" bit happens so quickly that it is functionally undetectable, so that 16 ms sleep will net you the promised 60 FPS. But... what if it doesn’t? What if it takes 10 ms to do all the simulation logic and rendering. Okay, that’s unlikely for a snake game, but maybe not!

The point is that if you really want a 60 FPS loop, you shouldn’t assume that nothing else in the loop is taking up any time. You are already getting the start time of the loop. Just use that to calculate the actual sleep time:

void bebop::snake2d::game::Game::run()
{
 // ...
 while (window->isOpen())
 {
 auto const frame_start_time = std::chrono::steady_clock::now();
 // Do all processing and rendering.
 auto const frame_end_time = std::chrono::steady_clock::now();
 auto const frame_duration = frame_end_time - frame_start_time;
 constexpr auto desired_frame_duration = std::chrono::milliseconds{1.0 / 60.0};
 if (frame_duration < desired_frame_duration)
 std::this_thread::sleep_for(desired_frame_duration - frame_duration);
 }
}

This is the "fixed time step with synchronization" style of game loop described here.

bebop::snake2d::game::Fruit bebop::snake2d::game::Game::spawnFruit()
{
 std::mt19937 rng(std::random_device{}());
 std::uniform_int_distribution<int> xdist(0, width / (CIRCLE_RADIUS * 2) - 1);
 std::uniform_int_distribution<int> ydist(0, height / (CIRCLE_RADIUS * 2) - 1);
 Fruit pos{xdist(rng), ydist(rng)};
 auto segmentsFound = std::find(snake.segments.begin(), snake.segments.end(), pos);
 auto spawnedFound = std::find(fruits.begin(), fruits.end(), pos);
 bool collides = segmentsFound != snake.segments.end() || spawnedFound != fruits.end();
 while (collides)
 {
 pos = Vec2{xdist(rng), ydist(rng)};
 auto segmentsFound = std::find(snake.segments.begin(), snake.segments.end(), pos);
 auto spawnedFound = std::find(fruits.begin(), fruits.end(), pos);
 collides = segmentsFound != snake.segments.end() || spawnedFound != fruits.end();
 }
 return pos;
}

Ah, this is really not a good idea.

You are constructing a new random number engine every single time a fruit needs to be spawned. Even worse, you are calling std::random_device every time. That is not how random number generation is supposed to work, on two counts.

First, it really defeats the purpose of a random number engine to throw it away and recreate it. You assume that each time it is recreated, it will be initialized to a distinct random state... but what if it’s not? Did you know that for a long time, the std::random_device used by MinGW on Windows was completely deterministic—it produced the same "random" numbers every time—and this was totally legal behaviour. In theory, you could get a string of "random" numbers that is just "4, 4, 4, 4, 4, 4, ...". This would mean that every time a fruit is spawned it is spawned in exactly the same place.

Retaining the random number engine would fix that problem. Even if it gets the same seed every time a new game is started (because std::random_device returns the same number over and over), at least the sequence of numbers it produces will be pseudo-random (as determined by your choice of engine).

The second reason this a bad idea is that std::random_device can be extremely slow. For starters, it might actually involved opening up /dev/random or /dev/urandom... which requires allocating and getting a whole new file descriptor, syscalls, and more. But then there might be a situation where it doesn’t have enough entropy, so it has to wait for a hardware source to produce it. In the worst case, your game could freeze for seconds... every time a fruit has to be spawned.

The correct thing to do is to initialize your random engine once... in the entire program. Not once per fruit spawning, once per frame, or even once per game. The ideal place to do it is probably in the Game constructor.

Recreating the distributions every time you need them is less of a problem. So you can just make those right in .spawnFruit().

Do note, however, that there is a small but non-zero possibility that you may get a string of "bad" numbers, where for thousands or millions of loops, it keeps spawning a fruit in an unacceptable location. You might want to limit the number of attempts to spawn a fruit, and give up if you can’t get a good one after some time.

(Alternatively, there is a trick that can avoid repeatedly regenerating random values altogether. The idea is that you don’t even generate "bad" values at all. Let’s say that the x-coordinate can be between 0 and 9, inclusive—so 10 possible x-coordinates: "0, 1, 2, 3, 4, 5, 6, 7, 8, 9"—but there are three fruits, at x-coordinates 3, 6, and 8. Instead of generating one of 10 possible values, and regenerating every time you hit a bad one, you only generate one of (10 − the number of fruits (3)) = 7 possible values—so "0, 1, 2, 3, 4, 5, 6"—then you logically adjust the values to skip over the "bad" ones, so: "0, 1, 2, 4, 5, 7, 9". So now you just need to randomly pick one of those, and it will be valid. No repeated random number generation required. Basically, just generate a number in the desired range minus the number of bad values, then while the generated value is bad, increment it. You will get a good value with a single call to the random number generator.)

struct IWindow
{

You have a rather dangerous bug in your code.

Any time a class is intended to be a base class used for runtime polymorphism, it must have a virtual destructor. IWindow is meant to be an abstract base class for a polymorphic window type, and thus it requires a virtual destructor.

Because you do not have one, what is happening is that when the window is being destroyed in Game’s (implicit) destructor, the (implicit) destructor of IWindow is being called... but not the destructor of the actual window. So, in the case of XWindow, none of the functions XDestroyImage(), XFreePixmap(), or XFreeGC() are being called. Put a print statement or breakpoint in the window destructor and see for yourself.

The fix is easy, just add a virtual destructor:

struct IWindow
{
 virtual ~IWindow() = default;

I think that’s enough for a review. Hope it helps, and happy coding!

• Davislor: Really nice. Nitpick to your nitpick: since window is actually a singleton, it doesn’t need to be constructed on the heap. It could be a reference to a static singleton. The only complication is that its destructor is never called, so if there is any platform-specific clean-up when you quit to the main menu, it needs to be called manually.
• indi: I don’t think that’s a good idea at all. Singleton is an anti-pattern for very good reasons; it is not only functionally untestable, it is inflexible. What if you want two windows? Some games/platforms could use that. Or even if you want just one window, you may need to destroy it and recreate it after some configuration changes. If you are really concerned about dynamic allocation (though I can’t see why in this case), it is not hard to use the same factory pattern without any dynamic allocation.
• Davislor: How would you recommend using the same factory pattern without dynamic allocation?
• Davislor: Thinking about it some more, a better approach might be just to use composition instead of inheritance. No version of the program will actually need to use both the Windows and X API implementations, so OP could eliminate the base class, duck-type X11Window and Win32Window, and just have a Game contain one or the other. If you still want to avoid including any OS-specific headers, it might become a Game<WindowT>. Otherwise, using window_type = X11Window;.
• indi: Avoiding dynamic allocation isn’t hard. To start, add a function pointer deleter to the unique pointer (a good idea to do anyway; no reason to assume an OS-specific module will want to use new to allocate the window object, and you make possible moving the window/graphics stuff to a DLL). Now, in the OS-specific module, have a static optional of the concrete window type. In make_window(), just engage the optional, and return a pointer to the concrete window in it, with a pointer to a deleter function that disengages the optional. Add synchronization guards as necessary. Easy peasy.

Answer 2

Use more libraries

While there is some value in creating everything "from scratch", it's not very productive. For example, you have created a Vec2, but it only has operator==() and operator+=(). What if you need other operations? Are you going to implement a complete 2D vector class yourself?

Similarly, it looks like you have written code to create X11 or Win32 windows from scratch. Do you really want to maintain all that code? What if you want to run your game on macOS? What if you want to support Wayland?

Your end goal is to write a Snake game, not to write a vector math library or a graphics library. So I recommend you find some libraries to handle the latter details, freeing you up to focus on the game itself.

For vector math in games, I recommend the GLM library. For graphics, I would use SDL to handle windows and input events, and perhaps SDL_gfx on top for rendering shapes.

Loops

I see a lot of old-style for loops, while loops, and some std::for_each() calls which look a bit more complex than necessary. I recommend that you first try to use a range-based for loop, followed by an STL algorithm if that makes more sense, before resorting to old-style for and while loops. For example,

int spawned = 0;
while (spawned < FRUIT_COUNT)
{
 fruits[spawned] = spawnFruit();
 ++spawned;
}

Could be replaced with:

for (auto& fruit: fruits)
{
 fruit = spawnFruit();
}

This is simple, clear, and less error-prone: you don't have to worry if fruits actually has size FRUIT_COUNT, and you can't accidentily write spawned <= FRUIT_COUNT and get an out-of-bounds error.

An algorithm would work here as well:

std::generate(fruits.begin(), fruits.end(), spawnFruit); // or:
std::ranges::generate(fruits, spawnFruit); // since C++20

Note that while algorithms are sometimes more compact, not everyone knows all STL algorithms by head, and a for loop might be easier to understand.

• Hrant Nurijanyan: Thanks for the suggestions, honestly I have not touched C++ 20 standard yet, so I don't know its new functionality yet. Yes, you are right, using libraries will always be helpful to do the actual thing you wanna do, in this case I wanted to create the Snake game. But that's not my case. I want to learn every aspect of game programming in depth, started from rendering and ending with ECS. Ofc you can always use the popular game engines or the home-baked ones to achieve your goal, but I want to learn and have at least basic understanding in everything. Thanks for the review!