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

Consistent ptr typedefs for kd tree flann #1607

Conversation

wsascha
Copy link
Contributor

@wsascha wsascha commented May 16, 2016

Consistent use of 'Dist' template type for users having custom distance functors.

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented May 16, 2016

First look into it looks OK. Two comments:
- Could you please write a test exactly exploiting a custom distance case
- Please squash into a single commit

@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Aug 18, 2016
@SergioRAgostinho SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Aug 18, 2016
@SergioRAgostinho SergioRAgostinho added status: ready to merge and removed needs: author reply Specify why not closed/merged yet labels Jul 6, 2017
Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

This should also be modified accordingly.

@SergioRAgostinho SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed status: ready to merge labels Jul 6, 2017
@taketwo
Copy link
Member

taketwo commented Jul 6, 2017

If that is modified, then this should also be updated, of course.

@wsascha
Copy link
Contributor Author

wsascha commented Jul 6, 2017

Sorry guys, I completely forgot about this!

I propose to instantiate only with the default Dist template parameter. If anyone needs special distances types, they can instantiate those by themselves.

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Jul 6, 2017

I propose to instantiate only with the default Dist template parameter. If anyone needs special distances types, they can instantiate those by themselves.

That is, to leave it unchanged? My idea was not to instantiate more types. I just wanted to make it clear when reading the instantiations that the class actually depends on two separate types.

But if the general acceptance is to leave it like, then this one is good to go.

@taketwo
Copy link
Member

taketwo commented Jul 6, 2017

I agree that we do not need to preinstantiate with other distance types. So far everyone was happy with the default one. (And which ones would you choose anyway?)

As per Sergio's comment, yes, we can update the instantiation macro. But in user code instantiation happens implicitly, so the macro is not really needed.

@taketwo taketwo removed the needs: author reply Specify why not closed/merged yet label Nov 7, 2017
@taketwo taketwo merged commit 7b3b571 into PointCloudLibrary:master Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants