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

refactor(autoware_pointcloud_preprocessor): rework filter parameters #8469

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

vividf
Copy link
Contributor

@vividf vividf commented Aug 14, 2024

Description

This PR includes the following changes

  1. Remove the default parameters for the filter class.
  2. Add parameter, and parameters schema for the class.

A big thank you to @Ariiees for the PRs (#8297 and #7422), where we worked together on the entire pointcloud preprocessor parameters, launch file, and schema.

Related links

Parent Issue:

  • Link

How was this PR tested?

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) labels Aug 14, 2024
Copy link

github-actions bot commented Aug 14, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@vividf vividf marked this pull request as ready for review August 14, 2024 15:49
@vividf vividf self-assigned this Aug 14, 2024
@vividf vividf added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Aug 14, 2024
Signed-off-by: vividf <[email protected]>
@vividf vividf added the tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) label Aug 15, 2024
@vividf
Copy link
Contributor Author

vividf commented Aug 29, 2024

Note that this PR needs to be merged together with PR autowarefoundation/sample_sensor_kit_launch#104 in the common_sensor _launch

tf_output_frame_ = static_cast<std::string>(declare_parameter("output_frame", ""));
tf_input_frame_ = declare_parameter<std::string>("input_frame");
tf_output_frame_ = declare_parameter<std::string>("output_frame");
max_queue_size_ = static_cast<size_t>(declare_parameter<int64_t>("max_queue_size"));
has_static_tf_only_ = static_cast<bool>(declare_parameter("has_static_tf_only", false));
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be using the old way of declaring parameters

Copy link
Contributor

Choose a reason for hiding this comment

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

*note: I am referring to has_static_tf_only_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amadeuszsz will remove the default parameters for has_static_tf_only_ for all of the nodes after schema-related PRs are merged :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@vividf
Sure, but if that does not happen in the near future, please send a PR. It will look weird otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

(I will be mark as approved but leave the conversation open for future reference)

Copy link
Contributor

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

Left some review comments

@vividf vividf requested a review from knzo25 September 30, 2024 06:56
Copy link
Contributor

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

LGTM

@SakodaShintaro
Copy link
Contributor

Some filters are launched in tier4_localization_launch.

<load_composable_node target="$(var localization_pointcloud_container_name)">
<composable_node pkg="autoware_pointcloud_preprocessor" plugin="autoware::pointcloud_preprocessor::CropBoxFilterComponent" name="crop_box_filter_measurement_range">
<param from="$(var ndt_scan_matcher/pointcloud_preprocessor/crop_box_filter_measurement_range_param_path)"/>
<remap from="input" to="$(var input_pointcloud)"/>
<remap from="output" to="measurement_range/pointcloud"/>
<extra_arg name="use_intra_process_comms" value="$(var use_intra_process)"/>
</composable_node>
<composable_node pkg="autoware_pointcloud_preprocessor" plugin="autoware::pointcloud_preprocessor::VoxelGridDownsampleFilterComponent" name="voxel_grid_downsample_filter">
<param from="$(var ndt_scan_matcher/pointcloud_preprocessor/voxel_grid_downsample_filter_param_path)"/>
<remap from="input" to="measurement_range/pointcloud"/>
<remap from="output" to="voxel_grid_downsample/pointcloud"/>
<extra_arg name="use_intra_process_comms" value="$(var use_intra_process)"/>
</composable_node>
<composable_node pkg="autoware_pointcloud_preprocessor" plugin="autoware::pointcloud_preprocessor::RandomDownsampleFilterComponent" name="random_downsample_filter">
<param from="$(var ndt_scan_matcher/pointcloud_preprocessor/random_downsample_filter_param_path)"/>
<remap from="input" to="voxel_grid_downsample/pointcloud"/>
<remap from="output" to="$(var output/pointcloud)"/>
<extra_arg name="use_intra_process_comms" value="$(var use_intra_process)"/>
</composable_node>

I feel like this also needs to be fixed, is that correct?
Also, should this be fixed by the localization team?

@SakodaShintaro
Copy link
Contributor

SakodaShintaro commented Oct 23, 2024

To run Autoware + AWSIM, I need the following changes.

(RobotecAI/awsim_sensor_kit_launch@main...SakodaShintaro:awsim_sensor_kit_launch:fix/filter_param is also needed)

Test procedure

(1) Download AWSIM v1.3.0
(2) Download nishishinjuku map
(3) Launch AWSIM $HOME/Downloads/AWSIM_v1.3.0/AWSIM.x86_64
(4) Source source ~/autoware/install/setup.bash
(5) Launch Autoware

ros2 launch autoware_launch e2e_simulator.launch.xml \
    map_path:=$HOME/Downloads/nishishinjuku_autoware_map_divided \
    vehicle_model:=sample_vehicle \
    sensor_model:=awsim_sensor_kit

(6) Set goal

ros2 topic pub -1 /planning/mission_planning/goal geometry_msgs/msg/PoseStamped '{
  header: {
    stamp: {sec: 181, nanosec: 289995947},
    frame_id: 'map'},
  pose: {
    position: { x: 81536.5703, y: 50199.3632, z: 34.2321 },
    orientation: { x: +0.0099193127825856, y: -0.0045161009766161, z: -0.0861385315656662, w: -0.9962236285209656 }
  }
}'

(7) Engage

ros2 topic pub /autoware/engage autoware_vehicle_msgs/msg/Engage "engage: true" -1

@vividf
Could you please check it on your PC and update the pull request on autoware.universe and create a pull request on autoware_launch?

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) tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) tag:require-cuda-build-and-test tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants