search

norlab-ulaval / libpointmatcher

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

Implemented a class to generate point clouds of 3D geometric primitives #508

Closed YoshuaNava closed 1 month ago

YoshuaNava commented 1 year ago

Description

This PR introduces the PointCloudGenerator, a class for procedural generation of point clouds corresponding to 3D geometric primitives (sphere, cylinder, plane, box). My motivation for introducing it is to have synthetic data with perfect normals that we can use to test ICP and filters.

I implemented unit tests covering 100% of the new code.

Open questions

  1. I was unsure about where to put this class. For now I added it into PointMatcher.h, but I think if we keep most declarations there that file will grow in size quickly.
  2. At the moment the class only implements 3D primitives, although libpointmatcher can also work in 2D. Should we handle this case?
  3. Is the API at a good level? What would you improve?
pomerlef commented 1 year ago

Add to whitelist

pomerlef commented 1 year ago

This looks good as it is. We are indeed generating these kinds of shapes often in our lab as toy examples to test new functionalities. Maybe @simonpierredeschenes and @boxanm want to comment.

A few improvements:

Since it is fresh in your mind, you might want to address them right away or we can transfer them to an issue.

simonpierredeschenes commented 1 year ago

Hi @YoshuaNava, thank you for this contribution! I don't think you need to add 2D shapes for now, this can be added later if people need it. As for the API, I think that with François' comments, it will be flexible enough for different use cases. It is true that this could be added to a different header file, something like PointMatcherTools.h, since it is not related to registration directly. If you have time, this would be greatly appreciated. Thank you!

YoshuaNava commented 1 year ago

This looks good as it is. We are indeed generating these kinds of shapes often in our lab as toy examples to test new functionalities. Maybe @simonpierredeschenes and @boxanm want to comment.

A few improvements:

  • add the generation of open-ended cylinders (one translation and one rotation unobservable)
  • add the generation of open-ended rectangular prisms (one translation unobservable)

This sounds great :+1:

I would have an API-design question: How would you like me to encode the property of which faces the shape has?

  • add scaling parameters on three axes (applied before rotations)

Separate scaling on X, Y and Z?

  • add thickness noise on surfaces

As some shapes use closed-form Polar/Cartesian expressions, I need to identify where to add the noise. I'll give the idea a spin and get back to you.

  • add documentation reusing your unit test to show the generated shapes

Separate documentation under doc, right?

Hi @YoshuaNava, thank you for this contribution! I don't think you need to add 2D shapes for now, this can be added later if people need it.

I can add assertions to prevent that users call the generator when using 2D-pointmatcher (e.g. a user calls generateSphere, thinking he/she would get a 2-sphere)

It is true that this could be added to a different header file, something like PointMatcherTools.h, since it is not related to registration directly.

Will do!

Since it is fresh in your mind, you might want to address them right away or we can transfer them to an issue.

A priori I think the aspect that could take me the most to address is the generation of open shapes, just because it needs some deeper thought to create a good API. But I'll give it a try before we move it to an issue.

YoshuaNava commented 1 year ago

@simonpierredeschenes @pomerlef Kind ping to get your view on https://github.com/ethz-asl/libpointmatcher/pull/508#issuecomment-1476013854 :slightly_smiling_face:

YoshuaNava commented 1 year ago

Hi @simonpierredeschenes @pomerlef,

I recently worked on the CoM conditioning topic, and recently pushed a fix with unit tests.

If possible I would like to get your feedback and check whether we can go forward with this PR, so that I can continue pushing the changes I did before I have to switch to work on a different project.

Note: before merging I need to push one more commit with a small fix to the orientation of normals for the Box shape.

Best, Yoshua

pomerlef commented 2 months ago

@boxanm , can you have a look?

boxanm commented 2 months ago

Looks like our build server complains about compilation on Ubuntu Jammy:

                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h: In instantiation of ‘void testing::internal::DefaultPrintTo(testing::internal::IsContainer, testing::internal::false_type, const C&, std::ostream*) [with C = Eigen::Matrix<double, 3, 3>; testing::internal::IsContainer = int; testing::internal::false_type = testing::internal::bool_constant<false>; std::ostream = std::basic_ostream<char>]’:
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:9567:17:   required from ‘void testing::internal::PrintTo(const T&, std::ostream*) [with T = Eigen::Matrix<double, 3, 3>; std::ostream = std::basic_ostream<char>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:9791:12:   required from ‘static void testing::internal::UniversalPrinter<T>::Print(const T&, std::ostream*) [with T = Eigen::Matrix<double, 3, 3>; std::ostream = std::basic_ostream<char>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:9947:30:   required from ‘void testing::internal::UniversalPrint(const T&, std::ostream*) [with T = Eigen::Matrix<double, 3, 3>; std::ostream = std::basic_ostream<char>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:9875:19:   required from ‘static void testing::internal::UniversalTersePrinter<T>::Print(const T&, std::ostream*) [with T = Eigen::Matrix<double, 3, 3>; std::ostream = std::basic_ostream<char>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:10040:44:   required from ‘std::string testing::PrintToString(const T&) [with T = Eigen::Matrix<double, 3, 3>; std::string = std::__cxx11::basic_string<char>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:18771:36:   required from ‘static std::string testing::internal::FormatForComparison<ToPrint, OtherOperand>::Format(const ToPrint&) [with ToPrint = Eigen::Matrix<double, 3, 3>; OtherOperand = Eigen::Block<const Eigen::Matrix<double, 4, 4>, 3, 3, false>; std::string = std::__cxx11::basic_string<char>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:18846:45:   required from ‘std::string testing::internal::FormatForComparisonFailureMessage(const T1&, const T2&) [with T1 = Eigen::Matrix<double, 3, 3>; T2 = Eigen::Block<const Eigen::Matrix<double, 4, 4>, 3, 3, false>; std::string = std::__cxx11::basic_string<char>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:18872:53:   required from ‘testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = Eigen::Block<const Eigen::Matrix<double, 4, 4>, 3, 3, false>; T2 = Eigen::Matrix<double, 3, 3>]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:18897:23:   required from ‘static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = Eigen::Block<const Eigen::Matrix<double, 4, 4>, 3, 3, false>; T2 = Eigen::Matrix<double, 3, 3>; bool lhs_is_null_literal = false]’
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/PointCloudGenerator.cpp:39:5:   required from here
17:55:30                   #32 114.7 /opt/percep3d_libraries/libpointmatcher/utest/ui/../../contrib/gtest/gtest.h:9466:35: error: variable or field ‘it’ declared void
17:55:30                   #32 115.4 make[2]: *** [utest/CMakeFiles/utest.dir/build.make:174: utest/CMakeFiles/utest.dir/ui/PointCloudGenerator.cpp.o] Error 1
17:55:30                   #32 115.4 make[2]: *** Waiting for unfinished jobs....
17:55:36                   #32 120.6 make[1]: *** [CMakeFiles/Makefile2:456: utest/CMakeFiles/utest.dir/all] Error 2

I'll try to build it locally and see what causes the issue. In the meantime, thanks for the contribution! I know it has been sitting here for a while, we've been waiting to stabilize our build infrastructure. I have some general comments before I test the code locally:

  1. Since this is a big enhancement of libpointmatcher's capabilities, I think it should be accompanied by a page in the documentation and an example file.
  2. We'll need to add Python bindings for all methods in pointmatcher/PointCloudGenerator.cpp.

Overall, I find such geometric primitives useful for testing. However, the same functionality can be achieved in Python with way fewer lines of code. So, without questioning the usefulness of this particular contribution, I thought about the general idea of the cases that libpointmatcher should cover. We only have limited resources to maintain the library, and as every additional function makes it more difficult, I don't think our goal is to become second point_cloud_library.

YoshuaNava commented 2 months ago

Overall, I find such geometric primitives useful for testing. However, the same functionality can be achieved in Python with way fewer lines of code. So, without questioning the usefulness of this particular contribution, I thought about the general idea of the cases that libpointmatcher should cover.

Indeed, the main motivation for this class was to have a way to test other classes. For example, we implemented an extensive test for ICP conditioning, that was enabled by the box point cloud generator: https://github.com/ANYbotics/libpointmatcher/blob/master/utest/ui/icp/Conditioning.cpp

It would be easy to take these primitives and test filters, as well as other operators, with the guarantee that you have total control over the data / a ground truth.

In my experience, relying on other languages for this task can block unit tests. As I understand it, one would need to implement C++ bindings for Python classes, which may run with overhead.

I don't think our goal is to become second point_cloud_library.

Personally I wouldn't see it as a bad thing if libpointmatcher grows. The foundations of the library are extremely solid, yet lightweight. They have passed the test of time, and they have a key differentiating factor: the representation of point clouds as a struct of arrays (a Matrix), which was quite a nice idea (relatively similar to sparse tensors) back in the days :rocket:

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud