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 auto type casting in io #3312

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DaniilSNikulin
Copy link
Contributor

On PCL all vertices and normals exist only in float32 format. However, a lot of libraries and tools use float64 format. In original version, PCL don't recognize fields if types are not equals.

This commit add auto type casting in conversion from pcl::PCLPointCloud2 to pcl::PointCloud. This is allow read files, which are use float64 format.

To reproduce test case, was written simple unit test.

   On PCL all vertices and normals exist only in float32 format. However,
   a lot of libraries and tools use float64 format. In original version,
   PCL don't recognize fields if types are not equals.

   This commit add auto type casting in conversion from
   pcl::PCLPointCloud2 to pcl::PointCloud<PointT>. This is allow read
   files, which are use float64 format.

   To reproduce test case, was written simple unit test.
@taketwo
Copy link
Member

taketwo commented Aug 29, 2019

Hey, thanks for the effort, this is a very interesting contribution. I personally was hit by this limitation recently when I tried to load a PLY file with f64 fields.

I'm very much in favor of merging this. However, since the implementation is not absolutely trivial, I'd like to have a closer look. I think it's important to make sure that we are not introducing bugs (obviously) and any noticeable time penalty (these conversion functions are used a lot in ROS code, so we will be affecting loooots of people). Unfortunately I don't have enough time to give this PR attention it deserves right now, but will try as soon as possible.

@DaniilSNikulin
Copy link
Contributor Author

DaniilSNikulin commented Oct 31, 2019

remark from 3337 issue:

During concatenation, a check of field datatype should be performed

Done

I am afraid this declaration from 3337 are not fully correct:

Currenty, PCLPointCloud2::concatenate assumed same name implies same datatype. WIth the PR #3312 this will no longer be true.

Master version of PCL already successfully read float64 fields into PCLPointCloud2, current PR just allow type casting to PointT in function fromPCLPointCloud2 after data was already in PCLPointCloud2. Also PCLPointCloud2 could be obtained from ROS message with any fields type.

This means that all functions in pcl with same hypotheses potentially have errors.

@stale
Copy link

stale bot commented Feb 21, 2020

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants