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

Re-enable costmap tests (footprint and inflation tests) #805

Merged
merged 4 commits into from
Jun 6, 2019

Conversation

bpwilcox
Copy link

@bpwilcox bpwilcox commented Jun 5, 2019

Basic Info

Info Please fill out this column
Ticket(s) this addresses #121
Primary OS tested on Ubuntu 18.04

Description of contribution in a few bullet points

  • Re-enables footprint and inflation tests
  • Removes obsolete static_tests from repo

Future work that may be required in bullet points

  • The costmap_tester.cpp test is still disabled since there was a dependency on rosbag. I believe rosbag is now supported in ros2, so we can look into re-enabling this test

Copy link
Contributor

@crdelsey crdelsey left a comment

Choose a reason for hiding this comment

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

Looks good, but could you fix the for_each that was introduced into the code earlier.

@@ -84,8 +84,8 @@ void
Layer::undeclareAllParameters()
{
std::for_each(begin(local_params_), end(local_params_), [this](const std::string & param_name) {
Copy link
Contributor

@crdelsey crdelsey Jun 5, 2019

Choose a reason for hiding this comment

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

std::for_each makes no sense here. This should just be a simple for loop.

Suggested change
std::for_each(begin(local_params_), end(local_params_), [this](const std::string & param_name) {
for(auto & param_name : local_params_) {
node_->undeclare_parameter(getFullName(param_name));
}

PS: don't just commit the suggestion. I was only able to get the suggestion mechanism to replace the first line, but you need to replace the whole for loop

@mjeronimo mjeronimo added the nav2_lifecycle Related to the manged/lifecycle node implementation label Jun 5, 2019
@mjeronimo mjeronimo merged commit e5be603 into ros-navigation:lifecycle Jun 6, 2019
mjeronimo pushed a commit that referenced this pull request Jun 6, 2019
* re-enabling obstacle, inflation, and footprint tests

* remove static tests, redesign footprint_tests without Costmap2DROS

* passing linters and uncrustify

* replace for_each with for loop
crdelsey pushed a commit that referenced this pull request Jun 10, 2019
* re-enabling obstacle, inflation, and footprint tests

* remove static tests, redesign footprint_tests without Costmap2DROS

* passing linters and uncrustify

* replace for_each with for loop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nav2_lifecycle Related to the manged/lifecycle node implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants