[libcamera-devel,v3,4/6] libcamera: Add geometry helper functions

9867 diff mbox series

These functions are aimed at making it easier to calculate cropping
rectangles, particularly in order to implement digital zoom.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/geometry.h |  20 ++++++
 src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+)

Hi David,

On 29/09/2020 17:39, David Plowman wrote:
> These functions are aimed at making it easier to calculate cropping
> rectangles, particularly in order to implement digital zoom.

Personally, I love seeing these helpers grow ;-)

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Some minor spelling errors below, but with those fixed:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  include/libcamera/geometry.h |  20 ++++++
>  src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
>  2 files changed, 149 insertions(+)
> 
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 02fb63c0..0447ee3e 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -13,6 +13,8 @@
>  
>  namespace libcamera {
>  
> +class Rectangle;
> +
>  class Size
>  {
>  public:
> @@ -93,8 +95,17 @@ public:
>  			std::max(height, expand.height)
>  		};
>  	}
> +
> +	Size alignedDownToAspectRatio(const Size &ratio) const;
> +	Size alignedUpToAspectRatio(const Size &ratio) const;
> +
> +	Rectangle centredTo(const Rectangle &region,
> +			    int offsetX = 0, int offsetY = 0) const;
>  };
>  
> +Size operator*(const Size &size, float f);
> +Size operator/(const Size &size, float f);
> +
>  bool operator==(const Size &lhs, const Size &rhs);
>  bool operator<(const Size &lhs, const Size &rhs);
>  
> @@ -176,6 +187,11 @@ public:
>  	{
>  	}
>  
> +	constexpr explicit Rectangle(const Size &size)
> +		: x(0), y(0), width(size.width), height(size.height)
> +	{
> +	}
> +
>  	int x;
>  	int y;
>  	unsigned int width;
> @@ -183,6 +199,10 @@ public:
>  
>  	bool isNull() const { return !width && !height; }
>  	const std::string toString() const;
> +
> +	Size size() const;
> +
> +	Rectangle boundedTo(const Rectangle &boundary) const;
>  };
>  
>  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index b12e1a62..649ee179 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -143,6 +143,88 @@ const std::string Size::toString() const
>   * height of this size and the \a expand size
>   */
>  
> +/**
> + * \brief Align down to the aspect ratio given by \a ratio
> + * \param[in] ratio The size whose aspect ratio to align down to
> + * \return A Size whose width and height are equal to the width and height
> + * of this Size aligned down to the aspect ratio of \a ratio
> + */
> +Size Size::alignedDownToAspectRatio(const Size &ratio) const
> +{
> +	uint64_t ratio1 = static_cast<uint64_t>(width) *
> +			  static_cast<uint64_t>(ratio.height);
> +	uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> +			  static_cast<uint64_t>(height);
> +
> +	if (ratio1 > ratio2)
> +		return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> +	else
> +		return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> +}
> +
> +/**
> + * \brief Align up to the aspect ratio given by \a ratio
> + * \param[in] ratio The size whose aspect ratio to align up to
> + * \return A Size whose width and height are equal to the width and height
> + * of this Size aligned up to the aspect ratio of \a ratio
> + */
> +Size Size::alignedUpToAspectRatio(const Size &ratio) const
> +{
> +	uint64_t ratio1 = static_cast<uint64_t>(width) *
> +			  static_cast<uint64_t>(ratio.height);
> +	uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> +			  static_cast<uint64_t>(height);
> +
> +	if (ratio1 < ratio2)
> +		return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> +	else
> +		return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> +}
> +
> +/**
> + * \brief Centre a rectangle of this size within another rectangular region,
> + * with optional offsets
> + * \param[in] region The rectangular region relative to which the returned
> + * rectangle can be position
> + * \param[in] offsetX The X offset of the mid-point of the returned rectangle
> + * relative to the mid-point of the region
> + * \param[in] offsetY The Y offset of the mid-point of the returned rectangle
> + * relative to the mid-point of the region
> + *
> + * A Rectangle of this object's size is positioned within the Rectangle
> + * given by \a region. It is positioned so that its mid-point coincides
> + * with the mid-point of \a region, and is then further offset by the
> + * values \a offsetX and \a offsetY.
> + *
> + * \return A Rectangle with the horizontal and vertical sizes of
> + * this Size instance, centred with offsets within a region
> + */
> +Rectangle Size::centredTo(const Rectangle &region, int offsetX, int offsetY) const
> +{
> +	int x = (region.width - width) / 2 + region.x + offsetX;
> +	int y = (region.height - height) / 2 + region.y + offsetY;
> +
> +	return Rectangle(x, y, width, height);
> +}
> +
> +/**
> + * \brief Scale size up by the given factor
> + * \return The scaled Size
> + */
> +Size operator*(const Size &size, float f)
> +{
> +	return Size(size.width * f, size.height * f);
> +}
> +
> +/**
> + * \brief Scale size down by the given factor
> + * \return The scaled Size
> + */
> +Size operator/(const Size &size, float f)
> +{
> +	return Size(size.width / f, size.height / f);
> +}
> +
>  /**
>   * \brief Compare sizes for equality
>   * \return True if the two sizes are equal, false otherwise
> @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
>   * \param[in] height The height
>   */
>  
> +/**
> + * \fn Rectangle::Rectangle(const Size &size)
> + * \brief Construct a Rectangle with zero offsets and the given \a size
> + * \param[in] size The desired Rectangle size
> + */
> +
>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const
>  	return ss.str();
>  }
>  
> +/**
> + * \brief Retrieve the size of this rectangle
> + * \return A Size reporting the Rectangle horizontal and vertical sizes
> + */
> +Size Rectangle::size() const
> +{
> +	return Size(width, height);
> +}
> +
> +/**
> + * \brief Bound a Rectangle so as not to exceeed another Rectangle

s/exceeed/exceed/

> + * \param[in] boundary The limit that the returned Rectangle will not exceed
> + *
> + * The Rectangle is bounded so that it does not exceeed the given \a boundary.

s/exceeed/exceed/

> + * This process involves translating the Rectangle if any of its edges
> + * lie beyond \a boundary, so that those edges then lie along the boundary
> + * instead.
> + *
> + * If either width or height are larger than \a bounary, then the returned

s/bounary/boundary/

> + * Rectangle is clipped to be no larger. But other than this, the
> + * Rectangle is not clipped or reduced in size, merely translated.
> + *
> + * We note that this is not a conventional Rectangle intersection function.
> + *
> + * \return A Rectangle that does not extend beyond a boundary Rectangle
> + */
> +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const
> +{
> +	Rectangle result(*this);
> +
> +	result.width = std::min(result.width, boundary.width);
> +	result.x = std::clamp<int>(result.x, boundary.x,
> +				   boundary.x + boundary.width - result.width);
> +
> +	result.height = std::min(result.height, boundary.height);
> +	result.y = std::clamp<int>(result.y, boundary.y,
> +				   boundary.y + boundary.height - result.height);
> +
> +	return result;
> +}
> +
>  /**
>   * \brief Compare rectangles for equality
>   * \return True if the two rectangles are equal, false otherwise
>

Hi David,

Thank you for the patch.

This looks good overall, but I'm afraid I have quite a few small
comments, possibly on the nitpicking side, given that I think these
functions need to be very explicit. Once we agree on the details I can
help with the implementation if desired.

Please also note that many of the comments below are open questions,
especially when they refer to naming of functions, so feel free to tell
me when you think my proposals are not good.

On Tue, Sep 29, 2020 at 05:39:58PM +0100, David Plowman wrote:
> These functions are aimed at making it easier to calculate cropping
> rectangles, particularly in order to implement digital zoom.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/geometry.h |  20 ++++++
>  src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++

We need test cases for the new functions, in test/geometry.cpp.

>  2 files changed, 149 insertions(+)
> 
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 02fb63c0..0447ee3e 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -13,6 +13,8 @@
>  
>  namespace libcamera {
>  
> +class Rectangle;
> +
>  class Size
>  {
>  public:
> @@ -93,8 +95,17 @@ public:
>  			std::max(height, expand.height)
>  		};
>  	}
> +
> +	Size alignedDownToAspectRatio(const Size &ratio) const;
> +	Size alignedUpToAspectRatio(const Size &ratio) const;

The names are a bit long but I don't have better alternatives to
propose.

> +
> +	Rectangle centredTo(const Rectangle &region,
> +			    int offsetX = 0, int offsetY = 0) const;
>  };
>  
> +Size operator*(const Size &size, float f);
> +Size operator/(const Size &size, float f);

To be complete, should we also have member operators that modify the
size in-place ?

	Size &operator*=(float f);
	Size &operator/=(float f);

> +
>  bool operator==(const Size &lhs, const Size &rhs);
>  bool operator<(const Size &lhs, const Size &rhs);
>  
> @@ -176,6 +187,11 @@ public:
>  	{
>  	}
>  
> +	constexpr explicit Rectangle(const Size &size)

Not saying it's a bad idea, but why do you think this constructor should
be explicit ?

> +		: x(0), y(0), width(size.width), height(size.height)
> +	{
> +	}
> +
>  	int x;
>  	int y;
>  	unsigned int width;
> @@ -183,6 +199,10 @@ public:
>  
>  	bool isNull() const { return !width && !height; }
>  	const std::string toString() const;
> +
> +	Size size() const;
> +
> +	Rectangle boundedTo(const Rectangle &boundary) const;
>  };
>  
>  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index b12e1a62..649ee179 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -143,6 +143,88 @@ const std::string Size::toString() const
>   * height of this size and the \a expand size
>   */
>  
> +/**
> + * \brief Align down to the aspect ratio given by \a ratio

Aligning down to an aspect ratio sounds a bit strange to me, as it's not
really about alignments. Would shrunkToAspectRatio() be a better name ?
The documentation could then become

 * \brief Compute the size shrunk to achieve the given aspect \a ratio
 * \param[in] ratio The desired aspect ratio
 *
 * This function computes the largest size that fits in this size and
 * that matches the aspect ratio of \ratio.
 *
 * \return The size shrunk to the aspect \ratio

Same for the next function, named expandedToAspectRatio().

> + * \param[in] ratio The size whose aspect ratio to align down to
> + * \return A Size whose width and height are equal to the width and height
> + * of this Size aligned down to the aspect ratio of \a ratio
> + */
> +Size Size::alignedDownToAspectRatio(const Size &ratio) const
> +{
> +	uint64_t ratio1 = static_cast<uint64_t>(width) *
> +			  static_cast<uint64_t>(ratio.height);
> +	uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> +			  static_cast<uint64_t>(height);
> +
> +	if (ratio1 > ratio2)
> +		return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> +	else
> +		return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> +}
> +
> +/**
> + * \brief Align up to the aspect ratio given by \a ratio
> + * \param[in] ratio The size whose aspect ratio to align up to
> + * \return A Size whose width and height are equal to the width and height
> + * of this Size aligned up to the aspect ratio of \a ratio
> + */
> +Size Size::alignedUpToAspectRatio(const Size &ratio) const
> +{
> +	uint64_t ratio1 = static_cast<uint64_t>(width) *
> +			  static_cast<uint64_t>(ratio.height);
> +	uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> +			  static_cast<uint64_t>(height);
> +
> +	if (ratio1 < ratio2)
> +		return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> +	else
> +		return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> +}
> +
> +/**
> + * \brief Centre a rectangle of this size within another rectangular region,
> + * with optional offsets
> + * \param[in] region The rectangular region relative to which the returned
> + * rectangle can be position
> + * \param[in] offsetX The X offset of the mid-point of the returned rectangle
> + * relative to the mid-point of the region
> + * \param[in] offsetY The Y offset of the mid-point of the returned rectangle
> + * relative to the mid-point of the region
> + *
> + * A Rectangle of this object's size is positioned within the Rectangle
> + * given by \a region. It is positioned so that its mid-point coincides
> + * with the mid-point of \a region, and is then further offset by the
> + * values \a offsetX and \a offsetY.
> + *
> + * \return A Rectangle with the horizontal and vertical sizes of
> + * this Size instance, centred with offsets within a region
> + */

This feels very ad-hoc, and the name isn't very explicit. Would it make
sense to split it in the following basic operations ? I'll leave any
documentation out as an exercise to see if their names are explicit.

Before:

Rectangle Size::centredTo(const Rectangle &region, int offsetX, int offsetY) const;

	Size size{ ... };
	Rectangle region{ ... };
	int dx = ..., dy = ...;

	Rectangle r = size.centredTo(region, dx, dy);

After:

class Point
{
public:
	Point()
		: x(0), y(0)
	{
	}

	int x;
	int y;
};

Point Rectangle::center() const;
Rectangle &Rectangle::moveCenter(const Point &position);
Rectangle &Rectangle::translate(int dx, int dx);

	Size size{ ... };
	Rectangle region{ ... };
	int dx = ..., dy = ...;

	Rectangle r{size};
	r.moveCenter(region.center()).translate(dx, dy);

or

	Rectangle r = Rectangle{size}.moveCenter(region.center()).translate(dx, dy);

It's a bit longer, but I think it's move explicit, and the operations
can be reused for other purposes.

> +Rectangle Size::centredTo(const Rectangle &region, int offsetX, int offsetY) const
> +{
> +	int x = (region.width - width) / 2 + region.x + offsetX;
> +	int y = (region.height - height) / 2 + region.y + offsetY;
> +
> +	return Rectangle(x, y, width, height);
> +}
> +
> +/**
> + * \brief Scale size up by the given factor

 * \param[in] size The size to scale
 * \param[in] f The scaling factor

> + * \return The scaled Size
> + */
> +Size operator*(const Size &size, float f)

I'd name the parameter factor.

> +{
> +	return Size(size.width * f, size.height * f);
> +}
> +
> +/**
> + * \brief Scale size down by the given factor
> + * \return The scaled Size
> + */
> +Size operator/(const Size &size, float f)

Same comments here.

> +{
> +	return Size(size.width / f, size.height / f);
> +}
> +
>  /**
>   * \brief Compare sizes for equality
>   * \return True if the two sizes are equal, false otherwise
> @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
>   * \param[in] height The height
>   */
>  
> +/**
> + * \fn Rectangle::Rectangle(const Size &size)
> + * \brief Construct a Rectangle with zero offsets and the given \a size

We don't use a concept of "offset" for the Rectangle class at the
moment. I'd write this

 * \brief Construct a Rectangle of \a size located at (0,0)

> + * \param[in] size The desired Rectangle size
> + */
> +
>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const
>  	return ss.str();
>  }
>  
> +/**
> + * \brief Retrieve the size of this rectangle
> + * \return A Size reporting the Rectangle horizontal and vertical sizes

I think "The Rectangle size" should be enough.

> + */
> +Size Rectangle::size() const
> +{
> +	return Size(width, height);
> +}
> +
> +/**
> + * \brief Bound a Rectangle so as not to exceeed another Rectangle

this operation is also commonly referred to as computing the
intersection of two rectangles. We would thus name the function
intersection(), intersected() or intersectedWith(). The related
operation of computing the union of two rectangles would then be named
union(), united() or unitedWith() I suppose, while with bountedTo() it
would be named expandedTo(). I don't have a strong preference.

On a separate note, if we want to shorten the code, we could also define
operator& as an alias for this function (and operator| for the union).

> + * \param[in] boundary The limit that the returned Rectangle will not exceed
> + *
> + * The Rectangle is bounded so that it does not exceeed the given \a boundary.
> + * This process involves translating the Rectangle if any of its edges
> + * lie beyond \a boundary, so that those edges then lie along the boundary
> + * instead.
> + *
> + * If either width or height are larger than \a bounary, then the returned

s/bounary/boundary/

> + * Rectangle is clipped to be no larger. But other than this, the
> + * Rectangle is not clipped or reduced in size, merely translated.
> + *
> + * We note that this is not a conventional Rectangle intersection function.

So much for my question above :-)

Hmmmm... I think the name is a bit misleading then, or at least not
self-explicit. I fear this will cause confusion. I wonder if we could
find a better name, or a different set of standard operations that would
result in the same outcome when composed. I'll try to propose
alternatives when reviewing the patch that uses this function.

> + *
> + * \return A Rectangle that does not extend beyond a boundary Rectangle
> + */
> +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const
> +{
> +	Rectangle result(*this);
> +
> +	result.width = std::min(result.width, boundary.width);
> +	result.x = std::clamp<int>(result.x, boundary.x,
> +				   boundary.x + boundary.width - result.width);
> +
> +	result.height = std::min(result.height, boundary.height);
> +	result.y = std::clamp<int>(result.y, boundary.y,
> +				   boundary.y + boundary.height - result.height);
> +
> +	return result;
> +}
> +
>  /**
>   * \brief Compare rectangles for equality
>   * \return True if the two rectangles are equal, false otherwise

On Sun, 11 Oct 2020 at 03:11, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> This looks good overall, but I'm afraid I have quite a few small
> comments, possibly on the nitpicking side, given that I think these
> functions need to be very explicit. Once we agree on the details I can
> help with the implementation if desired.
>
> Please also note that many of the comments below are open questions,
> especially when they refer to naming of functions, so feel free to tell
> me when you think my proposals are not good.
>
> On Tue, Sep 29, 2020 at 05:39:58PM +0100, David Plowman wrote:
> > These functions are aimed at making it easier to calculate cropping
> > rectangles, particularly in order to implement digital zoom.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/geometry.h |  20 ++++++
> >  src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
>
> We need test cases for the new functions, in test/geometry.cpp.
>
> >  2 files changed, 149 insertions(+)
> >
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index 02fb63c0..0447ee3e 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -13,6 +13,8 @@
> >
> >  namespace libcamera {
> >
> > +class Rectangle;
> > +
> >  class Size
> >  {
> >  public:
> > @@ -93,8 +95,17 @@ public:
> >                       std::max(height, expand.height)
> >               };
> >       }
> > +
> > +     Size alignedDownToAspectRatio(const Size &ratio) const;
> > +     Size alignedUpToAspectRatio(const Size &ratio) const;
>
> The names are a bit long but I don't have better alternatives to
> propose.

Maybe "Ar" instead of "AspectRatio"? e.g. alignedDownToAr
But I'm not convinced. Nor do I think it matters greatly...

In line with your comment just below ("To be complete"), do you think
we should include in-place versions of all these functions,
alginUpToAspectRatio, centreTo etc.?

>
> > +
> > +     Rectangle centredTo(const Rectangle &region,
> > +                         int offsetX = 0, int offsetY = 0) const;
> >  };
> >
> > +Size operator*(const Size &size, float f);
> > +Size operator/(const Size &size, float f);
>
> To be complete, should we also have member operators that modify the
> size in-place ?
>
>         Size &operator*=(float f);
>         Size &operator/=(float f);

Agreed - why not!

>
> > +
> >  bool operator==(const Size &lhs, const Size &rhs);
> >  bool operator<(const Size &lhs, const Size &rhs);
> >
> > @@ -176,6 +187,11 @@ public:
> >       {
> >       }
> >
> > +     constexpr explicit Rectangle(const Size &size)
>
> Not saying it's a bad idea, but why do you think this constructor should
> be explicit ?

I think I'm a bit paranoid. If you did a calculation with various
Rectangles and Sizes flying around, and then you want (for example,
and borrowing the translate function from below...)

answer = rect.translate(dx, dy)

but wrote

answer = size.translate(dx, dy)

It's a question of whether you want the compiler to catch those and
force you to say exactly what you mean. Maybe you end up with
something a little clearer to understand too. Again, I don't really
mind...

>
> > +             : x(0), y(0), width(size.width), height(size.height)
> > +     {
> > +     }
> > +
> >       int x;
> >       int y;
> >       unsigned int width;
> > @@ -183,6 +199,10 @@ public:
> >
> >       bool isNull() const { return !width && !height; }
> >       const std::string toString() const;
> > +
> > +     Size size() const;
> > +
> > +     Rectangle boundedTo(const Rectangle &boundary) const;
> >  };
> >
> >  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index b12e1a62..649ee179 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -143,6 +143,88 @@ const std::string Size::toString() const
> >   * height of this size and the \a expand size
> >   */
> >
> > +/**
> > + * \brief Align down to the aspect ratio given by \a ratio
>
> Aligning down to an aspect ratio sounds a bit strange to me, as it's not
> really about alignments. Would shrunkToAspectRatio() be a better name ?
> The documentation could then become
>
>  * \brief Compute the size shrunk to achieve the given aspect \a ratio
>  * \param[in] ratio The desired aspect ratio
>  *
>  * This function computes the largest size that fits in this size and
>  * that matches the aspect ratio of \ratio.
>  *
>  * \return The size shrunk to the aspect \ratio
>
> Same for the next function, named expandedToAspectRatio().

Yes, shrunk/expanded sounds good to me. An alternative might be
boundedToAspectRatio (instead of shrunk), as we tend to use bounded
elsewhere.

>
> > + * \param[in] ratio The size whose aspect ratio to align down to
> > + * \return A Size whose width and height are equal to the width and height
> > + * of this Size aligned down to the aspect ratio of \a ratio
> > + */
> > +Size Size::alignedDownToAspectRatio(const Size &ratio) const
> > +{
> > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > +                       static_cast<uint64_t>(ratio.height);
> > +     uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> > +                       static_cast<uint64_t>(height);
> > +
> > +     if (ratio1 > ratio2)
> > +             return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> > +     else
> > +             return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> > +}
> > +
> > +/**
> > + * \brief Align up to the aspect ratio given by \a ratio
> > + * \param[in] ratio The size whose aspect ratio to align up to
> > + * \return A Size whose width and height are equal to the width and height
> > + * of this Size aligned up to the aspect ratio of \a ratio
> > + */
> > +Size Size::alignedUpToAspectRatio(const Size &ratio) const
> > +{
> > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > +                       static_cast<uint64_t>(ratio.height);
> > +     uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> > +                       static_cast<uint64_t>(height);
> > +
> > +     if (ratio1 < ratio2)
> > +             return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> > +     else
> > +             return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> > +}
> > +
> > +/**
> > + * \brief Centre a rectangle of this size within another rectangular region,
> > + * with optional offsets
> > + * \param[in] region The rectangular region relative to which the returned
> > + * rectangle can be position
> > + * \param[in] offsetX The X offset of the mid-point of the returned rectangle
> > + * relative to the mid-point of the region
> > + * \param[in] offsetY The Y offset of the mid-point of the returned rectangle
> > + * relative to the mid-point of the region
> > + *
> > + * A Rectangle of this object's size is positioned within the Rectangle
> > + * given by \a region. It is positioned so that its mid-point coincides
> > + * with the mid-point of \a region, and is then further offset by the
> > + * values \a offsetX and \a offsetY.
> > + *
> > + * \return A Rectangle with the horizontal and vertical sizes of
> > + * this Size instance, centred with offsets within a region
> > + */
>
> This feels very ad-hoc, and the name isn't very explicit. Would it make
> sense to split it in the following basic operations ? I'll leave any
> documentation out as an exercise to see if their names are explicit.
>
> Before:
>
> Rectangle Size::centredTo(const Rectangle &region, int offsetX, int offsetY) const;
>
>         Size size{ ... };
>         Rectangle region{ ... };
>         int dx = ..., dy = ...;
>
>         Rectangle r = size.centredTo(region, dx, dy);
>
> After:
>
> class Point
> {
> public:
>         Point()
>                 : x(0), y(0)
>         {
>         }
>
>         int x;
>         int y;
> };
>
> Point Rectangle::center() const;
> Rectangle &Rectangle::moveCenter(const Point &position);
> Rectangle &Rectangle::translate(int dx, int dx);
>
>         Size size{ ... };
>         Rectangle region{ ... };
>         int dx = ..., dy = ...;
>
>         Rectangle r{size};
>         r.moveCenter(region.center()).translate(dx, dy);
>
> or
>
>         Rectangle r = Rectangle{size}.moveCenter(region.center()).translate(dx, dy);
>
> It's a bit longer, but I think it's move explicit, and the operations
> can be reused for other purposes.

I think that's a fair cop, as we say!

BTW, do we have a policy on UK vs. American spelling? (centre/center)

I think I still prefer something like "centredTo" instead of
"moveCenter", it feels more like the other functions. An in-place
version could be "centreTo". How about "translateBy" and
"translatedBy" instead of "translate"...?

>
> > +Rectangle Size::centredTo(const Rectangle &region, int offsetX, int offsetY) const
> > +{
> > +     int x = (region.width - width) / 2 + region.x + offsetX;
> > +     int y = (region.height - height) / 2 + region.y + offsetY;
> > +
> > +     return Rectangle(x, y, width, height);
> > +}
> > +
> > +/**
> > + * \brief Scale size up by the given factor
>
>  * \param[in] size The size to scale
>  * \param[in] f The scaling factor
>
> > + * \return The scaled Size
> > + */
> > +Size operator*(const Size &size, float f)
>
> I'd name the parameter factor.
>
> > +{
> > +     return Size(size.width * f, size.height * f);
> > +}
> > +
> > +/**
> > + * \brief Scale size down by the given factor
> > + * \return The scaled Size
> > + */
> > +Size operator/(const Size &size, float f)
>
> Same comments here.
>
> > +{
> > +     return Size(size.width / f, size.height / f);
> > +}
> > +
> >  /**
> >   * \brief Compare sizes for equality
> >   * \return True if the two sizes are equal, false otherwise
> > @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
> >   * \param[in] height The height
> >   */
> >
> > +/**
> > + * \fn Rectangle::Rectangle(const Size &size)
> > + * \brief Construct a Rectangle with zero offsets and the given \a size
>
> We don't use a concept of "offset" for the Rectangle class at the
> moment. I'd write this
>
>  * \brief Construct a Rectangle of \a size located at (0,0)

Sure. I might like "top left corner located at..." given that there's
increasing amounts of "centering" going-on.

>
> > + * \param[in] size The desired Rectangle size
> > + */
> > +
> >  /**
> >   * \var Rectangle::x
> >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const
> >       return ss.str();
> >  }
> >
> > +/**
> > + * \brief Retrieve the size of this rectangle
> > + * \return A Size reporting the Rectangle horizontal and vertical sizes
>
> I think "The Rectangle size" should be enough.
>
> > + */
> > +Size Rectangle::size() const
> > +{
> > +     return Size(width, height);
> > +}
> > +
> > +/**
> > + * \brief Bound a Rectangle so as not to exceeed another Rectangle
>
> this operation is also commonly referred to as computing the
> intersection of two rectangles. We would thus name the function
> intersection(), intersected() or intersectedWith(). The related
> operation of computing the union of two rectangles would then be named
> union(), united() or unitedWith() I suppose, while with bountedTo() it
> would be named expandedTo(). I don't have a strong preference.
>
> On a separate note, if we want to shorten the code, we could also define
> operator& as an alias for this function (and operator| for the union).
>
> > + * \param[in] boundary The limit that the returned Rectangle will not exceed
> > + *
> > + * The Rectangle is bounded so that it does not exceeed the given \a boundary.
> > + * This process involves translating the Rectangle if any of its edges
> > + * lie beyond \a boundary, so that those edges then lie along the boundary
> > + * instead.
> > + *
> > + * If either width or height are larger than \a bounary, then the returned
>
> s/bounary/boundary/
>
> > + * Rectangle is clipped to be no larger. But other than this, the
> > + * Rectangle is not clipped or reduced in size, merely translated.
> > + *
> > + * We note that this is not a conventional Rectangle intersection function.
>
> So much for my question above :-)
>
> Hmmmm... I think the name is a bit misleading then, or at least not
> self-explicit. I fear this will cause confusion. I wonder if we could
> find a better name, or a different set of standard operations that would
> result in the same outcome when composed. I'll try to propose
> alternatives when reviewing the patch that uses this function.

I guess there are two operations going on:

* Limiting the size of the rectangle (to be no larger than the boundary).

* Translating it so that no part of it exceeds the boundary.

For the first one... well, it could stay as "boundedTo". Might take a
Size rather than a Rectangle as the parameter? However it's not very
clear what happens to the top left corner, though in this case it
doesn't matter.

For the second one... "restrictedTo"? or even "pannedTo" (in the sense
that the source Rectangle is being "panned" to lie within the
boundary)? perhaps "shiftedTo" is less weird.

Tricky!

Thanks and best regards
David

>
> > + *
> > + * \return A Rectangle that does not extend beyond a boundary Rectangle
> > + */
> > +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const
> > +{
> > +     Rectangle result(*this);
> > +
> > +     result.width = std::min(result.width, boundary.width);
> > +     result.x = std::clamp<int>(result.x, boundary.x,
> > +                                boundary.x + boundary.width - result.width);
> > +
> > +     result.height = std::min(result.height, boundary.height);
> > +     result.y = std::clamp<int>(result.y, boundary.y,
> > +                                boundary.y + boundary.height - result.height);
> > +
> > +     return result;
> > +}
> > +
> >  /**
> >   * \brief Compare rectangles for equality
> >   * \return True if the two rectangles are equal, false otherwise
>
> --
> Regards,
>
> Laurent Pinchart

Hi David,

On Mon, Oct 12, 2020 at 11:19:09AM +0100, David Plowman wrote:
> On Sun, 11 Oct 2020 at 03:11, Laurent Pinchart wrote:
> >
> > Hi David,
> >
> > Thank you for the patch.
> >
> > This looks good overall, but I'm afraid I have quite a few small
> > comments, possibly on the nitpicking side, given that I think these
> > functions need to be very explicit. Once we agree on the details I can
> > help with the implementation if desired.
> >
> > Please also note that many of the comments below are open questions,
> > especially when they refer to naming of functions, so feel free to tell
> > me when you think my proposals are not good.
> >
> > On Tue, Sep 29, 2020 at 05:39:58PM +0100, David Plowman wrote:
> > > These functions are aimed at making it easier to calculate cropping
> > > rectangles, particularly in order to implement digital zoom.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  include/libcamera/geometry.h |  20 ++++++
> > >  src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
> >
> > We need test cases for the new functions, in test/geometry.cpp.
> >
> > >  2 files changed, 149 insertions(+)
> > >
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > index 02fb63c0..0447ee3e 100644
> > > --- a/include/libcamera/geometry.h
> > > +++ b/include/libcamera/geometry.h
> > > @@ -13,6 +13,8 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class Rectangle;
> > > +
> > >  class Size
> > >  {
> > >  public:
> > > @@ -93,8 +95,17 @@ public:
> > >                       std::max(height, expand.height)
> > >               };
> > >       }
> > > +
> > > +     Size alignedDownToAspectRatio(const Size &ratio) const;
> > > +     Size alignedUpToAspectRatio(const Size &ratio) const;
> >
> > The names are a bit long but I don't have better alternatives to
> > propose.
> 
> Maybe "Ar" instead of "AspectRatio"? e.g. alignedDownToAr
> But I'm not convinced. Nor do I think it matters greatly...

I'm not convinced either, Ar would be a bit cryptic I think.

> In line with your comment just below ("To be complete"), do you think
> we should include in-place versions of all these functions,
> alginUpToAspectRatio, centreTo etc.?

They could be useful. I'm fine leaving them out for now and adding them
later, or biting the bullet already.

> > > +
> > > +     Rectangle centredTo(const Rectangle &region,
> > > +                         int offsetX = 0, int offsetY = 0) const;
> > >  };
> > >
> > > +Size operator*(const Size &size, float f);
> > > +Size operator/(const Size &size, float f);
> >
> > To be complete, should we also have member operators that modify the
> > size in-place ?
> >
> >         Size &operator*=(float f);
> >         Size &operator/=(float f);
> 
> Agreed - why not!
> 
> > > +
> > >  bool operator==(const Size &lhs, const Size &rhs);
> > >  bool operator<(const Size &lhs, const Size &rhs);
> > >
> > > @@ -176,6 +187,11 @@ public:
> > >       {
> > >       }
> > >
> > > +     constexpr explicit Rectangle(const Size &size)
> >
> > Not saying it's a bad idea, but why do you think this constructor should
> > be explicit ?
> 
> I think I'm a bit paranoid. If you did a calculation with various
> Rectangles and Sizes flying around, and then you want (for example,
> and borrowing the translate function from below...)
> 
> answer = rect.translate(dx, dy)
> 
> but wrote
> 
> answer = size.translate(dx, dy)

This one wouldn't compile as Size::translate() doesn't exist. I do see
your point though.

> It's a question of whether you want the compiler to catch those and
> force you to say exactly what you mean. Maybe you end up with
> something a little clearer to understand too. Again, I don't really
> mind...

I don't really mind either, I was just curious.

> > > +             : x(0), y(0), width(size.width), height(size.height)
> > > +     {
> > > +     }
> > > +
> > >       int x;
> > >       int y;
> > >       unsigned int width;
> > > @@ -183,6 +199,10 @@ public:
> > >
> > >       bool isNull() const { return !width && !height; }
> > >       const std::string toString() const;
> > > +
> > > +     Size size() const;
> > > +
> > > +     Rectangle boundedTo(const Rectangle &boundary) const;
> > >  };
> > >
> > >  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > > index b12e1a62..649ee179 100644
> > > --- a/src/libcamera/geometry.cpp
> > > +++ b/src/libcamera/geometry.cpp
> > > @@ -143,6 +143,88 @@ const std::string Size::toString() const
> > >   * height of this size and the \a expand size
> > >   */
> > >
> > > +/**
> > > + * \brief Align down to the aspect ratio given by \a ratio
> >
> > Aligning down to an aspect ratio sounds a bit strange to me, as it's not
> > really about alignments. Would shrunkToAspectRatio() be a better name ?
> > The documentation could then become
> >
> >  * \brief Compute the size shrunk to achieve the given aspect \a ratio
> >  * \param[in] ratio The desired aspect ratio
> >  *
> >  * This function computes the largest size that fits in this size and
> >  * that matches the aspect ratio of \ratio.
> >  *
> >  * \return The size shrunk to the aspect \ratio
> >
> > Same for the next function, named expandedToAspectRatio().
> 
> Yes, shrunk/expanded sounds good to me. An alternative might be
> boundedToAspectRatio (instead of shrunk), as we tend to use bounded
> elsewhere.

Yes that sounds good.

I stumbled upon https://doc.qt.io/qt-5/qsize.html#scale-1 today (while
researching something else, but I often get inspiration from Qt when it
comes to API design). Do you think it would be a good option ? Maybe we
can borrow the flag idea, and have

class Size
{
public:
	enum AspectRatioMode {
		IgnoreAspectRatio,
		KeepAspectRatio,
	};

	constexpr Size boundedTo(const Size &bound,
				 AspectRatioMode mode = IgnoreAspectRatio);
	constexpr Size expandedTo(const Size &expand,
				  AspectRatioMode mode = IgnoreAspectRatio);
	...
};

But now that I've written this, I think we would have two very different
semantics for the same function, the function would compute min/max when
mode is IgnoreAspectRatio, and scale otherwise. I think that would be
confusing. I'd rather go for scale()/scaled() as Qt (not sure about
IgnoreAspectRatio though, that seems fairly useless in this case, we
should probably have two modes only, and possibly rename them), or keep
boundedToAspectRatio()/expandedToAspectRatio().

> > > + * \param[in] ratio The size whose aspect ratio to align down to
> > > + * \return A Size whose width and height are equal to the width and height
> > > + * of this Size aligned down to the aspect ratio of \a ratio
> > > + */
> > > +Size Size::alignedDownToAspectRatio(const Size &ratio) const
> > > +{
> > > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > > +                       static_cast<uint64_t>(ratio.height);
> > > +     uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> > > +                       static_cast<uint64_t>(height);
> > > +
> > > +     if (ratio1 > ratio2)
> > > +             return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> > > +     else
> > > +             return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> > > +}
> > > +
> > > +/**
> > > + * \brief Align up to the aspect ratio given by \a ratio
> > > + * \param[in] ratio The size whose aspect ratio to align up to
> > > + * \return A Size whose width and height are equal to the width and height
> > > + * of this Size aligned up to the aspect ratio of \a ratio
> > > + */
> > > +Size Size::alignedUpToAspectRatio(const Size &ratio) const
> > > +{
> > > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > > +                       static_cast<uint64_t>(ratio.height);
> > > +     uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> > > +                       static_cast<uint64_t>(height);
> > > +
> > > +     if (ratio1 < ratio2)
> > > +             return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> > > +     else
> > > +             return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> > > +}
> > > +
> > > +/**
> > > + * \brief Centre a rectangle of this size within another rectangular region,
> > > + * with optional offsets
> > > + * \param[in] region The rectangular region relative to which the returned
> > > + * rectangle can be position
> > > + * \param[in] offsetX The X offset of the mid-point of the returned rectangle
> > > + * relative to the mid-point of the region
> > > + * \param[in] offsetY The Y offset of the mid-point of the returned rectangle
> > > + * relative to the mid-point of the region
> > > + *
> > > + * A Rectangle of this object's size is positioned within the Rectangle
> > > + * given by \a region. It is positioned so that its mid-point coincides
> > > + * with the mid-point of \a region, and is then further offset by the
> > > + * values \a offsetX and \a offsetY.
> > > + *
> > > + * \return A Rectangle with the horizontal and vertical sizes of
> > > + * this Size instance, centred with offsets within a region
> > > + */
> >
> > This feels very ad-hoc, and the name isn't very explicit. Would it make
> > sense to split it in the following basic operations ? I'll leave any
> > documentation out as an exercise to see if their names are explicit.
> >
> > Before:
> >
> > Rectangle Size::centredTo(const Rectangle &region, int offsetX, int offsetY) const;
> >
> >         Size size{ ... };
> >         Rectangle region{ ... };
> >         int dx = ..., dy = ...;
> >
> >         Rectangle r = size.centredTo(region, dx, dy);
> >
> > After:
> >
> > class Point
> > {
> > public:
> >         Point()
> >                 : x(0), y(0)
> >         {
> >         }
> >
> >         int x;
> >         int y;
> > };
> >
> > Point Rectangle::center() const;
> > Rectangle &Rectangle::moveCenter(const Point &position);
> > Rectangle &Rectangle::translate(int dx, int dx);
> >
> >         Size size{ ... };
> >         Rectangle region{ ... };
> >         int dx = ..., dy = ...;
> >
> >         Rectangle r{size};
> >         r.moveCenter(region.center()).translate(dx, dy);
> >
> > or
> >
> >         Rectangle r = Rectangle{size}.moveCenter(region.center()).translate(dx, dy);
> >
> > It's a bit longer, but I think it's move explicit, and the operations
> > can be reused for other purposes.
> 
> I think that's a fair cop, as we say!
> 
> BTW, do we have a policy on UK vs. American spelling? (centre/center)

We've discussed that internally in the past, and why we would all prefer
to go for a British spelling (the documentation currently uses Oxford
English), I think it will be a lost battle when it comes to API
elements. Developers expect analog, color and center :-( I think we'll
need to go through the code base and rename the functions at some point.
While I'd like to keep the Oxford English spelling for the
documentation, I'm not sure the majority will agree, given that there
will then be a mismatch between the code and the documentation.

If only American spoke English :-)

> I think I still prefer something like "centredTo" instead of
> "moveCenter", it feels more like the other functions. An in-place
> version could be "centreTo".

Note that moveCenter() is the in-place version. But otherwise it sounds
good to me, moveCenter() doesn't give us a good name for the
non-in-place version.

> How about "translateBy" and "translatedBy" instead of "translate"...?

While I usually try to keep names short and fairly fluid to read, I
think standardizing the geometry functions with a suffix, or without
one, would make sense.

> > > +Rectangle Size::centredTo(const Rectangle &region, int offsetX, int offsetY) const
> > > +{
> > > +     int x = (region.width - width) / 2 + region.x + offsetX;
> > > +     int y = (region.height - height) / 2 + region.y + offsetY;
> > > +
> > > +     return Rectangle(x, y, width, height);
> > > +}
> > > +
> > > +/**
> > > + * \brief Scale size up by the given factor
> >
> >  * \param[in] size The size to scale
> >  * \param[in] f The scaling factor
> >
> > > + * \return The scaled Size
> > > + */
> > > +Size operator*(const Size &size, float f)
> >
> > I'd name the parameter factor.
> >
> > > +{
> > > +     return Size(size.width * f, size.height * f);
> > > +}
> > > +
> > > +/**
> > > + * \brief Scale size down by the given factor
> > > + * \return The scaled Size
> > > + */
> > > +Size operator/(const Size &size, float f)
> >
> > Same comments here.
> >
> > > +{
> > > +     return Size(size.width / f, size.height / f);
> > > +}
> > > +
> > >  /**
> > >   * \brief Compare sizes for equality
> > >   * \return True if the two sizes are equal, false otherwise
> > > @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
> > >   * \param[in] height The height
> > >   */
> > >
> > > +/**
> > > + * \fn Rectangle::Rectangle(const Size &size)
> > > + * \brief Construct a Rectangle with zero offsets and the given \a size
> >
> > We don't use a concept of "offset" for the Rectangle class at the
> > moment. I'd write this
> >
> >  * \brief Construct a Rectangle of \a size located at (0,0)
> 
> Sure. I might like "top left corner located at..." given that there's
> increasing amounts of "centering" going-on.

Sounds good to me.

> > > + * \param[in] size The desired Rectangle size
> > > + */
> > > +
> > >  /**
> > >   * \var Rectangle::x
> > >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > > @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const
> > >       return ss.str();
> > >  }
> > >
> > > +/**
> > > + * \brief Retrieve the size of this rectangle
> > > + * \return A Size reporting the Rectangle horizontal and vertical sizes
> >
> > I think "The Rectangle size" should be enough.
> >
> > > + */
> > > +Size Rectangle::size() const
> > > +{
> > > +     return Size(width, height);
> > > +}
> > > +
> > > +/**
> > > + * \brief Bound a Rectangle so as not to exceeed another Rectangle
> >
> > this operation is also commonly referred to as computing the
> > intersection of two rectangles. We would thus name the function
> > intersection(), intersected() or intersectedWith(). The related
> > operation of computing the union of two rectangles would then be named
> > union(), united() or unitedWith() I suppose, while with bountedTo() it
> > would be named expandedTo(). I don't have a strong preference.
> >
> > On a separate note, if we want to shorten the code, we could also define
> > operator& as an alias for this function (and operator| for the union).
> >
> > > + * \param[in] boundary The limit that the returned Rectangle will not exceed
> > > + *
> > > + * The Rectangle is bounded so that it does not exceeed the given \a boundary.
> > > + * This process involves translating the Rectangle if any of its edges
> > > + * lie beyond \a boundary, so that those edges then lie along the boundary
> > > + * instead.
> > > + *
> > > + * If either width or height are larger than \a bounary, then the returned
> >
> > s/bounary/boundary/
> >
> > > + * Rectangle is clipped to be no larger. But other than this, the
> > > + * Rectangle is not clipped or reduced in size, merely translated.
> > > + *
> > > + * We note that this is not a conventional Rectangle intersection function.
> >
> > So much for my question above :-)
> >
> > Hmmmm... I think the name is a bit misleading then, or at least not
> > self-explicit. I fear this will cause confusion. I wonder if we could
> > find a better name, or a different set of standard operations that would
> > result in the same outcome when composed. I'll try to propose
> > alternatives when reviewing the patch that uses this function.
> 
> I guess there are two operations going on:
> 
> * Limiting the size of the rectangle (to be no larger than the boundary).
> 
> * Translating it so that no part of it exceeds the boundary.
> 
> For the first one... well, it could stay as "boundedTo". Might take a
> Size rather than a Rectangle as the parameter? However it's not very
> clear what happens to the top left corner, though in this case it
> doesn't matter.
> 
> For the second one... "restrictedTo"? or even "pannedTo" (in the sense
> that the source Rectangle is being "panned" to lie within the
> boundary)? perhaps "shiftedTo" is less weird.
> 
> Tricky!

A bit :-) I need to check the code that uses this first.

> > > + *
> > > + * \return A Rectangle that does not extend beyond a boundary Rectangle
> > > + */
> > > +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const
> > > +{
> > > +     Rectangle result(*this);
> > > +
> > > +     result.width = std::min(result.width, boundary.width);
> > > +     result.x = std::clamp<int>(result.x, boundary.x,
> > > +                                boundary.x + boundary.width - result.width);
> > > +
> > > +     result.height = std::min(result.height, boundary.height);
> > > +     result.y = std::clamp<int>(result.y, boundary.y,
> > > +                                boundary.y + boundary.height - result.height);
> > > +
> > > +     return result;
> > > +}
> > > +
> > >  /**
> > >   * \brief Compare rectangles for equality
> > >   * \return True if the two rectangles are equal, false otherwise