On the other hand, we're using std::uint8_t but failing to include <cstdint> to declare it. Although it might be brought in by one of the other headers on a particular platform, we shouldn't depend on that if we want to be portable.
These are all easily fixed. For example, we can avoid the warning about an uninitialized member by providing a default initializer (though I'd be happier if my compiler were smart enough to know which types get constructed in a genuinely uninitialized state, and warn only about those):
std::vector<sf::Vertex> m_vertexPoints = {};
The const& qualifier on the return type lets client code view the contents of our vector without being able to modify it and without needing to make a copy.
Here, I've used emplace_back to reduce the likelihood of copying (that said, push_back() is overloaded to move-from an rvalue argument, so there's likely no real difference in the optimized binary). That takes us neatly to the next member, which can similarly be reduced:
The pragma omp critical is required when combining results in order to ensure that the threads don't try to modify the shared aliveCells simultaneously. The other shared variables are read but not modified within the parallel section.
Minor/style issues
There's no need to explicitly return 0 from main() if we always succeed - a common convention is to do so only when there's another code path that returns non-zero.
#include "WorldRenderer.h"
#include <omp.h>
void WorldRenderer::addQuad(const Cell& cell)
{
sf::Vector2f topLeft{cell.position.x * 1.0f, cell.position.y * 1.0f};
sf::Vector2f bottomRight{topLeft.x + 1, topLeft.y + 1};
addQuad(topLeft, bottomRight, cell.color);
}
void WorldRenderer::addQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
void WorldRenderer::render(sf::RenderWindow & window, GameOfLife & game)
{
// clear m_cellVertexPoints
m_vertexPoints.clear();
// draw backgrounds for "core zones"
renderBackgrounds(game);
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell);
}
// draw quads to window
window.draw(m_vertexPoints.data(), m_vertexPoints.size(), sf::Quads);
}
void WorldRenderer::renderBackgrounds(GameOfLife & world)
{
auto const maxThreads = omp_get_max_threads();
auto const threadHeight = world.worldSize.y / maxThreads;
sf::Vector2f topLeft{0, 0};
sf::Vector2f bottomRight{1.f * world.worldSize.x + 1, 1.f * world.worldSize.y / maxThreads + 1};
for (int i = 0; i < maxThreads; ++i) {
sf::Vector2f topLeft{0, 1.f * i * threadHeight};
sf::Vector2f bottomRight{1.f * world.worldSize.x + 1, topLeft.y + 1.f * world.worldSize.y / maxThreads + 1};
addQuad(topLeft, bottomRight, darkenColor(world.getThreadColor(i)));
}
}
sf::Color WorldRenderer::darkenColor(sf::Color input)
{
return sf::Color(input.r / 4, input.g / 4, input.b / 4);
}
# CMakeList.txt : CMake project for GameOfLife
#
cmake_minimum_required (VERSION 3.8)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3 -Werror -Wall -Wextra -Wwrite-strings -Wno-parentheses -Weffc++")
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
# Give the project a name
project(GameOfLife)
find_package(OpenMP)
find_package(SFML 2.5 COMPONENTS graphics REQUIRED)
set(SOURCES
src/Main.cpp
src/GameOfLife.cpp
src/GameOfLife.h
src/WorldRenderer.cpp
src/WorldRenderer.h
src/Cell.h
)
add_executable(GameOfLife ${SOURCES})
target_link_libraries(GameOfLife sfml-graphics OpenMP::OpenMP_CXX)
# CMakeList.txt : CMake project for GameOfLife
#
cmake_minimum_required (VERSION 3.8)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3 -Werror -Wall -Wextra -Wshadow -Wwrite-strings -Wno-parentheses -Weffc++")
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
# Give the project a name
project(GameOfLife)
find_package(OpenMP)
find_package(SFML 2.5 COMPONENTS graphics REQUIRED)
set(SOURCES
src/Main.cpp
src/GameOfLife.cpp
src/GameOfLife.h
src/WorldRenderer.cpp
src/WorldRenderer.h
src/Cell.h
)
add_executable(GameOfLife ${SOURCES})
target_link_libraries(GameOfLife sfml-graphics OpenMP::OpenMP_CXX)
Some minor changes I introduced while refactoring:
- I changed the loop with the
neighboursarray to avoid the branch for the {0,0} case and to make it a single loop rather than nested loops. I think this is clearer, and it might be a tiny bit faster (but I didn't profile that). - I tend to prefer snake_case for identifiers, and that has crept into the code where I should have been consistent with the original camelCase - sorry about that! The same goes for spacing around operators and the
&that indicates a reference variable. Being consistent is more important than any particular style, and I broke that rule because I was rushing.
On the other hand, we're using std::uint8_t but failing to include <cstdint> to declare it.
These are all easily fixed.
Here, I've used emplace_back to reduce the likelihood of copying. That takes us neatly to the next member, which can similarly be reduced:
#include "WorldRenderer.h"
#include <omp.h>
void WorldRenderer::addQuad(const Cell& cell)
{
sf::Vector2f topLeft{cell.position.x * 1.0f, cell.position.y * 1.0f};
sf::Vector2f bottomRight{topLeft.x + 1, topLeft.y + 1};
addQuad(topLeft, bottomRight, cell.color);
}
void WorldRenderer::addQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
void WorldRenderer::render(sf::RenderWindow & window, GameOfLife & game)
{
// clear m_cellVertexPoints
m_vertexPoints.clear();
// draw backgrounds for "core zones"
renderBackgrounds(game);
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell);
}
// draw quads to window
window.draw(m_vertexPoints.data(), m_vertexPoints.size(), sf::Quads);
}
void WorldRenderer::renderBackgrounds(GameOfLife & world)
{
auto const maxThreads = omp_get_max_threads();
auto const threadHeight = world.worldSize.y / maxThreads;
sf::Vector2f topLeft{0, 0};
sf::Vector2f bottomRight{1.f * world.worldSize.x + 1, 1.f * world.worldSize.y / maxThreads + 1};
for (int i = 0; i < maxThreads; ++i) {
sf::Vector2f topLeft{0, 1.f * i * threadHeight};
sf::Vector2f bottomRight{1.f * world.worldSize.x + 1, topLeft.y + 1.f * world.worldSize.y / maxThreads + 1};
addQuad(topLeft, bottomRight, darkenColor(world.getThreadColor(i)));
}
}
sf::Color WorldRenderer::darkenColor(sf::Color input)
{
return sf::Color(input.r / 4, input.g / 4, input.b / 4);
}
# CMakeList.txt : CMake project for GameOfLife
#
cmake_minimum_required (VERSION 3.8)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3 -Werror -Wall -Wextra -Wwrite-strings -Wno-parentheses -Weffc++")
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
# Give the project a name
project(GameOfLife)
find_package(OpenMP)
find_package(SFML 2.5 COMPONENTS graphics REQUIRED)
set(SOURCES
src/Main.cpp
src/GameOfLife.cpp
src/GameOfLife.h
src/WorldRenderer.cpp
src/WorldRenderer.h
src/Cell.h
)
add_executable(GameOfLife ${SOURCES})
target_link_libraries(GameOfLife sfml-graphics OpenMP::OpenMP_CXX)
On the other hand, we're using std::uint8_t but failing to include <cstdint> to declare it. Although it might be brought in by one of the other headers on a particular platform, we shouldn't depend on that if we want to be portable.
These are all easily fixed. For example, we can avoid the warning about an uninitialized member by providing a default initializer (though I'd be happier if my compiler were smart enough to know which types get constructed in a genuinely uninitialized state, and warn only about those):
std::vector<sf::Vertex> m_vertexPoints = {};
The const& qualifier on the return type lets client code view the contents of our vector without being able to modify it and without needing to make a copy.
Here, I've used emplace_back to reduce the likelihood of copying (that said, push_back() is overloaded to move-from an rvalue argument, so there's likely no real difference in the optimized binary). That takes us neatly to the next member, which can similarly be reduced:
The pragma omp critical is required when combining results in order to ensure that the threads don't try to modify the shared aliveCells simultaneously. The other shared variables are read but not modified within the parallel section.
Minor/style issues
There's no need to explicitly return 0 from main() if we always succeed - a common convention is to do so only when there's another code path that returns non-zero.
#include "WorldRenderer.h"
#include <omp.h>
void WorldRenderer::addQuad(const Cell& cell)
{
sf::Vector2f topLeft{cell.position.x * 1.0f, cell.position.y * 1.0f};
sf::Vector2f bottomRight{topLeft.x + 1, topLeft.y + 1};
addQuad(topLeft, bottomRight, cell.color);
}
void WorldRenderer::addQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
void WorldRenderer::render(sf::RenderWindow & window, GameOfLife & game)
{
// clear m_cellVertexPoints
m_vertexPoints.clear();
// draw backgrounds for "core zones"
renderBackgrounds(game);
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell);
}
// draw quads to window
window.draw(m_vertexPoints.data(), m_vertexPoints.size(), sf::Quads);
}
void WorldRenderer::renderBackgrounds(GameOfLife & world)
{
auto const maxThreads = omp_get_max_threads();
auto const threadHeight = world.worldSize.y / maxThreads;
for (int i = 0; i < maxThreads; ++i) {
sf::Vector2f topLeft{0, 1.f * i * threadHeight};
sf::Vector2f bottomRight{1.f * world.worldSize.x + 1, topLeft.y + 1.f * world.worldSize.y / maxThreads + 1};
addQuad(topLeft, bottomRight, darkenColor(world.getThreadColor(i)));
}
}
sf::Color WorldRenderer::darkenColor(sf::Color input)
{
return sf::Color(input.r / 4, input.g / 4, input.b / 4);
}
# CMakeList.txt : CMake project for GameOfLife
#
cmake_minimum_required (VERSION 3.8)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3 -Werror -Wall -Wextra -Wshadow -Wwrite-strings -Wno-parentheses -Weffc++")
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
# Give the project a name
project(GameOfLife)
find_package(OpenMP)
find_package(SFML 2.5 COMPONENTS graphics REQUIRED)
set(SOURCES
src/Main.cpp
src/GameOfLife.cpp
src/GameOfLife.h
src/WorldRenderer.cpp
src/WorldRenderer.h
src/Cell.h
)
add_executable(GameOfLife ${SOURCES})
target_link_libraries(GameOfLife sfml-graphics OpenMP::OpenMP_CXX)
Some minor changes I introduced while refactoring:
- I changed the loop with the
neighboursarray to avoid the branch for the {0,0} case and to make it a single loop rather than nested loops. I think this is clearer, and it might be a tiny bit faster (but I didn't profile that). - I tend to prefer snake_case for identifiers, and that has crept into the code where I should have been consistent with the original camelCase - sorry about that! The same goes for spacing around operators and the
&that indicates a reference variable. Being consistent is more important than any particular style, and I broke that rule because I was rushing.
#pragma once
#include <SFML/Graphics.hpp>
#include <vector>
#include "GameOfLife.h"
class WorldRenderer
{
public:
WorldRenderer() = default;
// Renders the given game to the given window.
void render(sf::RenderWindow& window, GameOfLife& world);
private:
// Vertex points for the pending draw call.
std::vector<sf::Vertex> m_vertexPoints = {};
// Adds a cell-sized quad in the "grid position" specified.
void addQuad(const Cell& cell);
// Adds a darker colored quad in the given coordinates.
void addBackgroundQuadaddQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color);
// Renders the background colors which correspond to the thread ID and the cells they are updating.
void renderBackgrounds(GameOfLife& world);
// Returns a darker variant of the given color.
sf::Color darkenColor(sf::Color input);
};
#include "WorldRenderer.h"
#include <omp.h>
void WorldRenderer::addQuad(const Cell& cell)
{
float gridXFloat =sf::Vector2f topLeft{cell.position.x * 1.0f;
float gridYFloat =0f, cell.position.y * 1.0f;
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat 0f}, cell.color); // top-left
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat + 1}, cell.color); // bottom-left
m_vertexPoints.emplace_back(sf::Vector2fbottomRight{gridXFloattopLeft.x + 1, gridYFloattopLeft.y + 1}, cell.color); // bottom-right
m_vertexPoints.emplace_backaddQuad(sf::Vector2f{gridXFloat + 1topLeft, gridYFloat }bottomRight, cell.color); // top-right
}
void WorldRenderer::addBackgroundQuadaddQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
void WorldRenderer::render(sf::RenderWindow & window, GameOfLife & game)
{
// clear m_cellVertexPoints
m_vertexPoints.clear();
// draw backgrounds for "core zones"
renderBackgrounds(game);
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell);
}
// draw quads to window
window.draw(m_vertexPoints.data(), m_vertexPoints.size(), sf::Quads);
}
void WorldRenderer::renderBackgrounds(GameOfLife & world)
{
auto const maxThreads = omp_get_max_threads();
int cellsPerCoreauto =const world.worldSize.xthreadHeight *= world.worldSize.y / maxThreads;
// first draw the background color of the final core index
addBackgroundQuad(
sf::Vector2f( topLeft{0, 0),};
sf::Vector2f( bottomRight{1.f * world.worldSize.x + 1, 1.f * world.worldSize.y),
/ darkenColor(world.getThreadColor(maxThreads -+ 1))
)};
// draw the remaining core background colors on top, in reverse order
for (int i = maxThreads -0; 2; i >=< 0;maxThreads; i-- ++i) {
auto endsf::Vector2f =topLeft{0, world1.get2D(cellsPerCoref * (i +* 1))threadHeight};
addBackgroundQuad(
sf::Vector2f(0, 0),
bottomRight{1.f * sf::Vector2f(world.worldSize.x + 1, endtopLeft.y),
+ 1.f * world.worldSize.y / maxThreads + 1};
addQuad(topLeft, bottomRight, darkenColor(world.getThreadColor(i)));
}
}
sf::Color WorldRenderer::darkenColor(sf::Color input)
{
return sf::Color(input.r / 4, input.g / 4, input.b / 4);
}
#pragma once
#include <SFML/Graphics.hpp>
#include <vector>
#include "GameOfLife.h"
class WorldRenderer
{
public:
WorldRenderer() = default;
// Renders the given game to the given window.
void render(sf::RenderWindow& window, GameOfLife& world);
private:
// Vertex points for the pending draw call.
std::vector<sf::Vertex> m_vertexPoints = {};
// Adds a cell-sized quad in the "grid position" specified.
void addQuad(const Cell& cell);
// Adds a darker colored quad in the given coordinates.
void addBackgroundQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color);
// Renders the background colors which correspond to the thread ID and the cells they are updating.
void renderBackgrounds(GameOfLife& world);
// Returns a darker variant of the given color.
sf::Color darkenColor(sf::Color input);
};
#include "WorldRenderer.h"
#include <omp.h>
void WorldRenderer::addQuad(const Cell& cell)
{
float gridXFloat = cell.position.x * 1.0f;
float gridYFloat = cell.position.y * 1.0f;
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat }, cell.color); // top-left
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat + 1}, cell.color); // bottom-left
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat + 1, gridYFloat + 1}, cell.color); // bottom-right
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat + 1, gridYFloat }, cell.color); // top-right
}
void WorldRenderer::addBackgroundQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
void WorldRenderer::render(sf::RenderWindow & window, GameOfLife & game)
{
// clear m_cellVertexPoints
m_vertexPoints.clear();
// draw backgrounds for "core zones"
renderBackgrounds(game);
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell);
}
// draw quads to window
window.draw(m_vertexPoints.data(), m_vertexPoints.size(), sf::Quads);
}
void WorldRenderer::renderBackgrounds(GameOfLife & world)
{
auto const maxThreads = omp_get_max_threads();
int cellsPerCore = world.worldSize.x * world.worldSize.y / maxThreads;
// first draw the background color of the final core index
addBackgroundQuad(
sf::Vector2f(0, 0),
sf::Vector2f(world.worldSize.x, world.worldSize.y),
darkenColor(world.getThreadColor(maxThreads - 1))
);
// draw the remaining core background colors on top, in reverse order
for (int i = maxThreads - 2; i >= 0; i--) {
auto end = world.get2D(cellsPerCore * (i + 1));
addBackgroundQuad(
sf::Vector2f(0, 0),
sf::Vector2f(world.worldSize.x, end.y),
darkenColor(world.getThreadColor(i)));
}
}
sf::Color WorldRenderer::darkenColor(sf::Color input)
{
return sf::Color(input.r / 4, input.g / 4, input.b / 4);
}
#pragma once
#include <SFML/Graphics.hpp>
#include <vector>
#include "GameOfLife.h"
class WorldRenderer
{
public:
WorldRenderer() = default;
// Renders the given game to the given window.
void render(sf::RenderWindow& window, GameOfLife& world);
private:
// Vertex points for the pending draw call.
std::vector<sf::Vertex> m_vertexPoints = {};
// Adds a cell-sized quad in the "grid position" specified.
void addQuad(const Cell& cell);
// Adds a darker colored quad in the given coordinates.
void addQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color);
// Renders the background colors which correspond to the thread ID and the cells they are updating.
void renderBackgrounds(GameOfLife& world);
// Returns a darker variant of the given color.
sf::Color darkenColor(sf::Color input);
};
#include "WorldRenderer.h"
#include <omp.h>
void WorldRenderer::addQuad(const Cell& cell)
{
sf::Vector2f topLeft{cell.position.x * 1.0f, cell.position.y * 1.0f};
sf::Vector2f bottomRight{topLeft.x + 1, topLeft.y + 1};
addQuad(topLeft, bottomRight, cell.color);
}
void WorldRenderer::addQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
void WorldRenderer::render(sf::RenderWindow & window, GameOfLife & game)
{
// clear m_cellVertexPoints
m_vertexPoints.clear();
// draw backgrounds for "core zones"
renderBackgrounds(game);
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell);
}
// draw quads to window
window.draw(m_vertexPoints.data(), m_vertexPoints.size(), sf::Quads);
}
void WorldRenderer::renderBackgrounds(GameOfLife & world)
{
auto const maxThreads = omp_get_max_threads();
auto const threadHeight = world.worldSize.y / maxThreads;
sf::Vector2f topLeft{0, 0};
sf::Vector2f bottomRight{1.f * world.worldSize.x + 1, 1.f * world.worldSize.y / maxThreads + 1};
for (int i = 0; i < maxThreads; ++i) {
sf::Vector2f topLeft{0, 1.f * i * threadHeight};
sf::Vector2f bottomRight{1.f * world.worldSize.x + 1, topLeft.y + 1.f * world.worldSize.y / maxThreads + 1};
addQuad(topLeft, bottomRight, darkenColor(world.getThreadColor(i)));
}
}
sf::Color WorldRenderer::darkenColor(sf::Color input)
{
return sf::Color(input.r / 4, input.g / 4, input.b / 4);
}
Avoid importing the whole of std namespace
Bringing all names in from a namespace is problematic; namespace std particularly so. See Why is "using namespace std" considered bad practice?.
Include the right headers
Main.cpp includes <iostream>, but appears not to use anything declared there.
The same is true of GameOfLife.cpp.
On the other hand, we're using std::uint8_t but failing to include <cstdint> to declare it.
Naming conventions
We normally reserve all-uppercase names for preprocessor macros, to mark them as dangerous in code. Using such names for plain constants subverts that convention, misleading the reader.
Fix compilation errors
Remove the extra qualification GameOfLife:: on member doUpdate.
GameOfLife::getThreadColor() fails to return a value when no switch cases match. Although we readers can tell that a case must always match, we should add a return statement after the switch to keep the compiler from reporting the error.
Enable and fix compilation warnings
You seem to be compiling with all warnings disabled. With g++ -Wall -Wextra -Weffc++, we get a few extra things to fix:
In file included from /home/tms/stackexchange/review/213814/GameOfLife/src/GameOfLife.cpp:1:
/home/tms/stackexchange/review/213814/GameOfLife/src/GameOfLife.h: In constructor ‘GameOfLife::GameOfLife(sf::Vector2i)’:
/home/tms/stackexchange/review/213814/GameOfLife/src/GameOfLife.h:46:23: warning: ‘GameOfLife::worldBuffer’ will be initialized after [-Wreorder]
std::vector<uint8_t> worldBuffer;
^~~~~~~~~~~
/home/tms/stackexchange/review/213814/GameOfLife/src/GameOfLife.h:33:12: warning: ‘const int GameOfLife::maxThreads’ [-Wreorder]
const int maxThreads;
^~~~~~~~~~
/home/tms/stackexchange/review/213814/GameOfLife/src/GameOfLife.cpp:11:1: warning: when initialized here [-Wreorder]
GameOfLife::GameOfLife(sf::Vector2i size) : worldSize(size), world(size.x * size.y, false), worldBuffer(world), maxThreads(std::thread::hardware_concurrency())
^~~~~~~~~~
/home/tms/stackexchange/review/213814/GameOfLife/src/GameOfLife.cpp:11:1: warning: ‘GameOfLife::aliveCells’ should be initialized in the member initialization list [-Weffc++]
[ 60%] Building CXX object CMakeFiles/GameOfLife.dir/src/WorldRenderer.cpp.o
/usr/bin/c++ -Wall -Wextra -Wwrite-strings -Wno-parentheses -Weffc++ -pthread -std=c++17 -o CMakeFiles/GameOfLife.dir/src/WorldRenderer.cpp.o -c /home/tms/stackexchange/review/213814/GameOfLife/src/WorldRenderer.cpp
/home/tms/stackexchange/review/213814/GameOfLife/src/WorldRenderer.cpp: In constructor ‘WorldRenderer::WorldRenderer()’:
/home/tms/stackexchange/review/213814/GameOfLife/src/WorldRenderer.cpp:3:1: warning: ‘WorldRenderer::m_vertexPoints’ should be initialized in the member initialization list [-Weffc++]
WorldRenderer::WorldRenderer()
^~~~~~~~~~~~~
/home/tms/stackexchange/review/213814/GameOfLife/src/WorldRenderer.cpp: In member function ‘void WorldRenderer::renderBackgrounds(sf::RenderWindow&, GameOfLife&)’:
/home/tms/stackexchange/review/213814/GameOfLife/src/WorldRenderer.cpp:79:58: warning: unused parameter ‘window’ [-Wunused-parameter]
void WorldRenderer::renderBackgrounds(sf::RenderWindow & window, GameOfLife & world)
~~~~~~~~~~~~~~~~~~~^~~~~~
These are all easily fixed.
We also want to turn on some compiler optimizations here; I'll use -O3. After all, there's little point conducting a performance review on unoptimized code.
Don't declare empty constructors and destructors
public: WorldRenderer(); ~WorldRenderer(); WorldRenderer::WorldRenderer() { } WorldRenderer::~WorldRenderer() { }
Let the compiler generate the special methods, so we don't have to:
public:
WorldRenderer() = default;
That's much simpler. And this class:
class Cell { public: Cell(sf::Vector2i position, sf::Color color); ~Cell(); sf::Vector2i position; sf::Color color; }; Cell::Cell(sf::Vector2i position, sf::Color color) : position(position), color(color) { } Cell::~Cell() { }
becomes simply
struct Cell
{
sf::Vector2i position;
sf::Color color;
};
if we change the constructor calls to plain aggregate initialization.
Reduce copying
Instead of taking a copy of game.aliveCells, it might be better to expose a read-only reference:
private:
// Update the cells from position start(inclusive) to position end(exclusive).
std::vector<Cell> doUpdate(int start, int end, int coreIdx);
// A cache of all the alive cells at the end of the update() call.
std::vector<Cell> aliveCells = {};
public:
auto const& getLivingCells() const { return aliveCells; }
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell.position.x, cell.position.y, cell.color);
}
And addQuad can accept a const Cell& instead of unpacking it here:
void WorldRenderer::addQuad(const Cell& cell)
{
float gridXFloat = cell.position.x * 1.0f;
float gridYFloat = cell.position.y * 1.0f;
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat }, cell.color); // top-left
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat + 1}, cell.color); // bottom-left
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat + 1, gridYFloat + 1}, cell.color); // bottom-right
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat + 1, gridYFloat }, cell.color); // top-right
}
Here, I've used emplace_back to reduce the likelihood of copying. That takes us neatly to the next member, which can similarly be reduced:
void WorldRenderer::addBackgroundQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
Prefer declarative threading to hand-built parallelism
I can see that great care has been put into dividing the work into threads and collating the results, so it's hard to recommend throwing that away. But I'm going to (don't worry; having written it gives you a better understanding of what will happen behind the scenes). If we enable OpenMP (i.e. add -fopenmp to our GCC arguments, or equivalent on other compilers; use find_package(OpenMP) in CMake), then we don't need to explicitly code the mechanism of parallelisation, and instead we can focus on the content.
Here's the new update() (which also replaces doUpdate()) using OpenMP:
#include <omp.h>
void GameOfLife::update()
{
// clear aliveCells cache
aliveCells.clear();
#pragma omp parallel
{
// private, per-thread variables
auto this_thread_color = getThreadColor(omp_get_thread_num());
std::vector<Cell> next_generation;
#pragma omp for
for (int i = 0; i < worldSize.x * worldSize.y; ++i) {
auto pos = get2D(i);
int aliveCount = 0;
// check all 8 neighbors
for (int nX = -1; nX <= 1; ++nX) {
for (int nY = -1; nY <= 1; ++nY) {
// skip the current cell
if (nX == 0 && nY == 0) continue;
// wrap around to other side if neighbor would be outside world
int newX = (nX + pos.x + worldSize.x) % worldSize.x;
int newY = (nY + pos.y + worldSize.y) % worldSize.y;
aliveCount += getCell(newX, newY);
}
}
// Evaluate game rules on current cell
bool dies = aliveCount == 2 || aliveCount == 3;
bool lives = aliveCount == 3;
worldBuffer[i] = world[i] ? dies : lives;
// if the cell's alive push it into the vector
if (worldBuffer[i])
next_generation.emplace_back(Cell{pos, this_thread_color});
}
#pragma omp critical
aliveCells.insert(aliveCells.end(), next_generation.begin(), next_generation.end());
}
// apply updates
world.swap(worldBuffer);
}
We can now play with things such as dynamic or guided scheduling without perturbing the logic. And we can control the maximum number of threads without recompiling (using OMP_NUM_THREADS environment variable).
Fix an arithmetic bug
This conversion doesn't work after the display window has been resized by the user:
// normalize mouse pos int x = (mousePosition.x / 512.0f) * WORLD_SIZE_X; int y = (mousePosition.y / 512.0f) * WORLD_SIZE_Y;
#Modified code
Main.cpp
#include "GameOfLife.h"
#include "WorldRenderer.h"
#include <SFML/Graphics.hpp>
static const sf::Vector2i World_Size = { 256, 256 };
int main()
{
// create the window
sf::RenderWindow window({256, 256}, "Game of Life");
// scale the image up 2x size
window.setSize({512, 512});
// disable vsync and uncap framerate limit
window.setVerticalSyncEnabled(false);
window.setFramerateLimit(0);
// Create the game
GameOfLife game(World_Size);
// Create a world renderer
WorldRenderer worldRenderer;
// Track if mouse button is being held down
bool mouseHeld = false;
// run the program as long as the window is open
while (window.isOpen()) {
// check all the window's events that were triggered since the last iteration of the loop
sf::Event event;
while (window.pollEvent(event)) {
// "close requested" event: we close the window
if (event.type == sf::Event::Closed)
window.close();
// capture if the user is holding left mouse button down
if (event.type == sf::Event::MouseButtonPressed) {
if (event.mouseButton.button == sf::Mouse::Left)
mouseHeld = true;
} else if (event.type == sf::Event::MouseButtonReleased) {
if (event.mouseButton.button == sf::Mouse::Left)
mouseHeld = false;
}
}
// clear the window with black color
window.clear(sf::Color::Black);
// if left mouse button held down then make cells under cursor alive and pause simulation
if (mouseHeld) {
auto mousePosition = sf::Mouse::getPosition(window);
// normalize mouse pos
int x = mousePosition.x * World_Size.x / window.getSize().x;
int y = mousePosition.y * World_Size.y / window.getSize().y;
// set cell under cursor to alive
game.setCell(x, y, true);
} else {
// update the game world
game.update();
}
// render the game
worldRenderer.render(window, game);
// end the current frame
window.display();
}
}
GameOfLife.h
#pragma once
#include "Cell.h"
#include <SFML/System/Vector2.hpp>
#include <cstdint>
#include <vector>
class GameOfLife
{
public:
GameOfLife(sf::Vector2i size);
// Set the value of the cell at the given grid position to the given alive state.
void setCell(int x, int y, bool alive);
// Updates the state of the game world by one tick.
void update();
// Returns a reference to the cell value at the given grid position.
std::uint8_t & getCell(int x, int y);
// Returns a vector of the given cell's grid position by its cell index.
sf::Vector2i get2D(int index) const;
auto const& getLivingCells() const { return aliveCells; }
// Returns a color to use for cells/backgrounds based on the thread ID #.
static sf::Color getThreadColor(int index);
// Represents the width and height of the simulated world.
const sf::Vector2i worldSize;
private:
// A cache of all the alive cells at the end of the update() call.
std::vector<Cell> aliveCells = {};
// A 1D representation of the 2D grid that is the world.
std::vector<std::uint8_t> world;
// A buffer where the next world state is prepared, swapped with world at end of update().
std::vector<std::uint8_t> worldBuffer;
};
GameOfLife.cpp
#include "GameOfLife.h"
#include <omp.h>
#include <array>
GameOfLife::GameOfLife(sf::Vector2i size)
: worldSize(size),
world(size.x * size.y, false),
worldBuffer(world)
{
aliveCells.reserve(size.x * size.y); // reserve space for worst-case(all cells are alive)
// place an "acorn"
int midX = worldSize.x / 2;
int midY = worldSize.y / 2;
getCell(midX + 0, midY + 0) = 1;
getCell(midX + 1, midY + 0) = 1;
getCell(midX + 4, midY + 0) = 1;
getCell(midX + 5, midY + 0) = 1;
getCell(midX + 6, midY + 0) = 1;
getCell(midX + 3, midY + 1) = 1;
getCell(midX + 1, midY + 2) = 1;
}
std::uint8_t& GameOfLife::getCell(int x, int y)
{
return world[y * worldSize.x + x];
}
sf::Vector2i GameOfLife::get2D(int index) const
{
int y = index / worldSize.x;
int x = index % worldSize.x;
return { x, y };
}
sf::Color GameOfLife::getThreadColor(int index)
{
switch (index % 4) {
case 0:
return sf::Color::Red;
case 1:
return sf::Color::Green;
case 2:
return sf::Color::Blue;
case 3:
return sf::Color::Yellow;
}
return sf::Color::White;
}
void GameOfLife::update()
{
// clear aliveCells cache
aliveCells.clear();
#pragma omp parallel
{
// private, per-thread variables
auto this_thread_color = getThreadColor(omp_get_thread_num());
std::vector<Cell> next_generation;
#pragma omp for
for (int i = 0; i < worldSize.x * worldSize.y; ++i) {
auto const pos = get2D(i);
int aliveCount = 0;
// check all 8 neighbors
static const std::array<std::array<int, 2>, 8> neighbours{{{-1, -1}, {0, -1}, {1, -1},
{-1, 0}, {1, 0},
{-1, 1}, {0, 1}, {1, 1}}};
for (auto const [nX, nY]: neighbours) {
// wrap around to other side if neighbor would be outside world
int newX = (nX + pos.x + worldSize.x) % worldSize.x;
int newY = (nY + pos.y + worldSize.y) % worldSize.y;
aliveCount += getCell(newX, newY);
}
// Evaluate game rules on current cell
bool dies = aliveCount == 2 || aliveCount == 3;
bool lives = aliveCount == 3;
worldBuffer[i] = world[i] ? dies : lives;
// if the cell's alive push it into the vector
if (worldBuffer[i])
next_generation.emplace_back(Cell{pos, this_thread_color});
}
#pragma omp critical
aliveCells.insert(aliveCells.end(), next_generation.begin(), next_generation.end());
}
// apply updates
world.swap(worldBuffer);
}
void GameOfLife::setCell(int x, int y, bool alive)
{
// constrain x and y
x = std::max(std::min(x, (int) worldSize.x - 1), 0);
y = std::max(std::min(y, (int) worldSize.y - 1), 0);
getCell(x, y) = alive;
aliveCells.push_back(Cell{sf::Vector2i(x, y), sf::Color::White});
}
WorldRenderer.h
#pragma once
#include <SFML/Graphics.hpp>
#include <vector>
#include "GameOfLife.h"
class WorldRenderer
{
public:
WorldRenderer() = default;
// Renders the given game to the given window.
void render(sf::RenderWindow& window, GameOfLife& world);
private:
// Vertex points for the pending draw call.
std::vector<sf::Vertex> m_vertexPoints = {};
// Adds a cell-sized quad in the "grid position" specified.
void addQuad(const Cell& cell);
// Adds a darker colored quad in the given coordinates.
void addBackgroundQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color);
// Renders the background colors which correspond to the thread ID and the cells they are updating.
void renderBackgrounds(GameOfLife& world);
// Returns a darker variant of the given color.
sf::Color darkenColor(sf::Color input);
};
WorldRenderer.cpp
#include "WorldRenderer.h"
#include <omp.h>
void WorldRenderer::addQuad(const Cell& cell)
{
float gridXFloat = cell.position.x * 1.0f;
float gridYFloat = cell.position.y * 1.0f;
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat }, cell.color); // top-left
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat, gridYFloat + 1}, cell.color); // bottom-left
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat + 1, gridYFloat + 1}, cell.color); // bottom-right
m_vertexPoints.emplace_back(sf::Vector2f{gridXFloat + 1, gridYFloat }, cell.color); // top-right
}
void WorldRenderer::addBackgroundQuad(sf::Vector2f topLeft, sf::Vector2f bottomRight, sf::Color color)
{
auto topRight = topLeft;
auto bottomLeft = bottomRight;
std::swap(topRight.x, bottomLeft.x);
m_vertexPoints.emplace_back(topLeft, color);
m_vertexPoints.emplace_back(bottomLeft, color);
m_vertexPoints.emplace_back(bottomRight, color);
m_vertexPoints.emplace_back(topRight, color);
}
void WorldRenderer::render(sf::RenderWindow & window, GameOfLife & game)
{
// clear m_cellVertexPoints
m_vertexPoints.clear();
// draw backgrounds for "core zones"
renderBackgrounds(game);
// populate m_cellVertexPoints
for (auto const& cell: game.getLivingCells()) {
addQuad(cell);
}
// draw quads to window
window.draw(m_vertexPoints.data(), m_vertexPoints.size(), sf::Quads);
}
void WorldRenderer::renderBackgrounds(GameOfLife & world)
{
auto const maxThreads = omp_get_max_threads();
int cellsPerCore = world.worldSize.x * world.worldSize.y / maxThreads;
// first draw the background color of the final core index
addBackgroundQuad(
sf::Vector2f(0, 0),
sf::Vector2f(world.worldSize.x, world.worldSize.y),
darkenColor(world.getThreadColor(maxThreads - 1))
);
// draw the remaining core background colors on top, in reverse order
for (int i = maxThreads - 2; i >= 0; i--) {
auto end = world.get2D(cellsPerCore * (i + 1));
addBackgroundQuad(
sf::Vector2f(0, 0),
sf::Vector2f(world.worldSize.x, end.y),
darkenColor(world.getThreadColor(i))
);
}
}
sf::Color WorldRenderer::darkenColor(sf::Color input)
{
return sf::Color(input.r / 4, input.g / 4, input.b / 4);
}
Cell.h
#pragma once
#include <SFML/Graphics/Color.hpp>
#include <SFML/System/Vector2.hpp>
struct Cell
{
sf::Vector2i position;
sf::Color color;
};
CMakeLists.txt
# CMakeList.txt : CMake project for GameOfLife
#
cmake_minimum_required (VERSION 3.8)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3 -Werror -Wall -Wextra -Wwrite-strings -Wno-parentheses -Weffc++")
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
# Give the project a name
project(GameOfLife)
find_package(OpenMP)
find_package(SFML 2.5 COMPONENTS graphics REQUIRED)
set(SOURCES
src/Main.cpp
src/GameOfLife.cpp
src/GameOfLife.h
src/WorldRenderer.cpp
src/WorldRenderer.h
src/Cell.h
)
add_executable(GameOfLife ${SOURCES})
target_link_libraries(GameOfLife sfml-graphics OpenMP::OpenMP_CXX)
lang-cpp