Add moveit_configs_utils package to simplify loading paramters by JafarAbdi · Pull Request #591 · moveit/moveit2

JafarAbdi · 2021-08-03T22:53:00Z

Description

Second attempt to simplify loading parameters

This's based on JafarAbdi/moveit_resources@44a82c4

Replace: #209

Checklist

Required by CI: Code is auto formatted using clang-format
Extend the tutorials / documentation reference
Document API changes relevant to the user in the MIGRATION.md notes
Create tests, which fail without this PR reference
Include a screenshot if changing a GUI
While waiting for someone to review your request, please help review another open pull request to support the maintainers

AdamPettinger

Overall I like this. I think there are pros/cons about this style of implementation but this (as written now) is already better than the current way of doing the same configs setup in every launch file

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

moveit_demo_nodes/run_move_group/launch/run_move_group_interface.launch.py

tylerjw · 2021-08-04T15:33:43Z

I like the pattern here. I wonder if there is a way to make it more generic so the sytax was more like:

parameters = ParameterBuilder('robot_config_package')
  .yaml('moveit_cpp.yaml')                                        # loads a yaml file into parameters
  .file_parameter('robot_description', 'xacro/panda.urdf.xacro')  # fills out full path based on package
  .parameter('planning_timeout', 0.1)                             # sets a parameter
  .to_dict()

Then it could be used for any sort of parameters, not just a moveit config. Then maybe there could be a moveit config builder that used the ParameterBuilder and was just a function or something like that.

vatanaksoytezer

Overall I like it that it is much more smaller, to be used around moveit. What I didn't like was this assumes default locations for some files such as semantic, kinematic, joint_limits, which can be hard to understand if you are looking at this for the first time and not familiar with moveit. I feel making all the parameters yaml is great and at least for example-wise specifying file names here would be more instructive and easier to understand imo.

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

AdamPettinger · 2021-08-05T16:16:44Z

I like the pattern here. I wonder if there is a way to make it more generic so the sytax was more like:
parameters = ParameterBuilder('robot_config_package')
  .yaml('moveit_cpp.yaml')                                        # loads a yaml file into parameters
  .file_parameter('robot_description', 'xacro/panda.urdf.xacro')  # fills out full path based on package
  .parameter('planning_timeout', 0.1)                             # sets a parameter
  .to_dict()
Then it could be used for any sort of parameters, not just a moveit config. Then maybe there could be a moveit config builder that used the ParameterBuilder and was just a function or something like that.

I like this a lot. It would let you do multiple inside one launch file (e.g. when getting parameters from multiple packages), and would also let you use it to return a specific set of all parameters for a config package (e.g. moveit_configs, or if some other user wants to have their own bringup/config package they could provide a getPackageConfigs() call)

moveit_configs_utils/src/moveit_configs_utils/moveit_configs_builder.py

moveit_configs_utils/setup.py

moveit_configs_utils/package.xml

moveit_ros/moveit_servo/test/launch/unit_test_servo_calcs.test.py

henningkayser

This is awesome, all launch files look so much cleaner! I'm just not sure if we should remove the default values from the builder functions.

Some thought: Let's say all moveit config packages provide python scripts that export their custom moveit configs. Couldn't we then simply build config hierarchies by taking another included moveit config as default and just modifying specific parts, say the kinematics yaml or something else? Given this PR it seems like a really small step to get this done. The only thing we need to watch out for is circular dependencies.

moveit_configs_utils/src/moveit_configs_utils/moveit_configs_builder.py

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

moveit_configs_utils/src/moveit_configs_utils/moveit_configs_builder.py

jrgnicho · 2021-08-26T22:56:10Z

moveit_configs_utils/src/moveit_configs_utils/moveit_configs_builder.py

+        self.__moveit_configs.robot_description_kinematics = {
+            self.__robot_description
+            + "_kinematics": load_yaml(
+                self._package_path / (file_path or "config/kinematics.yaml")


Perhaps these relative path strings should be stored in class variables

You mean config/kinematics.yaml right .? I'll update the class to have them

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

jrgnicho · 2021-08-26T23:06:25Z

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

+            moveit_config.robot_description_semantic,
+            moveit_config.robot_description_kinematics,
+            moveit_config.planning_pipelines,
+            moveit_config.joint_limits,


I assume rviz needs these parameters for the moveit planning panel correct?

Probably out of the scope of this PR but it would be really nice if the rviz plugin had the option of being pointed at the ros node that had the moveit config loaded in it (maybe even as something that could be changed in the gui) and it would use the get_parameters service that ros starts up to read the moveit config from that other node.

jrgnicho · 2021-08-26T23:43:52Z

I really like what's on here too. Coincidentally I had started working on something similar in order to port the ros-industrial pick and place demo and ended up creating a MoveItConfigHelper python class that looks a lot like what's on here although it isn't as polished. Eventually I was going to pull it out of the launch file and provide it in its own stand-alone package but since that's already being done here then I'll just wait until this PR gets merged. One thing I did that I did not see here is to parse the list of ros controllers from the yaml and provide them as another field in the MoveItConfig. I then use the list to spawn the controllers; I feel this could be done here too.

moveit_configs_utils/src/moveit_configs_utils/moveit_configs_builder.py

moveit2_rolling.repos

tylerjw

Overall I really like this PR. I just left a couple of comments.

moveit2_galactic.repos

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

tylerjw · 2021-09-09T21:22:31Z

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

+            moveit_config.robot_description_semantic,
+            moveit_config.robot_description_kinematics,
+            moveit_config.planning_pipelines,
+            moveit_config.joint_limits,


Probably out of the scope of this PR but it would be really nice if the rviz plugin had the option of being pointed at the ros node that had the moveit config loaded in it (maybe even as something that could be changed in the gui) and it would use the get_parameters service that ros starts up to read the moveit config from that other node.

codecov · 2021-09-10T02:33:01Z

Codecov Report

Merging #591 (28b6849) into main (4c75dcf) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
+ Coverage   57.96%   57.98%   +0.02%     
==========================================
  Files         309      309              
  Lines       26120    26120              
==========================================
+ Hits        15138    15143       +5     
+ Misses      10982    10977       -5

Impacted Files	Coverage Δ
...nning_scene_monitor/src/planning_scene_monitor.cpp	`53.90% <0.00%> (+0.59%)`	⬆️

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 4c75dcf...28b6849. Read the comment docs.

…er.py Co-authored-by: AndyZe <andyz@utexas.edu>

mergify · 2022-01-01T23:18:08Z

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

stephanie-eng

Small nit - some comments reference robot_name_moveit_configs as the default package name, which caused me a bit of confusion.

moveit_configs_utils/moveit_configs_utils/moveit_configs_builder.py

stephanie-eng · 2022-01-12T23:14:38Z

moveit_configs_utils/moveit_configs_utils/moveit_configs_builder.py

+        self.__planning_scene_monitor = value
+
+    @property
+    def move_group_capabilities(self):


Add move group capabilities loader function

stephanie-eng · 2022-01-12T23:28:17Z

moveit_configs_utils/moveit_configs_utils/moveit_configs_builder.py

+        return self
+
+    def robot_description_semantic(
+        self, file_path: Optional[str] = None, mappings: dict = None


Support passing LaunchConfiguration as a mapping

Co-authored-by: Stephanie Eng <stephanie-eng@users.noreply.github.com>

AndyZe · 2022-01-14T17:59:57Z

I just spot-checked this on some of the tutorials. It still works well 👍

We still haven't tested it on a dual-arm system due to some unexpected issues, but I don't want to block a great PR forever. We can always fix up whatever issues emerge.

moveit_configs_utils/moveit_configs_utils/moveit_configs_builder.py

Co-authored-by: AndyZe <zelenak@picknik.ai> Co-authored-by: Stephanie Eng <stephanie-eng@users.noreply.github.com> Co-authored-by: Tyler Weaver <tyler@picknik.ai> (cherry picked from commit 79e3fd7)

…ort #591) (#1019) Co-authored-by: AndyZe <zelenak@picknik.ai> Co-authored-by: Stephanie Eng <stephanie-eng@users.noreply.github.com> Co-authored-by: Tyler Weaver <tyler@picknik.ai>

moveit#591)" This reverts commit 79e3fd7.

JafarAbdi mentioned this pull request Aug 3, 2021

[WIP/For discussion]: Including parameters in python based launch file #209

Closed

JafarAbdi requested review from AdamPettinger, henningkayser, tylerjw and vatanaksoytezer August 3, 2021 22:56

AdamPettinger reviewed Aug 4, 2021

View reviewed changes

vatanaksoytezer reviewed Aug 4, 2021

View reviewed changes

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py Outdated Show resolved Hide resolved

JafarAbdi force-pushed the pr-moveit_configs_utils branch from befee65 to d48bbe6 Compare August 13, 2021 23:00

JafarAbdi commented Aug 13, 2021

View reviewed changes

JafarAbdi changed the title ~~[WIP]: Use moveit_configs_utils for run_move_group~~ Add moveit_configs_utils package to simplify loading paramters Aug 13, 2021

henningkayser reviewed Aug 16, 2021

View reviewed changes

jrgnicho reviewed Aug 26, 2021

View reviewed changes

moveit_configs_utils/src/moveit_configs_utils/moveit_configs_builder.py Outdated Show resolved Hide resolved

jrgnicho reviewed Aug 26, 2021

View reviewed changes

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py Outdated Show resolved Hide resolved

jrgnicho reviewed Aug 26, 2021

View reviewed changes

sjahr reviewed Aug 30, 2021

View reviewed changes

moveit_configs_utils/src/moveit_configs_utils/moveit_configs_builder.py Outdated Show resolved Hide resolved

JafarAbdi commented Sep 9, 2021

View reviewed changes

moveit2_rolling.repos Outdated Show resolved Hide resolved

JafarAbdi force-pushed the pr-moveit_configs_utils branch 2 times, most recently from 42272f7 to 79ccfb4 Compare September 9, 2021 19:38

tylerjw approved these changes Sep 9, 2021

View reviewed changes

JafarAbdi force-pushed the pr-moveit_configs_utils branch 2 times, most recently from a0dc18a to acfdc5a Compare September 9, 2021 23:52

JafarAbdi marked this pull request as draft September 10, 2021 00:18

JafarAbdi force-pushed the pr-moveit_configs_utils branch from de87648 to 518aa15 Compare September 10, 2021 01:19

JafarAbdi marked this pull request as ready for review September 10, 2021 01:49

JafarAbdi mentioned this pull request Sep 10, 2021

Only run cache upstream ws if no .repos file changed in PRs #674

Merged

6 tasks

JafarAbdi and others added 2 commits December 31, 2021 09:46

Update moveit_configs_utils/moveit_configs_utils/moveit_configs_build…

2d29ebb

…er.py Co-authored-by: AndyZe <andyz@utexas.edu>

Merge branch 'main' into pr-moveit_configs_utils

14d83dc

Merge branch 'main' into pr-moveit_configs_utils

ee8a636

stephanie-eng reviewed Jan 7, 2022

View reviewed changes

stephanie-eng reviewed Jan 12, 2022

View reviewed changes

AndyZe and others added 2 commits January 14, 2022 11:04

Merge branch 'main' into pr-moveit_configs_utils

ca68801

Rename robot_name_moveit_configs

0159e48

Co-authored-by: Stephanie Eng <stephanie-eng@users.noreply.github.com>

AndyZe approved these changes Jan 14, 2022

View reviewed changes

JafarAbdi mentioned this pull request Jan 17, 2022

Update yaml parameters for moveit_configs_utils moveit/moveit_resources#108

Merged

JafarAbdi and others added 3 commits January 17, 2022 10:50

Merge branch 'main' into pr-moveit_configs_utils

bb87983

Update moveit2.repos

5144564

Merge branch 'main' into pr-moveit_configs_utils

56abbeb

tylerjw reviewed Jan 20, 2022

View reviewed changes

moveit_configs_utils/moveit_configs_utils/moveit_configs_builder.py Outdated Show resolved Hide resolved

Fix package name

28b6849

tylerjw approved these changes Jan 21, 2022

View reviewed changes

tylerjw added backport-foxy Mergify label that triggers a PR backport to Foxy backport-galactic Mergify label that triggers a PR backport to Galactic and removed backport-foxy Mergify label that triggers a PR backport to Foxy labels Jan 21, 2022

tylerjw merged commit 79e3fd7 into moveit:main Jan 21, 2022

mergify bot mentioned this pull request Jan 21, 2022

Add moveit_configs_utils package to simplify loading paramters (backport #591) #1019

Merged

JafarAbdi deleted the pr-moveit_configs_utils branch January 21, 2022 18:48

tylerjw mentioned this pull request Jan 24, 2022

revert test #1028

Closed

6 tasks

tylerjw added a commit to tylerjw/moveit2 that referenced this pull request Jan 24, 2022

Revert "Add moveit_configs_utils package to simplify loading paramters (

a71f80f

moveit#591)" This reverts commit 79e3fd7.

DLu mentioned this pull request Mar 11, 2022

New Launch Files using moveit_configs_utils #1113

Merged

6 tasks

Abishalini mentioned this pull request Sep 14, 2022

Use MoveItConfigsBuilder in Pilz test launch file #1571

Merged

6 tasks

Conversation

JafarAbdi commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

AdamPettinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerjw commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vatanaksoytezer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AdamPettinger commented Aug 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henningkayser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrgnicho Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

JafarAbdi Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrgnicho Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

tylerjw Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

jrgnicho commented Aug 26, 2021

Uh oh!

Uh oh!

Uh oh!

tylerjw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerjw Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mergify bot commented Jan 1, 2022

Uh oh!

stephanie-eng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JafarAbdi commented Aug 3, 2021 •

edited

Loading

tylerjw commented Aug 4, 2021 •

edited

Loading

codecov bot commented Sep 10, 2021 •

edited

Loading