[mardgez]

With downsample_all_data_ (the default), VoxelGrid<PCLPointCloud2>::applyFilter averages every field by copying field_sizes_[d] raw bytes into one float of an Eigen::VectorXf and copying them back out:

• Since [common] Add bool, std::{u}int64_t as possible point field types #4133, field_sizes_ allows 8 bytes for FLOAT64, INT64 and UINT64 fields, so such a field was written into its float and the next one, and past the end of the vector when it is the last field (for example a double timestamp after x, y, z). The copy back reads past the end in the same way. AddressSanitizer reports a heap-buffer-overflow write in applyFilter for such a cloud with the default settings.
• For the fields that do fit, the average of the raw bytes is only meaningful for FLOAT32: signed integers, 32-bit integers and doubles came out wrong.
• A field named rgb or rgba was taken as a packed 4-byte color whatever its size, so a 1-byte field of that name was read and written as 4 bytes, overwriting the fields after it.

Each field is now read as a double according to its datatype, averaged in an Eigen::VectorXd and written back as its datatype, with integers rounded to the nearest value and saturated to their range. The rgb special case applies only to 4-byte rgb/rgba fields, and packs the color as std::uint32_t, so that an alpha of 128 or more no longer overflows a signed int. Clouds of float fields and a packed rgb/rgba are averaged as before, now accumulated in double precision.

The new tests VoxelGrid.DownsampleAllDataFieldTypes (x, y, z, signed and unsigned integers and a double as the last field) and VoxelGrid.DownsampleAllDataSmallRGBField (a 1-byte rgb followed by another field) check the averaged values. Without the fix, the first stops at the write past the end under AddressSanitizer and the second gets a wrong value for the field after rgb. The whole test_filters suite passes.

[mvieth]

Thanks for the pull request! Regarding loadFieldValueAs and loadFieldValue, we already have similar functions reinterpret_and_cast and point_field_as in common/io.h and common/impl/io.hpp. Can you use those instead? And for storeFieldValue and storeFieldValueAs, I think it would be good to also put them in common/io.h and common/impl/io.hpp because they could be useful in other classes, too.

[mardgez]

Thanks for the review! Done in the new commit:

• voxel_grid.cpp now reads the fields with pcl::point_field_as<double>. For that, point_field_as gets the INT8 case it was missing (it returned 0 for INT8 fields), and reinterpret_and_cast now copies the value with memcpy instead of dereferencing a cast pointer, since the fields of a PCLPointCloud2 need not be aligned (e.g. a double right after a 1 byte field, as in a packed PCD file). The old code in VoxelGrid read them with memcpy, so this keeps that safe.
• storeFieldValueAs and storeFieldValue are now in common/impl/io.hpp, with storeFieldValue declared and documented in common/io.h next to point_field_as.
• Added tests for both in test/common/test_io.cpp.

I kept it as a separate commit so the change is easy to see; I can squash it, or rename the functions if you prefer other names.

[mvieth]

Here, a std::round would probably make sense (before casting to an integer)? For example, if we have 3 points with red values 255, 255, and 254 respectively, the best value to represent the centroid would be 255, not 254.

[mardgez]

Good point, done in b6d482e: each averaged channel (r, g, b and a) is now rounded with std::round before the cast. I also added a test, VoxelGrid.DownsampleAllDataRGBARounded, which checks your example (255, 255 and 254 give 255).

[mvieth]

Suggested change

	* \param type point field type
	* \param type point field type (see pcl::PCLPointField::PointFieldTypes)

[mardgez]

Applied in b6d482e, thanks.