Cs50 filter problem - reflect. Pixels dont change values, not sure why (C)

Question

I'm trying to implement the reflect filter by saving the left pixel of a column and replacing it by its relative right pixel. Thing is I'm not getting the result I expected and the image stays the same. Maybe its not entering the while loop but logically it seems to me that it should.

// Reflect image horizontally
void reflect(int height, int width, RGBTRIPLE image[height][width])
{
    //int n = width;
    for(int i = 0; i < height; i++)
    {
        printf("%i\n",width);
        int j = 0;
        int left = 0 + j;
        int right = width - 1 - j;
        while (j != width )
        {
            int pixRed = image[i][left].rgbtRed;
            int pixGreen = image[i][left].rgbtGreen;
            int pixBlue = image[i][left].rgbtBlue;

            image[i][left].rgbtRed = image[i][right].rgbtRed;
            image[i][left].rgbtGreen = image[i][right].rgbtGreen;
            image[i][left].rgbtBlue = image[i][right].rgbtBlue;

            image[i][right].rgbtRed = pixRed;
            image[i][right].rgbtGreen = pixGreen;
            image[i][right].rgbtBlue = pixBlue;
            j++;
            printf("%i\n",j);
        }
        return;
    }

Answer 1

You have unbalanced braces in your posted code. But, I surmise your return is [would be] in the middle of the outer loop, so you'll only do one row.

You need to increment left and decrement right. As it is, you change j at the bottom of the inner loop, but don't adjust left/right

No need to use .whatever--we can just use whole RGBTRIPLE

Your indexing logic is a bit complex.

You're doing a lot of 2D indexing. Since the array has two dynamic dimensions, this means a lot of multiplying in the inner loop.

A separate pointer to the row can simplify the code and make it run a bit faster.

---

Here's the refactored version:

// Reflect image horizontally
void
reflect(int height, int width, RGBTRIPLE image[height][width])
{
    // int n = width;
    for (int i = 0; i < height; i++) {
        RGBTRIPLE *row = &image[i][0];

        int left = 0;
        int right = width - 1;
        for (;  left < right;  ++left, --right) {
            RGBTRIPLE tmp = row[left];
            row[left] = row[right];
            row[right] = tmp;
        }
    }
}

---

We can also have left and right be pointers:

// Reflect image horizontally
void
reflect(int height, int width, RGBTRIPLE image[height][width])
{
    // int n = width;
    for (int i = 0; i < height; i++) {
        RGBTRIPLE *left = &image[i][0];
        RGBTRIPLE *right = &left[width - 1];

        for (;  left < right;  ++left, --right) {
            RGBTRIPLE tmp = *left;
            *left = *right;
            *right = tmp;
        }
    }
}

---

UPDATE:

I don't get what RGBTRIPLE *right = &left[width - 1]; means. the expression in the squared brackets is assigned as an index to &image? how does it take one index if its had 2 dimensions? – yonatan goldin 8 hours ago

This is basic access of a 1D pointer within a 2D array. This is a very important concept to master.

Consider an array of WIDTH=8, HEIGHT=6. If we laid it out on paper, the cell indexing would look like:

+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 2D  | 0,0 | 0,1 | 0,2 | 0,3 | 0,4 | 0,5 | 0,6 | 0,7 |
| 1D  |  0  |  1  |  2  |  3  |  4  |  5  |  6  |  7  |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 2D  | 1,0 | 1,1 | 1,2 | 1,3 | 1,4 | 1,5 | 1,6 | 1,7 |
| 1D  |  8  |  9  | 10  | 11  | 12  | 13  | 14  | 15  |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 2D  | 2,0 | 2,1 | 2,2 | 2,3 | 2,4 | 2,5 | 2,6 | 2,7 |
| 1D  | 16  | 17  | 18  | 19  | 20  | 21  | 22  | 23  |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 2D  | 3,0 | 3,1 | 3,2 | 3,3 | 3,4 | 3,5 | 3,6 | 3,7 |
| 1D  | 24  | 25  | 26  | 27  | 28  | 29  | 30  | 31  |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 2D  | 4,0 | 4,1 | 4,2 | 4,3 | 4,4 | 4,5 | 4,6 | 4,7 |
| 1D  | 32  | 33  | 34  | 35  | 36  | 37  | 38  | 39  |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 2D  | 5,0 | 5,1 | 5,2 | 5,3 | 5,4 | 5,5 | 5,6 | 5,7 |
| 1D  | 40  | 41  | 42  | 43  | 44  | 45  | 46  | 47  |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+

Some definitions/code:

#define WIDTH   8
#define HEIGHT  6

// 2D array:
RGBTRIPLE d2[HEIGHT][WIDTH];

// pointer to access d2 as 1D array/vector:
RGBTRIPLE *d1 = &d2[0][0];

// pointer to cell from "2D" array:
RGBTRIPLE *p2;

// pointer to cell from "1D" array:
RGBTRIPLE *p1;

Sometimes, rather than referring to row and column numbers/indexes, it can be easier to talk about y and x coordinates/indexes respectively.

To get the address of a cell from the 2D array, for a given x,y point, we want:

p2 = &d2[y][x];

To get the same address using the 1D pointer, we want:

p1 = &d1[(y * WIDTH) + x];

At this point, p2 and p1 will have the same address.

Note that while computer languages (like C) know about [and support] 2D arrays, [most] computer architectures do not. So, the compiler must take a statement like the one above for p2, convert it [internally] into something like the one for p1 and generate the code from that.

For a point [1][0], the linear index to use is: (1 * WIDTH) + 0, which is: (1 * 8) + 0 or 8.

For a point [1][WIDTH - 1], the linear index to use is: (1 * WIDTH) + (WIDTH - 1), which is (1 * 8) + (8 - 1) or 15.

So, if we had:

left = &d2[1][0];
right = &d2[1][WIDTH - 1];

Then, we would also have:

left = &d1[8];
right = &d1[15];

But, this is also true:

right = &left[15 - 8];
right = &left[7];

Here, 7 is also WIDTH - 1.

If you look at the above table, for every 2D coordinate, we have a 1D offset. Again, for [3][5], the offset is: 29

This is also: (3 * 8) + 5

I'd print out the above table on paper and use a pencil to do some calculations by hand until you understand how to traverse an array using 2D and 1D.

For example, in the following code, all the for loops do exactly the same thing to the d2 array and access each individual cell in the same order:

for (int y = 0;  y < HEIGHT;  ++y) {
    for (int x = 0;  x < WIDTH;  ++x)
        d2[y][x] += 23;
}

int end = HEIGHT * WIDTH;
for (int off = 0;  off < end;  ++off)
    d1[off] += 23;

RGBTRIPLE *p1 = &d2[0][0];

// NOTE: e1 and e2 are the same
RGBTRIPLE *e2 = &d2[HEIGHT][WIDTH];
RGBTRIPLE *e1 = &p1[HEIGHT * WIDTH];

for (;  p1 < e1;  ++p1)
    *p1 += 23;