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.

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