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

Rename IncrementalICP IncrementalRegistration #1451

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

jspricke
Copy link
Member

No description provided.

@jspricke
Copy link
Member Author

As discussed in #1426

@taketwo
Copy link
Member

taketwo commented Nov 29, 2015

I was wondering if we need two separate classes for "incremental" and "meta" registration. The only difference is whether a full cloud is accumulated or not. This could be controlled just with a switch. What do you think?

@SergioRAgostinho
Copy link
Member

I'm ok with having separate class entities for incremental and meta registration.

Nevertheless, what I think it would be actually sexy to have is to use them in "similar" to what it's done in NormalEstimation.setSearchMethod(), but something called setRegistrationStrategy(). Both incremental and meta would derive from a RegistrationStrategy base class, and any class derived from Registrationwould have access to it, and further matching strategies.

Ps: I really think the prefix meta gives no clue whatsoever on the matching strategy applied.

@taketwo
Copy link
Member

taketwo commented Nov 29, 2015

Ps: I really think the prefix meta gives no clue whatsoever on the matching strategy applied.

Exactly. And from what I see the code doing, I would rather call it "incremental".

@SergioRAgostinho
Copy link
Member

Ignore my last comment, I should've looked before writing. You're right @taketwo, it's too similar to be worth an entire new class. A switch should be more than enough.

taketwo added a commit that referenced this pull request Dec 9, 2015
Rename IncrementalICP IncrementalRegistration
@taketwo taketwo merged commit 3f45cf9 into PointCloudLibrary:master Dec 9, 2015
@jspricke jspricke deleted the ic_registration branch December 9, 2015 23:18
@jspricke
Copy link
Member Author

We discussed the name over here and it's in use since at least since ten years in various papers.
Regarding putting both in one class, I think it's semantically sufficiently different to have two classes.
If no one objects, I will merge #1426 this evening.

@SergioRAgostinho
Copy link
Member

👍

@taketwo
Copy link
Member

taketwo commented Dec 15, 2015

Leaving aside semantics, the diff between the source code of these classes is very small.

@taketwo
Copy link
Member

taketwo commented Dec 15, 2015

But anyway, I'm fine with having two classes.

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.

3 participants