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

[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.

-- 
Regards,

Laurent Pinchart