-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add a common function called transformPointWithNormal #908
Conversation
|
||
ret.normal_x = static_cast<float> (transform (0, 0) * point.normal_x + transform (0, 1) * point.normal_y + transform (0, 2) * point.normal_z); | ||
ret.normal_y = static_cast<float> (transform (1, 0) * point.normal_x + transform (1, 1) * point.normal_y + transform (1, 2) * point.normal_z); | ||
ret.normal_z = static_cast<float> (transform (2, 0) * point.normal_x + transform (2, 1) * point.normal_y + transform (2, 2) * point.normal_z); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use matrix multiplications here? I.e. obtain xyz values as an Eigen vector via getVector3fMap()
and multiply with the transform matrix? The same for normals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can also use matrix multiplication.
I just copied the similar code in transformPoint here for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could update transformPoint
as well ;)
Matrix multiplication is faster and makes more obvious what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then I could update both functions.
By the way, it's so strange why this pull request cannot pass the Travis,
and I can't find clues in the output information. https://travis-ci.org/PointCloudLibrary/pcl/jobs/35188356
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately Travis became almost useless for us in the recent months. Compilation often fails not because of errors in code but because their machines can not fulfill memory/computational demands... So do not be intimidated by these red crosses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. So I have modified the code to use Eigen's matrix multiplication, both in transformPoint and transformPointWithNormal. And the unit test is also passed.
Actually I already asked Radu about this here. His reply suggested we need some more investigation as to find out what is faster nowadays. |
Thanks! |
Add a common function called transformPointWithNormal
This is a simple but common function and has passed the unit test.