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

[David Plowman]

Hi Laurent, everyone

On Tue, 17 May 2022 at 14:56, Laurent Pinchart
<laurent.pinchart at 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