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.

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

ObjRecRANSAC::addModel() error #269

Merged
merged 6 commits into from
Oct 17, 2013

Conversation

musi00
Copy link
Contributor

@musi00 musi00 commented Sep 17, 2013

ObjRecRANSAC::addModel() will core dump for certain values of the variables, pair_width_ and voxel_size_, in recognition:ObjRecogRANSAC. Certain values can cause the point pair feature calculation to return some nan value. These nan values are used as an index to access a hash table cell and therefore failing.

This happens because the method recognition::ModelLibrary::getFullLeavesIntersectedBySphere((const float* p, float radius, std::listORROctree::Node*& out) which returns the set of leaves that are a specific distance apart from the source point p can return the leaf of the source point in the solution set. This causes the calculation of the point pair feature of a the point with itself. When calculating the point pair feature of a point with itself a division by zero operation occurs which causes a nan valued point pair feature which is used as a key to a hash table resulting in a core dump. Fixed it by:

  1. Adding a new method to auxiliary.h, namely equal3() which compares two arrays of 3 elements to see if all elements are equal
  2. Used the equal3() method in getFullLeavesIntersectedBySphere()to ensure that the centre of mass of a leaf intersected by the sphere is not the same as the query point p (i.e the centre of mass of the query leaf). Thus, preventing self intersections which cause nan values when calculating the point pair feature.

@jspricke
Copy link
Member

Can you remove the commits from #245? Thanks!

@musi00
Copy link
Contributor Author

musi00 commented Sep 30, 2013

sorry, I am not a git expert. What is the best way to go about doing that

@jspricke
Copy link
Member

  • musi00 notifications@github.com [2013-09-30 12:16]:

    sorry, I am not a git expert. What is the best way to go about doing that

have a look at git rebase -i

@taketwo
Copy link
Member

taketwo commented Sep 30, 2013

Run git rebase -i HEAD~7, this will open an editor and let you "edit" the last 7 commits in the branch. Simply delete the lines corresponding to the commits you do not need and save the file. Then you can check the git log to see if you like the result. Finally, push the branch again with git push --force, the pull request will be updated automatically.

…esIntersectedBySphere to prevent inclusion of source point leaf
@jspricke
Copy link
Member

jspricke commented Oct 2, 2013

@musi00 could you reformat this according to our style guide: http://pointclouds.org/documentation/advanced/pcl_style_guide.php? Also, could you probably add a test case? Thanks!

@musi00
Copy link
Contributor Author

musi00 commented Oct 7, 2013

to create the test case. It seems I need to do the following:

  1. create a test_something.cpp file in the test folder
  2. write a test case using the TEST() Macto and call it in main using RUN_ALL_TESTS

I am not sure if there are any other Macros that I need to use. In my case, I need to test that the get pcl::recognition::ORROctree::getFullLeavesIntersectedBySphere() doesn't return the query leaf in the solution of leafs intersected by the sphere. So are there Macros to be used for this check?

Anything else I am missing?

@taketwo
Copy link
Member

taketwo commented Oct 7, 2013

You may write a for loop over the leaves in the solution and check that each of them is not equal to the query leaf with EXPECT_NE(val1, val2); macro.

In general, this primer is a good starting point if you are new to this testing framework.

@musi00
Copy link
Contributor Author

musi00 commented Oct 8, 2013

Two questions:

  1. does EXPECT_NE compare two float[3] arrays?
  2. I am unable to link to gtest. Do I need to download and install this library seperately?

@taketwo
Copy link
Member

taketwo commented Oct 8, 2013

  1. Not in the way you would expect. If you pass two float[3] arrays as arguments it will compare whether they point to the same memory address (I suppose). So you will need a loop. Note that for float comparison you should use EXPECT_FLOAT_EQ(expected, actual);
  2. As far as I know PCL already includes gtest1.6.

@musi00
Copy link
Contributor Author

musi00 commented Oct 9, 2013

yes, I do see the gtest source within the pcl source but it doesn't seem that the pcl install automatically compiles it into a library or am I mistaken.

@musi00
Copy link
Contributor Author

musi00 commented Oct 9, 2013

solved gtest issue by installing gtest library on my system

@taketwo
Copy link
Member

taketwo commented Oct 9, 2013

Have a look at 'test/CMakeLists.txt', you will notice rules to compile gtest. Furthermore, there are rules to build different unit tests, these may be taken as an example.

@musi00
Copy link
Contributor Author

musi00 commented Oct 9, 2013

Thanks for all the help Sergey


/** \brief a = b */
template <typename T> bool
equal3(const T a[3], const T b[3])
Copy link
Member

Choose a reason for hiding this comment

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

Please add a space before (

@jspricke
Copy link
Member

Could you please reformat this using two spaces for every indention level, according to the style guide? Thanks!

@musi00
Copy link
Contributor Author

musi00 commented Oct 11, 2013

spacing looked fine on my end when I opened the files in qt creator or vim but were not fine in the github diff. I replaced my 'tabs' with spaces and I think that fixed things.

jspricke added a commit that referenced this pull request Oct 17, 2013
@jspricke jspricke merged commit 86b6612 into PointCloudLibrary:master Oct 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants