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

Add a function to copy data between points of different types #465

Merged
merged 4 commits into from
Feb 11, 2014

Conversation

taketwo
Copy link
Member

@taketwo taketwo commented Jan 23, 2014

This pull request adds a new function that allows to copy data between points of different types:

template <typename PointInT, typename PointOutT> void
copyPoint (const PointInT& point_in, PointOutT& point_out);

The function has several specializations, which the compiler instantiates depending on the particular combination of input and output point types.

There are three cases:

  • Points have the same type.

    In this case a single memcpy is used.

  • Points have different types and one of the following is true:

    • both have RGB fields;
    • both have RGBA fields;
    • one or both have no RGB/RGBA fields.

    There is no RGB/RGBA mismatch involved, so we simply find the list of common fields and copy their contents one by one with NdConcatenateFunctor.

  • Points have different types and one of these types has RGB field, and the other has RGBA field.

    In this case we also find the list of common fields and copy their contents. In order to account for the fact that RGB and RGBA do not match we have an additional memcpy to copy the contents of one into another.

All decisions are made at compilation time, so at run time only the copy instructions are called.

This pull request also adds a unit test for copyPoint() and updates the family of copyPointCloud() functions to use it. This dramatically simplifies them.

Finally, I searched through the PCL code to identify places where the authors intended to copy data between different point types, and replaced it with copyPoint() calls. I should note that in all cases the existing code did not account for RGB/RGBA mismatch, so this update potentially fixes some not-yet-discovered bugs for certain pairs of point types.

@rbrusu
Copy link
Member

rbrusu commented Jan 24, 2014

That is an AWESOME contribution Sergey! Give me a few hours to mull it over, but you won't see any resistance from my behalf :) 👍

@taketwo
Copy link
Member Author

taketwo commented Jan 24, 2014

By the way, we can also instantiate the template for the catesian product of all point types. Do you think it makes any sense?

@sdmiller
Copy link
Contributor

This is fantastic! It simplifies so much code, and provides functionality I've found myself manually re-writing far too many times. Really solid contribution!

The logic in the code seems sound to me. As for the instantiation question, I can't think of a good reason not to for such a small (but fundamental) building block. Radu?

@jpapon
Copy link
Contributor

jpapon commented Jan 24, 2014

I always wondered why this function didn't exist - and why I was forced to do workarounds using copyPointCloud.
Great job! This is a really useful addition!

jspricke added a commit that referenced this pull request Feb 11, 2014
Add a function to copy data between points of different types
@jspricke jspricke merged commit 2d2272b into PointCloudLibrary:master Feb 11, 2014
@taketwo taketwo deleted the add-copy-point branch February 11, 2014 11:22
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.

5 participants