Refactored launch files#1118
Conversation
launching TB3 in simulation.
781d2a5 to
6d5ddd9
Compare
mkhansenbot
left a comment
There was a problem hiding this comment.
Overall, this is really good, but I have a few comments and suggestions.
bpwilcox
left a comment
There was a problem hiding this comment.
We had a short discussion about this last week, but should we consider moving the world, map, and the nav2_simulation_launch.py into a separate 'turtlebot3_example' directory under nav2_bringup? As is, we are still providing some defaults that are turtlebot3-specific in the launch files. I think separating out those files would make it more clear to the user. To that end, I think the base launch files would not provide any defaults that are specific to the turtlebot3 environment.
|
@mkhansen-intel @SteveMacenski I've pushed changes to address your feedback. Does everyone agree with @bpwilcox recommendations? i.e. to:
|
I'm good with this |
|
Ive got no blockers on this but I don’t know the launch API well enough to give any meaningful feedback |
|
@mkhansen-intel I've addressed your last feedback. @bpwilcox Matt and I had an offline discussion on your proposal. We would like to make those changes on a separate PR. |
|
@orduno That's okay if you want to do a follow-up PR, but I was thinking that those changes aren't very independent from this PR, considering you are creating and moving directories here. |
|
I'm thinking that factoring out TB3 dependencies logically corresponds to a separate PR. Can I get your approval to merge this? |
mkhansenbot
left a comment
There was a problem hiding this comment.
I approve, and agree the refactoring of TB3 into another folder can be a follow up, let's get this in
* Refactored launch files including fixes for launching TB3 in simulation.
Basic Info
Description of contribution in a few bullet points
Did some refactoring of
nav2_bringup:simulation_launch➡️bringup_launch➡️localization_launch&navigation_launchlaunch_ros.actions.Nodemechanism to launch the nodes instead ofExecuteProcess.I'm setting priority as high since running in simulation is currently broken (other than system test).
Future work that may be required in bullet points