1
\$\begingroup\$

This is a follow-up question for draw_if_possible Template Function Implementation for Image in C++. In G. Sliepen's answer, set member function has been mentioned. I am trying to add set member function implementation into the Image class. The first parameter of set function is the location encapsulated in std::tuple structure. The second parameter is the element value to be set in the given location. The return value represents set operation run successfully or not. What do you think about using std::tuple for location presentation?

The experimental implementation

  • set member function implementation (in file image.h)

    //  set template function implementation
    template<class TupleT>
    requires(is_tuple<TupleT>::value)
    constexpr bool set(const TupleT location, const ElementT draw_value)
    {
        if (checkBoundaryTuple(location))
        {
            image_data[tuple_location_to_index(location)] = draw_value;
            return true;
        }
        return false;
    }
    
  • is_tuple struct implementation (in file basic_functions.h)

    //  Reference: https://stackoverflow.com/a/48458312/6667035
    template <typename>
    struct is_tuple : std::false_type {};
    
    template <typename ...T>
    struct is_tuple<std::tuple<T...>> : std::true_type {};
    
  • The full Image class implementation

    namespace TinyDIP
    {
        template <typename ElementT>
        class Image
        {
        public:
            Image() = default;
    
            template<std::same_as<std::size_t>... Sizes>
            Image(Sizes... sizes): size{sizes...}, image_data((1 * ... * sizes))
            {}
    
            template<std::same_as<int>... Sizes>
            Image(Sizes... sizes)
            {
                size.reserve(sizeof...(sizes));
                (size.emplace_back(sizes), ...);
                image_data.resize(
                    std::reduce(
                        std::ranges::cbegin(size),
                        std::ranges::cend(size),
                        std::size_t{1},
                        std::multiplies<>()
                        )
                );
            }
    
            Image(const std::vector<std::size_t>& sizes)
            {
                size = std::move(sizes);
                image_data.resize(
                    std::reduce(
                        std::ranges::cbegin(sizes),
                        std::ranges::cend(sizes),
                        std::size_t{1},
                        std::multiplies<>()
                        ));
            }
    
            template<std::ranges::input_range Range,
                     std::same_as<std::size_t>... Sizes>
            Image(const Range& input, Sizes... sizes):
                size{sizes...}, image_data(begin(input), end(input))
            {
                if (image_data.size() != (1 * ... * sizes)) {
                    throw std::runtime_error("Image data input and the given size are mismatched!");
                }
            }
    
            template<std::same_as<std::size_t>... Sizes>
            Image(std::vector<ElementT>&& input, Sizes... sizes):
                size{sizes...}, image_data(begin(input), end(input))
            {
                if (image_data.size() != (1 * ... * sizes)) {
                    throw std::runtime_error("Image data input and the given size are mismatched!");
                }
            }
    
            Image(const std::vector<ElementT>& input, const std::vector<std::size_t>& sizes)
            {
                size = std::move(sizes);
                image_data = std::move(input);
                auto count = std::reduce(std::ranges::cbegin(sizes), std::ranges::cend(sizes), 1, std::multiplies());
                if (image_data.size() != count) {
                    throw std::runtime_error("Image data input and the given size are mismatched!");
                }
            }
    
    
            Image(const std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight)
            {
                size.reserve(2);
                size.emplace_back(newWidth);
                size.emplace_back(newHeight);
                if (input.size() != newWidth * newHeight)
                {
                    throw std::runtime_error("Image data input and the given size are mismatched!");
                }
                image_data = std::move(input);              //  Reference: https://stackoverflow.com/a/51706522/6667035
            }
    
            Image(const std::vector<std::vector<ElementT>>& input)
            {
                size.reserve(2);
                size.emplace_back(input[0].size());
                size.emplace_back(input.size());
                for (auto& rows : input)
                {
                    image_data.insert(image_data.end(), std::ranges::begin(input), std::ranges::end(input));    //  flatten
                }
                return;
            }
    
            //  at template function implementation
            template<typename... Args>
            constexpr ElementT& at(const Args... indexInput)
            {
                return const_cast<ElementT&>(static_cast<const Image &>(*this).at(indexInput...));
            }
    
            //  at template function implementation
            //  Reference: https://codereview.stackexchange.com/a/288736/231235
            template<typename... Args>
            constexpr ElementT const& at(const Args... indexInput) const
            {
                checkBoundary(indexInput...);
                constexpr std::size_t n = sizeof...(Args);
                if(n != size.size())
                {
                    throw std::runtime_error("Dimensionality mismatched!");
                }
                std::size_t i = 0;
                std::size_t stride = 1;
                std::size_t position = 0;
    
                auto update_position = [&](auto index) {
                    position += index * stride;
                    stride *= size[i++];
                };
                (update_position(indexInput), ...);
    
                return image_data[position];
            }
    
            //  at_without_boundary_check template function implementation
            template<typename... Args>
            constexpr ElementT& at_without_boundary_check(const Args... indexInput)
            {
                return const_cast<ElementT&>(static_cast<const Image &>(*this).at_without_boundary_check(indexInput...));
            }
    
            template<typename... Args>
            constexpr ElementT const& at_without_boundary_check(const Args... indexInput) const
            {
                std::size_t i = 0;
                std::size_t stride = 1;
                std::size_t position = 0;
    
                auto update_position = [&](auto index) {
                    position += index * stride;
                    stride *= size[i++];
                };
                (update_position(indexInput), ...);
    
                return image_data[position];
            }
    
            //  get function implementation
            constexpr ElementT get(std::size_t index) const noexcept
            {
                return image_data[index];
            }
    
            //  set function implementation
            constexpr ElementT& set(const std::size_t index) noexcept
            {
                return image_data[index];
            }
    
            //  set template function implementation
            template<class TupleT>
            requires(is_tuple<TupleT>::value)
            constexpr bool set(const TupleT location, const ElementT draw_value)
            {
                if (checkBoundaryTuple(location))
                {
                    image_data[tuple_location_to_index(location)] = draw_value;
                    return true;
                }
                return false;
            }
    
            //  cast template function implementation
            template<typename TargetT>
            constexpr Image<TargetT> cast()
            {
                std::vector<TargetT> output_data;
                output_data.resize(image_data.size());
                std::transform(
                    std::ranges::cbegin(image_data),
                    std::ranges::cend(image_data),
                    std::ranges::begin(output_data),
                    [](auto& input){ return static_cast<TargetT>(input); }
                    );
                Image<TargetT> output(output_data, size);
                return output;
            }
    
            constexpr std::size_t count() const noexcept
            {
                return std::reduce(std::ranges::cbegin(size), std::ranges::cend(size), 1, std::multiplies());
            }
    
            constexpr std::size_t getDimensionality() const noexcept
            {
                return size.size();
            }
    
            constexpr std::size_t getWidth() const noexcept
            {
                return size[0];
            }
    
            constexpr std::size_t getHeight() const noexcept
            {
                return size[1];
            }
    
            //  getSize function implementation
            constexpr auto getSize() const noexcept
            {
                return size;
            }
    
            //  getSize function implementation
            constexpr auto getSize(std::size_t index) const noexcept
            {
                return size[index];
            }
    
            //  getStride function implementation
            constexpr std::size_t getStride(std::size_t index) const noexcept
            {
                if(index == 0)
                {
                    return std::size_t{1};
                }
                std::size_t output = std::size_t{1};
                for(std::size_t i = 0; i < index; ++i)
                {
                    output *= size[i];
                }
                return output;
            }
    
            std::vector<ElementT> const& getImageData() const noexcept { return image_data; }      //  expose the internal data
    
            //  print function implementation
            void print(std::string separator = "\t", std::ostream& os = std::cout) const
            {
                if(size.size() == 1)
                {
                    for(std::size_t x = 0; x < size[0]; ++x)
                    {
                        //  Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
                        os << +at(x) << separator;
                    }
                    os << "\n";
                }
                else if(size.size() == 2)
                {
                    for (std::size_t y = 0; y < size[1]; ++y)
                    {
                        for (std::size_t x = 0; x < size[0]; ++x)
                        {
                            //  Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
                            os << +at(x, y) << separator;
                        }
                        os << "\n";
                    }
                    os << "\n";
                }
                else if (size.size() == 3)
                {
                    for(std::size_t z = 0; z < size[2]; ++z)
                    {
                        for (std::size_t y = 0; y < size[1]; ++y)
                        {
                            for (std::size_t x = 0; x < size[0]; ++x)
                            {
                                //  Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
                                os << +at(x, y, z) << separator;
                            }
                            os << "\n";
                        }
                        os << "\n";
                    }
                    os << "\n";
                }
                else if (size.size() == 4)
                {
                    for(std::size_t a = 0; a < size[3]; ++a)
                    {
                        os << "group = " << a << "\n";
                        for(std::size_t z = 0; z < size[2]; ++z)
                        {
                            for (std::size_t y = 0; y < size[1]; ++y)
                            {
                                for (std::size_t x = 0; x < size[0]; ++x)
                                {
                                    //  Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
                                    os << +at(x, y, z, a) << separator;
                                }
                                os << "\n";
                            }
                            os << "\n";
                        }
                        os << "\n";
                    }
                    os << "\n";
                }
            }
    
            //  Enable this function if ElementT = RGB
            void print(std::string separator = "\t", std::ostream& os = std::cout) const
            requires(std::same_as<ElementT, RGB>)
            {
                for (std::size_t y = 0; y < size[1]; ++y)
                {
                    for (std::size_t x = 0; x < size[0]; ++x)
                    {
                        os << "( ";
                        for (std::size_t channel_index = 0; channel_index < 3; ++channel_index)
                        {
                            //  Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
                            os << +at(x, y).channels[channel_index] << separator;
                        }
                        os << ")" << separator;
                    }
                    os << "\n";
                }
                os << "\n";
                return;
            }
    
            Image<ElementT>& setAllValue(const ElementT input)
            {
                std::fill(std::ranges::begin(image_data), std::ranges::end(image_data), input);
                return *this;
            }
    
            friend std::ostream& operator<<(std::ostream& os, const Image<ElementT>& rhs)
            {
                const std::string separator = "\t";
                rhs.print(separator, os);
                return os;
            }
    
            Image<ElementT>& operator+=(const Image<ElementT>& rhs)
            {
                check_size_same(rhs, *this);
                std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
                       std::ranges::begin(image_data), std::plus<>{});
                return *this;
            }
    
            Image<ElementT>& operator-=(const Image<ElementT>& rhs)
            {
                check_size_same(rhs, *this);
                std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
                       std::ranges::begin(image_data), std::minus<>{});
                return *this;
            }
    
            Image<ElementT>& operator*=(const Image<ElementT>& rhs)
            {
                check_size_same(rhs, *this);
                std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
                       std::ranges::begin(image_data), std::multiplies<>{});
                return *this;
            }
    
            Image<ElementT>& operator/=(const Image<ElementT>& rhs)
            {
                check_size_same(rhs, *this);
                std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
                       std::ranges::begin(image_data), std::divides<>{});
                return *this;
            }
    
            friend bool operator==(Image<ElementT> const&, Image<ElementT> const&) = default;
    
            friend bool operator!=(Image<ElementT> const&, Image<ElementT> const&) = default;
    
            friend Image<ElementT> operator+(Image<ElementT> input1, const Image<ElementT>& input2)
            {
                return input1 += input2;
            }
    
            friend Image<ElementT> operator-(Image<ElementT> input1, const Image<ElementT>& input2)
            {
                return input1 -= input2;
            }
    
            friend Image<ElementT> operator*(Image<ElementT> input1, ElementT input2)
            {
                return multiplies(input1, input2);
            }
    
            friend Image<ElementT> operator*(ElementT input1, Image<ElementT> input2)
            {
                return multiplies(input2, input1);
            }
    
    #ifdef USE_BOOST_SERIALIZATION
    
            void Save(std::string filename)
            {
                const std::string filename_with_extension = filename + ".dat";
                //    Reference: https://stackoverflow.com/questions/523872/how-do-you-serialize-an-object-in-c
                std::ofstream ofs(filename_with_extension, std::ios::binary);
                boost::archive::binary_oarchive ArchiveOut(ofs);
                //    write class instance to archive
                ArchiveOut << *this;
                //    archive and stream closed when destructors are called
                ofs.close();
            }
    
    #endif
        private:
            std::vector<std::size_t> size;
            std::vector<ElementT> image_data;
    
            template<typename... Args>
            void checkBoundary(const Args... indexInput) const
            {
                constexpr std::size_t n = sizeof...(Args);
                if(n != size.size())
                {
                    throw std::runtime_error("Dimensionality mismatched!");
                }
                std::size_t parameter_pack_index = 0;
                auto function = [&](auto index) {
                    if (index >= size[parameter_pack_index])
                        throw std::out_of_range("Given index out of range!");
                    parameter_pack_index = parameter_pack_index + 1;
                };
    
                (function(indexInput), ...);
            }
    
            //  checkBoundaryTuple template function implementation
            template<class TupleT>
            requires(is_tuple<TupleT>::value)
            constexpr bool checkBoundaryTuple(const TupleT location)
            {
                constexpr std::size_t n = std::tuple_size<TupleT>{};
                if(n != size.size())
                {
                    throw std::runtime_error("Dimensionality mismatched!");
                }
                std::size_t parameter_pack_index = 0;
                auto function = [&](auto index) {
                    if (std::cmp_greater_equal(index, size[parameter_pack_index]))
                        return false;
                    parameter_pack_index = parameter_pack_index + 1;
                    return true;
                };
                return std::apply([&](auto&&... args) { return ((function(args))&& ...);}, location);
            }
    
            //  tuple_location_to_index template function implementation
            template<class TupleT>
            requires(is_tuple<TupleT>::value)
            constexpr std::size_t tuple_location_to_index(TupleT location)
            {
                std::size_t i = 0;
                std::size_t stride = 1;
                std::size_t position = 0;
                auto update_position = [&](auto index) {
                        position += index * stride;
                        stride *= size[i++];
                    };
                std::apply([&](auto&&... args) {((update_position(args)), ...);}, location);
                return position;
            }
    
    #ifdef USE_BOOST_SERIALIZATION
            friend class boost::serialization::access;
            template<class Archive>
            void serialize(Archive& ar, const unsigned int version)
            {
                ar& size;
                ar& image_data;
            }
    #endif
    
        };
    
        template<typename T, typename ElementT>
        concept is_Image = std::is_same_v<T, Image<ElementT>>;
    }
    

The usage of set member function:

//  draw_circle template function implementation
//  https://codereview.stackexchange.com/q/293417/231235
//  Test: https://godbolt.org/z/7zKfhG3x9
//  Test with output.set: https://godbolt.org/z/1GYdrbs5q
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
        output.set(std::make_tuple(point_x + x, point_y + y), draw_value);
        output.set(std::make_tuple(point_x - x, point_y + y), draw_value);
        output.set(std::make_tuple(point_x + x, point_y - y), draw_value);
        output.set(std::make_tuple(point_x - x, point_y - y), draw_value);
        output.set(std::make_tuple(point_x + y, point_y + x), draw_value);
        output.set(std::make_tuple(point_x - y, point_y + x), draw_value);
        output.set(std::make_tuple(point_x + y, point_y - x), draw_value);
        output.set(std::make_tuple(point_x - y, point_y - x), draw_value);
    }
    return output;
}

TinyDIP on GitHub

All suggestions are welcome.

The summary information:

  • Which question it is a follow-up to?

    draw_if_possible Template Function Implementation for Image in C++

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

    I am trying to add set member function implementation into the Image class.

  • Why a new review is being asked for?

    Please review the implementation of set member function.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$
    template<class TupleT>
    requires(is_tuple<TupleT>::value)
    constexpr bool set(const TupleT location, const ElementT draw_value)

Check to ensure the tuple elements are integral types. Your class allows construction from either std::size_t or int extent types.

You could just templatize the tuple parameters IndexTypes and have location be a templated specialization std::tuple<IndexTypes...> to take any std::tuple.

Do you just want to support std::tuple? How about any tuple-like type? C++23 standardizes std::tuple, std::pair, std::array, std::ranges::subrange) as tuple-likes. C++26 adds std::complex. Perhaps you only want the tuple-like list types (no std::complex)?


    image_data[tuple_location_to_index(location)] = draw_value;

You created a helper that maps a location (tuple) to a 1-dimensional index.

    //  tuple_location_to_index template function implementation
    template<class TupleT>
    requires(is_tuple<TupleT>::value)
    constexpr std::size_t tuple_location_to_index(TupleT location)
    {
        std::size_t i = 0;
        std::size_t stride = 1;
        std::size_t position = 0;
        auto update_position = [&](auto index) {
            position += index * stride;
            stride *= size[i++];
            };

This same calculation is used in at and at_without_boundary_check.

        std::size_t i = 0;
        std::size_t stride = 1;
        std::size_t position = 0;

        auto update_position = [&](auto index) {
            position += index * stride;
            stride *= size[i++];
        };

These can all be refactored into a single function that just takes a variadic list of indices. Then all the call sites can use it.

You are already aware of boost::multi_array. There is currently work on a multidimensional owning mdarray for C++26. There is some overlap with the underlying multimensional access work you are doing here to work with either std::array, std::vector or any other underlying container. It's being built using the same utilities used by the C++23 std::mdspan, which uses std::extents/std::dextents and LayoutMappingPolicy types (std::layout_[left|right|stride]).


    //  get function implementation
    constexpr ElementT get(std::size_t index) const noexcept
    {
        return image_data[index];
    }

    //  set function implementation
    constexpr ElementT& set(const std::size_t index) noexcept
    {
        return image_data[index];
    }

With a set function that takes a multidimensional index and a value to assign, you should re-evaluate your existing interface. Does this set actually assign any value? Not really. There is no bounds checking and the return type is a reference instead of a bool. set(index) is really a non-const get(index).

Should image provide unmapped/direct index access to the underlying container?


Project-Wide

  • Be consistent with your header guards.

      #ifndef TinyDIP_Image_H  // image.h header guard
      #ifndef BASE_H           // base_types.h header guard
    

    Pick a convention and stick to it. A common convention uses UPPERCASE for preprocessor identifiers with an identifier structure of PROJECTNAME_FILEPATH_FILENAME_EXTENSION.

      #ifndef TINYDIP_IMAGE_H       // image.h header guard
      #ifndef TINYDIP_BASE_TYPES_H  // base_types.h header guard
    
  • Clean up your includes. Some aren't used. Some are missing.

      #include <algorithm>    // std::fill
      #include <concepts>     // std::same_as
      #include <cstddef>      // std::size_t
      #include <functional>   // std::multiplies
      #include <iostream>     // std::ostream
      #include <iterator>     // std::range::cbegin
      #include <numeric>      // std::reduce
      #include <stdexcept>    // std::runtime_error
      #include <string>       // std::string
      #include <tuple>        // std::apply
      #include <type_traits>  // std::is_same_v
      #include <vector>       // std::vector
    

    If you didn't use std::cout as a default output stream, you could replace <iostream> with <ostream>. Depending on the implementation, including <iostream> carries a static initialization cost for std::cout (and friends) in every translation unit.

  • Members that accept index types of int should be checked to ensure they are not negative. Similarly, should size types of either int or std::size_t allow a dimension size of 0?

  • Make sure you validate or document your preconditions.

      Image(const std::vector<std::vector<ElementT>>& input)
      {
          size.reserve(2);
          size.emplace_back(input[0].size());
          size.emplace_back(input.size());
          for (auto& rows : input)
          {
              image_data.insert(image_data.end(), std::ranges::begin(input), std::ranges::end(input));    //  flatten
          }
          return;
      }
    

    What happens if input is empty?

  • Several contructors call std::reduce, most accumulate a std::size_t buffer size (one is of type int). count() (also accumulates an int count) does this for you. Fix count() and have the constructors call it for the image_data.resize().

  • There is no need to use std::move on arguments passed by reference-to-const.

      Image(const std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight)
          image_data = std::move(input); // Reference: https://stackoverflow.com/a/51706522/6667035
    

    You cannot move from a const object as moving mutates the source. What you get here is a copy.

      Image(const std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          image_data = std::move(input); // this is a copy-assignment
          image_data = input;            // keep it simple
    
      Image(std::vector<ElementT> input, std::size_t newWidth, std::size_t newHeight)
            ^^^^^^^^^^^^^^^^^^^^^
          image_data = std::move(input); // this is a move-assignment
    
      Image(std::vector<ElementT>&& input, std::size_t newWidth, std::size_t newHeight)
            ^^^^^^^^^^^^^^^^^^^^^^^
          image_data = std::move(input); // this is a move-assignment
    
  • Don't pollute the global namespace. Your types in basic_types.h should be wrapped in a namespace.

  • Prefer to use the C++ headers over the C headers. C headers only exist for compatability for use with the C-standard library. C++ implementations are not required by the standard to provide this compatability but they are required to provide the <c...> versions.

      #include <cmath>      // <math.h>     
      #include <cstdint>
      #include <cstdio>     // <stdio.h>    
      #include <cstdlib>    // <stdlib.h>   
      #include <filesystem>
      #include <string>
      #include <utility>
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.