C++ Implementation of Conway's Game of Life with SDL2

Question

This is my first attempt at using a visual library in C++, so for practice I decided to create the Game of Life utilizing SDL2; however, I would like help in terms of optimization and OOP.

When running the program at the current config with a 200x200 vector, the screen updates with a new image about twice a second, but I think that there is definitely room for improvement in my code.

To explain the program, the initialization of the CellVector class creates a 2D-vector of boolean values that contains whether or not a cell on the grid is alive. Then, cellVector.generateSeed(RANDOM) randomly assigns alive or dead values to all booleans in the array (I have an enum of patterns in CellVector.h such as LOAF or GLIDER which RANDOM is a part of). The SDL window is created and opened, and the program enters the while loop.

I've included the main.cpp file and the CellVector::tick() and Window::render2DBoolVector() functions below.

main.cpp:

#include <string>
#include "CellVector.h"
#include "Window.h"
int main(int argc, char* args[])
{
 // Configuration Settings //
 int worldWidth = 200;
 int worldHeight = 200;
 // Window size as a percentage of the world size. EX: 50x50 world with 2.0 scale = 100x100 window
 double windowScale = 5; 
 // Percentage chance that a specific cell will be alive at world generation if RANDOM is the arg in CellVector.generateseed()
 double isAliveChance = 0.5; 
 // Name of the program window
 const char* gameTitle = "Conway's Game of Life"; 
 // Initialize cell vector
 CellVector cellVector(worldHeight, worldWidth, isAliveChance);
 // Generate seed for cell vector from given seed type
 cellVector.generateSeed(RANDOM);
 // Initialize the window class
 Window window(worldWidth, worldHeight, windowScale, "Conway's Game of Life");
 // Initialize SDL within the window class
 bool running = window.initSDL();
 while (running)
 {
 // Check if the X button on the window was clicked, stop looping if so
 running = (window.wasEventTriggered(SDL_QUIT)) ? false : true;
 // Render the cell vector on the SDL window
 window.render2DBoolVector(cellVector.getCellVector());
 // Calculate one generation of The Game of Life
 cellVector.tick();
 // Crude delay system for the moment
 SDL_Delay(static_cast<Uint32>(1 / 60 * 1e4));
 }
 // Deallocate and destroy all SDL variables and windows
 window.closeSDL();
 return 0;
}

CellVector::tick():

void CellVector::tick()
{
 std::vector<std::vector<bool>> vectorCopy = cellVector;
 for (int x = 0; x < height; x++)
 {
 for (int y = 0; y < width; y++)
 {
 int amountOfNeighbors = getAmountOfNeighbors(x, y);
 // Alive and has 2 neighbors - Cell Lives
 if (cellVector[x][y] && amountOfNeighbors == 2)
 {
 vectorCopy[x][y] = 1;
 }
 // Has 3 neighbors - Cell Lives/Is Born
 else if (amountOfNeighbors == 3)
 {
 vectorCopy[x][y] = 1;
 }
 // Neither previous conditions satisfied - Cell Dies
 else
 {
 vectorCopy[x][y] = 0;
 }
 }
 }
 cellVector.swap(vectorCopy);
}

Window::render2DBoolVector():

void Window::render2DBoolVector(std::vector<std::vector<bool>> boolVector)
{
 std::cout << "Rendering cell vector...\n";
 SDL_SetRenderDrawColor(renderer, 255, 255, 255, 255);
 SDL_RenderClear(renderer);
 int vectorWidth = boolVector.size();
 int vectorHeight = boolVector[0].size();
 for (int x = 0; x < vectorWidth; x++)
 {
 for (int y = 0; y < vectorHeight; y++)
 {
 // Renderer draws in black if the cell is alive, white if it is dead
 (boolVector[x][y]) ? SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255) 
 : SDL_SetRenderDrawColor(renderer, 255, 255, 255, 255);
 SDL_Rect cell;
 cell.x = static_cast<int>(ceil(x * (windowWidth / vectorWidth)));
 cell.w = static_cast<int>(ceil(windowWidth / vectorWidth));
 cell.y = static_cast<int>(ceil(y * (windowHeight / vectorHeight)));
 cell.h = static_cast<int>(ceil(windowHeight / vectorHeight));
 SDL_RenderFillRect(renderer, &cell);
 }
 }
 SDL_RenderPresent(renderer);
 std::cout << "Cell vector rendered...\n";
}

Answer 1

Here are some things that may help you improve your program.

Use the required #includes

The main routine calls SDL_Delay but doesn't have this line:

#include <SDL2/SDL.h>

It probably links and compiles anyway because that's inside Window.h, but it's best if any library calls that are used directly are represented in the various #includes. The reason for this is that if, for example, you completely refactored Window to use something other than SDL, the code would still compile. (There would be other problems, of course, but it's a good general guideline to follow anyway.)

Use only required #includes

The inverse of the suggestion above is to only include files that are actually needed. In this case, <string> is not used and not needed.

Eliminate unused variables

This code declares and initializes a variable gameTitle but then does nothing with it. Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so. What's probable is that you meant instead to pass it to the Window constructor.

Avoid leaking details

It's fine that you've chosen to use SDL to create this program, but does that fact need to be known outside of the Window class? Probably not. For that reason, I'd suggest avoiding SDL-specific details in the names and usage of the class and instead make the names descriptive of what they do rather than how they're implemented. Example: Window::initSDL() --> Window::init() or better, see the next suggestion.

Make use of constructor and destructor

Rather than forcing the user of the Window class to explicitly call init before first use and close after last use, why not just put those into the constructor and destructor? That way a Window instance is usable as soon as it is constructed, which is exactly what C++ constructors are supposed to do. If init fails, it could cause the constructor to throw an exception.

Be consistent

The cellVector class is initialized with height and width, in that order, but the Window class uses the reverse order. Consistency would help users of the code avoid subtle errors. And with that said, read the next suggestion.

Make it impossible to have inconsistent objects

What would happen if we had different height and width for the Window versus the CellVector? Probably nothing good, so for that reason, why not make it impossible to have inconsistent objects? I like the way you have separated the game state from the rendering of it, but it would be convenient for the user if both of those objects were hidden inside, perhaps a Game or Life object. It would then be that object's job to keep the two internal objects consistent.

Avoid making copies

Right now, the render2DBoolVector routine takes a std::vector<std::vector<bool>> as an argument. Unfortunately, this is a pass by value, and so the compiler may have to copy the entire structure. Better would be to pass by const reference:

void render2DBoolVector(const std::vector<std::vector<bool>> &boolVector);

Make full use of library code

The Window::render2DBoolVector() is much longer and more cmoplex than it needs to be. Instead, you could use SDL_RenderSetScale(renderer, scale, scale); within the Window::initSDL() and rely on SDL's scaling rather than creating your own. This makes the Window::render2DBoolVector() code much simpler:

void Window::render2DBoolVector(const std::vector<std::vector<bool>> &boolVector)
{
 SDL_SetRenderDrawColor(renderer, 255, 255, 255, 255);
 SDL_RenderClear(renderer);
 const int vectorWidth = boolVector.size();
 const int vectorHeight = boolVector[0].size();
 for (int x = 0; x < vectorWidth; x++)
 {
 for (int y = 0; y < vectorHeight; y++)
 {
 // Renderer draws in black if the cell is alive, white if it is dead
 (boolVector[x][y]) ? SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255) 
 : SDL_SetRenderDrawColor(renderer, 255, 255, 255, 255);
 SDL_Rect cell{x,y,1,1};
 SDL_RenderFillRect(renderer, &cell);
 }
 }
 SDL_RenderPresent(renderer);
}

Rethink the interface

From the comments, I infer that it's likely that isAliveChance has no meaning unless the generateSeed function is called with RANDOM. That should be a clue that isAliveChance should probably be a parameter of a polymorphic generateSeed function rather than part of the constructor parameter list.

Avoid unecessary work

In the render2DBoolVector class, the entire screen is cleared and then each bit painted. This is effectively setting every pixel twice, which really isn't needed. Instead, clear everything and then just write the live cells:

void Window::render2DBoolVector(const std::vector<std::vector<bool>> &boolVector)
{
 SDL_SetRenderDrawColor(renderer, 255, 255, 255, 255);
 SDL_RenderClear(renderer);
 const int vectorWidth = boolVector.size();
 const int vectorHeight = boolVector[0].size();
 SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255);
 for (int x = 0; x < vectorWidth; x++)
 {
 for (int y = 0; y < vectorHeight; y++)
 {
 if (boolVector[x][y]) {
 SDL_Rect cell{x,y,1,1};
 SDL_RenderFillRect(renderer, &cell);
 }
 }
 }
 SDL_RenderPresent(renderer);
}

Fix the bug

There appears to be a bug in the code in that CellVector::tick has x going from 0 to height but elsewhere x is the width. This bug shows up when the window is not square. I think that x should be a width in all cases to be consistent.

• Colin: Thank you for the answer. You are correct about isAliveChance not having any meaning unless RANDOM is passed through generateSeed. I've implemented suggestions from both you and janos, including a Game class containing both the CellVector and Window classes and an interface for interacting with them. I appreciate the help from both of you and can add the revised code to the question.
• Edward: Please do not add, remove, or edit code in a question after you've received an answer. The site policy is explained in What to do when someone answers. However, if you'd like, you can post the revised code as a new question.

Answer 2

You can use boolean expressions directly. For example instead of this:

running = (window.wasEventTriggered(SDL_QUIT)) ? false : true;

You can write simply:

running = !window.wasEventTriggered(SDL_QUIT);

Similarly, instead of this:

// Alive and has 2 neighbors - Cell Lives
if (cellVector[x][y] && amountOfNeighbors == 2)
{
 vectorCopy[x][y] = 1;
}
// Has 3 neighbors - Cell Lives/Is Born
else if (amountOfNeighbors == 3)
{
 vectorCopy[x][y] = 1;
}
// Neither previous conditions satisfied - Cell Dies
else
{
 vectorCopy[x][y] = 0;
}

You can write:

vectorCopy[x][y] = cellVector[x][y] && amountOfNeighbors == 2
 || amountOfNeighbors == 3;

Many of the comments don't explain anything the code doesn't already say. I suggest to remove them, for example:

// Name of the program window
const char* gameTitle = "Conway's Game of Life"; 
// Initialize cell vector
CellVector cellVector(worldHeight, worldWidth, isAliveChance);
// Initialize the window class
Window window(worldWidth, worldHeight, windowScale, "Conway's Game of Life");
// Initialize SDL within the window class
bool running = window.initSDL();