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

Optimized deserialization from sensor_msgs/PointCloud2 #3

Open
wants to merge 6 commits into
base: melodic
Choose a base branch
from

Conversation

YoshuaNava
Copy link
Collaborator

@YoshuaNava YoshuaNava commented Oct 26, 2020

I'm submitting this PR to optimize the deserialization of sensor_msgs/PointCloud2 into libpointmatcher/DataPoints.

For this PR I tried to reduce the original deserialization code size, move different parts into separate functions, and:

  • Rely on a sensor_msgs/PointCloud2Iterator instead of raw pointers.

  • Move the RemoveNaNDataPointsFilter out of the deserialization. This could be done afterwards.

  • Added setup for compiling doxygen comments and running unit tests.

In progress:

  • Finish unit tests implementation.

I've run some benchmarks on my laptop, and found that the new deserializer is running 3-5x faster than the previous implementation. According to Intel VTune, the previous implementation was getting vectorized to SSE3, but retired many more instructions per cycle than the new one. Do note that I still don't consider the performance of the new deserializer to be the best possible, but I think it is a positive improvement over the previous implementation. I'm following up on that here: ros2/common_interfaces#133

All feedback is warmly welcome.

PS: The code has been formatted using the clang format style sheet from this PR: norlab-ulaval/libpointmatcher#289

@YoshuaNava
Copy link
Collaborator Author

YoshuaNava commented Oct 26, 2020

I implemented unit tests today. Tomorrow I'll put a bit more time to hopefully implement them as gtest typed_tests, and cover float and double DataPoints.

Open questions:

  1. Is there any project that uses the libpointmatcher time field? To use it as reference

  2. The deserialization and serialization of color descriptors seem to have a negative effect on the accuracy of the values stored. In the unit tests I implemented I've compared the baseline implementation with the new one, and found that both can miss values in the [0,1] by as much as 0.5 units, or 50%. In some cases, values seem to have inverted sign (e.g. -0.3 instead of 0.9). Is the cause of this known?

@YoshuaNava YoshuaNava changed the title WIP: [libpointmatcher_ros] Optimized deserialization from sensor_msgs/PointCloud2 Optimized deserialization from sensor_msgs/PointCloud2 Oct 27, 2020
@pomerlef
Copy link
Contributor

For the time field, I did a quick survey and it's not yet in use on our side. It was a prospective feature, so you can do your best and we will test it once we have a use case (we should be close).

I recall that it is someone else who was doing color conversion. Knowledge about is probably lost in space...

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.

2 participants