[libcamera-devel,11/14] py: add geometry classes

15917 diff mbox series

Add libcamera's geometry classes to the Python bindings.

Note that this commit only adds the classes, but they are not used
anywhere yet.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 src/py/libcamera/meson.build    |   1 +
 src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++
 src/py/libcamera/pymain.cpp     |   2 +
 3 files changed, 107 insertions(+)
 create mode 100644 src/py/libcamera/pygeometry.cpp

Hi Tomi,

Thank you for the patch.

On Mon, May 16, 2022 at 05:10:19PM +0300, Tomi Valkeinen wrote:
> Add libcamera's geometry classes to the Python bindings.
> 
> Note that this commit only adds the classes, but they are not used
> anywhere yet.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  src/py/libcamera/meson.build    |   1 +
>  src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++
>  src/py/libcamera/pymain.cpp     |   2 +
>  3 files changed, 107 insertions(+)
>  create mode 100644 src/py/libcamera/pygeometry.cpp
> 
> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build
> index af8ba6a5..12e006e7 100644
> --- a/src/py/libcamera/meson.build
> +++ b/src/py/libcamera/meson.build
> @@ -14,6 +14,7 @@ pybind11_dep = pybind11_proj.get_variable('pybind11_dep')
>  
>  pycamera_sources = files([
>      'pyenums.cpp',
> +    'pygeometry.cpp',
>      'pymain.cpp',
>  ])
>  
> diff --git a/src/py/libcamera/pygeometry.cpp b/src/py/libcamera/pygeometry.cpp
> new file mode 100644
> index 00000000..2cd1432e
> --- /dev/null
> +++ b/src/py/libcamera/pygeometry.cpp
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + *
> + * Python bindings - Geometry classes
> + */
> +
> +#include <array>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/libcamera.h>
> +
> +#include <pybind11/operators.h>
> +#include <pybind11/smart_holder.h>
> +#include <pybind11/stl.h>
> +
> +namespace py = pybind11;
> +
> +using namespace libcamera;
> +
> +void init_py_geometry(py::module &m)
> +{
> +	py::class_<Point>(m, "Point")
> +		.def(py::init<>())
> +		.def(py::init<int, int>())
> +		.def_readwrite("x", &Point::x)
> +		.def_readwrite("y", &Point::y)
> +		.def(py::self == py::self)
> +		.def(-py::self)
> +		.def("__str__", &Point::toString)
> +		.def("__repr__", [](const Point &self) {
> +			return "libcamera.Point(" + std::to_string(self.x) + ", " + std::to_string(self.y) + ")";
> +		});
> +
> +	py::class_<Size>(m, "Size")
> +		.def(py::init<>())
> +		.def(py::init<unsigned int, unsigned int>())
> +		.def_readwrite("width", &Size::width)
> +		.def_readwrite("height", &Size::height)
> +		.def_property_readonly("is_null", &Size::isNull)
> +		.def(py::self == py::self)
> +		.def(py::self * float())
> +		.def(py::self / float())
> +		.def(py::self *= float())
> +		.def(py::self /= float())
> +		.def("__str__", &Size::toString)
> +		.def("__repr__", [](const Size &self) {
> +			return "libcamera.Size(" + std::to_string(self.width) + ", " + std::to_string(self.height) + ")";
> +		});
> +
> +	py::class_<SizeRange>(m, "SizeRange")
> +		.def(py::init<>())
> +		.def(py::init<Size>())
> +		.def(py::init<Size, Size>())
> +		.def(py::init<Size, Size, unsigned int, unsigned int>())
> +		.def(py::init<>([](const std::array<unsigned int, 2> &s) {
> +			return SizeRange(Size(std::get<0>(s), std::get<1>(s)));
> +		}))
> +		.def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max) {
> +			return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
> +					 Size(std::get<0>(max), std::get<1>(max)));
> +		}))
> +		.def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max,
> +				   unsigned int hStep, unsigned int vStep) {
> +			return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
> +					 Size(std::get<0>(max), std::get<1>(max)),
> +					 hStep, vStep);
> +		}))
> +		.def_readwrite("min", &SizeRange::min)
> +		.def_readwrite("max", &SizeRange::max)
> +		.def_readwrite("hStep", &SizeRange::hStep)
> +		.def_readwrite("vStep", &SizeRange::vStep)
> +		.def(py::self == py::self)
> +		.def("__str__", &SizeRange::toString)
> +		.def("__repr__", [](const SizeRange &self) {
> +			return py::str("libcamera.SizeRange(({}, {}), ({}, {}), {}, {})")
> +				.format(self.min.width, self.min.height,
> +					self.max.width, self.max.height,
> +					self.hStep, self.vStep);
> +		});
> +
> +	py::class_<Rectangle>(m, "Rectangle")
> +		.def(py::init<>())
> +		.def(py::init<int, int, Size>())
> +		.def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
> +			return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
> +		}))

I'm puzzled a bit by this constructor. Where do you use it, and could
then next constructor be used instead ? If it's meant to cover a common
use case, should the C++ API also implement this ?

> +		.def(py::init<int, int, unsigned int, unsigned int>())
> +		.def(py::init<Size>())
> +		.def_readwrite("x", &Rectangle::x)
> +		.def_readwrite("y", &Rectangle::y)
> +		.def_readwrite("width", &Rectangle::width)
> +		.def_readwrite("height", &Rectangle::height)
> +		.def_property_readonly("is_null", &Rectangle::isNull)
> +		.def_property_readonly("center", &Rectangle::center)
> +		.def_property_readonly("size", &Rectangle::size)
> +		.def_property_readonly("topLeft", &Rectangle::topLeft)
> +		.def(py::self == py::self)
> +		.def("__str__", &Rectangle::toString)
> +		.def("__repr__", [](const Rectangle &self) {
> +			return py::str("libcamera.Rectangle({}, {}, {}, {})")
> +				.format(self.x, self.y, self.width, self.height);
> +		});

Could you add todo comments to remind that the other members of the
geometry classes should be implemented too ?

> +}
> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
> index cc2ddee5..1dd70d9c 100644
> --- a/src/py/libcamera/pymain.cpp
> +++ b/src/py/libcamera/pymain.cpp
> @@ -137,11 +137,13 @@ static void handleRequestCompleted(Request *req)
>  
>  void init_pyenums(py::module &m);
>  void init_pyenums_generated(py::module &m);
> +void init_py_geometry(py::module &m);
>  
>  PYBIND11_MODULE(_libcamera, m)
>  {
>  	init_pyenums(m);
>  	init_pyenums_generated(m);
> +	init_py_geometry(m);

My OCD kicks in :-)

>  
>  	/* Forward declarations */

On 17/05/2022 11:37, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, May 16, 2022 at 05:10:19PM +0300, Tomi Valkeinen wrote:
>> Add libcamera's geometry classes to the Python bindings.
>>
>> Note that this commit only adds the classes, but they are not used
>> anywhere yet.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   src/py/libcamera/meson.build    |   1 +
>>   src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++
>>   src/py/libcamera/pymain.cpp     |   2 +
>>   3 files changed, 107 insertions(+)
>>   create mode 100644 src/py/libcamera/pygeometry.cpp
>>
>> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build
>> index af8ba6a5..12e006e7 100644
>> --- a/src/py/libcamera/meson.build
>> +++ b/src/py/libcamera/meson.build
>> @@ -14,6 +14,7 @@ pybind11_dep = pybind11_proj.get_variable('pybind11_dep')
>>   
>>   pycamera_sources = files([
>>       'pyenums.cpp',
>> +    'pygeometry.cpp',
>>       'pymain.cpp',
>>   ])
>>   
>> diff --git a/src/py/libcamera/pygeometry.cpp b/src/py/libcamera/pygeometry.cpp
>> new file mode 100644
>> index 00000000..2cd1432e
>> --- /dev/null
>> +++ b/src/py/libcamera/pygeometry.cpp
>> @@ -0,0 +1,104 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> + *
>> + * Python bindings - Geometry classes
>> + */
>> +
>> +#include <array>
>> +
>> +#include <libcamera/geometry.h>
>> +#include <libcamera/libcamera.h>
>> +
>> +#include <pybind11/operators.h>
>> +#include <pybind11/smart_holder.h>
>> +#include <pybind11/stl.h>
>> +
>> +namespace py = pybind11;
>> +
>> +using namespace libcamera;
>> +
>> +void init_py_geometry(py::module &m)
>> +{
>> +	py::class_<Point>(m, "Point")
>> +		.def(py::init<>())
>> +		.def(py::init<int, int>())
>> +		.def_readwrite("x", &Point::x)
>> +		.def_readwrite("y", &Point::y)
>> +		.def(py::self == py::self)
>> +		.def(-py::self)
>> +		.def("__str__", &Point::toString)
>> +		.def("__repr__", [](const Point &self) {
>> +			return "libcamera.Point(" + std::to_string(self.x) + ", " + std::to_string(self.y) + ")";
>> +		});
>> +
>> +	py::class_<Size>(m, "Size")
>> +		.def(py::init<>())
>> +		.def(py::init<unsigned int, unsigned int>())
>> +		.def_readwrite("width", &Size::width)
>> +		.def_readwrite("height", &Size::height)
>> +		.def_property_readonly("is_null", &Size::isNull)
>> +		.def(py::self == py::self)
>> +		.def(py::self * float())
>> +		.def(py::self / float())
>> +		.def(py::self *= float())
>> +		.def(py::self /= float())
>> +		.def("__str__", &Size::toString)
>> +		.def("__repr__", [](const Size &self) {
>> +			return "libcamera.Size(" + std::to_string(self.width) + ", " + std::to_string(self.height) + ")";
>> +		});
>> +
>> +	py::class_<SizeRange>(m, "SizeRange")
>> +		.def(py::init<>())
>> +		.def(py::init<Size>())
>> +		.def(py::init<Size, Size>())
>> +		.def(py::init<Size, Size, unsigned int, unsigned int>())
>> +		.def(py::init<>([](const std::array<unsigned int, 2> &s) {
>> +			return SizeRange(Size(std::get<0>(s), std::get<1>(s)));
>> +		}))
>> +		.def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max) {
>> +			return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
>> +					 Size(std::get<0>(max), std::get<1>(max)));
>> +		}))
>> +		.def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max,
>> +				   unsigned int hStep, unsigned int vStep) {
>> +			return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
>> +					 Size(std::get<0>(max), std::get<1>(max)),
>> +					 hStep, vStep);
>> +		}))
>> +		.def_readwrite("min", &SizeRange::min)
>> +		.def_readwrite("max", &SizeRange::max)
>> +		.def_readwrite("hStep", &SizeRange::hStep)
>> +		.def_readwrite("vStep", &SizeRange::vStep)
>> +		.def(py::self == py::self)
>> +		.def("__str__", &SizeRange::toString)
>> +		.def("__repr__", [](const SizeRange &self) {
>> +			return py::str("libcamera.SizeRange(({}, {}), ({}, {}), {}, {})")
>> +				.format(self.min.width, self.min.height,
>> +					self.max.width, self.max.height,
>> +					self.hStep, self.vStep);
>> +		});
>> +
>> +	py::class_<Rectangle>(m, "Rectangle")
>> +		.def(py::init<>())
>> +		.def(py::init<int, int, Size>())
>> +		.def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
>> +			return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
>> +		}))
> 
> I'm puzzled a bit by this constructor. Where do you use it, and could
> then next constructor be used instead ? If it's meant to cover a common
> use case, should the C++ API also implement this ?

It allows:

libcam.Rectangle(1, 2, (3, 4))

It's more obvious with SizeRange:

libcam.SizeRange((1, 2), (3, 4), 5, 6)

instead of

libcam.SizeRange(libcam.Size(1, 2), libcam.Size(3, 4), 5, 6)

I really like tuples in python. I'd also like to do unpacking, like:

w, h = libcam.Size(1, 2)

I think I figured out how to do that, but after sending the series.

These kind of features are perhaps something we need to do a decision 
on: include them, or keep the API as bare-bones as possible.

>> +		.def(py::init<int, int, unsigned int, unsigned int>())
>> +		.def(py::init<Size>())
>> +		.def_readwrite("x", &Rectangle::x)
>> +		.def_readwrite("y", &Rectangle::y)
>> +		.def_readwrite("width", &Rectangle::width)
>> +		.def_readwrite("height", &Rectangle::height)
>> +		.def_property_readonly("is_null", &Rectangle::isNull)
>> +		.def_property_readonly("center", &Rectangle::center)
>> +		.def_property_readonly("size", &Rectangle::size)
>> +		.def_property_readonly("topLeft", &Rectangle::topLeft)
>> +		.def(py::self == py::self)
>> +		.def("__str__", &Rectangle::toString)
>> +		.def("__repr__", [](const Rectangle &self) {
>> +			return py::str("libcamera.Rectangle({}, {}, {}, {})")
>> +				.format(self.x, self.y, self.width, self.height);
>> +		});
> 
> Could you add todo comments to remind that the other members of the
> geometry classes should be implemented too ?

Yes.

>> +}
>> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
>> index cc2ddee5..1dd70d9c 100644
>> --- a/src/py/libcamera/pymain.cpp
>> +++ b/src/py/libcamera/pymain.cpp
>> @@ -137,11 +137,13 @@ static void handleRequestCompleted(Request *req)
>>   
>>   void init_pyenums(py::module &m);
>>   void init_pyenums_generated(py::module &m);
>> +void init_py_geometry(py::module &m);
>>   
>>   PYBIND11_MODULE(_libcamera, m)
>>   {
>>   	init_pyenums(m);
>>   	init_pyenums_generated(m);
>> +	init_py_geometry(m);
> 
> My OCD kicks in :-)

Now that I look at it... Yes...

  Tomi

Hi Tomi,

On Tue, May 17, 2022 at 12:02:13PM +0300, Tomi Valkeinen wrote:
> On 17/05/2022 11:37, Laurent Pinchart wrote:
> > On Mon, May 16, 2022 at 05:10:19PM +0300, Tomi Valkeinen wrote:
> >> Add libcamera's geometry classes to the Python bindings.
> >>
> >> Note that this commit only adds the classes, but they are not used
> >> anywhere yet.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   src/py/libcamera/meson.build    |   1 +
> >>   src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++
> >>   src/py/libcamera/pymain.cpp     |   2 +
> >>   3 files changed, 107 insertions(+)
> >>   create mode 100644 src/py/libcamera/pygeometry.cpp
> >>
> >> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build
> >> index af8ba6a5..12e006e7 100644
> >> --- a/src/py/libcamera/meson.build
> >> +++ b/src/py/libcamera/meson.build
> >> @@ -14,6 +14,7 @@ pybind11_dep = pybind11_proj.get_variable('pybind11_dep')
> >>   
> >>   pycamera_sources = files([
> >>       'pyenums.cpp',
> >> +    'pygeometry.cpp',
> >>       'pymain.cpp',
> >>   ])
> >>   
> >> diff --git a/src/py/libcamera/pygeometry.cpp b/src/py/libcamera/pygeometry.cpp
> >> new file mode 100644
> >> index 00000000..2cd1432e
> >> --- /dev/null
> >> +++ b/src/py/libcamera/pygeometry.cpp
> >> @@ -0,0 +1,104 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> + *
> >> + * Python bindings - Geometry classes
> >> + */
> >> +
> >> +#include <array>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +#include <libcamera/libcamera.h>
> >> +
> >> +#include <pybind11/operators.h>
> >> +#include <pybind11/smart_holder.h>
> >> +#include <pybind11/stl.h>
> >> +
> >> +namespace py = pybind11;
> >> +
> >> +using namespace libcamera;
> >> +
> >> +void init_py_geometry(py::module &m)
> >> +{
> >> +	py::class_<Point>(m, "Point")
> >> +		.def(py::init<>())
> >> +		.def(py::init<int, int>())
> >> +		.def_readwrite("x", &Point::x)
> >> +		.def_readwrite("y", &Point::y)
> >> +		.def(py::self == py::self)
> >> +		.def(-py::self)
> >> +		.def("__str__", &Point::toString)
> >> +		.def("__repr__", [](const Point &self) {
> >> +			return "libcamera.Point(" + std::to_string(self.x) + ", " + std::to_string(self.y) + ")";
> >> +		});
> >> +
> >> +	py::class_<Size>(m, "Size")
> >> +		.def(py::init<>())
> >> +		.def(py::init<unsigned int, unsigned int>())
> >> +		.def_readwrite("width", &Size::width)
> >> +		.def_readwrite("height", &Size::height)
> >> +		.def_property_readonly("is_null", &Size::isNull)
> >> +		.def(py::self == py::self)
> >> +		.def(py::self * float())
> >> +		.def(py::self / float())
> >> +		.def(py::self *= float())
> >> +		.def(py::self /= float())
> >> +		.def("__str__", &Size::toString)
> >> +		.def("__repr__", [](const Size &self) {
> >> +			return "libcamera.Size(" + std::to_string(self.width) + ", " + std::to_string(self.height) + ")";
> >> +		});
> >> +
> >> +	py::class_<SizeRange>(m, "SizeRange")
> >> +		.def(py::init<>())
> >> +		.def(py::init<Size>())
> >> +		.def(py::init<Size, Size>())
> >> +		.def(py::init<Size, Size, unsigned int, unsigned int>())
> >> +		.def(py::init<>([](const std::array<unsigned int, 2> &s) {
> >> +			return SizeRange(Size(std::get<0>(s), std::get<1>(s)));
> >> +		}))
> >> +		.def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max) {
> >> +			return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
> >> +					 Size(std::get<0>(max), std::get<1>(max)));
> >> +		}))
> >> +		.def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max,
> >> +				   unsigned int hStep, unsigned int vStep) {
> >> +			return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
> >> +					 Size(std::get<0>(max), std::get<1>(max)),
> >> +					 hStep, vStep);
> >> +		}))
> >> +		.def_readwrite("min", &SizeRange::min)
> >> +		.def_readwrite("max", &SizeRange::max)
> >> +		.def_readwrite("hStep", &SizeRange::hStep)
> >> +		.def_readwrite("vStep", &SizeRange::vStep)
> >> +		.def(py::self == py::self)
> >> +		.def("__str__", &SizeRange::toString)
> >> +		.def("__repr__", [](const SizeRange &self) {
> >> +			return py::str("libcamera.SizeRange(({}, {}), ({}, {}), {}, {})")
> >> +				.format(self.min.width, self.min.height,
> >> +					self.max.width, self.max.height,
> >> +					self.hStep, self.vStep);
> >> +		});
> >> +
> >> +	py::class_<Rectangle>(m, "Rectangle")
> >> +		.def(py::init<>())
> >> +		.def(py::init<int, int, Size>())
> >> +		.def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
> >> +			return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
> >> +		}))
> > 
> > I'm puzzled a bit by this constructor. Where do you use it, and could
> > then next constructor be used instead ? If it's meant to cover a common
> > use case, should the C++ API also implement this ?
> 
> It allows:
> 
> libcam.Rectangle(1, 2, (3, 4))

And then someone will request being able to write

	libcam.Rectangle((1, 2), (3, 4))

and possibly even

	libcam.Rectangle((1, 2), 3, 4)

:-)

In C++ it's a bit different thanks to implicit constructors and
initializer lists, so I agree that we may want to expose a similar
feature manually to make the code more readable. I'm just concerned
about the possible large number of combinations.

Is there a Python coding style rule (or rather API design rule) about
this ?

> It's more obvious with SizeRange:
> 
> libcam.SizeRange((1, 2), (3, 4), 5, 6)
> 
> instead of
> 
> libcam.SizeRange(libcam.Size(1, 2), libcam.Size(3, 4), 5, 6)
> 
> I really like tuples in python. I'd also like to do unpacking, like:
> 
> w, h = libcam.Size(1, 2)
> 
> I think I figured out how to do that, but after sending the series.
> 
> These kind of features are perhaps something we need to do a decision 
> on: include them, or keep the API as bare-bones as possible.

Good question. I personally prefer using s.width and s.height instead of
unpacking and using w and h in most cases, but that's probably a matter
of personal preference.

I'm not against such "small" differences between the C++ and Python
APIs, if it makes the API better. "better" of course needs to be defined
here, my go-to reference is usually "The Little Manual of API Design" by
Jasmin Blanchette (https://www.cs.vu.nl/~jbe248/api-design.pdf). A good
API should, in my opinion, have two important characteristics:

- Easy to learn and memorize: guessing how to achieve a task should give
  the right answer. Consistency of the API is important here, but
  exceptions are totally fine when they make the API easier to guess.

- Readability: an API piece (e.g. the name of a function on an
  object) should do what you expect it to do when reading it.

> >> +		.def(py::init<int, int, unsigned int, unsigned int>())
> >> +		.def(py::init<Size>())
> >> +		.def_readwrite("x", &Rectangle::x)
> >> +		.def_readwrite("y", &Rectangle::y)
> >> +		.def_readwrite("width", &Rectangle::width)
> >> +		.def_readwrite("height", &Rectangle::height)
> >> +		.def_property_readonly("is_null", &Rectangle::isNull)
> >> +		.def_property_readonly("center", &Rectangle::center)
> >> +		.def_property_readonly("size", &Rectangle::size)
> >> +		.def_property_readonly("topLeft", &Rectangle::topLeft)
> >> +		.def(py::self == py::self)
> >> +		.def("__str__", &Rectangle::toString)
> >> +		.def("__repr__", [](const Rectangle &self) {
> >> +			return py::str("libcamera.Rectangle({}, {}, {}, {})")
> >> +				.format(self.x, self.y, self.width, self.height);
> >> +		});
> > 
> > Could you add todo comments to remind that the other members of the
> > geometry classes should be implemented too ?
> 
> Yes.
> 
> >> +}
> >> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
> >> index cc2ddee5..1dd70d9c 100644
> >> --- a/src/py/libcamera/pymain.cpp
> >> +++ b/src/py/libcamera/pymain.cpp
> >> @@ -137,11 +137,13 @@ static void handleRequestCompleted(Request *req)
> >>   
> >>   void init_pyenums(py::module &m);
> >>   void init_pyenums_generated(py::module &m);
> >> +void init_py_geometry(py::module &m);
> >>   
> >>   PYBIND11_MODULE(_libcamera, m)
> >>   {
> >>   	init_pyenums(m);
> >>   	init_pyenums_generated(m);
> >> +	init_py_geometry(m);
> > 
> > My OCD kicks in :-)
> 
> Now that I look at it... Yes...

On 17/05/2022 12:48, Laurent Pinchart wrote:

>>>> +	py::class_<Rectangle>(m, "Rectangle")
>>>> +		.def(py::init<>())
>>>> +		.def(py::init<int, int, Size>())
>>>> +		.def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
>>>> +			return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
>>>> +		}))
>>>
>>> I'm puzzled a bit by this constructor. Where do you use it, and could
>>> then next constructor be used instead ? If it's meant to cover a common
>>> use case, should the C++ API also implement this ?
>>
>> It allows:
>>
>> libcam.Rectangle(1, 2, (3, 4))
> 
> And then someone will request being able to write
> 
> 	libcam.Rectangle((1, 2), (3, 4))
> 
> and possibly even
> 
> 	libcam.Rectangle((1, 2), 3, 4)
> 
> :-)

Yep. As a user, I would expect/hope all these would work:

Rectangle(Point(1, 2), Size(3, 4))
Rectangle((1, 2), (3, 4))
Rectangle([1, 2], [3, 4])
Rectangle(size=(3,4))  # pos is (0, 0)
Rectangle(1, 2, 3, 4)  # not sure about this, it's a bit confusing

Managing Point, Size, tuples and lists in the above example is solved by 
expecting an object that gives us two ints, instead of expecting 
something specific. If both Point and Size support __iter__, and tuples 
and lists already do, then:

x,y = pos
w,h = size

will work for all those cases.

And looking at the transforming methods, e.g.:

rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))

is a bit annoying to use, compared to:

rect.scale_by((1, 2), (3,4)).translate_by((5,6))

To fix that, I think we would have to overwrite all the methods we want 
to support such conversions. Which does not sound nice.

I did some testing in the __init__.py, and I think we can (re-)implement 
anything we need there. E.g.:

def __Size_iter(self):
     yield from (self.width, self.height)

Size.__iter__ = __Size_iter


and


def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):
     x, y = pos
     w, h = size
     Rectangle.__old_init(self, x, y, w, h)

Rectangle.__old_init = Rectangle.__init__
Rectangle.__init__ = __Rectangle_init

> In C++ it's a bit different thanks to implicit constructors and
> initializer lists, so I agree that we may want to expose a similar
> feature manually to make the code more readable. I'm just concerned
> about the possible large number of combinations.
> 
> Is there a Python coding style rule (or rather API design rule) about
> this ?

I'm not aware of such. In my experience python classes/functions often 
take a wide range of different inputs, and they do what you expect them 
to do. I don't know if that's recommended or is it just just something 
that the authors have implemented because they liked it.

  Tomi

Hi everyone

I see there's been an interesting discussion going on. I'm just going
to toss in my tuppence worth without coming to any firm conclusions,
so I apologise in advance!

* Most importantly libcamera's Python bindings need to expose the
basic libcamera C++ API. I hope that's uncontroversial!

* Generally I'm not convinced it's worth worrying too much about how
"friendly" the Python API is. I'm not saying we should deliberately
make it difficult, but I think the libcamera API is too intimidating
for casual camera users ("help! I just want to capture a picture!") or
even those developing Python applications who would probably
appreciate something higher level ("I just want clicking on this
button to start the camera!").

* So I wouldn't do lots of work or add lots of features to try and
achieve that, though making it Pythonic and easy-to-use wherever we
can is of course still desirable.

* I think there's maybe an argument for a "friendly" and slightly
higher level (?) libcamera Python API on top of the basic one (a bit
like Picamera2 is for us). But maybe there should be a distinction?
Not sure.

* I'm not sure what I think of providing all the geometry classes. On
one hand there's no harm in it, on the other... wouldn't most Python
programmers prefer to deal with tuples?

Sorry if I'm being annoying!

David

On Tue, 17 May 2022 at 13:56, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 17/05/2022 12:48, Laurent Pinchart wrote:
>
> >>>> +  py::class_<Rectangle>(m, "Rectangle")
> >>>> +          .def(py::init<>())
> >>>> +          .def(py::init<int, int, Size>())
> >>>> +          .def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
> >>>> +                  return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
> >>>> +          }))
> >>>
> >>> I'm puzzled a bit by this constructor. Where do you use it, and could
> >>> then next constructor be used instead ? If it's meant to cover a common
> >>> use case, should the C++ API also implement this ?
> >>
> >> It allows:
> >>
> >> libcam.Rectangle(1, 2, (3, 4))
> >
> > And then someone will request being able to write
> >
> >       libcam.Rectangle((1, 2), (3, 4))
> >
> > and possibly even
> >
> >       libcam.Rectangle((1, 2), 3, 4)
> >
> > :-)
>
> Yep. As a user, I would expect/hope all these would work:
>
> Rectangle(Point(1, 2), Size(3, 4))
> Rectangle((1, 2), (3, 4))
> Rectangle([1, 2], [3, 4])
> Rectangle(size=(3,4))  # pos is (0, 0)
> Rectangle(1, 2, 3, 4)  # not sure about this, it's a bit confusing
>
> Managing Point, Size, tuples and lists in the above example is solved by
> expecting an object that gives us two ints, instead of expecting
> something specific. If both Point and Size support __iter__, and tuples
> and lists already do, then:
>
> x,y = pos
> w,h = size
>
> will work for all those cases.
>
> And looking at the transforming methods, e.g.:
>
> rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))
>
> is a bit annoying to use, compared to:
>
> rect.scale_by((1, 2), (3,4)).translate_by((5,6))
>
> To fix that, I think we would have to overwrite all the methods we want
> to support such conversions. Which does not sound nice.
>
> I did some testing in the __init__.py, and I think we can (re-)implement
> anything we need there. E.g.:
>
> def __Size_iter(self):
>      yield from (self.width, self.height)
>
> Size.__iter__ = __Size_iter
>
>
> and
>
>
> def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):
>      x, y = pos
>      w, h = size
>      Rectangle.__old_init(self, x, y, w, h)
>
> Rectangle.__old_init = Rectangle.__init__
> Rectangle.__init__ = __Rectangle_init
>
> > In C++ it's a bit different thanks to implicit constructors and
> > initializer lists, so I agree that we may want to expose a similar
> > feature manually to make the code more readable. I'm just concerned
> > about the possible large number of combinations.
> >
> > Is there a Python coding style rule (or rather API design rule) about
> > this ?
>
> I'm not aware of such. In my experience python classes/functions often
> take a wide range of different inputs, and they do what you expect them
> to do. I don't know if that's recommended or is it just just something
> that the authors have implemented because they liked it.
>
>   Tomi

Hi David,

On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:
> Hi everyone
> 
> I see there's been an interesting discussion going on. I'm just going
> to toss in my tuppence worth without coming to any firm conclusions,
> so I apologise in advance!
> 
> * Most importantly libcamera's Python bindings need to expose the
> basic libcamera C++ API. I hope that's uncontroversial!
> 
> * Generally I'm not convinced it's worth worrying too much about how
> "friendly" the Python API is. I'm not saying we should deliberately
> make it difficult, but I think the libcamera API is too intimidating
> for casual camera users ("help! I just want to capture a picture!") or
> even those developing Python applications who would probably
> appreciate something higher level ("I just want clicking on this
> button to start the camera!").
> 
> * So I wouldn't do lots of work or add lots of features to try and
> achieve that, though making it Pythonic and easy-to-use wherever we
> can is of course still desirable.

That seems reasonable, we can start by staying close to the C++ API with
a minimum amount of "pythonic" rework of the API, and then improve on
top later.

> * I think there's maybe an argument for a "friendly" and slightly
> higher level (?) libcamera Python API on top of the basic one (a bit
> like Picamera2 is for us). But maybe there should be a distinction?
> Not sure.
> 
> * I'm not sure what I think of providing all the geometry classes. On
> one hand there's no harm in it, on the other... wouldn't most Python
> programmers prefer to deal with tuples?

I can't tell about "most Python programmers", but I find

	foo(Size(100, 100), Size(500, 500))

more readable than

	foo((100, 100), (500, 500))

as the latter doesn't clearly say if we're dealing with points or sizes
(or something else). I don't know if that's relevant, but
https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html
doesn't provide a constructor that takes two lists or tuples.

> Sorry if I'm being annoying!

You're not :-)

> On Tue, 17 May 2022 at 13:56, Tomi Valkeinen wrote:
> > On 17/05/2022 12:48, Laurent Pinchart wrote:
> >
> > >>>> +  py::class_<Rectangle>(m, "Rectangle")
> > >>>> +          .def(py::init<>())
> > >>>> +          .def(py::init<int, int, Size>())
> > >>>> +          .def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
> > >>>> +                  return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
> > >>>> +          }))
> > >>>
> > >>> I'm puzzled a bit by this constructor. Where do you use it, and could
> > >>> then next constructor be used instead ? If it's meant to cover a common
> > >>> use case, should the C++ API also implement this ?
> > >>
> > >> It allows:
> > >>
> > >> libcam.Rectangle(1, 2, (3, 4))
> > >
> > > And then someone will request being able to write
> > >
> > >       libcam.Rectangle((1, 2), (3, 4))
> > >
> > > and possibly even
> > >
> > >       libcam.Rectangle((1, 2), 3, 4)
> > >
> > > :-)
> >
> > Yep. As a user, I would expect/hope all these would work:
> >
> > Rectangle(Point(1, 2), Size(3, 4))
> > Rectangle((1, 2), (3, 4))
> > Rectangle([1, 2], [3, 4])
> > Rectangle(size=(3,4))  # pos is (0, 0)
> > Rectangle(1, 2, 3, 4)  # not sure about this, it's a bit confusing
> >
> > Managing Point, Size, tuples and lists in the above example is solved by
> > expecting an object that gives us two ints, instead of expecting
> > something specific. If both Point and Size support __iter__, and tuples
> > and lists already do, then:
> >
> > x,y = pos
> > w,h = size
> >
> > will work for all those cases.
> >
> > And looking at the transforming methods, e.g.:
> >
> > rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))
> >
> > is a bit annoying to use, compared to:
> >
> > rect.scale_by((1, 2), (3,4)).translate_by((5,6))
> >
> > To fix that, I think we would have to overwrite all the methods we want
> > to support such conversions. Which does not sound nice.
> >
> > I did some testing in the __init__.py, and I think we can (re-)implement
> > anything we need there. E.g.:
> >
> > def __Size_iter(self):
> >      yield from (self.width, self.height)
> >
> > Size.__iter__ = __Size_iter
> >
> >
> > and
> >
> >
> > def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):
> >      x, y = pos
> >      w, h = size
> >      Rectangle.__old_init(self, x, y, w, h)
> >
> > Rectangle.__old_init = Rectangle.__init__
> > Rectangle.__init__ = __Rectangle_init
> >
> > > In C++ it's a bit different thanks to implicit constructors and
> > > initializer lists, so I agree that we may want to expose a similar
> > > feature manually to make the code more readable. I'm just concerned
> > > about the possible large number of combinations.
> > >
> > > Is there a Python coding style rule (or rather API design rule) about
> > > this ?
> >
> > I'm not aware of such. In my experience python classes/functions often
> > take a wide range of different inputs, and they do what you expect them
> > to do. I don't know if that's recommended or is it just just something
> > that the authors have implemented because they liked it.

On 17/05/2022 16:56, Laurent Pinchart wrote:
> Hi David,
> 
> On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:
>> Hi everyone
>>
>> I see there's been an interesting discussion going on. I'm just going
>> to toss in my tuppence worth without coming to any firm conclusions,
>> so I apologise in advance!
>>
>> * Most importantly libcamera's Python bindings need to expose the
>> basic libcamera C++ API. I hope that's uncontroversial!
>>
>> * Generally I'm not convinced it's worth worrying too much about how
>> "friendly" the Python API is. I'm not saying we should deliberately
>> make it difficult, but I think the libcamera API is too intimidating
>> for casual camera users ("help! I just want to capture a picture!") or
>> even those developing Python applications who would probably
>> appreciate something higher level ("I just want clicking on this
>> button to start the camera!").
>>
>> * So I wouldn't do lots of work or add lots of features to try and
>> achieve that, though making it Pythonic and easy-to-use wherever we
>> can is of course still desirable.
> 
> That seems reasonable, we can start by staying close to the C++ API with
> a minimum amount of "pythonic" rework of the API, and then improve on
> top later.
> 
>> * I think there's maybe an argument for a "friendly" and slightly
>> higher level (?) libcamera Python API on top of the basic one (a bit
>> like Picamera2 is for us). But maybe there should be a distinction?
>> Not sure.
>>
>> * I'm not sure what I think of providing all the geometry classes. On
>> one hand there's no harm in it, on the other... wouldn't most Python
>> programmers prefer to deal with tuples?

The geometry classes do offer quite many transformation functions. 
Having the classes also makes the bindings simpler, as we don't need to 
convert between tuples and the proper geometry class in multiple places 
(see the "py: use geometry classes").

> I can't tell about "most Python programmers", but I find
> 
> 	foo(Size(100, 100), Size(500, 500))
> 
> more readable than
> 
> 	foo((100, 100), (500, 500))

For "foo", I agree, as I have no clue what it does ;).

For Rect, I agree also, as you can well have Rect(Point, Point) and 
Rect(Point, Size).

I was a bit surprised to find Rectangle(int, int, uint, uint) 
constructor on the C++ side, as it's not obvious if the latter uints are 
for a Point or Size. It also doesn't have Rectangle(Point, Size) at all.

In any case, I've dropped the "friendly" code from the series for now.

> as the latter doesn't clearly say if we're dealing with points or sizes
> (or something else). I don't know if that's relevant, but
> https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html
> doesn't provide a constructor that takes two lists or tuples.

I don't think Qt is very pythonic.

  Tomi

Hi Tomi,

On Tue, May 17, 2022 at 05:17:46PM +0300, Tomi Valkeinen wrote:
> On 17/05/2022 16:56, Laurent Pinchart wrote:
> > On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:
> >> Hi everyone
> >>
> >> I see there's been an interesting discussion going on. I'm just going
> >> to toss in my tuppence worth without coming to any firm conclusions,
> >> so I apologise in advance!
> >>
> >> * Most importantly libcamera's Python bindings need to expose the
> >> basic libcamera C++ API. I hope that's uncontroversial!
> >>
> >> * Generally I'm not convinced it's worth worrying too much about how
> >> "friendly" the Python API is. I'm not saying we should deliberately
> >> make it difficult, but I think the libcamera API is too intimidating
> >> for casual camera users ("help! I just want to capture a picture!") or
> >> even those developing Python applications who would probably
> >> appreciate something higher level ("I just want clicking on this
> >> button to start the camera!").
> >>
> >> * So I wouldn't do lots of work or add lots of features to try and
> >> achieve that, though making it Pythonic and easy-to-use wherever we
> >> can is of course still desirable.
> > 
> > That seems reasonable, we can start by staying close to the C++ API with
> > a minimum amount of "pythonic" rework of the API, and then improve on
> > top later.
> > 
> >> * I think there's maybe an argument for a "friendly" and slightly
> >> higher level (?) libcamera Python API on top of the basic one (a bit
> >> like Picamera2 is for us). But maybe there should be a distinction?
> >> Not sure.
> >>
> >> * I'm not sure what I think of providing all the geometry classes. On
> >> one hand there's no harm in it, on the other... wouldn't most Python
> >> programmers prefer to deal with tuples?
> 
> The geometry classes do offer quite many transformation functions. 
> Having the classes also makes the bindings simpler, as we don't need to 
> convert between tuples and the proper geometry class in multiple places 
> (see the "py: use geometry classes").
> 
> > I can't tell about "most Python programmers", but I find
> > 
> > 	foo(Size(100, 100), Size(500, 500))
> > 
> > more readable than
> > 
> > 	foo((100, 100), (500, 500))
> 
> For "foo", I agree, as I have no clue what it does ;).
> 
> For Rect, I agree also, as you can well have Rect(Point, Point) and 
> Rect(Point, Size).
> 
> I was a bit surprised to find Rectangle(int, int, uint, uint) 
> constructor on the C++ side, as it's not obvious if the latter uints are 
> for a Point or Size. It also doesn't have Rectangle(Point, Size) at all.

You're right about that. At least the latter should be fixed, and
possibly the former too.

> In any case, I've dropped the "friendly" code from the series for now.
> 
> > as the latter doesn't clearly say if we're dealing with points or sizes
> > (or something else). I don't know if that's relevant, but
> > https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html
> > doesn't provide a constructor that takes two lists or tuples.
> 
> I don't think Qt is very pythonic.

That's a fair point, and maybe why my APIs are more "Qt-ic" than
pythonic :-)

Hi Laurent, everyone

On Tue, 17 May 2022 at 14:56, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:
> > Hi everyone
> >
> > I see there's been an interesting discussion going on. I'm just going
> > to toss in my tuppence worth without coming to any firm conclusions,
> > so I apologise in advance!
> >
> > * Most importantly libcamera's Python bindings need to expose the
> > basic libcamera C++ API. I hope that's uncontroversial!
> >
> > * Generally I'm not convinced it's worth worrying too much about how
> > "friendly" the Python API is. I'm not saying we should deliberately
> > make it difficult, but I think the libcamera API is too intimidating
> > for casual camera users ("help! I just want to capture a picture!") or
> > even those developing Python applications who would probably
> > appreciate something higher level ("I just want clicking on this
> > button to start the camera!").
> >
> > * So I wouldn't do lots of work or add lots of features to try and
> > achieve that, though making it Pythonic and easy-to-use wherever we
> > can is of course still desirable.
>
> That seems reasonable, we can start by staying close to the C++ API with
> a minimum amount of "pythonic" rework of the API, and then improve on
> top later.
>
> > * I think there's maybe an argument for a "friendly" and slightly
> > higher level (?) libcamera Python API on top of the basic one (a bit
> > like Picamera2 is for us). But maybe there should be a distinction?
> > Not sure.
> >
> > * I'm not sure what I think of providing all the geometry classes. On
> > one hand there's no harm in it, on the other... wouldn't most Python
> > programmers prefer to deal with tuples?
>
> I can't tell about "most Python programmers", but I find
>
>         foo(Size(100, 100), Size(500, 500))
>
> more readable than
>
>         foo((100, 100), (500, 500))

That's true, but from my vast experience of "a few months" writing
not-completely-trivial Python I think I might quite like:

foo(offset=(100, 100), size=(500, 500))

I really like keyword args as they reduce silly mistakes and are
self-documenting. Of course there's also "foo(offset=Point(100, 100),
size=Size(500, 500))", that works too.

In my (again, vast!) experience I find that Python often seems to use
lots of lists/tuples and dicts where in the C/C++ world we'd have
structs, so it can sometimes feel a bit unstructured to me. But I'm
not sure what I prefer...

David

>
> as the latter doesn't clearly say if we're dealing with points or sizes
> (or something else). I don't know if that's relevant, but
> https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html
> doesn't provide a constructor that takes two lists or tuples.
>
> > Sorry if I'm being annoying!
>
> You're not :-)
>
> > On Tue, 17 May 2022 at 13:56, Tomi Valkeinen wrote:
> > > On 17/05/2022 12:48, Laurent Pinchart wrote:
> > >
> > > >>>> +  py::class_<Rectangle>(m, "Rectangle")
> > > >>>> +          .def(py::init<>())
> > > >>>> +          .def(py::init<int, int, Size>())
> > > >>>> +          .def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
> > > >>>> +                  return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
> > > >>>> +          }))
> > > >>>
> > > >>> I'm puzzled a bit by this constructor. Where do you use it, and could
> > > >>> then next constructor be used instead ? If it's meant to cover a common
> > > >>> use case, should the C++ API also implement this ?
> > > >>
> > > >> It allows:
> > > >>
> > > >> libcam.Rectangle(1, 2, (3, 4))
> > > >
> > > > And then someone will request being able to write
> > > >
> > > >       libcam.Rectangle((1, 2), (3, 4))
> > > >
> > > > and possibly even
> > > >
> > > >       libcam.Rectangle((1, 2), 3, 4)
> > > >
> > > > :-)
> > >
> > > Yep. As a user, I would expect/hope all these would work:
> > >
> > > Rectangle(Point(1, 2), Size(3, 4))
> > > Rectangle((1, 2), (3, 4))
> > > Rectangle([1, 2], [3, 4])
> > > Rectangle(size=(3,4))  # pos is (0, 0)
> > > Rectangle(1, 2, 3, 4)  # not sure about this, it's a bit confusing
> > >
> > > Managing Point, Size, tuples and lists in the above example is solved by
> > > expecting an object that gives us two ints, instead of expecting
> > > something specific. If both Point and Size support __iter__, and tuples
> > > and lists already do, then:
> > >
> > > x,y = pos
> > > w,h = size
> > >
> > > will work for all those cases.
> > >
> > > And looking at the transforming methods, e.g.:
> > >
> > > rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))
> > >
> > > is a bit annoying to use, compared to:
> > >
> > > rect.scale_by((1, 2), (3,4)).translate_by((5,6))
> > >
> > > To fix that, I think we would have to overwrite all the methods we want
> > > to support such conversions. Which does not sound nice.
> > >
> > > I did some testing in the __init__.py, and I think we can (re-)implement
> > > anything we need there. E.g.:
> > >
> > > def __Size_iter(self):
> > >      yield from (self.width, self.height)
> > >
> > > Size.__iter__ = __Size_iter
> > >
> > >
> > > and
> > >
> > >
> > > def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):
> > >      x, y = pos
> > >      w, h = size
> > >      Rectangle.__old_init(self, x, y, w, h)
> > >
> > > Rectangle.__old_init = Rectangle.__init__
> > > Rectangle.__init__ = __Rectangle_init
> > >
> > > > In C++ it's a bit different thanks to implicit constructors and
> > > > initializer lists, so I agree that we may want to expose a similar
> > > > feature manually to make the code more readable. I'm just concerned
> > > > about the possible large number of combinations.
> > > >
> > > > Is there a Python coding style rule (or rather API design rule) about
> > > > this ?
> > >
> > > I'm not aware of such. In my experience python classes/functions often
> > > take a wide range of different inputs, and they do what you expect them
> > > to do. I don't know if that's recommended or is it just just something
> > > that the authors have implemented because they liked it.
>
> --
> Regards,
>
> Laurent Pinchart

Hi David,

On Tue, May 17, 2022 at 04:21:18PM +0100, David Plowman wrote:
> On Tue, 17 May 2022 at 14:56, Laurent Pinchart wrote:
> > On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:
> > > Hi everyone
> > >
> > > I see there's been an interesting discussion going on. I'm just going
> > > to toss in my tuppence worth without coming to any firm conclusions,
> > > so I apologise in advance!
> > >
> > > * Most importantly libcamera's Python bindings need to expose the
> > > basic libcamera C++ API. I hope that's uncontroversial!
> > >
> > > * Generally I'm not convinced it's worth worrying too much about how
> > > "friendly" the Python API is. I'm not saying we should deliberately
> > > make it difficult, but I think the libcamera API is too intimidating
> > > for casual camera users ("help! I just want to capture a picture!") or
> > > even those developing Python applications who would probably
> > > appreciate something higher level ("I just want clicking on this
> > > button to start the camera!").
> > >
> > > * So I wouldn't do lots of work or add lots of features to try and
> > > achieve that, though making it Pythonic and easy-to-use wherever we
> > > can is of course still desirable.
> >
> > That seems reasonable, we can start by staying close to the C++ API with
> > a minimum amount of "pythonic" rework of the API, and then improve on
> > top later.
> >
> > > * I think there's maybe an argument for a "friendly" and slightly
> > > higher level (?) libcamera Python API on top of the basic one (a bit
> > > like Picamera2 is for us). But maybe there should be a distinction?
> > > Not sure.
> > >
> > > * I'm not sure what I think of providing all the geometry classes. On
> > > one hand there's no harm in it, on the other... wouldn't most Python
> > > programmers prefer to deal with tuples?
> >
> > I can't tell about "most Python programmers", but I find
> >
> >         foo(Size(100, 100), Size(500, 500))
> >
> > more readable than
> >
> >         foo((100, 100), (500, 500))
> 
> That's true, but from my vast experience of "a few months" writing
> not-completely-trivial Python I think I might quite like:
> 
> foo(offset=(100, 100), size=(500, 500))

I agree with you, that's readable, and it's the style we should adopt in
our own code. It doesn't prevent people from shooting themselves in the
foot by writing

	foo((100, 100), (500, 500))

but maybe it's not our role to do so. I would however like to prevent

	foo(Point(100, 100), Point(100, 100))

from being silently ignored, which I think would be the case if the
bindings only took tuple arguments and relied on geometry classes
implementing __iter__(). I don't know if there's an easy way to handle
this though.

> I really like keyword args as they reduce silly mistakes and are
> self-documenting. Of course there's also "foo(offset=Point(100, 100),
> size=Size(500, 500))", that works too.
> 
> In my (again, vast!) experience I find that Python often seems to use
> lots of lists/tuples and dicts where in the C/C++ world we'd have
> structs, so it can sometimes feel a bit unstructured to me. But I'm
> not sure what I prefer...

I wonder if that coud be caused by the fact that creating structures
with named fields is more difficult in Python than in C/C++.

> > as the latter doesn't clearly say if we're dealing with points or sizes
> > (or something else). I don't know if that's relevant, but
> > https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html
> > doesn't provide a constructor that takes two lists or tuples.
> >
> > > Sorry if I'm being annoying!
> >
> > You're not :-)
> >
> > > On Tue, 17 May 2022 at 13:56, Tomi Valkeinen wrote:
> > > > On 17/05/2022 12:48, Laurent Pinchart wrote:
> > > >
> > > > >>>> +  py::class_<Rectangle>(m, "Rectangle")
> > > > >>>> +          .def(py::init<>())
> > > > >>>> +          .def(py::init<int, int, Size>())
> > > > >>>> +          .def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
> > > > >>>> +                  return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
> > > > >>>> +          }))
> > > > >>>
> > > > >>> I'm puzzled a bit by this constructor. Where do you use it, and could
> > > > >>> then next constructor be used instead ? If it's meant to cover a common
> > > > >>> use case, should the C++ API also implement this ?
> > > > >>
> > > > >> It allows:
> > > > >>
> > > > >> libcam.Rectangle(1, 2, (3, 4))
> > > > >
> > > > > And then someone will request being able to write
> > > > >
> > > > >       libcam.Rectangle((1, 2), (3, 4))
> > > > >
> > > > > and possibly even
> > > > >
> > > > >       libcam.Rectangle((1, 2), 3, 4)
> > > > >
> > > > > :-)
> > > >
> > > > Yep. As a user, I would expect/hope all these would work:
> > > >
> > > > Rectangle(Point(1, 2), Size(3, 4))
> > > > Rectangle((1, 2), (3, 4))
> > > > Rectangle([1, 2], [3, 4])
> > > > Rectangle(size=(3,4))  # pos is (0, 0)
> > > > Rectangle(1, 2, 3, 4)  # not sure about this, it's a bit confusing
> > > >
> > > > Managing Point, Size, tuples and lists in the above example is solved by
> > > > expecting an object that gives us two ints, instead of expecting
> > > > something specific. If both Point and Size support __iter__, and tuples
> > > > and lists already do, then:
> > > >
> > > > x,y = pos
> > > > w,h = size
> > > >
> > > > will work for all those cases.
> > > >
> > > > And looking at the transforming methods, e.g.:
> > > >
> > > > rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))
> > > >
> > > > is a bit annoying to use, compared to:
> > > >
> > > > rect.scale_by((1, 2), (3,4)).translate_by((5,6))
> > > >
> > > > To fix that, I think we would have to overwrite all the methods we want
> > > > to support such conversions. Which does not sound nice.
> > > >
> > > > I did some testing in the __init__.py, and I think we can (re-)implement
> > > > anything we need there. E.g.:
> > > >
> > > > def __Size_iter(self):
> > > >      yield from (self.width, self.height)
> > > >
> > > > Size.__iter__ = __Size_iter
> > > >
> > > >
> > > > and
> > > >
> > > >
> > > > def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):
> > > >      x, y = pos
> > > >      w, h = size
> > > >      Rectangle.__old_init(self, x, y, w, h)
> > > >
> > > > Rectangle.__old_init = Rectangle.__init__
> > > > Rectangle.__init__ = __Rectangle_init
> > > >
> > > > > In C++ it's a bit different thanks to implicit constructors and
> > > > > initializer lists, so I agree that we may want to expose a similar
> > > > > feature manually to make the code more readable. I'm just concerned
> > > > > about the possible large number of combinations.
> > > > >
> > > > > Is there a Python coding style rule (or rather API design rule) about
> > > > > this ?
> > > >
> > > > I'm not aware of such. In my experience python classes/functions often
> > > > take a wide range of different inputs, and they do what you expect them
> > > > to do. I don't know if that's recommended or is it just just something
> > > > that the authors have implemented because they liked it.