<p>I fixed it, It was just a coding mistake, there was nothing wrong with the implementation. So the error was in fact in the 2nd for-loop when I was parsing the image buffer. I was reaching for the last pixel in the vector and then popping it off from the back of the stack. </p>

<p>This means that in the for loop condition, the vector size will be changing but k will still be incrementing the same, so it will effectively only do half of the elements per vector. Because as K is incrementing stack.size() is decrementing so they meet in the middle and the loop is done.</p>

<p>To fix this, I just used index notation, so:</p>

<pre><code>rgbpacket &amp;rgbpixel = stack[index];
</code></pre>

<p>and then I deleted the line where I was popping off from the stack.</p>

<p>Even though this is a stupid bug,
I hope this helps other people.</p>

CC BY-SA 3.0
2018-04-24T06:37:36.247Z
2018-04-24T06:37:36.247Z
Varun Govind
<p>I'm having some issues with my multi-threaded ray tracing implementation. It seems to only render about half of the image. Here is the picture:
<a href="https://i.sstatic.net/kbYe6.png" rel="nofollow noreferrer"><img src="https://i.sstatic.net/kbYe6.png" alt="enter image description here"></a></p>

<p>Here is the code:</p>

<pre><code>int max = (nx * ny); //width * height
volatile std::atomic&lt;int&gt; count(0);

int cores = std::thread::hardware_concurrency();
cores -= 1;
vector&lt;future&lt;vector&lt;rgbpacket&gt;&gt;&gt; future_vector;
cout &lt;&lt; "Threads supported: " &lt;&lt; cores &lt;&lt; "\n";
while (cores--) {
    future_vector.emplace_back(
        std::async([=, &amp;world, &amp;cam, &amp;dim, &amp;orth, &amp;count] {
            vector&lt;rgbpacket&gt; stack;
            while (true) {

                int index = count++;

                if (index &gt;= max) {

                    return stack;
                } else {
                    int k = (index % nx); //mod width to get column
                    int j = (index / nx); //divide width to get row

                    rgbpacket curpx = render(j, k, dim, world, cam, orth);
                    curpx.k = (unsigned int)k;
                    curpx.j = (unsigned int)j;
                    stack.push_back(curpx);

                }
            }
        })
    );
}

//parse the image buffer
for (unsigned int i = 0; i &lt; future_vector.size(); i++) {
    vector&lt;rgbpacket&gt; stack = future_vector[i].get();
    for (unsigned int k = 0; k &lt; stack.size(); k++) {
        rgbpacket rgb_comp = stack.back();

        //Using the new compressed packet
        rgbaColor conv;
        conv.r = rgb_comp.r;
        conv.g = rgb_comp.g;
        conv.b = rgb_comp.b;

        hslaColor pix = rgb2hsl(conv);
        HSLAPixel newpixel(pix.h, pix.s, pix.l);

        HSLAPixel &amp;imgpx = img.getPixel(rgb_comp.k, rgb_comp.j);
        imgpx = newpixel;

        stack.pop_back();
    }
}
</code></pre>

<p>For some preface, I am calculating the pixel with respect to the position with the <strong>index</strong> variable. Once I calculate it, I store it (along with the position) in the vector. After I calculate all of the pixel values, I parse them into an image buffer and later print that image.</p>

<p>I've narrowed the issue (I think) to where I am calculating the rendered pixel and storing it in the vector. For some reason, it does not store all of the pixels in the vector and I am not sure why? Any help is appreciated. Thanks!</p>

CC BY-SA 3.0
2018-04-13T08:35:45.200Z
I don't think std::vector<> is thread safe, you should be using something like a mutex when calling push_back()
PaulHK
CC BY-SA 3.0
2018-04-14T18:00:54.953Z
Hi @PaulHK, Could this be part of the reason for the half-picture behavior? I got some more information about the memory, The objects are stored in memory so when I call the render function, the "hits" itself might not be registering because of the asynchronous memory accesses. But I was thinking that this would cause more random behavior, like seeing a random distribution of pixels on the picture every time.
Varun Govind
CC BY-SA 3.0
2018-04-15T04:04:54.733Z
You could remove your completion queue (future_vector) and just have your workers directly write to an RGB array, you don't need thread synchronisation for that. You will need a mechanism to detect when all workers are completed though, maybe something like an atomic counter and semaphore.
PaulHK
CC BY-SA 3.0
2018-04-15T04:09:08.320Z
The half written image is probably caused by the main thread not waiting for workers to complete. In addition to that it is hazardous for multiple threads to access the vector<> and that could cause issues if 2 threads update the internal state of the vector<> at the same time.
PaulHK
CC BY-SA 3.0
2018-04-15T04:34:14.767Z
I thought if I declare the vector in the async function, it makes the structure in thread memory, does it not? You are probably right, I will try and fix it.
Varun Govind
CC BY-SA 3.0
2018-04-24T06:38:39.570Z
Hey PaulHK, thanks for helping me out, I fixed the bug, it was dumb, I posted an answer to anyone who might look at this.
Varun Govind
<raytracing><c++><multithreading>
Issues with Multi-threaded Raytracing
Varun Govind
# Issues with Multi-threaded Raytracing

(Asked by: username_0 on 2018-04-09)

I'm having some issues with my multi-threaded ray tracing implementation. It seems to only render about half of the image. Here is the picture:
[![enter image description here](https://i.sstatic.net/kbYe6.png)](https://i.sstatic.net/kbYe6.png)

Here is the code:

```
int max = (nx * ny); //width * height
volatile std::atomic<int> count(0);

int cores = std::thread::hardware_concurrency();
cores -= 1;
vector<future<vector<rgbpacket>>> future_vector;
cout << "Threads supported: " << cores << "\n";
while (cores--) {
    future_vector.emplace_back(
        std::async([=, &world, &cam, &dim, &orth, &count] {
            vector<rgbpacket> stack;
            while (true) {

                int index = count++;

                if (index >= max) {

                    return stack;
                } else {
                    int k = (index % nx); //mod width to get column
                    int j = (index / nx); //divide width to get row

                    rgbpacket curpx = render(j, k, dim, world, cam, orth);
                    curpx.k = (unsigned int)k;
                    curpx.j = (unsigned int)j;
                    stack.push_back(curpx);

                }
            }
        })
    );
}

//parse the image buffer
for (unsigned int i = 0; i < future_vector.size(); i++) {
    vector<rgbpacket> stack = future_vector[i].get();
    for (unsigned int k = 0; k < stack.size(); k++) {
        rgbpacket rgb_comp = stack.back();

        //Using the new compressed packet
        rgbaColor conv;
        conv.r = rgb_comp.r;
        conv.g = rgb_comp.g;
        conv.b = rgb_comp.b;

        hslaColor pix = rgb2hsl(conv);
        HSLAPixel newpixel(pix.h, pix.s, pix.l);

        HSLAPixel &imgpx = img.getPixel(rgb_comp.k, rgb_comp.j);
        imgpx = newpixel;

        stack.pop_back();
    }
}

```

For some preface, I am calculating the pixel with respect to the position with the **index** variable. Once I calculate it, I store it (along with the position) in the vector. After I calculate all of the pixel values, I parse them into an image buffer and later print that image.

I've narrowed the issue (I think) to where I am calculating the rendered pixel and storing it in the vector. For some reason, it does not store all of the pixels in the vector and I am not sure why? Any help is appreciated. Thanks!

**Tags:** raytracing, c++, multithreading

> **Comments:**
> **username_1**: I don't think std::vector<> is thread safe, you should be using something like a mutex when calling push_back()  
> **username_0**: Hi @username_1, Could this be part of the reason for the half-picture behavior? I got some more information about the memory, The objects are stored in memory so when I call the render function, the "hits" itself might not be registering because of the asynchronous memory accesses. But I was thinking that this would cause more random behavior, like seeing a random distribution of pixels on the picture every time.  
> **username_1**: You could remove your completion queue (future_vector) and just have your workers directly write to an RGB array, you don't need thread synchronisation for that. You will need a mechanism to detect when all workers are completed though, maybe something like an atomic counter and semaphore.  
> **username_1**: The half written image is probably caused by the main thread not waiting for workers to complete. In addition to that it is hazardous for multiple threads to access the vector<> and that could cause issues if 2 threads update the internal state of the vector<> at the same time.  
> **username_0**: I thought if I declare the vector in the async function, it makes the structure in thread memory, does it not? You are probably right, I will try and fix it.  
> **username_0**: Hey username_1, thanks for helping me out, I fixed the bug, it was dumb, I posted an answer to anyone who might look at this.  

---

## 1 answers

### Answer by username_0 on 2018-04-24. Score: 0 | Accepted answer

I fixed it, It was just a coding mistake, there was nothing wrong with the implementation. So the error was in fact in the 2nd for-loop when I was parsing the image buffer. I was reaching for the last pixel in the vector and then popping it off from the back of the stack.

This means that in the for loop condition, the vector size will be changing but k will still be incrementing the same, so it will effectively only do half of the elements per vector. Because as K is incrementing stack.size() is decrementing so they meet in the middle and the loop is done.

To fix this, I just used index notation, so:

```
rgbpacket &rgbpixel = stack[index];

```

and then I deleted the line where I was popping off from the stack.

Even though this is a stupid bug,
I hope this helps other people.


---

