-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Good descriptor #1907
base: master
Are you sure you want to change the base?
Good descriptor #1907
Conversation
…nted in Pattern Recognition Letters Journal (JULY2016) and IROS2016 (October).
Thanks @SeyedHamidreza. Can you suppress these though? Also note, it's threshold not thereshold 😅 In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:227:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Xpositive < Xnegative) and (Xnegative - Xpositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:23: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZ>::signDisambiguationXAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>;
^
In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:257:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Ypositive < Ynegative) and (Ynegative - Ypositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:23: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZ>::signDisambiguationYAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>;
^
In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:227:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Xpositive < Xnegative) and (Xnegative - Xpositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:74: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZI>::signDisambiguationXAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>;
^
In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:257:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Ypositive < Ynegative) and (Ynegative - Ypositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:74: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZI>::signDisambiguationYAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>;
^
In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:227:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Xpositive < Xnegative) and (Xnegative - Xpositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:126: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZRGB>::signDisambiguationXAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>;
^
In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:257:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Ypositive < Ynegative) and (Ynegative - Ypositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:126: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZRGB>::signDisambiguationYAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>;
^
In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:227:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Xpositive < Xnegative) and (Xnegative - Xpositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:180: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZRGBA>::signDisambiguationXAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>;
^
In file included from /home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:39:
/home/travis/build/PointCloudLibrary/pcl/features/include/pcl/features/impl/good.hpp:257:58: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
if ((Ypositive < Ynegative) and (Ynegative - Ypositive >= thereshold))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/home/travis/build/PointCloudLibrary/pcl/features/src/good.cpp:46:180: note: in instantiation of member function 'pcl::GOODEstimation<pcl::PointXYZRGBA>::signDisambiguationYAxis' requested here
template class pcl::GOODEstimation<pcl::PointXYZ>; template class pcl::GOODEstimation<pcl::PointXYZI>; template class pcl::GOODEstimation<pcl::PointXYZRGB>; template class pcl::GOODEstimation<pcl::PointXYZRGBA>; |
* Software License Agreement (BSD License) | ||
* | ||
* Point Cloud Library (PCL) - www.pointclouds.org | ||
* Copyright (c) 2010-2012, Willow Garage, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you developed this code back in 2010-2012 while working at Willow Garage, please remove this copyright.
Instead, please add Copyright (c) 2017-, Open Perception, Inc.
Same for the remaining files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -1,6 +1,6 @@ | |||
set(SUBSYS_NAME features) | |||
set(SUBSYS_DESC "Point cloud features library") | |||
set(SUBSYS_DEPS common search kdtree octree filters 2d) | |||
set(SUBSYS_DEPS common sample_consensus search kdtree octree filters 2d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergioRAgostinho I hope this new dependency won't break your CI job distribution :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filters also has it fortunately :phew:
features/CMakeLists.txt
Outdated
@@ -62,6 +62,8 @@ if(build) | |||
"include/pcl/${SUBSYS_NAME}/usc.h" | |||
"include/pcl/${SUBSYS_NAME}/boundary.h" | |||
"include/pcl/${SUBSYS_NAME}/range_image_border_extractor.h" | |||
"include/pcl/${SUBSYS_NAME}/range_image_border_extractor.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate line is still here :x
features/include/pcl/features/good.h
Outdated
* \param[in] number_of_bins_ | ||
* \param[in] threshold_ | ||
*/ | ||
GOODEstimation(unsigned int, float); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add parameter names to the signature. Also, it would be nice to see explanation what these parameters mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
features/include/pcl/features/good.h
Outdated
* \param[in] cloud the boost shared pointer to a point cloud. | ||
*/ | ||
void | ||
setInputCloud (boost::shared_ptr<pcl::PointCloud<PointInT> > cloud); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use typename pcl::PointCloud<PointInT>::Ptr
here and everywhere. At some point in future we may want to switch to std::shared_ptr
, and it will be simpler if the typedef is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
features/include/pcl/features/good.h
Outdated
*/ | ||
|
||
template <typename PointInT> | ||
class GOODEstimation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you derive from pcl::Feature
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we need PCL_EXPORTS
here, i.e. class PCL_EXPORTS GOODEstimation
A general comment: please update to conform to the PCL Style guide. Especially spaces between function names and braces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't completed by there's considerable changes to be done:
- Not being derived from pcl::feature!
- Code style
- Copyright notices
- No use of const qualifiers
#include <boost/filesystem.hpp> | ||
|
||
typedef pcl::PointXYZRGBA PointT; | ||
using namespace pcl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refrain from using using namespace whatever;
, even though this is a contained example. Just define the symbols you use the most. I also noticed you then use the fully qualified names later on in the file, so in this case just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
int main(int argc, char* argv[]) | ||
{ | ||
|
||
if (argc < 2 ||argc > 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not simply != 2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std::cout << "Cloud reading failed." << std::endl; | ||
return (-1); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider replacing both these blocks for pcl::io::load
defined in <pcl/io/auto_io.h>. This will refrain you from having to check file extensions.
using namespace pcl; | ||
|
||
int | ||
readPointCloud(std::string object_path, boost::shared_ptr<pcl::PointCloud<PointT> > point_cloud) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding spacing of the parenthesis in functions have a look at http://www.pointclouds.org/documentation/advanced/pcl_style_guide.php#spacing
This would lead to
readPointCloud (std::string object_path, boost::shared_ptr<pcl::PointCloud<PointT>> point_cloud)
This comment is applicable to the remaining functions, methods and template instantiations of this PR.
return -1; | ||
|
||
std::vector< float > object_description; | ||
std::vector < boost::shared_ptr<pcl::PointCloud<PointT> > > vector_of_projected_views; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a required change. All these boost::shared_ptr<pcl::PointCloud<PointT>>
invocations can be replaced by the equivalent pcl::PointCloud<PointT>::Ptr
saving you some trouble writing the full name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
{ | ||
input_ = cloud; | ||
}; | ||
////////////////////////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please delete. To be derived from PCLBase
{ | ||
threshold_ = threshold; | ||
} | ||
////////////////////////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move both of these to the .h and define them inline and add the const qualifier to the function argument.
{ | ||
transformation = transformation_; | ||
} | ||
////////////////////////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the four above methods to the .h, declare them inline and declare the methods const
getTransformationMatrix (Eigen::Matrix4f &transformation) const
dimensions.x = (maximum_pt.x - minimum_pt.x); | ||
dimensions.y = (maximum_pt.y - minimum_pt.y); | ||
dimensions.z = (maximum_pt.z - minimum_pt.z); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not simply
dimensions = maximum_pt - minimum_pt;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the type of dimentions variable is XYZ, while the type of maximum_pt and minimum_pt can be XYZRGB or other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. What you need is maximum_pt.getVector3fMap () - minimum_pt.getVector3fMap ()
.
features/include/pcl/features/good.h
Outdated
* \param[out] pc_out the resultant projected point cloud | ||
*/ | ||
void | ||
projectPointCloudToPlane (boost::shared_ptr<pcl::PointCloud<PointInT> > pc_in, boost::shared_ptr<pcl::ModelCoefficients> coefficients, boost::shared_ptr<pcl::PointCloud<PointInT> > pc_out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here on out you start forgetting to include & on your function parameters...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is OK, I used pointers instead of references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you know, a shared_ptr is not just a trivial pointer. The difference in size compared to a raw ptr can in most cases be considered negligible, unless you specify its custom allocator through means of a functor object. But you are in fact triggering unnecessarily the reference counting mechanism because none of these methods really has the intent of "taking ownership" of the object. They just use it briefly and then release it.
Basically we're gaining nothing in this scenario, in passing the pointer by value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function forwards the point cloud parameter to projection.setInputCloud
. So you can just have same qualifiers as in that function.
@SergioRAgostinho @taketwo : Thanks for all your comments, I am going to check one by one and will commit a new version soon :D 💯 |
@SeyedHamidreza no need to put done under each comment, just push a commit with the change and Github will detect it. |
…o derive GOODEstimation from pcl::Feature, since I got confused.
So your proposed code looks like this: template <typename PointInT, int number_of_bins >
class PCL_EXPORTS GOODEstimation : public Feature<PointInT, PointOutT> First problem is that |
I made a mistake to write it here, In the code it was correct, |
Assuming the number of bins is fixed, do you know in advance the length of this descriptor vector? |
yes, the length of the descriptor will be 3 * (number_of_bins^2) |
Great, it means we can use the template <typename PointInT, int N>
class PCL_EXPORTS GOODEstimation : public Feature<PointInT, Histogram<3*N*N> > |
Thank you, I think about it since I see something similar in spin_image but still, the following items are not clear to me: what about the definition of a method, assume the following one: template <typename PointInT, int N>
void
pcl::GOODEstimation<PointInT, pcl::Histogram<3*N*N> >::computeBoundingBoxDimensions (PointCloudIn pc, pcl::PointXYZ &dimensions)
{
...
} and also about the cpp file: #ifdef PCL_ONLY_CORE_POINT_TYPES
PCL_INSTANTIATE_PRODUCT(GOODEstimation, ((pcl::PointXYZ)(pcl::PointXYZI)(pcl::PointXYZRGB)(pcl::PointXYZRGBA))(pcl::Histogram<3*N*N>) )
#else
PCL_INSTANTIATE_PRODUCT(GOODEstimation, (PCL_XYZ_POINT_TYPES)((pcl::Histogram<75>)))
#endif
#endif // PCL_NO_PRECOMPILE |
Method definitions should read like: template <typename PointInT, int N>
void
pcl::GOODEstimation<PointInT, N>::computeBoundingBoxDimensions (PointCloudIn pc, pcl::PointXYZ &dimensions)
{
...
} Regarding the instantiation in cpp file, do you have any |
yes, Actually N=5 is a good option that we put it also as a default value for the number of bins parameters, |
@SergioRAgostinho Every time before pushing the code, I checked it out with my original code and some synthetic inputs to be sure everything is in a right way. Thanks for your comments. |
I've pre selected this for the 1.9 release but this is still lacking unit tests so I'm gonna remove this for now. If you want to meet this release please let us know and get those unit tests out. |
I would like to release the GOOD in PCL 1.9, just let me know what should I do for the unit tests. :) |
Thank you. Best regards, |
I have added the unit test 💯 Best regards, |
@SergioRAgostinho @taketwo |
@SeyedHamidreza Not at this moment. Right now we're focused on other more pressing goals, so reviewing this became low priority. My apologies. |
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. |
Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this. |
This is an implementation of the Global Orthographic Object Descriptor (GOOD) which has been presented in the following papers:
The GOOD descriptor has been designed to be robust, descriptive and efficient to compute and use. GOOD descriptor has two outstanding characteristics:
(1) Providing a good trade-off among:
- descriptiveness,
- robustness,
- computation time,
- memory usage.
(2) Allowing concurrent object recognition and pose estimation for manipulation.
The performance of the proposed object descriptor is compared with the main state-of-the-art descriptors. Experimental results show that the overall classification performance obtained with GOOD is comparable to the best performances obtained with the state-of-the-art descriptors. Concerning memory and computation time, GOOD clearly outperforms the other descriptors. Therefore, GOOD is especially suited for real-time applications.
The current implementation of GOOD descriptor supports several functionalities for 3D Object Recognition and Object Manipulation. In the current distribution, you can find
To show all the functionalities and properties of the GOOD descriptor, a real demonstration was performed. A video of this demonstration is available in: https://youtu.be/iEq9TAaY9u8
Cheers,
Hamidreza