Home > database >  Filter Reflection, cs50 - What is wrong with the code? More info regarding my question in comments
Filter Reflection, cs50 - What is wrong with the code? More info regarding my question in comments

Time:12-10

void reflect(int height, int width, RGBTRIPLE image[height][width]) {
    temp_array temp[width];
    
    int row;
    
    for (row = 0; row < height; row  )
    {
        for (int j = 0; j < width; j  )
        {
            temp[j].red = image[row][j].rgbtRed;
            temp[j].green = image[row][j].rgbtGreen;
            temp[j].blue = image[row][j].rgbtBlue;
        }
    
        for (int i = 0; i < width; i  )
        {
            image[row][i].rgbtRed = temp[width - i].red;
            image[row][i].rgbtGreen = temp[width - i].green;
            image[row][i].rgbtBlue = temp[width - i].blue;
        }
    }
return;
}

CodePudding user response:

You have an off-by-1 issue in your loop to copy the pixels. I simplyfied it a bit for demonstration purposes.

       RGBTRIPLE *current_row = image[row];
       for (int j = 0; j < width; j  )
       {
           temp[j].red = current_row[j].rgbtRed;
       }
    
       for (int i = 0; i < width; i  )
       {
           current_row[i].rgbtRed = temp[width - i].red;
       }

You copy row[0] into temp[0]. Same for all elements up to width-1;

Then you copy temp[width-0] into row[0]. But your array temp does not contain an element width as it only has elements 0 .. width-1. The pixel should be taken from row[width - 1 - i] instead. That will avoid shifting the pixels by 1 position during copying. It will also avoid accessing your array beyond its maximum limit which causes undefined behaviour.

Besides that your code is rather inefficient. You are consuming way too much memory. Your task is to swap pixels. For swapping something you should not need more than 1 element in most cases. Not an arra to store a whole row.

Also don't invent new types for things that already exist. You swap pixels of type RGBTRIPLE. Use that type. Don't add another type to re-invent the wheel.

For swapping a pixel just use this:

           RGBTRIPLE temp = image[row][j];
           image[row][j] = image[row][width-1-j];
           image[row][width-1-j] = temp;

Then remove all the unused types, arrays and loops.

  • Related