writers.pcd upgrades

[loganbyers]

I am currently making improvements to writers.pcd under the branch pcdwriter_upgrade. The motivation is that the current writers.pcd only outputs a custom XYZ-Intensity-RGBA point type which is not a standard PointT within PCL. This seems to be used because those dimensions are common with regards to visualizing a cloud.

The pcd writer is not very robust, as many of the options available in other writers are not available for the pcd writer. There is currently no option to, for instance, output just an XYZ point, or specify a set of point dimensions to keep. Before pursuing this further, I want to get suggestions on what options would be desirable. I am also interested in getting input on whether to break backwards compatibility for the sake of logical options. The additions I have made so far retain backwards compatibility.

Improvements

• writers.pcd.xyz (bool) -- write only XYZ dimensions
• writers.pcd.binary (bool) -- write binary non-compressed file
• writers.pcd.compression (string) -- specify compression level
• writers.pcd.offset_x, offset_y, offsety_z (double) -- offsets to subtract from each point
• writers.pcd.subtract_minimum (bool) -- subtract the minimum XYZ dimension
• writers.pcd.scale_x, scale_y, scale_z (double) -- scale to be divided from XYZ dimensions
• writers.pcd.output_dims (string) -- dimensions to output

These changes would bring the pcd writer much more in line with the other writers. I don't want to go too far with any of these changes without first consulting the main contributors to PDAL. So I will ask some questions now.

Data compression

In regards to the point data compression (for which PCL has ascii, binary, and binary-compressed), is it more desirable to have a single option that takes a string to specify the compression or two compression flags (writers.pcd.binary, writers.pcd.compression) where writers.pcd.compression supersedes the binary option? writers.pgpointcloud.compression uses a single option with a string to specify its compression type. At current, writers.pcd.compression is a flag, so I don't want to break backwards compatibility.

output_dims

Across the current writers, the option output_dims is used to specify arbitrary dimensions to write. I added the option writers.pcd.xyz to output only the spatial dimensions because that is what I needed at the time. This is not a standard option from what I can tell. I know that building a database of common dimensions is a goal in the future, so I don't know if it is worth pushing too hard on this right now. I also don't need this functionality, so I am not too motivated to dig in and work on it.

Any comments would be appreciated, thanks.

[hobu]

I am also interested in getting input on whether to break backwards compatibility for the sake of logical options.

@chambbj will have to chime in here, but I think the current writers.pcd was a stop-gap and not in wide general use as far as we know. I think you have the go-ahead to rip it up and make it right.

Any comments would be appreciated, thanks.

Awesome!

[abellgithub]

Writers.pgpointcloud.compression uses a single option with a string to specify its compression type. At current, writers.pcd.compression is a flag, so I don't want to break backwards compatibility.

I prefer the compression type as a string. It tends to be easier to code to and is more flexible if changes are necessary in the future. In other drivers, we've accepted the flag-based option for backward compatibility, though I'm not sure it's a big deal.

Across the current writers, the option output_dims is used to specify arbitrary dimensions to write. I added the option writers.pcd.xyz to output only the spatial dimensions because that is what I needed at the time.

I prefer the use of output_dims because it's standard and flexible. If you don't want to make the change but you're willing to have it made, you can commit to a branch and I'd be happy to do the mod.

[chambbj]

At current, writers.pcd.compression is a flag, so I don't want to break backwards compatibility.

I can't think of anything that would be adversely affected by this change -- maybe some tutorials and other documentation. I probably wrote it with just the flag because I couldn't think of a time when I would want it in binary, but not compressed. If someone else needs it, a string would be fine.

Across the current writers, the option output_dims is used to specify arbitrary dimensions to write.

I think output_dims is the way to go.

I like the option of allowing the user to specify scales and offsets. The only thing I'd note is that while most writers probably just don't touch offsets and scales if they are not specified, the PCD writer should default to it's current behavior of at least subtracting the XYZ mins when converting to PCD.

[hobu]

@loganbyers Are you ready for us to include this in a PDAL 1.3.0 release? We were planning to issue a beta this week.

[loganbyers]

@hobu The must up-to-date version on my fork should be good to go.

I never got around to implementing an option for pcd.output_dims, but kept the pcd.xyz option. I think that is the only thing missing or incomplete.

Default behavior is to subtract min XYZ, do no scaling, and output XYZIRGBA point type. These are are backwards compatible.

• pcd.compression is now a string instead of a bool taking "ascii", "binary", "compressed"
• pcd.xyz outputs pcl::PointXYZ if true
• pcd.subtract_minimum won't subtract min XYZ if false
• pcd.offset_x/y/z is applied after minimum subtraction (if any)
• pcd.scale_x/y/z scales dimension by inverse of value after offsets applied; so pcd.scale_*=10 makes 150 [PDAL] -> 15 [PCD]

[hobu]

@loganbyers Can you make a PR against current master and we'll happily merge it?

Thanks!

[hobu]

merged in #1286