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

[registration] Don't move, or copy ICP #4167

Merged
merged 3 commits into from
Aug 14, 2020

Conversation

kunaltyagi
Copy link
Member

@kunaltyagi kunaltyagi commented Jun 5, 2020

This is a work around the bad design choices in ICP. I'd like for someone to overhaul the design, but with limited time, this is all I can do. Target milestone: 1.12.0

Closes #2965

THis is a work around the bad design choices in ICP. I'd like for someone to overhaul the desin, but with limited time, this is all I can do. Target milestone: 1.12.0

Closes PointCloudLibrary#2965
@kunaltyagi kunaltyagi added this to the pcl-1.12.0 milestone Jun 5, 2020
@kunaltyagi kunaltyagi added changelog: ABI break Meta-information for changelog generation changelog: API break Meta-information for changelog generation changelog: behavior change Meta-information for changelog generation changelog: fix Meta-information for changelog generation module: registration needs: code review Specify why not closed/merged yet labels Jun 5, 2020
@SergioRAgostinho SergioRAgostinho removed changelog: ABI break Meta-information for changelog generation changelog: behavior change Meta-information for changelog generation labels Jun 5, 2020
@kunaltyagi
Copy link
Member Author

I wonder if there can be a check for current milestone. The big green button is very tempting

@kunaltyagi
Copy link
Member Author

Why aren't ABI break and behavior change labels valid? Do we choose the best fit label per PR instead?

@SergioRAgostinho
Copy link
Member

API breaks implies an ABI break. When viewing the ABI changes section in the change log, you're really only interested in seeing items in which only the ABI changed but not the API.

My removal of behavior was incorrect. Effectively you can't copy an ICP object anymore.

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.

Before we merge this: (I'm requesting changes but not really. Just want to keep flag this as don't merge yet)
Is it reasonable to delete the move operations? There should be nothing wrong with moving an ICP object around. And I suspect that by erasing the move you might creating problems for factory like functions. I'll try to compose an example case to illustrate.

https://godbolt.org/z/m3wjak

@kunaltyagi
Copy link
Member Author

Is it reasonable to delete the move operations?

The convergence criteria can't be moved that easily since it contains references as well as hidden state that's not exposed by the API.

@kunaltyagi kunaltyagi added changelog: behavior change Meta-information for changelog generation needs: feedback Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jun 8, 2020
@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: feedback Specify why not closed/merged yet labels Jun 8, 2020
@kunaltyagi
Copy link
Member Author

Note: Please squash. Not to be merged before 1.12 release is decided

@kunaltyagi kunaltyagi merged commit 49385a6 into PointCloudLibrary:master Aug 14, 2020
@kunaltyagi kunaltyagi deleted the no-copy branch August 14, 2020 14:54
nlimpert added a commit to carologistics/fawkes-robotino that referenced this pull request Apr 8, 2022
Since PCL 1.12 the assignment operator of IterativeClosestPoint has been set to delete
which causes an implicit deletion of the inheriting class CustomICP.
Refer to PointCloudLibrary/pcl#4167 for further
details.
nlimpert added a commit to carologistics/fawkes-robotino that referenced this pull request Apr 8, 2022
Since PCL 1.12 the assignment operator of IterativeClosestPoint has been set to
delete which causes an implicit deletion of the inheriting class CustomICP.
Refer to PointCloudLibrary/pcl#4167 for further
details.
nlimpert added a commit to carologistics/fawkes-robotino that referenced this pull request Apr 8, 2022
Since PCL 1.12 the assignment operator of IterativeClosestPoint has been set to
delete which causes an implicit deletion of the inheriting class CustomICP.
Refer to PointCloudLibrary/pcl#4167 for further
details.
nlimpert added a commit to carologistics/fawkes-robotino that referenced this pull request Apr 8, 2022
Since PCL 1.12 the assignment operator of IterativeClosestPoint has been set to
delete which causes an implicit deletion of the inheriting class CustomICP.
Refer to PointCloudLibrary/pcl#4167 for further
details.
nlimpert added a commit to carologistics/fawkes-robotino that referenced this pull request May 21, 2022
Since PCL 1.12 the assignment operator of IterativeClosestPoint has been set to
delete which causes an implicit deletion of the inheriting class CustomICP.
Refer to PointCloudLibrary/pcl#4167 for further
details.
nlimpert added a commit to carologistics/fawkes-robotino that referenced this pull request Jul 2, 2022
Since PCL 1.12 the assignment operator of IterativeClosestPoint has been set to
delete which causes an implicit deletion of the inheriting class CustomICP.
Refer to PointCloudLibrary/pcl#4167 for further
details.
nlimpert added a commit to carologistics/fawkes-robotino that referenced this pull request Jul 2, 2022
Since PCL 1.12 the assignment operator of IterativeClosestPoint has been set to
delete which causes an implicit deletion of the inheriting class CustomICP.
Refer to PointCloudLibrary/pcl#4167 for further
details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: API break Meta-information for changelog generation changelog: behavior change Meta-information for changelog generation changelog: fix Meta-information for changelog generation module: registration needs: code review Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copying pcl::IterativeClosestPoint object causes issues
2 participants