[ROS2] grid_map_demos port 1/3 #259

Marwan99 · 2020-09-13T00:04:40Z

grid_map_demos porting will be split into 3 PRs.

This PR includes:

Linting of the whole package.
Port of the following nodes/demos:
- simple_demo 118ef1c
  - src/simple_demo_node.cpp
  - config/simple_demo.yaml
  - launch/simple_demo_launch.py
- tutorial_demo 118ef1c
  - src/tutorial_demo_node.cpp
  - config/tutorial_demo.yaml
  - launch/tutorial_demo_launch.py
- iterator_demo ad6e9ea
  - src/iterators_demo_node.cpp
  - src/IteratorsDemo.cpp
  - config/iterators_demo.yaml
  - launch/iterators_demo_launch.py
- image_to_grid_map_demo ad6e9ea
  - src/image_to_gridmap_demo_node.cpp
  - src/ImageToGridmapDemo.cpp
  - scripts/image_publisher.py
  - config/image_to_gridmap_demo.yaml
  - launch/image_to_gridmap_demo_launch.py
The CMakeLists.txt and package.xml have been updated according to the nodes/demos mentioned above, they should be fully ported and cleaned in the final grid_map_demos PR.

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

SteveMacenski

Looks good on a first run, did you test each of these to be working in rviz?

SteveMacenski · 2020-09-14T17:51:48Z

grid_map_demos/CMakeLists.txt

-  ${catkin_INCLUDE_DIRS}
-  ${EIGEN3_INCLUDE_DIR}
-  ${OCTOMAP_INCLUDE_DIR}
+  # ${EIGEN3_INCLUDE_DIR}


Dont you still need these?

These includes seemed redundant, things building and running fine as is (locally at least atm). octomap already included when depending on grid_map_octomap and same for eigen as well (not sure which package off the top of my head). Will delete them in the final grid_map_demos port to confirm that nothing needs, if thats okay.

SteveMacenski · 2020-09-14T17:52:06Z

grid_map_demos/CMakeLists.txt

  simple_demo
-  ${catkin_LIBRARIES}
+  "rclcpp"


Use dependencies these are all the same

I was planning on using dependencies once I all the nodes are ported to know all the common dependencies. So this should done in an upcoming PR, is that okay.

SteveMacenski · 2020-09-14T17:54:05Z

grid_map_demos/src/ImageToGridmapDemo.cpp

+
+  gridMapPublisher_ = this->create_publisher<grid_map_msgs::msg::GridMap>(
+    "grid_map", rclcpp::QoS(
+      1).transient_local());


This seems like an unnecessary line break

SteveMacenski · 2020-09-14T17:55:07Z

grid_map_demos/src/IteratorsDemo.cpp

-    ros::Duration duration(0.01);
-    duration.sleep();
+    auto sleep_duration = rclcpp::Duration::from_seconds(0.01);
+    rclcpp::sleep_for(std::chrono::nanoseconds(sleep_duration.nanoseconds()));


was 0.01*1e9 not good enough 😉

Fair point :')

Marwan99 · 2020-09-14T18:12:24Z

Looks good on a first run, did you test each of these to be working in rviz?

@SteveMacenski Yes, all tested with RVIZ and compared against the output from ros1.

… instead of a node for each visualization Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Marwan99 · 2020-09-14T19:54:42Z

CI failure should be resolved once grid_map_filters is merged.
The dependency on grid_map_filters can be commend out for now to get CI to continue uninterpreted, if need.

maximilianwulf

That is huge 👍 Thanks a lot. So you want to merge this and then do the rest?

maximilianwulf · 2020-09-24T16:17:26Z

grid_map_demos/launch/iterators_demo_launch.py

@@ -0,0 +1,61 @@
+import os


That is a lot of work, respect!

maximilianwulf · 2020-09-24T16:19:57Z

grid_map_demos/src/filters_demo_node.cpp

  return 0;
 }
-


maximilianwulf · 2020-09-24T16:20:40Z

grid_map_demos/src/move_demo_node.cpp

@@ -15,15 +12,16 @@ int main(int argc, char** argv)
  GridMap map({"layer"});
  map.setFrameId("map");
  map.setGeometry(Length(0.7, 0.7), 0.01, Position(0.0, 0.0));
-  ROS_INFO("Created map with size %f x %f m (%i x %i cells).\n The center of the map is located at (%f, %f) in the %s frame.",
+  ROS_INFO(


This file is not ported into this PR, it is just linted.
This will be addressed in an upcoming PR.

grid_map_demos/src/opencv_demo_node.cpp

maximilianwulf · 2020-09-24T16:22:20Z

grid_map_demos/src/tutorial_demo_node.cpp

-    unsigned int nCells = map.getSize().prod();
-    // cppcheck-suppress unreadVariable
-    double rootMeanSquaredError = sqrt((map["error"].array().pow(2).sum()) / nCells);
+    // unsigned int nCells = map.getSize().prod();  // unused variable


I think you can just delete it.

maximilianwulf · 2020-09-24T16:23:04Z

grid_map_visualization/include/grid_map_visualization/visualizations/GridCellsVisualization.hpp

@@ -30,7 +30,7 @@ class GridCellsVisualization : public VisualizationBase
   * Constructor.
   * @param name the name of the visualization.
   */
-  explicit GridCellsVisualization(const std::string & name);
+  explicit GridCellsVisualization(const std::string & name, rclcpp::Node::SharedPtr node_ptr);


Violation of naming convention: nodePtr.

maximilianwulf · 2020-09-24T16:23:28Z

..._map_visualization/include/grid_map_visualization/visualizations/PointCloudVisualization.hpp

@@ -32,7 +32,7 @@ class PointCloudVisualization : public VisualizationBase
   * Constructor.
   * @param name the name of the visualization.
   */
-  explicit PointCloudVisualization(const std::string & name);
+  explicit PointCloudVisualization(const std::string & name, rclcpp::Node::SharedPtr node_ptr);


In all of these files the naming convention was violated.

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

ghost · 2020-09-25T01:28:46Z

Congratulations 🎉. DeepCode analyzed your code in 902.65 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Marwan99 · 2020-09-25T01:34:31Z

So you want to merge this and then do the rest?

Yes, the rest will be the 2 upcoming PRs.

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Marwan99 · 2020-09-27T17:10:23Z

Regarding the deepcode warnings:

Container std::array is only queried but does not seem to be ever updated. Occurrences: InterpolationDemo.cpp:129

I guess that's fine in this context?

Calling size on empty container std::array will always evaluate to 0. Occurrences: InterpolationDemo.cpp:141

This doesn't seem right to me as std::array<..> g is being populated here so g is not an empty container, is that correct?

@maximilianwulf Can you please advice on the correct course of action for these warnings?

@maximilianwulf @SteveMacenski Can you please review the changes?

maximilianwulf · 2020-09-27T18:30:21Z

Container std::array is only queried but does not seem to be ever updated. Occurrences: InterpolationDemo.cpp:129
I guess that's fine in this context?

We add content in the for loop, right? I don't really get it.

This doesn't seem right to me as std::array<..> g is being populated here so g is not an empty container, is that correct?

Ah yes, that is what I think, can you add an ignore to it?

maximilianwulf

LGTM.

Marwan99 · 2020-09-28T14:56:21Z

We add content in the for loop, right?

Yes.
It is ambiguous about the array its referring to. Is it safe to ignore this warning as well?

grid_map/grid_map_demos/src/InterpolationDemo.cpp

Lines 129 to 135 in 17f4f87

    
           for (int i = 0; i < numGaussians; ++i) { 
        
             g.at(i).x0 = means.at(i).first; 
        
             g.at(i).y0 = means.at(i).second; 
        
             g.at(i).varX = vars.at(i).first; 
        
             g.at(i).varY = vars.at(i).second; 
        
             g.at(i).s = scales.at(i); 
        
           }

maximilianwulf · 2020-09-29T16:55:35Z

It is ambiguous about the array its referring to. Is it safe to ignore this warning as well?

I would say so.

Marwan99 · 2020-09-29T17:13:11Z

Retry DeepCode

Marwan99 · 2020-09-29T17:17:40Z

Very interesting, the warnings are now magically not there anymore.
The warnings were not showing up for some reason so I re-triggered deepcode to get them again but now they are completely gone.

@SteveMacenski Good to go?

SteveMacenski · 2020-09-29T17:36:59Z

You need to readd EIGEN3_INCLUDE_DIR to includes so that when the includes are explored these are gathered.

SteveMacenski · 2020-09-29T19:21:31Z

Ping me when CI passes and I'll merge

Marwan99 · 2020-09-29T19:43:29Z

@SteveMacenski Ready to merge.

Marwan99 added 4 commits September 12, 2020 23:53

mostly linted and removed COLCON_IGNORE

7e21096

simple_demo and tutorial_demo port

118ef1c

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

image_to_gridmap_demo and iterators_demo port

ad6e9ea

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

more linting

6f436fe

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Marwan99 mentioned this pull request Sep 13, 2020

ROS2 Port #219

Closed

SteveMacenski reviewed Sep 14, 2020

View reviewed changes

Marwan99 added 2 commits September 14, 2020 20:02

visualizations fix - using one shared node accross all visualizations…

da1c418

… instead of a node for each visualization Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

minor fixes and formatting

04c147e

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

maximilianwulf reviewed Sep 24, 2020

View reviewed changes

renamed node_ptr to nodePtr and removed unwanted commented code

192ca75

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Marwan99 added 2 commits September 25, 2020 18:33

Merge branch 'ros2' of github.com:ANYbotics/grid_map into demo-port

fadac84

changed grid_map_ros lib to be shared

17f4f87

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

maximilianwulf approved these changes Sep 27, 2020

View reviewed changes

added eigen to include dirs

ac8aca7

SteveMacenski merged commit a7157dd into ANYbotics:ros2 Sep 29, 2020

Marwan99 deleted the demo-port branch September 29, 2020 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ROS2] grid_map_demos port 1/3 #259

[ROS2] grid_map_demos port 1/3 #259

Marwan99 commented Sep 13, 2020 •

edited

Loading

SteveMacenski left a comment

SteveMacenski Sep 14, 2020

Marwan99 Sep 14, 2020

SteveMacenski Sep 14, 2020

Marwan99 Sep 14, 2020

SteveMacenski Sep 14, 2020

SteveMacenski Sep 14, 2020

Marwan99 Sep 14, 2020

Marwan99 commented Sep 14, 2020

Marwan99 commented Sep 14, 2020

maximilianwulf left a comment

maximilianwulf Sep 24, 2020

Marwan99 Sep 25, 2020

maximilianwulf Sep 24, 2020

maximilianwulf Sep 24, 2020

Marwan99 Sep 24, 2020

maximilianwulf Sep 24, 2020

maximilianwulf Sep 24, 2020

maximilianwulf Sep 24, 2020

maximilianwulf Sep 24, 2020

ghost commented Sep 25, 2020 •

edited by ghost

Loading

Marwan99 commented Sep 25, 2020

Marwan99 commented Sep 27, 2020

maximilianwulf commented Sep 27, 2020

maximilianwulf left a comment

Marwan99 commented Sep 28, 2020

maximilianwulf commented Sep 29, 2020

Marwan99 commented Sep 29, 2020

Marwan99 commented Sep 29, 2020

SteveMacenski commented Sep 29, 2020

SteveMacenski commented Sep 29, 2020

Marwan99 commented Sep 29, 2020

[ROS2] grid_map_demos port 1/3 #259

[ROS2] grid_map_demos port 1/3 #259

Conversation

Marwan99 commented Sep 13, 2020 • edited Loading

SteveMacenski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marwan99 commented Sep 14, 2020

Marwan99 commented Sep 14, 2020

maximilianwulf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 25, 2020 • edited by ghost Loading

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Marwan99 commented Sep 25, 2020

Marwan99 commented Sep 27, 2020

maximilianwulf commented Sep 27, 2020

maximilianwulf left a comment

Choose a reason for hiding this comment

Marwan99 commented Sep 28, 2020

maximilianwulf commented Sep 29, 2020

Marwan99 commented Sep 29, 2020

Marwan99 commented Sep 29, 2020

SteveMacenski commented Sep 29, 2020

SteveMacenski commented Sep 29, 2020

Marwan99 commented Sep 29, 2020

Marwan99 commented Sep 13, 2020 •

edited

Loading

ghost commented Sep 25, 2020 •

edited by ghost

Loading