[io] origin and orientation precision when writing to file can be too low

[flabrosse]

Describe the bug

The precision in PCD (and PLY) files of the origin and orientation is not the one provided to PCDWriter::writeASCII (and PLY equivalent).

Context

I get the origin of my sensor from RTK GPS and therefore get it at high precision. These are converted to UTM, leading to large numbers (see below).

Expected behavior

When saving a PCD file with PCDWriter::writeASCII() or writeBinary() (although in this case it is not possible to specify a precision), I would expect the precision in the header (for origin and orientation) to be the one that is specified.

Current Behavior

Currently I get the default (6 on my computer):

VIEWPOINT 492818 5.52752e+06 -0.483211 1 -0 -0 -0.000667558

when provided with (and this is only a print on a screen so also with limited but higher precision):

- Translation: [492818.039, 5527515.847, -0.483]

The default precision is not high enough for my application.

To Reproduce

Just save a PCD (or PLY) file with an origin that is a large number (more than your default precision).

Your Environment (please complete the following information):

• OS: Ubuntu 20.04
• Compiler: GCC 9.3.0
• PCL Version 1.10 from Ubuntu package

Possible Solution

The problem is due to the fact that the origin is written in a string using generateHeaderASCII() that does not use the precision.

The functions that generate the header either need to use the precision from the functions that call them or write directly in the file that is used by writeASCII() (and similar) rather than in a string.

[kunaltyagi]

Did you try using double (float64) instead of float?

[flabrosse]

I don't have a choice on that. The writeASCII() functions want the location and orientation specified as Eigen::Vector4f and Eigen::Quaternionf respectively.

I am not sure the default precision for float and double of extraction operator is different though.

[mvieth]

I see two options (mainly looking at PCDWriter, but should be similar for the PLY equivalent):

1. Always write the viewpoint with high precision. Since it is only 7 numbers and trailing zeros are not printed (e.g. zero would just be 0 and one would just be 1), this would have almost no effect on file size. The question is how many digits should be printed, Wikipedia says that 9 would be sufficient. This would be quite easy to implement.
2. Add a function to PCDWriter that makes it possible to specify the desired precision for the viewpoint (maybe something like setPrecisionViewpoint). I think simply using the precision that was given to writeASCII is not a good idea because one might want to print the viewpoint with a higher precision than the points, and (as you also noted) this is not a solution for writeBinary and writeBinaryCompressed anyway. This would require the user to call this function, whereas in option 1 the high precision would be used automatically.

You should also keep in mind that float is not precise enough to always store all three digits after the decimal point in your example, see also here.

[flabrosse]

Very good points about float vs double. I had a similar issue a long time ago with gpsd and RTK GPS units. I guess Vector4d and Quaterniond should be used.

As to how to allow setting the precision, I was going to implement using the one given to writeASCII(), adding an additional parameter with a sensible default to the functions that write the header. However adding a setPrecisionViewpoint() function is less disruptive to existing code. Obviously, not being involved in the development of PCL, this is not for me to say.

Thanks for looking into it.

[flabrosse]

I have just realised that pcl_transform_point_cloud probably also does the transformation in floats rather than doubles given the banding effect I get...

[mvieth]

PCL is designed for float in many parts because it is faster than double and the precision is usually sufficient.
Vector4f and Quaternionf can't be easily replaced with Vector4d and Quaterniond, because that might break code both inside PCL and for users.
For your case: assuming that all your points are roughly in the same region, you could simply de-mean your cloud (subtract an offset like (492818, 5527515, 0) from every point). Then float should be enough. Without de-meaning, the first few digits would be the same for all points, which essentially wastes some bits in the mantissa.

[flabrosse]

I am actually explicitly translating the pointcloud to geo-reference it. I tried too use the viewpoint so that I would only have the large number in one location (essentially de-meaning it), which would be OK if I could have enough precision on the viewpoint. Also, the viewpoint is only partially saved in PLY (only the location, not the orientation) and as far as I could see the viewpoint was being lost when converting to other formats. And I need to convert to other formats because the software after in the pipeline does not, I am told, understand PCD.

PS My own experience of double vs float speed was that doubles were much faster, but I have not done such comparison, or even looked at this, in a very long time, so I'm happy to take your word for it (and indeed google seems to agree with you ;-) ).

[mvieth]

The orientation of the viewpoint seems to be stored under x_axisx and so on ([xyz]_axis[xyz] as a regex) as a rotation matrix.
Do you want to store the data as a binary PLY file or an ASCII PLY file?
To store the viewpoint with double precision, the header would have to contain lines like property double view_px instead of the current property float view_px, but of course it would also be necessary to use Vector4d and Quaterniond instead of the float variants, and I don't see a nice and easy way to replace that in the parameters of the functions without breaking many things ...

[flabrosse]

What about having 2 versions of the functions, with the f and d variants, one calling the other. Surely copying a Vector4f into a Vector4d (and the same with the quaternion) is not that expensive.

[kunaltyagi]

It's not, but a quick glance shows that a lot of code would have to be changed. You're welcome to start a PR for the same, but it wouldn't be a short and easy journey getting this feature merged.