rearrange chomp modules for maintainability by rhaschke · Pull Request #1251 · moveit/moveit

rhaschke · 2018-12-09T05:27:05Z

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

v4hn

That's tedious work, thank you!

moveit_core/collision_distance_field/CMakeLists.txt

...distance_field/include/moveit/collision_distance_field/collision_detector_allocator_hybrid.h

v4hn · 2018-12-09T15:29:10Z

moveit_planners/chomp/chomp_interface/CMakeLists.txt

-    moveit_ros_planning_interface
-  )
-else()
-  set(CHOMP_TEST_DEPS)


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

+1 to reducing the first set to 1 line

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

v4hn · 2018-12-09T15:37:47Z

moveit_experimental/CMakeLists.txt

  DEPENDS
    EIGEN3
    Boost
+    OCTOMAP


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

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

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.

I will be fine with this ;-)

moveit_planners/chomp/chomp_optimizer_adapter/CMakeLists.txt

moveit_planners/chomp/chomp_optimizer_adapter/package.xml

moveit_planners/chomp/collision_distance_field/package.xml

v4hn · 2018-12-09T16:12:52Z

moveit_planners/chomp/chomp_optimizer_adapter/chomp_optimizer_adapter_plugin_description.xml

@@ -0,0 +1,9 @@
+<library path="libmoveit_chomp_optimizer_adapter">
+
+  <class name="chomp/OptimizerAdapter" type="chomp::OptimizerAdapter"


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

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

This is on my (long) list...

- mv collision_distance_field from moveit_experimental to moveit_planners/chomp - mv chomp_optimizer_adapter.cpp into own package in moveit_planners/chomp

... and removed collision_detector_hybrid_plugin_loader

…p/OptimizerAdapter

rhaschke · 2018-12-09T18:11:37Z

@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.

As of moveit/moveit#1251, CHOMP uses automatically the hybrid collision detector.

…g chomp modules)

v4hn · 2018-12-10T15:54:37Z

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 · 2018-12-10T16:15:48Z

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

As of moveit/moveit#1251, CHOMP uses automatically the hybrid collision detector.

rhaschke · 2018-12-10T22:59:12Z

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

…homp [kinetic] backport rearrange chomp modules for maintainability #1251

* cleanup definition of move_group capabilities * remove explicit loading of "Hybrid" collision detector As of moveit/moveit#1251, CHOMP uses automatically the hybrid collision detector.

… objects (moveit#3124) (moveit#1251) * Add test that trigger the bug in applying scene that have robot state diffs * Fix a bug in planning scene diffs where having applying two diffs would cause them to override the aco * Fix a bug where removing an attached collision object would keep it in the scene * Update moveit_core/planning_scene/test/test_planning_scene.cpp Co-authored-by: AndyZe <andyz@utexas.edu> * Update moveit_core/planning_scene/test/test_planning_scene.cpp Co-authored-by: AndyZe <andyz@utexas.edu> * Update moveit_core/planning_scene/src/planning_scene.cpp Co-authored-by: AndyZe <andyz@utexas.edu> * Check parent pointer Co-authored-by: AndyZe <andyz@utexas.edu> Co-authored-by: AndyZe <andyz@utexas.edu>

rhaschke mentioned this pull request Dec 9, 2018

201811 release #1225

Closed

12 tasks

rhaschke force-pushed the cleanup-chomp branch from cdba5d7 to a443d8f Compare December 9, 2018 06:03

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

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

v4hn requested changes Dec 9, 2018

View reviewed changes

rhaschke force-pushed the cleanup-chomp branch 2 times, most recently from f088a46 to 3f06d90 Compare December 9, 2018 16:36

rhaschke added 5 commits December 9, 2018 18:42

cleanup chomp folder structure

b8db15a

- 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

1aa4c28

... and removed collision_detector_hybrid_plugin_loader

fix configuration of hybrid collision detector

ed9ff1e

rename default_planner_request_adapters/CHOMPOptimizerAdapter -> chom…

ec79e4e

…p/OptimizerAdapter

fix memory leak: ChompOptimizer was not freed

e658caf

rhaschke force-pushed the cleanup-chomp branch from 3f06d90 to e658caf Compare December 9, 2018 17:42

rhaschke mentioned this pull request Dec 9, 2018

minor fixes moveit/moveit_tutorials#264

Merged

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

remove hybrid collision detector

51b21a7

As of moveit/moveit#1251, CHOMP uses automatically the hybrid collision detector.

rhaschke mentioned this pull request Dec 9, 2018

remove hybrid collision detector moveit/panda_moveit_config#16

Merged

rhaschke merged commit e658caf into moveit:melodic-devel Dec 9, 2018

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

Merge pull requests #1104 (fixing memory leaks) and #1251 (rearrangin…

8d4c59f

…g chomp modules)

rhaschke mentioned this pull request Dec 9, 2018

[Melodic] Ignore moveit_experimental ros-gbp/moveit-release#7

Merged

rhaschke deleted the cleanup-chomp branch December 9, 2018 23:31

v4hn mentioned this pull request Dec 10, 2018

chomp: explicitly set variable as empty when unused #1252

Merged

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

remove explicit loading of "Hybrid" collision detector

ba36187

As of moveit/moveit#1251, CHOMP uses automatically the hybrid collision detector.

This was referenced Dec 10, 2018

fanuc_moveit_config: cleanup definition of move_group capabilities moveit/moveit_resources#23

Merged

moveit_experimental: commenting out dependencies #1256

Merged

v4hn mentioned this pull request Dec 21, 2018

[kinetic] backport rearrange chomp modules for maintainability #1251 #1282

Merged

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

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

5a77af4

…homp [kinetic] backport rearrange chomp modules for maintainability #1251

rhaschke mentioned this pull request Apr 5, 2019

Hybrid collision checker plugin not found #1426

Closed

tnaka mentioned this pull request Aug 9, 2020

Fix the name of CHOMP optimizer adapter moveit/moveit_tutorials#512

Merged

2 tasks

		@@ -0,0 +1,9 @@
		<library path="libmoveit_chomp_optimizer_adapter">

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

Conversation

rhaschke commented Dec 9, 2018

Uh oh!

v4hn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

v4hn Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

rhaschke Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v4hn Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

rhaschke Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

v4hn Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

rhaschke Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

v4hn Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

rhaschke Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Dec 9, 2018

Uh oh!

v4hn commented Dec 10, 2018

Uh oh!

rhaschke commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhaschke commented Dec 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rhaschke Dec 9, 2018 •

edited

Loading

rhaschke commented Dec 10, 2018 •

edited

Loading