Intensities from Lidar PointCloud2 topic into the generated PCD #1762
Conversation
|
Hi, thanks for the pull request. You need to It takes 1 minute. The DCO check shows you what you need to do: https://github.com/cartographer-project/cartographer/pull/1762/checks?check_run_id=1253819084 |
ikhann
force-pushed
the
master
branch
2 times, most recently
from
e5bf488 to
79ac213
Compare
Signed-off-by: Daniil Khaninaev <[email protected]>
ikhann
changed the title
| << "SIZE 4 4 4" << color_header_size << "\n" | ||
| << "TYPE F F F" << color_header_type << "\n" | ||
| << "COUNT 1 1 1" << color_header_count << "\n" | ||
| << "FIELDS x y z" << intensity_header_field << color_header_field <<"\n" |
There was a problem hiding this comment.
Any reason why the ordering between color and intensity is swapped here compared to above for arguments? Some consistency would be nice.
There was a problem hiding this comment.
You mean not important for correctness, but this is setting the bar too low. Please have a look at https://google.github.io/styleguide/cppguide.html#Goals. Inconsistencies should be there for a reason, not consistency. If you aim for readable code everyone who later reads the code, including yourself, has an easier time.
|
|
||
| void WriteBinaryPcdIntensity(const float intensity, | ||
| FileWriter* const file_writer) { | ||
| CHECK(file_writer->Write(reinterpret_cast<const char*>(&intensity), |
There was a problem hiding this comment.
This is undefined behavior. See https://en.cppreference.com/w/cpp/language/reinterpret_cast. Use memcpy as above?
nit: You probably want to #include <cstring> for this and use std::memcpy. Similar for the existing code.
There was a problem hiding this comment.
Thanks. These comments were implemented in the commit below.
There was a problem hiding this comment.
Did you see my nit? It is good practice to include what you use (IWYU) which here means: #include <cstring>. Also, in C++ memcpy is in the std namespace.
1. Added the keyword const to PCD headers. 2. Changed writing intensity from reinterpret_cast to memcpy due undefined behavior Signed-off-by: Daniil Khaninaev <[email protected]>
| } | ||
|
|
||
| void WriteBinaryPcdIntensity(const float intensity, | ||
| FileWriter* const file_writer) { |
| WriteBinaryPcdPointColor(ToUint8Color(batch->colors[i]), | ||
| file_writer_.get()); | ||
| } | ||
| if (has_intensities_) { |
There was a problem hiding this comment.
Isn't this wrong? Which would be another argument for consistency. In case of both colors and intensities you declare that intensities come first, but here you right them in the wrong order. Or do I misread this?
|
@ikhann friendly ping |
4 participants
✸ Add intensity field from the Lidar topic into the generated PCD file.
If you have topic messages with intensities from the lidar measurements, it will be added when you generate a .pcd file.
For some reason, this functionality was implemented only when generating a .ply file.