Feature/open planner dev #1400

hatem-darweesh · 2018-07-27T01:50:48Z

Status

** DEVELOPMENT**

Description

Add OpenPlanner new Modifications and Tools to Autoware develop branch.

OpenPlanner - Global Planner
OpenPlanner - Local Planner
OpenPlanner - Utilities
OpenPlanner - Simulator
lidar_kf_contour_track
Remove Old OpenPlanner (dp_planner, way_planner)
Add readme for each module

**For More Details about the updated OpenPlanner, interface description and how to test **
OpenPlanner Nodes.pdf

Steps to Test or Reproduce

Should produce same behaviour as previous (way_planner, dp_planner).
Should show same output as Demo Video
Basic Functions To Test:

OpenPlanner - Global planner should load map successfully and generate global path from start to goal points specified by rviz or from .csv file.
OpenPlanner - Local Planner should generate local trajectories along the global path, follow other simulated vehicles, avoid obstacles, stop for stop lines, and stop in traffic lights if light is red.
OpenPlanner - Utilities should be able to play rosbag file including localization, pause, play backwards, and publish TF for ndt_pose
OpenPlanner - Simulator should simulate a vehicle moving with specific speed from start to goal position, simulate simple intersection traffic lights, and simulate perception by generating cloud_clusters for simulate vehicles.
lidar_kf_contour_track should track detected objects, find heading and velocity for objects detected by lidar_euclidean_cluster or op_perception_simulator.

Follow README files and Demo Video

Update ring ground filter with latest implementation Update lidar_kf_contour_track with latest implementation Add op_utilities nodes (op_bag_player, op_data_logger, op_pose2tf) Modify autoware_msgs for OpenPlanner use (CloudCluster, DetectedObject, lane, waypoint) Update UI computing.yaml for the new nodes and modifies parameters Update UI sensing.yaml for updated ring_ground_filter params

Update MappingHelpers with latest modifications Update PlanningHelpers with latest modifications add op_common_param node, for setting OpenPlanner parameter for all related nodes such as lidar_kf_contour_track Improve tracking by including size different in association function Update way_planner, dp_planner for compatibility with new Mapping Modifications, Map format is backward compatible

Fix Global Planning function for the new map modification Add OpenPlanner Simulator for perception, traffic lights, cars Add OpenPlanner new version to replace wp_planner and dp_planner Remove unnecessary files from OpenPlanner libraries Test Global and Local planning Test Tracking node (kf_contour_track) Test Simulation Nodes Test Utility Nodes

… map messages, old behavior still works but from launch file only. Delete way_planner, dp_planner from UI, but they still accessible from roslaunch.

Fix Simulated Vehicle Initialization Test Following Test Obstacle Avoidance Add Visualization information to rviz config file open_planner.rviz

…oware into feature/OpenPlanner-dev

kitsukawa · 2018-08-04T05:34:27Z

ros/src/util/packages/runtime_manager/scripts/computing.yaml

+        desc : Simulate traffic Signs status for intersections included in the map
+        cmd  : roslaunch op_simulation_package op_signs_simulator.launch
+        param: op_signs_simulator
+      - name : op_car_simulator_1


@hatem-darweesh There are 5 checkboxes for op_car_simulator. This should be only one here.

@hatem-darweesh How about @kitsukawa's comment?

@kitsukawa @kfunaoka Only 1 check box for simulated car remaining, I will add multi car simulation option in "Simulation" tab later.

kfunaoka

@hatem-darweesh I've added some comments, but it is difficult to review whole because the diff is very wide range. Would you list reasons why these changes are necessary for each items with comparing with the old implementation?

OpenPlanner - Global planner
OpenPlanner - Local Planner
OpenPlanner - Utilities
OpenPlanner - Simulator
lidar_kf_contour_track

kfunaoka · 2018-08-15T07:00:30Z

.../computing/perception/detection/lidar_tracker/packages/lidar_kf_contour_track/CMakeLists.txt

  )

-
-set(CMAKE_CXX_FLAGS "-O2 -Wall ${CMAKE_CXX_FLAGS}")


Is this deletion intended?

I saw a PR which adds common compiler flags to all cmake files "autoware_build_flags". so I deleted the special ones

Thanks! I should accomplish autoware_build_flags because it sets only -std=c++11 now.

The other flags are needed only locally for debugging and performance test so I don't want to push them to the server

kfunaoka · 2018-08-15T07:28:07Z

...erception/detection/lidar_tracker/packages/lidar_kf_contour_track/include/PolygonGenerator.h

@@ -0,0 +1,95 @@
+


Is it possible to distribute this file under BSD 3-Clause "New" or "Revised" License?
If yes, please insert the license terms.

can you give me an example to follow.

This is the license.
https://github.com/CPFL/Autoware/blob/master/LICENSE

An example is here. The year 2015 in it should be replaced with 2018 this year.
https://github.com/CPFL/Autoware/blob/develop/ros/src/computing/perception/detection/vision_detector/packages/vision_ssd_detect/src/vision_ssd_detect.cpp

kfunaoka · 2018-08-15T07:33:28Z

...erception/detection/lidar_tracker/packages/lidar_kf_contour_track/include/PolygonGenerator.h

+	PlannerHNS::WayPoint max_from_center;
+	bool bFirst;
+
+	QuarterView(const int& min_a, const int& max_a, const int& index)


It is better to use int or const int instead of const int&.

it is less confusing to use one parameter format for both objects and standard types.
and it will be really dangerous and will take plenty of time and testing to change it through the whole OpenPlanner library.

Thanks. I think so too in the point of view where it takes much time and dangerous. I think it is better to be as is in this pull request.

But using const int & instead of int simply incurs piled-up overhead. I think C++ programmers do not confuse about it because they should know C and C++ basically employs pass by value.

kfunaoka · 2018-08-15T07:38:22Z

...erception/detection/lidar_tracker/packages/lidar_kf_contour_track/include/PolygonGenerator.h

+
+	bool UpdateQuarterView(const PlannerHNS::WayPoint& v)
+	{
+		if(v.pos.a <= min_ang || v.pos.a > max_ang)


It seems that clang-format was not used. Did you use clang-format? If no, please use clang-format to all the files you modified. (https://github.com/CPFL/Autoware/wiki/Contribution-Rules#ros-c-coding)

I will apply it later with different PR, after merging this one.

@hatem-darweesh Your choice is the best method.

kfunaoka · 2018-08-15T07:46:22Z

...g/perception/detection/lidar_tracker/packages/lidar_kf_contour_track/include/SimpleTracker.h

+namespace ContourTrackerNS
+{
+
+#define DEBUG_TRACKER 0


It's better to use constexpr instead of #define. (https://en.cppreference.com/w/cpp/language/constexpr)

Can we add this as ToDo, I need to check for each #define whether it is a compiler directive or a constant. so it will take long time.

@hatem-darweesh NP :)

kfunaoka · 2018-08-15T08:16:22Z

...computing/planning/common/lib/openplanner/op_planner/include/op_planner/BehaviorPrediction.h

+			f.pTrack->UpdatePathAndIndex(obj.predTrajectories.at(f.new_index), f.new_index);
+
+			bool bFound = false;
+			for(unsigned int k=0; k < check_list.size(); k++)


It's better to use standard functions. It seems that this loop can be replaced by std::find.

personal preference

@hatem-darweesh NP. But std::find can reduce the number of lines 1/9 (one ninth) where we develop, review, and maintain. If there are ten loops finding an element, it can reduce 81 (= 90 - 9) lines in the Autoware code base. If you like it, please use it :)

The number of lines of this loop is 9 (from Line 688 to 695).

The number of lines of std::find is 1.

kfunaoka · 2018-08-15T08:18:38Z

...ar_tracker/packages/lidar_kf_contour_track/nodes/lidar_kf_contour_track/PolygonGenerator.cpp

+
+std::vector<PlannerHNS::GPSPoint> PolygonGenerator::EstimateClusterPolygon(const pcl::PointCloud<pcl::PointXYZ>& cluster, const PlannerHNS::GPSPoint& original_centroid, PlannerHNS::GPSPoint& new_centroid, const double& polygon_resolution)
+{
+	for(unsigned int i=0; i < m_Quarters.size(); i++)


You can use range-for.
https://en.cppreference.com/w/cpp/language/range-for

personal preference

Same as #1400 (comment)

kfunaoka · 2018-08-15T08:25:13Z

...rc/computing/planning/common/lib/openplanner/op_planner/include/op_planner/PlanningHelpers.h


 #ifndef PLANNINGHELPERS_H_
 #define PLANNINGHELPERS_H_

-#include <math.h>
+//#include <math.h>


Should be deleted. Unnecessary comments will be junk.

kfunaoka · 2018-08-15T08:45:38Z

ros/src/computing/planning/common/lib/openplanner/op_planner/src/BehaviorPrediction.cpp

+	}
+}
+
+int BehaviorPrediction::FromIndicatorToNumber(const PlannerHNS::LIGHT_INDICATOR& indi)


Why are FromIndicatorToNumber and FromNumbertoIndicator necessary? Is it impossible to use enum value itself?

the compiler give me an error casting from int to the enumerator, so I create a function to convert from one to another and the vice versa , just to be consistent

@hatem-darweesh Is it just a design problem or an unavoidable problem? Basically, it seems that it is not a good idea to have the same type of data (Particle::indicator and the data returned from BehaviorPrediction::FromNumbertoIndicator) with different types (int and lannerHNS::LIGHT_INDICATOR, respectively).

I see. Particle::indicator is defined as int in autoware_msgs::DetectedObject. It is difficult to avoid.

kfunaoka · 2018-08-15T08:46:59Z

ros/src/computing/planning/common/lib/openplanner/op_planner/src/BehaviorPrediction.cpp

+double BehaviorPrediction::CalcIndicatorWeight(PlannerHNS::LIGHT_INDICATOR p_ind, PlannerHNS::LIGHT_INDICATOR obj_ind)
+{
+	if((obj_ind == PlannerHNS::INDICATOR_LEFT || obj_ind == PlannerHNS::INDICATOR_RIGHT || obj_ind == PlannerHNS::INDICATOR_NONE) && p_ind == obj_ind)
+		return 0.99;


Please remove magic numbers as much as possible.

This part (Prediction) is under development (research work) , all numbers will be removed or added as parameters soon.

kfunaoka · 2018-08-15T09:18:13Z

@hatem-darweesh My comments are general principles. Would you try to apply the principles to the other places?

kfunaoka · 2018-08-15T09:23:48Z

ros/src/computing/planning/mission/packages/op_global_planner/package.xml

+  <!--   <run_depend>message_runtime</run_depend> -->
+  <!-- Use test_depend for packages you need only for testing: -->
+  <!--   <test_depend>gtest</test_depend> -->
+  <buildtool_depend>catkin</buildtool_depend>


Please add buildtool_depend of autoware_build_flags.
https://github.com/CPFL/Autoware/wiki/Quality-Control

kfunaoka · 2018-08-15T09:24:10Z

ros/src/computing/planning/mission/packages/op_global_planner/package.xml

+  <version>0.0.1</version>
+  <description>The OpenPlanner Global Planning package</description>
+
+  <!-- One maintainer tag required, multiple allowed, one person per tag --> 


Please delete unnecessary comments.

kfunaoka · 2018-08-15T09:25:16Z

ros/src/computing/planning/mission/packages/op_global_planner/CMakeLists.txt

+project(op_global_planner)
+
+find_package(catkin REQUIRED COMPONENTS
+  geometry_msgs


Please add autoware_build_flags.
https://github.com/CPFL/Autoware/wiki/Quality-Control

kfunaoka · 2018-08-15T10:32:13Z

Sorry for mistake.

It's better to use clang format after all codes are reviewed because it make the history unreadable.

pass pointer as "const" for function "DeleteFromList" in both (BehaviorPrediction and LLP) classes

Delete useless comment for including math.h

kfunaoka

@hatem-darweesh Though I've made a lot of comments, my mandatory requests now are:

Inserting license information into *.{h,cpp} files as mentioned at Feature/open planner dev #1400 (comment).
Inserting autoware_build_flags into package.xml and CMakeLists.txt as shown at Feature/open planner dev #1400 (review)
Updating the headline of this pull request to show what are different from before and why they are necessary. Your slide and descriptions will help us to understand your work.

kfunaoka · 2018-08-16T03:49:22Z

@hatem-darweesh Your work is great. To approve this pull request, please be reviewed by aohsato from other aspects. I think I am not the best person who reviews this pull request.

In your future work, it's better to make pull requests as accordingly-small as possible. Stepwise pull requests will help reviewers. Enough review will improve your work more and more. I think small feedback loop is better than large feedback loop for you and for reviews.

kfunaoka · 2018-08-16T04:03:29Z

I've added the following item into my review comment (#1400 (review)).

Inserting autoware_build_flags into package.xml and CMakeLists.txt as shown at Feature/open planner dev #1400 (review)

…esting Only one simulated car available in the runtime manager update for copywrite note insert autoware_build_flags to new nodes

hatem-darweesh · 2018-08-21T10:12:51Z

@kfunaoka can you check your review comments and current changes.

kfunaoka

@hatem-darweesh I've confirmed that all my mandatory request are OK. I might find an item that you want to change.

kfunaoka · 2018-08-22T05:41:23Z

.../computing/perception/detection/lidar_tracker/packages/lidar_kf_contour_track/CMakeLists.txt

  )

-
-set(CMAKE_CXX_FLAGS "-O2 -Wall ${CMAKE_CXX_FLAGS}")
+find_package(OpenCV REQUIRED)



- set(CMAKE_CXX_FLAGS "-O2 -Wall ${CMAKE_CXX_FLAGS}")

Should we revert this line because you said

The other flags are needed only locally for debugging and performance test so I don't want to push them to the server

@kfunaoka I will add to different PR if necessary

All my mandatory request are OK

…ner-dev Feature/open planner dev

hatem-darweesh added 12 commits July 2, 2018 20:41

Modify Map loading for OpenPlanner, now it reads from Autoware vector…

5d3169b

… map messages, old behavior still works but from launch file only. Delete way_planner, dp_planner from UI, but they still accessible from roslaunch.

Test Simulated Vehicles

a426f7a

Fix Simulated Vehicle Initialization Test Following Test Obstacle Avoidance Add Visualization information to rviz config file open_planner.rviz

Add ReadMe to lidar_kf_contour_track node

526720e

Update README.md

0d22a39

Add README files for OpenPlanner packages

1c011af

Merge branch 'feature/OpenPlanner-dev' of https://github.com/CPFL/Aut…

e64bced

…oware into feature/OpenPlanner-dev

Update README.md

485e51a

Update README.md

ee9e360

Merge branch 'develop' into feature/OpenPlanner-dev

3d9acbc

hatem-darweesh assigned kitsukawa Jul 27, 2018

kfunaoka mentioned this pull request Aug 1, 2018

[fix] PascalCase messages #1408

Merged

kfunaoka assigned aohsato Aug 1, 2018

hatem-darweesh and others added 3 commits August 1, 2018 22:26

Fix Indigo build issues

0955298

fix dependencies to make industrial_ci test pass

112006d

Merge branch 'develop' into feature/OpenPlanner-dev

1dec531

kitsukawa suggested changes Aug 4, 2018

View reviewed changes

kfunaoka reviewed Aug 15, 2018

View reviewed changes

kfunaoka closed this Aug 15, 2018

kfunaoka reopened this Aug 15, 2018

hatem-darweesh added 3 commits August 15, 2018 23:30

Merge branch 'develop' into feature/OpenPlanner-dev

e42c128

Update BehaviorPrediction.h

4476edb

pass pointer as "const" for function "DeleteFromList" in both (BehaviorPrediction and LLP) classes

Update PlanningHelpers.h

2b5b952

Delete useless comment for including math.h

kfunaoka previously requested changes Aug 16, 2018

View reviewed changes

hatem-darweesh added 3 commits August 19, 2018 20:59

Support old behavior of insert static object for obstacle avoidance t…

01f2ba7

…esting Only one simulated car available in the runtime manager update for copywrite note insert autoware_build_flags to new nodes

Merge branch 'develop' into feature/OpenPlanner-dev

5d1f7ac

Merge develop into OpenPlanner-dev

0261672

kitsukawa approved these changes Aug 21, 2018

View reviewed changes

kfunaoka reviewed Aug 22, 2018

View reviewed changes

hatem-darweesh merged commit 875625a into develop Aug 23, 2018

hatem-darweesh deleted the feature/OpenPlanner-dev branch August 23, 2018 03:06

kfunaoka mentioned this pull request Sep 6, 2018

Feature/beyond pixel tracker #1473

Merged

2 tasks

hatem-darweesh mentioned this pull request Dec 4, 2018

[Discussion] Planning Architecture autowarefoundation/autoware_ai#419

Closed

gbiggs unassigned aohsato and kitsukawa Mar 25, 2019

anubhavashok pushed a commit to NuronLabs/autoware.ai that referenced this pull request Sep 7, 2021

Merge pull request autowarefoundation#1400 from CPFL/feature/OpenPlan…

f64d0c6

…ner-dev Feature/open planner dev

mitsudome-r added the version:autoware-ai Autoware.AI label Jun 14, 2022

Feature/open planner dev #1400

Feature/open planner dev #1400

Conversation

hatem-darweesh commented Jul 27, 2018 • edited Loading

Status

Description

Steps to Test or Reproduce

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfunaoka left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfunaoka Aug 15, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hatem-darweesh Aug 15, 2018 • edited Loading

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

kfunaoka commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfunaoka commented Aug 15, 2018

kfunaoka left a comment • edited Loading

Choose a reason for hiding this comment

kfunaoka commented Aug 16, 2018

kfunaoka commented Aug 16, 2018

hatem-darweesh commented Aug 21, 2018

kfunaoka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hatem-darweesh commented Jul 27, 2018 •

edited

Loading

kfunaoka left a comment •

edited

Loading

kfunaoka Aug 15, 2018 •

edited

Loading

hatem-darweesh Aug 15, 2018 •

edited

Loading

kfunaoka left a comment •

edited

Loading