draw_if_possible Template Function Implementation for Image in C++

Question

This is a follow-up question for Midpoint Circle Algorithm Implementation for Image in C++, draw_circle Template Function Implementation for Image in C++, SIFT Keypoint Detection for Image in C++, difference_of_gaussian Template Function Implementation for Image in C++, conv2 Template Function Implementation for Image in C++ and imgaussfilt Template Function Implementation for Image in C++. To fix the issue mentioned in Rainer P.'s answer, I am trying to modify draw_circle template function and implement draw_if_possible template function in this post.

The experimental implementation

• draw_if_possible template function implementation (in file image_operations.h)

  namespace TinyDIP
  {
   // draw_if_possible template function implementation
   template<typename ElementT>
   constexpr static void draw_if_possible(
   Image<ElementT>& input,
   ElementT draw_value,
   std::ptrdiff_t x,
   std::ptrdiff_t y
   )
   {
   if (x < 0 || x >= input.getWidth() ||
   y < 0 || y >= input.getHeight())
   {
   return;
   }
   input.at_without_boundary_check(x, y) = draw_value;
   return;
   }
  }
  

• draw_circle template function implementation (in file image_operations.h)

  namespace TinyDIP
  {
   // draw_circle template function implementation
   // https://codereview.stackexchange.com/q/293417/231235
   // Test: https://godbolt.org/z/7zKfhG3x9
   template<typename ElementT>
   constexpr static auto draw_circle(
   const Image<ElementT>& input,
   std::tuple<std::size_t, std::size_t> central_point,
   std::size_t radius = 2,
   ElementT draw_value = ElementT{}
   )
   {
   if (input.getDimensionality() != 2)
   {
   throw std::runtime_error("Unsupported dimension!");
   }
   auto point_x = std::get<0>(central_point);
   auto point_y = std::get<1>(central_point);
   auto output = input;
   auto height = input.getHeight();
   auto width = input.getWidth();
   if (radius <= 0)
   {
   // early out avoids y going negative in loop
   return output;
   }
   for (std::ptrdiff_t x = 0, y = radius; x <= y; x++)
   {
   // try to decrement y, then accept or revert
   y--;
   if (x * x + y * y < radius * radius)
   {
   y++;
   }
   // do nothing if out of bounds, otherwise draw
   draw_if_possible(output, draw_value, point_x + x, point_y + y);
   draw_if_possible(output, draw_value, point_x - x, point_y + y);
   draw_if_possible(output, draw_value, point_x + x, point_y - y);
   draw_if_possible(output, draw_value, point_x - x, point_y - y);
   draw_if_possible(output, draw_value, point_x + y, point_y + x);
   draw_if_possible(output, draw_value, point_x - y, point_y + x);
   draw_if_possible(output, draw_value, point_x + y, point_y - x);
   draw_if_possible(output, draw_value, point_x - y, point_y - x);
   }
   return output;
   }
  }
  

The example output:

OutputImage

The usage of draw_circle template function:

int main()
{
 auto start = std::chrono::system_clock::now();
 // Read image file
 std::string file_path = "InputImages/1";
 auto bmp1 = TinyDIP::bmp_read(file_path.c_str(), false);
 // Ascend image size
 bmp1 = copyResizeBicubic(bmp1, bmp1.getWidth() * 2, bmp1.getHeight() * 2);
 // Get value plane
 auto v_plane = TinyDIP::getVplane(TinyDIP::rgb2hsv(bmp1));
 // Get SIFT keypoints
 auto SIFT_keypoints = TinyDIP::SIFT_impl::get_potential_keypoint(v_plane);
 std::cout << "SIFT_keypoints = " << SIFT_keypoints.size() << "\n";
 // Draw SIFT keypoints
 bmp1 = TinyDIP::draw_points(bmp1, SIFT_keypoints);
 for (auto&& each_SIFT_keypoint : SIFT_keypoints)
 {
 auto orientation_histogram = TinyDIP::SIFT_impl::get_orientation_histogram(v_plane, each_SIFT_keypoint);
 RGB rgb{ 255, 255, 255 };
 bmp1 = TinyDIP::draw_circle(bmp1, each_SIFT_keypoint, TinyDIP::recursive_max(orientation_histogram), rgb);
 }
 // Save file
 TinyDIP::bmp_write("test20240829", bmp1);
 auto end = std::chrono::system_clock::now();
 std::chrono::duration<double> elapsed_seconds = end - start;
 std::time_t end_time = std::chrono::system_clock::to_time_t(end);
 if (elapsed_seconds.count() != 1)
 {
 std::cout << "Computation finished at " << std::ctime(&end_time) << "elapsed time: " << elapsed_seconds.count() << "seconds.\n";
 }
 else
 {
 std::cout << "Computation finished at " << std::ctime(&end_time) << "elapsed time: " << elapsed_seconds.count() << "second.\n";
 }
 
 return EXIT_SUCCESS;
}

TinyDIP on GitHub

All suggestions are welcome.

The summary information:

• Which question it is a follow-up to?

  Midpoint Circle Algorithm Implementation for Image in C++,

  draw_circle Template Function Implementation for Image in C++,

  SIFT Keypoint Detection for Image in C++,

  difference_of_gaussian Template Function Implementation for Image in C++,

  conv2 Template Function Implementation for Image in C++ and

  imgaussfilt Template Function Implementation for Image in C++

• What changes has been made in the code since last question?

  I am trying to modify draw_circle template function and implement draw_if_possible template function in this post.

• Why a new review is being asked for?

  Please review the implementation of draw_if_possible and draw_circle template functions and its tests.

Answer 1

Why not make this a member function of Image?

Instead of having to write:

draw_if_possible(output, draw_value, point_x + x, point_y + y);

Consider making it possible (pun not intended) to write:

output.set(point_x + x, point_y + y, draw_value);

Not only is it less typing for the caller, it also avoids some annoying problems with type deduction. Consider writing:

TinyDIP::Image<float> image(1, 1);
TinyDIP::draw_if_possible(image, 42, 0, 0); // compile error!
TinyDIP::draw_if_possible(image, 3.1415, 0, 0); // compile error!

If it was a member function, Image::set() wouldn't be a template, and there would be no problem deducing the type of draw_value.

What about images that are not 2D?

Your TinyDIP::Image supports any number of dimensions, but draw_if_possible() only works for two-dimensional images. In fact, due to a total lack of checking in at_without_boundary_check(), it might crash if you try to call it on one-dimensional images.

Make sure you handle an arbitrary amount of coordinates. Perhaps even better would be to make the number of dimensions a template parameter of TinyDIP::Image, so it can be checked at compile time.

Use std::size_t for the coordinates

I get why you used std::ptrdiff_t for x and y, but it is inconsistent with the way you pass coordinates to any member function of Image. What if one dimension of the image is larger than can be stored in a std::ptrdiff_t?

The drawback of using std::size_t is that you might need to explicitly cast the std::ptrdiff_ts used in draw_circle() to std::size_t, which is annoying. Another solution would be to make the coordinate types template parameters, and then use std::cmp_less() and std::cmp_greater_equal() to compare them against the size of the image:

template<typename ElementT, typename... CoordinateTs>
constexpr static void draw_if_possible(
 Image<ElementT>& input,
 ElementT draw_value,
 CoordinateTs coordinates...)
)
{
 if ((std::cmp_less(coordinates, 0) || ...) ||
 (std::cmp_greater_equal(coordinates, /* something */) || ...))
 {
 return;
 }
 input.at_without_boundary_check(coordinates...) = draw_value;
}

It's not optimal for circle drawing

With your code you are checking every pixel you draw against the image size. However, you can be more efficient by doing the checks in draw_circle() itself. For example, you can already quickly check if the circle lies completely inside or completely outside the image. In the former case you can draw without per-pixel checks, in the latter case you can skip drawing any pixel. But even for circles that are only partially inside the image there are various ways you can do better.

• Rainer P.: Accepting computed coordinates (which may be negative or out of bounds) is the main feature here. The suggestion to use unsigned size_t and shifting the bounds check back to the caller defeats the purpose of this function. I agree that it should be a member function and work for non-2D images.
• G. Sliepen: I'm not suggesting shifting the bounds check back to the caller. That can still be done in draw_if_possible() on the std::size_ts. Thanks to two's complement, it doesn't matter if you do the checks on a ptrdiff_t or on that same value cast to a size_t.
• Rainer P.: The problem with unsigned arguments is that negative coordinates (which exist, even though they are out of bounds) wrap to large positive coordinates and mess up the callee-side bounds check. If the callee always assumes that large positive numbers are actually wrapped negative numbers, you should pass signed arguments in the first place.
• G. Sliepen: I would agree if ptrdiff_t was any better than size_t at handling wrapped numbers, but it's not. It's equally possible to wrap ptrdiff_ts if the circle's center_point is close to the minimum or maximum value of ptrdiff_t, or if the radius, which is a size_t by the way, is close to or large than ptrdiff_t's maximum value. In fact, all the issues with wrapping happen at the same coordinates, regardless of whether ptrdiff_t or size_t is used, thanks to two's complement.
• G. Sliepen: @JimmyHu Yes. Although I think having coordinates passed as a single value would be even better, then you can do output.set({point_x + x, point_y + y}, draw_value).