The definitions of clamp functions are inconsistent

[ycshao21]

Hello! Thanks for your tutorials!

I noticed something a bit confusing in the image_texture::value function:

...
        // Clamp input texture coordinates to [0,1] x [1,0]
        u = interval(0,1).clamp(u);
        v = 1.0 - interval(0,1).clamp(v);  // Flip V to image coordinates

        auto i = int(u * image.width());
        auto j = int(v * image.height());
        auto pixel = image.pixel_data(i,j);
...

Since u and v are clamped to [0,1], this suggests that i and j would range from [0, image.width()] and [0, image.height()] respectively.
However, calling image.pixel_data(i, j) looks like there is a potential access violation when i == image.width() and j == image.height().

So I checked the definition of rtw_image::pixel_data:

...
        x = clamp(x, 0, image_width);
        y = clamp(y, 0, image_height);
...

x and y are also clamped here, but it seems that the access violation still exists.

Then I checked rtw_image::clamp, and I realized that the function excludes the upper bound, so it actually works fine.

My point is that the behavior of rtw_image::clamp differs from std::clamp, and it is also inconsistent with interval::clamp. For readability and maintainability, it might be better to make these clamp functions consistent.

So, I suggest adjusting rtw_image::clamp to include the upper bound:

    static int clamp(int x, int low, int high) {
        // Return the value clamped to the range [low, high].
        if (x < low) return low;
        if (x < high) return x;
        return high;  // Adjusted
    }

Then in rtw_image::pixel_data, we'd have:

    const unsigned char* pixel_data(int x, int y) const {
        // Return the address of the three RGB bytes of the pixel at x,y. If there is no image
        // data, returns magenta.
        static unsigned char magenta[] = { 255, 0, 255 };
        if (bdata == nullptr) return magenta;

        x = clamp(x, 0, image_width-1);  // Adjusted
        y = clamp(y, 0, image_height-1);  // Adjusted

        return bdata + y*bytes_per_scanline + x*bytes_per_pixel;
    }

Lastly, I want to adjust how i and j are assigned in image_texture::value.
Since it is widely accepted that the range of uv is [0, 1], so keep it as it is. However, hard clipping on the upper bound may lead to multiple sampling on the edge of the texture. In my opinion, here's the most elegant way of implementation, which gives a smoother transition:

    color value(double u, double v, const point3& p) const override {
        // If we have no texture data, then return solid cyan as a debugging aid.
        if (image.height() <= 0) return color(0,1,1);

        // Clamp input texture coordinates to [0,1] x [1,0]
        u = interval(0,1).clamp(u);
        v = 1.0 - interval(0,1).clamp(v);  // Flip V to image coordinates

        auto i = int(u * (image.width()-1));  // Adjusted
        auto j = int(v * (image.height()-1));  // Adjusted
        auto pixel = image.pixel_data(i,j);

        auto color_scale = 1.0 / 255.0;
        return color(color_scale*pixel[0], color_scale*pixel[1], color_scale*pixel[2]);
    }

Given that i and j are already valid now, the clamps in rtw_image::pixel_data can be removed. I keep it here for robustness.

It's not a huge issue, but I think making these adjustments will offer less ambiguity and a clearer code to readers.

[dimitry-ishenko]

Thank you @ycshao21.

@hollasch he's got a point with regards to the rtw_image::clamp function being inconsistent with std::clamp and interval::clamp which both include upper bound. The fix is pretty trivial:

diff --git a/books/RayTracingTheNextWeek.html b/books/RayTracingTheNextWeek.html
index deca87df..497bbde9 100644
--- a/books/RayTracingTheNextWeek.html
+++ b/books/RayTracingTheNextWeek.html
@@ -1743,8 +1743,8 @@ header into a folder called `external`. Adjust according to your directory struc
             static unsigned char magenta[] = { 255, 0, 255 };
             if (bdata == nullptr) return magenta;
 
-            x = clamp(x, 0, image_width);
-            y = clamp(y, 0, image_height);
+            x = clamp(x, 0, image_width - 1);
+            y = clamp(y, 0, image_height - 1);
 
             return bdata + y*bytes_per_scanline + x*bytes_per_pixel;
         }
@@ -1761,7 +1761,7 @@ header into a folder called `external`. Adjust according to your directory struc
             // Return the value clamped to the range [low, high).
             if (x < low) return low;
             if (x < high) return x;
-            return high - 1;
+            return high;
         }
 
         static unsigned char float_to_byte(float value) {
diff --git a/src/TheNextWeek/rtw_stb_image.h b/src/TheNextWeek/rtw_stb_image.h
index 06fa19b2..52a4bc56 100644
--- a/src/TheNextWeek/rtw_stb_image.h
+++ b/src/TheNextWeek/rtw_stb_image.h
@@ -81,8 +81,8 @@ class rtw_image {
         static unsigned char magenta[] = { 255, 0, 255 };
         if (bdata == nullptr) return magenta;
 
-        x = clamp(x, 0, image_width);
-        y = clamp(y, 0, image_height);
+        x = clamp(x, 0, image_width - 1);
+        y = clamp(y, 0, image_height - 1);
 
         return bdata + y*bytes_per_scanline + x*bytes_per_pixel;
     }
@@ -99,7 +99,7 @@ class rtw_image {
         // Return the value clamped to the range [low, high).
         if (x < low) return low;
         if (x < high) return x;
-        return high - 1;
+        return high;
     }
 
     static unsigned char float_to_byte(float value) {
diff --git a/src/TheRestOfYourLife/rtw_stb_image.h b/src/TheRestOfYourLife/rtw_stb_image.h
index 06fa19b2..52a4bc56 100644
--- a/src/TheRestOfYourLife/rtw_stb_image.h
+++ b/src/TheRestOfYourLife/rtw_stb_image.h
@@ -81,8 +81,8 @@ class rtw_image {
         static unsigned char magenta[] = { 255, 0, 255 };
         if (bdata == nullptr) return magenta;
 
-        x = clamp(x, 0, image_width);
-        y = clamp(y, 0, image_height);
+        x = clamp(x, 0, image_width - 1);
+        y = clamp(y, 0, image_height - 1);
 
         return bdata + y*bytes_per_scanline + x*bytes_per_pixel;
     }
@@ -99,7 +99,7 @@ class rtw_image {
         // Return the value clamped to the range [low, high).
         if (x < low) return low;
         if (x < high) return x;
-        return high - 1;
+        return high;
     }
 
     static unsigned char float_to_byte(float value) {

[hollasch]

Rats. Agreed on the need to standardize. I think I prefer [0,1), and don't really care that much about std::clamp(), but either way leaves pain in certain scenarios. In my full-fledged design for an interval class, I'd use C++ templating to implement tightening/expansion methods on either bound in order to tweak for open or closed ends. Discrete math helps us here, particularly for floating-point (versus real) values. Of course, all architecture astronauts have to return to earth sometime, and simplicity is our number one goal for this project.

Blah blah blah, I agree with Dimitry here.