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

rearrange chomp modules for maintainability #1251

Merged
merged 5 commits into from Dec 9, 2018

Conversation

Projects
None yet
2 participants
@rhaschke
Copy link
Collaborator

commented Dec 9, 2018

Cleaning up chomp sources that are scattered around the repository, creating a circular dependency between chomp, moveit_core, and moveit_experimental.
This fixes #1136:

  • move collision_distance_field from moveit_experimental back to moveit_core
  • move chomp_optimizer_adapter.cpp into own package moveit_chomp_optimizer_adapter
  • rename default_planner_request_adapters/CHOMPOptimizerAdapter -> chomp/OptimizerAdapter

@rhaschke rhaschke referenced this pull request Dec 9, 2018

Closed

201811 release #1225

11 of 12 tasks complete

@rhaschke rhaschke force-pushed the ubi-agni:cleanup-chomp branch from cdba5d7 to a443d8f Dec 9, 2018

@rhaschke rhaschke changed the title cleanup chomp cleanup chomp source folders Dec 9, 2018

@v4hn v4hn changed the title cleanup chomp source folders rearrange chomp modules for maintainability Dec 9, 2018

@v4hn
Copy link
Member

left a comment

That's tedious work, thank you!

Show resolved Hide resolved moveit_experimental/collision_distance_field/CMakeLists.txt
Show resolved Hide resolved ...de/moveit/collision_distance_field/collision_detector_allocator_hybrid.h
moveit_ros_planning_interface
)
else()
set(CHOMP_TEST_DEPS)

This comment has been minimized.

Copy link
@v4hn

v4hn Dec 9, 2018

Member

I'm not a fan of using undefined variables.
cmake allows for it (although there were ideas about adding a warning for it in the past), but it also has a concept of DEFINED which can trigger different behavior.

👍 to reducing the first set to 1 line
👎 to leaving out the else

This comment has been minimized.

Copy link
@rhaschke

rhaschke Dec 9, 2018

Author Collaborator

+1 to reducing the first set to 1 line

This was my intention. For me it looked really ugly.
Should I revert then?

@@ -56,26 +45,17 @@ catkin_package(
DEPENDS
EIGEN3
Boost
OCTOMAP

This comment has been minimized.

Copy link
@v4hn

v4hn Dec 9, 2018

Member

I believe octomap is not required by moveit_experimental anymore with this patch.

This comment has been minimized.

Copy link
@rhaschke

rhaschke Dec 9, 2018

Author Collaborator

moveit_experimental doesn't build anything anymore...
I just moved the octomap dependency to the right place.

This comment has been minimized.

Copy link
@v4hn

v4hn Dec 10, 2018

Member

moveit_experimental doesn't build anything anymore...

True.
It feels stupid to keep all the dependencies for a module that does not build anything.
Shouldn't we at least comment out all the unnecessary bits?
They cost build time and are simply baggage at this point.

This comment has been minimized.

Copy link
@rhaschke

rhaschke Dec 10, 2018

Author Collaborator

I will be fine with this ;-)

Show resolved Hide resolved moveit_planners/chomp/chomp_optimizer_adapter/CMakeLists.txt Outdated
Show resolved Hide resolved moveit_planners/chomp/chomp_optimizer_adapter/package.xml
Show resolved Hide resolved moveit_planners/chomp/collision_distance_field/package.xml Outdated
@@ -0,0 +1,9 @@
<library path="libmoveit_chomp_optimizer_adapter">

<class name="chomp/OptimizerAdapter" type="chomp::OptimizerAdapter"

This comment has been minimized.

Copy link
@v4hn

v4hn Dec 9, 2018

Member

👍 for the name change. It's simply not a "default".

The tutorial has to be changed to the new name though.

This comment has been minimized.

Copy link
@rhaschke

rhaschke Dec 9, 2018

Author Collaborator

This is on my (long) list...

@rhaschke rhaschke force-pushed the ubi-agni:cleanup-chomp branch 2 times, most recently from f088a46 to 3f06d90 Dec 9, 2018

rhaschke added some commits Dec 8, 2018

cleanup chomp folder structure
- mv collision_distance_field from moveit_experimental to moveit_planners/chomp
- mv chomp_optimizer_adapter.cpp into own package in moveit_planners/chomp
move collision_distance_field to moveit_core
... and removed collision_detector_hybrid_plugin_loader

@rhaschke rhaschke force-pushed the ubi-agni:cleanup-chomp branch from 3f06d90 to e658caf Dec 9, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 9, 2018

@v4hn I fixed the remaining issues and addressed your comments. Do you want to have a final look?
I would like to continue with the release as announced.

@rhaschke rhaschke referenced this pull request Dec 9, 2018

Merged

minor fixes #264

rhaschke added a commit to rhaschke/panda_moveit_config that referenced this pull request Dec 9, 2018

remove hybrid collision detector
As of ros-planning/moveit#1251, CHOMP uses automatically
the hybrid collision detector.

@rhaschke rhaschke merged commit e658caf into ros-planning:melodic-devel Dec 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhaschke added a commit that referenced this pull request Dec 9, 2018

@rhaschke rhaschke deleted the ubi-agni:cleanup-chomp branch Dec 9, 2018

@v4hn

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I just stumbled over https://travis-ci.org/ros-planning/moveit_visual_tools/jobs/465994691#L479
on the build farm.
Was this only a temporary problem or are the shadow-fixed debs broken for now?

@rhaschke

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2018

I was so brave to update the binary sources on my machine. There I get a more verbose error msg:
E: /tmp/apt-dpkg-install-i6AXUC/02-ros-melodic-moveit-core_0.10.6-0bionic.20181210.112240_amd64.deb: trying to overwrite '/opt/ros/melodic/include/moveit/collision_distance_field/collision_common_distance_field.h', which is also in package ros-melodic-moveit-experimental 0.10.5-0bionic.20181117.194840

So, we need to "uninstall" the obsolete binary package ros-melodic-moveit-experimental.
@clalancette Is there an option to do so automatically? Manually doing it, resolved the problem locally.

For Travis, I guess we need to wait for dockerhub to rebuild our docker image:
https://hub.docker.com/r/moveit/moveit/builds

However, as ci-shadow-fixed builds on top of ci, this will probably fail too.

I would like to continue this discussion in #1225

rhaschke added a commit to rhaschke/moveit_resources that referenced this pull request Dec 10, 2018

remove explicit loading of "Hybrid" collision detector
As of ros-planning/moveit#1251, CHOMP uses automatically the hybrid collision detector.
@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2018

@v4hn I guess, we should back-port this to Kinetic as well. What do you think?

v4hn added a commit that referenced this pull request Jan 11, 2019

Merge pull request #1282 from ros-planning/pr-kinetic-backport-move-c…
…homp

[kinetic] backport rearrange chomp modules for maintainability #1251

mlautman added a commit to ros-planning/moveit_resources that referenced this pull request Apr 3, 2019

fanuc_moveit_config: cleanup definition of move_group capabilities (#23)
* cleanup definition of move_group capabilities

* remove explicit loading of "Hybrid" collision detector

As of ros-planning/moveit#1251, CHOMP uses automatically the hybrid collision detector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.