Support for correspondence estimation with different point types #1476
Conversation
|
LGTM. Can't comment on the point type use cases, never used this class in my projects. I think testing with XYZ points is enough, this verifies that the algorithm is correct. |
|
Seems to work with |
|
Gentle reminder. This is good for review and merge. |
|
My colleague (Thomas, actually) has recently enquired about the status of this PR. Has anything changed from your side in the last... two years? Maybe rebase, check CI results, and merge? |
SergioRAgostinho
added
needs: author reply
|
I'll have a new look at it later today with fresh eyes. |
- Added Specialization helper to prevent unnecessary copies when invoking nearestKSearch - Added unit tests
SergioRAgostinho
force-pushed
the
corr_est_dif_pointT
branch
from
372a79e to
1bc5285
Compare
|
This is all I managed to do before the weekend. I circumvented the unnecessary copy discussed in #329 with the use of a helper specialized struct. I verified that it performs no copy when the point type is the same and it invokes copyPoint (through nearestKSearchT) in the opposite case. That helper struct might require some reorganization though. I can't enclose it inside the CorrespondenceEstimation, because I lose the ability to specialize it. Nevertheless, the proof of concept is here. Now we need to decide how to tidy up things. |
SergioRAgostinho
added
needs: code review
|
I wonder why don't we apply this same specialization trick directly to |
|
No doubt that it would make more sense. On C++14 migration it will require a cleanup. Anything else you see out of place? |
Don't understand. Do you mean we can not implement it now?
No, everything else looks good. |
|
Yes I can. Only mid next week though. I'm restricted to a tablet for the duration of the weekend. I noticed I need to improve the doxygen documentation and perhaps try some obscure cases under which only the normals are the common fields. |
|
Ah, OK! |
taketwo
added
needs: more work
|
Just had a quick look at things and applying this change will require to modify |
|
I don't know how comfortable you are with template stuff, so here is a quick snippet: template <typename PointTDiff, typename boost::enable_if<boost::is_same<PointT, PointTDiff>, int>::type = 0> inline intThis is the template line for the version where point types are the same. For the opposite case just replace "enable" with "disable". |
|
Included your comment regarding the enable_if, disable_if. I also realized why the tests failed (one year ago) for anything other than point XYZ types. The KdTree's derived from Search are all for "simple" XYZ queries. For ND queries, one needs to use the KdTree from the kdtree module. Correspondence estimation uses the former. Edit: Here's a dump with some extra verbose from the added unit test |
|
Sorry, I should have been more explicit. There is no need for helper classes. All we need is to replace template <typename PointTDiff> inline int
nearestKSearchT (const PointTDiff &point, int k,
std::vector<int> &k_indices, std::vector<float> &k_sqr_distances) const
{
PointT p;
copyPoint (point, p);
return (nearestKSearch (p, k, k_indices, k_sqr_distances));
}with template <typename PointTDiff, typename boost::enable_if<boost::is_same<PointT, PointTDiff>, int>::type = 0> inline int
nearestKSearchT (const PointTDiff &point, int k,
std::vector<int> &k_indices, std::vector<float> &k_sqr_distances) const
{
return (nearestKSearch (point, k, k_indices, k_sqr_distances));
}
template <typename PointTDiff, typename boost::disable_if<boost::is_same<PointT, PointTDiff>, int>::type = 0> inline int
nearestKSearchT (const PointTDiff &point, int k,
std::vector<int> &k_indices, std::vector<float> &k_sqr_distances) const
{
PointT p;
copyPoint (point, p);
return (nearestKSearch (p, k, k_indices, k_sqr_distances));
} |
|
You can't specialize class methods without specializing the entire class. |
|
This rule does not apply here, because we are specializing not on the class template parameter, but on a method's own parameter. In other words, you can not specialize |
|
I'm trying to verify the what you said in cpp.sh as first step. Here's my snippet so far http://cpp.sh/2x7s4 did I miss the point behind what you described? My class is not templated on anything, just the methods. Another thing which stands out in what you wrote is this |
|
Ok... there are some additional nuances to what boost::enable_if does. I'll just go and try what you proposed. |
|
Hm, perhaps I'm wrong with my last statement. However, the thing still works, because we are actually not specializing anything! This part which you are wondering about |
|
So for the compiler only single unspecialized version of template<PointDiffT, int = 0> inline int nearestKSearchT()is visible, so everything is fine. |
|
Your approach is being killed by C++03 :/ If I remove that default setting to Still, this was definitely one of my meta lessons for this year. 👍 |
|
Unless you want to start invoking explicitly |
|
Damn, true, this requires C++11 😞 |
|
Just replace the template line with: template <typename T> inline typename boost::enable_if<boost::is_same<PointT, T>, int>::typeand |
|
Quick question, just in case you know it typename boost::enable_if<boost::is_same<PointT, T>, int>::typeexpands to Some of the methods are of void type. Should I modify to typename boost::enable_if<boost::is_same<PointT, T>, void>::type? |
Nothing. It's called "substitution failure" and the function is just silently discarded by the compiler. That's why the technique is called SFINAE (substitution failure is not an error).
Yes, just put |
|
👍 works. I'll clean it up. |
|
I'll squash my personal commits after review and CI validation. |
SergioRAgostinho
added
needs: code review
|
This pull request has been automatically marked as stale because it hasn't had Come back whenever you have time. We look forward to your contribution. |
|
This one has been superseded by #4901 ? |
Fixes #329. Credits for @aichim, @jpapon and @taketwo .
Added unit test for XYZ point types.
Still have some doubts:
pcl::registration::CorrespondenceEstimationseems to support with all point types. Is it supposed to work with more weird cases of source and target point types, e.g. between XYZ points types <-> Normal point types? Not sure if this is possible.