Codecov Report

Merging #764 (1c1b4c4) into sdf12 (ef48530) will increase coverage by 0.96%.
The diff coverage is 93.48%.

@@            Coverage Diff             @@
##            sdf12     #764      +/-   ##
==========================================
+ Coverage   89.60%   90.57%   +0.96%     
==========================================
  Files          78       78              
  Lines       12691    12336     -355     
==========================================
- Hits        11372    11173     -199     
+ Misses       1319     1163     -156     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef48530...1c1b4c4. Read the comment docs.

Per VC, consider having an (internal / private) base class. e.g. FrameBase -> {Frame, InterfaceFrame} (is-a); however, may break ABI.
Alternative, use has-a, some facade class.

Perhaps better is to produce InterfaceModel (possibly InterfaceWorld too) as the facade.

Converted to a draft to explore use of Interface elements for all frame building logic. @scpeters , @jennuine please hold off on reviewing until I put this back as ready for review.

The issue with trying to use only Interface elements for building frame graphs is they only provide a subset of functions needed for building the graph. For example, sdf::InterfaceLink currently has sdf::InterfaceLink::PoseInModelFrame which limits the possibility of using other frames as @relative_to for the pose of the link. However, sdf::Link has sdf::Link::RawPose and sdf::Link::PoseRelativeTo. Creating an sdf::InterfaceLink from the information contained in an sdf::Link, in the current state of sdf::InterfaceLink, is impossible because we have to provide sdf::InterfaceLink with its pose relative to its model frame, but if the sdf::Link's pose is relative to another frame, we'd have to resolve the pose, which we can't do until the pose graph is built. We can add sdf::InterfaceLink::RawPose andsdf::InterfaceLink::PoseRelativeTo, and change the constructor to take a relativeTo argument, but that would make sdf::InterfaceLink::PoseInModelFrame invalid as it cannot return the pose relative to the model frame.

So the approach I'm thinking of is to create wrapper structs for all elements and use those wrappers in the build* functions.
For example, a wrapper for sdf::Link and sdf::InterfaceLink would be the following:

struct LinkWrapper
{
  explicit LinkWrapper(const sdf::Link &_link)
      : name(_link.Name()),
        rawPose(_link.RawPose()),
        poseRelativeTo(_link.PoseRelativeTo()),
        elementType("Link"),
        frameType(FrameType::LINK)

  {
  }

  explicit LinkWrapper(const sdf::InterfaceLink &_ifaceLink)
      : name(_ifaceLink.Name()),
        rawPose(_ifaceLink.PoseInModelFrame()),
        poseRelativeTo("__model__"),
        elementType("Interface Link"),
        frameType(FrameType::LINK)
  {
  }

  std::string name;
  ignition::math::Pose3d rawPose;
  std::string poseRelativeTo;
  std::string elementType;
  FrameType frameType;
};

The build* functions can then use the members name, rawPose, etc. I'm opting for using a member variables directly instead of getters to limit verbosity. These wrappers would all be in FrameSemantics.cc so we won't have any issue of API/ABI stability.

This is now ready for review.

LGTM, maybe @scpeters can take a second look?

I'm looking right now

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1