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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart