-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(pointcloud preprocessor): fix ring outlier filter and concatenation to use PointXYZIRC #6870
fix(pointcloud preprocessor): fix ring outlier filter and concatenation to use PointXYZIRC #6870
Conversation
Signed-off-by: a-maumau <[email protected]>
Signed-off-by: a-maumau <[email protected]>
uint16_t _data; | ||
struct | ||
{ | ||
uint8_t padding{0U}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for all reviewers, is this padding
OK to be the field name?
@xmfcx I remember you mentioned this issue in S/P WG . Do you have any comment? |
The intensity should be 1 byte and you don't need any paddings. See here: This is how it should be. Then this pr was merged because the point types didn't match the repo: But we agreed to use 1 byte once we fixed the repo. But I believe this should start getting fixed from the nebula driver, the sensing layer, then the concatenator. Even in the latest documentation page, you can see how 1 byte is enough to represent the intensity: https://autowarefoundation.github.io/autoware-documentation/main/design/autoware-architecture/sensing/data-types/point-cloud/#intensity |
Before: 4 * 4 bytes; (x, y, z, i) : (f32, f32, f32, f32) Same size, same bandwidth, perfect padding. Even if you don't need the fields, same speed. |
Signed-off-by: a-maumau <[email protected]>
ab5777b
to
83e6d64
Compare
temporarily I've made the commit fixing the intensity type mentioned by @xmfcx in the comment: changed the intensity from |
#6996 overlaps with this PR so I will close this PR. |
Description
The current implementation of RingOutlierFilter and PointCloudConcatenateDataSynchronizer does not follow the documentation of the Autoware.
This PR will change the output of RingOutlierFilter and PointCloudConcatenateDataSynchronizer from
PointXYZI
toPointXYZIRC
.Also, after concatenation, the gathered data's channel is set to 0 (channel does not have consistent meaning).
Related links
doc: https://autowarefoundation.github.io/autoware-documentation/main/design/autoware-architecture/sensing/data-types/point-cloud/#recommended-processing-pipeline
Tests performed
Before
After
Notes for reviewers
I only checked the sensing part is working, please check the other parts if it's needed.
Interface changes
N/A
Effects on system behavior
N/A
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.