[sdf9] Support nested models in DOM and frame semantics

[scpeters]

Nested model elements (//model/model) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal, this PR adds support for nested models in the DOM API and frame semantics (fixing #283) through three steps:

• adding Model::Model* methods for accessing nested models via the DOM API (047ec96)
• loading nested models in Model::Load (b57fea2, step 3 of model parsing stages)
• supporting nested models (//model/model) in frame semantics as opaque frames to match how models in the world scope (//world/model) are treated (85e0b4f, steps 6-9 of model parsing stages)

The first two steps are straightforward, while the 3rd step adds a bit of new behavior. I think it is reasonable to add this new behavior to libsdformat9 because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. Currently in libsdformat 9.2.0, a //world/model supports frame semantics: they have frames that can be reference by name with //pose/@relative_to and //world/frame/@attached_to values can resolve to a //world/model. This PR extends that same behavior to nested //model/model elements; they would have their own frames in the frame and pose graphs, and a //model/frame is permitted to resolve to a model.

This does not add support for referencing elements within a model via the :: syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see #293).

A companion pull request to sdf_tutorials will be submitted that documents the changes proposed in this pull request. This pull request bumps the minor version to 9.3. I believe we can make this behavior change because libsdformat9 has not been included as an official package in Ubuntu 20.04, so we don't have to worry about lots of people being stuck on an old version. Version 9.2 is currently in packages.ros.org, but I think we could get that version updated. (cc @j-rivero)

---

This change is 

[codecov-commenter]

Codecov Report

Merging #316 into sdf9 will decrease coverage by 0.21%.
The diff coverage is 69.60%.

@@            Coverage Diff             @@
##             sdf9     #316      +/-   ##
==========================================
- Coverage   86.40%   86.18%   -0.22%     
==========================================
  Files          59       59              
  Lines        9053     9152      +99     
==========================================
+ Hits         7822     7888      +66     
- Misses       1231     1264      +33     

Impacted Files	Coverage Δ
src/parser.cc	77.43% <50.00%> (+0.02%)	⬆️
src/FrameSemantics.cc	76.23% <68.18%> (-0.99%)	⬇️
src/Model.cc	82.22% <69.81%> (-2.84%)	⬇️
src/SemanticPose.cc	100.00% <100.00%> (ø)
src/World.cc	93.12% <100.00%> (+0.05%)	⬆️

---

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 99b7d9f...82b03f6. Read the comment docs.

[EricCousineau-TRI]

Reviewable link: https://reviewable.io/reviews/osrf/sdformat/316#-

[EricCousineau-TRI]

Reviewed 3 of 15 files at r1.
Reviewable status: 3 of 15 files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, @iche033, and @scpeters)

a discussion (no related file):
In the overview, could you perhaps add permalinks cross-referencing the proposal sections you're implementing?

(And when this merges, can you ensure the overview + PR number is included in the commit?)

---

[EricCousineau-TRI]

Reviewed 4 of 15 files at r1.
Reviewable status: 7 of 15 files reviewed, 2 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, @iche033, and @scpeters)

---

src/FrameSemantics.cc, line 557 at r1 (raw file):

  }

  // add nested model vertices and default edge if relative_to is empty

nit This graph construction relies heavily on the assumption of fully scoped / "unrolled" names.

Can you add a comment to that effect in this header file? (if it's not already there?)
(Just so it's easy to see the change once the scoping is implemented, if it's done in a more structure fashion?)

---

src/ign_TEST.cc, line 296 at r1 (raw file):

    std::string output =
      custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion);
    EXPECT_EQ("Valid.\n", output) << output;

BTW Woot!

[EricCousineau-TRI]

pending minor comments and request for negative test for drill-down

Reviewed 8 of 15 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @azeey, @iche033, and @scpeters)

---

test/sdf/model_nested_model_relative_to.sdf, line 1 at r1 (raw file):

<?xml version="1.0" ?>

Can you add a (brief) test that shows how drilling down will fail, and xref the issue? (#293)

---

test/sdf/model_nested_model_relative_to.sdf, line 2 at r1 (raw file):

<?xml version="1.0" ?>
<sdf version='1.7'>

nit Can you add a comment that this does not yet support drilling, and xref the issue? (#293)

[azeey]

LGTM! It would be good to add tests showing how sibling nested models can be used as joint parent/child frames when we merge this forward to sdf10. Specifically the case where the nested model is static might need attention.

Reviewed 5 of 15 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @iche033 and @scpeters)

---

src/FrameSemantics.cc, line 603 at r1 (raw file):

          "relative_to name[" + relativeTo +
          "] specified by link with name[" + link->Name() +
          "] does not match a link, joint, or frame name "

nit: Do we also need to add "nested model" here?

---

src/Model.cc, line 700 at r1 (raw file):

  if (!graph)
  {
    errors.push_back({ErrorCode::ELEMENT_INVALID,

Could the error code be made more specific to frame semantics? In Gazebo11, we ignore errors not related to frame semantics.

---

test/integration/model_dom.cc, line 256 at r1 (raw file):

  }
  EXPECT_TRUE(
    model->ModelByName("M1")->SemanticPose().Resolve(pose,

nit: this could be simplified to errors->empty()

[iche033]

I've been testing this PR with gazebosim/gz-sim#258 (mostly with SDF DOM but not frame semantics) and it's working fine.

One case of nested model that used to work in gazebo classic but no longer works now is:

<model>
  <model>
     <link />
  </model>
</model>

Here it complains that the top level model has no links. Do we want to support this use case? On the ign-gazebo and ign-physics side, they are already setup to work with canonical links inside nested models so that's taken care of. So just checking to see if this is possible on the frame semantics side?

[EricCousineau-TRI]

Oh... dern haha, we missed that entirely. I think we should make it work, b/c it makes sense.
Will file a PR to sdf_tutorials to try to capture this.

[EricCousineau-TRI]

Here's a (kinda messy) proposed change: gazebosim/sdf_tutorials#36

[scpeters]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

---

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This graph construction relies heavily on the assumption of fully scoped / "unrolled" names.

Can you add a comment to that effect in this header file? (if it's not already there?)
(Just so it's easy to see the change once the scoping is implemented, if it's done in a more structure fashion?)

I'm not exactly sure what you mean by fully scoped / "unrolled" names

---

src/FrameSemantics.cc, line 603 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: Do we also need to add "nested model" here?

I updated this error message and several others in 7353fac

---

src/Model.cc, line 700 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

Could the error code be made more specific to frame semantics? In Gazebo11, we ignore errors not related to frame semantics.

I updated this error type along with a similar one in SemanticPose::Resolve in 61af87b

---

test/integration/model_dom.cc, line 256 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: this could be simplified to errors->empty()

instead I removed the print statement that I had added while debugging in 384c759

---

test/sdf/model_nested_model_relative_to.sdf, line 1 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you add a (brief) test that shows how drilling down will fail, and xref the issue? (#293)

I added an example file with a test that shows how drilling down will fail when specifying a nested link as a joint child in 33cfe4d. Should I add a similar example for :: in //pose/@relative_to?

[scpeters]

Reviewed 10 of 15 files at r1, 9 of 9 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

[EricCousineau-TRI]

Reviewed 6 of 9 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @azeey and @scpeters)

---

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

I'm not exactly sure what you mean by fully scoped / "unrolled" names

Ah, sorry. What I mean is that if I have two levels of nested models, e.g. Ma { Mb { Mc }}}, and for each model, I have L1 - J1 - L2, what should the names look like in this graph?

Should they always be absolute? e.g. Ma::Mb::Mc::L1, Ma::J1, etc.?
Or might they be relative? e.g. J1 in each respective scope?

I assume it's absolute. In that case, could a comment be added to FrameSemantics.hh stating that contract? e.g.

/// \warning All names passed to this graph must be *absolute*, e.g. `model::nested_model::my_frame`.

---

test/sdf/model_invalid_nested_joint_child.sdf, line 11 at r2 (raw file):

    <joint name="J" type="fixed">
      <parent>P</parent>
      <child>M::C</child>

Can you add a comment stating this will cause failure, and xref the issue (#293)?

---

test/sdf/model_nested_model_relative_to.sdf, line 1 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

I added an example file with a test that shows how drilling down will fail when specifying a nested link as a joint child in 33cfe4d. Should I add a similar example for :: in //pose/@relative_to?

Nah, I think that test is fine. Thanks!

---

test/sdf/model_nested_model_relative_to.sdf, line 2 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you add a comment that this does not yet support drilling, and xref the issue? (#293)

OK Retracting based on above discussion. (Moving to negative test.)

[scpeters]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

---

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, sorry. What I mean is that if I have two levels of nested models, e.g. Ma { Mb { Mc }}}, and for each model, I have L1 - J1 - L2, what should the names look like in this graph?

Should they always be absolute? e.g. Ma::Mb::Mc::L1, Ma::J1, etc.?
Or might they be relative? e.g. J1 in each respective scope?

I assume it's absolute. In that case, could a comment be added to FrameSemantics.hh stating that contract? e.g.

/// \warning All names passed to this graph must be *absolute*, e.g. `model::nested_model::my_frame`.

there are no absolute names with :: in this PR; they are still all relative. each model has its own set of graphs using relative names and is treated as an opaque frame by whatever model or world contains it.

---

test/sdf/model_invalid_nested_joint_child.sdf, line 11 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you add a comment stating this will cause failure, and xref the issue (#293)?

added in 4e99441

[EricCousineau-TRI]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

---

include/sdf/Model.hh, line 307 at r3 (raw file):

    /// \brief Give a weak pointer to the PoseRelativeToGraph to be used
    /// for resolving poses. This is private and is intended to be called by
    /// World::Load.

I believe this is called directly by things other than World::Load.

Can you update this to reflect that parent models will call this also?

---

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

there are no absolute names with :: in this PR; they are still all relative. each model has its own set of graphs using relative names and is treated as an opaque frame by whatever model or world contains it.

Gotcha. I looked at Model::SetPoseRelativeToGraph and see what you mean, as well as ModelPrivate::poseGraph, parentPoseGraph.

Can you indicate that this is the intended usage in FrameSemantics.hh, though?
e.g.

/// \ingroup sdf_frame_semantics
...
/// Note that all graphs should only contain relative names (e.g. "my_link"), not absolute names
/// ("top_model::nested_model::my_link"). Graphs across nested models
/// (currently via directly nested models in //model or //world elements) will be explicitly
/// separate graphs.
namespace sdf {
...
}

Then once we handle drilling down, we can revisit this assumption and explicitly show it in code diffs?

[EricCousineau-TRI]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @azeey and @scpeters)

[azeey]

Reviewed 5 of 15 files at r1, 8 of 9 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scpeters)

[EricCousineau-TRI]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @scpeters)

a discussion (no related file):
Per VC today, we think this should land once the proposal lands and is implemented for nested canonical links (perhaps in this PR?):
gazebosim/sdf_tutorials#36

---

[scpeters]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)

---

include/sdf/Model.hh, line 307 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I believe this is called directly by things other than World::Load.

Can you update this to reflect that parent models will call this also?

ok, fixed in eee2f64

---

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Gotcha. I looked at Model::SetPoseRelativeToGraph and see what you mean, as well as ModelPrivate::poseGraph, parentPoseGraph.

Can you indicate that this is the intended usage in FrameSemantics.hh, though?
e.g.

/// \ingroup sdf_frame_semantics
...
/// Note that all graphs should only contain relative names (e.g. "my_link"), not absolute names
/// ("top_model::nested_model::my_link"). Graphs across nested models
/// (currently via directly nested models in //model or //world elements) will be explicitly
/// separate graphs.
namespace sdf {
...
}

Then once we handle drilling down, we can revisit this assumption and explicitly show it in code diffs?

ok, fixed in eee2f64

[EricCousineau-TRI]

Per VC (again), the nested canonical link proposal may be implemented as part of this PR, or a preceding PR.

[scpeters]

Reviewable status: 19 of 20 files reviewed, 3 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

In the overview, could you perhaps add permalinks cross-referencing the proposal sections you're implementing?

(And when this merges, can you ensure the overview + PR number is included in the commit?)

I've updated the pull request description with links to the modified parsing stages.

---

[EricCousineau-TRI]

Reviewed 3 of 9 files at r2, 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)

[scpeters]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Per VC today, we think this should land once the proposal lands and is implemented for nested canonical links (perhaps in this PR?):
gazebosim/sdf_tutorials#36

I've started prototyping support for nested canonical links in libsdformat9 in scpeters@6c01718, but I'm not sure how much :: syntax we want to backport into SDFormat 1.7 (see the line in the proposal that says <model name="top" canonical_link="nested::link">).

I was very careful to avoid using :: syntax in gazebosim/sdf_tutorials#34, so I'd prefer to avoid it for the 1.7 implementation of gazebosim/sdf_tutorials#36 as well. :: syntax will be part of SDFormat 1.8 of course

---

[EricCousineau-TRI]

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

I've started prototyping support for nested canonical links in libsdformat9 in scpeters@6c01718, but I'm not sure how much :: syntax we want to backport into SDFormat 1.7 (see the line in the proposal that says <model name="top" canonical_link="nested::link">).

I was very careful to avoid using :: syntax in gazebosim/sdf_tutorials#34, so I'd prefer to avoid it for the 1.7 implementation of gazebosim/sdf_tutorials#36 as well. :: syntax will be part of SDFormat 1.8 of course

OK SGTM per this and Slack convo.

---

[azeey]

Reviewable status: complete! all files reviewed, all discussions resolved