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

VTK io fixes #1279

Merged
merged 2 commits into from
Jul 30, 2015
Merged

VTK io fixes #1279

merged 2 commits into from
Jul 30, 2015

Conversation

VictorLamoine
Copy link
Contributor

  • Return value is now a boolean instead of the number of faces in the model
  • Enable user to choose between ASCII/binary formats

@taketwo
Copy link
Member

taketwo commented Jul 22, 2015

Why do you think returning bool is better? int is anyway implicitly convertible to boolean, so I don't see any benefits in changing already existing interface. There might be people relying on it.

But nevertheless, your are correct that the output value of poly_writer->Write () should be taken in the account.

@VictorLamoine
Copy link
Contributor Author

In the case the writer didn't write all the faces (eg: full file system) the number of faces (different from 0) returned will be converted to a true boolean while the writing operation was not successful.

It's safer to check poly_writer->Write ()

There might be people relying on it.

I doubt people rely on something un-documented.

int error_code;
if (binary_format)
{
pcl::PointCloud<pcl::PointXYZRGB> cloud;
Copy link
Member

Choose a reason for hiding this comment

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

Hm, seems like my comment on this line disappeared somehow. Repeating: why this specific point type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been lost here.

Why not? I don't know how I should handle this kind of case 😅

Copy link
Member

Choose a reason for hiding this comment

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

What if the mesh cloud contained different channels than XYZRGBA?

But anyway, I'm not sure why you make this cast for the "binary" case. The savePCDFile() function allows to save blob point clouds in binary mode as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, I missed this one. Fixed

@taketwo
Copy link
Member

taketwo commented Jul 25, 2015

Ok, agree.

return (0);
return (static_cast<int> (mesh.cloud.width * mesh.cloud.height));
return (false);
return (true);
Copy link
Member

Choose a reason for hiding this comment

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

What about collapsing these 15 lines of code (85-99) into something like:

return (pcl::io::savePCDFile (file_name, mesh.cloud, Eigen::Vector4f::Zero (),
                              Eigen::Quaternionf::Identity (), binary_format) == 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes :)

taketwo added a commit that referenced this pull request Jul 30, 2015
@taketwo taketwo merged commit 1dce52b into PointCloudLibrary:master Jul 30, 2015
@VictorLamoine VictorLamoine deleted the vtk_io_fixes branch July 30, 2015 14:11
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.

3 participants