Explicitly nested canonical links with :: (#355)

This extends the `Model::*NameExists` and `Model::*ByName` APIs
(like LinkNameExists and LinkByName) that allow passing
nested names that can begin with a sequence of nested model
names separated by :: and may end with the name of an object
of the specified type, such as "outer_model::inner_model::inner_joint".

For now, if a nested model is not found that matches the nested name
preceding the final ::, then it checks for objects in the current model
that match the entire name. This extra check should be disabled
when "::" is reserved and not allowed in frame names.

Add test showing that Model::ModelByName and Model::ModelNameExists
have trouble with model names that contain "::" with TODO
comments to remind that they should be fixed.

* README: note master is unstable development branch

Split nested_invalid_explicit_canonical_link.sdf
into nested_explicit_canonical_link.sdf and
nested_without_links_invalid.sdf and update tests
to expect that nested_explicit_canonical_link.sdf is valid.

Part of #342.

Signed-off-by: Steve Peters <[email protected]>

Changelog.md

@@ -7,6 +7,15 @@
 1. Initial version of SDFormat 1.8 specification.
     * [BitBucket pull request 682](https://osrf-migration.github.io/sdformat-gh-pages/#!/osrf/sdformat/pull-requests/682)
 
+1. SDFormat 1.8: Support specifying frames as joint child/parent.
+    * [Pull request 304](https://github.com/osrf/sdformat/pull/304)
+
+1. SDFormat 1.8: Add support for `//include/placement_frame` and `//model/@placement_frame`.
+    * [Pull request 324](https://github.com/osrf/sdformat/pull/324)
+
+1. SDFormat 1.8: Support explicit nested canonical links with :: syntax.
+    * [Pull request 353](https://github.com/osrf/sdformat/pull/353)
+
 1. SDFormat 1.8: Deprecate //joint/axis/initial_position.
     * [BitBucket pull request 683](https://osrf-migration.github.io/sdformat-gh-pages/#!/osrf/sdformat/pull-requests/683)
 

Migration.md

@@ -20,6 +20,18 @@ but with improved human-readability..
     + Errors ResolveChildLink(std::string&) const
     + Errors ResolveParentLink(std::string&) const
 
+### Modifications
+
+1. **sdf/Model.hh**: the following methods now accept nested names
+      that can begin with a sequence of nested model names separated
+      by `::` and may end with the name of an object of the specified type.
+    + const Frame \*FrameByName(const std::string &) const
+    + const Joint \*JointByName(const std::string &) const
+    + const Link \*LinkByName(const std::string &) const
+    + bool FrameNameExists(const std::string &) const
+    + bool JointNameExists(const std::string &) const
+    + bool LinkNameExists(const std::string &) const
+
 ## SDFormat 9.x to 10.0
 
 ### Modifications

README.md

@@ -35,6 +35,10 @@ TODO(eric.cousineau): Move terminology section to sdf_tutorials?
 
 ## Installation
 
+**Note:** the `master` branch is under development for `libsdformat11` and is
+currently unstable. A release branch (`sdf10`, `sdf9`, etc.) is recommended
+for most users.
+
 Standard installation can be performed in UNIX systems using the following
 steps:
 

include/sdf/Model.hh

@@ -145,11 +145,15 @@ namespace sdf
 
     /// \brief Get a link based on a name.
     /// \param[in] _name Name of the link.
+    /// To get a link in a nested model, prefix the link name with the
+    /// sequence of nested models containing this link, delimited by "::".
     /// \return Pointer to the link. Nullptr if the name does not exist.
     public: const Link *LinkByName(const std::string &_name) const;
 
     /// \brief Get whether a link name exists.
     /// \param[in] _name Name of the link to check.
+    /// To check for a link in a nested model, prefix the link name with
+    /// the sequence of nested models containing this link, delimited by "::".
     /// \return True if there exists a link with the given name.
     public: bool LinkNameExists(const std::string &_name) const;
 
@@ -166,11 +170,15 @@ namespace sdf
 
     /// \brief Get whether a joint name exists.
     /// \param[in] _name Name of the joint to check.
+    /// To check for a joint in a nested model, prefix the link name with
+    /// the sequence of nested models containing this joint, delimited by "::".
     /// \return True if there exists a joint with the given name.
     public: bool JointNameExists(const std::string &_name) const;
 
     /// \brief Get a joint based on a name.
     /// \param[in] _name Name of the joint.
+    /// To get a joint in a nested model, prefix the joint name with the
+    /// sequence of nested models containing this joint, delimited by "::".
     /// \return Pointer to the joint. Nullptr if a joint with the given name
     ///  does not exist.
     /// \sa bool JointNameExists(const std::string &_name) const
@@ -190,12 +198,16 @@ namespace sdf
 
     /// \brief Get an explicit frame based on a name.
     /// \param[in] _name Name of the explicit frame.
+    /// To get a frame in a nested model, prefix the frame name with the
+    /// sequence of nested models containing this frame, delimited by "::".
     /// \return Pointer to the explicit frame. Nullptr if the name does not
     /// exist.
     public: const Frame *FrameByName(const std::string &_name) const;
 
     /// \brief Get whether an explicit frame name exists.
     /// \param[in] _name Name of the explicit frame to check.
+    /// To check for a frame in a nested model, prefix the frame name with
+    /// the sequence of nested models containing this frame, delimited by "::".
     /// \return True if there exists an explicit frame with the given name.
     public: bool FrameNameExists(const std::string &_name) const;
 
@@ -212,11 +224,15 @@ namespace sdf
 
     /// \brief Get whether a nested model name exists.
     /// \param[in] _name Name of the nested model to check.
+    /// To check for a model nested in other models, prefix the model name
+    /// with the sequence of nested model names, delimited by "::".
     /// \return True if there exists a nested model with the given name.
     public: bool ModelNameExists(const std::string &_name) const;
 
     /// \brief Get a nested model based on a name.
     /// \param[in] _name Name of the nested model.
+    /// To get a model nested in other models, prefix the model name
+    /// with the sequence of nested model names, delimited by "::".
     /// \return Pointer to the model. Nullptr if a model with the given name
     ///  does not exist.
     /// \sa bool ModelNameExists(const std::string &_name) const

src/Model.cc

@@ -579,14 +579,7 @@ const Link *Model::LinkByIndex(const uint64_t _index) const
 /////////////////////////////////////////////////
 bool Model::LinkNameExists(const std::string &_name) const
 {
-  for (auto const &l : this->dataPtr->links)
-  {
-    if (l.Name() == _name)
-    {
-      return true;
-    }
-  }
-  return false;
+  return nullptr != this->LinkByName(_name);
 }
 
 /////////////////////////////////////////////////
@@ -606,19 +599,28 @@ const Joint *Model::JointByIndex(const uint64_t _index) const
 /////////////////////////////////////////////////
 bool Model::JointNameExists(const std::string &_name) const
 {
-  for (auto const &j : this->dataPtr->joints)
-  {
-    if (j.Name() == _name)
-    {
-      return true;
-    }
-  }
-  return false;
+  return nullptr != this->JointByName(_name);
 }
 
 /////////////////////////////////////////////////
 const Joint *Model::JointByName(const std::string &_name) const
 {
+  auto index = _name.rfind("::");
+  if (index != std::string::npos)
+  {
+    const Model *model = this->ModelByName(_name.substr(0, index));
+    if (nullptr != model)
+    {
+      return model->JointByName(_name.substr(index + 2));
+    }
+
+    // The nested model name preceding the last "::" could not be found.
+    // For now, try to find a link that matches _name exactly.
+    // When "::" are reserved and not allowed in names, then uncomment
+    // the following line to return a nullptr.
+    // return nullptr;
+  }
+
   for (auto const &j : this->dataPtr->joints)
   {
     if (j.Name() == _name)
@@ -646,19 +648,28 @@ const Frame *Model::FrameByIndex(const uint64_t _index) const
 /////////////////////////////////////////////////
 bool Model::FrameNameExists(const std::string &_name) const
 {
-  for (auto const &f : this->dataPtr->frames)
-  {
-    if (f.Name() == _name)
-    {
-      return true;
-    }
-  }
-  return false;
+  return nullptr != this->FrameByName(_name);
 }
 
 /////////////////////////////////////////////////
 const Frame *Model::FrameByName(const std::string &_name) const
 {
+  auto index = _name.rfind("::");
+  if (index != std::string::npos)
+  {
+    const Model *model = this->ModelByName(_name.substr(0, index));
+    if (nullptr != model)
+    {
+      return model->FrameByName(_name.substr(index + 2));
+    }
+
+    // The nested model name preceding the last "::" could not be found.
+    // For now, try to find a link that matches _name exactly.
+    // When "::" are reserved and not allowed in names, then uncomment
+    // the following line to return a nullptr.
+    // return nullptr;
+  }
+
   for (auto const &f : this->dataPtr->frames)
   {
     if (f.Name() == _name)
@@ -686,27 +697,30 @@ const Model *Model::ModelByIndex(const uint64_t _index) const
 /////////////////////////////////////////////////
 bool Model::ModelNameExists(const std::string &_name) const
 {
-  for (auto const &m : this->dataPtr->models)
-  {
-    if (m.Name() == _name)
-    {
-      return true;
-    }
-  }
-  return false;
+  return nullptr != this->ModelByName(_name);
 }
 
 /////////////////////////////////////////////////
 const Model *Model::ModelByName(const std::string &_name) const
 {
+  auto index = _name.find("::");
+  const std::string nextModelName = _name.substr(0, index);
+  const Model *nextModel = nullptr;
+
   for (auto const &m : this->dataPtr->models)
   {
-    if (m.Name() == _name)
+    if (m.Name() == nextModelName)
     {
-      return &m;
+      nextModel = &m;
+      break;
     }
   }
-  return nullptr;
+
+  if (nullptr != nextModel && index != std::string::npos)
+  {
+    return nextModel->ModelByName(_name.substr(index + 2));
+  }
+  return nextModel;
 }
 
 /////////////////////////////////////////////////
@@ -832,6 +846,22 @@ sdf::SemanticPose Model::SemanticPose() const
 /////////////////////////////////////////////////
 const Link *Model::LinkByName(const std::string &_name) const
 {
+  auto index = _name.rfind("::");
+  if (index != std::string::npos)
+  {
+    const Model *model = this->ModelByName(_name.substr(0, index));
+    if (nullptr != model)
+    {
+      return model->LinkByName(_name.substr(index + 2));
+    }
+
+    // The nested model name preceding the last "::" could not be found.
+    // For now, try to find a link that matches _name exactly.
+    // When "::" are reserved and not allowed in names, then uncomment
+    // the following line to return a nullptr.
+    // return nullptr;
+  }
+
   for (auto const &l : this->dataPtr->links)
   {
     if (l.Name() == _name)

src/Model_TEST.cc

@@ -52,32 +52,56 @@ TEST(DOMModel, Construction)
   EXPECT_EQ(nullptr, model.ModelByIndex(1));
   EXPECT_EQ(nullptr, model.ModelByName(""));
   EXPECT_EQ(nullptr, model.ModelByName("default"));
+  EXPECT_EQ(nullptr, model.ModelByName("a::b"));
+  EXPECT_EQ(nullptr, model.ModelByName("a::b::c"));
+  EXPECT_EQ(nullptr, model.ModelByName("::::"));
   EXPECT_FALSE(model.ModelNameExists(""));
   EXPECT_FALSE(model.ModelNameExists("default"));
+  EXPECT_FALSE(model.ModelNameExists("a::b"));
+  EXPECT_FALSE(model.ModelNameExists("a::b::c"));
+  EXPECT_FALSE(model.ModelNameExists("::::"));
 
   EXPECT_EQ(0u, model.LinkCount());
   EXPECT_EQ(nullptr, model.LinkByIndex(0));
   EXPECT_EQ(nullptr, model.LinkByIndex(1));
   EXPECT_EQ(nullptr, model.LinkByName(""));
   EXPECT_EQ(nullptr, model.LinkByName("default"));
+  EXPECT_EQ(nullptr, model.LinkByName("a::b"));
+  EXPECT_EQ(nullptr, model.LinkByName("a::b::c"));
+  EXPECT_EQ(nullptr, model.LinkByName("::::"));
   EXPECT_FALSE(model.LinkNameExists(""));
   EXPECT_FALSE(model.LinkNameExists("default"));
+  EXPECT_FALSE(model.LinkNameExists("a::b"));
+  EXPECT_FALSE(model.LinkNameExists("a::b::c"));
+  EXPECT_FALSE(model.LinkNameExists("::::"));
 
   EXPECT_EQ(0u, model.JointCount());
   EXPECT_EQ(nullptr, model.JointByIndex(0));
   EXPECT_EQ(nullptr, model.JointByIndex(1));
   EXPECT_EQ(nullptr, model.JointByName(""));
   EXPECT_EQ(nullptr, model.JointByName("default"));
+  EXPECT_EQ(nullptr, model.JointByName("a::b"));
+  EXPECT_EQ(nullptr, model.JointByName("a::b::c"));
+  EXPECT_EQ(nullptr, model.JointByName("::::"));
   EXPECT_FALSE(model.JointNameExists(""));
   EXPECT_FALSE(model.JointNameExists("default"));
+  EXPECT_FALSE(model.JointNameExists("a::b"));
+  EXPECT_FALSE(model.JointNameExists("a::b::c"));
+  EXPECT_FALSE(model.JointNameExists("::::"));
 
   EXPECT_EQ(0u, model.FrameCount());
   EXPECT_EQ(nullptr, model.FrameByIndex(0));
   EXPECT_EQ(nullptr, model.FrameByIndex(1));
   EXPECT_EQ(nullptr, model.FrameByName(""));
   EXPECT_EQ(nullptr, model.FrameByName("default"));
+  EXPECT_EQ(nullptr, model.FrameByName("a::b"));
+  EXPECT_EQ(nullptr, model.FrameByName("a::b::c"));
+  EXPECT_EQ(nullptr, model.FrameByName("::::"));
   EXPECT_FALSE(model.FrameNameExists(""));
   EXPECT_FALSE(model.FrameNameExists("default"));
+  EXPECT_FALSE(model.FrameNameExists("a::b"));
+  EXPECT_FALSE(model.FrameNameExists("a::b::c"));
+  EXPECT_FALSE(model.FrameNameExists("::::"));
 
   EXPECT_TRUE(model.CanonicalLinkName().empty());
   EXPECT_EQ(nullptr, model.CanonicalLink());

src/ign_TEST.cc

@@ -372,14 +372,21 @@ TEST(check, SDF)
   // that is explicitly specified by //model/@canonical_link using ::
   // syntax.
   {
-    std::string path = pathBase +"/nested_invalid_explicit_canonical_link.sdf";
+    std::string path = pathBase +"/nested_explicit_canonical_link.sdf";
 
-    // Check nested_invalid_explicit_canonical_link.sdf
+    // Check nested_explicit_canonical_link.sdf
+    std::string output =
+      custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion);
+    EXPECT_EQ("Valid.\n", output) << output;
+  }
+
+  // Check an SDF file with a model that a nested model without a link.
+  {
+    std::string path = pathBase +"/nested_without_links_invalid.sdf";
+
+    // Check nested_without_links_invalid.sdf
     std::string output =
       custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion);
-    EXPECT_NE(output.find("Error: canonical_link with name[nested::link] not "
-                          "found in model with name[top]."),
-              std::string::npos) << output;
     EXPECT_NE(output.find("Error: A model must have at least one link."),
               std::string::npos) << output;
   }