Deprecates use of geometry_msgs/PoseArray for particle cloud in AMCL by abhishek47kashyap · Pull Request #2281 · ros-navigation/navigation2

abhishek47kashyap · 2021-03-30T23:35:18Z

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#1679
Primary OS tested on	Ubuntu 20.04.2 LTS
Robotic platform tested on	simulated Turtlebot3 in the Gazebo simulator

Description of contribution in a few bullet points

Removed publisher particlecloud_pub_ of type geometry_msgs/PoseArray
Removed particlecloud_pub_->on_activate() and particlecloud_pub_->on_deactivate()
Removed a warning message

Description of documentation updates required from your changes

New Particle Filter Messages could be updated to remove mention of PoseArray

Future work that may be required in bullet points

For Maintainers:

Check that any new parameters added are updated in navigation.ros.org
Check that any significant change is added to the migration guide
Check that any new functions have Doxygen added
Check that any new features have test coverage
Check that any new plugins is added to the plugins page
If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

SteveMacenski · 2021-03-31T00:15:55Z

nav2_amcl/src/amcl_node.cpp

-    cloud_msg->poses[i].position.y = set->samples[i].pose.v[1];
-    cloud_msg->poses[i].position.z = 0;
-    cloud_msg->poses[i].orientation = orientationAroundZAxis(set->samples[i].pose.v[2]);
-    cloud_with_weights_msg->particles[i].pose = (*cloud_msg).poses[i];


You deleted the population of the particle could poses / weights for the one that's keeping too... I thought you tested this in rviz?

cloud_msg is type PoseArray, and cloud_with_weights_msg is type ParticleCloud. I deleted the former.

I did test in rviz, here's a screenshot:

Then where is the particle cloud in that image?

Re-read that code, its very clear that you're not populating the poses or the weights from just removing that full for statement

cloud_with_weights_msg->particles[i].pose = (*cloud_msg).poses[i]; cloud_with_weights_msg->particles[i].weight = set->samples[i].weight;

Dumb mistake, does f21bed8 correct it?

codecov · 2021-03-31T16:47:08Z

Codecov Report

Merging #2281 (885a0c5) into main (6096221) will decrease coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2281      +/-   ##
==========================================
- Coverage   85.09%   84.94%   -0.15%     
==========================================
  Files         258      258              
  Lines       12798    12780      -18     
==========================================
- Hits        10890    10856      -34     
- Misses       1908     1924      +16

Flag	Coverage Δ
project	`84.94% <ø> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
nav2_amcl/src/amcl_node.cpp	`84.42% <ø> (-0.58%)`	⬇️
...tmap_2d/plugins/costmap_filters/costmap_filter.cpp	`73.68% <0.00%> (-10.53%)`	⬇️
...v2_util/include/nav2_util/simple_action_server.hpp	`89.37% <0.00%> (-5.00%)`	⬇️
...tree/include/nav2_behavior_tree/bt_action_node.hpp	`92.85% <0.00%> (-1.20%)`	⬇️
nav2_planner/src/planner_server.cpp	`86.86% <0.00%> (-0.73%)`	⬇️
nav2_controller/src/nav2_controller.cpp	`89.62% <0.00%> (-0.42%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6096221...885a0c5. Read the comment docs.

SteveMacenski

LGTM, waiting on CI!

SteveMacenski · 2021-03-31T20:39:29Z

--- src/amcl_node.cpp
+++ src/amcl_node.cpp.uncrustify
@@ -872 +872,2 @@
-    cloud_with_weights_msg->particles[i].pose.orientation = orientationAroundZAxis(set->samples[i].pose.v[2]);
+    cloud_with_weights_msg->particles[i].pose.orientation = orientationAroundZAxis(
+      set->samples[i].pose.v[2]);
@@ -875 +876 @@

You have a linting problem

SteveMacenski · 2021-04-01T05:34:48Z

Still linting issues :-)

abhishek47kashyap · 2021-04-01T05:47:30Z

Still linting issues :-)

Could I may be get the line numbers where my PR fails the lint test?

Suggestions to prevent general lint issues are also welcome. I presume ROS2 uses cppcheck and cpplint, as this ROS answer claims?

SteveMacenski · 2021-04-01T18:13:18Z

If you go into the circle CI job that failed right below, it tells you very directly

Diff with 8 lines
--- src/amcl_node.cpp
+++ src/amcl_node.cpp.uncrustify
@@ -873 +873 @@
-            set->samples[i].pose.v[2]);
+      set->samples[i].pose.v[2]);
@@ -876 +876 @@
-  
+

Please lint locally, you'd catch this too with ament_uncrustify

was incorrectly deleted in 0ca93b5

#2281 (comment)

commandline tool used: ament_uncrustify --reformat src/navigation2/nav2_amcl/src/amcl_node.cpp

abhishek47kashyap · 2021-04-02T04:52:19Z

test_spin_recovery_fake.missing_result

Originates from
[test_spin_recovery_fake_node-6] [ FAILED ] SpinRecoveryTests/SpinRecoveryTestFixture.testSpinRecovery/1570796_0100000, where GetParam() = (-1.5708, 0.1)

Not sure how to fix this ..

SteveMacenski · 2021-04-02T14:43:48Z

Must have been a flaky run, but the other test build in release mode passed so we can skip it

SteveMacenski · 2021-04-02T14:43:53Z

Thanks for the help!

…os-navigation#2281) * removed geometry_msgs/PoseArray in AMCL, updated rviz configs * ParticleFilter -> ParticuleCloud * put back adding of weights and poses for particles was incorrectly deleted in 0ca93b5 * complies with linter's max line length ros-navigation#2281 (comment) * reformatted file using ament_uncrustify commandline tool used: ament_uncrustify --reformat src/navigation2/nav2_amcl/src/amcl_node.cpp

SteveMacenski requested changes Mar 31, 2021

View reviewed changes

SteveMacenski approved these changes Mar 31, 2021

View reviewed changes

SteveMacenski added the merge label Mar 31, 2021

abhishek47kashyap added 5 commits April 2, 2021 12:22

removed geometry_msgs/PoseArray in AMCL, updated rviz configs

6648841

ParticleFilter -> ParticuleCloud

6250969

put back adding of weights and poses for particles

b8a5d26

was incorrectly deleted in 0ca93b5

complies with linter's max line length

bdc1707

#2281 (comment)

reformatted file using ament_uncrustify

42b0d66

commandline tool used: ament_uncrustify --reformat src/navigation2/nav2_amcl/src/amcl_node.cpp

SteveMacenski merged commit 1821ac3 into ros-navigation:main Apr 2, 2021

Conversation

abhishek47kashyap commented Mar 30, 2021 • edited by SteveMacenski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Basic Info

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

Uh oh!

SteveMacenski Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

abhishek47kashyap Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

abhishek47kashyap Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 31, 2021

Codecov Report

Uh oh!

SteveMacenski left a comment

Choose a reason for hiding this comment

Uh oh!

SteveMacenski commented Mar 31, 2021

Uh oh!

SteveMacenski commented Apr 1, 2021

Uh oh!

abhishek47kashyap commented Apr 1, 2021

Uh oh!

SteveMacenski commented Apr 1, 2021

Uh oh!

abhishek47kashyap commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Apr 2, 2021

Uh oh!

SteveMacenski commented Apr 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abhishek47kashyap commented Mar 30, 2021 •

edited by SteveMacenski

Loading

abhishek47kashyap Mar 31, 2021 •

edited

Loading

abhishek47kashyap commented Apr 2, 2021 •

edited

Loading