From 6d9aa644fc818b302623ea19963d60a93c5a8fbd Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Thu, 16 Jan 2020 18:33:34 -0800 Subject: [PATCH 1/4] Model::Load: fail fast if an sdf 1.7 file has name collisions between joints and links instead of making a workaround --- src/Model.cc | 31 ++++++++++++++++++++++--------- test/integration/joint_dom.cc | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/Model.cc b/src/Model.cc index aa3a8659a..9aaee1867 100644 --- a/src/Model.cc +++ b/src/Model.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include "sdf/Error.hh" #include "sdf/Frame.hh" #include "sdf/Joint.hh" @@ -180,6 +181,7 @@ Errors Model::Load(ElementPtr _sdf) Errors errors; this->dataPtr->sdf = _sdf; + ignition::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. @@ -279,17 +281,28 @@ Errors Model::Load(ElementPtr _sdf) std::string jointName = joint.Name(); if (frameNames.count(jointName) > 0) { - jointName += "_joint"; - int i = 0; - while (frameNames.count(jointName) > 0) + // This joint has a name collision + if (sdfVersion < ignition::math::SemanticVersion(1, 7)) { - jointName = joint.Name() + "_joint" + std::to_string(i++); + // This came from an old file, so try to workaround by renaming joint + jointName += "_joint"; + int i = 0; + while (frameNames.count(jointName) > 0) + { + jointName = joint.Name() + "_joint" + std::to_string(i++); + } + sdfwarn << "Joint with name [" << joint.Name() << "] " + << "in model with name [" << this->Name() << "] " + << "has a name collision, changing joint name to [" + << jointName << "].\n"; + joint.SetName(jointName); + } + else + { + sdferr << "Joint with name [" << joint.Name() << "] " + << "in model with name [" << this->Name() << "] " + << "has a name collision. Please rename this joint.\n"; } - sdfwarn << "Joint with name [" << joint.Name() << "] " - << "in model with name [" << this->Name() << "] " - << "has a name collision, changing joint name to [" - << jointName << "].\n"; - joint.SetName(jointName); } frameNames.insert(jointName); } diff --git a/test/integration/joint_dom.cc b/test/integration/joint_dom.cc index 8cfd6f4d9..23be91825 100644 --- a/test/integration/joint_dom.cc +++ b/test/integration/joint_dom.cc @@ -25,7 +25,9 @@ #include "sdf/Link.hh" #include "sdf/Model.hh" #include "sdf/Root.hh" +#include "sdf/SDFImpl.hh" #include "sdf/Types.hh" +#include "sdf/parser.hh" #include "test_config.h" ////////////////////////////////////////////////// @@ -373,7 +375,38 @@ TEST(DOMJoint, LoadInvalidChild) } ///////////////////////////////////////////////// -TEST(DOMJoint, LoadLinkJointSameName) +TEST(DOMJoint, LoadLinkJointSameName17Invalid) +{ + const std::string testFile = + sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf", + "model_link_joint_same_name.sdf"); + + // Read with sdf::readFile, which converts from 1.6 to latest + sdf::SDFPtr sdf(new sdf::SDF()); + sdf::init(sdf); + sdf::readFile(testFile, sdf); + + // Load the SDF file from the converted string and expect errors + sdf::Root root; + auto errors = root.LoadSdfString(sdf->Root()->ToString("")); + for (auto e : errors) + std::cout << e << std::endl; + EXPECT_FALSE(errors.empty()); + EXPECT_EQ(2u, errors.size()); + EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::DUPLICATE_NAME); + EXPECT_NE(std::string::npos, + errors[0].Message().find( + "Joint with non-unique name [attachment] detected in model with name " + "[link_joint_same_name].")); + EXPECT_EQ(errors[1].Code(), sdf::ErrorCode::DUPLICATE_NAME); + EXPECT_NE(std::string::npos, + errors[1].Message().find( + "Joint with non-unique name [attachment] detected in model with name " + "[link_joint_same_name].")); +} + +///////////////////////////////////////////////// +TEST(DOMJoint, LoadLinkJointSameName16Valid) { const std::string testFile = sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf", From 68748d30672535831584827e758b50e3a1617ab3 Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Fri, 17 Jan 2020 13:19:52 -0800 Subject: [PATCH 2/4] also fail fast for sdf 1.7 with frame name collisions --- src/Model.cc | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/Model.cc b/src/Model.cc index 9aaee1867..58775600f 100644 --- a/src/Model.cc +++ b/src/Model.cc @@ -318,17 +318,28 @@ Errors Model::Load(ElementPtr _sdf) std::string frameName = frame.Name(); if (frameNames.count(frameName) > 0) { - frameName += "_frame"; - int i = 0; - while (frameNames.count(frameName) > 0) + // This joint has a name collision + if (sdfVersion < ignition::math::SemanticVersion(1, 7)) + { + // 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++); + } + sdfwarn << "Frame with name [" << frame.Name() << "] " + << "in model with name [" << this->Name() << "] " + << "has a name collision, changing frame name to [" + << frameName << "].\n"; + frame.SetName(frameName); + } + else { - frameName = frame.Name() + "_frame" + std::to_string(i++); + sdferr << "Frame with name [" << frame.Name() << "] " + << "in model with name [" << this->Name() << "] " + << "has a name collision. Please rename this frame.\n"; } - sdfwarn << "Frame with name [" << frame.Name() << "] " - << "in model with name [" << this->Name() << "] " - << "has a name collision, changing frame name to [" - << frameName << "].\n"; - frame.SetName(frameName); } frameNames.insert(frameName); } From c36601f5553f333e3a484843c9877a8ab46b1e91 Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Wed, 22 Jan 2020 17:11:01 -0800 Subject: [PATCH 3/4] changelog --- Changelog.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Changelog.md b/Changelog.md index 92a206b9b..882ca9441 100644 --- a/Changelog.md +++ b/Changelog.md @@ -34,6 +34,9 @@ 1. Access the original parsed version of an SDF document with `Element::OriginalVersion`. * [Pull request 640](https://bitbucket.org/osrf/sdformat/pull-requests/640) +1. Model::Load: fail fast if an sdf 1.7 file has name collisions. + * [Pull request 648](https://bitbucket.org/osrf/sdformat/pull-requests/648) + ### SDFormat 9.0.0 (2019-12-10) 1. Move recursiveSameTypeUniqueNames from ign.cc to parser.cc and make public. From 39b1ca897e48003d676629d4a9ec59f987ee3481 Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Thu, 23 Jan 2020 01:11:44 +0000 Subject: [PATCH 4/4] Close branch name_collisions_fail_17