Not executing all writes before end of program

Question

For a school project we were tasked with writing a ray tracer. I chose to use C++ since it's the language I'm most comfortable with, but I've been getting some weird artifacts.

Please keep in mind that we are still in the first few lessons of the class, so right now we are limited to checking whether or not a ray hits a certain object.

When my raytracer finishes quickly (less than 1 second spent on actual ray tracing) I've noticed that not all hits get registered in my "framebuffer".

To illustrate, here are two examples:

1.: 2.:

In the first image, you can clearly see that there are horizontal artifacts. The second image contains a vertical artifact.

I was wondering if anyone could help me to figure out why this is happening?

I should mention that my application is multi-threaded, the multithreaded portion of the code looks like this:

Stats RayTracer::runParallel(const std::vector<Math::ivec2>& pixelList, const Math::vec3& eyePos, const Math::vec3& screenCenter, long numThreads) noexcept
{
    //...

    for (int i = 0; i < threads.size(); i++)
    {
        threads[i] = std::thread(&RayTracer::run, this, splitPixels[i], eyePos, screenCenter);
    }

    for (std::thread& thread: threads)
    {
        thread.join();
    }

    //...
}

The RayTracer::run method access the framebuffer as follows:

Stats RayTracer::run(const std::vector<Math::ivec2>& pixelList, const Math::vec3& eyePos, const Math::vec3& screenCenter) noexcept
{
    this->frameBuffer.clear(RayTracer::CLEAR_COLOUR);

    // ...

    for (const Math::ivec2& pixel : pixelList)
    {
        // ...

        for (const std::shared_ptr<Objects::Object>& object : this->objects)
        {
            std::optional<Objects::Hit> hit = object->hit(ray, pixelPos);

            if (hit)
            {
                // ...

                if (dist < minDist)
                {
                    std::lock_guard lock (this->frameBufferMutex);

                    // ...
                    this->frameBuffer(pixel.y, pixel.x) = hit->getColor();
                }
            }
        }
    }

    // ...
}

This is the operator() for the framebuffer class

class FrameBuffer
{
    private:
        PixelBuffer buffer;

    public:
        // ...

        Color& FrameBuffer::operator()(int row, int col) noexcept
        {
            return this->buffer(row, col);
        }

        // ...
}

Which make use of the PixelBuffer's operator()

class PixelBuffer
{
    private:
        int mRows;
        int mCols;

        Color* mBuffer;

    public:
        // ...

        Color& PixelBuffer::operator()(int row, int col) noexcept
        {
            return this->mBuffer[this->flattenIndex(row, col)];
        }

        // ...
}

I didn't bother to use any synchronization primitives because each thread gets assigned a certain subset of pixels from the complete image. The thread casts a ray for each of its assigned pixels and writes the resultant color back to the color buffer in that pixel's slot. This means that, while all my threads are concurrently accessing (and writing to) the same object, they don't write to the same memory locations.

After some initial testing, using a std::lock_guard to protect the shared framebuffer seems to help, but it's not a perfect solution, artifacts still occur (although much less common).

It should be noted that the way I divide pixels between threads determines the direction of the artifacts. If I give each thread a set of rows the artifacts will be horizontal lines, if I give each thread a set of columns, the artifacts will be vertical lines.

Another interesting conclusion is that when I trace more complex objects (These take anywhere between 30 seconds and 2 minutes) these aretifacts are extremely rare (I've seen it once in my 100's-1000's of traces so far)

I can't help but feel like this is a problem related to multithreading, but I don't really understand why std::lock_guard wouldn't completely solve the problem.

Edit: After suggestions by Jeremy Friesner I ran the raytracer about 10 times on a single thread, without any issues, so the problem does indeed appear to be a race condition.

Answer 1

I solved the problem thanks to Jeremy Friesner.

As you can see in the code, every thread calls framebuffer.clear() separately (without locking the mutex!). This means that thread A might have already hit 5-10 pixels because it was started first when thread B clears the framebuffer. this would erase thread A's already hit pixels.

By moving the framebuffer.clear() call to the beginning of the runParallel() method I was able to solve the issue.