Including GRSD a global radius-based surface descriptor #790

desinghkar · 2014-07-08T01:07:11Z

GRSD is developed by Zoltan-Marton, this was a old patch as part of his branch being included into the trunk.

desinghkar · 2014-07-08T08:10:21Z

Can somebody help me in merging this? Travis CI build failed twice and I have no clue of what should be done. Thanks in advance! :)

VictorLamoine · 2014-07-08T08:16:49Z

Sure,

Take a look at travis build (line 972)

In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/grsd.cpp:41:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/grsd.hpp:41:10: fatal error:
'/home/kar/workspace/gsoc_14/pcl_test/grsd/grsd.h' file not found
#include "/home/kar/workspace/gsoc_14/pcl_test/grsd/grsd.h"

You have set a global path to a file; this will only work on your machine with your folder tree.

VictorLamoine · 2014-07-08T08:17:35Z

features/include/pcl/features/impl/grsd.hpp

+#ifndef PCL_FEATURES_IMPL_GRSD_H_
+#define PCL_FEATURES_IMPL_GRSD_H_
+
+#include "/home/kar/workspace/gsoc_14/pcl_test/grsd/grsd.h"


This must be a relative path !

desinghkar · 2014-07-08T08:24:02Z

Oh my bad! Thanks Victor. I solved it and pushed again. Hopefully it should build. :)

taketwo · 2014-07-08T08:37:06Z

features/include/pcl/features/grsd.h

+
+      /** \brief Get the sphere radius used for determining the neighbors. */
+      inline double
+      getRadiusSearch () { return (search_radius_); }


Please add const qualifier.

Oh ok!
I changed it to getRadiusSearch() const { return search_radius_); }

taketwo · 2014-07-08T08:40:49Z

Hi, please check out the contributors guide. We'll need you to fix the code according to the style guide (add spaces between function names and braces) and clean up the commit history.

desinghkar · 2014-07-08T08:54:43Z

Is there a pcl style file for vim?
Any smart way of checking if my code satisfies the code style?

VictorLamoine · 2014-07-08T08:57:55Z

There is the Eclipse style guide :
http://www.pointclouds.org/documentation/tutorials/using_pcl_with_eclipse.php#using-pcl-with-eclipse
http://www.pointclouds.org/documentation/advanced/pcl_style_guide.php#eclipse

As for Vim, I don't think so

desinghkar · 2014-07-08T09:04:30Z

Oh ok I will check if I can convert the emacs style file for Vim. Thanks

desinghkar · 2014-07-08T09:13:51Z

Ok i am checking the checklist mentioned in the contributors guide and have following queries.

This code is developed by Zoltan-marton, I just changed the licence to the one shown in the guide. How to insert his id / or say its his code?
How to clean up the commit history (sorry not so familiar with git)?

taketwo · 2014-07-08T09:25:28Z

Usually it is sufficient to put the name in the \author tag of doxygen comment. (This is done already in your code.) But it would also be nice to check with him if he is okay with giving up the copyright to Open Perception.
Right now there are three commits. The first one is the "meat", and the others are just fixups. And you will add up more fixups with style changes. There is no point of keeping this sort of history in the main repository, so we need to squash them into a single commit. Check out this article.

VictorLamoine · 2014-07-08T09:25:33Z

\author Zoltan Csaba Marton he is explicitly the author in the code; if you have the rights to include the code in PCL and you respect his license I think it's fine.
Each commit should be a logical unit. For example you should merge the last two commits into one. For example:
git rebase -i upstream master
Then replace pick by s to squash a commit

taketwo · 2014-07-08T09:26:45Z

@VictorLamoine :)

desinghkar · 2014-07-08T09:55:34Z

That was really helpful guys! Really appreciate that.
I just have to check with the author about giving up the copyright to Open Perception. For now I have pushed it with the existing copyrights. I will update the same in my future requests. I hope its fine!

VictorLamoine · 2014-07-08T11:45:53Z

Compilation fails: line 1351

desinghkar · 2014-07-08T12:00:46Z

I checked it again locally and it builds just fine. RSD Estimation is enabled in the CMakeLists.txt of the features module. Is there a unit test phase that I should be aware off?

VictorLamoine · 2014-07-08T14:31:13Z

Compile it like the travis.sh script and you'll get the bug.
I guess it comes from PCL_ONLY_CORE_POINT_TYPES; I had the problem in a recent pull request.

Bye

desinghkar · 2014-07-09T11:43:25Z

@VictorLamoine: Ya it seems so but when I turn on the PCL_ONLY_CORE_POINT_TYPES option I don't see the error locally. Build is failing at clang and travis.sh has only PCL_ONLY_CORE_POINT_TYPES being turned on. Am I missing something. And the way I turned the PCL_ONLY_CORE_POINT_TYPES on is by using pcl_options.cmake in cmake folder.

VictorLamoine · 2014-07-09T12:05:15Z

I get the same errors as the Travis build with gcc 4.6.3.
You should not edit pcl_options.cmake but rather configure your PCL compilation like:

cd pcl/build && \
cmake .. -DPCL_ONLY_CORE_POINT_TYPES=ON && \
make -j2

desinghkar · 2014-07-09T14:48:55Z

@VictorLamoine : Thanks a lot that helped me debug it easly and I had to add the below lines into the grsd.cpp. I have been looking at RSD as the error suggested so but then GRSD was calling RSD and was failing for all the types.

#ifdef PCL_ONLY_CORE_POINT_TYPES
 PCL_INSTANTIATE_PRODUCT(GRSDEstimation, ((pcl::PointXYZ)(pcl::PointXYZI)(pcl::PointXYZRGBA))((pcl::Normal))((pcl::GRSDSignature21)))
#else
         PCL_INSTANTIATE_PRODUCT(GRSDEstimation, (PCL_XYZ_POINT_TYPES)(PCL_NORMAL_POINT_TYPES)((pcl::GRSDSignature21)))
#endif
#endif

desinghkar · 2014-07-09T17:25:30Z

Hi again :)
Trace build error again! It says

I'm sorry but your test run exceeded 50.0 minutes.
One possible solution is to split up your test run.

So how to do that. And I don't think I have written any tests for RSD and GRSD how to go about that?

VictorLamoine · 2014-07-10T06:35:30Z

Don't bother the timeout. Basically you don't have to do anything about this.

You could write a test unit for the RSD / GRSD if you think it is useful.
Don't hesitate to take a look at existing files in the test folder.

I don't think we need the features/doxyfile file from your pull request.

desinghkar · 2014-07-14T14:11:56Z

I removed the doxyfile in the latest commit and pushed it.

taketwo · 2014-07-16T10:37:15Z

features/src/grsd.cpp

+#ifdef PCL_ONLY_CORE_POINT_TYPES
+  PCL_INSTANTIATE_PRODUCT(GRSDEstimation, ((pcl::PointXYZ)(pcl::PointXYZI)(pcl::PointXYZRGBA))((pcl::Normal))((pcl::GRSDSignature21)))
+#else
+	  PCL_INSTANTIATE_PRODUCT(GRSDEstimation, (PCL_XYZ_POINT_TYPES)(PCL_NORMAL_POINT_TYPES)((pcl::GRSDSignature21)))


Please use spaces here.

taketwo · 2014-07-16T10:41:10Z

Please also edit the commit message. I think it suffices to keep just the first paragraph.

desinghkar · 2014-07-17T14:41:20Z

Sure Sergey! will fix them now.

desinghkar · 2014-07-23T18:15:08Z

Its a timeout error again! I have written global_tests for RSD and GRSD. I would like to include them in the trunk as well. But should I wait for this pull request to pass and raise another? Or should I push them along with this request since its still on hold. Let me know which is appropriate!

VictorLamoine · 2014-07-23T20:25:07Z

Push the test unit into an other commit of this pull request!
EDIT: And please correct the files to match PCL style guide 😉

VictorLamoine · 2014-07-23T20:29:16Z

features/include/pcl/features/grsd.h

+      double max_min_radius_diff = 0.050)
+  {
+    if (min_radius > min_radius_plane)
+      return 1; // plane


Can you put return values into braces please ?
return (1):
etc.

taketwo · 2014-07-23T20:31:20Z

Push the test unit into an other commit of this pull request!

👍 just add a second commit

desinghkar · 2014-07-25T13:07:23Z

Hope I have followed all the rules of style guide and the error is just with respect to the time out!

desinghkar · 2014-07-25T17:38:05Z

Is the clang error relevant to my push?

VictorLamoine · 2014-07-26T08:07:14Z

No that's a Travis bug!

desinghkar · 2014-07-26T14:47:48Z

Ok then! Anything I should do to merge this and close the request?

taketwo · 2014-07-27T11:56:50Z

features/include/pcl/features/grsd.h

+    */
+
+  inline int
+  getSimpleType (float min_radius, float max_radius,


I do not like this free documentation-less function in pcl namespace. I would suggest we make it a static member of GRSDEstimation. (Private or protected if it is not supposed to be used outside of the class, or public otherwise.)

Can you tell me on how to do that?
Or point me to similar modules so that i can replicate them accordingly!
Thanks in advance!

I am not sure what confuses you there. Static functions is not something PCL-specific... just have a look at some article (like this) to give yourself a refresher on the topic.

Thanks that was helpful. :)

taketwo · 2014-07-27T12:00:57Z

Hi Karthik, I think we are very close to merging, but please have a look at the comments I left.

desinghkar · 2014-07-28T10:17:52Z

Sure Sergey! I will look into them now.

GRSD is developed by Zoltan-Marton, this was a old patch as part of his branch being included into the trunk Adding test files for RSD and GRSD. style guide issues resolved

taketwo · 2014-07-31T08:28:32Z

Three weeks, 50+ comments, and we are finally done :)
@VictorLamoine thanks for helping with the review

Including GRSD a global radius-based surface descriptor

VictorLamoine reviewed Jul 8, 2014
View reviewed changes

taketwo reviewed Jul 8, 2014
View reviewed changes

taketwo reviewed Jul 16, 2014
View reviewed changes

VictorLamoine reviewed Jul 23, 2014
View reviewed changes

taketwo reviewed Jul 27, 2014
View reviewed changes

Including GRSD a global radius-based surface descriptor

6854ddb

GRSD is developed by Zoltan-Marton, this was a old patch as part of his branch being included into the trunk Adding test files for RSD and GRSD. style guide issues resolved

taketwo added a commit that referenced this pull request Jul 31, 2014

Merge pull request #790 from desinghkar/grsd_request

5a621c3

Including GRSD a global radius-based surface descriptor

taketwo merged commit 5a621c3 into PointCloudLibrary:master Jul 31, 2014

taketwo mentioned this pull request Jul 31, 2014

Move default argument values from definition to declaration #821

Merged

larshg mentioned this pull request Aug 4, 2014

Link error 1189 library limit of 65535 objects exceeded #833

Closed

Including GRSD a global radius-based surface descriptor #790

Including GRSD a global radius-based surface descriptor #790

Conversation

desinghkar commented Jul 8, 2014

desinghkar commented Jul 8, 2014

VictorLamoine commented Jul 8, 2014

Choose a reason for hiding this comment

desinghkar commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taketwo commented Jul 8, 2014

desinghkar commented Jul 8, 2014

VictorLamoine commented Jul 8, 2014

desinghkar commented Jul 8, 2014

desinghkar commented Jul 8, 2014

taketwo commented Jul 8, 2014

VictorLamoine commented Jul 8, 2014

taketwo commented Jul 8, 2014

desinghkar commented Jul 8, 2014

VictorLamoine commented Jul 8, 2014

desinghkar commented Jul 8, 2014

VictorLamoine commented Jul 8, 2014

desinghkar commented Jul 9, 2014

VictorLamoine commented Jul 9, 2014

desinghkar commented Jul 9, 2014

desinghkar commented Jul 9, 2014

VictorLamoine commented Jul 10, 2014

desinghkar commented Jul 14, 2014

Choose a reason for hiding this comment

taketwo commented Jul 16, 2014

desinghkar commented Jul 17, 2014

desinghkar commented Jul 23, 2014

VictorLamoine commented Jul 23, 2014

Choose a reason for hiding this comment

taketwo commented Jul 23, 2014

desinghkar commented Jul 25, 2014

desinghkar commented Jul 25, 2014

VictorLamoine commented Jul 26, 2014

desinghkar commented Jul 26, 2014

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

taketwo commented Jul 27, 2014

desinghkar commented Jul 28, 2014

taketwo commented Jul 31, 2014