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

[hybrid planning] Delete the pass-through option #986

Merged
merged 6 commits into from
Jan 22, 2022

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Jan 12, 2022

Delete this parameter. On two different projects I have tried, setting pass_through = true has caused the robot to get stuck. I think that's because here in forwardTrajectory::solve():

robot_command.addSuffixWayPoint(local_trajectory.getWayPoint(0), 0.0);

Waypoint index 0 is always used.

Video of enabling pass_through for the demo. The robot gets stuck and the global planner runs repeatedly.

This work is sponsored by RE2 Robotics.

passthrough_stuck

@AndyZe
Copy link
Member Author

AndyZe commented Jan 12, 2022

@sjahr

@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #986 (9a04492) into main (abbd8ff) will decrease coverage by 0.03%.
The diff coverage is 7.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #986      +/-   ##
==========================================
- Coverage   57.99%   57.97%   -0.02%     
==========================================
  Files         309      309              
  Lines       26120    26119       -1     
==========================================
- Hits        15146    15140       -6     
- Misses      10974    10979       +5     
Impacted Files Coverage Δ
..._planner_component/src/local_planner_component.cpp 32.88% <0.00%> (-0.22%) ⬇️
...oveit/trajectory_operator_plugins/simple_sampler.h 100.00% <ø> (ø)
...trajectory_operator_plugins/src/simple_sampler.cpp 17.86% <10.00%> (-5.47%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 74.82% <0.00%> (-1.11%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 53.78% <0.00%> (-0.11%) ⬇️

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 abbd8ff...9a04492. Read the comment docs.

@vatanaksoytezer
Copy link
Contributor

Seems like only clang-tidy is failing:

clang_tidy_check_moveit_hybrid_planning
  Filtering for files that actually changed since main
  run clang-tidy for 1/9 file(s) in 2 process(es).
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:47:63: warning: parameter 'node' is unused [misc-unused-parameters]
  bool SimpleSampler::initialize(const rclcpp::Node::SharedPtr& node, const moveit::core::RobotModelConstPtr& robot_model,
                                                                ^~~~
                                                                 /*node*/
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:47:63: note: FIX-IT applied suggested code changes
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:104:77: warning: parameter 'current_state' is unused [misc-unused-parameters]
  double SimpleSampler::getTrajectoryProgress(const moveit::core::RobotState& current_state)
                                                                              ^~~~~~~~~~~~~
                                                                               /*current_state*/
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:104:77: note: FIX-IT applied suggested code changes
  clang-tidy applied 2 of 2 suggested fixes.
  Suppressed 15185 warnings (15185 in non-user code).

@tylerjw
Copy link
Member

tylerjw commented Jan 13, 2022

Seems like only clang-tidy is failing:

clang_tidy_check_moveit_hybrid_planning
  Filtering for files that actually changed since main
  run clang-tidy for 1/9 file(s) in 2 process(es).
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:47:63: warning: parameter 'node' is unused [misc-unused-parameters]
  bool SimpleSampler::initialize(const rclcpp::Node::SharedPtr& node, const moveit::core::RobotModelConstPtr& robot_model,
                                                                ^~~~
                                                                 /*node*/
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:47:63: note: FIX-IT applied suggested code changes
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:104:77: warning: parameter 'current_state' is unused [misc-unused-parameters]
  double SimpleSampler::getTrajectoryProgress(const moveit::core::RobotState& current_state)
                                                                              ^~~~~~~~~~~~~
                                                                               /*current_state*/
  /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/hybrid_planning/local_planner/trajectory_operator_plugins/src/simple_sampler.cpp:104:77: note: FIX-IT applied suggested code changes
  clang-tidy applied 2 of 2 suggested fixes.
  Suppressed 15185 warnings (15185 in non-user code).

You should be able to quiet those warnings by telling the compiler that those parameters are not used. See: https://en.cppreference.com/w/cpp/language/attributes/maybe_unused

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

Just a couple of questions, otherwise this looks good.

@mergify
Copy link

mergify bot commented Jan 21, 2022

This pull request is in conflict. Could you fix it @AndyZe?

@AndyZe AndyZe merged commit b0e2cd0 into moveit:main Jan 22, 2022
@tylerjw tylerjw mentioned this pull request Jan 24, 2022
6 tasks
tylerjw added a commit to tylerjw/moveit2 that referenced this pull request Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants