--[BUGFIX] Make sure empty Markerset subconfigs are removed when appropriate (#2434)

* --make sure empty subconfigs are removed to minimize clutter in saved json.
* --cleanup configuration; handle get attempt from non-existing subconfig
     Instead of asserting, return a warning along with an empty vector <T> if the named subconfig is not found. This is consistent with the behavior of other indirect subconfig queries.
* --handle if the 'markers' subconfig does not exist in a MarkerSet
    It is possible to have a JSON marker set object associated with a link and task that lists no points. Due to the markersets being managed generically as nested configurations, with access accomplished via static casting, it is then possible to access a MarkerSet object that may not have a "markers" subconfig. This is a reasonable state, and should result in an assertion when attempting to query the MarkerSet.

Author: jturner65

src/esp/core/Configuration.h

@@ -1220,21 +1220,21 @@ class Configuration {
   /**
    * @brief Templated subconfig copy getter. Retrieves a shared pointer to a
    * copy of the subConfig @ref esp::core::config::Configuration that has the
-   * passed @p name .
+   * passed @p cfgName .
    *
    * @tparam Type to return. Must inherit from @ref
    * esp::core::config::Configuration
-   * @param name The name of the Configuration to retrieve.
+   * @param cfgName The name of the Configuration to retrieve.
    * @return A pointer to a copy of the Configuration having the requested
    * name, cast to the appropriate type, or nullptr if not found.
    */
 
   template <typename T>
-  std::shared_ptr<T> getSubconfigCopy(const std::string& name) const {
+  std::shared_ptr<T> getSubconfigCopy(const std::string& cfgName) const {
     static_assert(std::is_base_of<Configuration, T>::value,
                   "Configuration : Desired subconfig must be derived from "
                   "core::config::Configuration");
-    auto configIter = configMap_.find(name);
+    auto configIter = configMap_.find(cfgName);
     if (configIter != configMap_.end()) {
       // if exists return copy, so that consumers can modify it freely
       return std::make_shared<T>(
@@ -1244,91 +1244,92 @@ class Configuration {
   }
 
   /**
-   * @brief return pointer to read-only sub-Configuration of given @p name.
+   * @brief return pointer to read-only sub-Configuration of given @p cfgName.
    * Will fail if Configuration with given name dne.
-   * @param name The name of the desired Configuration.
+   * @param cfgName The name of the desired Configuration.
    */
   std::shared_ptr<const Configuration> getSubconfigView(
-      const std::string& name) const {
-    auto configIter = configMap_.find(name);
-    CORRADE_ASSERT(
-        configIter != configMap_.end(),
-        "SubConfiguration with name " << name << " not found in Configuration.",
-        nullptr);
+      const std::string& cfgName) const {
+    auto configIter = configMap_.find(cfgName);
+    CORRADE_ASSERT(configIter != configMap_.end(),
+                   "SubConfiguration with name "
+                       << cfgName << " not found in Configuration.",
+                   nullptr);
     // if exists return actual object
     return configIter->second;
   }
 
   /**
    * @brief Templated Version. Retrieves the stored shared pointer to the
-   * subConfig @ref esp::core::config::Configuration that has the passed @p name
+   * subConfig @ref esp::core::config::Configuration that has the passed @p cfgName
    * , cast to the specified type. This will create a shared pointer to a new
    * sub-Configuration if none exists and return it, cast to specified type.
    *
    * Use this function when you wish to modify this Configuration's
    * subgroup, possibly creating it in the process.
    * @tparam The type to cast the @ref esp::core::config::Configuration to. Type
    * is checked to verify that it inherits from Configuration.
-   * @param name The name of the Configuration to edit.
+   * @param cfgName The name of the Configuration to edit.
    * @return The actual pointer to the Configuration having the requested
    * name, cast to the specified type.
    */
   template <typename T>
-  std::shared_ptr<T> editSubconfig(const std::string& name) {
+  std::shared_ptr<T> editSubconfig(const std::string& cfgName) {
     static_assert(std::is_base_of<Configuration, T>::value,
                   "Configuration : Desired subconfig must be derived from "
                   "core::config::Configuration");
     // retrieve existing (or create new) subgroup, with passed name
     return std::static_pointer_cast<T>(
-        addOrEditSubgroup<T>(name).first->second);
+        addOrEditSubgroup<T>(cfgName).first->second);
   }
 
   /**
    * @brief move specified subgroup config into configMap at desired name.
    * Will replace any subConfiguration at given name without warning if
    * present.
-   * @param name The name of the subConfiguration to add
+   * @param cfgName The name of the subConfiguration to add
    * @param configPtr A pointer to a subConfiguration to add.
    */
   template <typename T>
-  void setSubconfigPtr(const std::string& name, std::shared_ptr<T>& configPtr) {
+  void setSubconfigPtr(const std::string& cfgName,
+                       std::shared_ptr<T>& configPtr) {
     static_assert(std::is_base_of<Configuration, T>::value,
                   "Configuration : Desired subconfig must be derived from "
                   "core::config::Configuration");
 
-    configMap_[name] = std::move(configPtr);
+    configMap_[cfgName] = std::move(configPtr);
   }  // setSubconfigPtr
 
   /**
    * @brief Removes and returns the named subconfig. If not found, returns an
    * empty subconfig with a warning.
-   * @param name The name of the subConfiguration to delete
+   * @param cfgName The name of the subConfiguration to delete
    * @return a shared pointer to the removed subConfiguration.
    */
-  std::shared_ptr<Configuration> removeSubconfig(const std::string& name) {
-    ConfigMapType::const_iterator mapIter = configMap_.find(name);
+  std::shared_ptr<Configuration> removeSubconfig(const std::string& cfgName) {
+    ConfigMapType::const_iterator mapIter = configMap_.find(cfgName);
     if (mapIter != configMap_.end()) {
       configMap_.erase(mapIter);
       return mapIter->second;
     }
-    ESP_WARNING() << "Name :" << name
+    ESP_WARNING() << "Name :" << cfgName
                   << "not present in map of subConfigurations.";
     return {};
   }
 
   /**
    * @brief Retrieve the number of entries held by the subconfig with the
    * given name
-   * @param name The name of the subconfig to query. If not found, returns 0
+   * @param cfgName The name of the subconfig to query. If not found, returns 0
    * with a warning.
    * @return The number of entries in the named subconfig
    */
-  int getSubconfigNumEntries(const std::string& name) const {
-    auto configIter = configMap_.find(name);
+  int getSubconfigNumEntries(const std::string& cfgName) const {
+    auto configIter = configMap_.find(cfgName);
     if (configIter != configMap_.end()) {
       return configIter->second->getNumEntries();
     }
-    ESP_WARNING() << "No Subconfig found named :" << name;
+    ESP_WARNING() << "No Subconfig found named :" << cfgName;
     return 0;
   }
 
@@ -1403,7 +1404,12 @@ class Configuration {
   std::vector<T> getSubconfigValsOfTypeInVector(
       const std::string& subCfgName) const {
     const ConfigValType desiredType = configValTypeFor<T>();
-    const auto subCfg = getSubconfigView(subCfgName);
+    auto configIter = configMap_.find(subCfgName);
+    if (configIter == configMap_.end()) {
+      ESP_WARNING() << "No Subconfig found named :" << subCfgName;
+      return {};
+    }
+    const auto subCfg = configIter->second;
     const auto& subCfgTags = subCfg->getKeysByType(desiredType, true);
     std::vector<T> res;
     res.reserve(subCfgTags.size());

src/esp/metadata/attributes/MarkerSets.h

@@ -26,14 +26,24 @@ class MarkerSet : public esp::core::config::Configuration {
   /**
    * @brief Returns the number of existing marker points in this MarkerSet.
    */
-  int getNumPoints() const {
+  int getNumPoints() {
+    // the 'markers' subconfig may not exist due to how the MarkerSet hierarchy
+    // is loaded from JSON.
+    if (!hasSubconfig("markers")) {
+      return 0;
+    }
     return getSubconfigView("markers")->getNumValues();
   }
 
   /**
    * @brief Returns a list of all the marker points in this MarkerSet
    */
   std::vector<Mn::Vector3> getAllPoints() const {
+    // the 'markers' subconfig may not exist due to how the MarkerSet hierarchy
+    // is loaded from JSON.
+    if (!hasSubconfig("markers")) {
+      return {};
+    }
     return getSubconfigValsOfTypeInVector<Mn::Vector3>("markers");
   }
 
@@ -51,6 +61,7 @@ class MarkerSet : public esp::core::config::Configuration {
    */
   int rekeyAllMarkers() { return rekeySubconfigValues("markers"); }
 
+ public:
   ESP_SMART_POINTERS(MarkerSet)
 };  // class MarkerSet
 
@@ -134,7 +145,11 @@ class LinkSet : public esp::core::config::Configuration {
    */
   void setMarkerSetPoints(const std::string& markerSetName,
                           const std::vector<Mn::Vector3>& markerList) {
-    editMarkerSet(markerSetName)->setAllPoints(markerList);
+    if (markerList.size() == 0) {
+      removeMarkerSet(markerSetName);
+    } else {
+      editMarkerSet(markerSetName)->setAllPoints(markerList);
+    }
   }
 
   /**
@@ -310,7 +325,12 @@ class TaskSet : public esp::core::config::Configuration {
   void setLinkMarkerSetPoints(const std::string& linkSetName,
                               const std::string& markerSetName,
                               const std::vector<Mn::Vector3>& markerList) {
-    editLinkSet(linkSetName)->setMarkerSetPoints(markerSetName, markerList);
+    auto linkSet = editLinkSet(linkSetName);
+    linkSet->setMarkerSetPoints(markerSetName, markerList);
+    // The above may result in the link set being empty. If so remove it.
+    if (linkSet->getNumMarkerSets() == 0) {
+      removeLinkSet(linkSetName);
+    }
   }
 
   /**
@@ -325,7 +345,12 @@ class TaskSet : public esp::core::config::Configuration {
       const std::string& linkSetName,
       const std::unordered_map<std::string, std::vector<Mn::Vector3>>&
           markers) {
-    editLinkSet(linkSetName)->setAllMarkerPoints(markers);
+    auto linkSet = editLinkSet(linkSetName);
+    linkSet->setAllMarkerPoints(markers);
+    // The above may result in the link set being empty. If so remove it.
+    if (linkSet->getNumMarkerSets() == 0) {
+      removeLinkSet(linkSetName);
+    }
   }
 
   /**
@@ -542,7 +567,6 @@ class MarkerSets : public esp::core::config::Configuration {
    * @param linkSetName the name of the LinkSet within @p taskSetName
    * @param markerSetName the name of the MarkerSet within @p linkSetName
    */
-
   void initTaskLinkMarkerSet(const std::string& taskSetName,
                              const std::string& linkSetName,
                              const std::string& markerSetName) {
@@ -561,8 +585,12 @@ class MarkerSets : public esp::core::config::Configuration {
                                   const std::string& linkSetName,
                                   const std::string& markerSetName,
                                   const std::vector<Mn::Vector3>& markerList) {
-    editTaskSet(taskSetName)
-        ->setLinkMarkerSetPoints(linkSetName, markerSetName, markerList);
+    auto taskSetPtr = editTaskSet(taskSetName);
+    taskSetPtr->setLinkMarkerSetPoints(linkSetName, markerSetName, markerList);
+    // After this process, the taskset might be empty, if so, delete it.
+    if (taskSetPtr->getNumLinkSets() == 0) {
+      removeTaskSet(taskSetName);
+    }
   }
 
   /**
@@ -579,7 +607,12 @@ class MarkerSets : public esp::core::config::Configuration {
       const std::string& linkSetName,
       const std::unordered_map<std::string, std::vector<Mn::Vector3>>&
           markerMap) {
-    editTaskSet(taskSetName)->setLinkSetPoints(linkSetName, markerMap);
+    auto taskSetPtr = editTaskSet(taskSetName);
+    taskSetPtr->setLinkSetPoints(linkSetName, markerMap);
+    // After this process, the taskset might be empty, if so, delete it.
+    if (taskSetPtr->getNumLinkSets() == 0) {
+      removeTaskSet(taskSetName);
+    }
   }
 
   /**
@@ -595,7 +628,14 @@ class MarkerSets : public esp::core::config::Configuration {
           std::string,
           std::unordered_map<std::string, std::vector<Mn::Vector3>>>&
           markerMap) {
-    editTaskSet(taskSetName)->setAllMarkerPoints(markerMap);
+    auto taskSetPtr = editTaskSet(taskSetName);
+    for (const auto& markers : markerMap) {
+      taskSetPtr->setLinkSetPoints(markers.first, markers.second);
+    }
+    // After this process, the taskset might be empty, if so, delete it.
+    if (taskSetPtr->getNumLinkSets() == 0) {
+      removeTaskSet(taskSetName);
+    }
   }
 
   /**