[v6,1/4] ipa: libipa: Add Vector class
diff mbox series

Message ID 20240607084301.2791258-2-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: Move Pwl from Raspberry Pi to libipa
Related show

Commit Message

Paul Elder June 7, 2024, 8:42 a.m. UTC
Add a vector class to libipa. The original purpose of this is to replace
the floating-point Point class that Raspberry Pi used in their Pwl, as
that implementation of Point seemed more akin to a Vector than a Point.

This is added to libipa instead of to geometry.h to avoid public API
issues, plus this is not expected to be needed by applications.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
No change in v6

New in v5
---
 src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++
 src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 368 insertions(+)
 create mode 100644 src/ipa/libipa/vector.cpp
 create mode 100644 src/ipa/libipa/vector.h

Comments

Kieran Bingham June 10, 2024, 10:21 a.m. UTC | #1
Quoting Paul Elder (2024-06-07 09:42:58)
> Add a vector class to libipa. The original purpose of this is to replace
> the floating-point Point class that Raspberry Pi used in their Pwl, as
> that implementation of Point seemed more akin to a Vector than a Point.
> 
> This is added to libipa instead of to geometry.h to avoid public API
> issues, plus this is not expected to be needed by applications.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> No change in v6
> 
> New in v5
> ---
>  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++
>  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 368 insertions(+)
>  create mode 100644 src/ipa/libipa/vector.cpp
>  create mode 100644 src/ipa/libipa/vector.h
> 
> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> new file mode 100644
> index 000000000..bdc3c447c
> --- /dev/null
> +++ b/src/ipa/libipa/vector.cpp
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * Vector and related operations
> + */
> +
> +#include "vector.h"
> +
> +#include <libcamera/base/log.h>
> +
> +/**
> + * \file vector.h
> + * \brief Vector class
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Vector)
> +
> +namespace ipa {
> +
> +/**
> + * \class Vector
> + * \brief Vector class
> + * \tparam T Type of numerical values to be stored in the vector
> + * \tparam R Number of rows in the vector
> + */
> +
> +/**
> + * \fn Vector::Vector()
> + * \brief Construct an identity vector
> + */
> +
> +/**
> + * \fn Vector::Vector(const std::array<T, R> &data)
> + * \brief Construct vector from supplied data
> + * \param data Data from which to construct a vector
> + *
> + * \a data is a one-dimensional vector and will be turned into a vector in
> + * row-major order. The size of \a data must be equal to the product of the
> + * number of rows and columns of the vector (RxC).

Rows and columns of a vector?

> + */
> +
> +/**
> + * \fn Vector::readYaml
> + * \brief Populate the vector with yaml data
> + * \param yaml Yaml data to populate the vector with
> + *
> + * Any existing data in the vector will be overwritten. The size of the data
> + * read from \a yaml must be equal to the product of the number of rows and
> + * columns of the vector (RxC).
> + *
> + * The yaml data is expected to be a list with elements of type T.
> + *
> + * \return 0 on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn Vector::toString
> + * \brief Assemble and return a string describing the vector
> + * \return A string describing the vector
> + */
> +
> +/**
> + * \fn T Vector::operator[](size_t i) const
> + * \brief Index to a row in the vector
> + * \param i Index of row to retrieve
> + *
> + * This operator[] returns a Span, which can then be indexed into again with
> + * another operator[], allowing a convenient m[i][j] to access elements of the
> + * vector. Note that the lifetime of the Span returned by this first-level
> + * operator[] is bound to that of the Vector itself, so it is not recommended
> + * to save the Span that is the result of this operator[].
> + *
> + * \return Row \a i from the vector, as a Span

Are these needed for a vector? I don't understand the indexing of a
vector? I could understand on a matrix - but isn't a vector just a set
of coordinates that represent direction/magnitude?

I don't understand what m[i][j] is used for ? (or represents?)

Perhaps the hint 'm' means this is a left over and needs to be removed?

> + */
> +
> +/**
> + * \fn T &Vector::operator[](size_t i)
> + * \copydoc Vector::operator[](size_t i) const
> + */
> +
> +/**
> + * \fn Vector::x()
> + * \brief Convenience function to access the first element of the vector
> + */
> +
> +/**
> + * \fn Vector::y()
> + * \brief Convenience function to access the second element of the vector
> + */

These I understand :-)



> +
> +/**
> + * \fn Vector::operator-() const
> + * \brief Negate a Vector by negating both all of its coordinates
> + * \return The negated vector
> + */
> +
> +/**
> + * \fn Vector::operator-(Vector const &other) const
> + * \brief Subtract one vector from another
> + * \param[in] other The other vector
> + * \return The difference of \a other from this vector
> + */
> +
> +/**
> + * \fn Vector::operator+()
> + * \brief Add two vectors together
> + * \param[in] other The other vector
> + * \return The sum of the two vectors
> + */
> +
> +/**
> + * \fn Vector::operator*(const Vector<T, R> &other) const
> + * \brief Compute the dot product
> + * \param[in] other The other vector
> + * \return The dot product of the two vectors
> + */
> +
> +/**
> + * \fn Vector::operator*(T factor) const
> + * \brief Scale up the vector
> + * \param[in] factor The factor
> + * \return The vector scaled up by \a factor
> + */
> +
> +/**
> + * \fn Vector::operator/()
> + * \brief Scale down the vector
> + * \param[in] factor The factor
> + * \return The vector scaled down by \a factor
> + */
> +
> +/**
> + * \fn Vector::len2()
> + * \brief Get the squared length of the vector
> + * \return The squared length of the vector
> + */
> +
> +/**
> + * \fn Vector::len()
> + * \brief Get the length of the vector
> + * \return The length of the vector
> + */

I think all of those probably deserve at least some level of a unit
test. How come we don't seem to have tests for libipa - Seems like the
sort of thing we should have... Particularly as you've got operations on
differing types above.



> +
> +/**
> + * \fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> + * \brief Compare vectors for equality
> + * \return True if the two vectors are equal, false otherwise
> + */
> +
> +/**
> + * \fn bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> + * \brief Compare vectors for inequality
> + * \return True if the two vectors are not equal, false otherwise
> + */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> new file mode 100644
> index 000000000..87aad28b5
> --- /dev/null
> +++ b/src/ipa/libipa/vector.h
> @@ -0,0 +1,206 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * Vector and related operations
> + */
> +#pragma once
> +
> +#include <algorithm>
> +#include <array>
> +#include <cmath>
> +#include <sstream>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/span.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Vector)
> +
> +namespace ipa {
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R,
> +        std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>
> +#else
> +template<typename T, unsigned int R>

Do we anticipate 3d vectors? (or other dimesions?)

How about defaulting R = 2 so that a 2d vector is the default case?

Then we can use
	Vector<int> v = {x, y};

or such. (Or some how typedef/using to create Vector2d, Vector3d?)

Is R an appropriate name for a vector? Is D for dimensions any better?
Or is it still better to call it Rows?



> +#endif /* __DOXYGEN__ */
> +class Vector
> +{
> +public:
> +       Vector() = default;
> +
> +       Vector(const std::array<T, R> &data)
> +       {
> +               ASSERT(data.size() == R);

Optional, but as we provide direct accessors for x() and y() perhaps:

	ASSERT(R >= 2);

That might stop accidentally creating a vector of one dimesion..


> +
> +               for (unsigned int i = 0; i < R; i++)
> +                       data_[i] = data[i];
> +       }
> +
> +       ~Vector() = default;
> +
> +       int readYaml(const libcamera::YamlObject &yaml)
> +       {
> +               if (yaml.size() != R) {
> +                       LOG(Vector, Error)
> +                               << "Wrong number of values in vector: expected "
> +                               << R << ", got " << yaml.size();
> +                       return -EINVAL;
> +               }
> +
> +               unsigned int i = 0;
> +               for (const auto &x : yaml.asList()) {
> +                       auto value = x.get<T>();
> +                       if (!value) {
> +                               LOG(Vector, Error) << "Failed to read vector value";
> +                               return -EINVAL;
> +                       }
> +
> +                       data_[i++] = *value;
> +               }
> +
> +               return 0;
> +       }
> +
> +       const std::string toString() const
> +       {
> +               std::stringstream out;
> +
> +               out << "Vector { ";
> +               for (unsigned int i = 0; i < R; i++) {
> +                       out << (*this)[i];
> +                       out << ((i + 1 < R) ? ", " : " ");
> +               }
> +               out << " }";
> +
> +               return out.str();
> +       }
> +
> +       const T operator[](size_t i) const
> +       {

ASSERT here if out of bounds?

> +               return data_[i];
> +       }
> +
> +       T &operator[](size_t i)
> +       {
> +               return data_[i];
> +       }
> +
> +       const T x() const
> +       {
> +               return data_[0];
> +       }
> +
> +       const T y() const
> +       {
> +               return data_[1];
> +       }
> +
> +       constexpr Vector<T, R> operator-() const
> +       {
> +               Vector<T, R> ret;
> +               for (unsigned int i = 0; i < R; i++)
> +                       ret[i] = -data_[i];
> +               return ret;
> +       }

How would this be used? (differently than below?)

Unit tests would help here I think to confirm and validate intended
purposes.

> +
> +       constexpr Vector<T, R> operator-(const Vector<T, R> &other) const
> +       {
> +               Vector<T, R> ret;
> +               for (unsigned int i = 0; i < R; i++)
> +                       ret[i] = data_[i] - other[i];
> +               return ret;
> +       }
> +
> +       constexpr Vector<T, R> operator+(const Vector<T, R> &other) const
> +       {
> +               Vector<T, R> ret;
> +               for (unsigned int i = 0; i < R; i++)
> +                       ret[i] = data_[i] + other[i];
> +               return ret;
> +       }
> +
> +       constexpr T operator*(const Vector<T, R> &other) const
> +       {
> +               T ret = 0;
> +               for (unsigned int i = 0; i < R; i++)
> +                       ret += data_[i] * other[i];
> +               return ret;
> +       }
> +
> +       constexpr Vector<T, R> operator*(T factor) const
> +       {
> +               Vector<T, R> ret;
> +               for (unsigned int i = 0; i < R; i++)
> +                       ret[i] = data_[i] * factor;
> +               return ret;
> +       }
> +
> +       constexpr Vector<T, R> operator/(T factor) const
> +       {
> +               Vector<T, R> ret;
> +               for (unsigned int i = 0; i < R; i++)
> +                       ret[i] = data_[i] / factor;
> +               return ret;
> +       }
> +
> +       constexpr T len2() const
> +       {
> +               T ret = 0;
> +               for (unsigned int i = 0; i < R; i++)
> +                       ret += data_[i] * data_[i];
> +               return ret;
> +       }
> +
> +       constexpr double len() const
> +       {
> +               return std::sqrt(len2());
> +       }
> +
> +private:
> +       std::array<T, R> data_;
> +};


> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R,
> +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +#endif /* __DOXYGEN__ */
> +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> +{
> +       for (unsigned int i = 0; i < R; i++)
> +               if (lhs[i] != rhs[i])
> +                       return false;
> +
> +       return true;
> +}
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R,
> +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +#endif /* __DOXYGEN__ */
> +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> +{
> +       for (unsigned int i = 0; i < R; i++)
> +               if (lhs[i] != rhs[i])
> +                       return true;
> +
> +       return false;
> +}
> +
> +} /* namespace ipa */
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R>
> +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)
> +{
> +       out << v.toString();
> +       return out;
> +}
> +#endif /* __DOXYGEN__ */
> +
> +} /* namespace libcamera */
> -- 
> 2.39.2
>
David Plowman June 10, 2024, 10:33 a.m. UTC | #2
Hi Paul

Thank you for this patch.

On Mon, 10 Jun 2024 at 11:21, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Paul Elder (2024-06-07 09:42:58)
> > Add a vector class to libipa. The original purpose of this is to replace
> > the floating-point Point class that Raspberry Pi used in their Pwl, as
> > that implementation of Point seemed more akin to a Vector than a Point.
> >
> > This is added to libipa instead of to geometry.h to avoid public API
> > issues, plus this is not expected to be needed by applications.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > No change in v6
> >
> > New in v5
> > ---
> >  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++
> >  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 368 insertions(+)
> >  create mode 100644 src/ipa/libipa/vector.cpp
> >  create mode 100644 src/ipa/libipa/vector.h
> >
> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > new file mode 100644
> > index 000000000..bdc3c447c
> > --- /dev/null
> > +++ b/src/ipa/libipa/vector.cpp
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * Vector and related operations
> > + */
> > +
> > +#include "vector.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +/**
> > + * \file vector.h
> > + * \brief Vector class
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Vector)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class Vector
> > + * \brief Vector class
> > + * \tparam T Type of numerical values to be stored in the vector
> > + * \tparam R Number of rows in the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::Vector()
> > + * \brief Construct an identity vector

I just wanted to comment that this expression "identity vector" had me
confused for a moment. I assume we just mean the "zero vector" which,
it's true, is the identity element under the addition operation. But
then we often multiply vectors by scalars, or matrices, so it does
just make you pause for thought. Anyway, "zero vector" would have
caused my brain cells slightly less cognitive dislocation!

Thanks
David

> > + */
> > +
> > +/**
> > + * \fn Vector::Vector(const std::array<T, R> &data)
> > + * \brief Construct vector from supplied data
> > + * \param data Data from which to construct a vector
> > + *
> > + * \a data is a one-dimensional vector and will be turned into a vector in
> > + * row-major order. The size of \a data must be equal to the product of the
> > + * number of rows and columns of the vector (RxC).
>
> Rows and columns of a vector?
>
> > + */
> > +
> > +/**
> > + * \fn Vector::readYaml
> > + * \brief Populate the vector with yaml data
> > + * \param yaml Yaml data to populate the vector with
> > + *
> > + * Any existing data in the vector will be overwritten. The size of the data
> > + * read from \a yaml must be equal to the product of the number of rows and
> > + * columns of the vector (RxC).
> > + *
> > + * The yaml data is expected to be a list with elements of type T.
> > + *
> > + * \return 0 on success, negative error code otherwise
> > + */
> > +
> > +/**
> > + * \fn Vector::toString
> > + * \brief Assemble and return a string describing the vector
> > + * \return A string describing the vector
> > + */
> > +
> > +/**
> > + * \fn T Vector::operator[](size_t i) const
> > + * \brief Index to a row in the vector
> > + * \param i Index of row to retrieve
> > + *
> > + * This operator[] returns a Span, which can then be indexed into again with
> > + * another operator[], allowing a convenient m[i][j] to access elements of the
> > + * vector. Note that the lifetime of the Span returned by this first-level
> > + * operator[] is bound to that of the Vector itself, so it is not recommended
> > + * to save the Span that is the result of this operator[].
> > + *
> > + * \return Row \a i from the vector, as a Span
>
> Are these needed for a vector? I don't understand the indexing of a
> vector? I could understand on a matrix - but isn't a vector just a set
> of coordinates that represent direction/magnitude?
>
> I don't understand what m[i][j] is used for ? (or represents?)
>
> Perhaps the hint 'm' means this is a left over and needs to be removed?
>
> > + */
> > +
> > +/**
> > + * \fn T &Vector::operator[](size_t i)
> > + * \copydoc Vector::operator[](size_t i) const
> > + */
> > +
> > +/**
> > + * \fn Vector::x()
> > + * \brief Convenience function to access the first element of the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::y()
> > + * \brief Convenience function to access the second element of the vector
> > + */
>
> These I understand :-)
>
>
>
> > +
> > +/**
> > + * \fn Vector::operator-() const
> > + * \brief Negate a Vector by negating both all of its coordinates
> > + * \return The negated vector
> > + */
> > +
> > +/**
> > + * \fn Vector::operator-(Vector const &other) const
> > + * \brief Subtract one vector from another
> > + * \param[in] other The other vector
> > + * \return The difference of \a other from this vector
> > + */
> > +
> > +/**
> > + * \fn Vector::operator+()
> > + * \brief Add two vectors together
> > + * \param[in] other The other vector
> > + * \return The sum of the two vectors
> > + */
> > +
> > +/**
> > + * \fn Vector::operator*(const Vector<T, R> &other) const
> > + * \brief Compute the dot product
> > + * \param[in] other The other vector
> > + * \return The dot product of the two vectors
> > + */
> > +
> > +/**
> > + * \fn Vector::operator*(T factor) const
> > + * \brief Scale up the vector
> > + * \param[in] factor The factor
> > + * \return The vector scaled up by \a factor
> > + */
> > +
> > +/**
> > + * \fn Vector::operator/()
> > + * \brief Scale down the vector
> > + * \param[in] factor The factor
> > + * \return The vector scaled down by \a factor
> > + */
> > +
> > +/**
> > + * \fn Vector::len2()
> > + * \brief Get the squared length of the vector
> > + * \return The squared length of the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::len()
> > + * \brief Get the length of the vector
> > + * \return The length of the vector
> > + */
>
> I think all of those probably deserve at least some level of a unit
> test. How come we don't seem to have tests for libipa - Seems like the
> sort of thing we should have... Particularly as you've got operations on
> differing types above.
>
>
>
> > +
> > +/**
> > + * \fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > + * \brief Compare vectors for equality
> > + * \return True if the two vectors are equal, false otherwise
> > + */
> > +
> > +/**
> > + * \fn bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > + * \brief Compare vectors for inequality
> > + * \return True if the two vectors are not equal, false otherwise
> > + */
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > new file mode 100644
> > index 000000000..87aad28b5
> > --- /dev/null
> > +++ b/src/ipa/libipa/vector.h
> > @@ -0,0 +1,206 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * Vector and related operations
> > + */
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <array>
> > +#include <cmath>
> > +#include <sstream>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/base/span.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(Vector)
> > +
> > +namespace ipa {
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > +        std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>
> > +#else
> > +template<typename T, unsigned int R>
>
> Do we anticipate 3d vectors? (or other dimesions?)
>
> How about defaulting R = 2 so that a 2d vector is the default case?
>
> Then we can use
>         Vector<int> v = {x, y};
>
> or such. (Or some how typedef/using to create Vector2d, Vector3d?)
>
> Is R an appropriate name for a vector? Is D for dimensions any better?
> Or is it still better to call it Rows?
>
>
>
> > +#endif /* __DOXYGEN__ */
> > +class Vector
> > +{
> > +public:
> > +       Vector() = default;
> > +
> > +       Vector(const std::array<T, R> &data)
> > +       {
> > +               ASSERT(data.size() == R);
>
> Optional, but as we provide direct accessors for x() and y() perhaps:
>
>         ASSERT(R >= 2);
>
> That might stop accidentally creating a vector of one dimesion..
>
>
> > +
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       data_[i] = data[i];
> > +       }
> > +
> > +       ~Vector() = default;
> > +
> > +       int readYaml(const libcamera::YamlObject &yaml)
> > +       {
> > +               if (yaml.size() != R) {
> > +                       LOG(Vector, Error)
> > +                               << "Wrong number of values in vector: expected "
> > +                               << R << ", got " << yaml.size();
> > +                       return -EINVAL;
> > +               }
> > +
> > +               unsigned int i = 0;
> > +               for (const auto &x : yaml.asList()) {
> > +                       auto value = x.get<T>();
> > +                       if (!value) {
> > +                               LOG(Vector, Error) << "Failed to read vector value";
> > +                               return -EINVAL;
> > +                       }
> > +
> > +                       data_[i++] = *value;
> > +               }
> > +
> > +               return 0;
> > +       }
> > +
> > +       const std::string toString() const
> > +       {
> > +               std::stringstream out;
> > +
> > +               out << "Vector { ";
> > +               for (unsigned int i = 0; i < R; i++) {
> > +                       out << (*this)[i];
> > +                       out << ((i + 1 < R) ? ", " : " ");
> > +               }
> > +               out << " }";
> > +
> > +               return out.str();
> > +       }
> > +
> > +       const T operator[](size_t i) const
> > +       {
>
> ASSERT here if out of bounds?
>
> > +               return data_[i];
> > +       }
> > +
> > +       T &operator[](size_t i)
> > +       {
> > +               return data_[i];
> > +       }
> > +
> > +       const T x() const
> > +       {
> > +               return data_[0];
> > +       }
> > +
> > +       const T y() const
> > +       {
> > +               return data_[1];
> > +       }
> > +
> > +       constexpr Vector<T, R> operator-() const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = -data_[i];
> > +               return ret;
> > +       }
>
> How would this be used? (differently than below?)
>
> Unit tests would help here I think to confirm and validate intended
> purposes.
>
> > +
> > +       constexpr Vector<T, R> operator-(const Vector<T, R> &other) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] - other[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr Vector<T, R> operator+(const Vector<T, R> &other) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] + other[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr T operator*(const Vector<T, R> &other) const
> > +       {
> > +               T ret = 0;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret += data_[i] * other[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr Vector<T, R> operator*(T factor) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] * factor;
> > +               return ret;
> > +       }
> > +
> > +       constexpr Vector<T, R> operator/(T factor) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] / factor;
> > +               return ret;
> > +       }
> > +
> > +       constexpr T len2() const
> > +       {
> > +               T ret = 0;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret += data_[i] * data_[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr double len() const
> > +       {
> > +               return std::sqrt(len2());
> > +       }
> > +
> > +private:
> > +       std::array<T, R> data_;
> > +};
>
>
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +#endif /* __DOXYGEN__ */
> > +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > +{
> > +       for (unsigned int i = 0; i < R; i++)
> > +               if (lhs[i] != rhs[i])
> > +                       return false;
> > +
> > +       return true;
> > +}
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +#endif /* __DOXYGEN__ */
> > +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > +{
> > +       for (unsigned int i = 0; i < R; i++)
> > +               if (lhs[i] != rhs[i])
> > +                       return true;
> > +
> > +       return false;
> > +}
> > +
> > +} /* namespace ipa */
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R>
> > +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)
> > +{
> > +       out << v.toString();
> > +       return out;
> > +}
> > +#endif /* __DOXYGEN__ */
> > +
> > +} /* namespace libcamera */
> > --
> > 2.39.2
> >
Kieran Bingham June 10, 2024, 10:49 a.m. UTC | #3
Quoting David Plowman (2024-06-10 11:33:39)
> Hi Paul
> 
> Thank you for this patch.
> 
> On Mon, 10 Jun 2024 at 11:21, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Paul Elder (2024-06-07 09:42:58)
> > > Add a vector class to libipa. The original purpose of this is to replace
> > > the floating-point Point class that Raspberry Pi used in their Pwl, as
> > > that implementation of Point seemed more akin to a Vector than a Point.
> > >
> > > This is added to libipa instead of to geometry.h to avoid public API
> > > issues, plus this is not expected to be needed by applications.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > ---
> > > No change in v6
> > >
> > > New in v5
> > > ---
> > >  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++
> > >  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 368 insertions(+)
> > >  create mode 100644 src/ipa/libipa/vector.cpp
> > >  create mode 100644 src/ipa/libipa/vector.h
> > >
> > > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > > new file mode 100644
> > > index 000000000..bdc3c447c
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/vector.cpp
> > > @@ -0,0 +1,162 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +/*
> > > + * Copyright (C) 2019, Raspberry Pi Ltd
> > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > > + *
> > > + * Vector and related operations
> > > + */
> > > +
> > > +#include "vector.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +/**
> > > + * \file vector.h
> > > + * \brief Vector class
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(Vector)
> > > +
> > > +namespace ipa {
> > > +
> > > +/**
> > > + * \class Vector
> > > + * \brief Vector class
> > > + * \tparam T Type of numerical values to be stored in the vector
> > > + * \tparam R Number of rows in the vector
> > > + */
> > > +
> > > +/**
> > > + * \fn Vector::Vector()
> > > + * \brief Construct an identity vector
> 
> I just wanted to comment that this expression "identity vector" had me
> confused for a moment. I assume we just mean the "zero vector" which,
> it's true, is the identity element under the addition operation. But
> then we often multiply vectors by scalars, or matrices, so it does
> just make you pause for thought. Anyway, "zero vector" would have
> caused my brain cells slightly less cognitive dislocation!

Now that I'm reading the Matrix documentation class, I have a sneaky
suspicion that Matrix documentation was duplicated to Vector
documentation ;-)

As this is a default constructor - the values will be zero - so that's
certainly not an identify vector! A Zero vector sounds far more
reasonable.

--
Kieran


> 
> Thanks
> David
Stefan Klug June 10, 2024, 12:41 p.m. UTC | #4
Hi Paul,

thank you for the patch.
Kieran already commented some points, which I won't repeat...
I only add a few more :-)

On Fri, Jun 07, 2024 at 05:42:58PM +0900, Paul Elder wrote:
> Add a vector class to libipa. The original purpose of this is to replace
> the floating-point Point class that Raspberry Pi used in their Pwl, as
> that implementation of Point seemed more akin to a Vector than a Point.
> 
> This is added to libipa instead of to geometry.h to avoid public API
> issues, plus this is not expected to be needed by applications.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> No change in v6
> 
> New in v5
> ---
>  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++
>  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 368 insertions(+)
>  create mode 100644 src/ipa/libipa/vector.cpp
>  create mode 100644 src/ipa/libipa/vector.h
> 
> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> new file mode 100644
> index 000000000..bdc3c447c
> --- /dev/null
> +++ b/src/ipa/libipa/vector.cpp
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * Vector and related operations
> + */
> +
> +#include "vector.h"
> +
> +#include <libcamera/base/log.h>
> +
> +/**
> + * \file vector.h
> + * \brief Vector class
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Vector)
> +
> +namespace ipa {
> +
> +/**
> + * \class Vector
> + * \brief Vector class
> + * \tparam T Type of numerical values to be stored in the vector
> + * \tparam R Number of rows in the vector
> + */
> +
> +/**
> + * \fn Vector::Vector()
> + * \brief Construct an identity vector
> + */
> +
> +/**
> + * \fn Vector::Vector(const std::array<T, R> &data)
> + * \brief Construct vector from supplied data
> + * \param data Data from which to construct a vector
> + *
> + * \a data is a one-dimensional vector and will be turned into a vector in
> + * row-major order. The size of \a data must be equal to the product of the
> + * number of rows and columns of the vector (RxC).
> + */
> +
> +/**
> + * \fn Vector::readYaml
> + * \brief Populate the vector with yaml data
> + * \param yaml Yaml data to populate the vector with
> + *
> + * Any existing data in the vector will be overwritten. The size of the data
> + * read from \a yaml must be equal to the product of the number of rows and
> + * columns of the vector (RxC).
> + *
> + * The yaml data is expected to be a list with elements of type T.
> + *
> + * \return 0 on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn Vector::toString
> + * \brief Assemble and return a string describing the vector
> + * \return A string describing the vector
> + */

Is this still needed when we have ostream << () ? I don't think we need
two functions with the same purpose. Maybe move the imple into ostream
<< ()?

> +
> +/**
> + * \fn T Vector::operator[](size_t i) const
> + * \brief Index to a row in the vector
> + * \param i Index of row to retrieve
> + *
> + * This operator[] returns a Span, which can then be indexed into again with
> + * another operator[], allowing a convenient m[i][j] to access elements of the
> + * vector. Note that the lifetime of the Span returned by this first-level
> + * operator[] is bound to that of the Vector itself, so it is not recommended
> + * to save the Span that is the result of this operator[].
> + *
> + * \return Row \a i from the vector, as a Span
> + */
> +
> +/**
> + * \fn T &Vector::operator[](size_t i)
> + * \copydoc Vector::operator[](size_t i) const
> + */
> +
> +/**
> + * \fn Vector::x()
> + * \brief Convenience function to access the first element of the vector
> + */
> +
> +/**
> + * \fn Vector::y()
> + * \brief Convenience function to access the second element of the vector
> + */
> +
> +/**
> + * \fn Vector::operator-() const
> + * \brief Negate a Vector by negating both all of its coordinates
> + * \return The negated vector
> + */
> +
> +/**
> + * \fn Vector::operator-(Vector const &other) const
> + * \brief Subtract one vector from another
> + * \param[in] other The other vector
> + * \return The difference of \a other from this vector
> + */
> +
> +/**
> + * \fn Vector::operator+()
> + * \brief Add two vectors together
> + * \param[in] other The other vector
> + * \return The sum of the two vectors
> + */
> +
> +/**
> + * \fn Vector::operator*(const Vector<T, R> &other) const
> + * \brief Compute the dot product
> + * \param[in] other The other vector
> + * \return The dot product of the two vectors
> + */
> +
> +/**
> + * \fn Vector::operator*(T factor) const
> + * \brief Scale up the vector

I stumbled over the wording "Scale up". I would just say "Multiply the
vector by a scalar"

> + * \param[in] factor The factor
> + * \return The vector scaled up by \a factor
> + */
> +
> +/**
> + * \fn Vector::operator/()
> + * \brief Scale down the vector

Maybe: Divide the vector by a scalar.

With the comments from Kieran resolved, I'm fine with it.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefan

> + * \param[in] factor The factor
> + * \return The vector scaled down by \a factor
> + */
> +
> +/**
> + * \fn Vector::len2()
> + * \brief Get the squared length of the vector
> + * \return The squared length of the vector
> + */
> +
> +/**
> + * \fn Vector::len()
> + * \brief Get the length of the vector
> + * \return The length of the vector
> + */
> +
> +/**
> + * \fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> + * \brief Compare vectors for equality
> + * \return True if the two vectors are equal, false otherwise
> + */
> +
> +/**
> + * \fn bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> + * \brief Compare vectors for inequality
> + * \return True if the two vectors are not equal, false otherwise
> + */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> new file mode 100644
> index 000000000..87aad28b5
> --- /dev/null
> +++ b/src/ipa/libipa/vector.h
> @@ -0,0 +1,206 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * Vector and related operations
> + */
> +#pragma once
> +
> +#include <algorithm>
> +#include <array>
> +#include <cmath>
> +#include <sstream>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/span.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Vector)
> +
> +namespace ipa {
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R,
> +	 std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>
> +#else
> +template<typename T, unsigned int R>
> +#endif /* __DOXYGEN__ */
> +class Vector
> +{
> +public:
> +	Vector() = default;
> +
> +	Vector(const std::array<T, R> &data)
> +	{
> +		ASSERT(data.size() == R);
> +
> +		for (unsigned int i = 0; i < R; i++)
> +			data_[i] = data[i];
> +	}
> +
> +	~Vector() = default;
> +
> +	int readYaml(const libcamera::YamlObject &yaml)
> +	{
> +		if (yaml.size() != R) {
> +			LOG(Vector, Error)
> +				<< "Wrong number of values in vector: expected "
> +				<< R << ", got " << yaml.size();
> +			return -EINVAL;
> +		}
> +
> +		unsigned int i = 0;
> +		for (const auto &x : yaml.asList()) {
> +			auto value = x.get<T>();
> +			if (!value) {
> +				LOG(Vector, Error) << "Failed to read vector value";
> +				return -EINVAL;
> +			}
> +
> +			data_[i++] = *value;
> +		}
> +
> +		return 0;
> +	}
> +
> +	const std::string toString() const
> +	{
> +		std::stringstream out;
> +
> +		out << "Vector { ";
> +		for (unsigned int i = 0; i < R; i++) {
> +			out << (*this)[i];
> +			out << ((i + 1 < R) ? ", " : " ");
> +		}
> +		out << " }";
> +
> +		return out.str();
> +	}
> +
> +	const T operator[](size_t i) const
> +	{
> +		return data_[i];
> +	}
> +
> +	T &operator[](size_t i)
> +	{
> +		return data_[i];
> +	}
> +
> +	const T x() const
> +	{
> +		return data_[0];
> +	}
> +
> +	const T y() const
> +	{
> +		return data_[1];
> +	}
> +
> +	constexpr Vector<T, R> operator-() const
> +	{
> +		Vector<T, R> ret;
> +		for (unsigned int i = 0; i < R; i++)
> +			ret[i] = -data_[i];
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, R> operator-(const Vector<T, R> &other) const
> +	{
> +		Vector<T, R> ret;
> +		for (unsigned int i = 0; i < R; i++)
> +			ret[i] = data_[i] - other[i];
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, R> operator+(const Vector<T, R> &other) const
> +	{
> +		Vector<T, R> ret;
> +		for (unsigned int i = 0; i < R; i++)
> +			ret[i] = data_[i] + other[i];
> +		return ret;
> +	}
> +
> +	constexpr T operator*(const Vector<T, R> &other) const
> +	{
> +		T ret = 0;
> +		for (unsigned int i = 0; i < R; i++)
> +			ret += data_[i] * other[i];
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, R> operator*(T factor) const
> +	{
> +		Vector<T, R> ret;
> +		for (unsigned int i = 0; i < R; i++)
> +			ret[i] = data_[i] * factor;
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, R> operator/(T factor) const
> +	{
> +		Vector<T, R> ret;
> +		for (unsigned int i = 0; i < R; i++)
> +			ret[i] = data_[i] / factor;
> +		return ret;
> +	}
> +
> +	constexpr T len2() const
> +	{
> +		T ret = 0;
> +		for (unsigned int i = 0; i < R; i++)
> +			ret += data_[i] * data_[i];
> +		return ret;
> +	}
> +
> +	constexpr double len() const
> +	{
> +		return std::sqrt(len2());
> +	}
> +
> +private:
> +	std::array<T, R> data_;
> +};
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R,
> +	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +#endif /* __DOXYGEN__ */
> +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> +{
> +	for (unsigned int i = 0; i < R; i++)
> +		if (lhs[i] != rhs[i])
> +			return false;
> +
> +	return true;
> +}
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R,
> +	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +#endif /* __DOXYGEN__ */
> +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> +{
> +	for (unsigned int i = 0; i < R; i++)
> +		if (lhs[i] != rhs[i])
> +			return true;
> +
> +	return false;
> +}
> +
> +} /* namespace ipa */
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R>
> +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)
> +{
> +	out << v.toString();
> +	return out;
> +}
> +#endif /* __DOXYGEN__ */
> +
> +} /* namespace libcamera */
> -- 
> 2.39.2
>
Paul Elder June 10, 2024, 12:57 p.m. UTC | #5
On Mon, Jun 10, 2024 at 11:21:35AM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-06-07 09:42:58)
> > Add a vector class to libipa. The original purpose of this is to replace
> > the floating-point Point class that Raspberry Pi used in their Pwl, as
> > that implementation of Point seemed more akin to a Vector than a Point.
> > 
> > This is added to libipa instead of to geometry.h to avoid public API
> > issues, plus this is not expected to be needed by applications.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > No change in v6
> > 
> > New in v5
> > ---
> >  src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++
> >  src/ipa/libipa/vector.h   | 206 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 368 insertions(+)
> >  create mode 100644 src/ipa/libipa/vector.cpp
> >  create mode 100644 src/ipa/libipa/vector.h
> > 
> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > new file mode 100644
> > index 000000000..bdc3c447c
> > --- /dev/null
> > +++ b/src/ipa/libipa/vector.cpp
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * Vector and related operations
> > + */
> > +
> > +#include "vector.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +/**
> > + * \file vector.h
> > + * \brief Vector class
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Vector)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class Vector
> > + * \brief Vector class
> > + * \tparam T Type of numerical values to be stored in the vector
> > + * \tparam R Number of rows in the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::Vector()
> > + * \brief Construct an identity vector
> > + */
> > +
> > +/**
> > + * \fn Vector::Vector(const std::array<T, R> &data)
> > + * \brief Construct vector from supplied data
> > + * \param data Data from which to construct a vector
> > + *
> > + * \a data is a one-dimensional vector and will be turned into a vector in
> > + * row-major order. The size of \a data must be equal to the product of the
> > + * number of rows and columns of the vector (RxC).
> 
> Rows and columns of a vector?

Oops, I copied from Matrix and forgot to update the documentation...

> 
> > + */
> > +
> > +/**
> > + * \fn Vector::readYaml
> > + * \brief Populate the vector with yaml data
> > + * \param yaml Yaml data to populate the vector with
> > + *
> > + * Any existing data in the vector will be overwritten. The size of the data
> > + * read from \a yaml must be equal to the product of the number of rows and
> > + * columns of the vector (RxC).
> > + *
> > + * The yaml data is expected to be a list with elements of type T.
> > + *
> > + * \return 0 on success, negative error code otherwise
> > + */
> > +
> > +/**
> > + * \fn Vector::toString
> > + * \brief Assemble and return a string describing the vector
> > + * \return A string describing the vector
> > + */
> > +
> > +/**
> > + * \fn T Vector::operator[](size_t i) const
> > + * \brief Index to a row in the vector
> > + * \param i Index of row to retrieve
> > + *
> > + * This operator[] returns a Span, which can then be indexed into again with
> > + * another operator[], allowing a convenient m[i][j] to access elements of the
> > + * vector. Note that the lifetime of the Span returned by this first-level
> > + * operator[] is bound to that of the Vector itself, so it is not recommended
> > + * to save the Span that is the result of this operator[].
> > + *
> > + * \return Row \a i from the vector, as a Span
> 
> Are these needed for a vector? I don't understand the indexing of a
> vector? I could understand on a matrix - but isn't a vector just a set
> of coordinates that represent direction/magnitude?
> 
> I don't understand what m[i][j] is used for ? (or represents?)
> 
> Perhaps the hint 'm' means this is a left over and needs to be removed?

Yeep...

> 
> > + */
> > +
> > +/**
> > + * \fn T &Vector::operator[](size_t i)
> > + * \copydoc Vector::operator[](size_t i) const
> > + */
> > +
> > +/**
> > + * \fn Vector::x()
> > + * \brief Convenience function to access the first element of the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::y()
> > + * \brief Convenience function to access the second element of the vector
> > + */
> 
> These I understand :-)
> 
> 
> 
> > +
> > +/**
> > + * \fn Vector::operator-() const
> > + * \brief Negate a Vector by negating both all of its coordinates
> > + * \return The negated vector
> > + */
> > +
> > +/**
> > + * \fn Vector::operator-(Vector const &other) const
> > + * \brief Subtract one vector from another
> > + * \param[in] other The other vector
> > + * \return The difference of \a other from this vector
> > + */
> > +
> > +/**
> > + * \fn Vector::operator+()
> > + * \brief Add two vectors together
> > + * \param[in] other The other vector
> > + * \return The sum of the two vectors
> > + */
> > +
> > +/**
> > + * \fn Vector::operator*(const Vector<T, R> &other) const
> > + * \brief Compute the dot product
> > + * \param[in] other The other vector
> > + * \return The dot product of the two vectors
> > + */
> > +
> > +/**
> > + * \fn Vector::operator*(T factor) const
> > + * \brief Scale up the vector
> > + * \param[in] factor The factor
> > + * \return The vector scaled up by \a factor
> > + */
> > +
> > +/**
> > + * \fn Vector::operator/()
> > + * \brief Scale down the vector
> > + * \param[in] factor The factor
> > + * \return The vector scaled down by \a factor
> > + */
> > +
> > +/**
> > + * \fn Vector::len2()
> > + * \brief Get the squared length of the vector
> > + * \return The squared length of the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::len()
> > + * \brief Get the length of the vector
> > + * \return The length of the vector
> > + */
> 
> I think all of those probably deserve at least some level of a unit
> test. How come we don't seem to have tests for libipa - Seems like the
> sort of thing we should have... Particularly as you've got operations on
> differing types above.
> 
> 
> 
> > +
> > +/**
> > + * \fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > + * \brief Compare vectors for equality
> > + * \return True if the two vectors are equal, false otherwise
> > + */
> > +
> > +/**
> > + * \fn bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > + * \brief Compare vectors for inequality
> > + * \return True if the two vectors are not equal, false otherwise
> > + */
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > new file mode 100644
> > index 000000000..87aad28b5
> > --- /dev/null
> > +++ b/src/ipa/libipa/vector.h
> > @@ -0,0 +1,206 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * Vector and related operations
> > + */
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <array>
> > +#include <cmath>
> > +#include <sstream>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/base/span.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(Vector)
> > +
> > +namespace ipa {
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > +        std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>
> > +#else
> > +template<typename T, unsigned int R>
> 
> Do we anticipate 3d vectors? (or other dimesions?)
> 
> How about defaulting R = 2 so that a 2d vector is the default case?
> 
> Then we can use
> 	Vector<int> v = {x, y};

I didn't think that Vector<int, 2> was a very high cost? It's only two
more characters.

> 
> or such. (Or some how typedef/using to create Vector2d, Vector3d?)

That would be the preferred alternative imo.

> 
> Is R an appropriate name for a vector? Is D for dimensions any better?
> Or is it still better to call it Rows?

Hm, maybe dimensions would be better...

> 
> 
> 
> > +#endif /* __DOXYGEN__ */
> > +class Vector
> > +{
> > +public:
> > +       Vector() = default;
> > +
> > +       Vector(const std::array<T, R> &data)
> > +       {
> > +               ASSERT(data.size() == R);
> 
> Optional, but as we provide direct accessors for x() and y() perhaps:
> 
> 	ASSERT(R >= 2);

That's already enforced by SFINAE in the class definition... unless
SFINAE doesn't kick in in this case...?

> 
> That might stop accidentally creating a vector of one dimesion..
> 
> 
> > +
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       data_[i] = data[i];
> > +       }
> > +
> > +       ~Vector() = default;
> > +
> > +       int readYaml(const libcamera::YamlObject &yaml)
> > +       {
> > +               if (yaml.size() != R) {
> > +                       LOG(Vector, Error)
> > +                               << "Wrong number of values in vector: expected "
> > +                               << R << ", got " << yaml.size();
> > +                       return -EINVAL;
> > +               }
> > +
> > +               unsigned int i = 0;
> > +               for (const auto &x : yaml.asList()) {
> > +                       auto value = x.get<T>();
> > +                       if (!value) {
> > +                               LOG(Vector, Error) << "Failed to read vector value";
> > +                               return -EINVAL;
> > +                       }
> > +
> > +                       data_[i++] = *value;
> > +               }
> > +
> > +               return 0;
> > +       }
> > +
> > +       const std::string toString() const
> > +       {
> > +               std::stringstream out;
> > +
> > +               out << "Vector { ";
> > +               for (unsigned int i = 0; i < R; i++) {
> > +                       out << (*this)[i];
> > +                       out << ((i + 1 < R) ? ", " : " ");
> > +               }
> > +               out << " }";
> > +
> > +               return out.str();
> > +       }
> > +
> > +       const T operator[](size_t i) const
> > +       {
> 
> ASSERT here if out of bounds?

Ah, right.

> 
> > +               return data_[i];
> > +       }
> > +
> > +       T &operator[](size_t i)
> > +       {
> > +               return data_[i];
> > +       }
> > +
> > +       const T x() const
> > +       {
> > +               return data_[0];
> > +       }
> > +
> > +       const T y() const
> > +       {
> > +               return data_[1];
> > +       }
> > +
> > +       constexpr Vector<T, R> operator-() const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = -data_[i];
> > +               return ret;
> > +       }
> 
> How would this be used? (differently than below?)

This is a unary negation operator, the below is a binary subtration
operator (did I forget to document it...?)

> 
> Unit tests would help here I think to confirm and validate intended
> purposes.
> 

I think I'll do that on top (along with the ones for matrices)...


Thanks,

Paul

> > +
> > +       constexpr Vector<T, R> operator-(const Vector<T, R> &other) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] - other[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr Vector<T, R> operator+(const Vector<T, R> &other) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] + other[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr T operator*(const Vector<T, R> &other) const
> > +       {
> > +               T ret = 0;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret += data_[i] * other[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr Vector<T, R> operator*(T factor) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] * factor;
> > +               return ret;
> > +       }
> > +
> > +       constexpr Vector<T, R> operator/(T factor) const
> > +       {
> > +               Vector<T, R> ret;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret[i] = data_[i] / factor;
> > +               return ret;
> > +       }
> > +
> > +       constexpr T len2() const
> > +       {
> > +               T ret = 0;
> > +               for (unsigned int i = 0; i < R; i++)
> > +                       ret += data_[i] * data_[i];
> > +               return ret;
> > +       }
> > +
> > +       constexpr double len() const
> > +       {
> > +               return std::sqrt(len2());
> > +       }
> > +
> > +private:
> > +       std::array<T, R> data_;
> > +};
> 
> 
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +#endif /* __DOXYGEN__ */
> > +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > +{
> > +       for (unsigned int i = 0; i < R; i++)
> > +               if (lhs[i] != rhs[i])
> > +                       return false;
> > +
> > +       return true;
> > +}
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +#endif /* __DOXYGEN__ */
> > +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > +{
> > +       for (unsigned int i = 0; i < R; i++)
> > +               if (lhs[i] != rhs[i])
> > +                       return true;
> > +
> > +       return false;
> > +}
> > +
> > +} /* namespace ipa */
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R>
> > +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)
> > +{
> > +       out << v.toString();
> > +       return out;
> > +}
> > +#endif /* __DOXYGEN__ */
> > +
> > +} /* namespace libcamera */
> > -- 
> > 2.39.2
> >

Patch
diff mbox series

diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
new file mode 100644
index 000000000..bdc3c447c
--- /dev/null
+++ b/src/ipa/libipa/vector.cpp
@@ -0,0 +1,162 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2019, Raspberry Pi Ltd
+ * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+ *
+ * Vector and related operations
+ */
+
+#include "vector.h"
+
+#include <libcamera/base/log.h>
+
+/**
+ * \file vector.h
+ * \brief Vector class
+ */
+
+namespace libcamera {
+
+LOG_DEFINE_CATEGORY(Vector)
+
+namespace ipa {
+
+/**
+ * \class Vector
+ * \brief Vector class
+ * \tparam T Type of numerical values to be stored in the vector
+ * \tparam R Number of rows in the vector
+ */
+
+/**
+ * \fn Vector::Vector()
+ * \brief Construct an identity vector
+ */
+
+/**
+ * \fn Vector::Vector(const std::array<T, R> &data)
+ * \brief Construct vector from supplied data
+ * \param data Data from which to construct a vector
+ *
+ * \a data is a one-dimensional vector and will be turned into a vector in
+ * row-major order. The size of \a data must be equal to the product of the
+ * number of rows and columns of the vector (RxC).
+ */
+
+/**
+ * \fn Vector::readYaml
+ * \brief Populate the vector with yaml data
+ * \param yaml Yaml data to populate the vector with
+ *
+ * Any existing data in the vector will be overwritten. The size of the data
+ * read from \a yaml must be equal to the product of the number of rows and
+ * columns of the vector (RxC).
+ *
+ * The yaml data is expected to be a list with elements of type T.
+ *
+ * \return 0 on success, negative error code otherwise
+ */
+
+/**
+ * \fn Vector::toString
+ * \brief Assemble and return a string describing the vector
+ * \return A string describing the vector
+ */
+
+/**
+ * \fn T Vector::operator[](size_t i) const
+ * \brief Index to a row in the vector
+ * \param i Index of row to retrieve
+ *
+ * This operator[] returns a Span, which can then be indexed into again with
+ * another operator[], allowing a convenient m[i][j] to access elements of the
+ * vector. Note that the lifetime of the Span returned by this first-level
+ * operator[] is bound to that of the Vector itself, so it is not recommended
+ * to save the Span that is the result of this operator[].
+ *
+ * \return Row \a i from the vector, as a Span
+ */
+
+/**
+ * \fn T &Vector::operator[](size_t i)
+ * \copydoc Vector::operator[](size_t i) const
+ */
+
+/**
+ * \fn Vector::x()
+ * \brief Convenience function to access the first element of the vector
+ */
+
+/**
+ * \fn Vector::y()
+ * \brief Convenience function to access the second element of the vector
+ */
+
+/**
+ * \fn Vector::operator-() const
+ * \brief Negate a Vector by negating both all of its coordinates
+ * \return The negated vector
+ */
+
+/**
+ * \fn Vector::operator-(Vector const &other) const
+ * \brief Subtract one vector from another
+ * \param[in] other The other vector
+ * \return The difference of \a other from this vector
+ */
+
+/**
+ * \fn Vector::operator+()
+ * \brief Add two vectors together
+ * \param[in] other The other vector
+ * \return The sum of the two vectors
+ */
+
+/**
+ * \fn Vector::operator*(const Vector<T, R> &other) const
+ * \brief Compute the dot product
+ * \param[in] other The other vector
+ * \return The dot product of the two vectors
+ */
+
+/**
+ * \fn Vector::operator*(T factor) const
+ * \brief Scale up the vector
+ * \param[in] factor The factor
+ * \return The vector scaled up by \a factor
+ */
+
+/**
+ * \fn Vector::operator/()
+ * \brief Scale down the vector
+ * \param[in] factor The factor
+ * \return The vector scaled down by \a factor
+ */
+
+/**
+ * \fn Vector::len2()
+ * \brief Get the squared length of the vector
+ * \return The squared length of the vector
+ */
+
+/**
+ * \fn Vector::len()
+ * \brief Get the length of the vector
+ * \return The length of the vector
+ */
+
+/**
+ * \fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
+ * \brief Compare vectors for equality
+ * \return True if the two vectors are equal, false otherwise
+ */
+
+/**
+ * \fn bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
+ * \brief Compare vectors for inequality
+ * \return True if the two vectors are not equal, false otherwise
+ */
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
new file mode 100644
index 000000000..87aad28b5
--- /dev/null
+++ b/src/ipa/libipa/vector.h
@@ -0,0 +1,206 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2019, Raspberry Pi Ltd
+ * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+ *
+ * Vector and related operations
+ */
+#pragma once
+
+#include <algorithm>
+#include <array>
+#include <cmath>
+#include <sstream>
+
+#include <libcamera/base/log.h>
+#include <libcamera/base/span.h>
+
+#include "libcamera/internal/yaml_parser.h"
+
+namespace libcamera {
+
+LOG_DECLARE_CATEGORY(Vector)
+
+namespace ipa {
+
+#ifndef __DOXYGEN__
+template<typename T, unsigned int R,
+	 std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>
+#else
+template<typename T, unsigned int R>
+#endif /* __DOXYGEN__ */
+class Vector
+{
+public:
+	Vector() = default;
+
+	Vector(const std::array<T, R> &data)
+	{
+		ASSERT(data.size() == R);
+
+		for (unsigned int i = 0; i < R; i++)
+			data_[i] = data[i];
+	}
+
+	~Vector() = default;
+
+	int readYaml(const libcamera::YamlObject &yaml)
+	{
+		if (yaml.size() != R) {
+			LOG(Vector, Error)
+				<< "Wrong number of values in vector: expected "
+				<< R << ", got " << yaml.size();
+			return -EINVAL;
+		}
+
+		unsigned int i = 0;
+		for (const auto &x : yaml.asList()) {
+			auto value = x.get<T>();
+			if (!value) {
+				LOG(Vector, Error) << "Failed to read vector value";
+				return -EINVAL;
+			}
+
+			data_[i++] = *value;
+		}
+
+		return 0;
+	}
+
+	const std::string toString() const
+	{
+		std::stringstream out;
+
+		out << "Vector { ";
+		for (unsigned int i = 0; i < R; i++) {
+			out << (*this)[i];
+			out << ((i + 1 < R) ? ", " : " ");
+		}
+		out << " }";
+
+		return out.str();
+	}
+
+	const T operator[](size_t i) const
+	{
+		return data_[i];
+	}
+
+	T &operator[](size_t i)
+	{
+		return data_[i];
+	}
+
+	const T x() const
+	{
+		return data_[0];
+	}
+
+	const T y() const
+	{
+		return data_[1];
+	}
+
+	constexpr Vector<T, R> operator-() const
+	{
+		Vector<T, R> ret;
+		for (unsigned int i = 0; i < R; i++)
+			ret[i] = -data_[i];
+		return ret;
+	}
+
+	constexpr Vector<T, R> operator-(const Vector<T, R> &other) const
+	{
+		Vector<T, R> ret;
+		for (unsigned int i = 0; i < R; i++)
+			ret[i] = data_[i] - other[i];
+		return ret;
+	}
+
+	constexpr Vector<T, R> operator+(const Vector<T, R> &other) const
+	{
+		Vector<T, R> ret;
+		for (unsigned int i = 0; i < R; i++)
+			ret[i] = data_[i] + other[i];
+		return ret;
+	}
+
+	constexpr T operator*(const Vector<T, R> &other) const
+	{
+		T ret = 0;
+		for (unsigned int i = 0; i < R; i++)
+			ret += data_[i] * other[i];
+		return ret;
+	}
+
+	constexpr Vector<T, R> operator*(T factor) const
+	{
+		Vector<T, R> ret;
+		for (unsigned int i = 0; i < R; i++)
+			ret[i] = data_[i] * factor;
+		return ret;
+	}
+
+	constexpr Vector<T, R> operator/(T factor) const
+	{
+		Vector<T, R> ret;
+		for (unsigned int i = 0; i < R; i++)
+			ret[i] = data_[i] / factor;
+		return ret;
+	}
+
+	constexpr T len2() const
+	{
+		T ret = 0;
+		for (unsigned int i = 0; i < R; i++)
+			ret += data_[i] * data_[i];
+		return ret;
+	}
+
+	constexpr double len() const
+	{
+		return std::sqrt(len2());
+	}
+
+private:
+	std::array<T, R> data_;
+};
+
+#ifndef __DOXYGEN__
+template<typename T, unsigned int R,
+	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
+#endif /* __DOXYGEN__ */
+bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
+{
+	for (unsigned int i = 0; i < R; i++)
+		if (lhs[i] != rhs[i])
+			return false;
+
+	return true;
+}
+
+#ifndef __DOXYGEN__
+template<typename T, unsigned int R,
+	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
+#endif /* __DOXYGEN__ */
+bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
+{
+	for (unsigned int i = 0; i < R; i++)
+		if (lhs[i] != rhs[i])
+			return true;
+
+	return false;
+}
+
+} /* namespace ipa */
+
+#ifndef __DOXYGEN__
+template<typename T, unsigned int R>
+std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)
+{
+	out << v.toString();
+	return out;
+}
+#endif /* __DOXYGEN__ */
+
+} /* namespace libcamera */