Split resources into multiple packages by v4hn · Pull Request #36 · moveit/moveit_resources

v4hn · 2020-07-06T16:04:44Z

This has been overdue for a long time.

The change allows to launch the robots via roslaunch moveit_resources_... demo.launch. This was not possible before because the package contained multiple demo.launch files.
Additionally, all moveit_config's can now be proper packages, which can be opened in the setup assistant.

When porting the moveit_config packages for fanuc and panda, I noticed they were quite outdated, so instead of porting fanuc forward, I regenerated a whole new fanuc configuration.
For panda, I pulled the latest version from the separate repository, adjusting things so that we still use our own panda_description.

This needs a coordinated pull-request in moveit to adapt all paths to the new locations.

This change was named a requirement for moveit/moveit#1893 because Pilz needs to add another robot to the resources for their tests.

v4hn · 2020-07-06T16:07:26Z

This is a huge change and I'm somewhat unsure with how to merge it soon without too much trouble...

Any help to migrate the main repository to the new file locations is very welcome.

tylerjw · 2020-07-06T16:11:59Z

Thank you for doing this. I can help with building a PR to migrate the main repo to point to these new locations. One thing this will be nice for is (as you updated the panda configs) the subframes test should work. Right now it only works with panda_moveit_config which will be a problem when we go to release into Noetic.

v4hn · 2020-07-06T18:02:48Z

I can help with building a PR to migrate the main repo to point to these new locations.

👍

One thing this will be nice for is (as you updated the panda configs) the subframes test should work.

Yes, it is mandatory that this succeeds. I am also rather convinced that migrating everything to use this patch will reveal some flaws in the patch. I can't even build my workspace today because of some new catkin_tools bug...

- package *_description folders

v4hn · 2020-07-07T07:41:47Z

I can't even build my workspace today because of some new catkin_tools bug...

It turned out that the catkin_tools bug was that in case of cyclic dependencies in the workspace it would add a comma-separated string of all involved packages to a list of Package objects.
I pushed a commit to remove the cyclic dependencies (which were introduced by the setup assistant) and the workspace builds now.

Of course, all interfaces still need to be adjusted in the main repo.

fanuc_moveit_config/launch/demo_chomp.launch

v4hn · 2020-07-07T09:55:44Z

Together with moveit/moveit_tutorials#507 and moveit/moveit#2199 I get at least a workspace that builds successfully, but of course most tests are broken.

rhaschke · 2020-07-07T12:30:57Z

It turned out that the catkin_tools bug was that in case of cyclic dependencies in the workspace it would add a comma-separated string of all involved packages to a list of Package objects.

I filed a PR to fix this: catkin/catkin_tools#617

I pushed a commit to remove the cyclic dependencies (which were introduced by the setup assistant).

For me, the workspace (comprising moveit and moveit_resources only), doesn't build when reverting ec4d175. I don't see yet, why the separation of moveit_resources into individual packages should introduce a circular dependency. Where do you see the circular dependency introduced by MSA? Where did you fixed it?

v4hn · 2020-07-07T13:27:41Z

For me, the workspace (comprising moveit and moveit_resources only), doesn't build when reverting ec4d175. I don't see yet, why the separation of moveit_resources into individual packages should introduce a circular dependency. Where do you see the circular dependency introduced by MSA? Where did you fixed it?

This commit was what I was talking about. The assistant generates package.xml files with dependency loops.
From my perspective, committing these additional dependencies is the way to go for moveit_resources.

fanuc_moveit_config/package.xml

jschleicher · 2020-07-07T14:27:12Z

Thank you very much, for pushing this split!
Implements #34

v4hn · 2020-07-07T15:14:02Z

I suggest to extend the comment slightly

I pushed a new commit which also enables the moveit-independent dependencies again.

fanuc_moveit_config/launch/planning_context.launch

panda_moveit_config/launch/move_group.launch

panda_moveit_config/package.xml

fanuc_moveit_config/package.xml

jschleicher · 2020-07-24T14:23:43Z

I just tried to run the tests, but got (probably unrelated?) two failures in MeshFilterTest:
https://gist.github.com/jschleicher/7df2fedd617eec10f88f25ec5ef6aafd
Any ideas?

Otherwise the commits look very well!

rhaschke · 2020-07-25T08:13:43Z

@jschleicher, I guess you already figured out that the mesh filter test is fixed via #2180. I'm going to close and reopen to re-trigger Travis against the latest git HEAD.

jschleicher · 2020-07-28T07:55:06Z

@rhaschke #2180 seems like rounding difference, while I got large differences.

The difference between filtered_depth[idx] and gt_depth[idx] is 3.599998950958252, which exceeds 1e-6, where
filtered_depth[idx] evaluates to 3.599998950958252,
gt_depth[idx] evaluates to 0

I'll still try to run the tests with #2180 merged.

tylerjw

I tested this using these commands locall on 18.04 with Melodic:

# clone repos
TEST_WS=~/tmp/ws_moveit_resources_refactor_test/
mkdir -p ${TEST_WS}/src/
cd ${TEST_WS}/src
git clone git@github.com:ros-planning/moveit_resources.git -b pr-split-resources
git clone git@github.com:ros-planning/moveit.git -b pr-master-rework-resources
git clone https://github.com/ros-planning/moveit_msgs.git
git clone https://github.com/ros-planning/geometric_shapes.git
git clone https://github.com/PickNikRobotics/rviz_visual_tools
git clone https://github.com/ros-planning/moveit_visual_tools.git
git clone https://github.com/ros-planning/panda_moveit_config.git

# build
cd $TEST_WS
catkin config --no-install --extend /opt/ros/melodic --cmake-args -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"
catkin build

# filter function from moveit_ci
filter_out() {
  local PATTERN="$1"; shift
  echo "$*" | tr ' ;,' '\n' | grep -Fvxf <(echo "$PATTERN" | tr ' ;,' '\n') | tr '\n' ' '
}

# configure what tests we are testing (also from moveit_ci)
all_pkgs=$(catkin_topological_order ${TEST_WS}/src --only-names)
source_pkgs=$(catkin_topological_order ${TEST_WS}/src/moveit --only-names)
blacklist_pkgs=$(filter_out "$source_pkgs" "$all_pkgs")
catkin config --append-args --blacklist $blacklist_pkgs

# build and run tests
catkin build --make-args tests
catkin build --summarize --catkin-make-args run_tests

# results
catkin_test_results

The result of this is I see the same test failure with the meshes tests but everything else seems to be passing.

martiniil · 2020-08-05T12:06:36Z

Link to issue: moveit/moveit/issues/2228

jschleicher

Thanks @v4hn for your work! Approving.

SansoneG · 2020-08-07T06:50:45Z

@v4hn Is there anything still holding this PR back from getting merged?

this turns it into an actual moveit_config editable by the setup assistant. As I regenerated the whole package, I removed most references to the ROS-Industrial source.

pulled from upstream repo and updated the paths as well as some minor changes required to use our version of panda_description.

Provide the same interface as fanuc

rhaschke · 2020-08-12T09:50:11Z

Final Travis test running: https://travis-ci.com/github/ros-planning/moveit/builds/179402207

- Add preprocessed panda.srdf required by robot_model_test_utils - Fix location of panda.urdf

rhaschke · 2020-08-12T15:54:10Z

Currently, this is blocked by moveit/moveit_ci#117, which will fix the broken Indigo build.

because packages require cmake > 3.1

v4hn requested review from davetcoleman, mlautman, rhaschke and tylerjw July 6, 2020 16:07

make moveit_resources a meta-package

2a95695

- package *_description folders

v4hn force-pushed the pr-split-resources branch from 140129f to ec4d175 Compare July 7, 2020 07:38

v4hn mentioned this pull request Jul 7, 2020

Adapt repository for new moveit_resources layout moveit/moveit#2199

Merged

v4hn commented Jul 7, 2020

View reviewed changes

fanuc_moveit_config/launch/demo_chomp.launch Show resolved Hide resolved

v4hn mentioned this pull request Jul 7, 2020

Adapt to new moveit_resources layout moveit/moveit_tutorials#507

Merged

rhaschke reviewed Jul 7, 2020

View reviewed changes

fanuc_moveit_config/package.xml Outdated Show resolved Hide resolved

rhaschke force-pushed the pr-split-resources branch from b9cb150 to 3b48212 Compare July 7, 2020 23:37

v4hn commented Jul 8, 2020

View reviewed changes

rhaschke closed this Jul 25, 2020

rhaschke reopened this Jul 25, 2020

tylerjw reviewed Jul 29, 2020

View reviewed changes

jschleicher approved these changes Aug 5, 2020

View reviewed changes

v4hn mentioned this pull request Aug 5, 2020

Add ready + open group_states #38

Closed

henningkayser mentioned this pull request Aug 6, 2020

changelogs and version bump for foxy release #37

Closed

v4hn and others added 8 commits August 12, 2020 08:46

regenerate fanuc_moveit_config

a73a66b

this turns it into an actual moveit_config editable by the setup assistant. As I regenerated the whole package, I removed most references to the ROS-Industrial source.

fanuc_moveit_config: simplify test_environment.launch

3552ae4

update panda_moveit_config

986a6e3

pulled from upstream repo and updated the paths as well as some minor changes required to use our version of panda_description.

add all subpackages to the metapackage

4ef35fa

add test_environment.launch for panda

cf9f71e

Provide the same interface as fanuc

avoid circular dependencies in the moveit_config's

c1cc11f

Unify package.xml version to 0.6.5

07ef0fd

Adapt mesh resources to new package locations

8be4f81

rhaschke force-pushed the pr-split-resources branch from 3b48212 to a0b09d1 Compare August 12, 2020 09:49

Fix panda_moveit_config

ee078e1

- Add preprocessed panda.srdf required by robot_model_test_utils - Fix location of panda.urdf

rhaschke force-pushed the pr-split-resources branch from 9d422a5 to ee078e1 Compare August 12, 2020 14:12

rhaschke mentioned this pull request Aug 12, 2020

Add missing default value for ROS_PYTHON_VERSION moveit/moveit_ci#117

Merged

rhaschke force-pushed the pr-split-resources branch from ee078e1 to aa48929 Compare August 12, 2020 20:13

Drop Indigo support

bf880dc

because packages require cmake > 3.1

rhaschke force-pushed the pr-split-resources branch from aa48929 to bf880dc Compare August 12, 2020 20:25

rhaschke approved these changes Aug 12, 2020

View reviewed changes

rhaschke merged commit 2d26746 into master Aug 12, 2020

rhaschke deleted the pr-split-resources branch August 12, 2020 21:28

henningkayser mentioned this pull request Aug 13, 2020

Split folders into ROS packages #34

Closed

Conversation

v4hn commented Jul 6, 2020

Uh oh!

v4hn commented Jul 6, 2020

Uh oh!

tylerjw commented Jul 6, 2020

Uh oh!

v4hn commented Jul 6, 2020 via email

Uh oh!

v4hn commented Jul 7, 2020

Uh oh!

Uh oh!

v4hn commented Jul 7, 2020

Uh oh!

rhaschke commented Jul 7, 2020

Uh oh!

v4hn commented Jul 7, 2020

Uh oh!

Uh oh!

jschleicher commented Jul 7, 2020

Uh oh!

v4hn commented Jul 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jschleicher commented Jul 24, 2020

Uh oh!

rhaschke commented Jul 25, 2020

Uh oh!

jschleicher commented Jul 28, 2020

Uh oh!

tylerjw left a comment

Choose a reason for hiding this comment

Uh oh!

martiniil commented Aug 5, 2020

Uh oh!

jschleicher left a comment

Choose a reason for hiding this comment

Uh oh!

SansoneG commented Aug 7, 2020

Uh oh!

rhaschke commented Aug 12, 2020

Uh oh!

rhaschke commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants