autoware.universe icon indicating copy to clipboard operation
autoware.universe copied to clipboard

fix(pointcloud preprocessor): fix ring outlier filter and concatenation to use PointXYZIRC

Open a-maumau opened this issue 10 months ago • 5 comments

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.

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.

a-maumau avatar Apr 23 '24 08:04 a-maumau

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.

vividf avatar Apr 23 '24 09:04 vividf

@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?

badai-nguyen avatar Apr 25 '24 00:04 badai-nguyen

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

xmfcx avatar Apr 25 '24 00:04 xmfcx

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.

xmfcx avatar Apr 25 '24 00:04 xmfcx

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).

a-maumau avatar May 07 '24 09:05 a-maumau

https://github.com/autowarefoundation/autoware.universe/pull/6996 overlaps with this PR so I will close this PR.

a-maumau avatar May 17 '24 05:05 a-maumau