Optimize load time and memory consumption by reusing element descriptions

Migration.md

@@ -18,6 +18,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**:

include/sdf/Element.hh

@@ -20,8 +20,10 @@
 #include <any>
 #include <map>
 #include <memory>
+#include <optional>
 #include <set>
 #include <string>
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -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<ElementConstPtr> 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<unsigned int> ElementDescriptionIndex(
+        const std::string &_key);
   };
 
   ///////////////////////////////////////////////
@@ -1038,7 +1080,7 @@ namespace sdf
       }
       else if (this->HasElementDescription(_key))
       {
-        result.first = this->GetElementDescription(_key)->Get<T>(_errors);
+        result.first = this->ElementDescription(_key)->Get<T>(_errors);
       }
       else
       {

python/src/sdf/pyElement.cc

@@ -21,6 +21,7 @@
 
 #include <memory>
 #include <string>
+#include <gz/utils/SuppressWarning.hh>
 
 #include "pyParam.hh"
 #include "pybind11_helpers.hh"
@@ -59,6 +60,10 @@ void defineElement(py::object module)
 {
   using PyClassElement = py::class_<Element, ElementPtr>;
 
+  // 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<unsigned int>(&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<unsigned int>(&Element::ElementDescription,
                                            py::const_),
            "Get an element description using an index")
-      .def("get_element_description",
+      .def("element_description",
+           py::overload_cast<const std::string &>(&Element::ElementDescription,
+                                                  py::const_),
+           "Get an element description using a key")
+      .def("mutable_element_description",
+           py::overload_cast<unsigned int>(&Element::MutableElementDescription),
+           "Get an element description using an index")
+      .def("mutable_element_description",
            py::overload_cast<const std::string &>(
-               &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.")

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()