PSP_3: Fix pointmatcher support #5350

maxGimeno · 2021-01-14T13:04:07Z

Summary of Changes

Fixes to OpenGR and pointmatcher examples.

Release Management

Affected package(s):Point_set_processing_3_Examples

…en used to build pointmatcher.

lrineau · 2021-01-14T13:08:26Z

Point_set_processing_3/examples/Point_set_processing_3/CMakeLists.txt

@@ -93,6 +93,7 @@ if ( CGAL_FOUND )
    find_package(libpointmatcher QUIET)
    include(CGAL_pointmatcher_support)
    if (TARGET CGAL::pointmatcher_support)
+      find_package(Boost COMPONENTS thread filesystem system program_options date_time chrono REQUIRED)


What is that find_package(Boost ...) for? It does not seem used in the following lines.

libpointmatcher forwards all its boost dependencies, and those are the said dependencies. Without this find_package, cmake generation fails with

CMake Error at CMakeLists.txt:117 (add_executable): Target "registration_with_opengr_pointmatcher_pipeline" links to target "Boost::thread" but the target was not found. Perhaps a find_package() call is missing for an IMPORTED target, or an ALIAS target is missing?

and more.

I suggest to put that line in CGAL_pointmatcher_support.cmake.

Actually, it should be also upstreamed in libpointmatcherConfig.cmake.

…dows.

lrineau · 2021-01-18T10:07:07Z

Documentation/doc/Documentation/Third_party.txt

+\attention On Windows, we only test the following setup : PointMatcher 1.3.1 with Eigen 3.3.7. Also, to make it work, you should follow the instructions at
+`https://github.com/ethz-asl/libpointmatcher/blob/master/doc/CompilationWindows.md`, but replace `NABO_INCLUDE_DIR` by `libnabo_INCLUDE_DIRS`
+and `NABO_LIBRARY` by `libnabo_LIBRARIES` when configuring PointMatcher.


I think that paragraph needs a bit a polish and rewording.

lrineau · 2021-01-18T10:11:47Z

Installation/cmake/modules/CGAL_pointmatcher_support.cmake

+  if(WIN32 OR CMAKE_SYSTEM_NAME STREQUAL Windows)
+    find_package(Boost COMPONENTS thread filesystem system program_options date_time chrono)
+  endif()
+  if(NOT (WIN32 OR CMAKE_SYSTEM_NAME STREQUAL Windows)
+      OR ( Boost_chrono_FOUND
+      AND Boost_thread_FOUND
+      AND Boost_filesystem_FOUND
+      AND Boost_system_FOUND
+      AND Boost_program_options_FOUND
+      AND Boost_date_time_FOUND) )
+    add_library(CGAL::pointmatcher_support INTERFACE IMPORTED)
+    target_compile_definitions(CGAL::pointmatcher_support INTERFACE "CGAL_LINKED_WITH_POINTMATCHER")
+    target_include_directories(CGAL::pointmatcher_support INTERFACE "${libpointmatcher_INCLUDE_DIR}")
+    target_link_libraries(CGAL::pointmatcher_support INTERFACE "${libpointmatcher_LIBRARIES}")
+  else()
+    message(STATUS "NOTICE : the libpointmatcher library requires the following boost components: thread filesystem system program_options date_time chrono.")
+  endif()


I do not know the details... Are those Boost libraries only needed on Windows?

What I do not like is that this code seems fragile: it works with the version of libpointmatcher that was tested, and there is no guarantee that it will still work with the next one.

@sgiraudot @maxGimeno If that library is important for us, I think we should work with the maintainers of libpointmatcher to improve their CMake support. In the end, that code should simply be:

add_library(CGAL::pointmatcher_support INTERFACE IMPORTED) target_compile_definitions(CGAL::pointmatcher_support INTERFACE "CGAL_LINKED_WITH_POINTMATCHER") target_link_libraries(CGAL::pointmatcher_support INTERFACE libpointmatcher::libpointmatcher)

At the current time, either we fix the support in our code, either we completely drop the support of the Windows platforms. I already reported the bugs in Pointmatcher's cmake scripts here

Also, we don't need it to work with the next version, as it is documented that we do not support another version. At least until they fix the bugs we reported.

Also, we don't need it to work with the next version, as it is documented that we do not support another version. At least until they fix the bugs we reported.

I know that is documented that way, but that is very aggressive for our users. Imagine somebody said "we only support CGAL version 4.13.2"... That is why, if that library is important for us (@sgiraudot?) then we should collaborate with libpointmatcher to improve their CMake support. And I think we should contribute pull-requests, and not only issues, against if that is important for us.

It's really "just" a wrapper.

It's important in the sense that it is something that we do provide and thus we should make sure it works, but I'm not sure how much time and effort it makes sense to put into that (especially if they have an almost non-existent Windows support, it might be a long run to make it clean).

It's really "just" a wrapper.

It's important in the sense that it is something that we do provide and thus we should make sure it works, but I'm not sure how much time and effort it makes sense to put into that (especially if they have an almost non-existent Windows support, it might be a long run to make it clean).

The same way we helped Eigen3 with their Windows support, maybe we could help libpointmatcher with their Windows support. I do not know how much effort we can put, though. We should ask Andreas (@afabri).

maxGimeno · 2021-01-26T10:58:19Z

My PR for the min/max problem in OpenGR on windows STORM-IRIT/OpenGR#87

maxGimeno · 2021-01-26T12:31:39Z

My PRs for pointmatcher :

lrineau · 2021-02-02T11:36:04Z

My PRs for pointmatcher :

* [ethz-asl/libpointmatcher#437](https://github.com/ethz-asl/libpointmatcher/pull/437)

* [ethz-asl/libpointmatcher#436](https://github.com/ethz-asl/libpointmatcher/pull/436)

Both are merged now. Congratulation! 🍾

maxGimeno · 2021-02-18T13:45:42Z

Successfully tested in https://cgal.geometryfactory.com/CGAL/testsuite/results-5.3-Ic-65.shtml

lrineau · 2021-02-18T14:26:50Z

Documentation/doc/Documentation/Third_party.txt

+\attention On Windows, we only support version 1.3.1 of PointMatcher with version 3.3.7 of Eigen, with some changes to the recipe at
+`https://github.com/ethz-asl/libpointmatcher/blob/master/doc/CompilationWindows.md`:`NABO_INCLUDE_DIR` becomes `libnabo_INCLUDE_DIRS`
+and `NABO_LIBRARY` becomes `libnabo_LIBRARIES` in the "Build libpointmatcher" section.
+


@sgiraudot Please review.

…maxGimeno PSP_3: Fix pointmatcher support

Change libpointmatcher_support so it won't fail when old boost has be…

fa22cd9

…en used to build pointmatcher.

maxGimeno added the Pkg::Point_set_processing_3 label Jan 14, 2021

maxGimeno added this to the 5.1.3 milestone Jan 14, 2021

maxGimeno self-assigned this Jan 14, 2021

Fix input

ff9d714

lrineau reviewed Jan 14, 2021

View reviewed changes

Move the Boost components in the support file and condition it to win…

99a31e0

…dows.

maxGimeno marked this pull request as ready for review January 15, 2021 13:11

maxGimeno requested a review from lrineau January 15, 2021 13:11

maxGimeno changed the base branch from master to 5.1.x-branch January 15, 2021 13:14

lrineau reviewed Jan 18, 2021

View reviewed changes

maxGimeno mentioned this pull request Jan 21, 2021

Add OpenGR and libpointmatcher on Ubuntu images CGAL/cgal-testsuite-dockerfiles#117

Merged

maxGimeno added 2 commits January 25, 2021 15:28

Rephrase the doc

631aa06

make the dependencies to boost general, not just for windows

4ed3b01

maxGimeno added the Ready to be tested label Jan 25, 2021

maxGimeno added the Under Testing label Feb 4, 2021

maxGimeno added the Tested label Feb 18, 2021

lrineau added rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' rm only: ready for release branch For the release team only: that indicates that a PR is about to be merged in a release branch labels Feb 18, 2021

lrineau reviewed Feb 18, 2021

View reviewed changes

lrineau merged commit 7d760d8 into CGAL:5.1.x-branch Feb 18, 2021

lrineau added a commit that referenced this pull request Feb 18, 2021

Merge pull request #5350 from maxGimeno/PSP_3-Fixes-for_pointmatcher-…

7edf0f2

…maxGimeno PSP_3: Fix pointmatcher support

lrineau removed Ready to be tested Under Testing rm only: ready for release branch For the release team only: that indicates that a PR is about to be merged in a release branch labels Feb 18, 2021

lrineau deleted the PSP_3-Fixes-for_pointmatcher-maxGimeno branch February 18, 2021 14:58

lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Feb 18, 2021

lrineau added Merged_in_5.1.3 Merged_in_5.2.1 labels Mar 17, 2021

lrineau added Merged_in_5.1.4 Merged_in_5.2.2 labels Jun 2, 2021

lrineau added Merged_in_5.3-beta1 Merged_in_5.1.5 Merged_in_5.2.3 Merged_in_5.3 labels Jul 2, 2021

lrineau added the Merged_in_5.2.4 label Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSP_3: Fix pointmatcher support #5350

PSP_3: Fix pointmatcher support #5350

maxGimeno commented Jan 14, 2021

lrineau Jan 14, 2021

maxGimeno Jan 14, 2021

lrineau Jan 14, 2021

lrineau Jan 18, 2021

lrineau Jan 18, 2021

maxGimeno Jan 25, 2021

maxGimeno Jan 25, 2021

lrineau Jan 25, 2021

sgiraudot Jan 25, 2021

lrineau Jan 25, 2021

maxGimeno commented Jan 26, 2021

maxGimeno commented Jan 26, 2021

lrineau commented Feb 2, 2021

maxGimeno commented Feb 18, 2021

lrineau Feb 18, 2021

PSP_3: Fix pointmatcher support #5350

PSP_3: Fix pointmatcher support #5350

Conversation

maxGimeno commented Jan 14, 2021

Summary of Changes

Release Management

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxGimeno commented Jan 26, 2021

maxGimeno commented Jan 26, 2021

lrineau commented Feb 2, 2021

maxGimeno commented Feb 18, 2021

Choose a reason for hiding this comment