Fixes PLY parsing of char and uchar scalars

[SergioRAgostinho]

WIP of #1381

int8 and uint8 types were being evaluated as char and unsigned char, their actual original type, which breaks their lexical_cast evaluation as integers. Functionally-wise, this PR will make color parsing work.

[SergioRAgostinho]

Some feedback requested here. Am I missing something?

To clear a little bit the purpose of this PR, I was going through the items mentioned in #1381, and noticed I wasn't able to load color information from PLY files if I used pcl::io::loadPLYFile().

This was my test code

#include <pcl/common/common.h>
#include <pcl/console/parse.h>
#include <pcl/point_cloud.h>
#include <pcl/io/ply_io.h>
#include <pcl/visualization/pcl_visualizer.h>

int main(int argc, char* argv[])
{
  std::vector<int> ply_args = pcl::console::parse_file_extension_argument (argc, argv, ".ply");
  if (ply_args.size() != 1)
  {
    PCL_ERROR("Usage: %s input.ply\n", argv[0]);
    return -1;
  }

  pcl::PointCloud<pcl::PointXYZRGB>::Ptr cloud (new pcl::PointCloud<pcl::PointXYZRGB>);
  if (pcl::io::loadPLYFile(argv[ply_args[0]], *cloud) != 0)
  {
    PCL_ERROR("Could not load %s\n", argv[ply_args[0]]);
    return -1;
  }

  pcl::visualization::PCLVisualizer viewer;
  viewer.addPointCloud(cloud);
  viewer.spin();
  return 0;
}

and this was my test ply file

ply
format ascii 1.0
element vertex 4
property float x
property float y
property float z
property uchar red
property uchar green
property uchar blue
element face 0
property list uchar int vertex_indices
end_header
4.23607 0 1.61803 255 0 0 
2.61803 2.61803 2.61803 0 255 0 
0 1.61803 4.23607 0 0 255
0 -1.61803 4.23607 255 255 255

[taketwo]

So for your example file the old code would do:

value = boost::lexical_cast<uint8> (value_s);

and the new one:

value = static_cast<uint8> (boost::lexical_cast<uint16> (value_s));

Do I understand right that the former does not work as expected? Could you please elaborate on what happens exactly?

[SergioRAgostinho]

For my example file, the old code (in my platform) would evaluate scalar_type as unsigned char because of typedef unsigned char uint8_t;.

value = boost::lexical_cast<unsigned char> (value_s);

and value would always be 0 :/

[taketwo]

Aha, okay. Apparently, whenever we do lexical cast from a multi-digit string to an (unsigned) char, a bad_lexical_cast exception is thrown and we set value to quiet NaN, which happens to be 0 in this case.

👍 for the patch. Would you mind to change the commit message to explain the problem it solves in greater detail? (To reduce confusion for posterity.)
Also would be great to add a check or two here, but that's up to you.

[SergioRAgostinho]

Thanks for trying it out. I'll probably amend the commit description with this small summary you just wrote on your comment.

Regarding the test, don't merge this PR right yet, I'll write the test as well. I already gave a look into that file and test_io.cpp which has the PLYReaderWriter test and I was wondering if it would not be preferable to gather all these PLY specific tests in a new test_ply.cpp file.

I also found very strange the absence of PLY files in the test folder. Going through the source I realised that most code is actually saving and loading PLY files on-the-fly for each test. Which makes it more challenging to understand if the issue is actually on the reader/load or the writer/save. We should at least make sure the reader is working reliably before using it to evaluate the outputs of the writer.

[taketwo]

Yep, you are right. Our coverage is very far from 100% and does not always do tests appropriately.

[SergioRAgostinho]

@taketwo please take a look at the test if you can and once that's done I'll squash the commits and amend the message.

[taketwo]

I think the actual name of the file and the path you specified in CMakeLists do not match (ply vs. vtk). In fact, the file is so small and simple, why not embedding it directly in the test (e.g. like it is done here)?

Also I noticed that the test is not built/executed on Travis, likely because of this guard. Looking at the unit tests, it's clear that OpenNI is not required. So I think we should drop this condition.

[SergioRAgostinho]

Done 👍. Added the fixture for the teardown functionality (in case an assertion fails). I'm not really fond of dumping things in the HDD and not cleaning up afterwards :/

[SergioRAgostinho]

Apparently it blew up in Travis -_-