-- Articulated Object creation restructuring (#2359)

* --initial commit; minor cleanups
* --remove model-based references to link visuals rendering
Handle this directly in the AO creation code using the values in the AO attributes.
* --remove model-based references to semantic ID;
Handle semantic ID specification directly via attributes on creation.
* --remove model-based references to render asset
Handle render asset specification directly upon instantiation via config values
* --remove unnecessary references to attributes in parser::parseURDF
* --pass attributes to BulletAOInitialize; migrate importer-based stuff to importer
* --construct instance attr prepop'ed with object attr values.
* --further reorganization and streamlining.
* --move attributes manager-based test to AttributesManagersTest
These tests are more appropriate in AttributesManagersTest and this move makes SimTest a bit less unruly.
* --expanded configuration tests
* --further expand configuration testing
* --add ConfigValue equality check
* --expanded tests for subconfigurations

src/esp/core/Configuration.cpp

@@ -223,6 +223,31 @@ std::string ConfigValue::getAsString() const {
   }  // switch
 }  // ConfigValue::getAsString
 
+bool operator==(const ConfigValue& a, const ConfigValue& b) {
+  if (a._type != b._type) {
+    return false;
+  }
+  // Types are equal, check values
+  // if trivial type, compare data arrays
+  if (a._type < ConfigStoredType::_nonTrivialTypes) {
+    return std::equal(std::begin(a._data), std::end(a._data),
+                      std::begin(b._data));
+  } else {
+    // Nontrivial, get values as appropriate
+    if (a._type == ConfigStoredType::String) {
+      return a.get<std::string>() == b.get<std::string>();
+    } else {
+      // Shouldn't get here
+      CORRADE_ASSERT_UNREACHABLE(
+          "Unknown/unsupported Type in ConfigValue equality check", false);
+    }
+  }
+}
+
+bool operator!=(const ConfigValue& a, const ConfigValue& b) {
+  return !(a == b);
+}
+
 io::JsonGenericValue ConfigValue::writeToJsonObject(
     io::JsonAllocator& allocator) const {
   // unknown is checked before this function is called, so does not need support

src/esp/core/Configuration.h

@@ -345,9 +345,19 @@ class ConfigValue {
                              Cr::Utility::ConfigurationGroup& cfg) const;
 
  public:
+  /**
+   * @brief Comparison
+   */
+  friend bool operator==(const ConfigValue& a, const ConfigValue& b);
+
   ESP_SMART_POINTERS(ConfigValue)
 };  // ConfigValue
 
+/**
+ * @brief Inequality Comparison
+ */
+bool operator!=(const ConfigValue& a, const ConfigValue& b);
+
 /**
  * @brief provide debug stream support for @ref ConfigValue
  */
@@ -660,8 +670,8 @@ class Configuration {
   }
 
   /**
-   * @brief This method will build a vector of all the config values this
-   * configuration holds and the types of these values.
+   * @brief This method will build a map of the keys of all the config values
+   * this configuration holds and the types of each of these values.
    */
   std::unordered_map<std::string, ConfigStoredType> getValueTypes() const {
     std::unordered_map<std::string, ConfigStoredType> res{};
@@ -717,7 +727,10 @@ class Configuration {
   std::shared_ptr<const Configuration> getSubconfigView(
       const std::string& name) const {
     auto configIter = configMap_.find(name);
-    CORRADE_ASSERT(configIter != configMap_.end(), "", nullptr);
+    CORRADE_ASSERT(
+        configIter != configMap_.end(),
+        "Subconfiguration with name " << name << " not found in Configuration.",
+        nullptr);
     // if exists return actual object
     return configIter->second;
   }
@@ -778,7 +791,7 @@ class Configuration {
   }
 
   /**
-   * @brief Retrieve the number of entries held by the subconfig with the give
+   * @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
    * with a warning.
@@ -800,7 +813,7 @@ class Configuration {
    * @param src The source of configuration data we wish to merge into this
    * configuration.
    */
-  void overwriteWithConfig(const std::shared_ptr<Configuration>& src) {
+  void overwriteWithConfig(const std::shared_ptr<const Configuration>& src) {
     if (src->getNumEntries() == 0) {
       return;
     }
@@ -809,7 +822,7 @@ class Configuration {
       valueMap_[elem.first] = elem.second;
     }
     // merge subconfigs
-    for (const auto& subConfig : configMap_) {
+    for (const auto& subConfig : src->configMap_) {
       const auto name = subConfig.first;
       // make if DNE and merge src subconfig
       addSubgroup(name)->overwriteWithConfig(subConfig.second);

src/esp/metadata/MetadataMediator.cpp

@@ -494,7 +494,7 @@ MetadataMediator::makeSceneAndReferenceStage(
   // create stage instance attributes and set its name (from stage attributes)
   sceneInstanceAttributes->setStageInstanceAttrs(
       dsSceneAttrMgr->createEmptyInstanceAttributes(
-          stageAttributes->getHandle()));
+          stageAttributes->getHandle(), stageAttributes));
 
   // The following is to manage stage configs that have navmesh handles defined
   // in them. This mechanism has been deprecated, but in order to provide

src/esp/metadata/URDFParser.cpp

@@ -103,29 +103,8 @@ void Model::setMassScaling(float massScaling) {
   m_massScaling = massScaling;
 }
 
-void Model::setModelInitAttributes(
-    metadata::attributes::ArticulatedObjectAttributes::ptr artObjAttributes) {
-  initializationAttributes_ = std::move(artObjAttributes);
-  // TODO these fields are redundant - can just use the attributes fields
-  // directly
-  m_renderAsset = initializationAttributes_->getRenderAssetHandle();
-  m_semanticId = initializationAttributes_->getSemanticId();
-  // TODO : Use the enum value instead of settting a boolean here
-
-  auto renderMode = initializationAttributes_->getRenderMode();
-
-  m_renderLinkVisualShapes =
-      (renderMode ==
-       metadata::attributes::ArticulatedObjectRenderMode::LinkVisuals) ||
-      (renderMode == metadata::attributes::ArticulatedObjectRenderMode::Both);
-
-}  // Model::setModelInitAttributes
-
-bool Parser::parseURDF(
-    const esp::metadata::attributes::ArticulatedObjectAttributes::ptr&
-        artObjAttributes,
-    std::shared_ptr<Model>& urdfModel) {
-  auto filename = artObjAttributes->getURDFPath();
+bool Parser::parseURDF(const std::string& filename,
+                       std::shared_ptr<Model>& urdfModel) {
   // override the previous model with a fresh one
   urdfModel = std::make_shared<Model>();
   sourceFilePath_ = filename;
@@ -250,9 +229,6 @@ bool Parser::parseURDF(
     return false;
   }
 
-  // Set the creation attributes
-  urdfModel->setModelInitAttributes(artObjAttributes);
-
   ESP_VERY_VERBOSE() << "Done parsing URDF for" << filename;
   return true;
 }  // Parser::parseURDF

src/esp/metadata/URDFParser.h

@@ -15,7 +15,6 @@
 #include <string>
 #include <vector>
 #include "Corrade/Containers/Optional.h"
-#include "attributes/ArticulatedObjectAttributes.h"
 #include "esp/core/Configuration.h"
 
 /** @file
@@ -363,82 +362,7 @@ class Model {
    */
   float getMassScaling() const { return m_massScaling; }
 
-  /**
-   * @brief Set the path to the render asset to attach to this URDF model.
-   */
-  void setRenderAsset(Cr::Containers::Optional<std::string> renderAsset) {
-    m_renderAsset = std::move(renderAsset);
-  }
-
-  /**
-   * @brief Get the path to the render asset to attach to this URDF model.
-   */
-  Cr::Containers::Optional<std::string> getRenderAsset() const {
-    return m_renderAsset;
-  }
-
-  /**
-   * @brief Set the semantic ID of the URDF model.
-   */
-  void setSemanticId(int semanticId) { m_semanticId = semanticId; }
-
-  /**
-   * @brief Get the semantic ID of this URDF model.
-   */
-  int getSemanticId() const { return m_semanticId; }
-
-  /**
-   * @brief Set hint to render articulated object primitives even if a render
-   * asset is present.
-   */
-  void setRenderLinkVisualShapes(bool renderLinkVisualShapes) {
-    m_renderLinkVisualShapes = renderLinkVisualShapes;
-  }
-
-  /**
-   * @brief Get hint to render articulated object visual shapes as defined in
-   * the URDF even if a render asset/skin is present.
-   */
-  bool getRenderLinkVisualShapes() const { return m_renderLinkVisualShapes; }
-
-  /**
-   * @brief This function will set the
-   * @ref metadata::attributes::ArticulatedObjectAttributes used to create this model.
-   */
-  void setModelInitAttributes(
-      metadata::attributes::ArticulatedObjectAttributes::ptr artObjAttributes);
-
-  /**
-   * @brief Gets a smart pointer reference to a copy of the user-specified
-   * configuration data from a config file. Habitat does not parse or
-   * process this data, but it will be available to the user via python
-   * bindings for each object.
-   */
-  std::shared_ptr<core::config::Configuration> getUserConfiguration() const {
-    return initializationAttributes_->getUserConfiguration();
-  }
-
-  /**
-   * @brief Get a copy of the template used to initialize this object.
-   *
-   * @return A copy of the @ref esp::metadata::attributes::ArticulatedObjectAttributes
-   * template used to create this object.
-   */
-  std::shared_ptr<metadata::attributes::ArticulatedObjectAttributes>
-  getInitializationAttributes() const {
-    return initializationAttributes_;
-  };
-
  protected:
-  /**
-   * @brief Json-based attributes defining characteristics of this model not
-   * specified in the source XML/URDF. Primarily to support default user-defined
-   * attributes. This data is read in from a json file with the same base name
-   * as the source XML/URDF for this model but the extension ".ao_config.json".
-   */
-  metadata::attributes::ArticulatedObjectAttributes::ptr
-      initializationAttributes_ = nullptr;
-
   // scaling values which can be applied to the model after parsing
   //! Global euclidean scaling applied to the model's transforms, asset scales,
   //! and prismatic joint limits. Does not affect mass.
@@ -447,16 +371,6 @@ class Model {
   //! Mass scaling of the model's Link inertias.
   float m_massScaling = 1.0;
 
-  //! Path to a render asset associated with this articulated object.
-  Cr::Containers::Optional<std::string> m_renderAsset = Cr::Containers::NullOpt;
-
-  //! Semantic ID of this model.
-  int m_semanticId = 0;
-
-  //! Forces link visual shapes to be rendered even if a render asset(skin) is
-  //! present.
-  bool m_renderLinkVisualShapes = false;
-
   //! Scale the transformation and parameters of a Shape
   void scaleShape(Shape& shape, float scale);
 };  // class model
@@ -610,9 +524,7 @@ class Parser {
 
   // parse a loaded URDF string into relevant general data structures
   // return false if the string is not a valid urdf or other error causes abort
-  bool parseURDF(const metadata::attributes::ArticulatedObjectAttributes::ptr&
-                     artObjAttributes,
-                 std::shared_ptr<Model>& model);
+  bool parseURDF(const std::string& filename, std::shared_ptr<Model>& model);
 
   // This is no longer used, instead set the urdf and physics subsystem to
   // veryverbose, i.e. export HABITAT_SIM_LOG="urdf,physics=veryverbose" bool

src/esp/metadata/attributes/ArticulatedObjectAttributes.cpp

@@ -59,7 +59,7 @@ std::string ArticulatedObjectAttributes::getObjectInfoHeaderInternal() const {
 
 std::string ArticulatedObjectAttributes::getObjectInfoInternal() const {
   return Cr::Utility::formatString(
-      "{},{},{},{},{},{},{},{}", getURDFPath(), getRenderAssetHandle(),
+      "{},{},{},{},{},{},{},{},{},{}", getURDFPath(), getRenderAssetHandle(),
       getAsString("semantic_id"), getAsString("uniform_scale"),
       getAsString("mass_scale"), getAOBaseTypeName(getBaseType()),
       getAOInertiaSourceName(getInertiaSource()),

src/esp/metadata/attributes/ArticulatedObjectAttributes.h

@@ -74,7 +74,7 @@ class ArticulatedObjectAttributes : public AbstractAttributes {
       ESP_ERROR(Mn::Debug::Flag::NoSpace)
           << "'" << baseType
           << "' is an illegal value for "
-             "ArticulatedObjectAttributes::setBaseType, so default value "
+             "ArticulatedObjectAttributes::setBaseType, so current value "
           << get<std::string>("base_type") << " not changed.";
       return;
     }
@@ -110,7 +110,7 @@ class ArticulatedObjectAttributes : public AbstractAttributes {
       ESP_ERROR(Mn::Debug::Flag::NoSpace)
           << "'" << inertiaSrc
           << "' is an illegal value for "
-             "ArticulatedObjectAttributes::setInertiaSource, so default value '"
+             "ArticulatedObjectAttributes::setInertiaSource, so current value '"
           << get<std::string>("inertia_source") << "' not changed.";
       return;
     }
@@ -146,7 +146,7 @@ class ArticulatedObjectAttributes : public AbstractAttributes {
       ESP_ERROR(Mn::Debug::Flag::NoSpace)
           << "'" << linkOrder
           << "' is an illegal value for "
-             "ArticulatedObjectAttributes::setLinkOrder, so default value '"
+             "ArticulatedObjectAttributes::setLinkOrder, so current value '"
           << get<std::string>("link_order") << "' not changed.";
       return;
     }