-
Notifications
You must be signed in to change notification settings - Fork 490
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
Add URDF Loader Exceptions and Fix Infinite While-Loop when URDF file isn't in a ROS Package #2032
Add URDF Loader Exceptions and Fix Infinite While-Loop when URDF file isn't in a ROS Package #2032
Conversation
…andling to start screen widget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me. Thanks for adding in all these safety rails!
I have cosmetic suggestions about the error messages, but that's about it.
moveit_setup_assistant/moveit_setup_core_plugins/src/start_screen_widget.cpp
Outdated
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_core_plugins/src/start_screen_widget.cpp
Outdated
Show resolved
Hide resolved
Oh also, you need to go through the formatting failure ( https://github.com/ros-planning/moveit2/actions/runs/4473208743/jobs/7868393023?pr=2032#step:12:9505 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the fix! My request about using std::optional is a nice to have, I'd go with either version.
@@ -53,7 +53,8 @@ bool extractPackageNameFromPath(const std::filesystem::path& path, std::string& | |||
} | |||
|
|||
// truncate path step by step and check if it contains a package.xml | |||
while (!sub_path.empty()) | |||
// This runs until the path is either empty "" or at the root "/" or "C:\\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch! great find
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2032 +/- ##
==========================================
- Coverage 50.91% 50.84% -0.06%
==========================================
Files 374 391 +17
Lines 31362 32147 +785
==========================================
+ Hits 15964 16343 +379
- Misses 15398 15804 +406
... and 16 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Increase priority for constrained planning state space (moveit#1300) * Change priority for the constrained planning state space * Fix constrained planning tests * Use PRM instead of RRTConnect --------- Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai> Remove "new" from smart pointer instantiation (moveit#2019) Temporarily disable TestPathConstraints with the Panda robot (moveit#2016) This test has become flaky since it was modified to use the OMPL constrained state space (moveit#2015). Fix mimic joints with TOTG (moveit#1989) Fix include install destination (moveit#2008) Co-authored-by: Henning Kayser <henningkayser@picknik.ai> Co-authored-by: Tyler Weaver <maybe@tylerjw.dev> Ruckig-smoothing : reduce number of duration extensions (moveit#1990) * extend duration only for failed segment * update comment * Remove trajectory reset before extension * readability improvement * Remove call to RobotState update --------- Co-authored-by: ibrahiminfinite <ibrahimjkd@@gmail.com> Co-authored-by: AndyZe <andyz@utexas.edu> Add stale GHA (moveit#2022) * Issues and PRs are labeled as stale after 45 days. * Stale issues are closed after another 45 days. Enable workflow_dispatch for stale GHA Remove invalid description field in GHA Add callback for velocity scaling override + fix params namespace not being set (moveit#2021) Fix python tests (moveit#1979) * ensure joint models in robot_model submodule * add build tests Upgrade apt dependencies --with-new-pkgs (moveit#2039) 2.7.1 🛠️ Bump actions/stale from 7 to 8 (moveit#2037) Allow ci-testing Dockerfile to update the ROS_DISTRO (moveit#2035) Move displaced launch file into planning_component_tools (moveit#2044) Delete the Ruckig "batches" option, deprecated by moveit#1990 (moveit#2028) Added set_robot_trajectory_msg to python bindings (moveit#2050) Use $DISPLAY rather than assuming :0 (moveit#2049) * Use $DISPLAY rather than assuming : * Double quotes --------- Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai> Optionally mitigate Ruckig overshoot (moveit#2051) * Optionally mitigate Ruckig overshoot * Cleanup Update description of moveit_ros_planning_interface (moveit#2045) * Update description of moveit_ros_planning_interface * Update moveit_ros/planning_interface/package.xml Co-authored-by: Henning Kayser <henningkayser@picknik.ai> --------- Co-authored-by: Henning Kayser <henningkayser@picknik.ai> Add URDF Loader Exceptions and Fix Infinite While-Loop when URDF file isn't in a ROS Package (moveit#2032) * Fixed infinite while loop in utilities.cpp and added some exception handling to start screen widget * Fix trailing whitespace, fix getSharePath exception catch on empty request * Fix clang tidy suggestion and error message updates based on pr comments [hybrid planning] improve planning scene monitoring (moveit#1090) * Create new PSM in local constraint solver. Different type of collision checking. * Small boolean logic fixup * Don't configure planning scene monitor twice and pass scene as const * Remove not required call of updateSceneWithCurrentState() * Revert PlanningSceneMonitorConstPtr to PlanningSceneMonitorPtr * Set planning_scene_monitor update rate * Decrease planning scene update rate * Use `updateSceneWithCurrentState()` in psm * Revert the manner of collision checking --------- Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai> Document how collision checking includes descendent links (moveit#2058) Move stateless PlanningScene helper functions out of the class (moveit#2025) Readability: kinematic_state -> robot_state (moveit#2078) moveit_py citation (moveit#2029) Extract parallel planning from moveit cpp (moveit#2043) * Add parallel_planning_interface * Add parallel planning interface * Rename package to pipeline_planning_interface * Move plan_responses_container into own header + source file * Add plan_responses_contrainer source file * Add solution selection and stopping criterion function files * Remove parallel planning from moveit_cpp * Move parallel planning into planning package * Update moveit_cpp * Drop planning_interface changes * Add documentation * Update other moveit packages * Remove removed header * Address CI complains * Address clang-tidy complains * Address clang-tidy complains 2 * Address clang-tidy complains 3 * Extract planning pipeline map creation function from moveit_cpp * Cleanup comment * Use const moveit::core::RobotModelConstPtr& * Formatting * Add header descriptions * Remove superfluous TODOs * Cleanup PILZ: Throw if IK solver doesn't exist (moveit#2082) * Throw if IK solver doesn't exist * Format enabled -wformat (moveit#2065) Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai> Add test and debug issue where TOTG returns accels > limit (moveit#2084) lint fix lint fix 1 lint fix 2 lint fix 3
The exceptions introduced with moveit#2032 prevented from running the collisions updater CLI without a ROS package. This fix makes ROS packages optional again, allowing to use the CLI with absolute paths only.
* Fix collisions_updater CLI if no package is used The exceptions introduced with #2032 prevented from running the collisions updater CLI without a ROS package. This fix makes ROS packages optional again, allowing to use the CLI with absolute paths only. * Improve warn message wording
* Fix collisions_updater CLI if no package is used The exceptions introduced with #2032 prevented from running the collisions updater CLI without a ROS package. This fix makes ROS packages optional again, allowing to use the CLI with absolute paths only. * Improve warn message wording (cherry picked from commit 16ac53c)
) * Fix collisions_updater CLI if no package is used The exceptions introduced with #2032 prevented from running the collisions updater CLI without a ROS package. This fix makes ROS packages optional again, allowing to use the CLI with absolute paths only. * Improve warn message wording (cherry picked from commit 16ac53c) Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
Description
This PR fixes Issue 1944. When selecting a URDF from a file path, MSA would hang forever before needing to be killed if the URDF was not in a ROS package; with no error messages printed. This was due to an infinite while-loop in the moveit_setup_framework/src/utilities.cpp file when extracting the package name from the file path. This fixes that while-condition to check against the root file path when truncating the path. Also added a catch so if ament throws an error finding the package it will just return an empty filepath instead.
This PR also adds some error handling to the start screen widget to propagate URDF loading errors to the user via the GUI, rather than just crash. Previous behavior was that when selecting an invalid URDF, the MSA tool crashed and the ROS logs displayed the exception.
Partially based off of initial work done by @mikeferguson
Checklist