5
\$\begingroup\$

Been working in games development professionally now for about ~5 years, but have only been doing OOP & haven't worked with multi threading before as I'm mostly a gameplay programmer, I've also been getting back to C++ which I haven't written in a long time. I wrote this small program as an exercise in DOP & multi threading.

Please ignore my "physics" equations if you can call them that, just threw together something.

The code defines entities split up between arrays of types such as Position, Velocity, and Physics. These are minimalistic types, just data containers really.

I've created two jobs, a RandomizeJob which just initializes the entities with random values, and a SimulateMotionJob which takes the intial random values and updates the speed, acceleration and position over time.

Lastly, we first print out the randomized intitial values in the start, and then we print the final simulated value at the end to compare how the entities have changed.

The goal of this was to try and run the job tasks in parallel, and avoid object oriented programming patterns which I'm used to.

I've gotten much feedback concerning how to write modern C++, which is great, don't get me wrong, I need that too. But please try and keep the feedback aimed towards the jobs, threading and data oriented part of this program apart for some obvious misses.

Thanks to everyone who has given me feedback, I truly appreciate it!

Here is the code:

#include <thread>
#include <random>
#include <cmath>
#include <mutex>
#include <algorithm>

#define GRAVITY 9.82f

#define SIM_TIME_SECONDS 10.0f
#define NUM_ENTITIES 500

struct Vector2
{
    float x;
    float y;
};

struct Position
{
    Vector2 pos;
};

struct Velocity
{
    float speed;
    Vector2 direction;
};

struct Physics
{
    float mass;
    float acceleration;
};

class RandomizeJob
{
public:
    typedef void (*RandomizeJobFinishedCallback)(Position outPositions[], Velocity outVelocities[], Physics outPhysics[]);
    RandomizeJob(Position positions[], Velocity velocities[], Physics physics[], RandomizeJobFinishedCallback onFinishedCallback)
    {
        std::thread positionsThread = std::thread(&RandomizeJob::RandomizePositions, positions);       
        std::thread velocitiesThread = std::thread(&RandomizeJob::RandomizeVelocities, velocities);
        std::thread physicsThread = std::thread(&RandomizeJob::RandomizePhysics, physics);

        positionsThread.join();
        velocitiesThread.join();
        physicsThread.join();

        onFinishedCallback(positions, velocities, physics);
    }
    
private:
    static std::mt19937 GetRandomGenerator();
    static void RandomizePositions(Position positions[]);
    static void RandomizeVelocities(Velocity velocities[]);
    static void RandomizePhysics(Physics physics[]);
};

std::mt19937 RandomizeJob::GetRandomGenerator()
{
    std::random_device randomDevice;
    return std::mt19937(randomDevice());
}

void RandomizeJob::RandomizePositions(Position positions[])
{
    std::mt19937 generator = GetRandomGenerator();
    std::uniform_real_distribution<float> range(-50.0f, 50.0f);

    for (size_t i = 0; i < NUM_ENTITIES; i++)
    {
        positions[i].pos.x = range(generator);       
        positions[i].pos.y = range(generator);       
    }
}

void RandomizeJob::RandomizeVelocities(Velocity velocities[])
{
    std::mt19937 generator = GetRandomGenerator();
    std::uniform_real_distribution<float> speedRange(0.0f, 80.0f);
    std::uniform_real_distribution<float> directionRange(-1.0f, 1.0f);

    for (size_t i = 0; i < NUM_ENTITIES; i++)
    {
        velocities[i].speed = speedRange(generator);
        
        const float randomDirX = directionRange(generator);       
        const float randomDirY = directionRange(generator);
        const float magnitude = std::sqrt(randomDirX * randomDirX + randomDirY * randomDirY);
        velocities[i].direction.x = randomDirX / magnitude;
        velocities[i].direction.y = randomDirY / magnitude;
    }
}

void RandomizeJob::RandomizePhysics(Physics physics[])
{
    std::mt19937 generator = GetRandomGenerator();
    std::uniform_real_distribution<float> massRange(40.0f, 100.0f);
    std::uniform_real_distribution<float> accelerationRange(-8.0f, 8.0f);

    for (size_t i = 0; i < NUM_ENTITIES; i++)
    {
        physics[i].mass = massRange(generator);
        physics[i].acceleration = accelerationRange(generator);
    }
}

class SimulateMotionJob
{
public:
    typedef void (*SimulateMotionJobFinishedCallback)(Position resultPositions[], Velocity resultVelocities[], Physics resultPhysics[]);
    SimulateMotionJob(Position positions[], Velocity velocities[], Physics physics[], SimulateMotionJobFinishedCallback onFinishedCallback)
    {
        const std::chrono::high_resolution_clock::time_point start = std::chrono::high_resolution_clock::now();
        std::chrono::high_resolution_clock::time_point last = start;
        std::chrono::duration<float> totalTime = last - start;
        while (totalTime.count() < SIM_TIME_SECONDS)
        {
            const std::chrono::high_resolution_clock::time_point current = std::chrono::high_resolution_clock::now();
            totalTime = current - start;
            const float deltaTime = static_cast<std::chrono::duration<float>>(current - last).count();

            const int lastSecondInt = static_cast<int>(static_cast<std::chrono::duration<float>>(last - start).count());
            const int currentSecondInt = static_cast<int>(totalTime.count());
            if (lastSecondInt != currentSecondInt)
            {
                printf("\n%i", currentSecondInt);
            }
            
            std::thread velocitiesThread = std::thread(&SimulateMotionJob::UpdateVelocities, velocities, physics, deltaTime);
            std::thread positionsThread = std::thread(&SimulateMotionJob::UpdatePositions, positions, velocities, deltaTime);

            velocitiesThread.join();
            positionsThread.join();
            
            last = current;
        }

        onFinishedCallback(positions, velocities, physics);
    }

private:
    static void UpdateVelocities(Velocity velocities[], Physics physics[], const float deltaTime);
    static void UpdatePositions(Position positions[], Velocity velocities[], const float deltaTime);
    
private:
    static std::mutex mutexes[2];
};

std::mutex SimulateMotionJob::mutexes[2];

void SimulateMotionJob::UpdateVelocities(Velocity velocities[], Physics physics[], const float deltaTime)
{
    for (size_t i = 0; i < NUM_ENTITIES; i++)
    {
        const size_t flipFlop = i % 2;
        std::lock_guard<std::mutex> lock(mutexes[flipFlop]);
        physics[i].acceleration = physics[i].acceleration - GRAVITY * deltaTime;
        velocities[i].speed = std::clamp(velocities[i].speed + physics[i].acceleration * deltaTime, 0.0f, std::numeric_limits<float>::max());
    }
}

void SimulateMotionJob::UpdatePositions(Position positions[], Velocity velocities[], const float deltaTime)
{
    for (size_t i = 0; i < NUM_ENTITIES; i++)
    {
        const size_t flipFlop = i % 2;
        std::lock_guard<std::mutex> lock(mutexes[flipFlop]);
        positions[i].pos.x += velocities[i].direction.x * velocities[i].speed * deltaTime;
        positions[i].pos.y += velocities[i].direction.y * velocities[i].speed * deltaTime;
    }
}

void PrintEntities(Position positions[], Velocity velocities[], Physics physics[])
{
    for (size_t i = 0; i < NUM_ENTITIES; i++)
    {
        printf("\nEntity: %i", i);
        printf("\nPos: x=%f, y=%f", positions[i].pos.x, positions[i].pos.y);
        printf("\nVelocity: x=%f, y=%f, speed=%f", velocities[i].direction.x, velocities[i].direction.y, velocities[i].speed);
        printf("\nPhysics: mass=%f, acceleration=%f", physics[i].mass, physics[i].acceleration);
        printf("\n----------------------------");
    }
}

int main(int argc, char* argv[])
{
    Position positions[NUM_ENTITIES];
    Velocity velocities[NUM_ENTITIES];
    Physics physics[NUM_ENTITIES];

    RandomizeJob randomizeJob(positions, velocities, physics,
        [](Position resultPositions[], Velocity resultVelocities[], Physics resultPhysics[])
    {
        PrintEntities(resultPositions, resultVelocities, resultPhysics);
            
        SimulateMotionJob motionJob(resultPositions, resultVelocities, resultPhysics,
            [](Position resultPositions[], Velocity resultVelocities[], Physics resultPhysics[])
        {
            PrintEntities(resultPositions, resultVelocities, resultPhysics);
        });
    });
    
    return 0;
}

\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! This question is incomplete. To help reviewers give you better answers, please edit to add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does. \$\endgroup\$ Jul 18 at 7:06

3 Answers 3

9
\$\begingroup\$

Consider Parallelizing by Range, not by Job

The form of parallelism this currently uses is to spawn one thread per task, and join the tasks. Sometimes, there are fewer tasks than there are threads available. Sometimes, one of the tasks is much longer than the others, and running a much smaller one in parallel doesn’t gain much.

You should seriously consider using either a C++20 <algorithm> that can take an execution policy, such as std::transform, together with the std::ranges library. Or, alternatively, a loop with OpenMP directives, such as #pragma omp parallel for simd.

The most difficult part of the program to parallelize this way would be the initialization with random data, since you’re using a deterministic pseudo-random number generator whose results are sequential. Often in networked games, you need to do this to maintain synchronization without excessive network traffic, but you might fill each partition in a different thread that starts from a different random seed. Be careful: if you use the same generator to provide the seeds, your partitions will repeat themselves in predictable ways.

Not Everything Needs to be Inside a Class

If you just want to encapsulate the things in each Job class, you could put them in different source files and expose only the public functions in headers. If you just want to be able to qualify their names, you can write a namespace for them.

Use constexpr and noexcept Where Appropriate

These can help the compiler optimize your code, or warn you when you’re doing something that inhibits optimization.

Consider Structures-of-Arrays Instead of Arrays-of-Structures

Currently, you store the arrays of structures that represent (x,y) coordinates. But many physics calculations operate on x and y coordinates either independently or in parallel. Especially when storing triples (which won't fit evenly into the CPU’s vector registers), you can often get better performance by looking up velocities.xs[i] instead of velocities[i].x.

Don’t Repeat Yourself

You currently have a lot of identical classes that amount to, “fill this array with pseudo-random numbers.” This should be a single function that fills a range. Or you can use the one in the standard library: std::generate with your RNG as the generator.

Write Code that Can be Vectorized

I know you said not to comment on the physics code, but it’s a great example. Right now, you’ve written inherently-sequential code that generates pseudo-random numbers and applies them to each x and y. If you stored your random values first in a std::vector<float> (or a std::experimental::native_simd<float>, or just an array of the right size and alignment for your target CPU), you’d be able to update your positions, velocities and accelerations with SIMD instructions, in addition to running them on multiple threads.

If you are not enabling SIMD, you should at least be calculating with double precision for the higher accuracy. Game programming only uses shorter floating-point numbers to be able to perform more SIMD operations at once.

Look for Efficiencies

You currently avoid race conditions by having separate positions and resultPositions, velocities and resultVelocities, etc.

Since these are native arrays and not std::vector<float>, they cannot be moved, and their contents must be copied instead.

Here’s a simple (and untested) mock-up of a loop that both vectorizes and parallelizes the update loop without making copies:

#include <algorithm> // for generate
#include <array>
#include <cassert>
#include <cmath> // for exp2f
#include <cstddef>
#include <cstdlib>
#include <functional> // for std::ref
#include <iterator> // for begin, end
#include <omp.h>
#include <random>
#include <thread>
#include <utility> // for declval

using std::exp2f, std::ptrdiff_t, std::size_t;

constexpr float deltaT = 0.0625f; // seconds.
constexpr size_t NUM_ENTITIES = 1024;
constexpr unsigned VECTOR_ALIGNMENT = 32; // Optimal for x86-64-v3.

struct Coordinates {
    alignas(VECTOR_ALIGNMENT) float xs[NUM_ENTITIES];
    alignas(VECTOR_ALIGNMENT) float ys[NUM_ENTITIES];
};

/* A class that generates random floats in [-500,500), from a
 * uniform distribution.
 */
class BigRandom {
    public:
        // Default constructor, copy constructor, etc.
        float operator()() {
            return static_cast<float>(rng()) * scaling_factor * 1000.0f
                - 500.0f;
        }

    private:
        using RNG = std::mt19937;
        static const float scaling_factor;
        RNG rng = RNG();
};
const float BigRandom::scaling_factor = exp2f(-static_cast<float>(BigRandom::RNG::word_size));

/* A class that generates random floats in [-0.5,0.5), from a
 * uniform distribution.
 */
class LittleRandom {
    public:
        // Default constructor, copy constructor, etc.
        float operator()() {
            return static_cast<float>(rng()) * scaling_factor - 0.5f;
        }

    private:
        using RNG = std::mt19937;
        static const float scaling_factor;
        RNG rng = RNG();
};
const float LittleRandom::scaling_factor = exp2f(-static_cast<float>(LittleRandom::RNG::word_size));

// Convenient wrapper to make thread creation simpler.
void fillBigRandom(decltype(std::declval<Coordinates>().xs) &range, BigRandom&& rng) {
     std::generate(std::begin(range), std::end(range), rng);
}

// Convenient wrapper to make thread creation simpler.
void fillLittleRandom(decltype(std::declval<Coordinates>().xs) &range, LittleRandom&& rng) {
     std::generate(std::begin(range), std::end(range), rng);
}

static void init(Coordinates& positions, Coordinates& velocities, Coordinates& accelerations, Coordinates& deltas) {
    // Because of the alignment of each array, there should not have false sharing.
    std::thread pxThread = std::thread{fillBigRandom,
                                       std::ref(positions.xs),
                                       BigRandom()};
    std::thread pyThread = std::thread{fillBigRandom,
                                       std::ref(positions.ys),
                                       BigRandom()};
    std::thread vxThread = std::thread{fillLittleRandom,
                                       std::ref(velocities.xs),
                                       LittleRandom()};
    std::thread vyThread = std::thread{fillLittleRandom,
                                       std::ref(velocities.ys),
                                       LittleRandom()};
    std::thread axThread = std::thread{fillLittleRandom,
                                       std::ref(accelerations.xs),
                                       LittleRandom()};
    std::thread ayThread = std::thread{fillLittleRandom,
                                       std::ref(accelerations.ys),
                                       LittleRandom()};
    std::thread dxThread = std::thread{fillLittleRandom,
                                       std::ref(deltas.xs),
                                       LittleRandom()};
    std::thread dyThread = std::thread{fillLittleRandom,
                                       std::ref(deltas.ys),
                                       LittleRandom()};

    // Move these around to control how many threads run at once:
    pxThread.join();
    pyThread.join();
    vxThread.join();
    vyThread.join();
    axThread.join();
    ayThread.join();
    dxThread.join();
    dyThread.join();
}

int main() {
    static Coordinates positions, velocities, accelerations, deltas;
    init(positions, velocities, accelerations, deltas);

    #pragma omp parallel for schedule(static)
    for (ptrdiff_t i = 0; i < (ptrdiff_t)NUM_ENTITIES; ++i) {
        positions.xs[i] += deltaT * (0.5f * deltaT * accelerations.xs[i]
                                     + velocities.xs[i]);
        positions.ys[i] += deltaT * (0.5f * deltaT * accelerations.ys[i]
                                     + velocities.ys[i]);
        velocities.xs[i] += deltaT * accelerations.xs[i];
        velocities.ys[i] += deltaT * accelerations.ys[i];
        accelerations.xs[i] += deltas.xs[i];
        accelerations.ys[i] += deltas.ys[i];
    }

    return EXIT_SUCCESS;
}

Except that you wouldn’t use such a crude formula as the one in the game loop for your game physics. But this code demonstrates both <thread> and OpenMP parallelism, and doesn’t make any deep copies.

On the Godbolt compiler explorer, the inner loop compiles (on ICX 2023.1 with -std=c++20 -O3 -march=x86-64-v3 -fiopenmp -fp-model=fast) to:

.LBB9_3:                                # =>This Inner Loop Header: Depth=1
        vmovups         ymm2, ymmword ptr [rsi + 4*rdi + main::accelerations]
        vmovups         ymm3, ymmword ptr [rsi + 4*rdi + main::velocities]
        vmovaps         ymm4, ymm0
        vfmadd213ps     ymm4, ymm2, ymm3        # ymm4 = (ymm2 * ymm4) + ymm3
        vfmadd213ps     ymm4, ymm1, ymmword ptr [rsi + 4*rdi + main::positions] # ymm4 = (ymm1 * ymm4) + mem
        vmovups         ymmword ptr [rsi + 4*rdi + main::positions], ymm4
        vmovups         ymm4, ymmword ptr [rsi + 4*rdi + main::accelerations+4096]
        vmovups         ymm5, ymmword ptr [rsi + 4*rdi + main::velocities+4096]
        vmovaps         ymm6, ymm0
        vfmadd213ps     ymm6, ymm4, ymm5        # ymm6 = (ymm4 * ymm6) + ymm5
        vfmadd213ps     ymm6, ymm1, ymmword ptr [rsi + 4*rdi + main::positions+4096] # ymm6 = (ymm1 * ymm6) + mem
        vmovups         ymmword ptr [rsi + 4*rdi + main::positions+4096], ymm6
        vfmadd231ps     ymm3, ymm2, ymm1        # ymm3 = (ymm2 * ymm1) + ymm3
        vmovups         ymmword ptr [rsi + 4*rdi + main::velocities], ymm3
        vfmadd231ps     ymm5, ymm4, ymm1        # ymm5 = (ymm4 * ymm1) + ymm5
        vmovups         ymmword ptr [rsi + 4*rdi + main::velocities+4096], ymm5
        vaddps          ymm2, ymm2, ymmword ptr [rsi + 4*rdi + main::deltas]
        vmovups         ymmword ptr [rsi + 4*rdi + main::accelerations], ymm2
        vaddps          ymm2, ymm4, ymmword ptr [rsi + 4*rdi + main::deltas+4096]
        vmovups         ymmword ptr [rsi + 4*rdi + main::accelerations+4096], ymm2
        add     rdi, 8
        cmp     rdi, rcx
        jl      .LBB9_3

The code generated for the original code does not vectorize. Here’s the inner loop of SimulateMotionJob::UpdatePositions with the same settings:

.LBB7_4:                                # =>This Inner Loop Header: Depth=1
        mov     eax, r12d
        and     eax, 1
        lea     rax, [rax + 4*rax]
        lea     r15, [8*rax + SimulateMotionJob::mutexes]
        mov     rdi, r15
        call    pthread_mutex_lock
        test    eax, eax
        jne     .LBB7_7
        vmovss  xmm0, dword ptr [rsp + 4]       # 4-byte Reload
        vmulss  xmm0, xmm0, dword ptr [rbx - 4]
        vmovsd  xmm1, qword ptr [rbx]           # xmm1 = mem[0],zero
        vbroadcastss    xmm0, xmm0
        vmovsd  xmm2, qword ptr [r14 + 8*r12]   # xmm2 = mem[0],zero
        vfmadd231ps     xmm2, xmm1, xmm0        # xmm2 = (xmm1 * xmm0) + xmm2
        vmovlps qword ptr [r14 + 8*r12], xmm2
        mov     rdi, r15
        call    pthread_mutex_unlock
        inc     r12
        add     rbx, 12
        cmp     r12, 500
        jne     .LBB7_4

Notice the load of four-byte dwords of data, rather than 32-byte ymmwords, and two system calls (with high overhead) on each iteration. (This is because of the incorrect use of mutexes pointed out elsewhere; you in fact should split the workload such that each thread should only be able to read constant shared data, and write to unshared data.)

Alternatively, you could use std::valarray<float> to get something almost like the parallel array operations of Fortran, where the equivalent is

    ps = ps + dt*(vs + dt*0.5*as)
    vs = vs + dt*as

There is at Least One Bug

The line

printf("\nEntity: %i", i);

should have a format specifier of %zu, since i is a size_t. The right warning would have caught this. Or you could use <iostream>.

\$\endgroup\$
3
  • \$\begingroup\$ When using namespaces instead. Since the classes I wrote aren't storing any state, they can be namespaces, and instead expose functions in the header. However, if I'm using std::array and no longer define a global const for the entity size, I need to use a template function: template<std::size_t SIZE> std::array<Position, SIZE>. So now my implementation would go in the header file. Now I cant expose the declaration in the header for outside access, since the implementation cannot access the other functions in the cpp, and if I include those too, now everything is accessable in the namespace. \$\endgroup\$ Jul 20 at 8:54
  • \$\begingroup\$ @larssonmartin You should be able to do that. Put using elem_t = float; and constexpr size_t ROW_SIZE = 1024; in the namespace, or even using row_t = std::array<elem_t, ROW_SIZE>;. You cam now use either my::row_t or std::array<my::elem_t, my::ROW_SIZE> in your .cpp files. \$\endgroup\$
    – Davislor
    Jul 20 at 17:28
  • \$\begingroup\$ @larssonmartin If you’re thinking of using code like this, bear in mind that it’s very simplified. Especially the random-number generators, which in this implementation initialize every thread using the same default seed. \$\endgroup\$
    – Davislor
    Jul 20 at 17:30
8
\$\begingroup\$

Initial Impressions

Using struct For Custom Types With No Invariants

I like your use of struct for your custom types having no apparent invariants - Vector2, Velocity, Physics, etc.

This fits with the C++ Core Guidelines C.2 guideline: "Use class if the class has an invariant; use struct if the data members can vary independently".

Position Contains A Vector2

This is concerning to me.

It seems what is done here is the making of a new type that is in essence still a Vector2. Vector2 is unit-less while the concept of position is units of distance relative to an origin.

Have you considered using a strongly typed units library, like boost units, and then making Position be an aggregate of length-units in the dimensions of the problem domain (x & y)? Then Position could be like:

struct Position {
    length x;
    length y;
};

No Strongly Typed Physical Units

While your request is to "ignore... physics equations", I'd recommend using a strongly typed units library as a point of design as recommended by C++ Core Guidelines guideline P.1: "Express ideas directly in code". At least in my opinion, using a strongly typed units library would better express ideas in code per this guidance.

Using C-Preprocessor Macros

C++ Core Guideline Enum.1 is: "Prefer enumerations over macros". I concur. I'd recommend replacing the uses of the C preprocessor macros with enumerations, or with constexpr, or even const inline variables.

For example, replace #define GRAVITY 9.82f with:

constexpr auto Gravity = 9.82f;

Incidentally, you haven't said which version of the C++ language standard you need to be using and/or constrained by, but the use of names like std::mt19937 (and some includes) suggests you're minimally using C++11. C++11 gives us the constexpr specifier so using it should work with your existing uses.

Using C-style Arrays

This code makes definitions like:

Position positions[NUM_ENTITIES];
Velocity velocities[NUM_ENTITIES];
Physics physics[NUM_ENTITIES];

These are what I'm (and others) are calling "C-style arrays". C++11 onwards, gives us std::array. A benefit of using std::array is in carrying the number of elements as part of the type. The number of elements this way doesn't decay away from our code. I'd suggest using std::array instead of C-style arrays for their storage types:

std::array<Position, NUM_ENTITIES> positions;
std::array<Velocity, NUM_ENTITIES> velocities;
std::array<Physics, NUM_ENTITIES> physics;

Then passing these arrays as references to these std::array types, or from C++20 onwards using std::span of the elements. Example (just showing constructor's declaration here for sake of exposition):

RandomizeJob::RandomizeJob(std::array<Position, NUM_ENTITIES> &positions, std::array<Velocity, NUM_ENTITIES> &velocities, std::array<Physics, NUM_ENTITIES> &physics, RandomizeJobFinishedCallback onFinishedCallback);

RandomizeJob As A Type

This code has RandomizeJob as a custom type. It has only the one explicitly defined public member function - a constructor. The RandomizeJob type doesn't even hold any state; it's all within the constructor. This confuses type with function.

The intention to randomize the data is more idiomatically expressed using a free function, than a type's constructor member function. It's arguably, more encapsulated to use free functions in this case. I really love Scott Meyer's article "How Non-Member Functions Improve Encapsulation" on this (see https://www.aristeia.com/Papers/CUJ_Feb_2000.pdf).

\$\endgroup\$
3
  • \$\begingroup\$ @greybeard Thank you for pointing out the typo I introduced. I have fixed the number to now hopefully be the same as the OP. \$\endgroup\$ Jul 16 at 16:53
  • 1
    \$\begingroup\$ Thanks for the pointers! Just to explain the Vector2 type, as I'm a gameplay programmer I mostly ever work inside the Unity Game Engine and Unreal Engine which comes pre-defined vector types (and a lot more). I just wanted something similar to what I'm already used to, especially since I was mostly focusing on the DOP/MT parts. That said, I haven't coded modern C++ since school, and I've never really coded C so I am definitely not up to speed on some of the standards of modern C++, especially since the only C++ I write is in Unreal Engine which has strong guidelines to follow. \$\endgroup\$ Jul 16 at 19:08
  • 1
    \$\begingroup\$ Sorry, forgot to mention that, but I'm compiling this with c++11, however, as this is meant to be just an exercise, I have no problem of upgrading it to incorporate more modern ideas. I also have some reading up to do, I wasn't even aware of std::array, or std::span, I can also see your argument for RandomizeJob as a type, it's a habbit from many years of C# which I'm working already trying to get better at, however, I haven't even heard the argument against typing everything like so as clearly presented before, so thank you! \$\endgroup\$ Jul 16 at 19:18
8
\$\begingroup\$

Use an existing vector library

Just to explain the Vector2 type, as I'm a gameplay programmer I mostly ever work inside the Unity Game Engine and Unreal Engine which comes pre-defined vector types (and a lot more). I just wanted something similar to what I'm already used to, [...]

Consider using one of the many open source libraries for C++ that implement vector types. Especially if you are into game development, a good one is GLM. Your Vector2 could then be replaced with glm::vec2, which also has all the mathematical operators you expect on vector types.

SimulateMotionJob should not be a class

Louis Langholtz already mentioned that RandomizeJob should just be a function instead of a class. The same actually goes for SimulateMotionJob: it only has one public function (the constructor), the private functions and the member variables are only used for executing the public function.

Overly verbose names

While it's often better to be verbose than to abbreviate unnecessarily, I would say that you are overly verbose in some cases. For example, why is RandomizeJob a Job? I don't think that adds anything useful, and would name it Randomize (verb, if it's a function) or Randomizer (noun, if it's an object) instead.

Within class RandomizeJob you have this typedef:

typedef void (*RandomizeJobFinishedCallback)(Position outPositions[], Velocity outVelocities[], Physics outPhysics[]);

So now you have something with the full name RandomizeJob::RandomizeJobFinishedCallback. That's a very long name, with RandomizeJob mentioned twice. I would at least remove the redundant part and name it FinishedCallBack.

Use more auto

Related to the above, you write out types in many cases where it can easily be avoided. For example, instead of:

std::thread positionsThread = std::thread(&RandomizeJob::RandomizePositions, positions);       

I'd write:

auto positionsThread = std::thread(&RandomizeJob::RandomizePositions, positions);

You can also use it in lambda expressions:

SimulateMotionJob motionJob(resultPositions, resultVelocities, resultPhysics,
    [](auto positions, auto velocities, auto physics)
    {
        PrintEntities(positions, velocities, physics);
    }
);

Although if you are going to just pass everything 1:1 to another function, you could just have written:

SimulateMotionJob motionJob(resultPositions, resultVelocities, resultPhysics,
    PrintEntities
);

Or even better:

onFinishedCallback does not provide any benefits

Why have a callback that's only called exactly once at the end of a function? The caller can easily call it themselves. Consider that without that callback, you could have written:

int main(int argc, char* argv[])
{
    Position positions[NUM_ENTITIES];
    Velocity velocities[NUM_ENTITIES];
    Physics physics[NUM_ENTITIES];

    RandomizeJob randomizeJob(positions, velocities, physics);
    PrintEntities(positions, velocities, physics);
    SimulateMotionJob motionJob(positions, velocities, physics);
    PrintEntities(positions, velocities, physics);
}

Avoid using printf() in C++

printf() is a C function, and while you can use it in C++, you should avoid it. Instead, use the C++ way of printing, which is type safe. In fact, you made an error here:

for (size_t i = 0; i < NUM_ENTITIES; i++)
{
    printf("\nEntity: %i", i);
    …
}

Note that i is a size_t, but that's not the same as an int. But in the format string you use %i, so printf() expects an int as the next argument. It might not cause a crash, and even print the right value on little-endian systems, but on 64-bit big-endian systems this could cause it to always print 0. Instead, you should have written:

std::cout << "\nEntity: " << i;

Note that C++20 introduced std::format() and C++23 will bring std::print(), giving you the benefits of format strings and the type safety of C++.

Incorrect use of mutexes

It seems like you use mutexes so the velocity and position threads don't step on each other's toes. However, it doesn't guarantee that the threads stay in sync with each other. In particular, consider that the velocity thread has just unlocked its mutex, and gets ready to lock the other mutex, but just at that point the operating system decides to pause that thread for something else to run. Then the position thread can continue, will not see any mutex being locked, and will happily run far ahead of the velocity thread, seeing old velocities instead of new ones.

In fact, there's not even a guarantee that the velocity thread will be the first one that's actually starting to run.

You could use atomic counters, std::barrier or other constructions to ensure it runs in the order you want. But having to synchronize on each individual entity is going to be slow anyway. It would be better to run the velocity and position updates in serial, but to parallelize updating entities in the velocity and later the position step (as Davislor mentioned, you can do this with the parallel version of std::transform()).

\$\endgroup\$
3
  • \$\begingroup\$ If I don't use the callbacks, how can I guarantee that the other thread has finished running? I might be mistaken, but doesn't PrintEntities run on the MT before the other threads have a chance to finish now? \$\endgroup\$ Jul 17 at 10:57
  • \$\begingroup\$ You join()all threads in the constructors, so it's guaranteed that the threads won't be running anymore when the constructors returns on the main thread. \$\endgroup\$
    – G. Sliepen
    Jul 17 at 11:07
  • \$\begingroup\$ That’s great feedback, I totally had that backwards, thanks! \$\endgroup\$ Jul 17 at 15:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.