--[BE] - Miscellaneous Clean-up and Refactors (#2513)

* --loop iterator signed-ness matching
* --remove unused values; improve field validation feedback
* --cleanup config bindings
* --mark unused arguments
* --signed-ness; moving a temp prevents copy elision
* --only 1 copy of constexpr string; unused args cleanup.
* --properly handle the move constructors

src/esp/assets/ResourceManager.cpp

@@ -1238,7 +1238,8 @@ void ResourceManager::buildPrimitiveAssetData(
       << "Primitive Asset Added : ID : " << primTemplate->getID()
       << ": Attributes key : `" << primTemplate->getHandle() << "`| Class : `"
       << primClassName << "` | Importer Conf has group for this obj type : "
-      << conf.hasGroup(primClassName);
+      << conf.hasGroup(primClassName) << " | Asset placed in resourceDict : "
+      << (inserted.second ? "True" : "False");
 
 }  // ResourceManager::buildPrimitiveAssetData
 
@@ -1397,7 +1398,7 @@ bool ResourceManager::loadRenderAssetSemantic(const AssetInfo& info) {
 
   // specify colormap to use to build TextureVisualizerShader
   // If this is true, we want to build a colormap from the vertex colors.
-  for (int meshIDLocal = 0; meshIDLocal < instanceMeshes.size();
+  for (uint32_t meshIDLocal = 0; meshIDLocal < instanceMeshes.size();
        ++meshIDLocal) {
     if (getCreateRenderer()) {
       instanceMeshes[meshIDLocal]->uploadBuffersToGPU(false);
@@ -1546,7 +1547,7 @@ bool ResourceManager::loadRenderAssetGeneral(const AssetInfo& info) {
           "Error loading general mesh data from file '{}'", filename));
 
   // load file and add it to the dictionary
-  LoadedAssetData loadedAssetData{info};
+  LoadedAssetData loadedAssetData{info, {}};
   if (requiresTextures_) {
     loadTextures(*fileImporter_, loadedAssetData);
     loadMaterials(*fileImporter_, loadedAssetData);
@@ -1826,8 +1827,8 @@ bool ResourceManager::buildTrajectoryVisualization(
   // if (resourceDict_.count(trajVisName) != 0) {
   //   resourceDict_.erase(trajVisName);
   // }
-  auto inserted =
-      resourceDict_.emplace(trajVisName, std::move(loadedAssetData));
+  // auto inserted =
+  resourceDict_.emplace(trajVisName, std::move(loadedAssetData));
 
   return true;
 }  // ResourceManager::loadTrajectoryVisualization
@@ -2046,7 +2047,7 @@ Mn::Trade::MaterialData createUniversalMaterial(
       arrayAppend(newAttributes,
                   {{MaterialAttribute::AmbientTexture, BCTexture},
                    {MaterialAttribute::DiffuseTexture, BCTexture}});
-      if (metalness >= 0.5) {
+      if (metalness >= 0.5f) {
         arrayAppend(newAttributes,
                     {MaterialAttribute::SpecularTexture, BCTexture});
       }
@@ -2634,7 +2635,7 @@ void ResourceManager::loadMeshes(Importer& importer,
   nextMeshID_ = meshEnd + 1;
   loadedAssetData.meshMetaData.setMeshIndices(meshStart, meshEnd);
 
-  for (int iMesh = 0; iMesh < importer.meshCount(); ++iMesh) {
+  for (uint32_t iMesh = 0; iMesh < importer.meshCount(); ++iMesh) {
     // don't need normals if we aren't using lighting
     auto gltfMeshData = std::make_unique<GenericMeshData>(
         !loadedAssetData.assetInfo.forceFlatShading);
@@ -2659,7 +2660,7 @@ void ResourceManager::loadSkins(Importer& importer,
   nextSkinID_ = skinEnd + 1;
   loadedAssetData.meshMetaData.setSkinIndices(skinStart, skinEnd);
 
-  for (int iSkin = 0; iSkin < importer.skin3DCount(); ++iSkin) {
+  for (uint32_t iSkin = 0; iSkin < importer.skin3DCount(); ++iSkin) {
     auto skinData = std::make_shared<gfx::SkinData>();
 
     Cr::Containers::Optional<Mn::Trade::SkinData3D> skin =
@@ -2694,10 +2695,10 @@ Mn::Image2D ResourceManager::convertRGBToSemanticId(
       srcImage.pixels<Mn::Color3ub>();
   Cr::Containers::StridedArrayView2D<Mn::UnsignedShort> output =
       resImage.pixels<Mn::UnsignedShort>();
-  for (std::size_t y = 0; y != size.y(); ++y) {
+  for (long y = 0; y != size.y(); ++y) {
     Cr::Containers::StridedArrayView1D<const Mn::Color3ub> inputRow = input[y];
     Cr::Containers::StridedArrayView1D<Mn::UnsignedShort> outputRow = output[y];
-    for (std::size_t x = 0; x != size.x(); ++x) {
+    for (long x = 0; x != size.x(); ++x) {
       const Mn::Color3ub color = inputRow[x];
       /* Fugly. Sorry. Needs better API on Magnum side. */
       const Mn::UnsignedInt colorInt = geo::getValueAsUInt(color);
@@ -2739,7 +2740,8 @@ void ResourceManager::loadTextures(Importer& importer,
           static_cast<Mn::UnsignedShort>(i);
     }
 
-    for (int iTexture = 0; iTexture < importer.textureCount(); ++iTexture) {
+    for (uint32_t iTexture = 0; iTexture < importer.textureCount();
+         ++iTexture) {
       auto currentTextureID = textureStart + iTexture;
       auto txtrIter = textures_.emplace(currentTextureID,
                                         std::make_shared<Mn::GL::Texture2D>());
@@ -2775,7 +2777,8 @@ void ResourceManager::loadTextures(Importer& importer,
       // Whether semantic RGB or not
     }
   } else {
-    for (int iTexture = 0; iTexture < importer.textureCount(); ++iTexture) {
+    for (uint32_t iTexture = 0; iTexture < importer.textureCount();
+         ++iTexture) {
       auto currentTextureID = textureStart + iTexture;
       auto txtrIter = textures_.emplace(currentTextureID,
                                         std::make_shared<Mn::GL::Texture2D>());

src/esp/bindings/ConfigBindings.cpp

@@ -20,10 +20,12 @@ py::object getObjectForConfigValue(const ConfigValue& value) {
       return py::cast(value.get<bool>());
     case ConfigValType::Integer:
       return py::cast(value.get<int>());
+    case ConfigValType::MagnumRad:
+      return py::cast(static_cast<Mn::Radd>(value.get<Mn::Rad>()));
+    case ConfigValType::MagnumDeg:
+      return py::cast(static_cast<Mn::Degd>(value.get<Mn::Deg>()));
     case ConfigValType::Double:
       return py::cast(value.get<double>());
-    case ConfigValType::String:
-      return py::cast(value.get<std::string>());
     case ConfigValType::MagnumVec2:
       return py::cast(value.get<Mn::Vector2>());
     case ConfigValType::MagnumVec2i:
@@ -38,10 +40,8 @@ py::object getObjectForConfigValue(const ConfigValue& value) {
       return py::cast(value.get<Mn::Matrix4>());
     case ConfigValType::MagnumQuat:
       return py::cast(value.get<Mn::Quaternion>());
-    case ConfigValType::MagnumRad:
-      return py::cast(static_cast<Mn::Radd>(value.get<Mn::Rad>()));
-    case ConfigValType::MagnumDeg:
-      return py::cast(static_cast<Mn::Degd>(value.get<Mn::Deg>()));
+    case ConfigValType::String:
+      return py::cast(value.get<std::string>());
   }
   return py::cast(nullptr);
 }
@@ -83,17 +83,17 @@ void initConfigBindings(py::module& m) {
       .value("Unknown", ConfigValType::Unknown)
       .value("Boolean", ConfigValType::Boolean)
       .value("Integer", ConfigValType::Integer)
+      .value("MagnumRad", ConfigValType::MagnumRad)
+      .value("MagnumDeg", ConfigValType::MagnumDeg)
       .value("Float", ConfigValType::Double)
-      .value("String", ConfigValType::String)
       .value("MagnumVec2", ConfigValType::MagnumVec2)
       .value("MagnumVec2i", ConfigValType::MagnumVec2i)
       .value("MagnumVec3", ConfigValType::MagnumVec3)
       .value("MagnumVec4", ConfigValType::MagnumVec4)
       .value("MagnumQuat", ConfigValType::MagnumQuat)
       .value("MagnumMat3", ConfigValType::MagnumMat3)
       .value("MagnumMat4", ConfigValType::MagnumMat4)
-      .value("MagnumRad", ConfigValType::MagnumRad)
-      .value("MagnumDeg", ConfigValType::MagnumDeg);
+      .value("String", ConfigValType::String);
 
   auto pyConfiguration =
       py::class_<Configuration, Configuration::ptr>(m, "Configuration");

src/esp/metadata/URDFParser.cpp

@@ -453,24 +453,25 @@ bool Parser::parseLink(const std::shared_ptr<Model>& model,
       ESP_VERY_VERBOSE() << link.m_name;
       return false;
     }
-    link.m_inertia.m_mass *= model->getGlobalScaling();
+    link.m_inertia.m_mass *= static_cast<double>(model->getGlobalScaling());
   } else {
     if ((strlen(linkName) == 5) && (strncmp(linkName, "world", 5)) == 0) {
-      link.m_inertia.m_mass = 0.f;
+      link.m_inertia.m_mass = 0.0;
       link.m_inertia.m_linkLocalFrame = Mn::Matrix4();  // Identity
-      link.m_inertia.m_ixx = 0.f;
-      link.m_inertia.m_iyy = 0.f;
-      link.m_inertia.m_izz = 0.f;
+      link.m_inertia.m_ixx = 0.0;
+      link.m_inertia.m_iyy = 0.0;
+      link.m_inertia.m_izz = 0.0;
     } else {
       ESP_VERY_VERBOSE()
           << "W - No inertial data for link:" << link.m_name
           << ", using mass=1, localinertiadiagonal = 1,1,1, identity local "
              "inertial frame";
-      link.m_inertia.m_mass = 1.f * model->getMassScaling();
+      link.m_inertia.m_mass =
+          1.0 * static_cast<double>(model->getMassScaling());
       link.m_inertia.m_linkLocalFrame = Mn::Matrix4();  // Identity
-      link.m_inertia.m_ixx = 1.f;
-      link.m_inertia.m_iyy = 1.f;
-      link.m_inertia.m_izz = 1.f;
+      link.m_inertia.m_ixx = 1.0;
+      link.m_inertia.m_iyy = 1.0;
+      link.m_inertia.m_izz = 1.0;
     }
   }
 
@@ -625,9 +626,9 @@ bool Parser::parseTransform(Mn::Matrix4& tr, const XMLElement* xml) const {
   if (rpy_str != nullptr) {
     Mn::Vector3 rpy;
     if (parseVector3(rpy, std::string(rpy_str))) {
-      double roll = rpy[0];
-      double pitch = rpy[1];
-      double yaw = rpy[2];
+      double roll = static_cast<double>(rpy[0]);
+      double pitch = static_cast<double>(rpy[1]);
+      double yaw = static_cast<double>(rpy[2]);
 
       double phi = roll / 2.0;
       double the = pitch / 2.0;
@@ -766,7 +767,7 @@ bool Parser::parseGeometry(Geometry& geom, const XMLElement* g) {
 
 bool Parser::parseInertia(Inertia& inertia, const XMLElement* config) {
   inertia.m_linkLocalFrame = Mn::Matrix4();  // Identity
-  inertia.m_mass = 0.f;
+  inertia.m_mass = 0.0;
 
   // Origin
   const XMLElement* o = config->FirstChildElement("origin");
@@ -913,12 +914,12 @@ bool Parser::initTreeAndRoot(const std::shared_ptr<Model>& model) const {
 
 bool Parser::parseJointLimits(Joint& joint,
                               const tinyxml2::XMLElement* config) const {
-  joint.m_lowerLimit = 0.f;
-  joint.m_upperLimit = -1.f;
-  joint.m_effortLimit = 0.f;
-  joint.m_velocityLimit = 0.f;
-  joint.m_jointDamping = 0.f;
-  joint.m_jointFriction = 0.f;
+  joint.m_lowerLimit = 0.0;
+  joint.m_upperLimit = -1.0;
+  joint.m_effortLimit = 0.0;
+  joint.m_velocityLimit = 0.0;
+  joint.m_jointDamping = 0.0;
+  joint.m_jointFriction = 0.0;
 
   const char* lower_str = config->Attribute("lower");
   if (lower_str) {

src/esp/metadata/attributes/AbstractAttributes.h

@@ -19,7 +19,7 @@ namespace metadata {
  * attributes should replace this tag with the base filename (minus all paths
  * and extensions)
  */
-constexpr char CONFIG_NAME_AS_ASSET_FILENAME[] =
+static constexpr char CONFIG_NAME_AS_ASSET_FILENAME[] =
     "%%CONFIG_NAME_AS_ASSET_FILENAME%%";
 
 namespace attributes {

src/esp/metadata/attributes/AttributesEnumMaps.cpp

@@ -344,23 +344,26 @@ const std::map<std::string, esp::sensor::FisheyeSensorModelType>
  * the string key it maps to in the FisheyeSensorModelTypeMap
  */
 std::string getFisheyeSensorModelTypeName(
-    esp::sensor::FisheyeSensorModelType fisheyeSensorModelTypeEnum) {
+    CORRADE_UNUSED esp::sensor::FisheyeSensorModelType
+        fisheyeSensorModelTypeEnum) {
   // this verifies that enum value being checked is supported by string-keyed
   // map. The values below should be the minimum and maximum enums supported by
   // FisheyeSensorModelTypeMap
-  if ((fisheyeSensorModelTypeEnum <=
-       esp::sensor::FisheyeSensorModelType::DoubleSphere) ||
-      (fisheyeSensorModelTypeEnum >=
-       esp::sensor::FisheyeSensorModelType::EndFisheyeSensorModelType)) {
-    // default to double_sphere for illegal values
-    return "double_sphere";
-  }
-  // Must always be valid value
-  for (const auto& it : FisheyeSensorModelTypeMap) {
-    if (it.second == fisheyeSensorModelTypeEnum) {
-      return it.first;
-    }
-  }
+
+  // TODO expand to handle more types of FisheyeSensors
+  // if ((fisheyeSensorModelTypeEnum <=
+  //      esp::sensor::FisheyeSensorModelType::DoubleSphere) ||
+  //     (fisheyeSensorModelTypeEnum >=
+  //      esp::sensor::FisheyeSensorModelType::EndFisheyeSensorModelType)) {
+  //   // default to double_sphere for illegal values
+  //   return "double_sphere";
+  // }
+  // // Must always be valid value
+  // for (const auto& it : FisheyeSensorModelTypeMap) {
+  //   if (it.second == fisheyeSensorModelTypeEnum) {
+  //     return it.first;
+  //   }
+  // }
   return "double_sphere";
 }  // getFisheyeSensorModelTypeName
 

src/esp/metadata/attributes/LightLayoutAttributes.cpp

@@ -58,7 +58,7 @@ LightLayoutAttributes::LightLayoutAttributes(const LightLayoutAttributes& otr)
 }
 LightLayoutAttributes::LightLayoutAttributes(
     LightLayoutAttributes&& otr) noexcept
-    : AbstractAttributes(std::move(static_cast<AbstractAttributes>(otr))),
+    : AbstractAttributes(std::move(static_cast<AbstractAttributes&&>(otr))),
       availableLightIDs_(std::move(otr.availableLightIDs_)) {
   lightInstConfig_ = editSubconfig<Configuration>("lights");
 }

src/esp/metadata/attributes/SceneInstanceAttributes.cpp

@@ -229,17 +229,13 @@ SceneAOInstanceAttributes::SceneAOInstanceAttributes(
 std::string SceneAOInstanceAttributes::getSceneObjInstanceInfoHeaderInternal()
     const {
   std::string infoHdr{"Base Type,Inertia Source,Link Order,Render Mode,"};
-  int iter = 0;
   const auto initJointPose = getInitJointPose();
-  for (const auto& it : initJointPose) {
-    Cr::Utility::formatInto(infoHdr, infoHdr.size(), "Init Pose {},",
-                            std::to_string(iter++));
+  for (uint32_t iter = 0; iter < initJointPose.size(); ++iter) {
+    Cr::Utility::formatInto(infoHdr, infoHdr.size(), "Init Pose {},", iter);
   }
-  iter = 0;
   const auto initJointVel = getInitJointVelocities();
-  for (const auto& it : initJointVel) {
-    Cr::Utility::formatInto(infoHdr, infoHdr.size(), "Init Vel {},",
-                            std::to_string(iter++));
+  for (uint32_t iter = 0; iter < initJointPose.size(); ++iter) {
+    Cr::Utility::formatInto(infoHdr, infoHdr.size(), "Init Vel {},", iter);
   }
   return infoHdr;
 }  // SceneAOInstanceAttributes::getSceneObjInstanceInfoHeaderInternal
@@ -319,7 +315,7 @@ SceneInstanceAttributes::SceneInstanceAttributes(
 }
 SceneInstanceAttributes::SceneInstanceAttributes(
     SceneInstanceAttributes&& otr) noexcept
-    : AbstractAttributes(std::move(static_cast<AbstractAttributes>(otr))),
+    : AbstractAttributes(std::move(static_cast<AbstractAttributes&&>(otr))),
       availableObjInstIDs_(std::move(otr.availableObjInstIDs_)),
       availableArtObjInstIDs_(std::move(otr.availableArtObjInstIDs_)) {
   // get refs to internal subconfigs for object and ao instances

src/esp/metadata/attributes/SemanticAttributes.cpp

@@ -22,7 +22,7 @@ std::string SemanticVolumeAttributes::getObjectInfoHeaderInternal() const {
   std::string res =
       "Name,Label,Floor Height,Extrusion Height,Min Bounds,Max Bounds,";
   const auto& polyLoop = getPolyLoop();
-  for (int iter = 0; iter < polyLoop.size(); ++iter) {
+  for (uint32_t iter = 0; iter < polyLoop.size(); ++iter) {
     Cr::Utility::formatInto(res, res.size(), "Poly Vert {} XYZ,",
                             std::to_string(iter));
   }
@@ -90,7 +90,7 @@ SemanticAttributes::SemanticAttributes(const SemanticAttributes& otr)
 }  // SemanticAttributes copy ctor
 
 SemanticAttributes::SemanticAttributes(SemanticAttributes&& otr) noexcept
-    : AbstractAttributes(std::move(static_cast<AbstractAttributes>(otr))),
+    : AbstractAttributes(std::move(static_cast<AbstractAttributes&&>(otr))),
       availableRegionInstIDs_(std::move(otr.availableRegionInstIDs_)) {
   // get refs to internal subconfigs for semantic region attributes
   // original data were moved over so should retain full derived class

src/esp/metadata/managers/AbstractAttributesManager.h

@@ -413,7 +413,7 @@ AbstractAttributesManager<T, Access>::loadAllFileBasedTemplates(
     std::string dir = CrPath::split(paths[0]).first();
     ESP_DEBUG() << "Loading" << paths.size() << "" << this->objectType_
                 << "templates found in" << dir;
-    for (int i = 0; i < paths.size(); ++i) {
+    for (uint32_t i = 0; i < paths.size(); ++i) {
       auto attributesFilename = paths[i];
       ESP_VERY_VERBOSE() << "Load" << this->objectType_ << "template:"
                          << CrPath::split(attributesFilename).second();

src/esp/metadata/managers/AssetAttributesManager.cpp

@@ -250,8 +250,8 @@ AbstractPrimitiveAttributes::ptr AssetAttributesManager::buildObjectFromJSONDoc(
 }  // AssetAttributesManager::buildObjectFromJSONDoc
 
 void AssetAttributesManager::setValsFromJSONDoc(
-    AttribsPtr attribs,
-    const io::JsonGenericValue& jsonConfig) {
+    CORRADE_UNUSED AttribsPtr attribs,
+    CORRADE_UNUSED const io::JsonGenericValue& jsonConfig) {
   // TODO support loading values from JSON docs
   // check for user defined attributes
   // this->parseUserDefinedJsonVals(attribs, jsonConfig);

src/esp/metadata/managers/LightLayoutAttributesManager.cpp

@@ -325,8 +325,9 @@ gfx::LightSetup LightLayoutAttributesManager::createLightSetupFromAttributes(
             lightVector = {lightAttr->getPosition(), 1.0f};
           }
         }  // switch on type
-        res.push_back(
-            {.vector = lightVector, .color = color, .model = posModelEnum});
+
+        res.push_back({lightVector, color, posModelEnum});
+        //{.vector = lightVector, .color = color, .model = posModelEnum});
       }  // for each light instance described
     }    // if >0 light instances described
   }      // lightLayoutAttributes of requested name exists

src/esp/metadata/managers/PbrShaderAttributesManager.cpp

@@ -231,7 +231,8 @@ PbrShaderAttributesManager::preRegisterObjectFinalize(
 }  // PbrShaderAttributesManager::preRegisterObjectFinalize
 
 void PbrShaderAttributesManager::finalizeAttrPathsBeforeRegister(
-    const attributes::PbrShaderAttributes::ptr& attributes) const {
+    CORRADE_UNUSED const attributes::PbrShaderAttributes::ptr& attributes)
+    const {
   // TODO Verify getIBLBrdfLUTAssetHandle and getIBLEnvMapAssetHandle exist as
   // either file-based assets or resources and build paths to be relative if
   // file-based

src/esp/metadata/managers/SceneDatasetAttributesManager.cpp

@@ -151,7 +151,7 @@ void SceneDatasetAttributesManager::readDatasetJSONCell(
       // Get the count of the number of expected members found. If more members
       // than this exist in the cell, then we will read for extra key-value
       // mappings.
-      int numMembersFound = 0;
+      uint32_t numMembersFound = 0;
       // process JSON jCell here - this cell potentially holds :
       // 1. "default_attributes" : a single attributes default of the
       // specified type.

src/esp/metadata/managers/SceneDatasetAttributesManager.h

@@ -105,7 +105,7 @@ class SceneDatasetAttributesManager
    * @param attributes The attributes to be filtered.
    */
   void finalizeAttrPathsBeforeRegister(
-      const attributes::SceneDatasetAttributes::ptr& attributes)
+      CORRADE_UNUSED const attributes::SceneDatasetAttributes::ptr& attributes)
       const override {}
 
  protected: