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): load_distortion_parameter_from_yaml #93

Merged

Conversation

vividf
Copy link
Contributor

@vividf vividf commented Jun 27, 2024

Description

PR: autowarefoundation/autoware.universe#7137 removes the default parameters from the distortion correction node and add the parameters file for the distortion node.

As the default parameters are removed, nebula_node_container.launch.py needs to load the parameters from yaml file.

Related links

Tests performed

tested the command

ros2 launch autoware_launch logging_simulator.launch.xml map_path:=$HOME/autoware_map/sample-map-rosbag vehicle_model:=sample_vehicle sensor_model:=sample_sensor_kit`

with sample_rosbag

Notes for reviewers

This PR needs to be merged with autowarefoundation/autoware.universe#7137

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: vividf <yihsiang.fang@tier4.jp>
@vividf vividf requested review from drwnz, knzo25 and YoshiRi June 27, 2024 03:24
@vividf vividf changed the title feature/load_distortion_parameter_from_yaml feat/load_distortion_parameter_from_yaml Jun 27, 2024
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@vividf vividf changed the title feat/load_distortion_parameter_from_yaml feat(pointcloud_preprocessor):load_distortion_parameter_from_yaml Jun 27, 2024
@vividf vividf changed the title feat(pointcloud_preprocessor):load_distortion_parameter_from_yaml feat(pointcloud_preprocessor): load_distortion_parameter_from_yaml Jun 27, 2024
…_sensor_launch

Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@vividf vividf self-assigned this Jun 27, 2024
@vividf vividf requested a review from badai-nguyen June 27, 2024 05:17
Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

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

LGTM

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.

In the past we used the yaml way of loading parameters due to a combination of

    # https://github.com/ros2/launch_ros/issues/156
    def load_composable_node_param(param_path):
        with open(LaunchConfiguration(param_path).perform(context), "r") as f:
            return yaml.safe_load(f)["/**"]["ros__parameters"]

and the need of doing algebra with the vehicle dimensions (as far as I could deduce, never heard it from someone).

Do we not use ParameterFile now? If not, can you tell me the reason?

@vividf
Copy link
Contributor Author

vividf commented Jul 2, 2024

@knzo25
Thanks for your review.
I added the load_composable_node_param based on the https://github.com/tier4/aip_launcher/blob/tier4/universe/common_sensor_launch/launch/nebula_node_container.launch.py
in aip_launcher. Becasue I think both of the files should have the same implementation for loading parameters

@vividf vividf requested a review from knzo25 July 2, 2024 01:43
@knzo25
Copy link
Contributor

knzo25 commented Jul 2, 2024

@knzo25 Thanks for your review. I added the load_composable_node_param based on the https://github.com/tier4/aip_launcher/blob/tier4/universe/common_sensor_launch/launch/nebula_node_container.launch.py in aip_launcher. Becasue I think both of the files should have the same implementation for loading parameters

That is not an appropriate answer ("because somewhere else it is done" it not correct. It may be wrong there as well).
We should not use non-standard pipelines whenever possible (I explained the reasons why in some cases we need to do it as of now)

@vividf
Copy link
Contributor Author

vividf commented Jul 2, 2024

@knzo25
Thanks for your reply.
Do you mean that using the ParameterFile is the standard way to do it?
If it is the correct way, can you check this b680d4d (which I implemented before trying to make both nebular_node_container.py consistent)

@knzo25
Copy link
Contributor

knzo25 commented Jul 2, 2024

I invite you to read the original repository if you have doubts:
https://github.com/ros2/launch_ros

The commit you mention looks ok. Did you make another PR for the company launchers (sorry if I missed that one but haven't gotten any notifications)?

*consistent

Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@vividf
Copy link
Contributor Author

vividf commented Jul 2, 2024

@knzo25
Thanks, I fixed it in 7dda587

Regarding to the company one, I just request review now, but I also need to modify the change there

@knzo25
Copy link
Contributor

knzo25 commented Jul 3, 2024

@vividf
I tested locally the aip_launcher. Can you double check if the node launcher correctly with the parameters being reflected (e.g., using the 3d undistortion). After that we can merge the 3 PRs together (time the merge of the launchers after the one in universe has been merged). I suggest to merge them today, since the point type changes may be merged soon, and if that one gets merged you will have to solve the conflicts

@vividf
Copy link
Contributor Author

vividf commented Jul 3, 2024

@knzo25
Yes, I tested the parameters change on both sample_sensor_kit_launch and aip_launcher.
Thanks!

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

@vividf vividf merged commit bbf6398 into autowarefoundation:main Jul 3, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants