Feedback on the Proof of Concept

[SergioRAgostinho]

The proof of concept looks interesting. I have some questions, comments and suggestions.

Questions

Tooling Choices

• You picked binder and pybind11. From quickly skimming through binder it seems to address the most fundamental problem of automatically identifying which bindings to generate. PCL already uses boost in its pipeline, so there's a natural gravitation from us to adopt boost.python instead pybind11, by default. Do you have a preference for any of them and why?
• Do you know if there are other binder alike tools which perform the same role but integrate with Boost.Python?

State of the Art

There are currently other projects which provide partial and some full PCL bindings:

• https://github.com/strawlab/python-pcl
• https://github.com/dimatura/pypcd
• https://github.com/davidcaron/pclpy
• https://github.com/cmpute/pcl.py

It's important to research them a little and understand what are their current limitations and why. What sort of tooling they use, what bindings are they exactly offering. Believe me when I say that learning a little bit more about them is not wasted time. Present your conclusions in the proposal.

There's also value in understanding how other projects solved the issue:

• OpenCV
• VTK
• Open3D

Comments

Package organization

PCL's modules naturally fall into common python package/module organization. In this proof of concept, the ideal syntax would have been to have TimeTrigger under pcl.common.TimeTrigger

from pcl.common import TimeTrigger # this is my ideal usage

A good rule of thumb you can use to understand how to organize things is to look at header paths e.g.: pcl::PointXYZ is defined under <pcl/point_types.h> and as such I would expect it to reside under pcl.PointXYZ; TimeTrigger on the other hand is under <pcl/common/time_trigger.h> and hence it should reside in pcl.common.TimeTrigger

Build system

PCL uses CMake as its build system and everything you did on your shell script, in theory can be accomplished with CMake: finding dependencies and launching your build commands. I would recommend to migrate the shell script to a proper CMake approach

Next Step Suggestions

1. CMake adoption: what I mentioned earlier, to be merged in to the project, these bindings need to be generated for all our platforms. CMake will help you accomplish that.
2. Make bindings for a full PCL module -> common. This one might be a steep curve but it will help you identify more problems that are yet to be solved.

[kunaltyagi]

I concur on the overall aspect of package organization, next steps and the existing PCL bindings research. Some of them also present higher level algorithms which can benefit C++ community also if implemented in C++.

[shiveshkhaitan]

@SergioRAgostinho @kunaltyagi Thank you for the feedback.

PCL already uses boost in its pipeline, so there's a natural gravitation from us to adopt boost.python instead pybind11, by default. Do you have a preference for any of them and why?

I just started with binder as it was mentioned in the ideas list. And it used pybind11 by default. I will look into boost.python.

• Do you know if there are other binder alike tools which perform the same role but integrate with Boost.Python?

Currently I am unaware of any such tools. I will try to find one.

I will look into the other bindings and fix the package organization.

Regarding the build system, I have incorporated make_bindings_via_cmake.py in 9fc7c404964ada06c80a6c2be9e01252e766d90a which generates a CMakeLists.txt.

[SergioRAgostinho]

Regarding the build system, I have incorporated make_bindings_via_cmake.py in 9fc7c40 which generates a CMakeLists.txt.

Why do you need to generate the cmake file indirectly through a python script?

[shiveshkhaitan]

Why do you need to generate the cmake file indirectly through a python script?

It is not necessary. This just automates the binding process.

[SergioRAgostinho]

This just automates the binding process.

Does this imply that you cannot automate the binding generation process by simply using cmake?

3rd party discovery

There are mechanisms to do this in cmake through find_package, find_library, find_path, find_program, etc... https://github.com/shiveshkhaitan/pcl_bindings/blob/9fc7c404964ada06c80a6c2be9e01252e766d90a/time_trigger/make_bindings_via_cmake.py#L11-L26

Globbing

Check the command file https://github.com/shiveshkhaitan/pcl_bindings/blob/9fc7c404964ada06c80a6c2be9e01252e766d90a/time_trigger/make_bindings_via_cmake.py#L29-L49

Invoking binder

Check add_custom_target https://github.com/shiveshkhaitan/pcl_bindings/blob/9fc7c404964ada06c80a6c2be9e01252e766d90a/time_trigger/make_bindings_via_cmake.py#L52-L67

Building stuff

You're literally writing cmake at this point.

https://github.com/shiveshkhaitan/pcl_bindings/blob/9fc7c404964ada06c80a6c2be9e01252e766d90a/time_trigger/make_bindings_via_cmake.py#L70-L91

The goal is to have a custom target in CMake in which I can literally invoke $ make python and it generates the bindings. The use of python should be limited to strictly necessary.

[shiveshkhaitan]

The goal is to have a custom target in CMake in which I can literally invoke

So binder should be executed from the CMake directly?

[shiveshkhaitan]

Ok I will incorporate the required changes. Currently I am facing some issues with Eigen and Anonymous structs. Once I fix those, I can go ahead and bind the complete common module.

[kunaltyagi]

Official documentation regarding use with CMake might help you

[shiveshkhaitan]

I was able to bind a major portion of common module here. Following is the status of the work so far.

• There are some operator functions and few others which did not get bound (I still need to figure out the reason why) but lot of the PCL_EXPORTS entities were successfully bound.
• The common.config which sets the functions, classes etc to be bound were generated by libtooling.

• There were a lot of changes required in PCL which I have done here. I would appreciate a feedback on whether I am allowed to do the changes. The changes include :

  1. defining PCL_EXPORTS as __attribute__((visibility("default"))) for libtooling to identify which entities need to be bound
  2. Since pybind11 does not support overloaded functions of which one instance is static and the other non-static, I had to change a function name
  3. Since pybind11 does not support function arguments with double pointers, I had to pass uintptr_t* and then do type-casting.
  4. Since pybind11 does not support anonymous structs and unions, I removed them and moved out the variables inside anonymous structs and unions.
  5. In point_types.hpp I had to move the operator function declarations after the struct declaration they were using.
  6. The others were changing # include " ... " to # include < ... > and adding some addtional includes in the impl files.

Next steps

• Integrate the clang plugin for extracting PCL_EXPORTS with binder
• Correct the project structure to allow from pcl import common
• Execute binder from cmake.

[kunaltyagi]

Nice work. A few comments regarding the problems faced and the modifications:

1. I expected PCL_EXPORT needs to be changed for GCC/Clang. My personal end goal is to have PCL working with -fvisibility=hidden and -fvisibility-inlines-hidden or equivalent flags. This is a step in the right direction
2. Rather than changing the functions for pybind11's inability to deal with double pointers, we might create 2 versions, one for pybind11, one for normal, switched by macros
3. Forward declarations could also be a way forward, but it's ok to move around things in a header.
4. The changes to include files sound reasonable.
5. One function being static and overload not being static might be an error, can't say without looking at the actual code.
6. I can't comment on other changes, without looking at a patch.
7. Maintaining common.config sounds like a weird headache. Any way to only declare what not to export (since those are only a few lines compared to the export list)?
8. Any idea how extensive would the changes need to be to achieve from pcl import common?
9. Moving things out of union sounds dangerous. This might require a longer discussion on how to handle it. It such structs are internal interface, then it's not an issue. Anonymous structs should not be on the API boundaries though.

This is some nice progress. You should look into integrating with CMake as well as creating this out of the module, ie. from an independent module. Eg:

- pcl
|- common
|- python_bindings
  |- pcl.config
  |- common.config
  |- CMakeLists.txt
  |- binding_generator.py (or binding_generator.sh)

Then make python_bindings would create bindings from a whitelist, similar to how make format works. At least that's how I feel. Others might be able to provide more feedback on this topic.

Have you thought how to generate bindings for different python and PCL versions? Eg: PCL 1.10, 1.9 as well as python 3.6 and python 3.8? They have different ABI and will need different bindings if I'm not wrong.

[shiveshkhaitan]

Thank you for the feedback @kunaltyagi.

I expected PCL_EXPORT needs to be changed for GCC/Clang. My personal end goal is to have PCL working with -fvisibility=hidden and -fvisibility-inlines-hidden or equivalent flags. This is a step in the right direction

Can you please elaborate on this. As far as I understood the PCL_EXPORTS equivalent is -fvisibility=default and not -fvisibility=hidden

One function being static and overload not being static might be an error, can't say without looking at the actual code.

This is the function which I had to change from getEigenVector3f -> getEigenVector3f_static as there are other getEigenVector3f which are non-static.

Maintaining common.config sounds like a weird headache. Any way to only declare what not to export (since those are only a few lines compared to the export list)?

This does not have to be done manually. The clang plugin would be handling this. Actually it does not even need to be generated separately once I incorporate the plugin into Binder itself.

Any idea how extensive would the changes need to be to achieve from pcl import common?

It should not be very difficult. Probably I just have to set the root_module right and modify the include folder structure.

Moving things out of union sounds dangerous. This might require a longer discussion on how to handle it. It such structs are internal interface, then it's not an issue. Anonymous structs should not be on the API boundaries though.

I will see if I can modify binder to bind anonymous entities

Have you thought how to generate bindings for different python and PCL versions? Eg: PCL 1.10, 1.9 as well as python 3.6 and python 3.8? They have different ABI and will need different bindings if I'm not wrong.

Thank you for pointing this out. I had not thought of it before. I will look into it and get back to you.

[kunaltyagi]

I will see if I can modify binder to bind anonymous entities

Where are they on the API boundaries? In the Point Types?

As far as I understood the PCL_EXPORTS equivalent is -fvisibility=default and not -fvisibility=hidden

By default GCC compiles with default visibility 'visible' which can be switched to visibility 'hidden' manually. PCL_EXPORTS is like a local message to the compiler to change the visibility to 'visible' (what happens for MSVC)

Let me take a look at the functions you pointed out.

EDIT: Regarding that function, maybe simply remove the static keyword conditional on a compile-time variable (say PCL_BINDING_GENERATION or something similar)

[SergioRAgostinho]

- pcl
|- common
|- python
  |- pcl.config
  |- common.config
  |- CMakeLists.txt
  |- binding_generator.py (or binding_generator.sh)

Then make python_bindings would create bindings from a whitelist, similar to how make format works. At least that's how I feel. Others might be able to provide more feedback on this topic.

Just make python for me.

Have you thought how to generate bindings for different python and PCL versions? Eg: PCL 1.10, 1.9 as well as python 3.6 and python 3.8? They have different ABI and will need different bindings if I'm not wrong.

I would not care about this.

By default GCC compiles with default visibility 'visible' which can be switched to visibility 'hidden' manually. PCL_EXPORTS is like a local message to the compiler to change the visibility to 'visible' (what happens for MSVC)

I fiddled up with this before in https://github.com/PointCloudLibrary/pcl/issues/1913 and it was a messy problem back then.

[shiveshkhaitan]

Where are they on the API boundaries? In the Point Types?

Yes it is in Point Types

[shiveshkhaitan]

Update

I have made bindings a PCL module named 'python' in my pcl fork which can be built using CMake. To build the bindings, the command is make pcl_python (I appended 'pcl_' as it is also the prefix for all other targets). The binding gets built under pcl/build/lib. The code generation takes place during cmake . and the generated code is compiled during make.

Any idea how extensive would the changes need to be to achieve from pcl import common?

Seems that it is not that easy. I could not figure out a way without removing pcl namespace from PCL codebase. The current version is from pcl_python import pcl. I will look into it again and try to find a way out.

[kunaltyagi]

The current version is from pcl_python import pcl

If I understand this correctly, pcl_python is the name given by you and pcl is the namespace.

Can we mix python bindings with a python module layout as follows:

pcl/common/__init__.py: from pcl_python_common import pcl
pcl/two_d/__init__.py: from pcl_python_2d import pcl

[shiveshkhaitan]

If I understand this correctly, pcl_python is the name given by you and pcl is the namespace.

Yes

Can we mix python bindings with a python module layout as follows:

Yes we can do this. For every module, binder will be run once. But even with this layout if we do import common, we will have to do common.pcl.common.rad2deg. Instead if we have pcl/__init__.py as:

from pcl_python import pcl
common = pcl.common
2d = pcl.2d
...

Then we can have import pcl; pcl.common.rad2deg.

[shiveshkhaitan]

Update

I have integrated the clang plugin to extract required functions in Binder which removes the need to maintain a headers and config file. Now, the python module contains just a CMakeLists.txt. Just make pcl_python would be creating the bindings.

Further Steps

• Fix for anonymous structs
• Matlab or C binding
• Check for performance of Binder (Currently it takes quite some time for generating the binding code)
• Continue with bindings for other modules
• Tests for common module

@SergioRAgostinho @kunaltyagi any suggestions on what should I take up next