Skip to content
This repository has been archived by the owner on Nov 13, 2017. It is now read-only.

planning_scene updates: expose success state to caller #297

Merged
merged 1 commit into from
Jun 14, 2016

Conversation

v4hn
Copy link
Contributor

@v4hn v4hn commented Jun 14, 2016

This is required to get the information back for the
ApplyPlanningSceneService.

This is required for moveit/moveit_ros#686

As it turns out, according to abi-compliance-checker these changes
are also ABI compliant... I didn't expect that.

@@ -1185,14 +1187,16 @@ void planning_scene::PlanningScene::setPlanningSceneDiffMsg(const moveit_msgs::P

// process collision object updates
for (std::size_t i = 0 ; i < scene_msg.world.collision_objects.size() ; ++i)
processCollisionObjectMsg(scene_msg.world.collision_objects[i]);
ret &= processCollisionObjectMsg(scene_msg.world.collision_objects[i]);
Copy link
Member

@davetcoleman davetcoleman Jun 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this is out of style for MoveIt!'s code base - we have almost no &= in the code base (except in the rviz plugin, twice) and I found it hard to understand at first. Wouldn't += accomplish the same thing but be easier to quickly understand? Its probably just me being ignorant but I'm not used to seeing it

This is required to get the information back for the ApplyPlanningSceneService.
@davetcoleman davetcoleman merged commit b097d6f into moveit:indigo-devel Jun 14, 2016
@130s
Copy link
Contributor

130s commented Jun 28, 2016

Looks like this is already cherry-picked into jade-devel 702c3e7 w/o PR.

@130s 130s mentioned this pull request Jun 28, 2016
@v4hn
Copy link
Contributor Author

v4hn commented Jun 28, 2016

Yes. When I asked about a cherry-pick policy dave replied
that it's fine to pick merged commits to newer branches without PR here.
Would you prefer to have a PR for each cherry-pick @130s ?

@130s
Copy link
Contributor

130s commented Jun 28, 2016

@v4hn Yeah I think so,,,although I know it's cumbersome.
As I commented #301 (comment), currently I don't know an easy, cleaner way to find commits that need synced to other branches, and going through PRs is robust enough I think. Suggestion is much welcomed.

@v4hn
Copy link
Contributor Author

v4hn commented Jun 28, 2016 via email

@davetcoleman
Copy link
Member

I still think if you just had a PR merged and you want to reflect it in a newer branch, its no big deal to cherry-pick and commit it without a commit. I say its fine because its what I've seen others do here and it makes sense to me. I created those PRs for syncing in the past day because they were old and messy and potential error-prone. I think we should just do a better job of always keeping the branches in sync

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants