norlab-ulaval / libpointmatcher

An Iterative Closest Point (ICP) library for 2D and 3D mapping in Robotics
BSD 3-Clause "New" or "Revised" License
1.61k stars 544 forks source link

Embedding libpointmatcher in Unity plugin #412

Open stephanemagnenat opened 3 years ago

stephanemagnenat commented 3 years ago

Dear all,

In the context of a research project on augmented reality for architecture and city planning, we are using Unity XR to generate a point cloud and then align it in background with libpointmatcher.

To do that, we need to embed libpointmatcher in a Unity plugin. @dhuettig has been working on that and has a functioning implementation, put the build process was not fully straightforward.

Thinking about it, I believe that it could be improved as some libpointmatcher dependencies are not necessary for that use case. In particular:

But of course, I know from experience that sometimes in production, especially in research and industrial areas, the toolchains can be ancient, so before investing time to such an endeavour, I would like to have a feedback from the community of developers/users. So my questions are:

pomerlef commented 3 years ago

@AlexandreG87 and @simonpierredeschenes I know that you started removing dependency to Boost, any concerns or advices?

@peci1 any concerns for the DARPA infrastructure for C++17? Same question for @YoshuaNava.

aguenette commented 3 years ago

@pomerlef As far as I'm concerned, I've only replaced the BOOST_AUTO macros for the auto specifier from C++11 in the Registrar.h file. And I think that it could safely be done everywhere since libpointmatcher support C++11. For the other dependencies, I'm not familiar enough with boost to suggest a C++ version that would be the best to replace most of the them.

PhiBabin commented 3 years ago

I checked last year what needed to be done to remove our dependency on boost. Since you will not be using filesystem. It should be easy to do: https://github.com/ethz-asl/libpointmatcher/issues/306

peci1 commented 3 years ago

C++17 is okay ;)

YoshuaNava commented 3 years ago

Hi all, Thank you for tagging me.

I also think C++17 is quite good, and I see many advantages in it. For example: aligned allocators which could potentially speed-up our computations with minimal effort. Or parallel algorithms, optionals.

However, C++17 is not supported by default by ROS Noetic (released this year, last version until EOL of ROS1), neither by ROS2 Foxy Fitzroy link. Some developers from the codebase of libpointmatcher might be using ROS 1/2, and moving directly to C++17 could introduce some ABI compatibilities.

I would propose going first for C++14, or if we go for C++17, introduce a symbol so the library can still be compiled with C++14.

peci1 commented 3 years ago

It is not true that Noetic doesn't support C++17. It just doesn't allow any C++17-only feature to be present in the .h files (the API of the codebase). https://www.ros.org/reps/rep-0003.html#melodic

However, these restrictions are mostly for the "core" ROS packages. It is nice if other packages comply with these rules, too, but it is not a blocker - and the buildfarm definitely doesn't block building and releasing C++17 code. What's IMO important is whether all GCC versions used in the supported OS versions support C++17 (or at least the used features of it).

The oldest supported OS for Melodic is Debian Stretch with GCC 6.3. According to https://gcc.gnu.org/projects/cxx-status.html#cxx17, GCC supports about a half of C++17 features in 6.x, and almost all in 7.x. So using C++17 in PM would mean dropping support for compiling on Stretch (maybe, depending on the features used).

There is a workaround for many of the missing features, e.g. for std::optional. It autoselects either std::optional or std::experimental::optional based on the available compiler and language level: https://github.com/peci1/robot_body_filter/blob/95d8da4475283845066a7cbfcbe71b8b069009be/CMakeLists.txt#L4-L12 .