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.

Sign up for GitHub

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

Jump to bottom

refactor(mission_planner): delete default values #3127

Merged
merged 4 commits into from
Apr 10, 2023
Merged

refactor(mission_planner): delete default values #3127

merged 4 commits into from
Apr 10, 2023

Conversation

yamazakiTasuku
Copy link
Contributor

@yamazakiTasuku yamazakiTasuku commented Mar 21, 2023 •
edited by satoshi-ota
Loading

Description

Removed default values defined in declare_parameter function.
mission_planner_delete_param.webm

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.

@github-actions github-actions bot added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Mar 21, 2023
@yamazakiTasuku yamazakiTasuku changed the title Mission planner delete param refactor(mission_planner):delete default values Mar 21, 2023
isamu-takagi
isamu-takagi reviewed Mar 22, 2023
View reviewed changes
planning/mission_planner/src/lanelet2_plugins/default_planner.cpp Outdated
Comment on lines 148 to 149
param_.goal_angle_threshold_deg = node_->declare_parameter("goal_angle_threshold_deg");
param_.enable_correct_goal_pose = node_->declare_parameter("enable_correct_goal_pose");
Copy link
Contributor

Choose a reason for hiding this comment

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

A template type is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

@isamu-takagi Thanks for your comment.

@yamazakiTasuku Could you fix it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed in 2e7e30a.

@satoshi-ota satoshi-ota changed the title refactor(mission_planner):delete default values refactor(mission_planner): delete default values Mar 22, 2023
@satoshi-ota
Copy link
Contributor

satoshi-ota commented Mar 22, 2023 •
edited
Loading

@yamazakiTasuku Thanks for your work.

Btw, please don't check following checkbox before the review was NOT finished 🙏

image

@satoshi-ota satoshi-ota force-pushed the mission_planner_delete_param branch from 9d8cc60 to 2e7e30a Compare March 31, 2023 02:25
satoshi-ota
satoshi-ota approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@satoshi-ota satoshi-ota left a comment

Choose a reason for hiding this comment

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

These default values are defined here. LGTM.

https://github.com/autowarefoundation/autoware_launch/blob/133a3d94b87ff7278f1b0585747b762609c645e1/autoware_launch/config/planning/mission_planning/mission_planner/mission_planner.param.yaml#L7-L8

@isamu-takagi
Copy link
Contributor

@yamazakiTasuku
Could you apply this change to static_centerline_optimizer to pass CI? This package depends on mission_planner here.

@satoshi-ota
Copy link
Contributor

@yamazakiTasuku @isamu-takagi I take over this PR.

satoshi-ota

This comment was marked as resolved.

Sign in to view

yamazakiTasuku and others added 3 commits April 7, 2023 21:38
@satoshi-ota
delete param
ab49c67 
Signed-off-by: yamazakiTasuku <tasuku.yamazaki@tier4.jp>
@satoshi-ota
fix(mission_planner): add type
0a50eeb 
Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>
@satoshi-ota
fix(static_centerline_optimizer): add mission planner param path
62919c1 
Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>
@satoshi-ota satoshi-ota force-pushed the mission_planner_delete_param branch from 32e299d to 62919c1 Compare April 7, 2023 12:51
@codecov
Copy link

codecov bot commented Apr 7, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (88fc83d) 12.36% compared to head (9dff064) 12.37%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3127   +/-   ##
=======================================
  Coverage   12.36%   12.37%           
=======================================
  Files        1368     1368           
  Lines       96023    95979   -44     
  Branches    27574    27537   -37     
=======================================
+ Hits        11874    11876    +2     
+ Misses      71609    71564   -45     
+ Partials    12540    12539    -1
Flag Coverage Δ *Carryforward flag
differential 21.17% <0.00%> (?)
total 12.37% <ø> (+<0.01%) ⬆️ Carriedforward from 62919c1

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

Impacted Files Coverage Δ
...n_planner/src/lanelet2_plugins/default_planner.cpp 22.88% <0.00%> (ø)

... and 4 files with indirect coverage changes

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.

@satoshi-ota
Copy link
Contributor

@kosuke55 @isamu-takagi I would like you to check this PR as code owner.

@satoshi-ota @kosuke55
fix
9dff064 
Co-authored-by: Kosuke Takeuchi <kosuke.tnp@gmail.com>
@satoshi-ota satoshi-ota requested a review from kosuke55 April 9, 2023 08:07
@satoshi-ota satoshi-ota merged commit 6b55253 into autowarefoundation:main Apr 10, 2023
@tier4-autoware-public-bot tier4-autoware-public-bot bot mentioned this pull request Apr 11, 2023
Merged
purewater0901 pushed a commit that referenced this pull request Apr 11, 2023
@yamazakiTasuku @satoshi-ota
@kosuke55 @purewater0901
refactor(mission_planner): delete default values (#3127)
e2e9131 
* delete param

Signed-off-by: yamazakiTasuku <tasuku.yamazaki@tier4.jp>

* fix(mission_planner): add type

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

* fix(static_centerline_optimizer): add mission planner param path

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

* fix

Co-authored-by: Kosuke Takeuchi <kosuke.tnp@gmail.com>

---------

Signed-off-by: yamazakiTasuku <tasuku.yamazaki@tier4.jp>
Signed-off-by: satoshi-ota <satoshi.ota928@gmail.com>
Co-authored-by: yamazakiTasuku <tasuku.yamazaki@tier4.jp>
Co-authored-by: satoshi-ota <satoshi.ota928@gmail.com>
Co-authored-by: Satoshi OTA <44889564+satoshi-ota@users.noreply.github.com>
Co-authored-by: Kosuke Takeuchi <kosuke.tnp@gmail.com>
@takayuki5168 takayuki5168 mentioned this pull request Apr 11, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kosuke55 kosuke55 kosuke55 approved these changes

@isamu-takagi isamu-takagi isamu-takagi approved these changes

@satoshi-ota satoshi-ota satoshi-ota approved these changes

@takayuki5168 takayuki5168 Awaiting requested review from takayuki5168

@mitsudome-r mitsudome-r Awaiting requested review from mitsudome-r

@TakaHoribe TakaHoribe Awaiting requested review from TakaHoribe

Assignees

@isamu-takagi isamu-takagi

@satoshi-ota satoshi-ota

Labels
component:planning Route planning, decision-making, and navigation. (auto-assigned)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yamazakiTasuku @satoshi-ota @isamu-takagi @kosuke55