From 54d9018599ce6605cee13d372c7dc51f0c0fcd2d Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Wed, 10 Mar 2021 11:20:49 -0600 Subject: [PATCH 1/4] Store include info in Element Signed-off-by: Addisu Z. Taddese --- include/sdf/Element.hh | 38 ++++++++++++++- src/Element.cc | 29 +++++++++++ src/Element_TEST.cc | 31 ++++++++++++ src/parser.cc | 41 +++++++++++----- test/integration/nested_model.cc | 84 ++++++++++++++++++++++++++++++++ 5 files changed, 210 insertions(+), 13 deletions(-) diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index 214e3d2ae..6dd2a3bbc 100644 --- a/include/sdf/Element.hh +++ b/include/sdf/Element.hh @@ -378,11 +378,23 @@ namespace sdf /// \brief Set the include filename to the passed in filename. /// \param[in] _filename the filename to set the include filename to. - public: void SetInclude(const std::string &_filename); + public: void SetInclude(const std::string &_filename) SDF_DEPRECATED(11.0); /// \brief Get the include filename. /// \return The include filename. - public: std::string GetInclude() const; + public: std::string GetInclude() const SDF_DEPRECATED(11.0); + + /// \brief Set the element that was used to load this element. + /// This set by the parser on the first element of the included object (eg. + /// Model, Actor, Light, etc). It is not passed down to children elements. + /// \param[in] _includeELem The Element + public: void SetIncludeElement(sdf::ElementPtr _includeElem); + + /// \brief Get the element that was used to load this element. + /// \return The Element pointer to the element, if this element + /// was loaded from an included file. Otherwise, nullptr. This is also + /// nullptr for Elements that cannot be included. + public: sdf::ElementPtr GetIncludeElement() const; /// \brief Set the path to the SDF document where this element came from. /// \param[in] _path Full path to SDF document. @@ -479,6 +491,28 @@ namespace sdf // The possible child elements public: ElementPtr_V elementDescriptions; + /// \brief The element that was used to load this entity. For + /// example, given the following SDFormat: + /// + /// + /// + /// model_uri + /// 1 2 3 0 0 0 + /// + /// + /// + /// The ElementPtr associated with the model loaded from 'model_uri' will + /// have the includeElement set to + /// + /// model_uri + /// 1 2 3 0 0 0 + /// + /// + /// This can be used to retrieve additional information available under the + /// tag after the entity has been loaded. An example use case for + /// this is when saving a loaded world back to SDFormat. + public: ElementPtr includeElement; + /// name of the include file that was used to create this element public: std::string includeFilename; diff --git a/src/Element.cc b/src/Element.cc index da4156541..d7a35e4a5 100644 --- a/src/Element.cc +++ b/src/Element.cc @@ -195,6 +195,11 @@ ElementPtr Element::Clone() const clone->dataPtr->value = this->dataPtr->value->Clone(); } + if (this->dataPtr->includeElement) + { + clone->dataPtr->includeElement = this->dataPtr->includeElement->Clone(); + } + return clone; } @@ -250,6 +255,18 @@ void Element::Copy(const ElementPtr _elem) elem->SetParent(shared_from_this()); this->dataPtr->elements.push_back(elem); } + + if (_elem->dataPtr->includeElement) + { + if (!this->dataPtr->includeElement) + { + this->dataPtr->includeElement = _elem->dataPtr->includeElement->Clone(); + } + else + { + this->dataPtr->includeElement->Copy(_elem->dataPtr->includeElement); + } + } } ///////////////////////////////////////////////// @@ -912,6 +929,18 @@ std::string Element::GetInclude() const return this->dataPtr->includeFilename; } +///////////////////////////////////////////////// +void Element::SetIncludeElement(sdf::ElementPtr _includeElem) +{ + this->dataPtr->includeElement = _includeElem; +} + +///////////////////////////////////////////////// +sdf::ElementPtr Element::GetIncludeElement() const +{ + return this->dataPtr->includeElement; +} + ///////////////////////////////////////////////// void Element::SetFilePath(const std::string &_path) { diff --git a/src/Element_TEST.cc b/src/Element_TEST.cc index 31fcf8f7d..bcf150fa0 100644 --- a/src/Element_TEST.cc +++ b/src/Element_TEST.cc @@ -158,11 +158,28 @@ TEST(Element, Include) { sdf::Element elem; + SDF_SUPPRESS_DEPRECATED_BEGIN ASSERT_EQ(elem.GetInclude(), ""); elem.SetInclude("foo.txt"); ASSERT_EQ(elem.GetInclude(), "foo.txt"); + SDF_SUPPRESS_DEPRECATED_END + + auto includeElemToStore = std::make_shared(); + includeElemToStore->SetName("include"); + auto uriDesc = std::make_shared(); + uriDesc->SetName("uri"); + uriDesc->AddValue("string", "", true); + includeElemToStore->AddElementDescription(uriDesc); + + includeElemToStore->GetElement("uri")->Set("foo.txt"); + elem.SetIncludeElement(includeElemToStore); + + auto includeElem = elem.GetIncludeElement(); + ASSERT_NE(nullptr, includeElem); + ASSERT_TRUE(includeElem->HasElement("uri")); + EXPECT_EQ("foo.txt", includeElem->GetElement("uri")->Get()); } ///////////////////////////////////////////////// @@ -276,6 +293,10 @@ TEST(Element, Clone) parent->SetFilePath("/path/to/file.sdf"); parent->SetOriginalVersion("1.5"); + auto includeElemToStore = std::make_shared(); + includeElemToStore->SetName("include"); + parent->SetIncludeElement(includeElemToStore); + sdf::ElementPtr newelem = parent->Clone(); EXPECT_EQ("/path/to/file.sdf", newelem->FilePath()); @@ -283,6 +304,8 @@ TEST(Element, Clone) ASSERT_NE(newelem->GetFirstElement(), nullptr); ASSERT_EQ(newelem->GetElementDescriptionCount(), 1UL); ASSERT_EQ(newelem->GetAttributeCount(), 1UL); + ASSERT_NE(newelem->GetIncludeElement(), nullptr); + EXPECT_EQ("include", newelem->GetIncludeElement()->GetName()); } ///////////////////////////////////////////////// @@ -438,11 +461,13 @@ TEST(Element, ToStringInclude) { sdf::Element elem; + SDF_SUPPRESS_DEPRECATED_BEGIN ASSERT_EQ(elem.GetInclude(), ""); elem.SetInclude("foo.txt"); ASSERT_EQ(elem.GetInclude(), "foo.txt"); + SDF_SUPPRESS_DEPRECATED_END std::string stringval = elem.ToString("myprefix"); ASSERT_EQ(stringval, "myprefix\n"); @@ -547,6 +572,10 @@ TEST(Element, Copy) src->AddAttribute("test", "string", "foo", false, "foo description"); src->InsertElement(std::make_shared()); + auto includeElemToStore = std::make_shared(); + includeElemToStore->SetName("include"); + src->SetIncludeElement(includeElemToStore); + dest->Copy(src); EXPECT_EQ("/path/to/file.sdf", dest->FilePath()); @@ -568,6 +597,8 @@ TEST(Element, Copy) ASSERT_EQ(param->GetDescription(), "foo description"); ASSERT_NE(dest->GetFirstElement(), nullptr); + ASSERT_NE(dest->GetIncludeElement(), nullptr); + EXPECT_EQ("include", dest->GetIncludeElement()->GetName()); } ///////////////////////////////////////////////// diff --git a/src/parser.cc b/src/parser.cc index 4db688d72..8968586b3 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -47,6 +47,20 @@ namespace sdf { inline namespace SDF_VERSION_NAMESPACE { + +sdf::ElementPtr getIncludeDescription(sdf::ElementPtr _root) +{ + for (uint64_t i = 0; i < _root->GetElementDescriptionCount(); ++i) + { + auto desc = _root->GetElementDescription(i); + if ("include" == desc->GetName()) + { + return desc; + } + } + return nullptr; +} + ////////////////////////////////////////////////// /// \brief Internal helper for readFile, which populates the SDF values /// from a file @@ -1038,10 +1052,11 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, if (std::string("include") == elemXml->Value()) { std::string modelPath; + std::string uri; if (elemXml->FirstChildElement("uri")) { - std::string uri = elemXml->FirstChildElement("uri")->GetText(); + uri = elemXml->FirstChildElement("uri")->GetText(); modelPath = sdf::findFile(uri, true, true, _config); // Test the model path @@ -1236,14 +1251,18 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, } } - includeSDF->Root()->GetFirstElement()->SetParent(_sdf); - _sdf->InsertElement(includeSDF->Root()->GetFirstElement()); - // TODO: This was used to store the included filename so that when - // a world is saved, the included model's SDF is not stored in the - // world file. This highlights the need to make model inclusion - // a core feature of SDF, and not a hack that that parser handles - // includeSDF->Root()->GetFirstElement()->SetInclude( - // elemXml->Attribute("filename")); + auto includeSDFFirstElem = includeSDF->Root()->GetFirstElement(); + includeSDFFirstElem->SetParent(_sdf); + auto includeDesc = _sdf->GetElementDescription("include"); + if (includeDesc) + { + // Store the contents of the tag as the includeElement of + // the entity that was loaded from the included URI. + auto includeInfo = includeDesc->Clone(); + copyChildren(includeInfo, elemXml, false); + includeSDFFirstElem->SetIncludeElement(includeInfo); + } + _sdf->InsertElement(includeSDFFirstElem); continue; } @@ -1351,8 +1370,8 @@ void copyChildren(ElementPtr _sdf, } // copy value - std::string value = elemXml->GetText(); - if (!value.empty()) + const char *value = elemXml->GetText(); + if (value) { element->GetValue()->SetFromString(value); } diff --git a/test/integration/nested_model.cc b/test/integration/nested_model.cc index 50cf7526c..d1ae47e09 100644 --- a/test/integration/nested_model.cc +++ b/test/integration/nested_model.cc @@ -1371,3 +1371,87 @@ TEST(NestedReference, PlacementFrameElement) EXPECT_EQ(placementPose, pose.Inverse()); } } + +///////////////////////////////////////////////// +TEST(NestedModel, IncludeElements) +{ + const std::string modelRootPath = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "model"); + + sdf::setFindCallback( + [&](const std::string &_file) + { + return sdf::filesystem::append(modelRootPath, _file); + }); + + auto checkIncludeElem = [](sdf::ElementPtr _includeElem, + const std::string &_uri, const std::string &_name) + { + ASSERT_NE(nullptr, _includeElem); + ASSERT_TRUE(_includeElem->HasElement("uri")); + EXPECT_EQ(_uri, _includeElem->Get("uri")); + + ASSERT_TRUE(_includeElem->HasElement("name")); + EXPECT_EQ(_name, _includeElem->Get("name")); + + ASSERT_TRUE(_includeElem->HasElement("pose")); + EXPECT_EQ(ignition::math::Pose3d(1, 2, 3, 0, 0, 0), + _includeElem->Get("pose")); + + ASSERT_TRUE(_includeElem->HasElement("placement_frame")); + EXPECT_EQ("link", _includeElem->Get("placement_frame")); + + ASSERT_TRUE(_includeElem->HasElement("plugin")); + auto pluginElem = _includeElem->GetElement("plugin"); + ASSERT_TRUE(pluginElem->HasAttribute("name")); + EXPECT_EQ("test_plugin", pluginElem->Get("name")); + ASSERT_TRUE(pluginElem->HasAttribute("filename")); + EXPECT_EQ("test_plugin_file", pluginElem->Get("filename")); + }; + + // Test with //world/include + { + std::ostringstream stream; + stream << R"( + + + + box + test_box + 1 2 3 0 0 0 + link + + + + + test_model + test_model + 1 2 3 0 0 0 + link + + + + + )"; + sdf::Root root; + sdf::Errors errors = root.LoadSdfString(stream.str()); + EXPECT_TRUE(errors.empty()) << errors; + + auto *world = root.WorldByIndex(0); + ASSERT_NE(nullptr, world); + auto *box = world->ModelByIndex(0); + ASSERT_NE(nullptr, box); + + checkIncludeElem( + box->Element()->GetIncludeElement(), "box", "test_box"); + + auto *test2 = world->ModelByIndex(1); + ASSERT_NE(nullptr, test2); + EXPECT_EQ(nullptr, test2->Element()->GetIncludeElement()); + + auto *testModel2 = test2->ModelByIndex(0); + ASSERT_NE(nullptr, testModel2); + checkIncludeElem( + testModel2->Element()->GetIncludeElement(), "test_model", "test_model"); + } +} From 69dc1542641df2586d2e8dcfb02e307a8a865fdb Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Thu, 11 Mar 2021 10:40:15 -0600 Subject: [PATCH 2/4] Remove unused function Signed-off-by: Addisu Z. Taddese --- src/parser.cc | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/parser.cc b/src/parser.cc index 8968586b3..0397444d3 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -47,20 +47,6 @@ namespace sdf { inline namespace SDF_VERSION_NAMESPACE { - -sdf::ElementPtr getIncludeDescription(sdf::ElementPtr _root) -{ - for (uint64_t i = 0; i < _root->GetElementDescriptionCount(); ++i) - { - auto desc = _root->GetElementDescription(i); - if ("include" == desc->GetName()) - { - return desc; - } - } - return nullptr; -} - ////////////////////////////////////////////////// /// \brief Internal helper for readFile, which populates the SDF values /// from a file From 2631da14dff94ea7c66e2480be626ffd3e33e72e Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Mon, 22 Mar 2021 15:56:22 -0500 Subject: [PATCH 3/4] Address reviewer feedback Signed-off-by: Addisu Z. Taddese --- include/sdf/Element.hh | 4 ++-- test/integration/nested_model.cc | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index 6dd2a3bbc..f42a95a32 100644 --- a/include/sdf/Element.hh +++ b/include/sdf/Element.hh @@ -493,7 +493,7 @@ namespace sdf /// \brief The element that was used to load this entity. For /// example, given the following SDFormat: - /// + /// /// /// /// model_uri @@ -501,7 +501,7 @@ namespace sdf /// /// /// - /// The ElementPtr associated with the model loaded from 'model_uri' will + /// The ElementPtr associated with the model loaded from `model_uri` will /// have the includeElement set to /// /// model_uri diff --git a/test/integration/nested_model.cc b/test/integration/nested_model.cc index 0609e9cb6..1cbd6c4a7 100644 --- a/test/integration/nested_model.cc +++ b/test/integration/nested_model.cc @@ -1403,7 +1403,8 @@ TEST(NestedModel, IncludeElements) const std::string modelRootPath = sdf::filesystem::append( PROJECT_SOURCE_PATH, "test", "integration", "model"); - sdf::setFindCallback( + sdf::ParserConfig config; + config.SetFindCallback( [&](const std::string &_file) { return sdf::filesystem::append(modelRootPath, _file); @@ -1445,21 +1446,21 @@ TEST(NestedModel, IncludeElements) test_box 1 2 3 0 0 0 link - + - + test_model test_model 1 2 3 0 0 0 link - + )"; sdf::Root root; - sdf::Errors errors = root.LoadSdfString(stream.str()); + sdf::Errors errors = root.LoadSdfString(stream.str(), config); EXPECT_TRUE(errors.empty()) << errors; auto *world = root.WorldByIndex(0); From 51d0c0f175171d33faf032f8eac37532697efd50 Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Mon, 22 Mar 2021 21:37:21 -0500 Subject: [PATCH 4/4] Fix typo Signed-off-by: Addisu Z. Taddese --- include/sdf/Element.hh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index f42a95a32..73d73e428 100644 --- a/include/sdf/Element.hh +++ b/include/sdf/Element.hh @@ -385,8 +385,9 @@ namespace sdf public: std::string GetInclude() const SDF_DEPRECATED(11.0); /// \brief Set the element that was used to load this element. - /// This set by the parser on the first element of the included object (eg. - /// Model, Actor, Light, etc). It is not passed down to children elements. + /// This is set by the parser on the first element of the included object + /// (eg. Model, Actor, Light, etc). It is not passed down to children + /// elements. /// \param[in] _includeELem The Element public: void SetIncludeElement(sdf::ElementPtr _includeElem);