Possible problem with operator overloading in c++

[colt]

Hello I have this line of code:

specular_color += light_intensity * std::pow (std::max(0.f,reflected*camera_dir),mat.ns);

that is generating serious wrong results (I know by the color)

however if I change it to:

specular_color = light_intensity * std::pow (std::max(0.f,reflected*camera_dir),mat.ns);

the main issue goes away. (removed the '+' )

where specular_color and light_intensity are custom structs of the kind "Color" with consists of three float and a bunch of operator overload.

Since pow returns float, the operator overloading that are used by the above line are:

inline Color& operator += (const Color& c) {
         //cout << "c: " << c.red << ' ' << c.green << ' ' << c.blue << endl;
         //cout << "this: " << this->red << ' ' << this->green << ' ' << this->blue << endl;
         this->red = this->red + c.red;
         this->green = this->green + c.green;
         this->blue = this->blue + c.blue;
         return *this;
      }

inline Color operator * (const Color& c, float a) {
   Color color;
   color.red = c.red * a;
   color.green = c.green * a;
   color.blue = c.blue * a;
   return color;
}

So my question is: Is there something wrong with any of these operator overloading? If not, where might it be the cause (fell free to ask for more code)? I know by fact that the main issue that I have is caused by the + operator, since if I remove it, the problem vanishes.

Thanks for any suggestions.

[ne555]

you have different results because the operations are different

a += b; //equivalent to a = a+b
a = b;

[jlb]

Have you tried breaking down that equation down to the individual equations?

Ie:

value = reflected * camera_dir;
value2 = std::max((0.f, value);
value3 = std::pow(value2, mat.ns);
value4 = light_intensity * value3;

Make sure all the "value" are the correct type for the equation and verify each "value" is correct.

Also you should be able to verify your overloads with a simple test program, but the += appears correct at a quick glance.

[Duthomhas]

None of the technical details of the code are wrong.

Using += is wrong. Take another look at your raytracing how-to book.

Good luck!

[colt]

Thanks for the answers. Duthomas, are you sure about using "+=" ? I ask it because my "raytracing how-to book." uses it: https://www.scratchapixel.com/code.php?id=32&origin=/lessons/3d-basic-rendering/phong-shader-BRDF (line 565). To my understanding, it makes sense. I have to accumulate the contributions of several light sources, so += seems to make sense. On the diffuse light is also += and in that case it does work as intended.

[keskiverto]

That book code calls clamp() when writing the colours to a file. Do you?

[ne555]

notice that if you go overflow it's too late to clamp.

[colt]

I limit the color values with:

a [ctr] =  min (final_color.blue*255.0f,255.0f);
a [ctr+1] = min (final_color.green*255.0f,255.0f);
a [ctr+2] = min (final_color.red*255.0f,255.0f);

but I guess it is already too late. Not sure I would recognize if the value overflowed while still being calculated as a float. How I would know what is an acceptable value? I mean, I am currently printing the values of two different pixels on the terminal's screen for two different images.

For the first test case, to swap "+=" by "= "does not produce any change for one pixel, for the other the value goes from 0 to 22 (for all the colors). In the second, the specular component goes from 1.6 to 0 for the first pixel. However, in the second, it goes from about 1500 to 0! I realize that being 0 is likely correct, since the specular only affects a few pixels

Also, still intrigued by Duthomas comment about += being wrong.

Anyway link to two test images with += and only with +. Still different issues with "+", although obviously smaller: https://postimg.cc/gallery/di05ynmw/

[ne555]

¿could you provide a minimal testcase that does reproduce your issue?

[jonnin]

are you working in float for byte element colors just because of pow?
if so, your code will be faster and give better results if you stick to bytes and write yourself an integer power routine. If not, carry on.

[Duthomhas]

Sorry to have been gone so long.

The final color is, yes, a composite of the contributing colors.

Be careful how you combine them. Right now you are treating them as equal contributors and simply adding their full values, which is blowing the RGB bounds and manifesting as weird colors.

Hope this helps.

[colt]

could you provide a minimal testcase that does reproduce your issue?

Complicated, I think it requires so many things that a striped up version would require so many effort and would still be huge for a testcase. I could without so many effort create a makefile and update it to my gitlab to make it easy to compile it. Plus to add the current problematic version

are you working in float for byte element colors just because of pow?
if so, your code will be faster and give better results if you stick to bytes and write yourself an integer power routine. If not, carry on.

Not sure if I understood, but I want working with float because most variables that take part of the color calculations (diffuse and specular) are float. So, as far as I can imagine, int would result in wrongs values, hence colors that would be incorrect.

Be careful how you combine them. Right now you are treating them as equal contributors and simply adding their full values, which is blowing the RGB bounds and manifesting as weird colors.

How could I fix it?

Anyway, thanks for the answers so far.