added error when color element is empty

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

src/Material.cc

@@ -72,10 +72,11 @@ class sdf::Material::Implementation
   public: std::string filePath = "";
 
   /// \brief Get a valid color. This checks the color components specified in
-  /// this->sdf are r,g,b or r,g,b,a and each value is between [0,1].
-  /// If the checks do not pass, the default color 0,0,0,1 is returned.
+  /// this->sdf are r,g,b or r,g,b,a (expects 3 or 4 values) and each value is
+  /// between [0,1]. When only 3 values are present (r,g,b), then alpha is set
+  /// to 1. If the checks do not pass, the default color 0,0,0,1 is returned.
   /// \param[in] _colorType The color type (i.e., ambient, diffuse, specular,
-  /// or emissive)
+  /// or emissive), which is case sensitive and expects a lowercase string
   /// \param[out] _errors Parsing errors will be appended to this variable
   /// \return A color for _colorType. If no parsing errors occurred then the
   /// specified color component from this->sdf is returned, otherwise 0,0,0,1
@@ -380,7 +381,11 @@ ignition::math::Color Material::Implementation::GetColor(
                                   ->GetValue()
                                   ->GetAsString();
   if (colorStr.empty())
+  {
+    _errors.push_back({ErrorCode::ELEMENT_INVALID, "No values found inside <" +
+        _colorType + "> element. Using default values 0 0 0 1."});
     return color;
+  }
 
   std::stringstream ss(colorStr);
   std::string token;
@@ -394,7 +399,7 @@ ignition::math::Color Material::Implementation::GetColor(
       c = std::stof(token);
       colors.push_back(c);
     }
-    catch(const std::exception &e)
+    catch(const std::exception &/*e*/)
     {
       std::cerr << "Error converting color value: can not convert ["
                 << token << "] to a float" << std::endl;
@@ -410,9 +415,9 @@ ignition::math::Color Material::Implementation::GetColor(
   }
 
   size_t colorSize = colors.size();
-  if (colorSize == 3)
+  if (colorSize == 3u)
     colors.push_back(1.0f);
-  else if (colorSize < 4 || colorSize > 4)
+  else if (colorSize != 4u)
     isValidColor = false;
 
   if (!isValidColor)

src/Material_TEST.cc

@@ -292,7 +292,7 @@ TEST(DOMMaterial, InvalidSdf)
 
 void PrintErrors(sdf::Errors &_errors)
 {
-  for (auto e : _errors)
+  for (const auto &e : _errors)
     std::cout << e << std::endl;
 }
 
@@ -302,11 +302,10 @@ TEST(DOMMaterial, Colors)
   sdf::ElementPtr elem(new sdf::Element());
   elem->SetName("material");
 
-  // invalid diffuse
+  // invalid diffuse (empty)
   sdf::ElementPtr elemDiffuse(new sdf::Element());
   elemDiffuse->SetName("diffuse");
-  elemDiffuse->AddValue("string", "0 0 0 1", false, "description");
-  elemDiffuse->Set<std::string>("1.5 0 0 1");
+  elemDiffuse->AddValue("string", "", false, "description");
   elem->InsertElement(elemDiffuse);
 
   sdf::Material material;
@@ -320,7 +319,17 @@ TEST(DOMMaterial, Colors)
   EXPECT_EQ(material.Specular(), ignition::math::Color(0, 0, 0, 1));
   EXPECT_EQ(material.Emissive(), ignition::math::Color(0, 0, 0, 1));
 
-  // another invalid diffuse
+  // another invalid diffuse (value is > 1)
+  elemDiffuse->Set<std::string>("1.5 0 0 1");
+  errors.clear();
+  errors = material.Load(elem);
+  PrintErrors(errors);
+
+  ASSERT_EQ(errors.size(), 1u);
+  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::ELEMENT_INVALID);
+  EXPECT_EQ(material.Diffuse(), ignition::math::Color(0, 0, 0, 1));
+
+  // another invalid diffuse (only 2 values)
   elemDiffuse->Set<std::string>("0.1       0.2");
   errors.clear();
   errors = material.Load(elem);
@@ -332,21 +341,21 @@ TEST(DOMMaterial, Colors)
 
   // invalid ambient
   sdf::ElementPtr elemAmbient(new sdf::Element());
-  elemAmbient->AddValue("string", "0 0 0 1", false, "description");
+  elemAmbient->AddValue("string", "", false, "description");
   elemAmbient->SetName("ambient");
   elemAmbient->Set<std::string>("0.1 0.2    test");
   elem->InsertElement(elemAmbient);
 
   // invalid specular
   sdf::ElementPtr elemSpecular(new sdf::Element());
-  elemSpecular->AddValue("string", "0 0 0 1", false, "description");
+  elemSpecular->AddValue("string", "", false, "description");
   elemSpecular->SetName("specular");
   elemSpecular->Set<std::string>("0.1 0.2 0.3 0.4 0.5");
   elem->InsertElement(elemSpecular);
 
   // invalid emissive
   sdf::ElementPtr elemEmissive(new sdf::Element());
-  elemEmissive->AddValue("string", "0 0 0 1", false, "description");
+  elemEmissive->AddValue("string", "", false, "description");
   elemEmissive->SetName("emissive");
   elemEmissive->Set<std::string>("-0.1 0.2 0.3 0.4");
   elem->InsertElement(elemEmissive);
@@ -356,9 +365,14 @@ TEST(DOMMaterial, Colors)
   PrintErrors(errors);
 
   EXPECT_EQ(errors.size(), 4u);
-  for (auto e : errors)
+  for (const auto &e : errors)
     EXPECT_EQ(e.Code(), sdf::ErrorCode::ELEMENT_INVALID);
 
+  EXPECT_EQ(material.Ambient(), ignition::math::Color(0, 0, 0, 1));
+  EXPECT_EQ(material.Diffuse(), ignition::math::Color(0, 0, 0, 1));
+  EXPECT_EQ(material.Specular(), ignition::math::Color(0, 0, 0, 1));
+  EXPECT_EQ(material.Emissive(), ignition::math::Color(0, 0, 0, 1));
+
   // valid diffuse, ambient, specular, emissive
   elemDiffuse->Set<std::string>("0 0.1 0.2");
   elemAmbient->Set<std::string>("0.3 0.4 0.55 1");
@@ -370,8 +384,9 @@ TEST(DOMMaterial, Colors)
   PrintErrors(errors);
 
   EXPECT_EQ(errors.size(), 0u);
-  EXPECT_EQ(material.Diffuse(), ignition::math::Color(0, 0.1, 0.2, 1));
-  EXPECT_EQ(material.Ambient(), ignition::math::Color(0.3, 0.4, 0.55, 1));
-  EXPECT_EQ(material.Specular(), ignition::math::Color(0, 0.1, 0.2, 0.3));
-  EXPECT_EQ(material.Emissive(), ignition::math::Color(0.12, 0.23, 0.34, 0.56));
+  EXPECT_EQ(material.Diffuse(), ignition::math::Color(0, 0.1f, 0.2f, 1));
+  EXPECT_EQ(material.Ambient(), ignition::math::Color(0.3f, 0.4f, 0.55f, 1));
+  EXPECT_EQ(material.Specular(), ignition::math::Color(0, 0.1f, 0.2f, 0.3f));
+  EXPECT_EQ(material.Emissive(),
+            ignition::math::Color(0.12f, 0.23f, 0.34f, 0.56f));
 }