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

Preserve viewpoint on sift computation #1508

Merged

Conversation

SergioRAgostinho
Copy link
Member

Fixes #1498. Now the header, sensor_origin and sensor_orientation are copied to the keypoints resultant pointcloud. Added small checks on the tests.

Fixes PointCloudLibrary#1498. Now the header, sensor_origin and sensor_orientation are copied to the keypoints resultant pointcloud. Added small checks on the tests.
return (&lhs == &rhs) ||
(lhs.seq == rhs.seq && lhs.stamp == rhs.stamp && lhs.frame_id == rhs.frame_id);
}

} // namespace pcl
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is an operator overload for == comparison right?
Why don't you use it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is implicitly used on the test here, and that is what forced me to define it. I thought that it was probably safer to write the == overload than to make the check on each individual member of the struct on the test. That way, if the struct changes in the future, the editor will know that he needs to update the operator as well, and test will still be valid.

@VictorLamoine
Copy link
Contributor

👍 apart from what I don't understand, which is probably legitimate.

@SergioRAgostinho
Copy link
Member Author

Thanks for the review 👍

@taketwo
Copy link
Member

taketwo commented Jan 25, 2016

LGTM. A trivial fix, I think it's safe to include it in 1.8.0.

VictorLamoine added a commit that referenced this pull request Jan 26, 2016
Preserve viewpoint on sift computation
@VictorLamoine VictorLamoine merged commit 93b7758 into PointCloudLibrary:master Jan 26, 2016
@SergioRAgostinho SergioRAgostinho deleted the sift_viewpoint branch August 19, 2016 15:59
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