Correspondence estimation with different point types #4901

al-tu · 2021-08-17T07:24:44Z

isSamePointType function

is constexpr now
uses type traits instead of typeid comparison
correspondence estimation functions forward point selection to helper template functions specializations instead of if blocks with runtime type checks

al-tu · 2021-08-17T07:25:51Z

common/include/pcl/common/io.h

@@ -319,10 +319,10 @@ namespace pcl
                  pcl::PCLPointCloud2 &cloud_out);

  /** \brief Check if two given point types are the same or not. */
-  template <typename Point1T, typename Point2T> inline bool
+  template <typename Point1T, typename Point2T> inline constexpr bool


should this function live in common/include/pcl/type_traits.h perhaps?

We can simplify this a lot more (and deprecate it) since this is just a function call instead of std::is_same_v<A, B>. Thoughts: @mvieth ?

I am not necessarily against deprecating isSamePointType, but I don't really see a problem with people using pcl::isSamePointType<A, B>() instead of std::is_same<A, B>::value (or is_same_v starting with C++17).
How would you simplify it?

Alright. If we don't deprecate it, should we move it to type_traits.h or some other file than io.h?

Apart from this, LGTM

The problem I see with moving the function to a different header file is that if someone uses the function and includes the current header file (io.h), they would get an error that the function can not be found (unless they also include the new header file by chance), without any prior warning that the function will move or similar. I agree that, if isSamePointType was new and we had to decide where to put it, type_traits.h seems more fitting than io.h, but I honestly don't think it is worth to move this existing function

registration/include/pcl/registration/impl/correspondence_estimation.hpp

kunaltyagi · 2021-08-20T19:48:58Z

test/common/test_common.cpp

+  constexpr const bool status0 = isSamePointType<PointXYZ, PointXYZ> ();
+  EXPECT_TRUE (status0);


Thoughts on combining into a single line?

done
also, only two of these tests seem like relevant, should i remove duplication, eg there's no point to test
EXPECT_TRUE (isSamePointType<PointNormal, PointNormal> ());
if there's
EXPECT_TRUE (isSamePointType<PointXYZ, PointXYZ> ());
what do you think?

TBH I would rather keep those tests. Yes, knowing the implementation of isSamePointType, it is clear that they will pass, but the tests should not make any assumptions about the implementation. If, for some reason, isSamePointType is changed in the future, we would want to test it with all these tests. Plus they cost practically nothing (compilation time/...).

conversely, someone reading those tests would (mistakenly) assume that there is some difference in comparison logic for PointXYZ and PointNormal types (because we have two tests and you usually don't pay for what you don't need, especially in library code). and checking out the implementation of isSamePointType would cause even more confusion, because std::is_same works the same in both cases, of course.
so, if there's some need in description forisSamePointType logic, i suggest we leave it as a comment, just quoting cppreference.com:

If T and U name the same type (taking into account const/volatile qualifications), provides the member constant value equal to true. Otherwise value is false. Commutativity is satisfied, i.e. for any two types T and U, is_same<T, U>::value == true if and only if is_same<U, T>::value == true.

if for some reason it does not seem sufficient, i will revert removal of those extra tests, of course

hi! should i revert my changes in tests?

Sorry for the delay.

someone reading those tests would (mistakenly) assume that there is some difference in comparison logic for PointXYZ and PointNormal types

I don't really agree with that thought. I think it is normal to test a function with many different parameters, also template parameters, without any implication that the function works differently for some parameters. I think the EXPECT_TRUE and EXPECT_FALSE also make clear what the function should do in each case. If you want to add a clarifying comment somewhere, that is of course always a good idea. I just think removing already existing tests is counter-intuitive, considering that too much code in PCL is untested.

decreasing code coverage is certainly not a good idea, that's right, so i did roll back my changes. hopefully this PR is an overall improvement and may be merged without changes in tests

there's also an opened question whether isSamePointType should live in io.h

kunaltyagi · 2021-08-20T19:51:33Z

common/include/pcl/common/io.h

@@ -319,10 +319,10 @@ namespace pcl
                  pcl::PCLPointCloud2 &cloud_out);

  /** \brief Check if two given point types are the same or not. */
-  template <typename Point1T, typename Point2T> inline bool
+  template <typename Point1T, typename Point2T> inline constexpr bool


We can simplify this a lot more (and deprecate it) since this is just a function call instead of std::is_same_v<A, B>. Thoughts: @mvieth ?

registration/include/pcl/registration/impl/correspondence_estimation_backprojection.hpp

registration/include/pcl/registration/impl/correspondence_estimation.hpp

kunaltyagi

If CI is 🟢

kunaltyagi · 2021-08-24T12:42:09Z

common/include/pcl/common/io.h

@@ -319,10 +319,10 @@ namespace pcl
                  pcl::PCLPointCloud2 &cloud_out);

  /** \brief Check if two given point types are the same or not. */
-  template <typename Point1T, typename Point2T> inline bool
+  template <typename Point1T, typename Point2T> inline constexpr bool


Apart from this, LGTM

mvieth · 2021-10-13T13:30:00Z

registration/include/pcl/registration/impl/correspondence_estimation_normal_shooting.hpp

+    // Check if the correspondence is reciprocal
+    target_idx = nn_indices[min_index];
+    tree_reciprocal_->nearestKSearch(
+        (*target_)[target_idx], 1, index_reciprocal, distance_reciprocal);


Maybe I am missing something here: target_ contains points of type PointTarget, but tree_reciprocal_ uses points of type PointSource, so this should not work, right? I see that you did not change this line, so is this an error that existed before but was not discovered? In correspondence_estimation_backprojection.hpp, this is the same, but in correspondence_estimation.hpp, the point is copied (copyPoint before, pointCopyOrRef with your changes). Do you have an idea?
Apart from this, looks good to me!

suppose that you're right, it seems like only the cases when point types are the same were covered, i've added another one for completeness (though ), so it should be correct now, but that required some additional changes
PTAL, i might have messed up that source-target logic (hope not)

mvieth · 2021-12-23T10:41:53Z

@al-tu The new test(s) don't seem to compile yet

al-tu · 2022-03-13T07:56:03Z

@kunaltyagi @mvieth hello! do you have any idea why one particular test on clang/macos catalina fails? may this be a flaky situation? it would be nice to be able to restart ci via bot command

mvieth · 2022-03-13T09:22:36Z

@kunaltyagi @mvieth hello! do you have any idea why one particular test on clang/macos catalina fails? may this be a flaky situation? it would be nice to be able to restart ci via bot command

I restarted the CI. Unfortunately, there are a few tests that fail randomly (but rarely)

al-tu · 2022-03-13T12:07:49Z

@kunaltyagi @mvieth hello! do you have any idea why one particular test on clang/macos catalina fails? may this be a flaky situation? it would be nice to be able to restart ci via bot command

I restarted the CI. Unfortunately, there are a few tests that fail randomly (but rarely)

thanks a lot! passes now, so yes, that was a random fail

al-tu · 2022-06-07T04:46:12Z

hello. it's actually ready now, could you please re-review? thanks!

larshg · 2022-10-20T19:15:15Z

@al-tu Do you have time to rebase this?

test/registration/test_correspondence_estimation.cpp

test/common/test_io.cpp

more tests for correspondence estimation to cover cases when source and target pointclouds have different point types

al-tu · 2022-10-21T20:34:13Z

@al-tu Do you have time to rebase this?

sure, done

mvieth

Thank you, and sorry that we took so long with the reviews

al-tu commented Aug 17, 2021

View reviewed changes

al-tu mentioned this pull request Aug 17, 2021

memset for Eigen::Matrix in integral_image2D.cpp #3368

Open

al-tu changed the title ~~no more typeid its constexpr~~ no more typeid, it's constexpr Aug 17, 2021

al-tu mentioned this pull request Aug 17, 2021

correspondence estimation for different point types #4902

Draft

kunaltyagi reviewed Aug 20, 2021

View reviewed changes

kunaltyagi reviewed Aug 24, 2021

View reviewed changes

registration/include/pcl/registration/impl/correspondence_estimation_backprojection.hpp Outdated Show resolved Hide resolved

registration/include/pcl/registration/impl/correspondence_estimation.hpp Outdated Show resolved Hide resolved

kunaltyagi previously approved these changes Aug 24, 2021

View reviewed changes

al-tu dismissed kunaltyagi’s stale review via fa3b2ca October 12, 2021 05:47

mvieth reviewed Oct 13, 2021

View reviewed changes

al-tu force-pushed the no-more-typeid-its-constexpr branch 3 times, most recently from 061ccb3 to a206b9a Compare November 18, 2021 07:07

al-tu force-pushed the no-more-typeid-its-constexpr branch 14 times, most recently from c53fdec to 7e7e479 Compare March 11, 2022 07:01

larshg added this to the pcl-1.13.0 milestone Oct 18, 2022

larshg reviewed Oct 20, 2022

View reviewed changes

test/registration/test_correspondence_estimation.cpp Outdated Show resolved Hide resolved

larshg reviewed Oct 20, 2022

View reviewed changes

test/registration/test_correspondence_estimation.cpp Outdated Show resolved Hide resolved

larshg reviewed Oct 20, 2022

View reviewed changes

test/common/test_io.cpp Show resolved Hide resolved

isSamePointType function made constexpr

75bd2d1

more tests for correspondence estimation to cover cases when source and target pointclouds have different point types

al-tu force-pushed the no-more-typeid-its-constexpr branch from 7e7e479 to 75bd2d1 Compare October 21, 2022 20:34

mvieth modified the milestones: pcl-1.13.0, pcl-1.13.1 Nov 19, 2022

Merge branch 'master' into no-more-typeid-its-constexpr

7bc1e23

mvieth modified the milestones: pcl-1.13.1, pcl-1.14.0 Mar 6, 2023

mvieth mentioned this pull request May 28, 2023

[registration] Make ICP work with TransformationEstimationPointToPlane when only target point type has normals #5727

Closed

mvieth requested a review from larshg June 9, 2023 19:28

mvieth approved these changes Jun 9, 2023

View reviewed changes

larshg approved these changes Jun 10, 2023

View reviewed changes

mvieth merged commit 956f89c into PointCloudLibrary:master Jun 11, 2023

This was referenced Jun 14, 2023

CorrespondenceEstimation with different point types #329

Open

Support for correspondence estimation with different point types #1476

Open

al-tu deleted the no-more-typeid-its-constexpr branch June 16, 2023 22:19

mvieth added the module: registration label Dec 21, 2023

mvieth changed the title ~~no more typeid, it's constexpr~~ Correspondence estimation with different point types Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correspondence estimation with different point types #4901

Correspondence estimation with different point types #4901

al-tu commented Aug 17, 2021

al-tu Aug 17, 2021

kunaltyagi Aug 20, 2021

mvieth Aug 22, 2021

kunaltyagi Aug 23, 2021

kunaltyagi Aug 24, 2021

mvieth Oct 13, 2021

kunaltyagi Aug 20, 2021

al-tu Aug 23, 2021

mvieth Aug 27, 2021

al-tu Aug 27, 2021

al-tu Sep 28, 2021

mvieth Sep 28, 2021

al-tu Oct 12, 2021

al-tu Oct 12, 2021

kunaltyagi Aug 20, 2021

kunaltyagi left a comment

kunaltyagi Aug 24, 2021

mvieth Oct 13, 2021

al-tu Mar 13, 2022

mvieth commented Dec 23, 2021

al-tu commented Mar 13, 2022

mvieth commented Mar 13, 2022

al-tu commented Mar 13, 2022

al-tu commented Jun 7, 2022

larshg commented Oct 20, 2022

al-tu commented Oct 21, 2022

mvieth left a comment

		constexpr const bool status0 = isSamePointType<PointXYZ, PointXYZ> ();
		EXPECT_TRUE (status0);

Correspondence estimation with different point types #4901

Correspondence estimation with different point types #4901

Conversation

al-tu commented Aug 17, 2021

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

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

kunaltyagi left a 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

mvieth commented Dec 23, 2021

al-tu commented Mar 13, 2022

mvieth commented Mar 13, 2022

al-tu commented Mar 13, 2022

al-tu commented Jun 7, 2022

larshg commented Oct 20, 2022

al-tu commented Oct 21, 2022

mvieth left a comment

Choose a reason for hiding this comment