Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSP_3: Fix pointmatcher support #5350

Merged
merged 5 commits into from
Feb 18, 2021
Merged

PSP_3: Fix pointmatcher support #5350

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa22cd9
Change libpointmatcher_support so it won't fail when old boost has be…
maxGimeno Jan 14, 2021
ff9d714
Fix input
maxGimeno Jan 14, 2021
99a31e0
Move the Boost components in the support file and condition it to win…
maxGimeno Jan 15, 2021
631aa06
Rephrase the doc
maxGimeno Jan 25, 2021
4ed3b01
make the dependencies to boost general, not just for windows
maxGimeno Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Documentation/doc/Documentation/Third_party.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@ executables should be linked with the CMake imported target

The \sc{libpointmatcher} web site is <A
HREF="https://github.com/ethz-asl/libpointmatcher">`https://github.com/ethz-asl/libpointmatcher`</A>.
\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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



\subsection thirdpartyLeda LEDA
<b>Version 6.2 or later</b>
Expand Down
22 changes: 17 additions & 5 deletions Installation/cmake/modules/CGAL_pointmatcher_support.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,19 @@
if(libpointmatcher_FOUND AND NOT TARGET CGAL::pointmatcher_support)
add_library(CGAL::pointmatcher_support INTERFACE IMPORTED)
set_target_properties(CGAL::pointmatcher_support PROPERTIES
INTERFACE_COMPILE_DEFINITIONS "CGAL_LINKED_WITH_POINTMATCHER"
INTERFACE_INCLUDE_DIRECTORIES "${libpointmatcher_INCLUDE_DIR}"
INTERFACE_LINK_LIBRARIES "${libpointmatcher_LIBRARIES}")
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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

endif()
4 changes: 2 additions & 2 deletions Point_set_processing_3/examples/Point_set_processing_3/registration_with_OpenGR.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ int main(int argc, const char** argv)
const char* fname2 = (argc>2)?argv[2]:"data/hippo2.ply";

std::vector<Pwn> pwns1, pwns2;
std::ifstream input(fname1);
std::ifstream input(fname1, std::ios::binary);
if (!input ||
!CGAL::read_ply_points(input, std::back_inserter(pwns1),
CGAL::parameters::point_map (CGAL::First_of_pair_property_map<Pwn>()).
Expand All @@ -36,7 +36,7 @@ int main(int argc, const char** argv)
}
input.close();

input.open(fname2);
input.open(fname2, std::ios::binary);
if (!input ||
!CGAL::read_ply_points(input, std::back_inserter(pwns2),
CGAL::parameters::point_map (Point_map()).
Expand Down
4 changes: 2 additions & 2 deletions ...sing_3/examples/Point_set_processing_3/registration_with_opengr_pointmatcher_pipeline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ int main(int argc, const char** argv)
const char* fname2 = (argc>2)?argv[2]:"data/hippo2.ply";

std::vector<Pwn> pwns1, pwns2;
std::ifstream input(fname1);
std::ifstream input(fname1, std::ios::binary);
if (!input ||
!CGAL::read_ply_points(input, std::back_inserter(pwns1),
CGAL::parameters::point_map (CGAL::First_of_pair_property_map<Pwn>()).
Expand All @@ -39,7 +39,7 @@ int main(int argc, const char** argv)
}
input.close();

input.open(fname2);
input.open(fname2, std::ios::binary);
if (!input ||
!CGAL::read_ply_points(input, std::back_inserter(pwns2),
CGAL::parameters::point_map (Point_map()).
Expand Down
4 changes: 2 additions & 2 deletions Point_set_processing_3/examples/Point_set_processing_3/registration_with_pointmatcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ int main(int argc, const char** argv)
const char* fname2 = (argc>2)?argv[2]:"data/hippo2.ply";

std::vector<Pwn> pwns1, pwns2;
std::ifstream input(fname1);
std::ifstream input(fname1, std::ios::binary);
if (!input ||
!CGAL::read_ply_points(input, std::back_inserter(pwns1),
CGAL::parameters::point_map (CGAL::First_of_pair_property_map<Pwn>()).
Expand All @@ -39,7 +39,7 @@ int main(int argc, const char** argv)
}
input.close();

input.open(fname2);
input.open(fname2, std::ios::binary);
if (!input ||
!CGAL::read_ply_points(input, std::back_inserter(pwns2),
CGAL::parameters::point_map (Point_map()).
Expand Down