Proposer: E. Gil Jones
Present at review:
A bit of system design exposition
To give a bit of context on what's up with the planning scene stuff here's a system diagram specifically of how the planning scene system functions. The goal of this meeting is not system design, but I still thought it would be useful in reviewing the messages.
The EnvironmentServer node is at the center of every thing. It serves two primary roles - the first is to monitor the current state of the system (current robot state, collision map, collision objects) to provide a unified view of the state of the world as it affects arm navigation, and the other is to provide a unified view of the current or altered state of the world to all the arm navigation components - nodes like planners, trajectory filters, constraint-aware kinematics.
One primary use case for the system is for requests to MoveArm to actually move from the current position to another desired position. The other primary use case is for nodes other than move_<group_name> to make requests to arm navigation components. In particular, these other nodes may want to consider states other than the current state - for instance, whether a plan can be found to put a cup down in a particular location after the robot has actually picked up the cup. In either case the function of the planning scene system is identical. The calling node makes a GetPlanningScene.srv request to the EnvironmentServer - the request contains a planning_scene_diff, which contains all the changes from the current state of the system the calling node would like to make. For the move_<group_name> case this is likely to only be changes in link padding, allowed contacts, or ordered_collision_operations. For the latter use case this could also be changing collision objects or providing a new current robot state.
In either case, once the EnvironmentServer receives the call it first applies the diff to the current planning scene state to get a resolved planning scene. It then calls the SetPlanningScene.action for each of the arm navigation components that have previously registered that they are alive and would like planning scenes. The mechanism that is used for this registration is that each of the components contains a CollisionModelsInterface class that first advertises an action in the local namespace and then call a registration service in the EnvironmentServer on start-up. The EnvironmentServer looks up the calling node's name in the service request and in the registration callback creates an action client back to the CollisionModelsInterface. It then stores these actions in a map and will iterate through the map when a new call to the GetPlanningScene service occurs. The action in CollisionModelsInterface sets the internal collision model using the resolved planning scene and optionally calls a callback within the owner code that can take appropriate action when there's a new planning scene. The idea is that all arm navigation components get a unified view of the world such that planning, filtering, and kinematics all occur in a unified view of the world which may or may not be the current state of the world. Calls can then be made to plan, filter, find kinematics without requiring changing the world model. There's an implied resource given that only one planning scene can be shared among the arm navigation components at any given time.
Once all the components have done what they need to do for the new planning scene all the actions return and the EnvironmentServer returns the resolved planning scene in the response to GetPlanningScene. The calling node can then set its own CollisionModels class with the scene and make calls that directly manipulate the CollisionModels. Calling nodes can then safely also make calls to kinematics, planners, and trajectory filters repeatedly and efficiently.
What to review
All messages in arm navigation from unstable are fair game for review, though most have been previously reviewed. There are a whole bunch of message packages in arm_navigation (in rough dependency order):
One thing to note is that all fields have been removed from messages since review - references to LinkPadding, OrderedCollisionOperations, and AllowedContactSpecification are gone from some messages. The reason is that these are part of the planning scene.
New messages that are especially important to review
Messages that I propose to delete before release (no longer used, supplanted by PlanningScene, available from C++ API in a more complete manner)
Question / concerns / comments
GetPlanningScene is a little bit of a confusing name. It seems in my use case at least I generally use it to set the planning scene on the remote node, and I never do anything with the planning scene it returns. As such, SetPlanningScene would be closer. Of course, what you are passing in is a diff, not a full scene, so maybe ApplyPlanningSceneChanges.
Maybe there is room for a GetPlanningScene that does not affect the scene on the Environment Server - the server just sends you back its current scene with the diff you requested applied, but its local scene stays unchanged.
What happens if you call GetPlanningScene twice with different diffs? Let's say the Environment Server has its current scene, say "scene0". If you call GetPlanningScene twice, once with "diff1" and one with "diff2", what do you get the second time? Is it scene0+diff1+diff2, or scene0+diff2?
- Is there a list of messages that are used to communicate with the Environment Server? Is that planning_environment_msgs? What is the difference between an Environment Server and a Planning Environment? Since the Environment Server is such a central concept, should it not get its own messages package?
should RobotState also go in there?
- why are there so many message packages? Is it mostly for historical reasons, or are those valid semantic separations?
- how do we avoid race conditions and resource contention, either now or in the future, since there is one Environment Server and potentially many clients, all with their own diffs in mind?
- geometric_shapes_msgs/Shape.msg: Just to check: is something like this not already elsewhere, such as in visualization_msgs/Marker?
- No comments / docs for: mapping_msgs/PolygonalMap, collision_environment_msgs/CloudSettings.msg, collision_environment_msgs/GetCloudSettings.srv, collision_environment_msgs/OccupancyPointQuery.srv, collision_environment_msgs/SetCloudSettings.srv, planning_environment_msgs/MakeStaticCollisionMap.action, maybe others.
collision_environment_msgs: Looks like auto-generated messages for MakeStaticCollisionMap action were checked in. Same for move_arm_msgs/MoveArm.action.
Normalize BBX vs. BoundingBox?
- collision_environment_msgs/OccupancyBBXSizeQuery.srv request should be an instance of mapping_msgs/OrientedBoundingBox.msg?
To be filled out by proposer based on comments gathered during API review period
Stack status change mark change manifest)
Action items that need to be taken.
Major issues that need to be resolved