World: handle name collisions like Model (#1311)

* Add 1.6 world file for existing test
* Add failing 1.7 test
* Update Migration guide

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

Migration.md

@@ -12,6 +12,20 @@ forward programmatically.
 This document aims to contain similar information to those files
 but with improved human-readability..
 
+## libsdformat 13.x to 14.x
+
+### Additions
+
+1. **New SDFormat specification version 1.11**
+    + Details about the 1.10 to 1.11 transition are explained below in this same
+      document
+
+### Modifications
+
+1. World class only renames frames with name collisions if original file version
+   is 1.6 or earlier. Name collisions in newer files will cause DUPLICATE_NAME
+   errors, which now matches the behavior of the Model class.
+
 ## libsdformat 12.x to 13.x
 
 ### Additions
@@ -554,6 +568,10 @@ ABI was broken for `sdf::Element`, and restored on version 11.2.1.
     + Changed to `_fixed_joint_lump__` to avoid confusion with scoped names
     + [BitBucket pull request 245](https://osrf-migration.github.io/sdformat-gh-pages/#!/osrf/sdformat/pull-requests/245)
 
+## SDFormat specification 1.10 to 1.11
+
+### Additions
+
 ## SDFormat specification 1.9 to 1.10
 
 ### Additions

src/World.cc

@@ -18,6 +18,7 @@
 #include <unordered_set>
 #include <vector>
 #include <optional>
+#include <gz/math/SemanticVersion.hh>
 #include <gz/math/Vector3.hh>
 
 #include "sdf/Actor.hh"
@@ -134,6 +135,7 @@ Errors World::Load(sdf::ElementPtr _sdf, const ParserConfig &_config)
   Errors errors;
 
   this->dataPtr->sdf = _sdf;
+  gz::math::SemanticVersion sdfVersion(_sdf->OriginalVersion());
 
   // Check that the provided SDF element is a <world>
   // This is an error that cannot be recovered, so return an error.
@@ -284,21 +286,35 @@ Errors World::Load(sdf::ElementPtr _sdf, const ParserConfig &_config)
     std::string frameName = frame.Name();
     if (frameNames.count(frameName) > 0)
     {
-      frameName += "_frame";
-      int i = 0;
-      while (frameNames.count(frameName) > 0)
+      // This frame has a name collision
+      if (sdfVersion < gz::math::SemanticVersion(1, 7))
       {
-        frameName = frame.Name() + "_frame" + std::to_string(i++);
+        // This came from an old file, so try to workaround by renaming frame
+        frameName += "_frame";
+        int i = 0;
+        while (frameNames.count(frameName) > 0)
+        {
+          frameName = frame.Name() + "_frame" + std::to_string(i++);
+        }
+        std::stringstream ss;
+        ss << "Frame with name [" << frame.Name() << "] "
+           << "in world with name [" << this->Name() << "] "
+           << "has a name collision, changing frame name to ["
+           << frameName << "].";
+        Error err(ErrorCode::WARNING, ss.str());
+        enforceConfigurablePolicyCondition(
+            _config.WarningsPolicy(), err, errors);
+
+        frame.SetName(frameName);
+      }
+      else
+      {
+        std::stringstream ss;
+        ss << "Frame with name [" << frame.Name() << "] "
+           << "in world with name [" << this->Name() << "] "
+           << "has a name collision. Please rename this frame.";
+        errors.push_back({ErrorCode::DUPLICATE_NAME, ss.str()});
       }
-      std::stringstream ss;
-      ss << "Frame with name [" << frame.Name() << "] "
-          << "in world with name [" << this->Name() << "] "
-          << "has a name collision, changing frame name to ["
-          << frameName << "].\n";
-      Error err(ErrorCode::WARNING, ss.str());
-      enforceConfigurablePolicyCondition(
-          _config.WarningsPolicy(), err, errors);
-      frame.SetName(frameName);
     }
     frameNames.insert(frameName);
   }

test/integration/error_output.cc

@@ -359,7 +359,7 @@ TEST(ErrorOutput, WorldErrorOutput)
     "model with name[common_name] already exists."));
   EXPECT_NE(std::string::npos, errors[2].Message().find(
     "Frame with name [common_name] in world with name [test_world] has a name"
-    " collision, changing frame name to [common_name_frame]."));
+    " collision. Please rename this frame."));
   // Check nothing has been printed
   EXPECT_TRUE(buffer.str().empty()) << buffer.str();
 }

test/integration/world_dom.cc

@@ -157,7 +157,7 @@ TEST(DOMWorld, Load)
 TEST(DOMWorld, LoadModelFrameSameName)
 {
   const std::string testFile =
-    sdf::testing::TestFile("sdf", "world_model_frame_same_name.sdf");
+    sdf::testing::TestFile("sdf", "world_model_frame_same_name_1_6.sdf");
 
   // Load the SDF file
   sdf::Root root;
@@ -227,6 +227,22 @@ TEST(DOMWorld, LoadModelFrameSameName)
   EXPECT_EQ(Pose(0, -2, 3, 0, 0, 0), pose);
 }
 
+/////////////////////////////////////////////////
+TEST(DOMWorld, LoadModelFrameSameName_1_7)
+{
+  const std::string testFile =
+    sdf::testing::TestFile("sdf", "world_model_frame_same_name.sdf");
+
+  // Load the SDF file
+  sdf::Root root;
+  auto errors = root.Load(testFile);
+  for (auto e : errors)
+    std::cout << e << std::endl;
+  EXPECT_FALSE(errors.empty());
+  EXPECT_EQ(10u, errors.size());
+  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::DUPLICATE_NAME);
+}
+
 /////////////////////////////////////////////////
 TEST(DOMWorld, NestedModels)
 {

test/sdf/world_model_frame_same_name_1_6.sdf

@@ -0,0 +1,20 @@
+<?xml version="1.0" ?>
+<sdf version='1.6'>
+  <world name="world_model_frame_same_name">
+
+    <model name="base">
+      <pose>1 0 0 0 0 0</pose>
+      <link name="L"/>
+    </model>
+
+    <model name="ground">
+      <pose>0 2 0 0 0 0</pose>
+      <link name="L"/>
+    </model>
+
+    <frame name="ground">
+      <pose>0 0 3 0 0 0</pose>
+    </frame>
+
+  </world>
+</sdf>