Truncation when ppm format exceeded upper limit

[tatsuya-ogawa]

Since "int(255.99*col[xx])" can be over 256 in windows environment,
so add std::min for truncation that.

[hollasch]

It looks like this change needs to be made to the most recent version of the source code, as GitHub is unable to merge this change.

Secondly, I think there's a better way to express this. Here, we're mapping (actually, clamping) the range [0,1) to the range [0,255]. The current expression mixes mapping, clamping, and type conversion in a single expression, where I think well-defined steps would work better.

1. Clamp the input value to [0,1]. This addresses problematic real values that exceed the range of int (like, say, an intensity of 1e207), and also negative values (which can happen in some situations, particularly if you're using fun light sources like darkbulbs). This also handles legal (and possible) values like ±infinity.
2. Scale the reals proportionally as [0,1] → [0,256]. This is just a linear scaling by 256.
3. Convert the real-valued result to integer.
4. Perform one final clamp to a max of 255, to handle the boundary case of an input value of 256.0, where all positive out-of-gamut input intensities would fall.

Oh, I also realized a tiny bug that with the existing code, an input value of 255.009/256.0 will map to an integer value of 254, as the upper band is compressed more than the other bands due to the multiplication by 255.99.

I think it makes sense to create a utility function for this, like so:

int intensity (double x)
{
    return (x <= 0.0) ? 0
         : (x >= 1.0) ? 255
         : std::min(int(256.0 * x), 255);
}

Then the output block would change from this:

int ir = int(255.99*col[0]);
int ig = int(255.99*col[1]);
int ib = int(255.99*col[2]);
std::cout << ir << " " << ig << " " << ib << "\n";

to this:

std::cout << intensity(col[0]) << " "
          << intensity(col[1]) << " "
          << intensity(col[2]) << "\n";

Now ideally, it would be nice to have a distinct color class, with a dedicated method for output, but I think that strays from the down-and-dirty nature of the current codebase. The above intensity() function should suffice, make the intent more clear, and yields superior results. It should likely be a utility function defined in material.h, or could just be defined as a function local to main.cc.

[trevordblack]

Something to chew on:
https://stackoverflow.com/questions/31225264/what-is-the-result-of-comparing-a-number-with-nan

If x happens down the pipe and is NaN, any boolean comparison that isn't != will return false
then

int intensity (double x)
{
    return (x <= 0.0) ? 0
         : (x >= 1.0) ? 255
         : std::min(int(256.0 * x), 255);
}

truncates down to

int intensity (double x)
{
    return std::min(int(256.0 * x), 255);
}

for NaNs

Whereas we may want to convert NaNs into 0s.

int intensity (double x)
{
    if ( x > 0.0 )
        return (x >= 1.0) ? 255 : std::min(int(256.0 * x), 255)
    else
        return 0;
}

Doing this implicitly could be a big problem for learning, though.

[hollasch]

Interesting idea about handling NaNs. One general approach to this is to convert colors with NaN components to a specific, uncommon signature color (often pink or cyan). Black is problematic, as this is a common color, and easy to miss.

However, such a strategy is more appropriate at the color level, rather than component intensities. That points back to a single color-to-string function. Something like this:

#include <cmath>

std::ostream& printColor (std::ostream& out, const vec3& v) {
    if (std::isnan(v[0]) || std::isnan(v[1]) || std::isnan(v[2]))
        return out << "0 255 255\n";

    return out << intensity(v[0]) << " " << intensity(v[1]) << " " << intensity(v[2]);
}

This can be pretty handy when debugging.

[hollasch]

See de_nan function, new chapter 11 (old chapter 10). This should be integrated into the fix.

[hollasch]

Putting this on hold until the web release, then we'll proceed with the solutions outlined above.