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 fileimage.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 filebasic_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 implementationnamespace 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;
}
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 theImage
class.Why a new review is being asked for?
Please review the implementation of
set
member function.
1 Answer 1
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 forstd::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 eitherint
orstd::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 astd::size_t
buffer size (one is of typeint
).count()
(also accumulates anint
count) does this for you. Fixcount()
and have the constructors call it for theimage_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>
Explore related questions
See similar questions with these tags.