Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

feat(localization_launch): replace localization_launch with tier4_localization_launch #651

Merged
merged 5 commits into from
Jan 16, 2023

Conversation

kminoda
Copy link
Contributor

@kminoda kminoda commented Dec 23, 2022

Signed-off-by: kminoda koji.minoda@tier4.jp

PR Type

  • Improvement

Related Links

Also need to merge tier4/aip_launcher#98

Description

Remove localization_launch and use tier4_localization_launch in autoware.universe.

The parameter configuration in autoware.universe and autoware_launch has been modified as described in this issue.
With this modification, we are now able to pass parameter paths to tier4_xx_launch from the outside of the package, i.e. autoware_launch packages. Thus, I would like to remove xx_launch from tier4/autoware_launch and replace it with tier4_xx_launch in autoware.universe.

With this PR, we can reduce the maintenance cost of xx_launch.

(Before this PR: we need to modify both tier4_xx_launch and xx_launch. After this PR: we only need to modify tier4_xx_launch)

Review Procedure

Remarks

Please refer to this TIER IV internal link, too.

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • Code follows coding guidelines
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets
  • Write release notes

CI Checks

  • Build and test for PR: Required to pass before the merge.
  • Check spelling: NOT required to pass before the merge. It is up to the reviewer(s). See here if you want to add some words to the spell check dictionary.

@kminoda kminoda changed the title remove localization_launch feat(tier4_localization_launch): Remove localization_launch Dec 23, 2022
@kminoda kminoda self-assigned this Dec 23, 2022
@kminoda kminoda changed the title feat(tier4_localization_launch): Remove localization_launch feat(localization_launch): Remove localization_launch Dec 23, 2022
@h-ohta h-ohta changed the title feat(localization_launch): Remove localization_launch feat(localization_launch): remove localization_launch Dec 23, 2022
@YamatoAndo YamatoAndo removed their request for review December 23, 2022 08:47
@YamatoAndo
Copy link
Contributor

@tkimura4 @h-ohta I'll leave it to your judgment.

@h-ohta
Copy link
Contributor

h-ohta commented Dec 26, 2022

@kminoda Could you add some reason why you make this PR, e.g. issues etc?

Signed-off-by: kminoda <koji.minoda@tier4.jp>
@h-ohta h-ohta force-pushed the feat/replace_localization_launch branch from cd11f1e to a4609f6 Compare December 27, 2022 10:46
@takayuki5168
Copy link
Contributor

The change looks good to me.
It's fine for me to merge if psim and lsim work well with this PR.

@h-ohta
Copy link
Contributor

h-ohta commented Jan 4, 2023

I confirmed starting autoware.launch.xml and logging_simulator.launch.xml

h-ohta
h-ohta previously approved these changes Jan 4, 2023
@kminoda kminoda changed the title feat(localization_launch): remove localization_launch feat(localization_launch): replace localization_launch with tier4_localization_launch Jan 6, 2023
@kminoda
Copy link
Contributor Author

kminoda commented Jan 10, 2023

@h-ohta Sorry, would you approve this again?

h-ohta
h-ohta previously approved these changes Jan 10, 2023
Copy link
Contributor

@h-ohta h-ohta left a comment

Choose a reason for hiding this comment

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

LGTM.
However, I'd like to discuss how we can understand easily which parameter is changed in the next step.

@kminoda
Copy link
Contributor Author

kminoda commented Jan 13, 2023

@h-ohta Sorry, would you approve this again? 🙇

h-ohta
h-ohta previously approved these changes Jan 13, 2023
…ng on vehicles

Signed-off-by: kminoda <koji.minoda@tier4.jp>
@kminoda
Copy link
Contributor Author

kminoda commented Jan 16, 2023

@YamatoAndo Regarding the issue due to autowarefoundation/autoware.universe#2114, I think there are two solutions

  1. Fix aip_launcher and incorporate the modification in feat(sample_sensor_kit_launch): pass container to velodyne nodes autowarefoundation/sample_sensor_kit_launch#48
  2. Provide /sensing/lidar/top/... as a pointcloud_container_name from autoware_launch (no need for modification in aip_launcher)

I thought 2 would be a simple and thus better at least as a temporary solution. What do you think?

@YamatoAndo
Copy link
Contributor

@kminoda If it is a temporary solution, option 2 would be fine.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants