API review

Proposer: Bhaskara

Present at review:

• List reviewers

Question / concerns / comments

This is a preliminary review intended to figure out what exactly the API for the graph structure for mapping should be. The current API is in trunk of graph_mapping. Get it using:

svn co https://code.ros.org/svn/ros-pkg/stacks/graph_mapping/trunk graph_mapping

You can either look at the documentation in the header files, or generate html documentation using

cd graph_mapping
export ROS_PACKAGE_PATH=$PWD:$ROS_PACKAGE_PATH
rosrun rosdoc rosdoc pose_graph graph_slam laser_slam graph_mapping_msgs

which you can then browse to in graph_mapping/doc.

It's structured as follows:

• ROS messages and basic data structures in graph_mapping_msgs.

• The pose_graph package contains general ROS-independent graph data structure, in the PoseGraph and ConstraintGraph classes, as well as functions for visualizing, message conversion, loading/saving, and optimization.

• Generic SLAM node template in graph_slam package. This defines abstract classes SensorSnapshotter, Localizer, and ConstraintGenerator that specific SLAM algorithms must instantiate.

• Example prototype that uses odometry constraints and laser data for reconstruction, in the laser_slam package.

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Kevin

• The PoseWithPrecision message is the same as a geometry_msgs/PoseWithCovariance. Can we use that instead?

  • bm - I'd be reluctant to do that since Precision and Covariance mean different things.

• The GraphLocalization message uses an array for the precision, but the precision of the PoseWithPrecision message is a fixed-length 36 array.

  • bm - this was intended as an optimization for the case in which we want to store a simplified precision (e.g., an empty array representing infinite precision).
• Should the graph messages store the sensor data associated with each pose? Would that be useful for reconstruction?

  • bm - Yes. But we need to define a new message for each value of the template parameter, and would be done in the sensor-specific packages rather than in graph_mapping_msgs.

Stu

• What's the benefit of inheriting from ConstraintGraph? Are there functions that operate on all subclasses of ConstraintGraph?\

  • Improved compilation time: by making the PoseGraph<D> all inherit from the non-templated class ConstraintGraph, most of the library functionality can be put into its own compilation unit.

• I'm not a fan of the various "*Id" classes. I don't see much benefit over using "long" or "int".
  • I originally had int's but had a couple of occasions where there were bugs caused by using an edge id to refer to a node or vice versa, so added this for type checking. I can take it out if it seems really clunky though.

• Your ID lookups seem to use std::set. I think std::set is O(log(n)) for lookups and insertions. You probably want std::hash_set, which is O(1) for lookups and insertions (though it's not ordered). Actually, you might want tr1::unordered_set.

  • Ok, will do.

• If we're keeping the *Id classes, then addEdge and addNode should take a const& to the id.

  • Ok.

• pose_graph.cpp has typedefs for EdgeIdSet, NodeIdSet, etc... Should these be included from elsewhere?

Conclusion

Stack status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved