autoware.universe
autoware.universe copied to clipboard
fix(pointcloud preprocessor): fix ring outlier filter and concatenation to use PointXYZIRC
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
to PointXYZIRC
.
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
...
fields:
- name: x
offset: 0
datatype: 7
count: 1
- name: y
offset: 4
datatype: 7
count: 1
- name: z
offset: 8
datatype: 7
count: 1
- name: intensity
offset: 12
datatype: 7
count: 1
is_bigendian: false
point_step: 16
...
After
...
fields:
- name: x
offset: 0
datatype: 7
count: 1
- name: y
offset: 4
datatype: 7
count: 1
- name: z
offset: 8
datatype: 7
count: 1
- name: intensity
offset: 12
datatype: 7
count: 1
- name: padding
offset: 16
datatype: 2
count: 1
- name: return_type
offset: 17
datatype: 2
count: 1
- name: channel
offset: 18
datatype: 4
count: 1
is_bigendian: false
point_step: 20
...
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.
- [x] I've confirmed the contribution guidelines.
- [x] The PR follows the pull request guidelines.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
- [ ] The PR follows the pull request guidelines.
- [ ] The PR has been properly tested.
- [ ] The PR has been reviewed by the code owners.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
- [ ] There are no open discussions or they are tracked via tickets.
- [ ] The PR is ready for merge.
After all checkboxes are checked, anyone who has write access can merge the PR.
Thanks for the PR! However, adding additional return types and channels probably will increase the latency. If the nodes in perception don't need this information, then we shouldn't change it. Probably it is better to change the documentation.
@xmfcx I remember you mentioned this issue in S/P WG . Do you have any comment? I think you prososed idea to keep the same size of pointcloud, did you?
The intensity should be 1 byte and you don't need any paddings.
See here:
https://github.com/autowarefoundation/autoware-documentation/blob/01e6b75fe73b1831aba61d52d165c4f67eedd7d7/docs/design/autoware-architecture/sensing/data-types/point-cloud.md#point-cloud-fields
This is how it should be.
Then this pr was merged because the point types didn't match the repo:
- https://github.com/autowarefoundation/autoware-documentation/pull/478
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) After: 3 * 4 bytes + 2 * 1 byte + 1 * 2 byte (x, y, z, i, r, c) : (f32, f32, f32, ui8, ui8, ui16)
Same size, same bandwidth, perfect padding. Even if you don't need the fields, same speed.
temporarily I've made the commit fixing the intensity type mentioned by @xmfcx in the comment: changed the intensity from float
to uint8
.
Since the code assumed the intensity of input to be float
, it requires updating the casts in the ring_outlier_filter_nodelet.cpp after modifying other parts (e.g., the nebula driver).
https://github.com/autowarefoundation/autoware.universe/pull/6996 overlaps with this PR so I will close this PR.