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

New interface for LCCP #1389

Merged
merged 1 commit into from
Oct 27, 2015

Conversation

mschoeler
Copy link
Contributor

Changed the interface of LCCP.

  1. Added setInputSupervoxels method
  2. Changed segment method to not return data.
  3. Made mergeSmallSegments protected (it is now called by segment)
  4. Added setMinSegmentSize (const uint32_t min_segment_size_arg) as a public method
  5. Sorted the definitions in lccp_segmentation.hpp by public first and protected second.

@VictorLamoine
Copy link
Contributor

Just a suggestion: use \ref in your doxygen documentation every time a member function or member is named. It allows to easily navigate into the documentation and makes it more read-able.

Example : pcl::EnsensoGrabber::storeEEPROMExtrinsicCalibration

@taketwo
Copy link
Member

taketwo commented Oct 19, 2015

Victor, AFAIK in most cases doxygen is smart enough to figure out that we are referring to a member function even without \ref. For example, check this description (paragraph starting with "Several overloads..."), and the source that produced it here. Magic!

Proposed changes LGTM, and since this has not been released yet, we are free to change the interface.

@mschoeler
Copy link
Contributor Author

ok, added the \refs as well as some const to method variables.

@taketwo taketwo mentioned this pull request Oct 21, 2015
7 tasks
@mschoeler
Copy link
Contributor Author

do you have any more comments on this one or should we merge it? Than I would change the interface of CPC accordingly.
Bzw: I do not think the failed test (icp) comes from my changes.

@taketwo
Copy link
Member

taketwo commented Oct 27, 2015

As I said, I'm fine with the changes. I thought that @jspricke might want to comment, but since he doesn't, I'll merge tomorrow.

@jspricke
Copy link
Member

Sorry, had no time. Apart from the very minor rather big "Public Functions", I'm fine.

jspricke added a commit that referenced this pull request Oct 27, 2015
@jspricke jspricke merged commit 2a44982 into PointCloudLibrary:master Oct 27, 2015
@mschoeler mschoeler deleted the NewInterface_LCCP branch October 29, 2015 15:08
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.

4 participants