SDF parsing

[amcastro-tri]

Implements a representation for a specification read with the sdformat library.
This representation could be used to instantiate multibody models (with MBT) or geometric models (with GS), though visual and collision are not parsed in this first iteration.

---

This change is 

[amcastro-tri]

+@SeanCurtis-TRI for feature review please.

---

Review status: 0 of 11 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jamiesnape]

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.

---

drake/multibody/parsing/sdf_parser.h, line 7 at r1 (raw file):

#include <unordered_map>

#include "sdf/sdf.hh"

Assuming this header is installed, this adds "sdf/sdf.hh" to the list in #7451 and therefore needs changes to drake.cps (move SDFormat:sdformat from Link-Requires to Requires).

---

Comments from Reviewable

[jwnimmer-tri]

drake/multibody/parsing/sdf_parser.h, line 7 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Assuming this header is installed, this adds "sdf/sdf.hh" to the list in #7451 and therefore needs changes to drake.cps (move SDFormat:sdformat from Link-Requires to Requires).

In this case @amcastro-tri, it seems totally worthwhile to hide the include into the cc file. Its only used by private methods, and not even clear anyway why they have to be instance methods, since there are no member fields anyway. Seems like free functions in the cc file would work just as well.

---

Comments from Reviewable

[amcastro-tri]

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.

---

drake/multibody/parsing/sdf_parser.h, line 7 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In this case @amcastro-tri, it seems totally worthwhile to hide the include into the cc file. Its only used by private methods, and not even clear anyway why they have to be instance methods, since there are no member fields anyway. Seems like free functions in the cc file would work just as well.

Good point. This was me making a quick first spiral so that someone else can own this code (and write it well :-) ). I can refactor this to free functions, and move the include to the source.

---

Comments from Reviewable

[SeanCurtis-TRI]

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.

---

drake/multibody/parsing/sdf_parser.h, line 7 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Good point. This was me making a quick first spiral so that someone else can own this code (and write it well :-) ). I can refactor this to free functions, and move the include to the source.

BTW The goal here is to minimize the impact on the installed version of drake, yes? And, in this particular case, burying references to headers outside of drake?

---

Comments from Reviewable

[jwnimmer-tri]

drake/multibody/parsing/sdf_parser.h, line 7 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW The goal here is to minimize the impact on the installed version of drake, yes? And, in this particular case, burying references to headers outside of drake?

Two things. First, anytime we have an include to a third-party library, we need to make sure that the installed version of drake provides the correct include paths to make that work (the drake.cps edit). Second, if we keep the includes hidden (unmentioned by our installed headers), it enables us if need be to later link SDF privately with hidden symbols, which makes it easy for us to use a fork of it or otherwise shield it from libdrake.so consumers and their cross-compatibility concerns.

---

Comments from Reviewable

[SeanCurtis-TRI]

Ok, lots of little things....

---

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 44 unresolved discussions.

---

drake/multibody/parsing/sdf_joint.h, line 16 at r1 (raw file):

/// A representation of a `<joint>` element in an SDF file.
/// For details on the specification of links, including conventions and default

BTW Is this copypasta? "...specification of links"? The url references joint.

---

drake/multibody/parsing/sdf_joint.h, line 26 at r1 (raw file):

  /// Creates a new joint object specification with the given `joint_name`.
  /// Per SDF specification, `joint_name` must be unique within the scope of the
  /// joint's model.

This last statement introduces some ambiguity. It is unclear whether this requirement is enforce or not; probably worth noting explicitly that it is not enforced. This applies to the other places (e.g., sdf_link.h where similar language is used).

---

drake/multibody/parsing/sdf_joint.h, line 38 at r1 (raw file):

  ///   The type of the joint. E.g: revolute, prismatic. See documentation on
  ///   the <a href="http://sdformat.org/spec?ver=1.6&elem=joint#joint_type">
  ///   &lt;<type>&gt; element</a> for details.

BTW I'm assuming this is a doxygen error. You have both @lt; and <.

---

drake/multibody/parsing/sdf_joint.h, line 40 at r1 (raw file):

  ///   &lt;<type>&gt; element</a> for details.
  SDFJoint(
      const std::string& joint_name,

BTW joint_name isn't documented in the doxygen like the other parameters. Seems conspicuously absent.

---

drake/multibody/parsing/sdf_joint.h, line 46 at r1 (raw file):

      name_(joint_name),
      parent_link_name_(parent_link_name), child_link_name_(child_link_name),
      joint_type_(joint_type) {}

BTW I suspect this is a formatting error against the style guide (http://drake.mit.edu/styleguide/cppguide.html#Constructor_Initializer_Lists) If you hit these lines with clang format, I'm confident it'll change things around.

---

drake/multibody/parsing/sdf_joint.h, line 63 at r1 (raw file):

  /// For details on how this frame is defined refer to the documenatation for
  /// the <a href="http://sdformat.org/spec?ver=1.6&elem=joint#joint_axis">
  /// &lt;<axis>&gt; element</a>.

Given this aborts when called erroneously:

1. That behavior should be documented.
2. There should be a method to determine if it can be called safely.

(Same for set_axis.)

---

drake/multibody/parsing/sdf_link.h, line 37 at r1 (raw file):

  void set_mass(double mass_value) { mass_ = mass_value; }

  /// Returns the rotational inertia `I_Icm` of `this` link about its center of

BTW This is unfortunate notation. Do we use I to indicate the inertial frame lots of places in Drake? I understand N is also a common symbol for the inertial frame. Anything would feel better than I_Icm where the two Is mean something different. This applies to the I_Io_iI below as well.

---

drake/multibody/parsing/sdf_link.h, line 56 at r1 (raw file):

  }

  /// Returns the pose `X_DL` of `this` link's frame L in the parent model

FYI It isn't clear to me why we're using D to mean "parent model frame". Is there some intuition at play here that I'm missing? Otherwise, as mnemonics go, D isn't very satisfying.

I'm coming back around to this having read through the rest of the PR. I have a new comment (but I'm leaving the old one so you can see the confused thought process). I'd advocate eliminating the use of the word "parent" as defining the relationship between a link and its model. The term has very loaded meaning and I think it confuses things here. I'd say it's enough to say "pose of this links' frame in the link's parent model frame". And then I'd make the model frame M.

---

drake/multibody/parsing/sdf_link.h, line 78 at r1 (raw file):

 private:
  // Name of the root frame of this cache.

BTW copypasta s/cache/link

---

drake/multibody/parsing/sdf_link.h, line 90 at r1 (raw file):
BTW typo, insert commas:

...which, per SDF specification, must be...

---

drake/multibody/parsing/sdf_model.h, line 31 at r1 (raw file):

  /// Creates a new SDF model object specification with the given `model_name`.
  /// Per SDF specification, `model_name` must not match any other model name in
  /// the world (<world>).

BTW back ticks so the < survives doxygen?

---

drake/multibody/parsing/sdf_model.h, line 55 at r1 (raw file):

    const int link_index = get_num_links();
    links_.emplace_back(link_name);
    links_name_to_index_map_.insert({link_name, link_index});

FYI This could be an opportunity to report on the uniqueness of the link name.

---

drake/multibody/parsing/sdf_model.h, line 61 at r1 (raw file):

  /// Adds a new joint named `joint_name` to `this` model and returns a
  /// reference to the newly added joint. Please refer to the SDFJoint class's
  /// documentation for details on the arguments for this method.

FYI A comment about the fact that nothing is validated? (i.e., a user can reference a non-existent link and/or a joint type that doesn't actually exist?)

Generally, for this kind of thing, it might be worth adding documentation to the class documentation on all of these that this is a naive wrapper of the string data contained in the file -- it provides convenient access but does not guarantee the viability of the values.

---

drake/multibody/parsing/sdf_model.h, line 69 at r1 (raw file):

    joints_.emplace_back(
        joint_name, parent_link_name, child_link_name, joint_type);
    joints_name_to_index_map_.insert({joint_name, joint_index});

FYI This could be an opportunity to report on the uniqueness of the joint name.

---

drake/multibody/parsing/sdf_model.h, line 93 at r1 (raw file):

    if (it == links_name_to_index_map_.end()) {
      throw std::runtime_error(
          "Link \"" + link_name + "\" not found in model \"" + this->name()

FYI You could consider using single quotes in your error message to save the escaping. :) It still stands out in the final error message, and makes the code more manageable.

(Same below)

---

drake/multibody/parsing/sdf_model.h, line 118 at r1 (raw file):

                 const std::string& frame_name,
                 const Isometry3<double>& X_MF) {
    frame_cache_.Update(measured_in_frame_name, frame_name, X_MF);

BTW This method allows the possibility of the pose stored inside the link and the cached pose to disagree. It might be better to make the SDFLink::set_pose_in_model private, make SDFModel a friend of SDFLink and then have this call cache and set a single value in both places.

---

drake/multibody/parsing/sdf_model.h, line 129 at r1 (raw file):

 private:
  // Name of the root frame of this cache.

BTW copypasta?

---

drake/multibody/parsing/sdf_model.h, line 136 at r1 (raw file):

  // Mapping from link name to an index into std::vector links_.
  std::unordered_map<std::string, int> links_name_to_index_map_;

FYI This would be named slightly better as link_name_to_index_map_ (singular link instead of plural).

---

drake/multibody/parsing/sdf_model.h, line 136 at r1 (raw file):

  // Mapping from link name to an index into std::vector links_.
  std::unordered_map<std::string, int> links_name_to_index_map_;

This code doesn't make use of the indices in the API. Currently, you're accessing links/joints by name. Why not make the maps map strings to objects (which are all copyable/movable) instead of ints?

---

drake/multibody/parsing/sdf_model.h, line 142 at r1 (raw file):

  // Mapping from joint name to an index into std::vector joints_.
  std::unordered_map<std::string, int> joints_name_to_index_map_;

FYI Same comment as above: s/joints/joint

---

drake/multibody/parsing/sdf_parser.cc, line 8 at r1 (raw file):

#include <utility>

#include "sdf/sdf.hh"

BTW Swap " for <>

---

drake/multibody/parsing/sdf_parser.cc, line 27 at r1 (raw file):

// an Isometry3<double> instance.
Isometry3<double> ToIsometry(const ignition::math::Pose3d& pose) {
  const Isometry3<double>::TranslationType translation(ToVector(pose.Pos()));

FYI I wasn't aware of this TranslationType. Thanks. :)

---

drake/multibody/parsing/sdf_parser.cc, line 41 at r1 (raw file):

// Parses inertial information (mass, COM) from the given SDF
// element and updates the given body accordingly.

FYI s/body/link

---

drake/multibody/parsing/sdf_parser.cc, line 67 at r1 (raw file):

  // Parse the <inertia> element.
  if (sdf_inertial_element->HasElement("inertia")) {
    sdf::ElementPtr sdf_inertia_element =

FYI Two members named sdf_inertial_element and sdf_inertia_element are so close, that it could lead to reading issues down the road. An alternative name for one of them, perhaps? Certainly, the scope of the latter is so short, that a less evocative name would be appropriate.

---

drake/multibody/parsing/sdf_parser.cc, line 100 at r1 (raw file):

  // Parse the pose of the link's frame (L) pose in the model frame (D).
  auto X_DL = Isometry3<double>::Identity();

FYI See note (sdf_link.h) on preference for indicating "model frame" as M instead of D.

---

drake/multibody/parsing/sdf_parser.cc, line 107 at r1 (raw file):

  sdf_link.set_pose_in_model(X_DL);
  // "Remember" the pose of this new link in the model frame D.
  sdf_model->CachePose(sdf_model->name(), link_name, X_DL);

BTW See note in sdf_model.h about merging these two lines (well, three if I include the comment).

---

drake/multibody/parsing/sdf_parser.cc, line 115 at r1 (raw file):

  }

  // TODO(amcastro-tri): parse visual and collision elements.

FYI If you like: s/amcastro-tri/SeanCurtis-TRI

---

drake/multibody/parsing/sdf_parser.cc, line 119 at r1 (raw file):

// Parses a joint type from the given SDF element and instantiates the
// corresponding DrakeJoint instance. It is assumed that the parent body

BTW copypasta: DrakeJoint

---

drake/multibody/parsing/sdf_parser.cc, line 126 at r1 (raw file):

    SDFJoint* sdf_joint) {
  DRAKE_DEMAND(sdf_joint_element != nullptr);
  DRAKE_DEMAND(sdf_joint != nullptr);

FYI Strange ordering; perhaps cluster the demands on sdf_joint_element together?

---

drake/multibody/parsing/sdf_parser.cc, line 133 at r1 (raw file):

    sdf::ElementPtr sdf_axis_element = sdf_joint_element->GetElement("axis");
    sdf::ElementPtr sdf_xyz_element = sdf_axis_element->GetElement("xyz");
    // axis can be in either the "joint" frame M or in the model

I assume M for "mobilizer", yes? It might make more sense for this local code to have joint frame J and model frame M. (Related to earlier comments.)

---

drake/multibody/parsing/sdf_parser.cc, line 143 at r1 (raw file):

          sdf_joint_element->GetElement("use_parent_model_frame");
      if (sdf_use_parent_frame_element->Get<bool>()) {
        // Axis of rotation is defined in the model frame (D).

BTW This comment seems to be a lie.

---

drake/multibody/parsing/sdf_parser.h, line 22 at r1 (raw file):

  SDFParser() {}

  /// Parses a single `<model>` from file a file named `sdf_path`.

BTW Given the implementation, this would be better called ParseFirstModelFromFile and the documentation can be updated to be more explicit about what happens to a file with multiple models.

---

drake/multibody/parsing/sdf_parser.h, line 29 at r1 (raw file):

 private:
  /// Parses a <model> from the <sdf> element, referenced by
  /// `sdf_model_element`.

BTW replace /// with //

---

drake/multibody/parsing/sdf_parser.h, line 32 at r1 (raw file):
FYI This function (and the subsequent functions) have awkward language in having the "from ..." clause up front. It might be better to say:

Parses a new link from...

---

drake/multibody/parsing/sdf_parser.h, line 46 at r1 (raw file):

  void ParseJoint(::sdf::ElementPtr sdf_joint_element, SDFModel* sdf_model);

  // Parses joints by their particular joint type filling in addition fields

BTW I don't think this comment communicates what you want it to (and I suggest that, because it seems to contradict itself and the method).

---

drake/multibody/parsing/sdf_spec.h, line 21 at r1 (raw file):

/// values, please refer to the documentation for the
/// <a href="http://sdformat.org">SDFormat library</a>.
class SDFSpec {

Any particular reason why this class is fully defined in the header file?

---

drake/multibody/parsing/sdf_spec.h, line 41 at r1 (raw file):

  bool HasModel(const std::string& model_name) const {
    const auto it = model_ids_.find(model_name);
    return it != model_ids_.end();

FYI @jwnimmer-tri has turned me on to:

return model_ids_.count(model_name) > 0;

as a nice, compact version of this.

---

drake/multibody/parsing/sdf_spec.h, line 66 at r1 (raw file):

  // model_ids_.
  std::unordered_map<std::string, int>::const_iterator
  GetValidModelIteratorOrThrow(const std::string& model_name) const {

It is unclear to me why this is its own method; it is only ever called once by another method whose whole body is basically just calling this method. Is there a future version of this where this method is invoked in multiple sites?

---

drake/multibody/parsing/models/double_pendulum.sdf, line 1 at r1 (raw file):

<?xml version='1.0' ?>

The location of this file is surprising; does it belong here or in test? It seems to be used for testing.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 17 at r1 (raw file):

const double kEpsilon = std::numeric_limits<double>::epsilon();

static const char* const kTestSdfPath =

FYI Seems strange that this would have "global" scope, but all of the associated data (model names, joint names, link names, etc.) all have local scope in the test.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

    "drake/multibody/parsing/models/double_pendulum.sdf";

GTEST_TEST(SDFParserTest, ParsingTest) {

This parses a single file; but you have at least one branch that I suspect isn't being tested. (Specifically, the parse joint method based on what frame the axis is defined in.)

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 33 at r1 (raw file):

  EXPECT_EQ(model.name(), kModelName);
  EXPECT_EQ(sdf_spec->version(), "1.6");

FYI I'd move this test up to right after acquiring sdf_spec. This section of the code if model-centric.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 41 at r1 (raw file):

  const std::vector<SDFLink>& links = model.get_links();

  const auto& is_link_in_vector = [](

FYI I think the & is viable, but misleading. This would be slightly clearer as:

const auto is_link_in_vector =...;

In this case, you're declaring that you won't be re-assigning any other function, which seems to be the real intent.

---

Comments from Reviewable

[amcastro-tri]

Review status: 1 of 11 files reviewed at latest revision, 20 unresolved discussions.

---

drake/multibody/parsing/sdf_joint.h, line 26 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This last statement introduces some ambiguity. It is unclear whether this requirement is enforce or not; probably worth noting explicitly that it is not enforced. This applies to the other places (e.g., sdf_link.h where similar language is used).

Done.

---

drake/multibody/parsing/sdf_joint.h, line 63 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Given this aborts when called erroneously:

1. That behavior should be documented.
2. There should be a method to determine if it can be called safely.

(Same for set_axis.)

Done.

---

drake/multibody/parsing/sdf_link.h, line 37 at r1 (raw file):
BTW fair enough. I decided to refactor to J_Icm.

I understand N is also a common symbol for the inertial frame
not really, but a notation that Paul likes. However, be aware that he uses the word "inertial" meaning Newtonian! (hence the N).

SDF uses "inertial" just as a cute name to refer to the frame in which you specify "inertia" properties (i.e. inertia tensor).

---

drake/multibody/parsing/sdf_link.h, line 56 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI It isn't clear to me why we're using D to mean "parent model frame". Is there some intuition at play here that I'm missing? Otherwise, as mnemonics go, D isn't very satisfying.

I'm coming back around to this having read through the rest of the PR. I have a new comment (but I'm leaving the old one so you can see the confused thought process). I'd advocate eliminating the use of the word "parent" as defining the relationship between a link and its model. The term has very loaded meaning and I think it confuses things here. I'd say it's enough to say "pose of this links' frame in the link's parent model frame". And then I'd make the model frame M.

I agree. We are using M internally to refer to the outboard frame of a mobilzer and in some discussions we had in the past with Sherm and Paul we settled on this other letter, D. One of the disadvantages of using single letter frame names I suppose. I am happy as long as it is documented. Also, this documentation and implementation specifics will most likely change as someone owns this code. So don't get super attached to it :-)

---

drake/multibody/parsing/sdf_model.h, line 61 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI A comment about the fact that nothing is validated? (i.e., a user can reference a non-existent link and/or a joint type that doesn't actually exist?)

Generally, for this kind of thing, it might be worth adding documentation to the class documentation on all of these that this is a naive wrapper of the string data contained in the file -- it provides convenient access but does not guarantee the viability of the values.

Good point. I was thinking more like this class would have some sort of VerifyModel() (or Finalize()) that would do exactly that once you are done parsing before starting to use this representation on building the actual model. But also thought that could be a separate PR for whoever owns this code.
Added TODO with this observation.

---

drake/multibody/parsing/sdf_model.h, line 118 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This method allows the possibility of the pose stored inside the link and the cached pose to disagree. It might be better to make the SDFLink::set_pose_in_model private, make SDFModel a friend of SDFLink and then have this call cache and set a single value in both places.

good point. Removed both set/get_pose_in_model(). Now we can ask model.GetPosInModel(frame_name).

---

drake/multibody/parsing/sdf_model.h, line 136 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This code doesn't make use of the indices in the API. Currently, you're accessing links/joints by name. Why not make the maps map strings to objects (which are all copyable/movable) instead of ints?

the only reason so far is because I like how get_links() reads so simple, returning a std::vector. Clients of these classes (say MBT) will most likely get this vector, iterate through its elements, and create the actual model components. I can always do link.name() if I need the name. Thoughts?

---

drake/multibody/parsing/sdf_model.h, line 142 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI Same comment as above: s/joints/joint

same answer. Not super strong about that decision I made. I replied what my train of thought was but I can be convinced otherwise.

---

drake/multibody/parsing/sdf_parser.cc, line 27 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI I wasn't aware of this TranslationType. Thanks. :)

yeap, and we have drake::Translation3 in there.

---

drake/multibody/parsing/sdf_parser.cc, line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI Two members named sdf_inertial_element and sdf_inertia_element are so close, that it could lead to reading issues down the road. An alternative name for one of them, perhaps? Certainly, the scope of the latter is so short, that a less evocative name would be appropriate.

well... the tags are called <inertia> and <inertial>. I know the names are ugly this way by I opted to keep the parallelism with the xml element names.

---

drake/multibody/parsing/sdf_parser.cc, line 100 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI See note (sdf_link.h) on preference for indicating "model frame" as M instead of D.

Please see my reply there.

---

drake/multibody/parsing/sdf_parser.cc, line 115 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI If you like: s/amcastro-tri/SeanCurtis-TRI

Thanks!

---

drake/multibody/parsing/sdf_parser.cc, line 133 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I assume M for "mobilizer", yes? It might make more sense for this local code to have joint frame J and model frame M. (Related to earlier comments.)

We had some discussions about this. Not real clear what "joint frame" means or should mean. See related issue here. Something it'd be very nice to resolve in coordination with OSRC.

---

drake/multibody/parsing/sdf_parser.cc, line 143 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This comment seems to be a lie.

BTW, nope, that is true. If you are inside this if() your axis was provided in the model frame.

---

drake/multibody/parsing/sdf_spec.h, line 21 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Any particular reason why this class is fully defined in the header file?

No other than minimizing the number of files.

---

drake/multibody/parsing/sdf_spec.h, line 66 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

It is unclear to me why this is its own method; it is only ever called once by another method whose whole body is basically just calling this method. Is there a future version of this where this method is invoked in multiple sites?

Done.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This parses a single file; but you have at least one branch that I suspect isn't being tested. (Specifically, the parse joint method based on what frame the axis is defined in.)

I made <use_parent_model_frame> false for lower_joint in the sdf file. Clearly more testing would be needed. I'd prefer someone at OSRC to own those tests since to be honest I can imagine cases for which parsing an SDF can lead to unambiguous models. See #4615

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 41 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI I think the & is viable, but misleading. This would be slightly clearer as:

const auto is_link_in_vector =...;

In this case, you're declaring that you won't be re-assigning any other function, which seems to be the real intent.

Completely agree with you.

---

drake/multibody/parsing/models/double_pendulum.sdf, line 1 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The location of this file is surprising; does it belong here or in test? It seems to be used for testing.

Done.

---

Comments from Reviewable

[SeanCurtis-TRI]

Definitely ready for platform review. Still a couple of ticky tacky details to discuss.

---

Reviewed 5 of 12 files at r2, 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

drake/multibody/parsing/sdf_joint.h, line 49 at r3 (raw file):

      const std::string& child_link_name,
      const std::string& joint_type)
      : name_(joint_name),

BTW This has a style guide problem. If the initializer list doesn't fit on one line, it should be one item per line.

http://drake.mit.edu/styleguide/cppguide.html#Constructor_Initializer_Lists

---

drake/multibody/parsing/sdf_link.h, line 37 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

BTW fair enough. I decided to refactor to J_Icm.

I understand N is also a common symbol for the inertial frame
not really, but a notation that Paul likes. However, be aware that he uses the word "inertial" meaning Newtonian! (hence the N).

SDF uses "inertial" just as a cute name to refer to the frame in which you specify "inertia" properties (i.e. inertia tensor).

FYI I'm not even going to pretend to understand why J. It's not alluded to anywhere else in the documentation. But I'm assuming that this is some mathematical standard for the domain of which I'm unaware.

---

drake/multibody/parsing/sdf_link.h, line 19 at r3 (raw file):

/// <a href="http://sdformat.org/spec?ver=1.6&elem=link">
/// &lt;link&gt; element</a>.
class SdfLink {

FYI Nice catch. I totally missed that. :)

---

drake/multibody/parsing/sdf_model.h, line 136 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

the only reason so far is because I like how get_links() reads so simple, returning a std::vector. Clients of these classes (say MBT) will most likely get this vector, iterate through its elements, and create the actual model components. I can always do link.name() if I need the name. Thoughts?

I see the reasoning. I have a couple of thoughts that are completely non-binding:

1. So, this presumes that a consumer of this data can march through the std::vector<SdfLink> data in vector order without any problem. (Given your other assertion that you can't guarantee a "first" model based on XML structure, you also can't guarantee what order the links/joints will appear here.)
2. An alternative (that I've used in GeometryState is to provide a range iterator on the keys of the values of a map. That allows me to do something like:

SdfModel model;
 
for (const auto& link : model.get_links() {
   ...
}

---

drake/multibody/parsing/sdf_model.h, line 142 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

same answer. Not super strong about that decision I made. I replied what my train of thought was but I can be convinced otherwise.

(Sorry, the "same comment" alluded to singular vs plural. :) Not the int/string map thing. You addressed my issue.)

---

drake/multibody/parsing/sdf_parser.cc, line 133 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

We had some discussions about this. Not real clear what "joint frame" means or should mean. See related issue here. Something it'd be very nice to resolve in coordination with OSRC.

By the time I got to this comment, I'd had an add'l thought on the whole model/mobilizer conflict. I think it's unfortunate that the mobilizer (which is intended to be an under-the-hood implementation detail) has introduced notation that renders this public-level parsing to be unclear.

In the internals of drake, M can refer to a mobilizer frame. But because mobilizers don't appear anywhere near the sdf file, it's a shame that we can't make a local notation convention for this. That said, I accept things as is (even with some mild dissatisfaction).

---

drake/multibody/parsing/sdf_parser.cc, line 143 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

BTW, nope, that is true. If you are inside this if() your axis was provided in the model frame.

Good point -- it's the whole "parent model" nomenclature that caught me again. The fact that "parent frame" means something very different than "parent model frame" although it's easy to perform a cursory read and walk away with the wrong interpretation (I know, I've done it twice).

---

drake/multibody/parsing/sdf_parser.cc, line 120 at r3 (raw file):

// Parses a joint type from the given SDF element and reads its parameters
// accordingly into sdf_joint. It is assumed that the parent body frame is

BTW I don't think "accordingly" is correct or needed. You can just cut it.

---

drake/multibody/parsing/sdf_parser.cc, line 156 at r3 (raw file):

        // is applied.
        // Here we are doing: axis_M = R_MD * axis_D:
        axis = X_DM.inverse().linear() * axis;

FYI This surprised me. I wouldn't have thought this was better than simply inverting the linear(). (And, really, it seems what should be strictly necessary would be linear().transpose() as the most efficient. Why the change?

---

drake/multibody/parsing/sdf_parser.cc, line 204 at r3 (raw file):

// within the given tree.
//
// Returns the model instance id.

This appears to be a lie.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

I made <use_parent_model_frame> false for lower_joint in the sdf file. Clearly more testing would be needed. I'd prefer someone at OSRC to own those tests since to be honest I can imagine cases for which parsing an SDF can lead to unambiguous models. See #4615

Does that cause problems with the fact that we've included an OSRC license with the file? Do we have to document changes?

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Does that cause problems with the fact that we've included an OSRC license with the file? Do we have to document changes?

And does it need to be in third_party? The usual way forward would be to put a pristine version from upstream into the repo in third_party and then make customizations in a separate commit. It is Apache-2.0 so you are not required to document changes, but it makes life easier on maintainers if you do.

---

Comments from Reviewable

[amcastro-tri]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

drake/multibody/parsing/sdf_model.h, line 136 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I see the reasoning. I have a couple of thoughts that are completely non-binding:

1. So, this presumes that a consumer of this data can march through the std::vector<SdfLink> data in vector order without any problem. (Given your other assertion that you can't guarantee a "first" model based on XML structure, you also can't guarantee what order the links/joints will appear here.)
2. An alternative (that I've used in GeometryState is to provide a range iterator on the keys of the values of a map. That allows me to do something like:

SdfModel model;
 
for (const auto& link : model.get_links() {
   ...
}

as is, I can still do the same thing right? also, that'd require me to write extra code for the iterator.... (easy enough but not willing to spend my time on it now with other priorities on my list).
If you want I can add a TODO for whoever takes ownership of this.

---

drake/multibody/parsing/sdf_parser.cc, line 133 at r1 (raw file):

I assume M for "mobilizer", yes?

Not really, it refers to the "mobilized" frame. The mobilizer concept does not even appear and/or leak here. To define a joints and how it constraints two bodies precisely you will always need to define two frames, one on each body. We had decided to use F and M, yes mirroring what we used for Mobilizers but really here more like an arbitrary choice of names. We could probably in this scope call them Jp and Jc for "joint frame on parent" and "joint frame on child" if that helps the mental picture (and since parent/child is the nomenclature used by SDF).

---

drake/multibody/parsing/sdf_parser.cc, line 143 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Good point -- it's the whole "parent model" nomenclature that caught me again. The fact that "parent frame" means something very different than "parent model frame" although it's easy to perform a cursory read and walk away with the wrong interpretation (I know, I've done it twice).

I know, I had the same problem when I saw this tag....

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

And does it need to be in third_party? The usual way forward would be to put a pristine version from upstream into the repo in third_party and then make customizations in a separate commit. It is Apache-2.0 so you are not required to document changes, but it makes life easier on maintainers if you do.

I just followed the example in drake/examples/double_pendulum. I really don't know what the standard practice would be. I'm open to suggestions.

---

Comments from Reviewable

[SeanCurtis-TRI]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

drake/multibody/parsing/sdf_model.h, line 136 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

as is, I can still do the same thing right? also, that'd require me to write extra code for the iterator.... (easy enough but not willing to spend my time on it now with other priorities on my list).
If you want I can add a TODO for whoever takes ownership of this.

It was non-binding. If it doesn't inspire you to action, then that's fine. As is is sufficient.

---

drake/multibody/parsing/sdf_parser.cc, line 133 at r1 (raw file):
Your comment explicitly says:

the "joint" frame M

Hard to reconcile that is the "mobilized frame". And I don't know that the term "mobilized" frame is necessarily meaningful outside of Sherm's circle. If that's true, "M" for "mobilized" indicates that there is a leakage of the mobilizer concept. Notation should serve as a mnemonic. I don't think it succeeds in this documentation. Nevertheless, I'm marked as satisfied.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

I just followed the example in drake/examples/double_pendulum. I really don't know what the standard practice would be. I'm open to suggestions.

This kind of thing is outside my bailiwick. Others know more. I'm getting out of the way.

---

Comments from Reviewable

[amcastro-tri]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

a discussion (no related file):
Regarding the frame's nomenclature @SeanCurtis-TRI. I decided for new names that I think you'll like better. Also, after further thinking, I realize these new names are in better agreement with SDF's specification. The new frames are:

• L: a link frame.
• J: the "joint frame". As defined by the SDF spec, discussed now in the SdfJoint class's docs.
• M: now the Model frame (yaii!).
• Icm: the "inertial" frame. Since this, by definition, IS at the COM, it is perfectly valid to call it Icm in accordance to our monogram notation. Everybody's happy :-).
• P/C: Frames of the parent/child link's of a joint. Typically used in the context of joint parsing.

I think that covers them all. PTAL.

---

drake/multibody/parsing/sdf_parser.cc, line 133 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Your comment explicitly says:

the "joint" frame M

Hard to reconcile that is the "mobilized frame". And I don't know that the term "mobilized" frame is necessarily meaningful outside of Sherm's circle. If that's true, "M" for "mobilized" indicates that there is a leakage of the mobilizer concept. Notation should serve as a mnemonic. I don't think it succeeds in this documentation. Nevertheless, I'm marked as satisfied.

I realize you made some very good points during this discussion. Please see the description of my new updates in the thread above.

---

drake/multibody/parsing/sdf_parser.cc, line 204 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This appears to be a lie.

Done.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This kind of thing is outside my bailiwick. Others know more. I'm getting out of the way.

@jamiesnape, what do you recommend?, this is also outside my knowledge. Is it ok as it is now? with the disclaimer in the sdf file citing the source and mentioning that file was modified for our purposes?

---

Comments from Reviewable

[amcastro-tri]

+@sammy-tri for platform please.

---

Review status: 6 of 11 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[jamiesnape]

Reviewed 1 of 13 files at r1, 4 of 12 files at r2, 3 of 7 files at r3, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

@jamiesnape, what do you recommend?, this is also outside my knowledge. Is it ok as it is now? with the disclaimer in the sdf file citing the source and mentioning that file was modified for our purposes?

You are complying with the license and double_pendulum does the same, so this is probably fine.

---

Comments from Reviewable

[amcastro-tri]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

drake/multibody/parsing/test/sdf_parser_test.cc, line 20 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

You are complying with the license and double_pendulum does the same, so this is probably fine.

ok, thank you @jamiesnape

---

Comments from Reviewable

[SeanCurtis-TRI]

Reviewed 1 of 12 files at r2, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Regarding the frame's nomenclature @SeanCurtis-TRI. I decided for new names that I think you'll like better. Also, after further thinking, I realize these new names are in better agreement with SDF's specification. The new frames are:

• L: a link frame.
• J: the "joint frame". As defined by the SDF spec, discussed now in the SdfJoint class's docs.
• M: now the Model frame (yaii!).
• Icm: the "inertial" frame. Since this, by definition, IS at the COM, it is perfectly valid to call it Icm in accordance to our monogram notation. Everybody's happy :-).
• P/C: Frames of the parent/child link's of a joint. Typically used in the context of joint parsing.

I think that covers them all. PTAL.

You are correct, I love this. I'm certainly happy.

---

drake/multibody/parsing/sdf_parser.cc, line 133 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

I realize you made some very good points during this discussion. Please see the description of my new updates in the thread above.

As previously commented, I love the resolution.

---

Comments from Reviewable

[sammy-tri]

Partial first pass done. I don't think I'll be able to finish this today. Sorry for the delay.

---

Reviewed 1 of 13 files at r1, 3 of 12 files at r2, 1 of 7 files at r3, 2 of 5 files at r4.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

drake/multibody/parsing/sdf_joint.h, line 3 at r4 (raw file):

#pragma once

#include <limits>

unused?

---

drake/multibody/parsing/sdf_joint.h, line 4 at r4 (raw file):

#include <limits>
#include <map>

unused?

---

drake/multibody/parsing/sdf_joint.h, line 8 at r4 (raw file):

#include "drake/common/drake_copyable.h"
#include "drake/common/drake_throw.h"

consider using the narrower drake_assert.h since we don't use the extra functionality from drake_throw.h in this file

---

drake/multibody/parsing/sdf_link.h, line 3 at r4 (raw file):

#pragma once

#include <map>

unused?

---

drake/multibody/parsing/sdf_link.h, line 7 at r4 (raw file):

#include "drake/common/drake_copyable.h"
#include "drake/common/drake_throw.h"

unused?

---

drake/multibody/parsing/sdf_link.h, line 81 at r4 (raw file):

  // The pose of the <inertial> frame I in a measured-in frame L.
  // TODO(amcastro-tri): default value must be provided by sdformat library.

BTW I don't really understand this TODO, but that's probably fine if someone who knew this code better would get it.

---

drake/multibody/parsing/sdf_model.h, line 3 at r4 (raw file):

#pragma once

#include <map>

unused? (I think only unordered_map is used in this file?)

---

drake/multibody/parsing/sdf_model.h, line 9 at r4 (raw file):

#include "drake/common/drake_copyable.h"
#include "drake/common/drake_throw.h"

consider narrower drake_assert.h

---

drake/multibody/parsing/test/models/double_pendulum.sdf, line 1 at r4 (raw file):

<?xml version='1.0' ?>

BTW It's surprising to see such nearly identical files in both examples and in this test with the same name and different content. Is there some way to tell them apart like a comment (or filename change) to explain what makes one or the other special?

---

Comments from Reviewable

[sammy-tri]

First pass complete.

---

Reviewed 2 of 7 files at r3, 2 of 5 files at r4.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

drake/multibody/parsing/sdf_parser.cc, line 11 at r4 (raw file):

#include "drake/common/drake_assert.h"
#include "drake/common/drake_throw.h"

unused?

---

drake/multibody/parsing/sdf_parser.cc, line 24 at r4 (raw file):

// Helper function to express an ignition::math::Vector3d instance as
// a Vector3<double> instance.
Vector3<double> ToVector(const ignition::math::Vector3d& vector) {

The free functions in this file should be in the anonymous namespace. http://drake.mit.edu/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables

---

drake/multibody/parsing/sdf_parser.h, line 15 at r4 (raw file):

/// A new SdfSpec object is created which will contain the single model from
/// the file.
// TODO(amcastro-tri): throw an exception if more than one <model> is found.

Parsing only one model from the SDF when SdfSpec clearly has support for multiple models is an unusual decision. I think failing silently in the presence of multiple models, however, is clearly problematic. Can we resolve this TODO one way or the other?

---

drake/multibody/parsing/sdf_spec.h, line 9 at r4 (raw file):

#include "drake/common/drake_copyable.h"
#include "drake/common/drake_throw.h"

consider narrower drake_assert.h

---

drake/multibody/parsing/sdf_spec.h, line 17 at r4 (raw file):

namespace parsing {

/// This class provides a representation for an SDF specification.

Is "specification" a term of art in the SDF world to mean "a particular world defined by some SDF "? This seems confusing, as I think of the word "specification" as having the meaning in the next sentence (defining how to describe/implement something) rather than being a concrete instance of a thing meeting that description, which is how I think it's being used in this sentence (and in the name of the class).

---

Comments from Reviewable

[amcastro-tri]

Review status: 5 of 11 files reviewed at latest revision, 14 unresolved discussions.

---

drake/multibody/parsing/sdf_joint.h, line 3 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused?

Done.

---

drake/multibody/parsing/sdf_joint.h, line 4 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused?

Done.

---

drake/multibody/parsing/sdf_joint.h, line 8 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

consider using the narrower drake_assert.h since we don't use the extra functionality from drake_throw.h in this file

Done.

---

drake/multibody/parsing/sdf_link.h, line 3 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused?

Done.

---

drake/multibody/parsing/sdf_link.h, line 7 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused?

Done.

---

drake/multibody/parsing/sdf_link.h, line 81 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW I don't really understand this TODO, but that's probably fine if someone who knew this code better would get it.

I was trying to convey that idea that ideally default values should be provided somehow by the sdformat library so that if the SDF specification decides to change them we would not need to go searching through this code to make changes. Probably much better if this code lived as part of sdformat!

---

drake/multibody/parsing/sdf_model.h, line 3 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused? (I think only unordered_map is used in this file?)

Good call. Done.

---

drake/multibody/parsing/sdf_model.h, line 9 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

consider narrower drake_assert.h

Done.

---

drake/multibody/parsing/sdf_parser.cc, line 11 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

unused?

Done.

---

drake/multibody/parsing/sdf_parser.cc, line 24 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

The free functions in this file should be in the anonymous namespace. http://drake.mit.edu/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables

Done.

---

drake/multibody/parsing/sdf_parser.h, line 15 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Parsing only one model from the SDF when SdfSpec clearly has support for multiple models is an unusual decision. I think failing silently in the presence of multiple models, however, is clearly problematic. Can we resolve this TODO one way or the other?

I would love to but I already spent a lot more of time on this that I should. I actually tried but I found myself reverse engineering the sdformat library to figure what some methods do in the lack for documentation.
We are working for someone at OSRC to own this code and that is why you see the TODO's around. They will do a much better job that I can here since they own the sdformat library and can even modify it to make more specific queries.

---

drake/multibody/parsing/sdf_spec.h, line 9 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

consider narrower drake_assert.h

Done.

---

drake/multibody/parsing/sdf_spec.h, line 17 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Is "specification" a term of art in the SDF world to mean "a particular world defined by some SDF "? This seems confusing, as I think of the word "specification" as having the meaning in the next sentence (defining how to describe/implement something) rather than being a concrete instance of a thing meeting that description, which is how I think it's being used in this sentence (and in the name of the class).

yes, that is the term of art they use to describe their file format. See here. I make the class to be named SdfSpec in an attempt to meant that this describes what they call the "SDF specification". Open to ideas for a better name if this one doesn't seem appropriate.

---

drake/multibody/parsing/test/models/double_pendulum.sdf, line 1 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW It's surprising to see such nearly identical files in both examples and in this test with the same name and different content. Is there some way to tell them apart like a comment (or filename change) to explain what makes one or the other special?

BTW good idea. I added a comment with the changes introduced.

---

Comments from Reviewable

[amcastro-tri]

Thank you @sammy-tri!. Back to you.

---

Review status: 5 of 11 files reviewed at latest revision, 12 unresolved discussions.

---

Comments from Reviewable

[SeanCurtis-TRI]

Reviewed 1 of 12 files at r2, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

If this purpose of this PR is handing off a development branch to a different author, then maybe a dev folder is the best solution to having incomplete and heavily TODO'd code?

[amcastro-tri]

That's a fine solution. However, a lot of the TODO's have to do with things not offered by the sdformat library, so really many of them I could not even solved. So I was using the TODO's as a way of documenting those features we'd like to have. Maybe a github issue would be more appropriate? what do you think?

---

Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

Just a suggestion, I'm fine either way. If the code already meets the quality bar for non-dev then its better without the dev. I haven't read closely. I thought perhaps it was turning into a challenge to get it banged into well-documented enough. shape to pass non-dev review.

[sammy-tri]

(pending a fix to CI)

---

Reviewed 1 of 12 files at r2, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

drake/multibody/parsing/sdf_joint.h, line 3 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Done.

oops, CI says I was wrong about that one. Sorry!

---

drake/multibody/parsing/sdf_spec.h, line 17 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

yes, that is the term of art they use to describe their file format. See here. I make the class to be named SdfSpec in an attempt to meant that this describes what they call the "SDF specification". Open to ideas for a better name if this one doesn't seem appropriate.

Sigh, naming is hard... SdfRoot seems literally true but I don't like it. SdfElement sounds like something much more generic than what this is. SdfWorld makes it sound like it's the world element... SdfScene maybe? Or leave it if the feature reviewer doesn't mind, it's easy enough to change later.

---

Comments from Reviewable

[sammy-tri]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

drake/multibody/parsing/sdf_parser.h, line 15 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

I would love to but I already spent a lot more of time on this that I should. I actually tried but I found myself reverse engineering the sdformat library to figure what some methods do in the lack for documentation.
We are working for someone at OSRC to own this code and that is why you see the TODO's around. They will do a much better job that I can here since they own the sdformat library and can even modify it to make more specific queries.

It's not just as simple as calling sdf_element->GetNextElement("model"); in ParseSdfModelFromFile and throwing an exception if the return value isn't nullptr?

---

Comments from Reviewable

[amcastro-tri]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

drake/multibody/parsing/sdf_joint.h, line 3 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

oops, CI says I was wrong about that one. Sorry!

thanks. I should've checked myself actually.

---

drake/multibody/parsing/sdf_parser.h, line 15 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

It's not just as simple as calling sdf_element->GetNextElement("model"); in ParseSdfModelFromFile and throwing an exception if the return value isn't nullptr?

That does seem easy enough. Thanks. Done.

---

drake/multibody/parsing/sdf_spec.h, line 17 at r4 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Sigh, naming is hard... SdfRoot seems literally true but I don't like it. SdfElement sounds like something much more generic than what this is. SdfWorld makes it sound like it's the world element... SdfScene maybe? Or leave it if the feature reviewer doesn't mind, it's easy enough to change later.

SGTM, that was my thinking also.

---

Comments from Reviewable

[sammy-tri]

Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable