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

fix(launch): add missing launch args and defaults to lidar_based_detection.launch.xml #4596

Merged
merged 2 commits into from
Aug 23, 2023
Merged

fix(launch): add missing launch args and defaults to lidar_based_detection.launch.xml #4596

merged 2 commits into from
Aug 23, 2023

Conversation

alireza-moayyedi
Copy link
Contributor

@alireza-moayyedi alireza-moayyedi commented Aug 11, 2023 •
edited by github-actions bot
Loading

Some launch arguments were missing. These arguments and their defaults were added.

Description

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

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.

Summary by CodeRabbit

Release Notes:

  • Bug fix: Added missing launch arguments and their default values to the lidar_based_detection.launch.xml file.

"Fixing bugs, launching with pride,
Missing arguments no longer hide.
With this update, we're on track,
Detection improved, no turning back!"

@alireza-moayyedi
Update lidar_based_detection.launch.xml
cff0b07 
Some launch arguments were missing. These arguments and their defaults were added.

Signed-off-by: Alireza Moayyedi <alireza.moayyedi@nobleo.nl>
@github-actions github-actions bot added the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Aug 11, 2023
@yukkysaito yukkysaito requested a review from yukke42 August 12, 2023 09:03
@miursh
Copy link
Contributor

miursh commented Aug 14, 2023

@alireza-moayyedi
Thank you so much for your contribution to Autoware!

Similar to other perception mode launchers, like camera_lidar_fusion_based_detection, even without adding these args to lidar_based_detection.launch.xml, the ones defined in the perception.launch will be inherited, so it will work without any issues.

Is there a specific reason for adding these arguments?

@alireza-moayyedi
Copy link
Contributor Author

alireza-moayyedi commented Aug 14, 2023 •
edited
Loading

@miursh Thanks for the explanation but I am only using lidar_based_detection.launch.xml for a specific usecase that I'm working on. Therefore the missing args were not being inherited from anywhere and I could not run it without those.

@miursh
Copy link
Contributor

miursh commented Aug 14, 2023

@alireza-moayyedi
I see. You mean you don't use perception.launch but use lidar_based_detection.launch directly.

@alireza-moayyedi
Copy link
Contributor Author

@miursh precisely.

@alireza-moayyedi
changed default of objects_filter_method
d3fae92 
changed default of the "objects_filter_method" to "lanelet_filter" as requested.

Signed-off-by: Alireza Moayyedi <alireza.moayyedi@nobleo.nl>
@miursh miursh added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Aug 16, 2023
miursh
miursh approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@miursh miursh left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

github-actions bot commented Aug 16, 2023 •
edited
Loading

Image description CodeRabbit

Walkthrough

The changes made to the codebase involve adding missing launch arguments and their default values to the lidar_based_detection.launch.xml file. This update ensures that all necessary launch arguments are defined and have appropriate default values.

Changes

File Summary
launch/tier4_perception_launch/launch/object_recognition/detection/lidar_based_detection.launch.xml The diff adds missing launch arguments and their default values to the lidar_based_detection.launch.xml file.

github-actions[bot]
github-actions bot reviewed Aug 16, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Image description CodeRabbit

Commits Files that changed from the base of the PR and between 2b7afb1 and d3fae92 commits.
Files selected (1)
  • launch/tier4_perception_launch/launch/object_recognition/detection/lidar_based_detection.launch.xml (1)
Files skipped from review due to trivial changes (1)
  • launch/tier4_perception_launch/launch/object_recognition/detection/lidar_based_detection.launch.xml
Review comments generated (0)
  • Review: 0
  • LGTM: 0
Tips

Chat with Image description CodeRabbit Bot (@coderabbitai)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @coderabbitai in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

  • Add @coderabbitai: ignore anywhere in the PR description to pause further reviews from the bot.

@miursh miursh added run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) and removed run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) labels Aug 17, 2023
@alireza-moayyedi
Copy link
Contributor Author

@miursh Hello, can this PR be merged or is there something more to do?

@miursh miursh merged commit 9407021 into autowarefoundation:main Aug 23, 2023
@miursh
Copy link
Contributor

miursh commented Aug 23, 2023

@alireza-moayyedi Thank you for the notification. I have just pressed the merge button

kminoda pushed a commit to kminoda/autoware.universe that referenced this pull request Aug 23, 2023
@alireza-moayyedi @kminoda
fix(launch): add missing launch args and defaults to lidar_based_dete…
df57c12 
…ction.launch.xml (autowarefoundation#4596)

* Update lidar_based_detection.launch.xml

Some launch arguments were missing. These arguments and their defaults were added.

Signed-off-by: Alireza Moayyedi <alireza.moayyedi@nobleo.nl>

* changed default of objects_filter_method

changed default of the "objects_filter_method" to "lanelet_filter" as requested.

Signed-off-by: Alireza Moayyedi <alireza.moayyedi@nobleo.nl>

---------

Signed-off-by: Alireza Moayyedi <alireza.moayyedi@nobleo.nl>
@tier4-autoware-public-bot tier4-autoware-public-bot bot mentioned this pull request Aug 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@miursh miursh miursh approved these changes

@YoshiRi YoshiRi Awaiting requested review from YoshiRi YoshiRi is a code owner

@yukkysaito yukkysaito Awaiting requested review from yukkysaito yukkysaito is a code owner

@yukke42 yukke42 Awaiting requested review from yukke42

Assignees
No one assigned
Labels
component:launch Launch files, scripts and initialization tools. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alireza-moayyedi @miursh