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

feat(pointcloud_preprocessor): add data layout check utils #3306

Merged
merged 3 commits into from
Apr 14, 2023

Conversation

VRichardJP
Copy link
Contributor

@VRichardJP VRichardJP commented Apr 7, 2023

Description

Several filters somewhat assume the input data layout is compatible with PointXYZI or PointXYZIRADRT to do faster data copy. However these assumptions are never verified, leading to hard-to-detect bugs such as #2618

I introduce 2 utils functions is_data_layout_compatible_with_PointXYZI and is_data_layout_compatible_with_PointXYZIRADRT to make the data layout verification task easy.

For example, the current ring_outlier_filter assumes the input data layout is the same as PointXYZIRADRT. As pointed out here this is not the case. Such issue would have been easily detected with a simple check such as:

void RingOutlierFilterComponent::filter(
  const PointCloud2ConstPtr & input, [[maybe_unused]] const IndicesPtr & indices,
  PointCloud2 & output)
{
  if (!utils::is_data_layout_compatible_with_PointXYZI(*input)) {
    RCLCPP_ERROR(get_logger(), "Invalid input data layout. Output may be incorrect");
  }

Note: this PR only introduce the util functions. More PR are necessary to use them in the relevant filters.

Tests performed

With the snippet above, issue #2618 would have been detected immediately:

[component_container_mt-1] [ERROR 1680848531.443909204] [sensing.lidar.top.ring_outlier_filter]: Invalid input data layout. Output may be incorrect (filter() at /home/sig/autoware/src/universe/autoware.universe/sensing/pointcloud_preprocessor/src/outlier_filter/ring_outlier_filter_nodelet.cpp:53)

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.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@VRichardJP VRichardJP requested review from amc-nu, miursh, yukkysaito and a team as code owners April 7, 2023 06:27
@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Apr 7, 2023
@VRichardJP VRichardJP added the type:new-feature New functionalities or additions, feature requests. label Apr 7, 2023
Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (93e040a) 12.33% compared to head (45d00d3) 12.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3306      +/-   ##
==========================================
- Coverage   12.33%   12.33%   -0.01%     
==========================================
  Files        1381     1381              
  Lines       96692    96750      +58     
  Branches    27978    27978              
==========================================
  Hits        11930    11930              
- Misses      72094    72152      +58     
  Partials    12668    12668              
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 12.33% <ø> (ø) Carriedforward from 93e040a

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
.../pointcloud_preprocessor/src/utility/utilities.cpp 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

VRichardJP and others added 3 commits April 11, 2023 08:11
When input cloud data layout is compatible filters can copy data faster.

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
cleaning

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
@miursh miursh merged commit 1d6d9ad into autowarefoundation:main Apr 14, 2023
@miursh
Copy link
Contributor

miursh commented Apr 14, 2023

@VRichardJP I have just merged some of your PRs. If there is any problem, please let me know.

kyoichi-sugahara pushed a commit to kyoichi-sugahara/autoware.universe that referenced this pull request Apr 14, 2023
…oundation#3306)

* feat(pointcloud_preprocessor): add data layout check utils

When input cloud data layout is compatible filters can copy data faster.

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>

* style(pre-commit): autofix

* add "name" field check

cleaning

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>

---------

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
kyoichi-sugahara added a commit that referenced this pull request Apr 15, 2023
* add planning_interface_test_manager class

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add counter function

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add interface test for motion_velocity_smoother

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add param in default_motion_velocity_smoother,param.yaml

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* successfully launch target node

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* successfully build the test

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add test for test_motion_velocity_smoother

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add abnormal trajectory test

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* delete unnecessary part

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* style(pre-commit): autofix

* style(pre-commit): autofix

* run pre-commit

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add declaration

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* Refactor callback functions for standardization

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refacotoring

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactored

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* Refactor functions into a single template function

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* Refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* style(pre-commit): autofix

* refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* relete unnecessary definition

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* Revert "delete unnecessary definition"

This reverts commit 6cd13f8.

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* delete unnecessary definition

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* delete unnecessary definition

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* delete module dependent part

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* delete unnecessary arg

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* apply motion_velocity_smoother change

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add interface test for obstacle stop planner

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* run pre-commit

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add test fot obstacle_cruise_planner

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add test fot planning_vaildator

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add interface test for freespace_planner_node

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add interface test for obstacle_stop_planner with slow down

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add part of interface test for freespace

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* change package name

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* apply change of package name

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* fix build error

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp commit for debug

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add planning interface test manager for scenario selector

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update parameter

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp for behavior_path_planner

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add param file

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add test free space planner module

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update map

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add test for behavior path planner

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* pass freespace test

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* pass freespace test

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update map

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update map detection area

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add no stopping area

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* for print debug

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update param

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add param file

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor(behavior_path_planner): remove unnecessary functions (#3271)

* refactor(behavior_path_planner): remove unnecessary functions

Signed-off-by: yutaka <purewater0901@gmail.com>

* update

Signed-off-by: yutaka <purewater0901@gmail.com>

---------

Signed-off-by: yutaka <purewater0901@gmail.com>

* Revert "temp"

This reverts commit b82805e.

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* suppress build error (-Werror=pedantic)

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add interface test for behavior_velocity_planner

* build success

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add param

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* pass test

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* pass test

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* build success

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update param

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* delete param file

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* Resolve differences outside of parameter files

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update test manager

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* update test manager

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* upload cloud map

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* fix typo

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor utils

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor utils

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor test_manager

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* temp

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* modify test for obstacle cruise planner

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* modify test for obstacle_stop_planner

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* revert parameter change

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add obstacle_avoidance_planner

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* Revert "add obstacle_avoidance_planner"

This reverts commit efb5d50.

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add planning_validator

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* add rosidl_default_generators in CMake

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* Revert "add rosidl_default_generators in CMake"

This reverts commit 7f87b0a.

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* delete unnecessary files

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

* refactor(behavior_velocity_planner::intersection): organize param intersection (#3409)

* reorganize intersection param for readability

Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>

* reorganized PlannerParam

Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>

---------

Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>

* refactor(lane_following): remove duplicated reference path generation process (#3398)

* feat(utils): add new function

Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>

* refactor(behavior_path_planner): use utils function

Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>

---------

Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>

* fix(planning_test_utils): fix build error (#3410)

Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>

* fix(pointcloud_preprocessor): fix broken logic (#3122)

`if (a && b, c)` is the same as `if (c)` not `if (a && b && c)` !!

Signed-off-by: Vincent Richard <vincent.francois.richard@gmail.com>

* refactor(crop_box_filter): remove always true condition (#3245)

* refactor(crop_box_filter): remove always true condition

tf_input_frame_ can't be empty in crop box filter.

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>

* style(pre-commit): autofix

---------

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* feat(pointcloud_preprocessor): add data layout check utils (#3306)

* feat(pointcloud_preprocessor): add data layout check utils

When input cloud data layout is compatible filters can copy data faster.

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>

* style(pre-commit): autofix

* add "name" field check

cleaning

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>

---------

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* update cmake minimum version

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>

---------

Signed-off-by: kyoichi-sugahara <kyoichi.sugahara@tier4.jp>
Signed-off-by: yutaka <purewater0901@gmail.com>
Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>
Signed-off-by: Vincent Richard <vincent.francois.richard@gmail.com>
Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yutaka Shimizu <43805014+purewater0901@users.noreply.github.com>
Co-authored-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
Co-authored-by: Satoshi OTA <44889564+satoshi-ota@users.noreply.github.com>
Co-authored-by: Vincent Richard <vincent.francois.richard@gmail.com>
@VRichardJP VRichardJP deleted the data_layout_check branch April 25, 2023 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) type:new-feature New functionalities or additions, feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants