Fix edge case handling for SubMesh::MaterialIndex (#319)

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

Migration.md

@@ -7,6 +7,12 @@ release will remove the deprecated code.
 
 ## Ignition Common 4.X to 5.X
 
+### Deprecations
+
+1. `Submesh::MaterialIndex` is deprecated. `SubMesh::GetMaterialIndex` should
+   be used instead, which properly handles submeshes having no material index
+   applied to them.
+
 ### Additions
 
 1. **geospatial** component that loads heightmap images and DEMs

graphics/include/ignition/common/SubMesh.hh

@@ -18,6 +18,7 @@
 #define IGNITION_COMMON_SUBMESH_HH_
 
 #include <memory>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -284,8 +285,15 @@ namespace ignition
       public: void SetMaterialIndex(const unsigned int _index);
 
       /// \brief Get the material index
-      /// \return The assigned material index.
-      public: unsigned int MaterialIndex() const;
+      /// \return The assigned material index. If no material index is assigned
+      /// to this submesh, std::numeric_limits<unsigned int>::max() is returned.
+      /// \note This method is deprecated, use GetMaterialIndex instead
+      public: unsigned int IGN_DEPRECATED(5) MaterialIndex() const;
+
+      /// \brief Get the material index
+      /// \return The assigned material index. Nullopt is returned if the
+      /// submesh has no assigned material index
+      public: std::optional<unsigned int> GetMaterialIndex() const;
 
       /// \brief Return true if this submesh has the vertex
       /// \param[in] _v Vertex coordinate

graphics/src/ColladaExporter.cc

@@ -488,12 +488,17 @@ void ColladaExporter::Implementation::ExportGeometries(
 {
   for (unsigned int i = 0; i < this->subMeshCount; ++i)
   {
-    unsigned int materialIndex =
-      this->mesh->SubMeshByIndex(i).lock()->MaterialIndex();
+    std::shared_ptr<SubMesh> subMesh = this->mesh->SubMeshByIndex(i).lock();
+    if (!subMesh)
+      continue;
 
     char meshId[100], materialId[100];
     snprintf(meshId, sizeof(meshId), "mesh_%u", i);
-    snprintf(materialId, sizeof(materialId), "material_%u", materialIndex);
+    if (subMesh->GetMaterialIndex())
+    {
+      snprintf(materialId, sizeof(materialId), "material_%u",
+          subMesh->GetMaterialIndex().value());
+    }
 
     tinyxml2::XMLElement *geometryXml =
       _libraryGeometriesXml->GetDocument()->NewElement("geometry");
@@ -504,10 +509,6 @@ void ColladaExporter::Implementation::ExportGeometries(
       _libraryGeometriesXml->GetDocument()->NewElement("mesh");
     geometryXml->LinkEndChild(meshXml);
 
-    std::shared_ptr<SubMesh> subMesh = this->mesh->SubMeshByIndex(i).lock();
-    if (!subMesh)
-      continue;
-
     this->ExportGeometrySource(subMesh.get(), meshXml, POSITION, meshId);
     this->ExportGeometrySource(subMesh.get(), meshXml, NORMAL, meshId);
     if (subMesh->TexCoordCountBySet(0) != 0)
@@ -887,9 +888,10 @@ void ColladaExporter::Implementation::ExportVisualScenes(
     snprintf(attributeValue, sizeof(attributeValue), "#%s", meshId);
     instanceGeometryXml->SetAttribute("url", attributeValue);
 
-    unsigned int materialIndex =
-      this->mesh->SubMeshByIndex(i).lock()->MaterialIndex();
-
+    const auto subMesh = this->mesh->SubMeshByIndex(i).lock();
+    if (!subMesh->GetMaterialIndex())
+      continue;
+    const auto materialIndex = subMesh->GetMaterialIndex().value();
     const ignition::common::MaterialPtr material =
       this->mesh->MaterialByIndex(materialIndex);
 

graphics/src/ColladaExporter_TEST.cc

@@ -227,20 +227,24 @@ TEST_F(ColladaExporter, ExportMeshWithSubmeshes)
   std::vector<math::Matrix4d> subMeshMatrix;
   math::Pose3d localPose = math::Pose3d::Zero;
 
+  const auto subMesh = boxMesh->SubMeshByIndex(0).lock();
+  ASSERT_NE(nullptr, subMesh);
+  ASSERT_TRUE(subMesh->GetMaterialIndex().has_value());
   int i = outMesh.AddMaterial(
-    boxMesh->MaterialByIndex(
-      boxMesh->SubMeshByIndex(0).lock()->MaterialIndex()));
-  subm = outMesh.AddSubMesh(*boxMesh->SubMeshByIndex(0).lock().get());
+    boxMesh->MaterialByIndex(subMesh->GetMaterialIndex().value()));
+  subm = outMesh.AddSubMesh(*subMesh.get());
   subm.lock()->SetMaterialIndex(i);
 
   localPose.SetX(10);
   math::Matrix4d matrix(localPose);
   subMeshMatrix.push_back(matrix);
 
+  const auto drillSubMesh = drillMesh->SubMeshByIndex(0).lock();
+  ASSERT_NE(nullptr, drillSubMesh);
+  ASSERT_TRUE(drillSubMesh->GetMaterialIndex().has_value());
   i = outMesh.AddMaterial(
-    drillMesh->MaterialByIndex(
-      drillMesh->SubMeshByIndex(0).lock()->MaterialIndex()));
-  subm = outMesh.AddSubMesh(*drillMesh->SubMeshByIndex(0).lock().get());
+    drillMesh->MaterialByIndex(drillSubMesh->GetMaterialIndex().value()));
+  subm = outMesh.AddSubMesh(*drillSubMesh.get());
   subm.lock()->SetMaterialIndex(i);
 
   localPose.SetX(-10);

graphics/src/SubMesh.cc

@@ -16,7 +16,9 @@
  */
 
 #include <algorithm>
+#include <limits>
 #include <map>
+#include <optional>
 #include <string>
 
 #include "ignition/math/Helpers.hh"
@@ -52,7 +54,7 @@ class ignition::common::SubMesh::Implementation
 
   /// \brief The material index for this mesh. Relates to the parent
   /// mesh material list.
-  public: int materialIndex = -1;
+  public: std::optional<unsigned int> materialIndex = std::nullopt;
 
   /// \brief The name of the sub-mesh
   public: std::string name;
@@ -493,6 +495,15 @@ void SubMesh::SetMaterialIndex(const unsigned int _index)
 
 //////////////////////////////////////////////////
 unsigned int SubMesh::MaterialIndex() const
+{
+  if (this-dataPtr->materialIndex.has_value())
+    return this->dataPtr->materialIndex.value();
+
+  return std::numeric_limits<unsigned int>::max();
+}
+
+//////////////////////////////////////////////////
+std::optional<unsigned int> SubMesh::GetMaterialIndex() const
 {
   return this->dataPtr->materialIndex;
 }

graphics/src/SubMesh_TEST.cc

@@ -31,16 +31,18 @@ class SubMeshTest : public common::testing::AutoLogFixture { };
 TEST_F(SubMeshTest, SubMesh)
 {
   common::SubMeshPtr submesh(new common::SubMesh());
-  EXPECT_TRUE(submesh != NULL);
+  ASSERT_NE(nullptr, submesh);
 
   submesh->SetName("new_submesh");
   EXPECT_EQ(submesh->Name(), "new_submesh");
 
   submesh->SetPrimitiveType(common::SubMesh::TRIANGLES);
   EXPECT_EQ(submesh->SubMeshPrimitiveType(), common::SubMesh::TRIANGLES);
 
+  EXPECT_FALSE(submesh->GetMaterialIndex().has_value());
   submesh->SetMaterialIndex(3u);
-  EXPECT_EQ(submesh->MaterialIndex(), 3u);
+  ASSERT_TRUE(submesh->GetMaterialIndex().has_value());
+  EXPECT_EQ(submesh->GetMaterialIndex(), 3u);
 
   // verify empty submesh
   EXPECT_EQ(submesh->Min(), math::Vector3d::Zero);
@@ -238,9 +240,9 @@ TEST_F(SubMeshTest, SubMesh)
 
   // copy constructor
   common::SubMeshPtr submeshCopy(new common::SubMesh(*(submesh.get())));
-  EXPECT_TRUE(submeshCopy != NULL);
+  ASSERT_NE(nullptr, submeshCopy);
   EXPECT_EQ(submeshCopy->Name(), submesh->Name());
-  EXPECT_EQ(submeshCopy->MaterialIndex(), submesh->MaterialIndex());
+  EXPECT_EQ(submeshCopy->GetMaterialIndex(), submesh->GetMaterialIndex());
   EXPECT_EQ(submeshCopy->SubMeshPrimitiveType(),
       submesh->SubMeshPrimitiveType());
   EXPECT_EQ(submeshCopy->VertexCount(), submesh->VertexCount());