search

PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.91k stars 4.61k forks source link

Sac model perpendicular plane with normals missing #5211

Open 777Gerald777 opened 2 years ago

777Gerald777 commented 2 years ago

There is a SACMODEL_PERPENDICULAR_PLANE but no SACMODEL_NORMAL_PERPENDICULAR_PLANE, should be easy to add it to pcl/sample_consensus/model_types.h.

mvieth commented 2 years ago

You are welcome to open a pull request to add new features to PCL. Note however, that simply adding that new entry to model_types.h is not enough, you also have to actually create the new model (e.g. with sac_model_normal_parallel_plane.h[pp] as a reference), and add it to a few other files like sac_segmentation.h[pp]. In newer PCL versions, there is the function setModelConstraints(). You can use this function to get a normal-perpendicular-plane model.

777Gerald777 commented 2 years ago

setModelConstraints sounds cool but the function pcl::SACSegmentationFromNormals::getModel returns an empty NULL pointer before calling the segment execution, so i have to make an own class which inherits from pcl::SACSegmentationFromNormals to override initSACModel function so that i can use setModelConstraints. And also this complicated method does not work because initSACModel is not virtual, lol. so i have to inherit from SACSegmentation and copy code from SACSegmentationFromNormals....

ya, those git stuff with pull and push requests is too complicated for me, sorry (i only work with svn)... only wanted to make a feedback that there was something forgotten.....

mvieth commented 2 years ago

Yes, using setModelConstraints with SACSegmentation or SACSegmentationFromNormals is currently not easy. But it is when using this other way for sample consensus.

777Gerald777 commented 2 years ago

i found also something funny:

SACMODEL_PERPENDICULAR_PLANE behaves like SACMODEL_NORMAL_PARALLEL_PLANE..... (referring to the axis orientation, for sure normals are not used in first)

so, either, SACMODEL_NORMAL_PARALLEL_PLANE should be called SACMODEL_NORMAL_PERPENDICULAR_PLANE or SACMODEL_PERPENDICULAR_PLANE and SACMODEL_PARALLEL_PLANE should be switched, otherwise the term PERPENDICULAR and PARALLEL does not mean the same on the normal variant, lol

mvieth commented 2 years ago

Yes, but that is well documented, see for example here: ... In addition, the plane normal must lie parallel to a user-specified axis. This means that the plane itself will lie perpendicular to that axis, similar to SACMODEL_PERPENDICULAR_PLANE ... I agree that the naming can be confusing, but we can't change that now without breaking the code/programs from users.

777Gerald777 commented 2 years ago

i know that it will break code, but i currently implemented my own "SAC_MODEL_PERPENDICULAR_PLANE" and debugged some hours of finding out why my plane was the wrong orientation....

i think that when i want to "upgrade" my algorithm from SAC_MODEL_PARALLEL to SAC_MODEL_NORMAL_PARALLEL i should expect that the plane orientation of the result does not flip...

i dont know what a beautiful solution would be, maybe adding SAC_MODEL_NORMAL_PARALLEL2 and making/marking SAC_MODEL_NORMAL_PARALLEL as depricated and remove in future releses and make the naming consistent....

mvieth commented 2 years ago

i dont know what a beautiful solution would be, maybe adding SAC_MODEL_NORMAL_PARALLEL2 and making/marking SAC_MODEL_NORMAL_PARALLEL as depricated and remove in future releses and make the naming consistent....

That would be a thing we could consider, but I don't see a nice way to deprecate the enum value (the deprecated attribute appears to only work for that since C++17, PCL aims to also compile with C++14), and there are more things to rename/deprecate than just the enum values, e.g. sac_model_normal_parallel_plane.h[pp] and the classes inside. I fear that trying to rename everything might just cause more confusion than the current state.

777Gerald777 commented 2 years ago

dont worry, for me it is ok. i only wanted to feedback it and provide possible solutions...