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

Relax requirements in eigen22d test. Always provide a normalized result in pcl::transformPlane. #2503

Merged
merged 3 commits into from
Oct 11, 2018

Conversation

SergioRAgostinho
Copy link
Member

No description provided.

@SergioRAgostinho
Copy link
Member Author

I just discovered the eigen22f test is still failing on my 18.04 environment. Gonna have a look at it again although I'm fearing this is gonna get us back to the original problems in #1702.

@SergioRAgostinho
Copy link
Member Author

This one is also good to go.

// From Eigen 3.3.2 onwards, the Eigen::Hyperplane::transform always returns
// coefficients with the normal normalized
#if EIGEN_VERSION_AT_LEAST (3, 3, 2)
test /= test.head<3> ().norm ();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we rather tweak the function to return consistent results no matter what the Eigen version is? That is, to return normalized normal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that sounds cleaner.

@SergioRAgostinho
Copy link
Member Author

Don't let this one be forgotten.

@taketwo taketwo merged commit 42f31d3 into PointCloudLibrary:master Oct 11, 2018
@taketwo taketwo changed the title Relax requirements in eigen22d test. Adapt result from transformPlane test based on used Eigen version. Relax requirements in eigen22d test. Always provide a normalized result in pcl::transformPlane. Oct 11, 2018
@taketwo
Copy link
Member

taketwo commented Oct 11, 2018

Thanks for the ping.

@SergioRAgostinho SergioRAgostinho deleted the test-eigen branch October 11, 2018 08:54
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.

2 participants