--[Bugfix] - Address ObjectInstanceAttributes save issues (#2472)

* --access init instance attributes for an rigid/AO via const ref or copy
* --access sim directly to query for defaultCOMCorrection state
So that objects added to an existing scene will use the correct defaultCOMCorrection state.
* --verify requested type is appropriate
* --add queries for current state of Configuration fields
Query whether a field is a default value, a hidden value or backed by an enum (i.e. 'translated')
* --properly set enum-backed/'translated' string fields in instance attrs
* --add is_visible boolean for AOs
For instance consistency - not yet supported.
* --only set instance vals if not default config vals; clean up JSON write
Configuration now supports not writing initialization/default fields to JSON.
* --Add access to user_defined view and setter
* --better names for object init attrs and object instance  init attrs
* --use base obj's current state to build ObjectInstanceAttr and obj copy
* --only change fields if they are different than defaults.

src/esp/core/Configuration.h

@@ -751,6 +751,8 @@ class Configuration {
     return {};
   }
 
+  // ****************** Value Status ******************
+
   /**
    * @brief Return the @ref ConfigValType enum representing the type of the
    * value referenced by the passed @p key or @ref ConfigValType::Unknown
@@ -765,6 +767,46 @@ class Configuration {
     return ConfigValType::Unknown;
   }
 
+  /**
+   * @brief Returns whether or not the @ref ConfigValue specified
+   * by @p key is a default/initialization value or was intentionally set.
+   */
+  bool isDefaultVal(const std::string& key) const {
+    ValueMapType::const_iterator mapIter = valueMap_.find(key);
+    if (mapIter != valueMap_.end()) {
+      return mapIter->second.isDefaultVal();
+    }
+    ESP_ERROR() << "Key :" << key << "not present in Configuration.";
+    return false;
+  }
+
+  /**
+   * @brief Returns whether or not the @ref ConfigValue specified
+   * by @p key is a hidden value intended to be be only used internally.
+   */
+  bool isHiddenVal(const std::string& key) const {
+    ValueMapType::const_iterator mapIter = valueMap_.find(key);
+    if (mapIter != valueMap_.end()) {
+      return mapIter->second.isHiddenVal();
+    }
+    ESP_ERROR() << "Key :" << key << "not present in Configuration.";
+    return false;
+  }
+
+  /**
+   * @brief Returns whether or not the @ref ConfigValue specified
+   * by @p key is a translated value, meaning a string that corresponds to, and
+   * is translated into, an enum value for consumption.
+   */
+  bool isTranslated(const std::string& key) const {
+    ValueMapType::const_iterator mapIter = valueMap_.find(key);
+    if (mapIter != valueMap_.end()) {
+      return mapIter->second.isTranslated();
+    }
+    ESP_ERROR() << "Key :" << key << "not present in Configuration.";
+    return false;
+  }
+
   // ****************** String Conversion ******************
 
   /**

src/esp/metadata/attributes/AbstractAttributes.h

@@ -121,16 +121,36 @@ class AbstractAttributes
     return getSubconfigCopy<Configuration>("user_defined");
   }
 
+  /**
+   * @brief Gets a const smart pointer reference to a view of the user-specified
+   * configuration data from 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<const Configuration> getUserConfigurationView() const {
+    return getSubconfigView("user_defined");
+  }
+
   /**
    * @brief Gets a smart pointer reference to the actual user-specified
    * configuration data from config file. Habitat does not parse or process this
    * data, but it will be available to the user via python bindings for each
-   * object.  This method is for editing the configuration.
+   * object. This method is for editing the configuration.
    */
   std::shared_ptr<Configuration> editUserConfiguration() {
     return editSubconfig<Configuration>("user_defined");
   }
 
+  /**
+   * @brief Move an existing user_defined subconfiguration into this
+   * configuration, overwriting the existing copy if it exists. Habitat does not
+   * parse or process this data, but it will be available to the user via python
+   * bindings for each object. This method is for editing the configuration.
+   */
+  void setUserConfiguration(std::shared_ptr<Configuration>& userAttr) {
+    setSubconfigPtr("user_defined", userAttr);
+  }
+
   /**
    * @brief Returns the number of user-defined values (within the "user-defined"
    * sub-ConfigurationGroup) this attributes has.

src/esp/metadata/attributes/AbstractObjectAttributes.h

@@ -77,11 +77,18 @@ class AbstractObjectAttributes : public AbstractAttributes {
   double getUnitsToMeters() const { return get<double>("units_to_meters"); }
 
   /**
-   * @brief If not visible can add dynamic non-rendered object into a scene. If
-   * is not visible then should not add object to drawables.
+   * @brief Set whether visible or not. If not visible can add dynamic
+   * non-rendered object into a scene. If is not visible then should not add
+   * object to drawables.
    */
   void setIsVisible(bool isVisible) { set("is_visible", isVisible); }
+  /**
+   * @brief Get whether visible or not. If not visible can add dynamic
+   * non-rendered object into a scene. If is not visible then should not add
+   * object to drawables.
+   */
   bool getIsVisible() const { return get<bool>("is_visible"); }
+
   void setFrictionCoefficient(double frictionCoefficient) {
     set("friction_coefficient", frictionCoefficient);
   }

src/esp/metadata/attributes/ArticulatedObjectAttributes.cpp

@@ -16,6 +16,7 @@ ArticulatedObjectAttributes::ArticulatedObjectAttributes(
 
   init("render_asset", "");
   init("semantic_id", 0);
+  init("is_visible", true);
   // Initialize the default base type to be free joint
   initTranslated("base_type",
                  getAOBaseTypeName(ArticulatedObjectBaseType::Free));

src/esp/metadata/attributes/ArticulatedObjectAttributes.h

@@ -99,6 +99,19 @@ class ArticulatedObjectAttributes : public AbstractAttributes {
    */
   double getMassScale() const { return get<double>("mass_scale"); }
 
+  /**
+   * @brief Set whether visible or not. If not visible can add dynamic
+   * non-rendered object into a scene. If is not visible then should not add
+   * object to drawables.
+   */
+  void setIsVisible(bool isVisible) { set("is_visible", isVisible); }
+  /**
+   * @brief Get whether visible or not. If not visible can add dynamic
+   * non-rendered object into a scene. If is not visible then should not add
+   * object to drawables.
+   */
+  bool getIsVisible() const { return get<bool>("is_visible"); }
+
   /**
    * @brief Set the type of base/root joint to use to add this Articulated
    * Object to the world. Cannot be "UNSPECIFIED"

src/esp/metadata/attributes/AttributesEnumMaps.cpp

@@ -206,6 +206,7 @@ std::string getShaderTypeName(ObjectInstanceShaderType shaderTypeVal) {
 
 const std::map<std::string, SceneInstanceTranslationOrigin>
     InstanceTranslationOriginMap = {
+        {"default", SceneInstanceTranslationOrigin::Unknown},
         {"asset_local", SceneInstanceTranslationOrigin::AssetLocal},
         {"com", SceneInstanceTranslationOrigin::COM},
 };

src/esp/metadata/attributes/SceneInstanceAttributes.cpp

@@ -21,19 +21,22 @@ SceneObjectInstanceAttributes::SceneObjectInstanceAttributes(
     : AbstractAttributes(type, handle) {
   // default to unknown for object instances, to use attributes-specified
   // defaults
-  init("shader_type", getShaderTypeName(ObjectInstanceShaderType::Unspecified));
+  initTranslated("shader_type",
+                 getShaderTypeName(ObjectInstanceShaderType::Unspecified));
 
   // defaults to unknown/undefined
-  init("motion_type", getMotionTypeName(esp::physics::MotionType::UNDEFINED));
+  initTranslated("motion_type",
+                 getMotionTypeName(esp::physics::MotionType::UNDEFINED));
   // set to no rotation or translation
   init("rotation", Mn::Quaternion(Mn::Math::IdentityInit));
   init("translation", Mn::Vector3());
   // don't override attributes-specified visibility. - sets int value to
   // ID_UNDEFINED
   init("is_instance_visible", ID_UNDEFINED);
   // defaults to unknown so that obj instances use scene instance setting
-  init("translation_origin",
-       getTranslationOriginName(SceneInstanceTranslationOrigin::Unknown));
+  initTranslated(
+      "translation_origin",
+      getTranslationOriginName(SceneInstanceTranslationOrigin::Unknown));
   // set default multiplicative scaling values
   init("uniform_scale", 1.0);
   init("non_uniform_scale", Mn::Vector3{1.0, 1.0, 1.0});
@@ -50,18 +53,27 @@ SceneObjectInstanceAttributes::SceneObjectInstanceAttributes(
   // to a scene instance.
   // Handle is set via init in base class, which would not be written out to
   // file if we did not explicitly set it.
+  // NOTE : this will not call a virtual override
+  // (SceneAOInstanceAttributes::setHandle) of AbstractAttributes::setHandle due
+  // to virtual dispatch not being available in constructor
   setHandle(handle);
   // set appropriate fields from abstract object attributes
   // Not initialize, since these are not default values
-  set("shader_type", getShaderTypeName(baseObjAttribs->getShaderType()));
-  // set to match attributes setting
-  set("is_instance_visible", (baseObjAttribs->getIsVisible() ? 1 : 0));
+
+  // Need to verify that the baseObjAttribs values are not defaults before we
+  // set these values.
+
+  if (!baseObjAttribs->isDefaultVal("shader_type")) {
+    setShaderType(getShaderTypeName(baseObjAttribs->getShaderType()));
+  }
+  if (!baseObjAttribs->isDefaultVal("is_visible")) {
+    setIsInstanceVisible(baseObjAttribs->getIsVisible());
+  }
 
   // set nonuniform scale to match attributes scale
-  setNonUniformScale(baseObjAttribs->getScale());
-  // Prepopulate user config to match baseObjAttribs' user config.
-  editUserConfiguration()->overwriteWithConfig(
-      baseObjAttribs->getUserConfiguration());
+  if (!baseObjAttribs->isDefaultVal("scale")) {
+    setNonUniformScale(baseObjAttribs->getScale());
+  }
 }
 
 std::string SceneObjectInstanceAttributes::getObjectInfoHeaderInternal() const {
@@ -118,41 +130,23 @@ void SceneObjectInstanceAttributes::writeValuesToJson(
     io::JsonAllocator& allocator) const {
   // map "handle" to "template_name" key in json
   writeValueToJson("handle", "template_name", jsonObj, allocator);
-  if (getTranslation() != Mn::Vector3()) {
-    writeValueToJson("translation", jsonObj, allocator);
-  }
-  if (getTranslationOrigin() != SceneInstanceTranslationOrigin::Unknown) {
-    writeValueToJson("translation_origin", jsonObj, allocator);
-  }
-  if (getRotation() != Mn::Quaternion(Mn::Math::IdentityInit)) {
-    writeValueToJson("rotation", jsonObj, allocator);
-  }
+  writeValueToJson("translation", jsonObj, allocator);
+  writeValueToJson("translation_origin", jsonObj, allocator);
+  writeValueToJson("rotation", jsonObj, allocator);
+
   // map "is_instance_visible" to boolean only if not -1, otherwise don't save
   int visSet = getIsInstanceVisible();
   if (visSet != ID_UNDEFINED) {
     // set JSON value based on visSet (0,1) as bool
     auto jsonVal = io::toJsonValue(static_cast<bool>(visSet), allocator);
     jsonObj.AddMember("is_instance_visible", jsonVal, allocator);
   }
-  if (getMotionType() != esp::physics::MotionType::UNDEFINED) {
-    writeValueToJson("motion_type", jsonObj, allocator);
-  }
-  if (getShaderType() != ObjectInstanceShaderType::Unspecified) {
-    writeValueToJson("shader_type", jsonObj, allocator);
-  }
-  if (getUniformScale() != 1.0f) {
-    writeValueToJson("uniform_scale", jsonObj, allocator);
-  }
-  if (getNonUniformScale() != Mn::Vector3(1.0, 1.0, 1.0)) {
-    writeValueToJson("non_uniform_scale", jsonObj, allocator);
-  }
-  if (!getApplyScaleToMass()) {
-    writeValueToJson("apply_scale_to_mass", jsonObj, allocator);
-  }
-  if (getMassScale() != 1.0) {
-    writeValueToJson("mass_scale", jsonObj, allocator);
-  }
-
+  writeValueToJson("motion_type", jsonObj, allocator);
+  writeValueToJson("shader_type", jsonObj, allocator);
+  writeValueToJson("uniform_scale", jsonObj, allocator);
+  writeValueToJson("non_uniform_scale", jsonObj, allocator);
+  writeValueToJson("apply_scale_to_mass", jsonObj, allocator);
+  writeValueToJson("mass_scale", jsonObj, allocator);
   // take care of child class values, if any exist
   writeValuesToJsonInternal(jsonObj, allocator);
 
@@ -168,19 +162,21 @@ SceneAOInstanceAttributes::SceneAOInstanceAttributes(const std::string& handle)
 
   // Set the instance base type to be unspecified - if not set in instance json,
   // use ao_config value
-  init("base_type", getAOBaseTypeName(ArticulatedObjectBaseType::Unspecified));
+  initTranslated("base_type",
+                 getAOBaseTypeName(ArticulatedObjectBaseType::Unspecified));
   // Set the instance source for the inertia calculation to be unspecified - if
   // not set in instance json, use ao_config value
-  init("inertia_source",
-       getAOInertiaSourceName(ArticulatedObjectInertiaSource::Unspecified));
+  initTranslated(
+      "inertia_source",
+      getAOInertiaSourceName(ArticulatedObjectInertiaSource::Unspecified));
   // Set the instance link order to use as unspecified - if not set in instance
   // json, use ao_config value
-  init("link_order",
-       getAOLinkOrderName(ArticulatedObjectLinkOrder::Unspecified));
+  initTranslated("link_order",
+                 getAOLinkOrderName(ArticulatedObjectLinkOrder::Unspecified));
   // Set render mode to be unspecified - if not set in instance json, use
   // ao_config value
-  init("render_mode",
-       getAORenderModeName(ArticulatedObjectRenderMode::Unspecified));
+  initTranslated("render_mode",
+                 getAORenderModeName(ArticulatedObjectRenderMode::Unspecified));
   editSubconfig<Configuration>("initial_joint_pose");
   editSubconfig<Configuration>("initial_joint_velocities");
 }
@@ -194,27 +190,40 @@ SceneAOInstanceAttributes::SceneAOInstanceAttributes(
   // a scene instance.
   // Handle is set via init in base class, which would not be written out to
   // file if we did not explicitly set it.
+  // NOTE : this will not call a virtual override
+  // (SceneAOInstanceAttributes::setHandle) of AbstractAttributes::setHandle due
+  // to virtual dispatch not being available in constructor
   setHandle(handle);
 
   // Should not initialize these values but set them, since these are not
   // default values, but from an existing AO attributes.
+
+  // Need to verify that the aObjAttribs values are not defaults before we
+  // set these values.
   // Set shader type to use aObjAttribs value
-  setShaderType(getShaderTypeName(aObjAttribs->getShaderType()));
-  // Set the instance base type to use aObjAttribs value
-  setBaseType(getAOBaseTypeName(aObjAttribs->getBaseType()));
-  // Set the instance source for the inertia calculation to use aObjAttribs
-  // value
-  setInertiaSource(getAOInertiaSourceName(aObjAttribs->getInertiaSource()));
-  // Set the instance link order to use aObjAttribs value
-  setLinkOrder(getAOLinkOrderName(aObjAttribs->getLinkOrder()));
-  // Set render mode to use aObjAttribs value
-  setRenderMode(getAORenderModeName(aObjAttribs->getRenderMode()));
-
-  // Prepopulate user config to match attribs' user config.
-  editUserConfiguration()->overwriteWithConfig(
-      aObjAttribs->getUserConfiguration());
-  editSubconfig<Configuration>("initial_joint_pose");
-  editSubconfig<Configuration>("initial_joint_velocities");
+  if (!aObjAttribs->isDefaultVal("shader_type")) {
+    setShaderType(getShaderTypeName(aObjAttribs->getShaderType()));
+  }
+  if (!aObjAttribs->isDefaultVal("is_visible")) {
+    setIsInstanceVisible(aObjAttribs->getIsVisible());
+  }
+  if (!aObjAttribs->isDefaultVal("base_type")) {
+    // Set the instance base type to use aObjAttribs value
+    setBaseType(getAOBaseTypeName(aObjAttribs->getBaseType()));
+  }
+  if (!aObjAttribs->isDefaultVal("inertia_source")) {
+    // Set the instance source for the inertia calculation to use aObjAttribs
+    // value
+    setInertiaSource(getAOInertiaSourceName(aObjAttribs->getInertiaSource()));
+  }
+  if (!aObjAttribs->isDefaultVal("link_order")) {
+    // Set the instance link order to use aObjAttribs value
+    setLinkOrder(getAOLinkOrderName(aObjAttribs->getLinkOrder()));
+  }
+  if (!aObjAttribs->isDefaultVal("render_mode")) {
+    // Set render mode to use aObjAttribs value
+    setRenderMode(getAORenderModeName(aObjAttribs->getRenderMode()));
+  }
 }
 
 std::string SceneAOInstanceAttributes::getSceneObjInstanceInfoHeaderInternal()
@@ -262,19 +271,10 @@ std::string SceneAOInstanceAttributes::getSceneObjInstanceInfoInternal() const {
 void SceneAOInstanceAttributes::writeValuesToJsonInternal(
     io::JsonGenericValue& jsonObj,
     io::JsonAllocator& allocator) const {
-  if (getBaseType() != ArticulatedObjectBaseType::Unspecified) {
-    writeValueToJson("base_type", jsonObj, allocator);
-  }
-  if (getInertiaSource() != ArticulatedObjectInertiaSource::Unspecified) {
-    writeValueToJson("inertia_source", jsonObj, allocator);
-  }
-  if (getLinkOrder() != ArticulatedObjectLinkOrder::Unspecified) {
-    writeValueToJson("link_order", jsonObj, allocator);
-  }
-  if (getRenderMode() != ArticulatedObjectRenderMode::Unspecified) {
-    writeValueToJson("render_mode", jsonObj, allocator);
-  }
-
+  writeValueToJson("base_type", jsonObj, allocator);
+  writeValueToJson("inertia_source", jsonObj, allocator);
+  writeValueToJson("link_order", jsonObj, allocator);
+  writeValueToJson("render_mode", jsonObj, allocator);
   writeValueToJson("auto_clamp_joint_limits", jsonObj, allocator);
 
 }  // SceneAOInstanceAttributes::writeValuesToJsonInternal