diff --git a/Migration.md b/Migration.md index f16b810aa..dd4283fa8 100644 --- a/Migration.md +++ b/Migration.md @@ -22,6 +22,22 @@ but with improved human-readability.. package.xml package name. Use `find_package(sdformat)` instead of `find_package(sdformatX)` going forward. +### Additions + +- **sdf/Element.hh**: + + `sdf::ElementConstPtr ElementDescription(unsigned int) const` + + `sdf::ElementConstPtr ElementDescription(const std::string &) const` + + `sdf::ElementPtr MutableElementDescription(unsigned int)` + + `sdf::ElementPtr MutableElementDescription(const std::string &)` + +### Deprecations +- **sdf/Element.hh**: + + Mutable access to an element description via `Element::GetElementDescription` has been deprecated. Please use `Element::MutableElementDescription` instead. For immutable access, please use `Element::ElementDescription`. + + ***Deprecation:*** `sdf::ElementPtr GetElementDescription(unsigned int)` + + ***Replacement:*** `sdf::ElementConstPtr ElementDescription(unsigned int) const` + + ***Deprecation:*** `sdf::ElementPtr GetElementDescription(const std::string &)` + + ***Replacement:*** `sdf::ElementConstPtr ElementDescription(const std::string &) const` + ### Removals - **sdf/config.hh**: diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index cfd4adb3e..7867b77dd 100644 --- a/include/sdf/Element.hh +++ b/include/sdf/Element.hh @@ -20,8 +20,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -378,12 +380,40 @@ namespace sdf /// \brief Get an element description using an index /// \param[in] _index the index of the element description to get. /// \return An Element pointer to the found element. - public: ElementPtr GetElementDescription(unsigned int _index) const; + /// \deprecated See ElementDescription + public: + GZ_DEPRECATED(16) + ElementPtr GetElementDescription(unsigned int _index) const; /// \brief Get an element description using a key /// \param[in] _key the key to use to find the element. /// \return An Element pointer to the found element. - public: ElementPtr GetElementDescription(const std::string &_key) const; + /// \deprecated See ElementDescription + public: + GZ_DEPRECATED(16) + ElementPtr GetElementDescription(const std::string &_key) const; + + /// \brief Get an element description using an index + /// \param[in] _index the index of the element description to get. + /// \return An Element pointer to the found element. + /// \sa MutableElementDescription + public: ElementConstPtr ElementDescription(unsigned int _index) const; + + /// \brief Get an element description using a key + /// \param[in] _key the key to use to find the element. + /// \return An Element pointer to the found element. + /// \sa MutableElementDescription + public: ElementConstPtr ElementDescription(const std::string &_key) const; + + /// \brief Get a mutable element description using an index + /// \param[in] _index the index of the element description to get. + /// \return An Element pointer to the found element. + public: ElementPtr MutableElementDescription(unsigned int _index); + + /// \brief Get a mutable element description using a key + /// \param[in] _key the key to use to find the element. + /// \return An Element pointer to the found element. + public: ElementPtr MutableElementDescription(const std::string &_key); /// \brief Return true if an element description exists. /// \param[in] _name the name of the element to find. @@ -935,6 +965,10 @@ namespace sdf /// \brief XML path of this element. public: std::string xmlPath; + /// \brief Used to keep track of which element descriptions we have cloned + /// in order to provide a mutable pointer. + public: std::unordered_set clonedElementDescriptions; + /// \brief Generate the string (XML) for the attributes. /// \param[in] _includeDefaultAttributes flag to include default attributes. /// \param[in] _config Configuration for printing attributes. @@ -952,6 +986,14 @@ namespace sdf bool _includeDefaultAttributes, const PrintConfig &_config, std::ostringstream &_out) const; + + /// \brief Helper function to get the index of an element description + /// identified by a given key + /// \param[in] _key Key of the element description + /// \return An optional index. nullopt of the key was not found + public: + std::optional ElementDescriptionIndex( + const std::string &_key); }; /////////////////////////////////////////////// @@ -1038,7 +1080,7 @@ namespace sdf } else if (this->HasElementDescription(_key)) { - result.first = this->GetElementDescription(_key)->Get(_errors); + result.first = this->ElementDescription(_key)->Get(_errors); } else { diff --git a/python/src/sdf/pyElement.cc b/python/src/sdf/pyElement.cc index 935b95187..66c487e7b 100644 --- a/python/src/sdf/pyElement.cc +++ b/python/src/sdf/pyElement.cc @@ -21,6 +21,7 @@ #include #include +#include #include "pyParam.hh" #include "pybind11_helpers.hh" @@ -59,6 +60,10 @@ void defineElement(py::object module) { using PyClassElement = py::class_; + // TODO(azeey): GetElementDescription has been deprecated. Remove in sdformat17 + auto warnings = py::module::import("warnings"); + auto builtins = py::module::import("builtins"); + auto elemClass = PyClassElement(module, "Element"); elemClass.def(py::init<>()) .def("clone", ErrorWrappedCast<>(&Element::Clone, py::const_), @@ -143,13 +148,47 @@ void defineElement(py::object module) .def("get_element_description_count", &Element::GetElementDescriptionCount, "Get the number of element descriptions.") - .def("get_element_description", - py::overload_cast(&Element::GetElementDescription, + .def( + "get_element_description", + [warnings, builtins](const Element &_self, unsigned int _index) + { + warnings.attr("warn")( + "GetElementDescription is deprecated. Use ElementDescription " + "or MutableElementDescription instead", + builtins.attr("DeprecationWarning")); + GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + return _self.GetElementDescription(_index); + GZ_UTILS_WARN_RESUME__DEPRECATED_DECLARATION + }, + "Get an element description using an index") + .def( + "get_element_description", + [warnings, builtins](const Element &_self, const std::string &_key) + { + warnings.attr("warn")( + "GetElementDescription is deprecated. Use ElementDescription " + "or MutableElementDescription instead", + builtins.attr("DeprecationWarning")); + + GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + return _self.GetElementDescription(_key); + GZ_UTILS_WARN_RESUME__DEPRECATED_DECLARATION + }, + "Get an element description using a key") + .def("element_description", + py::overload_cast(&Element::ElementDescription, py::const_), "Get an element description using an index") - .def("get_element_description", + .def("element_description", + py::overload_cast(&Element::ElementDescription, + py::const_), + "Get an element description using a key") + .def("mutable_element_description", + py::overload_cast(&Element::MutableElementDescription), + "Get an element description using an index") + .def("mutable_element_description", py::overload_cast( - &Element::GetElementDescription, py::const_), + &Element::MutableElementDescription), "Get an element description using a key") .def("has_element_description", &Element::HasElementDescription, "Return true if an element description exists.") diff --git a/python/test/pyElement_TEST.py b/python/test/pyElement_TEST.py index 2af47d3a1..70420cf8b 100644 --- a/python/test/pyElement_TEST.py +++ b/python/test/pyElement_TEST.py @@ -15,6 +15,7 @@ from sdformat import Element, SDFErrorsException from gz.math import Vector3d import unittest +import warnings class ElementTEST(unittest.TestCase): @@ -475,6 +476,39 @@ def test_find_element(self): self.assertEqual("first_child", child_elem_b.get_attribute("name").get_as_string()) + def test_mutable_element_description(self): + elem = Element() + desc = Element() + desc.set_name("radius") + desc.add_value("double", "1", True, "radius") + elem.add_element_description(desc) + + elem_clone = elem.clone() + self.assertEqual(elem_clone.element_description("radius"), desc) + self.assertEqual(elem_clone.element_description(0), desc) + + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + self.assertEqual(elem_clone.get_element_description("radius"), desc) + self.assertEqual(elem_clone.get_element_description(0), desc) + + newDesc = elem_clone.mutable_element_description("radius") + self.assertNotEqual(newDesc, desc) + ## If we call mutable_element_description multiple times, it shouldn't create clone new Elements + self.assertEqual(newDesc, elem_clone.mutable_element_description("radius")) + self.assertEqual(newDesc, elem_clone.mutable_element_description(0)) + + # After mutable_element_description is called, calling the read-only + # element_description will return a different pointer than the original + # description + self.assertNotEqual(elem_clone.element_description("radius"), desc) + self.assertNotEqual(elem_clone.element_description(0), desc) + + + # Resetting the element clears the element descriptions. + elem_clone.reset() + self.assertIsNone(elem_clone.element_description("radius")) + self.assertIsNone(elem_clone.mutable_element_description("radius")) if __name__ == '__main__': unittest.main() diff --git a/src/Element.cc b/src/Element.cc index 5795cb44e..67c670814 100644 --- a/src/Element.cc +++ b/src/Element.cc @@ -17,6 +17,9 @@ #include #include +#include +#include +#include #include #include @@ -270,7 +273,7 @@ ElementPtr Element::Clone(sdf::Errors &_errors) const for (eiter = this->dataPtr->elementDescriptions.begin(); eiter != this->dataPtr->elementDescriptions.end(); ++eiter) { - clone->dataPtr->elementDescriptions.push_back((*eiter)->Clone(_errors)); + clone->dataPtr->elementDescriptions.push_back(*eiter); } clone->dataPtr->elements.reserve(this->dataPtr->elements.size()); @@ -348,11 +351,12 @@ void Element::Copy(const ElementPtr _elem, sdf::Errors &_errors) } this->dataPtr->elementDescriptions.clear(); + this->dataPtr->clonedElementDescriptions.clear(); for (ElementPtr_V::const_iterator iter = _elem->dataPtr->elementDescriptions.begin(); iter != _elem->dataPtr->elementDescriptions.end(); ++iter) { - this->dataPtr->elementDescriptions.push_back((*iter)->Clone(_errors)); + this->dataPtr->elementDescriptions.push_back(*iter); } this->dataPtr->elements.clear(); @@ -690,6 +694,21 @@ void ElementPrivate::PrintAttributes(sdf::Errors &_errors, } } +///////////////////////////////////////////////// +std::optional ElementPrivate::ElementDescriptionIndex( + const std::string &_key) +{ + for (auto iter = this->elementDescriptions.begin(); + iter != this->elementDescriptions.end(); ++iter) + { + if ((*iter)->GetName() == _key) + { + return std::distance(this->elementDescriptions.begin(), iter); + } + } + return std::nullopt; +} + ///////////////////////////////////////////////// void Element::PrintValues(std::string _prefix, const PrintConfig &_config) const @@ -896,6 +915,18 @@ size_t Element::GetElementDescriptionCount() const ///////////////////////////////////////////////// ElementPtr Element::GetElementDescription(unsigned int _index) const +{ + return std::const_pointer_cast(this->ElementDescription(_index)); +} + +///////////////////////////////////////////////// +ElementPtr Element::GetElementDescription(const std::string &_key) const +{ + return std::const_pointer_cast(this->ElementDescription(_key)); +} + +///////////////////////////////////////////////// +ElementConstPtr Element::ElementDescription(unsigned int _index) const { ElementPtr result; if (_index < this->dataPtr->elementDescriptions.size()) @@ -906,7 +937,7 @@ ElementPtr Element::GetElementDescription(unsigned int _index) const } ///////////////////////////////////////////////// -ElementPtr Element::GetElementDescription(const std::string &_key) const +ElementConstPtr Element::ElementDescription(const std::string &_key) const { ElementPtr_V::const_iterator iter; for (iter = this->dataPtr->elementDescriptions.begin(); @@ -921,6 +952,48 @@ ElementPtr Element::GetElementDescription(const std::string &_key) const return ElementPtr(); } +///////////////////////////////////////////////// +ElementPtr Element::MutableElementDescription(unsigned int _index) +{ + auto desc = static_cast(this)->ElementDescription(_index); + if (!desc) + { + return ElementPtr(); + } + + // To improve the performance of libsdformat, element descriptions are + // shared among elements of the same type. If the user requests a mutable + // element description, we make a clone here so that other elements are not + // affected by the potential modifications the user makes. We keep track of + // the clones we've made so that a clone is made at most once for each + // element description. + if (this->dataPtr->clonedElementDescriptions.count(desc) == 0) + { + auto clonedDesc = desc->Clone(); + this->dataPtr->clonedElementDescriptions.insert(clonedDesc); + this->dataPtr->elementDescriptions[_index] = clonedDesc; + return clonedDesc; + } + else + { + return std::const_pointer_cast(desc); + } +} + +///////////////////////////////////////////////// +ElementPtr Element::MutableElementDescription(const std::string &_key) +{ + auto index = this->dataPtr->ElementDescriptionIndex(_key); + if (index) + { + return this->MutableElementDescription(*index); + } + else + { + return sdf::ElementPtr(); + } +} + ///////////////////////////////////////////////// ParamPtr Element::GetValue() const { @@ -1179,7 +1252,7 @@ void Element::InsertElement(ElementPtr _elem, bool _setParentToSelf) ///////////////////////////////////////////////// bool Element::HasElementDescription(const std::string &_name) const { - return this->GetElementDescription(_name) != ElementPtr(); + return this->ElementDescription(_name) != ElementConstPtr(); } ///////////////////////////////////////////////// @@ -1204,7 +1277,7 @@ ElementPtr Element::AddElement(const std::string &_name, sdf::Errors &_errors) for (unsigned int i = 0; i < parent->GetElementDescriptionCount(); ++i) { this->dataPtr->elementDescriptions.push_back( - parent->GetElementDescription(i)->Clone(_errors)); + parent->dataPtr->elementDescriptions[i]); } } @@ -1313,6 +1386,7 @@ void Element::Reset() } this->dataPtr->elements.clear(); this->dataPtr->elementDescriptions.clear(); + this->dataPtr->clonedElementDescriptions.clear(); this->dataPtr->value.reset(); @@ -1480,10 +1554,10 @@ std::any Element::GetAny(sdf::Errors &_errors, const std::string &_key) const } else { - tmp = this->GetElementDescription(_key); - if (tmp != ElementPtr()) + auto desc = this->ElementDescription(_key); + if (desc != ElementConstPtr()) { - result = tmp->GetAny(_errors); + result = desc->GetAny(_errors); } else { diff --git a/src/Element_TEST.cc b/src/Element_TEST.cc index 2d96e1bbc..394c5d2b3 100644 --- a/src/Element_TEST.cc +++ b/src/Element_TEST.cc @@ -17,6 +17,8 @@ #include +#include + #include "sdf/Element.hh" #include "sdf/Filesystem.hh" #include "sdf/Param.hh" @@ -350,17 +352,17 @@ TEST(Element, AddElementReferenceSDF) ASSERT_EQ(child->GetElementDescriptionCount(), 1UL); - sdf::ElementPtr check = child->GetElementDescription(4); + sdf::ElementConstPtr check = child->ElementDescription(4); ASSERT_EQ(check, sdf::ElementPtr()); - check = child->GetElementDescription(0); + check = child->ElementDescription(0); ASSERT_NE(check, sdf::ElementPtr()); ASSERT_EQ(check->GetName(), "parent"); - check = child->GetElementDescription("bad"); + check = child->ElementDescription("bad"); ASSERT_EQ(check, sdf::ElementPtr()); - check = child->GetElementDescription("parent"); + check = child->ElementDescription("parent"); ASSERT_NE(check, sdf::ElementPtr()); ASSERT_EQ(check->GetName(), "parent"); @@ -1090,3 +1092,40 @@ TEST(Element, FindElement) EXPECT_EQ("first_child", childElemB->GetAttribute("name")->GetAsString()); } } + +TEST(Element, MutableElementDescription) +{ + sdf::ElementPtr elem = std::make_shared(); + sdf::ElementPtr desc = std::make_shared(); + desc->SetName("radius"); + desc->AddValue("double", "1", true, "radius"); + elem->AddElementDescription(desc); + + auto elemClone = elem->Clone(); + + EXPECT_EQ(elemClone->ElementDescription("radius"), desc); + EXPECT_EQ(elemClone->ElementDescription(0), desc); + + GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + EXPECT_EQ(elemClone->GetElementDescription("radius"), desc); + EXPECT_EQ(elemClone->GetElementDescription(0), desc); + GZ_UTILS_WARN_RESUME__DEPRECATED_DECLARATION + + auto newDesc = elemClone->MutableElementDescription("radius"); + EXPECT_NE(newDesc, desc); + // If we call MutableElementDescription multiple times, it shouldn't create + // clone new Elements + EXPECT_EQ(newDesc, elemClone->MutableElementDescription("radius")); + EXPECT_EQ(newDesc, elemClone->MutableElementDescription(0)); + + // After MutableElementDescription is called, calling the read-only + // ElementDescription will return a different pointer than the original + // description + EXPECT_NE(elemClone->ElementDescription("radius"), desc); + EXPECT_NE(elemClone->ElementDescription(0), desc); + + // Resetting the element clears the element descriptions. + elemClone->Reset(); + EXPECT_EQ(nullptr, elemClone->ElementDescription("radius")); + EXPECT_EQ(nullptr, elemClone->MutableElementDescription("radius")); +} diff --git a/src/ParamPassing.cc b/src/ParamPassing.cc index f30eeb7ea..2d836d729 100644 --- a/src/ParamPassing.cc +++ b/src/ParamPassing.cc @@ -461,7 +461,7 @@ void handleIndividualChildActions(const ParserConfig &_config, continue; } - ElementPtr elemChild = elemDesc->GetElementDescription(elemName); + ElementPtr elemChild = elemDesc->MutableElementDescription(elemName); if (!xmlToSdf(_config, _source, xmlChild, elemChild, _errors)) { diff --git a/src/parser.cc b/src/parser.cc index 992893987..d33c0de11 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -1615,7 +1615,7 @@ static void validateIncludeElement(tinyxml2::XMLElement *_xml, for (unsigned int descCounter = 0; descCounter != _sdf->GetElementDescriptionCount(); ++descCounter) { - ElementPtr elemDesc = _sdf->GetElementDescription(descCounter); + ElementConstPtr elemDesc = _sdf->ElementDescription(descCounter); if (elemDesc->GetName() == _xml->Value()) { ElementPtr element = elemDesc->Clone(); @@ -1957,7 +1957,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, } auto includeSDFFirstElem = includeSDF->Root()->GetFirstElement(); - auto includeDesc = _sdf->GetElementDescription("include"); + auto includeDesc = _sdf->ElementDescription("include"); if (includeDesc) { // Store the contents of the tag as the includeElement of @@ -1981,7 +1981,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, for (descCounter = 0; descCounter != _sdf->GetElementDescriptionCount(); ++descCounter) { - ElementPtr elemDesc = _sdf->GetElementDescription(descCounter); + ElementConstPtr elemDesc = _sdf->ElementDescription(descCounter); if (elemDesc->GetName() == elemXml->Value()) { std::string elemXmlPath = _sdf->XmlPath() + "/" + elemXml->Value(); @@ -2047,7 +2047,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, for (unsigned int descCounter = 0; descCounter != _sdf->GetElementDescriptionCount(); ++descCounter) { - ElementPtr elemDesc = _sdf->GetElementDescription(descCounter); + ElementConstPtr elemDesc = _sdf->ElementDescription(descCounter); if (elemDesc->GetRequired() == "1" || elemDesc->GetRequired() == "+") {