[{"id":29795,"web_url":"https://patchwork.libcamera.org/comment/29795/","msgid":"<171801489599.1489778.10050343972693994261@ping.linuxembedded.co.uk>","date":"2024-06-10T10:21:35","subject":"Re: [PATCH v6 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-07 09:42:58)\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 <paul.elder@ideasonboard.com>\n> \n> ---\n> No change in v6\n> \n> New in v5\n> ---\n>  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++\n>  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 368 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 000000000..bdc3c447c\n> --- /dev/null\n> +++ b/src/ipa/libipa/vector.cpp\n> @@ -0,0 +1,162 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Vector and related operations\n> + */\n> +\n> +#include \"vector.h\"\n> +\n> +#include <libcamera/base/log.h>\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 R Number of rows in the vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::Vector()\n> + * \\brief Construct an identity vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::Vector(const std::array<T, R> &data)\n> + * \\brief Construct vector from supplied data\n> + * \\param data Data from which to construct a vector\n> + *\n> + * \\a data is a one-dimensional vector and will be turned into a vector in\n> + * row-major order. The size of \\a data must be equal to the product of the\n> + * number of rows and columns of the vector (RxC).\n\nRows and columns of a vector?\n\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 product of the number of rows and\n> + * columns of the vector (RxC).\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 Vector::toString\n> + * \\brief Assemble and return a string describing the vector\n> + * \\return A string describing the vector\n> + */\n> +\n> +/**\n> + * \\fn T Vector::operator[](size_t i) const\n> + * \\brief Index to a row in the vector\n> + * \\param i Index of row to retrieve\n> + *\n> + * This operator[] returns a Span, which can then be indexed into again with\n> + * another operator[], allowing a convenient m[i][j] to access elements of the\n> + * vector. Note that the lifetime of the Span returned by this first-level\n> + * operator[] is bound to that of the Vector itself, so it is not recommended\n> + * to save the Span that is the result of this operator[].\n> + *\n> + * \\return Row \\a i from the vector, as a Span\n\nAre these needed for a vector? I don't understand the indexing of a\nvector? I could understand on a matrix - but isn't a vector just a set\nof coordinates that represent direction/magnitude?\n\nI don't understand what m[i][j] is used for ? (or represents?)\n\nPerhaps the hint 'm' means this is a left over and needs to be removed?\n\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\nThese I understand :-)\n\n\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<T, R> &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 Scale up the vector\n> + * \\param[in] factor The factor\n> + * \\return The vector scaled up by \\a factor\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator/()\n> + * \\brief Scale down the vector\n> + * \\param[in] factor The factor\n> + * \\return The vector scaled down 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\nI think all of those probably deserve at least some level of a unit\ntest. How come we don't seem to have tests for libipa - Seems like the\nsort of thing we should have... Particularly as you've got operations on\ndiffering types above.\n\n\n\n> +\n> +/**\n> + * \\fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &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<T, R> &lhs, const Vector<T, R> &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 000000000..87aad28b5\n> --- /dev/null\n> +++ b/src/ipa/libipa/vector.h\n> @@ -0,0 +1,206 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Vector and related operations\n> + */\n> +#pragma once\n> +\n> +#include <algorithm>\n> +#include <array>\n> +#include <cmath>\n> +#include <sstream>\n> +\n> +#include <libcamera/base/log.h>\n> +#include <libcamera/base/span.h>\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<typename T, unsigned int R,\n> +        std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>\n> +#else\n> +template<typename T, unsigned int R>\n\nDo we anticipate 3d vectors? (or other dimesions?)\n\nHow about defaulting R = 2 so that a 2d vector is the default case?\n\nThen we can use\n\tVector<int> v = {x, y};\n\nor such. (Or some how typedef/using to create Vector2d, Vector3d?)\n\nIs R an appropriate name for a vector? Is D for dimensions any better?\nOr is it still better to call it Rows?\n\n\n\n> +#endif /* __DOXYGEN__ */\n> +class Vector\n> +{\n> +public:\n> +       Vector() = default;\n> +\n> +       Vector(const std::array<T, R> &data)\n> +       {\n> +               ASSERT(data.size() == R);\n\nOptional, but as we provide direct accessors for x() and y() perhaps:\n\n\tASSERT(R >= 2);\n\nThat might stop accidentally creating a vector of one dimesion..\n\n\n> +\n> +               for (unsigned int i = 0; i < R; i++)\n> +                       data_[i] = data[i];\n> +       }\n> +\n> +       ~Vector() = default;\n> +\n> +       int readYaml(const libcamera::YamlObject &yaml)\n> +       {\n> +               if (yaml.size() != R) {\n> +                       LOG(Vector, Error)\n> +                               << \"Wrong number of values in vector: expected \"\n> +                               << R << \", 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<T>();\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 std::string toString() const\n> +       {\n> +               std::stringstream out;\n> +\n> +               out << \"Vector { \";\n> +               for (unsigned int i = 0; i < R; i++) {\n> +                       out << (*this)[i];\n> +                       out << ((i + 1 < R) ? \", \" : \" \");\n> +               }\n> +               out << \" }\";\n> +\n> +               return out.str();\n> +       }\n> +\n> +       const T operator[](size_t i) const\n> +       {\n\nASSERT here if out of bounds?\n\n> +               return data_[i];\n> +       }\n> +\n> +       T &operator[](size_t i)\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<T, R> operator-() const\n> +       {\n> +               Vector<T, R> ret;\n> +               for (unsigned int i = 0; i < R; i++)\n> +                       ret[i] = -data_[i];\n> +               return ret;\n> +       }\n\nHow would this be used? (differently than below?)\n\nUnit tests would help here I think to confirm and validate intended\npurposes.\n\n> +\n> +       constexpr Vector<T, R> operator-(const Vector<T, R> &other) const\n> +       {\n> +               Vector<T, R> ret;\n> +               for (unsigned int i = 0; i < R; i++)\n> +                       ret[i] = data_[i] - other[i];\n> +               return ret;\n> +       }\n> +\n> +       constexpr Vector<T, R> operator+(const Vector<T, R> &other) const\n> +       {\n> +               Vector<T, R> ret;\n> +               for (unsigned int i = 0; i < R; i++)\n> +                       ret[i] = data_[i] + other[i];\n> +               return ret;\n> +       }\n> +\n> +       constexpr T operator*(const Vector<T, R> &other) const\n> +       {\n> +               T ret = 0;\n> +               for (unsigned int i = 0; i < R; i++)\n> +                       ret += data_[i] * other[i];\n> +               return ret;\n> +       }\n> +\n> +       constexpr Vector<T, R> operator*(T factor) const\n> +       {\n> +               Vector<T, R> ret;\n> +               for (unsigned int i = 0; i < R; i++)\n> +                       ret[i] = data_[i] * factor;\n> +               return ret;\n> +       }\n> +\n> +       constexpr Vector<T, R> operator/(T factor) const\n> +       {\n> +               Vector<T, R> ret;\n> +               for (unsigned int i = 0; i < R; 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 < R; 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<T, R> data_;\n> +};\n\n\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, unsigned int R,\n> +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#endif /* __DOXYGEN__ */\n> +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> +{\n> +       for (unsigned int i = 0; i < R; i++)\n> +               if (lhs[i] != rhs[i])\n> +                       return false;\n> +\n> +       return true;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, unsigned int R,\n> +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#endif /* __DOXYGEN__ */\n> +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> +{\n> +       for (unsigned int i = 0; i < R; 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<typename T, unsigned int R>\n> +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)\n> +{\n> +       out << v.toString();\n> +       return out;\n> +}\n> +#endif /* __DOXYGEN__ */\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.39.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 96188C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jun 2024 10:21:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8973D65455;\n\tMon, 10 Jun 2024 12:21:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98E9E65446\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 12:21:39 +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 7DE24512;\n\tMon, 10 Jun 2024 12:21:27 +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=\"NGVlejA8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718014887;\n\tbh=+K/M9OznfGS02A2Ssh/y1hguStCUfM42JD2hXYpoQC8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=NGVlejA87FL09cdNmk0p6Y+kjig8VbvjGgPbSgmC/qG4Tvcr36OJ3Vr1vFAMQr1AY\n\tT3EBLwIwxXfep2cnF46LIasOXLx+PjdFs84+JCbHTvdUSnaaPj8cK1FXjuJRTyEU++\n\tKxYFxXuv0rV/Ebhh6HHSzWYE5WuO2zuoZJQU0NTY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240607084301.2791258-2-paul.elder@ideasonboard.com>","References":"<20240607084301.2791258-1-paul.elder@ideasonboard.com>\n\t<20240607084301.2791258-2-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Paul Elder <paul.elder@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 10 Jun 2024 11:21:35 +0100","Message-ID":"<171801489599.1489778.10050343972693994261@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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29799,"web_url":"https://patchwork.libcamera.org/comment/29799/","msgid":"<CAHW6GYJhyJOUytNwPs0_5bYKFtSz8RRR7S2gqktkkrw=jnk9ew@mail.gmail.com>","date":"2024-06-10T10:33:39","subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Paul\n\nThank you for this patch.\n\nOn Mon, 10 Jun 2024 at 11:21, Kieran Bingham\n<kieran.bingham@ideasonboard.com> wrote:\n>\n> Quoting Paul Elder (2024-06-07 09:42:58)\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 <paul.elder@ideasonboard.com>\n> >\n> > ---\n> > No change in v6\n> >\n> > New in v5\n> > ---\n> >  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++\n> >  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 368 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 000000000..bdc3c447c\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/vector.cpp\n> > @@ -0,0 +1,162 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> > + *\n> > + * Vector and related operations\n> > + */\n> > +\n> > +#include \"vector.h\"\n> > +\n> > +#include <libcamera/base/log.h>\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 R Number of rows in the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::Vector()\n> > + * \\brief Construct an identity vector\n\nI just wanted to comment that this expression \"identity vector\" had me\nconfused for a moment. I assume we just mean the \"zero vector\" which,\nit's true, is the identity element under the addition operation. But\nthen we often multiply vectors by scalars, or matrices, so it does\njust make you pause for thought. Anyway, \"zero vector\" would have\ncaused my brain cells slightly less cognitive dislocation!\n\nThanks\nDavid\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::Vector(const std::array<T, R> &data)\n> > + * \\brief Construct vector from supplied data\n> > + * \\param data Data from which to construct a vector\n> > + *\n> > + * \\a data is a one-dimensional vector and will be turned into a vector in\n> > + * row-major order. The size of \\a data must be equal to the product of the\n> > + * number of rows and columns of the vector (RxC).\n>\n> Rows and columns of a vector?\n>\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 product of the number of rows and\n> > + * columns of the vector (RxC).\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 Vector::toString\n> > + * \\brief Assemble and return a string describing the vector\n> > + * \\return A string describing the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn T Vector::operator[](size_t i) const\n> > + * \\brief Index to a row in the vector\n> > + * \\param i Index of row to retrieve\n> > + *\n> > + * This operator[] returns a Span, which can then be indexed into again with\n> > + * another operator[], allowing a convenient m[i][j] to access elements of the\n> > + * vector. Note that the lifetime of the Span returned by this first-level\n> > + * operator[] is bound to that of the Vector itself, so it is not recommended\n> > + * to save the Span that is the result of this operator[].\n> > + *\n> > + * \\return Row \\a i from the vector, as a Span\n>\n> Are these needed for a vector? I don't understand the indexing of a\n> vector? I could understand on a matrix - but isn't a vector just a set\n> of coordinates that represent direction/magnitude?\n>\n> I don't understand what m[i][j] is used for ? (or represents?)\n>\n> Perhaps the hint 'm' means this is a left over and needs to be removed?\n>\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> These I understand :-)\n>\n>\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<T, R> &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 Scale up the vector\n> > + * \\param[in] factor The factor\n> > + * \\return The vector scaled up by \\a factor\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator/()\n> > + * \\brief Scale down the vector\n> > + * \\param[in] factor The factor\n> > + * \\return The vector scaled down 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> I think all of those probably deserve at least some level of a unit\n> test. How come we don't seem to have tests for libipa - Seems like the\n> sort of thing we should have... Particularly as you've got operations on\n> differing types above.\n>\n>\n>\n> > +\n> > +/**\n> > + * \\fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &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<T, R> &lhs, const Vector<T, R> &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 000000000..87aad28b5\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/vector.h\n> > @@ -0,0 +1,206 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> > + *\n> > + * Vector and related operations\n> > + */\n> > +#pragma once\n> > +\n> > +#include <algorithm>\n> > +#include <array>\n> > +#include <cmath>\n> > +#include <sstream>\n> > +\n> > +#include <libcamera/base/log.h>\n> > +#include <libcamera/base/span.h>\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<typename T, unsigned int R,\n> > +        std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>\n> > +#else\n> > +template<typename T, unsigned int R>\n>\n> Do we anticipate 3d vectors? (or other dimesions?)\n>\n> How about defaulting R = 2 so that a 2d vector is the default case?\n>\n> Then we can use\n>         Vector<int> v = {x, y};\n>\n> or such. (Or some how typedef/using to create Vector2d, Vector3d?)\n>\n> Is R an appropriate name for a vector? Is D for dimensions any better?\n> Or is it still better to call it Rows?\n>\n>\n>\n> > +#endif /* __DOXYGEN__ */\n> > +class Vector\n> > +{\n> > +public:\n> > +       Vector() = default;\n> > +\n> > +       Vector(const std::array<T, R> &data)\n> > +       {\n> > +               ASSERT(data.size() == R);\n>\n> Optional, but as we provide direct accessors for x() and y() perhaps:\n>\n>         ASSERT(R >= 2);\n>\n> That might stop accidentally creating a vector of one dimesion..\n>\n>\n> > +\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       data_[i] = data[i];\n> > +       }\n> > +\n> > +       ~Vector() = default;\n> > +\n> > +       int readYaml(const libcamera::YamlObject &yaml)\n> > +       {\n> > +               if (yaml.size() != R) {\n> > +                       LOG(Vector, Error)\n> > +                               << \"Wrong number of values in vector: expected \"\n> > +                               << R << \", 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<T>();\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 std::string toString() const\n> > +       {\n> > +               std::stringstream out;\n> > +\n> > +               out << \"Vector { \";\n> > +               for (unsigned int i = 0; i < R; i++) {\n> > +                       out << (*this)[i];\n> > +                       out << ((i + 1 < R) ? \", \" : \" \");\n> > +               }\n> > +               out << \" }\";\n> > +\n> > +               return out.str();\n> > +       }\n> > +\n> > +       const T operator[](size_t i) const\n> > +       {\n>\n> ASSERT here if out of bounds?\n>\n> > +               return data_[i];\n> > +       }\n> > +\n> > +       T &operator[](size_t i)\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<T, R> operator-() const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = -data_[i];\n> > +               return ret;\n> > +       }\n>\n> How would this be used? (differently than below?)\n>\n> Unit tests would help here I think to confirm and validate intended\n> purposes.\n>\n> > +\n> > +       constexpr Vector<T, R> operator-(const Vector<T, R> &other) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = data_[i] - other[i];\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr Vector<T, R> operator+(const Vector<T, R> &other) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = data_[i] + other[i];\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr T operator*(const Vector<T, R> &other) const\n> > +       {\n> > +               T ret = 0;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret += data_[i] * other[i];\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr Vector<T, R> operator*(T factor) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = data_[i] * factor;\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr Vector<T, R> operator/(T factor) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; 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 < R; 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<T, R> data_;\n> > +};\n>\n>\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template<typename T, unsigned int R,\n> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> > +#endif /* __DOXYGEN__ */\n> > +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> > +{\n> > +       for (unsigned int i = 0; i < R; i++)\n> > +               if (lhs[i] != rhs[i])\n> > +                       return false;\n> > +\n> > +       return true;\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template<typename T, unsigned int R,\n> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> > +#endif /* __DOXYGEN__ */\n> > +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> > +{\n> > +       for (unsigned int i = 0; i < R; 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<typename T, unsigned int R>\n> > +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)\n> > +{\n> > +       out << v.toString();\n> > +       return out;\n> > +}\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.39.2\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0CD7ABD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jun 2024 10:33:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17B7F65462;\n\tMon, 10 Jun 2024 12:33:53 +0200 (CEST)","from mail-qk1-x72a.google.com (mail-qk1-x72a.google.com\n\t[IPv6:2607:f8b0:4864:20::72a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 445CE65446\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 12:33:51 +0200 (CEST)","by mail-qk1-x72a.google.com with SMTP id\n\taf79cd13be357-7955af79812so91023885a.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 03:33:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"OrIoDOZG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1718015630; x=1718620430;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=f7Waf3Ge4tDisxqhQpPn0NYZ3mMsq3JdoLJ+RHMkF1o=;\n\tb=OrIoDOZGHkh9v6YhbSIgPfBzfKEdW3DCLGNl2UM6OPzsD6mEX9W8Vyx4pn21KWQhrU\n\tuHTlyNK8ZF6XLDLalqQCOTKTnD6GhWHC5TdIywMLomuP5qSuir8JaY5tMk9k35No0R67\n\tOLgI2rD3XNVefefu0/aKWXT08AjlXgwvehf2XlB9yfTQUOrg9aHKqFgAlEaLXX34d8kz\n\tScrnrS7FRuH2ohjBT3k3xYOgJL4Ihg+TuW8Nz7uoZ+joTRL7tPBBXVUpj5mK2m6bCN6q\n\tRyUgqVLl/biGdWPz4DcjaqY5bU0w1EVxm+K5uTJUBp9qF1rEu5I+7ZoTz92QxM7qE9Ak\n\tKxBQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1718015630; x=1718620430;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=f7Waf3Ge4tDisxqhQpPn0NYZ3mMsq3JdoLJ+RHMkF1o=;\n\tb=fqCCOqiINtM2Tpy3elQWf5iHI+ZT/jHwM5HAmGiYHRkjpL5jFA0nvy92LDQRUtEtVe\n\tzWkh2h09Qx0cKPpUfeSwS9CsLTuHrEfmWt4ipom7vzUVKPYKYJpxPRcXUQe6eb3qrWJn\n\tK4aDSEuVwTS4TzRBGwM5rHwnnfcWPma3x6Jso4VFYVitVnSF7r8BS5iHZVhzWGGIHJ8S\n\tNBBY37RCVORC6zV32HznrTwXDbJ2qd7qsk9sSs2WEpISTglNFiUMBBX2jfon3DT8Fjkq\n\t6sy47LDiv4SUxeO8v6/uQRcip2xV+fl3wMtpQn+WTSyIyEU6hmJc+SenR+dJce67dd90\n\tLUbA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWZfqFB7ucEgs2N4hRghhpQFr73aZifx0RSergYt1ZFyedhHA152927vrrdXt6ZNiBx5ddIzRfY9DQOy0EumN273/mc7ni/cv9lbbeJK+cEDrBi5Q==","X-Gm-Message-State":"AOJu0YzCOot3LtkLfN01NmviSE47O+GzgMA3M8j0szbNn8qnER795BNM\n\tkT1TV+ApMpHRCBOn0hVPU2V/j0oinbyuFBbAfgpWdnLDOWlzR48eLxkBmfIP5glDgBGKY8n0OMl\n\twCeS3Hcwh20jmSs9Axn5ittYOwJOEo4Psd7mgGr1wGBjvWYd/","X-Google-Smtp-Source":"AGHT+IGDRtzxyb362Z6vgcpKmAwCu9NKFjf5mUzyUVG53Q0AnrivfZDJPBDvdVyJkeTeSDHJk8VwMt6THGPelHTrUys=","X-Received":"by 2002:a05:620a:2492:b0:795:570a:b9e3 with SMTP id\n\taf79cd13be357-795570abc62mr491333585a.34.1718015629831;\n\tMon, 10 Jun 2024 03:33:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20240607084301.2791258-1-paul.elder@ideasonboard.com>\n\t<20240607084301.2791258-2-paul.elder@ideasonboard.com>\n\t<171801489599.1489778.10050343972693994261@ping.linuxembedded.co.uk>","In-Reply-To":"<171801489599.1489778.10050343972693994261@ping.linuxembedded.co.uk>","From":"David Plowman <david.plowman@raspberrypi.com>","Date":"Mon, 10 Jun 2024 11:33:39 +0100","Message-ID":"<CAHW6GYJhyJOUytNwPs0_5bYKFtSz8RRR7S2gqktkkrw=jnk9ew@mail.gmail.com>","Subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29800,"web_url":"https://patchwork.libcamera.org/comment/29800/","msgid":"<171801659897.2248009.14495218581998625373@ping.linuxembedded.co.uk>","date":"2024-06-10T10:49:58","subject":"Re: [PATCH v6 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 David Plowman (2024-06-10 11:33:39)\n> Hi Paul\n> \n> Thank you for this patch.\n> \n> On Mon, 10 Jun 2024 at 11:21, Kieran Bingham\n> <kieran.bingham@ideasonboard.com> wrote:\n> >\n> > Quoting Paul Elder (2024-06-07 09:42:58)\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 <paul.elder@ideasonboard.com>\n> > >\n> > > ---\n> > > No change in v6\n> > >\n> > > New in v5\n> > > ---\n> > >  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++\n> > >  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++\n> > >  2 files changed, 368 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 000000000..bdc3c447c\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/vector.cpp\n> > > @@ -0,0 +1,162 @@\n> > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > +/*\n> > > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> > > + *\n> > > + * Vector and related operations\n> > > + */\n> > > +\n> > > +#include \"vector.h\"\n> > > +\n> > > +#include <libcamera/base/log.h>\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 R Number of rows in the vector\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Vector::Vector()\n> > > + * \\brief Construct an identity vector\n> \n> I just wanted to comment that this expression \"identity vector\" had me\n> confused for a moment. I assume we just mean the \"zero vector\" which,\n> it's true, is the identity element under the addition operation. But\n> then we often multiply vectors by scalars, or matrices, so it does\n> just make you pause for thought. Anyway, \"zero vector\" would have\n> caused my brain cells slightly less cognitive dislocation!\n\nNow that I'm reading the Matrix documentation class, I have a sneaky\nsuspicion that Matrix documentation was duplicated to Vector\ndocumentation ;-)\n\nAs this is a default constructor - the values will be zero - so that's\ncertainly not an identify vector! A Zero vector sounds far more\nreasonable.\n\n--\nKieran\n\n\n> \n> Thanks\n> David","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 18CF9C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jun 2024 10:50:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 29AF26545E;\n\tMon, 10 Jun 2024 12:50:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFB8E65446\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 12:50:01 +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 E9A32512;\n\tMon, 10 Jun 2024 12:49:49 +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=\"amvi+qQS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718016590;\n\tbh=D92zc381Di6RuMYW6rbniwPIJNCFKZ9r1kVqxzxAIkA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=amvi+qQSL0hLmoKs8F0JXnwmBkpFfhYfrC1QoGLpsi/1fBKbGgvap5lAM+/annSRN\n\t8wZleeHMHdZMbXdeuxCIHO5W8MV6ZjAh3QfBBJovzEFxj3fbGvWz01tTPsCrP0EqUi\n\tO3373fgnW3BqW63GUHo6Vs3AdyoLvxtvLYzL50SE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<CAHW6GYJhyJOUytNwPs0_5bYKFtSz8RRR7S2gqktkkrw=jnk9ew@mail.gmail.com>","References":"<20240607084301.2791258-1-paul.elder@ideasonboard.com>\n\t<20240607084301.2791258-2-paul.elder@ideasonboard.com>\n\t<171801489599.1489778.10050343972693994261@ping.linuxembedded.co.uk>\n\t<CAHW6GYJhyJOUytNwPs0_5bYKFtSz8RRR7S2gqktkkrw=jnk9ew@mail.gmail.com>","Subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","To":"David Plowman <david.plowman@raspberrypi.com>","Date":"Mon, 10 Jun 2024 11:49:58 +0100","Message-ID":"<171801659897.2248009.14495218581998625373@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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29805,"web_url":"https://patchwork.libcamera.org/comment/29805/","msgid":"<yrtxehenpranqnxn3s6n4xnhvdvuid35ibtniqydjrszgqibur@gxobfq7g7s2o>","date":"2024-06-10T12:41:59","subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthank you for the patch.\nKieran already commented some points, which I won't repeat...\nI only add a few more :-)\n\nOn Fri, Jun 07, 2024 at 05:42:58PM +0900, Paul Elder wrote:\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 <paul.elder@ideasonboard.com>\n> \n> ---\n> No change in v6\n> \n> New in v5\n> ---\n>  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++\n>  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 368 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 000000000..bdc3c447c\n> --- /dev/null\n> +++ b/src/ipa/libipa/vector.cpp\n> @@ -0,0 +1,162 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Vector and related operations\n> + */\n> +\n> +#include \"vector.h\"\n> +\n> +#include <libcamera/base/log.h>\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 R Number of rows in the vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::Vector()\n> + * \\brief Construct an identity vector\n> + */\n> +\n> +/**\n> + * \\fn Vector::Vector(const std::array<T, R> &data)\n> + * \\brief Construct vector from supplied data\n> + * \\param data Data from which to construct a vector\n> + *\n> + * \\a data is a one-dimensional vector and will be turned into a vector in\n> + * row-major order. The size of \\a data must be equal to the product of the\n> + * number of rows and columns of the vector (RxC).\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 product of the number of rows and\n> + * columns of the vector (RxC).\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 Vector::toString\n> + * \\brief Assemble and return a string describing the vector\n> + * \\return A string describing the vector\n> + */\n\nIs this still needed when we have ostream << () ? I don't think we need\ntwo functions with the same purpose. Maybe move the imple into ostream\n<< ()?\n\n> +\n> +/**\n> + * \\fn T Vector::operator[](size_t i) const\n> + * \\brief Index to a row in the vector\n> + * \\param i Index of row to retrieve\n> + *\n> + * This operator[] returns a Span, which can then be indexed into again with\n> + * another operator[], allowing a convenient m[i][j] to access elements of the\n> + * vector. Note that the lifetime of the Span returned by this first-level\n> + * operator[] is bound to that of the Vector itself, so it is not recommended\n> + * to save the Span that is the result of this operator[].\n> + *\n> + * \\return Row \\a i from the vector, as a Span\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<T, R> &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 Scale up the vector\n\nI stumbled over the wording \"Scale up\". I would just say \"Multiply the\nvector by a scalar\"\n\n> + * \\param[in] factor The factor\n> + * \\return The vector scaled up by \\a factor\n> + */\n> +\n> +/**\n> + * \\fn Vector::operator/()\n> + * \\brief Scale down the vector\n\nMaybe: Divide the vector by a scalar.\n\nWith the comments from Kieran resolved, I'm fine with it.\n\nReviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> \n\nCheers,\nStefan\n\n> + * \\param[in] factor The factor\n> + * \\return The vector scaled down 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<T, R> &lhs, const Vector<T, R> &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<T, R> &lhs, const Vector<T, R> &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 000000000..87aad28b5\n> --- /dev/null\n> +++ b/src/ipa/libipa/vector.h\n> @@ -0,0 +1,206 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Vector and related operations\n> + */\n> +#pragma once\n> +\n> +#include <algorithm>\n> +#include <array>\n> +#include <cmath>\n> +#include <sstream>\n> +\n> +#include <libcamera/base/log.h>\n> +#include <libcamera/base/span.h>\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<typename T, unsigned int R,\n> +\t std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>\n> +#else\n> +template<typename T, unsigned int R>\n> +#endif /* __DOXYGEN__ */\n> +class Vector\n> +{\n> +public:\n> +\tVector() = default;\n> +\n> +\tVector(const std::array<T, R> &data)\n> +\t{\n> +\t\tASSERT(data.size() == R);\n> +\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tdata_[i] = data[i];\n> +\t}\n> +\n> +\t~Vector() = default;\n> +\n> +\tint readYaml(const libcamera::YamlObject &yaml)\n> +\t{\n> +\t\tif (yaml.size() != R) {\n> +\t\t\tLOG(Vector, Error)\n> +\t\t\t\t<< \"Wrong number of values in vector: expected \"\n> +\t\t\t\t<< R << \", got \" << yaml.size();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tunsigned int i = 0;\n> +\t\tfor (const auto &x : yaml.asList()) {\n> +\t\t\tauto value = x.get<T>();\n> +\t\t\tif (!value) {\n> +\t\t\t\tLOG(Vector, Error) << \"Failed to read vector value\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tdata_[i++] = *value;\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tconst std::string toString() const\n> +\t{\n> +\t\tstd::stringstream out;\n> +\n> +\t\tout << \"Vector { \";\n> +\t\tfor (unsigned int i = 0; i < R; i++) {\n> +\t\t\tout << (*this)[i];\n> +\t\t\tout << ((i + 1 < R) ? \", \" : \" \");\n> +\t\t}\n> +\t\tout << \" }\";\n> +\n> +\t\treturn out.str();\n> +\t}\n> +\n> +\tconst T operator[](size_t i) const\n> +\t{\n> +\t\treturn data_[i];\n> +\t}\n> +\n> +\tT &operator[](size_t i)\n> +\t{\n> +\t\treturn data_[i];\n> +\t}\n> +\n> +\tconst T x() const\n> +\t{\n> +\t\treturn data_[0];\n> +\t}\n> +\n> +\tconst T y() const\n> +\t{\n> +\t\treturn data_[1];\n> +\t}\n> +\n> +\tconstexpr Vector<T, R> operator-() const\n> +\t{\n> +\t\tVector<T, R> ret;\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tret[i] = -data_[i];\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconstexpr Vector<T, R> operator-(const Vector<T, R> &other) const\n> +\t{\n> +\t\tVector<T, R> ret;\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tret[i] = data_[i] - other[i];\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconstexpr Vector<T, R> operator+(const Vector<T, R> &other) const\n> +\t{\n> +\t\tVector<T, R> ret;\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tret[i] = data_[i] + other[i];\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconstexpr T operator*(const Vector<T, R> &other) const\n> +\t{\n> +\t\tT ret = 0;\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tret += data_[i] * other[i];\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconstexpr Vector<T, R> operator*(T factor) const\n> +\t{\n> +\t\tVector<T, R> ret;\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tret[i] = data_[i] * factor;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconstexpr Vector<T, R> operator/(T factor) const\n> +\t{\n> +\t\tVector<T, R> ret;\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tret[i] = data_[i] / factor;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconstexpr T len2() const\n> +\t{\n> +\t\tT ret = 0;\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tret += data_[i] * data_[i];\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconstexpr double len() const\n> +\t{\n> +\t\treturn std::sqrt(len2());\n> +\t}\n> +\n> +private:\n> +\tstd::array<T, R> data_;\n> +};\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, unsigned int R,\n> +\t std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#endif /* __DOXYGEN__ */\n> +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> +{\n> +\tfor (unsigned int i = 0; i < R; i++)\n> +\t\tif (lhs[i] != rhs[i])\n> +\t\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, unsigned int R,\n> +\t std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#endif /* __DOXYGEN__ */\n> +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> +{\n> +\tfor (unsigned int i = 0; i < R; i++)\n> +\t\tif (lhs[i] != rhs[i])\n> +\t\t\treturn true;\n> +\n> +\treturn false;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, unsigned int R>\n> +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)\n> +{\n> +\tout << v.toString();\n> +\treturn out;\n> +}\n> +#endif /* __DOXYGEN__ */\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.39.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A22D1C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jun 2024 12:42:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4820165461;\n\tMon, 10 Jun 2024 14:42:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7098B634D5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 14:42:02 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:5729:5358:82cd:263f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FD0B230;\n\tMon, 10 Jun 2024 14:41:50 +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=\"QGO5ITTF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718023310;\n\tbh=0GJQ+gzwuk/8RcS9mXLj2bPgUFTHV5QRdR3Zh8vWKU0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QGO5ITTFos4Mk5tH5Qib6BjfauI4V2L5g3ORraDO39JApE6DBjakb3358+jLH5a3L\n\tKz735F+9if38wZ8tBTtC1KLI7lgzRgc1zdSHnf8TkTzxtFhpTzCWXISJD1zGyFogoT\n\tiUlXv9T+E/80+jlcNFBA83ZoihYQwzHSJr6XvDDI=","Date":"Mon, 10 Jun 2024 14:41:59 +0200","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","Message-ID":"<yrtxehenpranqnxn3s6n4xnhvdvuid35ibtniqydjrszgqibur@gxobfq7g7s2o>","References":"<20240607084301.2791258-1-paul.elder@ideasonboard.com>\n\t<20240607084301.2791258-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240607084301.2791258-2-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29808,"web_url":"https://patchwork.libcamera.org/comment/29808/","msgid":"<Zmb4JFenExFePwCz@pyrite.rasen.tech>","date":"2024-06-10T12:57:08","subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Jun 10, 2024 at 11:21:35AM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-06-07 09:42:58)\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 <paul.elder@ideasonboard.com>\n> > \n> > ---\n> > No change in v6\n> > \n> > New in v5\n> > ---\n> >  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++\n> >  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 368 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 000000000..bdc3c447c\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/vector.cpp\n> > @@ -0,0 +1,162 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> > + *\n> > + * Vector and related operations\n> > + */\n> > +\n> > +#include \"vector.h\"\n> > +\n> > +#include <libcamera/base/log.h>\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 R Number of rows in the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::Vector()\n> > + * \\brief Construct an identity vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::Vector(const std::array<T, R> &data)\n> > + * \\brief Construct vector from supplied data\n> > + * \\param data Data from which to construct a vector\n> > + *\n> > + * \\a data is a one-dimensional vector and will be turned into a vector in\n> > + * row-major order. The size of \\a data must be equal to the product of the\n> > + * number of rows and columns of the vector (RxC).\n> \n> Rows and columns of a vector?\n\nOops, I copied from Matrix and forgot to update the documentation...\n\n> \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 product of the number of rows and\n> > + * columns of the vector (RxC).\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 Vector::toString\n> > + * \\brief Assemble and return a string describing the vector\n> > + * \\return A string describing the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn T Vector::operator[](size_t i) const\n> > + * \\brief Index to a row in the vector\n> > + * \\param i Index of row to retrieve\n> > + *\n> > + * This operator[] returns a Span, which can then be indexed into again with\n> > + * another operator[], allowing a convenient m[i][j] to access elements of the\n> > + * vector. Note that the lifetime of the Span returned by this first-level\n> > + * operator[] is bound to that of the Vector itself, so it is not recommended\n> > + * to save the Span that is the result of this operator[].\n> > + *\n> > + * \\return Row \\a i from the vector, as a Span\n> \n> Are these needed for a vector? I don't understand the indexing of a\n> vector? I could understand on a matrix - but isn't a vector just a set\n> of coordinates that represent direction/magnitude?\n> \n> I don't understand what m[i][j] is used for ? (or represents?)\n> \n> Perhaps the hint 'm' means this is a left over and needs to be removed?\n\nYeep...\n\n> \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> These I understand :-)\n> \n> \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<T, R> &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 Scale up the vector\n> > + * \\param[in] factor The factor\n> > + * \\return The vector scaled up by \\a factor\n> > + */\n> > +\n> > +/**\n> > + * \\fn Vector::operator/()\n> > + * \\brief Scale down the vector\n> > + * \\param[in] factor The factor\n> > + * \\return The vector scaled down 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> I think all of those probably deserve at least some level of a unit\n> test. How come we don't seem to have tests for libipa - Seems like the\n> sort of thing we should have... Particularly as you've got operations on\n> differing types above.\n> \n> \n> \n> > +\n> > +/**\n> > + * \\fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &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<T, R> &lhs, const Vector<T, R> &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 000000000..87aad28b5\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/vector.h\n> > @@ -0,0 +1,206 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> > + *\n> > + * Vector and related operations\n> > + */\n> > +#pragma once\n> > +\n> > +#include <algorithm>\n> > +#include <array>\n> > +#include <cmath>\n> > +#include <sstream>\n> > +\n> > +#include <libcamera/base/log.h>\n> > +#include <libcamera/base/span.h>\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<typename T, unsigned int R,\n> > +        std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>\n> > +#else\n> > +template<typename T, unsigned int R>\n> \n> Do we anticipate 3d vectors? (or other dimesions?)\n> \n> How about defaulting R = 2 so that a 2d vector is the default case?\n> \n> Then we can use\n> \tVector<int> v = {x, y};\n\nI didn't think that Vector<int, 2> was a very high cost? It's only two\nmore characters.\n\n> \n> or such. (Or some how typedef/using to create Vector2d, Vector3d?)\n\nThat would be the preferred alternative imo.\n\n> \n> Is R an appropriate name for a vector? Is D for dimensions any better?\n> Or is it still better to call it Rows?\n\nHm, maybe dimensions would be better...\n\n> \n> \n> \n> > +#endif /* __DOXYGEN__ */\n> > +class Vector\n> > +{\n> > +public:\n> > +       Vector() = default;\n> > +\n> > +       Vector(const std::array<T, R> &data)\n> > +       {\n> > +               ASSERT(data.size() == R);\n> \n> Optional, but as we provide direct accessors for x() and y() perhaps:\n> \n> \tASSERT(R >= 2);\n\nThat's already enforced by SFINAE in the class definition... unless\nSFINAE doesn't kick in in this case...?\n\n> \n> That might stop accidentally creating a vector of one dimesion..\n> \n> \n> > +\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       data_[i] = data[i];\n> > +       }\n> > +\n> > +       ~Vector() = default;\n> > +\n> > +       int readYaml(const libcamera::YamlObject &yaml)\n> > +       {\n> > +               if (yaml.size() != R) {\n> > +                       LOG(Vector, Error)\n> > +                               << \"Wrong number of values in vector: expected \"\n> > +                               << R << \", 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<T>();\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 std::string toString() const\n> > +       {\n> > +               std::stringstream out;\n> > +\n> > +               out << \"Vector { \";\n> > +               for (unsigned int i = 0; i < R; i++) {\n> > +                       out << (*this)[i];\n> > +                       out << ((i + 1 < R) ? \", \" : \" \");\n> > +               }\n> > +               out << \" }\";\n> > +\n> > +               return out.str();\n> > +       }\n> > +\n> > +       const T operator[](size_t i) const\n> > +       {\n> \n> ASSERT here if out of bounds?\n\nAh, right.\n\n> \n> > +               return data_[i];\n> > +       }\n> > +\n> > +       T &operator[](size_t i)\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<T, R> operator-() const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = -data_[i];\n> > +               return ret;\n> > +       }\n> \n> How would this be used? (differently than below?)\n\nThis is a unary negation operator, the below is a binary subtration\noperator (did I forget to document it...?)\n\n> \n> Unit tests would help here I think to confirm and validate intended\n> purposes.\n> \n\nI think I'll do that on top (along with the ones for matrices)...\n\n\nThanks,\n\nPaul\n\n> > +\n> > +       constexpr Vector<T, R> operator-(const Vector<T, R> &other) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = data_[i] - other[i];\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr Vector<T, R> operator+(const Vector<T, R> &other) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = data_[i] + other[i];\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr T operator*(const Vector<T, R> &other) const\n> > +       {\n> > +               T ret = 0;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret += data_[i] * other[i];\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr Vector<T, R> operator*(T factor) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; i++)\n> > +                       ret[i] = data_[i] * factor;\n> > +               return ret;\n> > +       }\n> > +\n> > +       constexpr Vector<T, R> operator/(T factor) const\n> > +       {\n> > +               Vector<T, R> ret;\n> > +               for (unsigned int i = 0; i < R; 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 < R; 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<T, R> data_;\n> > +};\n> \n> \n> > +\n> > +#ifndef __DOXYGEN__\n> > +template<typename T, unsigned int R,\n> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> > +#endif /* __DOXYGEN__ */\n> > +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> > +{\n> > +       for (unsigned int i = 0; i < R; i++)\n> > +               if (lhs[i] != rhs[i])\n> > +                       return false;\n> > +\n> > +       return true;\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template<typename T, unsigned int R,\n> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> > +#endif /* __DOXYGEN__ */\n> > +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)\n> > +{\n> > +       for (unsigned int i = 0; i < R; 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<typename T, unsigned int R>\n> > +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)\n> > +{\n> > +       out << v.toString();\n> > +       return out;\n> > +}\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > +} /* namespace libcamera */\n> > -- \n> > 2.39.2\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 8E3CCC31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jun 2024 12:57:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4DD5A6546A;\n\tMon, 10 Jun 2024 14:57:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2A296545E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 14:57:14 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F2C3A397;\n\tMon, 10 Jun 2024 14:57:01 +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=\"v/BNnzqv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718024222;\n\tbh=DHhhAsmrQB3Y3vYmMC21wKzOQHcGGN/pcsvqmCIddsI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v/BNnzqvf9nrrdyYnhdyuFXW6sH+csEwlQJWFqfrS3LZ/csoQ2P9N2PbEig7WKKJl\n\tYFks7qqmRhCBUhcKdOFS3foQvsIPVIdjzRhtFpHnk3NCMPigLrH38l7uUWeyZHdvJB\n\tZZ6KhnXcKOhBiFyZTCJUTPMh0ILsElhtXoqUb/qM=","Date":"Mon, 10 Jun 2024 21:57:08 +0900","From":"Paul Elder <paul.elder@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v6 1/4] ipa: libipa: Add Vector class","Message-ID":"<Zmb4JFenExFePwCz@pyrite.rasen.tech>","References":"<20240607084301.2791258-1-paul.elder@ideasonboard.com>\n\t<20240607084301.2791258-2-paul.elder@ideasonboard.com>\n\t<171801489599.1489778.10050343972693994261@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<171801489599.1489778.10050343972693994261@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]