Add writeBinary to ostream for PCDWriter

[ptrckschcknbch]

Add method writeBinary to serialize point clouds in memory uncompressed.

Solves #5539

[mvieth]

Hi, thanks for the PR. I think, in the interest of keeping the existing writeBinary function as fast as possible, it would be better if the function with the filename does not call the function with the ostream, even if we can save a few lines by doing so. Luckily, the function with the ostream is quite short anyway since it does not need all the error checking and platform specific behaviour.
Regarding testing: you could adapt the test LZFInMem in test/io/test_io.cpp.
What do you think?

[ptrckschcknbch]

Hi, thanks for the PR. I think, in the interest of keeping the existing writeBinary function as fast as possible, it would be better if the function with the filename does not call the function with the ostream, even if we can save a few lines by doing so. Luckily, the function with the ostream is quite short anyway since it does not need all the error checking and platform specific behaviour. Regarding testing: you could adapt the test LZFInMem in test/io/test_io.cpp. What do you think?

Yes, I see your point and adapted the code accordingly.

Regarding testing: I copied and adapted the test LZFInMem. However, this is currently failing. But I'm not sure if the failure is related to my changes. As far as I could see, the generateHeaderBinary function, which is called in the new function (and also in the existing writeBinary function), seems to generate a header, which is not accepted by the readHeader function used in the test. Apparently the logic to create fake fields in generateHeaderBinary expects cloud.fields to be sorted by their offset. In my case, field rgb comes before the normal vector, but has a bigger offset, which leads to a negative COUNT value in the header.

I guess I need to dig a bit deeper here to understand what's going on. Maybe you have some ideas?

[ptrckschcknbch]

Hi, thanks for the PR. I think, in the interest of keeping the existing writeBinary function as fast as possible, it would be better if the function with the filename does not call the function with the ostream, even if we can save a few lines by doing so. Luckily, the function with the ostream is quite short anyway since it does not need all the error checking and platform specific behaviour. Regarding testing: you could adapt the test LZFInMem in test/io/test_io.cpp. What do you think?

Yes, I see your point and adapted the code accordingly.

Regarding testing: I copied and adapted the test LZFInMem. However, this is currently failing. But I'm not sure if the failure is related to my changes. As far as I could see, the generateHeaderBinary function, which is called in the new function (and also in the existing writeBinary function), seems to generate a header, which is not accepted by the readHeader function used in the test. Apparently the logic to create fake fields in generateHeaderBinary expects cloud.fields to be sorted by their offset. In my case, field rgb comes before the normal vector, but has a bigger offset, which leads to a negative COUNT value in the header.

I guess I need to dig a bit deeper here to understand what's going on. Maybe you have some ideas?

I guess following change in common/include/pcl/impl/point_types.hpp might fix this issue. But I have now clue, what other effects this change might have.

Maybe you can have a look if I'm on the right track here? You probably don't want to have such changes in this PR, but rather separated?

[mvieth]

Interesting, thanks for investigating. To my knowledge, this was never noticed before. Intuitively, I would agree that the fields should be ordered to match the order in the memory, but I would like to think about whether correcting it for the three point types can break something.
I would propose to sort the fields by their offset at the start of generateHeaderBinary (maybe copy cloud.fields, then use https://en.cppreference.com/w/cpp/algorithm/sort with a custom comparison lambda), and put all changes in point_types.hpp in a separate PR. Sorting makes sense anyway because people might store clouds with custom point types which do not follow the necessary field ordering.

[mvieth]

Great, thank you!