Skip to content
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

Closed

Conversation

a-maumau
Copy link
Contributor

@a-maumau a-maumau commented Apr 23, 2024

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.

Signed-off-by: a-maumau <maumaumaumaumaumaumaumaumaumau@gmail.com>
Signed-off-by: a-maumau <maumaumaumaumaumaumaumaumaumau@gmail.com>
@github-actions github-actions bot added component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) component:common Common packages from the autoware-common repository. (auto-assigned) labels Apr 23, 2024
uint16_t _data;
struct
{
uint8_t padding{0U};
Copy link
Contributor Author

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?

@vividf
Copy link

vividf commented Apr 23, 2024

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.

@badai-nguyen
Copy link
Contributor

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

@xmfcx
Copy link
Contributor

xmfcx commented Apr 25, 2024

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:

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
Copy link
Contributor

xmfcx commented Apr 25, 2024

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.

Signed-off-by: a-maumau <maumaumaumaumaumaumaumaumaumau@gmail.com>
@a-maumau a-maumau force-pushed the fix/pointcloud_prepro_types branch from ab5777b to 83e6d64 Compare May 7, 2024 08:37
@a-maumau
Copy link
Contributor Author

a-maumau commented May 7, 2024

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
Copy link
Contributor Author

#6996 overlaps with this PR so I will close this PR.

@a-maumau a-maumau closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common Common packages from the autoware-common repository. (auto-assigned) component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants