search

moveit / moveit_core

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
30 stars 76 forks source link

[reopen #274] Send Octomap diff instead of whole tree #302

Closed 130s closed 8 years ago

130s commented 8 years ago

This enables Moveit to pass only the diff of the Octomap in a scene diff message when change detection is enabled on the Octree. This pull request depends on OctoMap/octomap#85 to compile. DO NOT MERGE until the Octomap pull request is accepted. ros-planning/moveit_ros#602 is the corresponding pull request in Moveit ROS for enabling change detection on the Octomap.

@dornhege I've rebased #274 to include updates (esp. CI config for testing). Thanks.

130s commented 8 years ago

Comment from the original PR:

This is essentially ros-planning/moveit_core#255 by @TheBrewCrew with some minor fixes.

The main reason is that in indigo we can only depend on OctoMap/octomap#103 and not OctoMap/octomap#85. This is handled in 28a6eb4. OctoMap/octomap#103 is already released as 1.6.9 in indigo (currently only shadow-fixed) and thus this can be merged once it is active.

I have left my additions as individual commits, so it is clear for reviewing this, what I changed. I can also squash them in the original commits before merging, if someone gives me the go-ahead.

The corresponding pull request is moveit_ros is ros-planning/moveit_ros#636.

dornhege commented 8 years ago

+1 I've tested this on the current indigo head in connection with ros-planning/moveit_ros#636 and it still works. I shouldn't merge this though as I did some of the changes on #274.

davetcoleman commented 8 years ago

I wish we would all move away from deep changes like this in an LTS (indigo) release...

v4hn commented 8 years ago

I wish we would all move away from deep changes like this in an LTS (indigo) release...

Long-Term-Support means Stable API / Stable ABI. It does not mean "no new features". As most people using MoveIt use indigo, it totally makes sense to add interesting features, and also next year I don't see a reason not to consider merging new features in LTS branches.

davetcoleman commented 8 years ago

I agree it does not mean no new features, but given our lack of good unit tests and integration tests its very risky we'll break current user's setups - for example it seems like this week's changes are causing problems. I'm fine making mistakes, but it should be in the right branch.

v4hn commented 8 years ago

On Thu, Jul 21, 2016 at 01:18:03PM -0700, Dave Coleman wrote:

I agree it does not mean no new features, but given our lack of good unit tests and integration tests its very risky we'll break current user's setups for example it seems like this week's changes are causing problems. I'm fine making mistakes, but it should be in the right branch.

I agree. But in my opinion neither indigo-devel, nor kinetic-devel are the right branch. I would like to add this to next weeks agenda if we have time for it.

Btw: the message you referenced is not actually a problem of the indigo-devel branch. Robert noticed an issue that was worse before, so this is still an improvement. :-) I believe the biggest problem with the patch-set is https://github.com/ros-planning/moveit_ros/pull/716#discussion_r71799182 .

davetcoleman commented 8 years ago

The right branch is technically the ROS-L branch ;-)

v4hn commented 8 years ago

On Fri, Jul 22, 2016 at 11:58:18AM -0700, Dave Coleman wrote:

The right branch is technically the ROS-L branch ;-)

I would prefer a plain "master" branch.

davetcoleman commented 8 years ago

it'd be great to get this merged in before friday's merge!

dornhege commented 8 years ago

@130s : Do you want to address the proposed changes before the merge? Otherwise I can re-open a PR based on this one.

130s commented 8 years ago

@dornhege thanks for the headsup and offering. Yes I appreciate if you could open a new PR.

dornhege commented 8 years ago

I can currently not test as ros-planning/moveit_ros#736 is preventing this from working. This might be resolved quickly, if it has something to do with my setup or is easily fixable. I could test with ros-planning/moveit_ros#ae51d07d5b1b848d87acb8c7dacb078b17d9c5ca and still provide this PR cleaned, if someone feels confident with merging. These should be independent as one is in moveit_core and the other in moveit_ros.

dornhege commented 8 years ago

Continued in #322.