[{"id":29816,"web_url":"https://patchwork.libcamera.org/comment/29816/","msgid":"<171803223111.2248009.6256969235873618292@ping.linuxembedded.co.uk>","date":"2024-06-10T15:10:31","subject":"Re: [PATCH v7 1/4] ipa: libipa: Add Vector class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-06-10 15:19:38)\n> Add a vector class to libipa. The original purpose of this is to replace\n> the floating-point Point class that Raspberry Pi used in their Pwl, as\n> that implementation of Point seemed more akin to a Vector than a Point.\n> \n> This is added to libipa instead of to geometry.h to avoid public API\n> issues, plus this is not expected to be needed by applications.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Stefan Klug \n> \n> ---\n> Changes in v7:\n> - fix documentation\n> - fix license and copyright\n> - remove toString()\n> \n> No change in v6\n> \n> New in v5\n> ---\n> src/ipa/libipa/vector.cpp | 145 +++++++++++++++++++++++++++\n> src/ipa/libipa/vector.h | 199 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 344 insertions(+)\n> create mode 100644 src/ipa/libipa/vector.cpp\n> create mode 100644 src/ipa/libipa/vector.h\n> \n> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> new file mode 100644\n> index 000000000000..e7d0be95812c\n> --- /dev/null\n> +++ b/src/ipa/libipa/vector.cpp\n> @@ -0,0 +1,145 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Vector and related operations\n> + */\n> +\n> +#include \"vector.h\"\n> +\n> +#include \n> +\n> +/**\n> + * \\file vector.h\n> + * \\brief Vector class\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Vector)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Vector\n> + * \\brief Vector class\n> + * \\tparam T Type of numerical values to be stored in the vector\n> + * \\tparam D Number of dimension of the vector (= number of elements)\n> + */\n> +\n> +/**\n> + * \\fn Vector::Vector()\n> + * \\brief Construct a zero vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::Vector(const std::array &data)\n> + * \\brief Construct vector from supplied data\n> + * \\param data Data from which to construct a vector\n> + *\n> + * The size of \\a data must be equal to the dimension size D of the vector.\n> + */\n> +\n> +/**\n> + * \\fn Vector::readYaml\n> + * \\brief Populate the vector with yaml data\n> + * \\param yaml Yaml data to populate the vector with\n> + *\n> + * Any existing data in the vector will be overwritten. The size of the data\n> + * read from \\a yaml must be equal to the dimension size D of the vector.\n> + *\n> + * The yaml data is expected to be a list with elements of type T.\n> + *\n> + * \\return 0 on success, negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn T Vector::operator[](size_t i) const\n> + * \\brief Index to an element in the vector\n> + * \\param i Index of element to retrieve\n> + * \\return Element at index \\a i from the vector\n> + */\n> +\n> +/**\n> + * \\fn T &Vector::operator[](size_t i)\n> + * \\copydoc Vector::operator[](size_t i) const\n> + */\n> +\n> +/**\n> + * \\fn Vector::x()\n> + * \\brief Convenience function to access the first element of the vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::y()\n> + * \\brief Convenience function to access the second element of the vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator-() const\n> + * \\brief Negate a Vector by negating both all of its coordinates\n> + * \\return The negated vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator-(Vector const &other) const\n> + * \\brief Subtract one vector from another\n> + * \\param[in] other The other vector\n> + * \\return The difference of \\a other from this vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator+()\n> + * \\brief Add two vectors together\n> + * \\param[in] other The other vector\n> + * \\return The sum of the two vectors\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator*(const Vector &other) const\n> + * \\brief Compute the dot product\n> + * \\param[in] other The other vector\n> + * \\return The dot product of the two vectors\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator*(T factor) const\n> + * \\brief Multiply the vector by a scalar\n> + * \\param[in] factor The factor\n> + * \\return The vector multiplied by \\a factor\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator/()\n> + * \\brief Divide the vector by a scalar\n> + * \\param[in] factor The factor\n> + * \\return The vector divided by \\a factor\n> + */\n> +\n> +/**\n> + * \\fn Vector::len2()\n> + * \\brief Get the squared length of the vector\n> + * \\return The squared length of the vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::len()\n> + * \\brief Get the length of the vector\n> + * \\return The length of the vector\n> + */\n> +\n> +/**\n> + * \\fn bool operator==(const Vector &lhs, const Vector &rhs)\n> + * \\brief Compare vectors for equality\n> + * \\return True if the two vectors are equal, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn bool operator!=(const Vector &lhs, const Vector &rhs)\n> + * \\brief Compare vectors for inequality\n> + * \\return True if the two vectors are not equal, false otherwise\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> new file mode 100644\n> index 000000000000..b5a6a0e021e0\n> --- /dev/null\n> +++ b/src/ipa/libipa/vector.h\n> @@ -0,0 +1,199 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Vector and related operations\n> + */\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Vector)\n> +\n> +namespace ipa {\n> +\n> +#ifndef __DOXYGEN__\n> +template + std::enable_if_t && D >= 2> * = nullptr>\n> +#else\n> +template\n> +#endif /* __DOXYGEN__ */\n> +class Vector\n> +{\n> +public:\n> + Vector() = default;\n> +\n> + Vector(const std::array &data)\n> + {\n> + ASSERT(data.size() == D);\n> +\n> + for (unsigned int i = 0; i < D; i++)\n> + data_[i] = data[i];\n> + }\n> +\n> + ~Vector() = default;\n> +\n> + int readYaml(const libcamera::YamlObject &yaml)\n> + {\n> + if (yaml.size() != D) {\n> + LOG(Vector, Error)\n> + << \"Wrong number of values in vector: expected \"\n> + << D << \", got \" << yaml.size();\n> + return -EINVAL;\n> + }\n> +\n> + unsigned int i = 0;\n> + for (const auto &x : yaml.asList()) {\n> + auto value = x.get();\n> + if (!value) {\n> + LOG(Vector, Error) << \"Failed to read vector value\";\n> + return -EINVAL;\n> + }\n> +\n> + data_[i++] = *value;\n> + }\n> +\n> + return 0;\n> + }\n> +\n> + const T operator[](size_t i) const\n> + {\n> + ASSERT(0 < i && i < data_.size());\n\nWhy 0 < i ? What's wrong with data_[0] ?\n\n> + return data_[i];\n> + }\n> +\n> + T &operator[](size_t i)\n> + {\n> + ASSERT(0 < i && i < data_.size());\n\nSame...\n\n\nWith those checked/fixed if needed.\n\n\nReviewed-by: Kieran Bingham \n\n> + return data_[i];\n> + }\n> +\n> + const T x() const\n> + {\n> + return data_[0];\n> + }\n> +\n> + const T y() const\n> + {\n> + return data_[1];\n> + }\n> +\n> + constexpr Vector operator-() const\n> + {\n> + Vector ret;\n> + for (unsigned int i = 0; i < D; i++)\n> + ret[i] = -data_[i];\n> + return ret;\n> + }\n> +\n> + constexpr Vector operator-(const Vector &other) const\n> + {\n> + Vector ret;\n> + for (unsigned int i = 0; i < D; i++)\n> + ret[i] = data_[i] - other[i];\n> + return ret;\n> + }\n> +\n> + constexpr Vector operator+(const Vector &other) const\n> + {\n> + Vector ret;\n> + for (unsigned int i = 0; i < D; i++)\n> + ret[i] = data_[i] + other[i];\n> + return ret;\n> + }\n> +\n> + constexpr T operator*(const Vector &other) const\n> + {\n> + T ret = 0;\n> + for (unsigned int i = 0; i < D; i++)\n> + ret += data_[i] * other[i];\n> + return ret;\n> + }\n> +\n> + constexpr Vector operator*(T factor) const\n> + {\n> + Vector ret;\n> + for (unsigned int i = 0; i < D; i++)\n> + ret[i] = data_[i] * factor;\n> + return ret;\n> + }\n> +\n> + constexpr Vector operator/(T factor) const\n> + {\n> + Vector ret;\n> + for (unsigned int i = 0; i < D; i++)\n> + ret[i] = data_[i] / factor;\n> + return ret;\n> + }\n> +\n> + constexpr T len2() const\n> + {\n> + T ret = 0;\n> + for (unsigned int i = 0; i < D; i++)\n> + ret += data_[i] * data_[i];\n> + return ret;\n> + }\n> +\n> + constexpr double len() const\n> + {\n> + return std::sqrt(len2());\n> + }\n> +\n> +private:\n> + std::array data_;\n> +};\n> +\n> +#ifndef __DOXYGEN__\n> +template + std::enable_if_t> * = nullptr>\n> +#endif /* __DOXYGEN__ */\n> +bool operator==(const Vector &lhs, const Vector &rhs)\n> +{\n> + for (unsigned int i = 0; i < D; i++)\n> + if (lhs[i] != rhs[i])\n> + return false;\n> +\n> + return true;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template + std::enable_if_t> * = nullptr>\n> +#endif /* __DOXYGEN__ */\n> +bool operator!=(const Vector &lhs, const Vector &rhs)\n> +{\n> + for (unsigned int i = 0; i < D; i++)\n> + if (lhs[i] != rhs[i])\n> + return true;\n> +\n> + return false;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +#ifndef __DOXYGEN__\n> +template\n> +std::ostream &operator<<(std::ostream &out, const ipa::Vector &v)\n> +{\n> + out << \"Vector { \";\n> + for (unsigned int i = 0; i < D; i++) {\n> + out << v[i];\n> + out << ((i + 1 < D) ? \", \" : \" \");\n> + }\n> + out << \" }\";\n> +\n> + return out;\n> +}\n> +#endif /* __DOXYGEN__ */\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.39.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E6EC5C31E9\n\tfor ;\n\tMon, 10 Jun 2024 15:10:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C10265462;\n\tMon, 10 Jun 2024 17:10:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19E9F634D5\n\tfor ;\n\tMon, 10 Jun 2024 17:10:34 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EAEA539F;\n\tMon, 10 Jun 2024 17:10:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XgS6E2JQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718032222;\n\tbh=WpCfqb/Oq7KSj1bl+N0TXw/VHTo2IV6MZT/etKBkDPI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=XgS6E2JQjgGyFu4ts/2jnWUALT9SsJ4my/wg9A0PLmFMlP6MlMk38QndZ2nNNeQbe\n\tescP2ztT2wJ34gaOgXWIx1oE50pX1rVCCO56lHubyEyacEMql95eCVc7vc97SXHrGg\n\t70dQuoDD2/9lw4pGQuhOol2noT+oW/tT5X2Hyl5Y=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240610141941.2947785-2-paul.elder@ideasonboard.com>","References":"<20240610141941.2947785-1-paul.elder@ideasonboard.com>\n\t<20240610141941.2947785-2-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v7 1/4] ipa: libipa: Add Vector class","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tStefan Klug ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 10 Jun 2024 16:10:31 +0100","Message-ID":"<171803223111.2248009.6256969235873618292@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29820,"web_url":"https://patchwork.libcamera.org/comment/29820/","msgid":"<20240610232921.GA31989@pendragon.ideasonboard.com>","date":"2024-06-10T23:29:21","subject":"Re: [PATCH v7 1/4] ipa: libipa: Add Vector class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Jun 10, 2024 at 04:10:31PM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-06-10 15:19:38)\n> > Add a vector class to libipa. The original purpose of this is to replace\n> > the floating-point Point class that Raspberry Pi used in their Pwl, as\n> > that implementation of Point seemed more akin to a Vector than a Point.\n> > \n> > This is added to libipa instead of to geometry.h to avoid public API\n> > issues, plus this is not expected to be needed by applications.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Stefan Klug \n> > \n> > ---\n> > Changes in v7:\n> > - fix documentation\n> > - fix license and copyright\n> > - remove toString()\n> > \n> > No change in v6\n> > \n> > New in v5\n> > ---\n> > src/ipa/libipa/vector.cpp | 145 +++++++++++++++++++++++++++\n> > src/ipa/libipa/vector.h | 199 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 344 insertions(+)\n> > create mode 100644 src/ipa/libipa/vector.cpp\n> > create mode 100644 src/ipa/libipa/vector.h\n> > \n> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> > new file mode 100644\n> > index 000000000000..e7d0be95812c\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/vector.cpp\n> > @@ -0,0 +1,145 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * Vector and related operations\n> > + */\n> > +\n> > +#include \"vector.h\"\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file vector.h\n> > + * \\brief Vector class\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Vector)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\class Vector\n> > + * \\brief Vector class\n> > + * \\tparam T Type of numerical values to be stored in the vector\n> > + * \\tparam D Number of dimension of the vector (= number of elements)\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::Vector()\n> > + * \\brief Construct a zero vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::Vector(const std::array &data)\n> > + * \\brief Construct vector from supplied data\n> > + * \\param data Data from which to construct a vector\n> > + *\n> > + * The size of \\a data must be equal to the dimension size D of the vector.\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::readYaml\n> > + * \\brief Populate the vector with yaml data\n> > + * \\param yaml Yaml data to populate the vector with\n> > + *\n> > + * Any existing data in the vector will be overwritten. The size of the data\n> > + * read from \\a yaml must be equal to the dimension size D of the vector.\n> > + *\n> > + * The yaml data is expected to be a list with elements of type T.\n> > + *\n> > + * \\return 0 on success, negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn T Vector::operator[](size_t i) const\n> > + * \\brief Index to an element in the vector\n> > + * \\param i Index of element to retrieve\n> > + * \\return Element at index \\a i from the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn T &Vector::operator[](size_t i)\n> > + * \\copydoc Vector::operator[](size_t i) const\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::x()\n> > + * \\brief Convenience function to access the first element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::y()\n> > + * \\brief Convenience function to access the second element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator-() const\n> > + * \\brief Negate a Vector by negating both all of its coordinates\n> > + * \\return The negated vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator-(Vector const &other) const\n> > + * \\brief Subtract one vector from another\n> > + * \\param[in] other The other vector\n> > + * \\return The difference of \\a other from this vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator+()\n> > + * \\brief Add two vectors together\n> > + * \\param[in] other The other vector\n> > + * \\return The sum of the two vectors\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator*(const Vector &other) const\n> > + * \\brief Compute the dot product\n> > + * \\param[in] other The other vector\n> > + * \\return The dot product of the two vectors\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator*(T factor) const\n> > + * \\brief Multiply the vector by a scalar\n> > + * \\param[in] factor The factor\n> > + * \\return The vector multiplied by \\a factor\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator/()\n> > + * \\brief Divide the vector by a scalar\n> > + * \\param[in] factor The factor\n> > + * \\return The vector divided by \\a factor\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::len2()\n> > + * \\brief Get the squared length of the vector\n> > + * \\return The squared length of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::len()\n> > + * \\brief Get the length of the vector\n> > + * \\return The length of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn bool operator==(const Vector &lhs, const Vector &rhs)\n> > + * \\brief Compare vectors for equality\n> > + * \\return True if the two vectors are equal, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn bool operator!=(const Vector &lhs, const Vector &rhs)\n> > + * \\brief Compare vectors for inequality\n> > + * \\return True if the two vectors are not equal, false otherwise\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> > new file mode 100644\n> > index 000000000000..b5a6a0e021e0\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/vector.h\n> > @@ -0,0 +1,199 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * Vector and related operations\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Vector)\n> > +\n> > +namespace ipa {\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > + std::enable_if_t && D >= 2> * = nullptr>\n\nWhy 2 ? Is that because of the x() and y() functions ? You can handle\nthat in those functions directly:\n\n\ttemplate>\n T &x()\n\t{\n\t\treturn data_[0];\n\t}\n\n\ttemplate>\n T &y()\n\t{\n\t\treturn data_[1];\n\t}\n\nand drop the constraint here.\n\n> > +#else\n> > +template\n> > +#endif /* __DOXYGEN__ */\n> > +class Vector\n\nWhat would you think about naming the template parameters Scalar and\nRows instead of T and D ? That would be more explicit.\n\n> > +{\n> > +public:\n> > + Vector() = default;\n> > +\n> > + Vector(const std::array &data)\n\nPlease make the constructors constexpr if possible.\n\n> > + {\n> > + ASSERT(data.size() == D);\n\nThat's not needed, by definition std::array::size() == D.\n\n> > +\n> > + for (unsigned int i = 0; i < D; i++)\n> > + data_[i] = data[i];\n> > + }\n> > +\n> > + ~Vector() = default;\n\nYou can drop this, it's literally the default :-)\n\n> > +\n> > + int readYaml(const libcamera::YamlObject &yaml)\n> > + {\n> > + if (yaml.size() != D) {\n> > + LOG(Vector, Error)\n> > + << \"Wrong number of values in vector: expected \"\n> > + << D << \", got \" << yaml.size();\n> > + return -EINVAL;\n> > + }\n> > +\n> > + unsigned int i = 0;\n> > + for (const auto &x : yaml.asList()) {\n> > + auto value = x.get();\n> > + if (!value) {\n> > + LOG(Vector, Error) << \"Failed to read vector value\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + data_[i++] = *value;\n> > + }\n> > +\n> > + return 0;\n> > + }\n\nThis is the only part that really bothers me. This tight coupling\nbetween the Vector class, which is a very generic math object, and\nYamlObject, isn't nice.\n\nOne possible option would be to implement this as a specialization of\nYamlObject::get(), in vector.cpp. I haven't tested it though.\n\n> > +\n> > + const T operator[](size_t i) const\n\nWhy do you return a copy instead of a const reference ?\n\n> > + {\n> > + ASSERT(0 < i && i < data_.size());\n> \n> Why 0 < i ? What's wrong with data_[0] ?\n\nOn a side note, it should be written 'i > 0', that's the order we use\neverywhere. I understand that the above syntex is meant to check\n\n\t0 < i < data_.size()\n\nand I'm fine with that as a mathematical notation, but code-wise,\n'0 < i' takes more mental energy to read.\n\n> > + return data_[i];\n> > + }\n> > +\n> > + T &operator[](size_t i)\n> > + {\n> > + ASSERT(0 < i && i < data_.size());\n> \n> Same...\n> \n> With those checked/fixed if needed.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > + return data_[i];\n> > + }\n> > +\n> > + const T x() const\n> > + {\n> > + return data_[0];\n> > + }\n> > +\n> > + const T y() const\n> > + {\n> > + return data_[1];\n> > + }\n> > +\n> > + constexpr Vector operator-() const\n> > + {\n> > + Vector ret;\n> > + for (unsigned int i = 0; i < D; i++)\n> > + ret[i] = -data_[i];\n> > + return ret;\n> > + }\n> > +\n> > + constexpr Vector operator-(const Vector &other) const\n> > + {\n> > + Vector ret;\n> > + for (unsigned int i = 0; i < D; i++)\n> > + ret[i] = data_[i] - other[i];\n> > + return ret;\n> > + }\n> > +\n> > + constexpr Vector operator+(const Vector &other) const\n> > + {\n> > + Vector ret;\n> > + for (unsigned int i = 0; i < D; i++)\n> > + ret[i] = data_[i] + other[i];\n> > + return ret;\n> > + }\n> > +\n> > + constexpr T operator*(const Vector &other) const\n> > + {\n> > + T ret = 0;\n> > + for (unsigned int i = 0; i < D; i++)\n> > + ret += data_[i] * other[i];\n> > + return ret;\n> > + }\n> > +\n> > + constexpr Vector operator*(T factor) const\n> > + {\n> > + Vector ret;\n> > + for (unsigned int i = 0; i < D; i++)\n> > + ret[i] = data_[i] * factor;\n> > + return ret;\n> > + }\n> > +\n> > + constexpr Vector operator/(T factor) const\n> > + {\n> > + Vector ret;\n> > + for (unsigned int i = 0; i < D; i++)\n> > + ret[i] = data_[i] / factor;\n> > + return ret;\n> > + }\n> > +\n> > + constexpr T len2() const\n\nlength2() ? And length() below.\n\n> > + {\n> > + T ret = 0;\n> > + for (unsigned int i = 0; i < D; i++)\n> > + ret += data_[i] * data_[i];\n\nIf T is an integer short integer type (8 or 16 bits) there's a high\nchance this will overflow silently. Is that an issue ? Should the\nfunction return a double ?\n\n> > + return ret;\n> > + }\n> > +\n> > + constexpr double len() const\n> > + {\n> > + return std::sqrt(len2());\n> > + }\n> > +\n> > +private:\n> > + std::array data_;\n> > +};\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > + std::enable_if_t> * = nullptr>\n\nIs this needed, given that the Vector class checks that already ? Same\nbelow.\n\nI wrote a long time ago an RGB class which morphed into a Vector, before\nI dropped it because I thought I was reinventing the wheel. As it seems\nwe haven't found a better wheel, I'll probably post a patch on top of\nthis as it extends your implementation with quite a few functions.\n\n> > +#endif /* __DOXYGEN__ */\n> > +bool operator==(const Vector &lhs, const Vector &rhs)\n> > +{\n> > + for (unsigned int i = 0; i < D; i++)\n> > + if (lhs[i] != rhs[i])\n> > + return false;\n> > +\n> > + return true;\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > + std::enable_if_t> * = nullptr>\n> > +#endif /* __DOXYGEN__ */\n> > +bool operator!=(const Vector &lhs, const Vector &rhs)\n> > +{\n> > + for (unsigned int i = 0; i < D; i++)\n> > + if (lhs[i] != rhs[i])\n> > + return true;\n> > +\n> > + return false;\n> > +}\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template\n> > +std::ostream &operator<<(std::ostream &out, const ipa::Vector &v)\n> > +{\n> > + out << \"Vector { \";\n> > + for (unsigned int i = 0; i < D; i++) {\n> > + out << v[i];\n> > + out << ((i + 1 < D) ? \", \" : \" \");\n> > + }\n> > + out << \" }\";\n> > +\n> > + return out;\n> > +}\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > +} /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0401FBD87C\n\tfor ;\n\tMon, 10 Jun 2024 23:29:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E59C865462;\n\tTue, 11 Jun 2024 01:29:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D71F65458\n\tfor ;\n\tTue, 11 Jun 2024 01:29:41 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFC2629A;\n\tTue, 11 Jun 2024 01:29:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kIO/jwUm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718062168;\n\tbh=zCbCkop1yLHSi1UY9x8ibbKG/Jp7fO+TLeqtZnfunEk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kIO/jwUmcydI+vLQ67aaEOrFN0t/ZdC/xjN1lOO8iny2o2shE4GCDuGF5laDre7zT\n\tsgwCiypWKgoP9u9J0+ZRdGEOTSmlwsMax/Aj86RzDJxdFJJM4RqaegqvTt1BKUUsUA\n\t51+IQPJku+DJiB06NU3ZyyewFJbdEFeyy4e8Is6k=","Date":"Tue, 11 Jun 2024 02:29:21 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v7 1/4] ipa: libipa: Add Vector class","Message-ID":"<20240610232921.GA31989@pendragon.ideasonboard.com>","References":"<20240610141941.2947785-1-paul.elder@ideasonboard.com>\n\t<20240610141941.2947785-2-paul.elder@ideasonboard.com>\n\t<171803223111.2248009.6256969235873618292@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171803223111.2248009.6256969235873618292@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]