search::KdTree accepts different types of KdTree

[fran6co]

Now search::KdTree can be used with different KdTree implementations.

[aichim]

Hi,

Was this discussed in any way on the mailing lists? I would completely say no to this as it brings tons of trouble with no advantages to 95% of the users. I agree that there is the use case of using something different than FLANN, but how many people will opt for that? Plus, we do not have a FLANN-alternative in PCL right now.

Cheers,
Alex

[fran6co]

Hi @aichim,

I'm working on a KdTree implementation based on https://code.google.com/p/nanoflann/ and I had to create my own pcl::search::KdTree class to use it in the segmentation algorithms.
This code is backward compatible as the default tree is still the FLANN implementation, I don't see any trouble with it as the users that don't know or care about changing the underlying KdTree would use it as it is.
As a library PCL should offer an easy way to be extended, it's not about how many people would use it. Particulary in this case I don't see any harm on doing so.

I didn't bring it up in the mailing lists because I had the code already and I thought it would be easier to discuss it here on GitHub. If the proper way to is to go to the mailing list first I think it would be nice to create a CONTRIBUTING.md file on the repository (https://github.com/blog/1184-contributing-guidelines).

[jkammerl]

I agree, this patch might not be relevant to a lot of users. But PCL shouldn't be bound to FLANN. Since this patch only slightly extends the interface of the pcl::search::KdTree wrapper class without changing its behavior, I don't see lot's of trouble here.

[aichim]

Hi,

@fran6co That is very nice, thanks for the initiative! And thanks for the suggestion with the .md file.

@both, check this out, I don't need to explain more - http://en.wikipedia.org/wiki/You_aren't_gonna_need_it .
@fran6co I think you should first do the nanoFLANN port on your branch and benchmark it, and if it proves better than FLANN, we can accept such pull requests (OR we can contact Marius Muja to make FLANN even better!).

So I still vote for a no on this for now, as it is really not needed, and we should not add things just for the sake of it, especially when it comes to low-level functionality and very essential things.

Cheers,
Alex

[fran6co]

I'm working on the benchmarking of the nanoFLANN port and if it's better than the current implementation I'll do a PR of it. (I made the PR anyways, It's better to get some reviews on it even if it's not faster)

As much as I agree with YAGNI principle, I don't see how this PR is against that principle as it doesn't:

• take extra time to test it
• need extra documentation and support, as it just needs to comply to the existing interfaces
• add constraints to what be done in the future, I would argue it's the other way around
• need extra definitions, as the feature is very clear already
• create code bloat
• suggest other new feature, as the interface for pcl::kdtree is already there ready to be extended

I think the pcl::search::kdtree should accept anything that is a kdtree, just to keep things modular.

[aichim]

@jkammerl , is it a go from your side?

[rbrusu]

Looks good to me, without any problems. I'll defer to Julius's judgement though if this should be included or not. He's more familiar with search methods in PCL than I am.

[rbrusu]

Is there any follow up to this issue? @aichim @jkammerl ?

[rbrusu]

What about this one?

[jkammerl]

Sorry for losing track on this one. It would be nicer to avoid the additional template parameter by instantiating the search class with the desired kdtree instance (and use it as a class member). But since this would require a larger revision of the search interface, I am fine with this patch.