[PATCH v4 1/3] libcamera: Add rectangle two-point constructor

[Jacopo Mondi]

Hi Harvey

On Mon, Sep 16, 2024 at 09:24:23AM GMT, Jacopo Mondi wrote:
> Hello
>
> On Mon, Sep 16, 2024 at 12:34:54PM GMT, Cheng-Hao Yang wrote:
> > Hi Barnabás and Jacopo,
> >
> > On Mon, Sep 16, 2024 at 1:27 AM Barnabás Pőcze <pobrn at protonmail.com> wrote:
> >
> > > Hi
> > >
> > >
> > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > jacopo.mondi at ideasonboard.com> írta:
> > >
> > > > Hi Barnabás
> > > >
> > > > On Wed, Sep 04, 2024 at 11:26:37AM GMT, Barnabás Pőcze wrote:
> > > > > Hi
> > > > >
> > > > >
> > > > > 2024. szeptember 3., kedd 13:39 keltezéssel, Harvey Yang <
> > > chenghaoyang at chromium.org> írta:
> > > > >
> > > > > > From: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > > > >
> > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > topLeft and bottomRight.
> > > > > >
> > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > > > ---
> > > > > >  include/libcamera/geometry.h |  2 ++
> > > > > >  src/libcamera/geometry.cpp   | 14 ++++++++++++++
> > > > > >  2 files changed, 16 insertions(+)
> > > > > >
> > > > > > diff --git a/include/libcamera/geometry.h
> > > b/include/libcamera/geometry.h
> > > > > > index 3e6f0f5d7..dc56f180f 100644
> > > > > > --- a/include/libcamera/geometry.h
> > > > > > +++ b/include/libcamera/geometry.h
> > > > > > @@ -262,6 +262,8 @@ public:
> > > > > >   {
> > > > > >   }
> > > > > >
> > > > > > + constexpr Rectangle(const Point &topLeft, const Point
> > > &bottomRight);
> > > > >
> > > > > Don't make this `constexpr` because it is not useful since the
> > > definition is not available.
> > > >
> > > > I found references online that constexpr constuctors are implcitly
> > > > inline, is this the reason of your comment ?
> > >
> > > Yes.
> > >
> > >
> > > >
> > > > However, I can't find it clearly specified in cppreference. Do you
> > > > have any pointer ?
> > >
> > >   "A constexpr specifier used in a function or static data member(since
> > > C++17) declaration implies inline."
> > >   -- https://en.cppreference.com/w/cpp/language/constexpr
> > >
>
>
> One day I'll lear to read properly
>
> > > >
> > > > Anyway, if inline is the reason, isn't it better to inline the
> > > > definition and maintain the constexpr specifier ?
> > > > [...]
> > >
> > > That is an option as well.
> > >
> >
> > IIUC, you're suggesting to put the definition of the new c'tor
> > back in the header file? As we use `ASSERT` in this c'tor
> > definition, there will be a compile error if we do that:
> > ```
> >
> > In file included from ../include/libcamera/base/log.h:12,
> >
> >                  from ../include/libcamera/geometry.h:16,
> >
> >                  from ../include/libcamera/controls.h:21,
> >
> >                  from ../include/libcamera/camera.h:22,
> >
> >                  from ../src/apps/common/dng_writer.h:13,
> >
> >                  from ../src/apps/common/dng_writer.cpp:8:
> >
> > ../include/libcamera/base/private.h:21:2: error: #error "Private headers
> > must not be included in the libcamera API"
> >
> >    21 | #error "Private headers must not be included in the libcamera API"
> > ```
>
> I see. log.h is private, and the fact ASSERT is not exposed in the
> public API makes me think aborting execution on an invalid user input
> is probably not the best idea ? That's maybe the reason why ASSERT is not
> exposed to the public API ?
>

Assuming the top-left corner is always defined as the point with the
smaller 'x' and the higher 'y' whatever the reference system the
rectangle is placed in:

In example:

(0,0)
  -------------------------------->
  |
  |      -------------------
  |      |                 |
  |      |                 |
  |      x------------------
  |
  V

Or:

  ^
  |
  |
  |      x------------------
  |      |                 |
  |      |                 |
  |      -------------------
  |
  |
  -------------------------------->
(0,0)

The assertion you have introduced in your proposed constructor

	constexpr Rectangle(const Point &topLeft, const Point &bottomRight)
		: x(topLeft.x), y(topLeft.y),
		  width(bottomRight.x - x),
		  height(bottomRight.y - y)
	{
		ASSERT(bottomRight.x >= x && bottomRight.y >= y);
	}

is wrong as bottomRight.y shall always be smaller than topLeft.y

Has this code been ever run ?

> I'll ask others what they think about this

As they're smarter than me, they made me notice that you should rather
define a constructor with Point1 and Point2 and construct a Rectangle
that spans between these two points defined using the existing
Rectangle constructor as:

	constexpr Rectangle(const Point &point1, const Point &point2)
                : Rectangle(std::min(point1.x, point2.x), std::max(point1.y, point2.y),
                            std::max(point1.x, point2.x) - std::min(point1.x, point2.x),
                            std::max(point1.y, point2.y) - std::min(point1.y, point2.y))
	{
	}

Please make sure to add proper documentation for this new constructor.

You can also add the following snippet to test/geometry.cpp

		Point topLeft(3, 30);
		Point bottomRight(30, 3);
		Point topRight(30, 30);
		Point bottomLeft(3, 3);
		Rectangle rect1(topLeft, bottomRight);
		Rectangle rect2(topRight, bottomLeft);
		Rectangle rect3(bottomRight, topLeft);
		Rectangle rect4(bottomLeft, topRight);

		if (rect1 != rect2 || rect1 != rect3 || rect1 != rect4) {
			cout << "Point-from-point construction failed" << endl;
			return TestFail;
		}

Thanks
  j

> >
> > BR,
> > Harvey
> >
> >
> > >
> > > Regards,
> > > Barnabás Pőcze
> > >
> > >