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

[PSM] Add proccess Collision Object to PSM and request planning scene to moveit py to allow syncing of mutliple PSM #2536

Conversation

JensVanhooydonck
Copy link
Contributor

Add proccess collision object and proccess attached collision object to PlanningSceneMonitor and add request planning scene to moveit py to allow syncing of multiple planning scene monitors.

Description

When altering a(n) (attached-)collision object in the planning scene by using the applyCollisionObject or processAttachedCollisionObject function on the planning scene object, this does not trigger an updated in the PSM.

When using the same logic in PSM we can alter a collsiion object directly, and sync this to other planning scene monitors.

To make this complete I also added the request planning scene function to the MoveitPy interface

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (c2292a7) 50.89% compared to head (a71b56c) 50.87%.
Report is 1 commits behind head on main.

❗ Current head a71b56c differs from pull request most recent head a2be672. Consider uploading reports for the commit a2be672 to get more accurate results

Files Patch % Lines
...nning_scene_monitor/src/planning_scene_monitor.cpp 0.00% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2536      +/-   ##
==========================================
- Coverage   50.89%   50.87%   -0.01%     
==========================================
  Files         388      388              
  Lines       32245    32247       +2     
==========================================
- Hits        16408    16403       -5     
- Misses      15837    15844       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JensVanhooydonck
Copy link
Contributor Author

JensVanhooydonck commented Nov 21, 2023

Known issues when trying to sync multiple planning scene:

Copy link
Member

@peterdavidfagan peterdavidfagan left a comment

Choose a reason for hiding this comment

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

These changes look good to me as they enable a more expressive Python API.

@henningkayser are you ok with the changes made to the planning_scene_monitor cpp code? They should be mostly inconsequential from what I can see, as they simply split a callback into a method which is called by the callback and the original callback (changes to collision and attached collision object callbacks on the psm). The purpose of this is to create a method for processing collision objects directly in the Python API essentially exposing the callback functionality in the binded Python method.

Copy link
Collaborator

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @JensVanhooydonck! Just a small comment and then I think this is good to go

}

void PlanningSceneMonitor::attachObjectCallback(const moveit_msgs::msg::AttachedCollisionObject::ConstSharedPtr& obj)
{
if (scene_)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be even nicer to completely get rid of the callback member functions and just call the new function with a lambda when the services are initialized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I just remove the callback functions and change the call of these functions from

  collision_object_subscriber_ = pnode_->create_subscription<moveit_msgs::msg::CollisionObject>(
        collision_objects_topic, rclcpp::SystemDefaultsQoS(),
        [this](const moveit_msgs::msg::CollisionObject::ConstSharedPtr& obj) { return collisionObjectCallback(obj); });
    RCLCPP_INFO(logger_, "Listening to '%s'", collision_objects_topic.c_str());

to

  collision_object_subscriber_ = pnode_->create_subscription<moveit_msgs::msg::CollisionObject>(
        collision_objects_topic, rclcpp::SystemDefaultsQoS(),
        [this](const moveit_msgs::msg::CollisionObject::ConstSharedPtr& obj) { return processCollisionObjectMsg(obj); });
    RCLCPP_INFO(logger_, "Listening to '%s'", collision_objects_topic.c_str());

And the same for the AttachedCollisionObjects?

These functions do not seem to be used somewhere else. I've added this to make sure there would not be any breaking changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes 👍 They aren't used anywhere else and if they only call another function we don't need them. I think the lambda needs be

-   [this](const moveit_msgs::msg::CollisionObject::ConstSharedPtr& obj) { return processCollisionObjectMsg(obj); }
+   [this](const moveit_msgs::msg::CollisionObject::ConstSharedPtr& obj) { processCollisionObjectMsg(obj); }

because the create_subscription expects a function argument that returns void but just use whatever compiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it

JensVanhooydonck and others added 3 commits November 27, 2023 10:32
…monitor.cpp


First catch empty scene to not have a unneeded indents.

Co-authored-by: Sebastian Jahr <sebastian.jahr@tuta.io>
@sjahr sjahr enabled auto-merge (squash) November 27, 2023 14:45
@sjahr sjahr merged commit f5d8afd into ros-planning:main Nov 27, 2023
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants