diff --git a/Migration.md b/Migration.md index 9358a0087..0ad0a3400 100644 --- a/Migration.md +++ b/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 diff --git a/src/World.cc b/src/World.cc index aacb571c9..8cedf399d 100644 --- a/src/World.cc +++ b/src/World.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #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 // 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); } diff --git a/test/integration/error_output.cc b/test/integration/error_output.cc index 6c7378db5..16585152f 100644 --- a/test/integration/error_output.cc +++ b/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(); } diff --git a/test/integration/world_dom.cc b/test/integration/world_dom.cc index ff40132ac..fdc5bd3a3 100644 --- a/test/integration/world_dom.cc +++ b/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) { diff --git a/test/sdf/world_model_frame_same_name_1_6.sdf b/test/sdf/world_model_frame_same_name_1_6.sdf new file mode 100644 index 000000000..b304f2ab4 --- /dev/null +++ b/test/sdf/world_model_frame_same_name_1_6.sdf @@ -0,0 +1,20 @@ + + + + + + 1 0 0 0 0 0 + + + + + 0 2 0 0 0 0 + + + + + 0 0 3 0 0 0 + + + +