From 8359bbf842bd387332541543d1a25ac321d2b0c5 Mon Sep 17 00:00:00 2001 From: Jenn Nguyen Date: Wed, 17 Mar 2021 19:16:32 -0700 Subject: [PATCH 1/4] updated loading //material colors Signed-off-by: Jenn Nguyen --- src/Material.cc | 85 ++++++++++++++++++++++++++++++++++++++----- src/Material_TEST.cc | 86 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+), 8 deletions(-) diff --git a/src/Material.cc b/src/Material.cc index d4459801b..84cd01c46 100644 --- a/src/Material.cc +++ b/src/Material.cc @@ -14,6 +14,7 @@ * limitations under the License. * */ +#include #include #include #include @@ -69,6 +70,17 @@ class sdf::Material::Implementation /// \brief The path to the file where this material was defined. 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. + /// \param[in] _colorType The color type (i.e., ambient, diffuse, specular, + /// or emissive) + /// \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 + public: ignition::math::Color GetColor(const std::string &_colorType, + Errors &_errors); }; ///////////////////////////////////////////////// @@ -168,17 +180,13 @@ Errors Material::Load(sdf::ElementPtr _sdf) this->dataPtr->renderOrder = _sdf->Get("render_order", this->dataPtr->renderOrder).first; - this->dataPtr->ambient = _sdf->Get("ambient", - this->dataPtr->ambient).first; + this->dataPtr->ambient = this->dataPtr->GetColor("ambient", errors); - this->dataPtr->diffuse = _sdf->Get("diffuse", - this->dataPtr->diffuse).first; + this->dataPtr->diffuse = this->dataPtr->GetColor("diffuse", errors); - this->dataPtr->specular = _sdf->Get("specular", - this->dataPtr->specular).first; + this->dataPtr->specular = this->dataPtr->GetColor("specular", errors); - this->dataPtr->emissive = _sdf->Get("emissive", - this->dataPtr->emissive).first; + this->dataPtr->emissive = this->dataPtr->GetColor("emissive", errors); this->dataPtr->lighting = _sdf->Get("lighting", this->dataPtr->lighting).first; @@ -358,3 +366,64 @@ void Material::SetFilePath(const std::string &_filePath) { this->dataPtr->filePath = _filePath; } + +////////////////////////////////////////////////// +ignition::math::Color Material::Implementation::GetColor( + const std::string &_colorType, Errors &_errors) +{ + ignition::math::Color color(0, 0, 0, 1); + + if (!this->sdf->HasElement(_colorType)) + return color; + + std::string colorStr = this->sdf->GetElement(_colorType) + ->GetValue() + ->GetAsString(); + if (colorStr.empty()) + return color; + + std::stringstream ss(colorStr); + std::string token; + std::vector colors; + float c; // r,g,b,a values + bool isValidColor = true; + while (ss >> token) + { + try + { + c = std::stof(token); + colors.push_back(c); + } + catch(const std::exception &e) + { + std::cerr << "Error converting color value: can not convert [" + << token << "] to a float" << std::endl; + isValidColor = false; + break; + } + + if (c < 0.0f || c > 1.0f) + { + isValidColor = false; + break; + } + } + + size_t colorSize = colors.size(); + if (colorSize == 3) + colors.push_back(1.0f); + else if (colorSize < 4 || colorSize > 4) + isValidColor = false; + + if (!isValidColor) + { + _errors.push_back({ErrorCode::ELEMENT_INVALID, "The value <" + _colorType + + ">" + colorStr + " is invalid. " + + "Using default values 0 0 0 1 for <" + _colorType + "> element."}); + return color; + } + + color.Set(colors[0], colors[1], colors[2], colors[3]); + + return color; +} diff --git a/src/Material_TEST.cc b/src/Material_TEST.cc index 115a56849..12b0b1641 100644 --- a/src/Material_TEST.cc +++ b/src/Material_TEST.cc @@ -289,3 +289,89 @@ TEST(DOMMaterial, InvalidSdf) sdf::Errors errors = material.Load(elem); EXPECT_EQ(sdf::ErrorCode::ELEMENT_INCORRECT_TYPE, errors[0].Code()); } + +void PrintErrors(sdf::Errors &_errors) +{ + for (auto e : _errors) + std::cout << e << std::endl; +} + +///////////////////////////////////////////////// +TEST(DOMMaterial, Colors) +{ + sdf::ElementPtr elem(new sdf::Element()); + elem->SetName("material"); + + // invalid diffuse + sdf::ElementPtr elemDiffuse(new sdf::Element()); + elemDiffuse->SetName("diffuse"); + elemDiffuse->AddValue("string", "0 0 0 1", false, "description"); + elemDiffuse->Set("1.5 0 0 1"); + elem->InsertElement(elemDiffuse); + + sdf::Material material; + sdf::Errors errors = material.Load(elem); + PrintErrors(errors); + + ASSERT_EQ(errors.size(), 1u); + EXPECT_EQ(errors[0].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)); + + // another invalid diffuse + elemDiffuse->Set("0.1 0.2"); + 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)); + + // invalid ambient + sdf::ElementPtr elemAmbient(new sdf::Element()); + elemAmbient->AddValue("string", "0 0 0 1", false, "description"); + elemAmbient->SetName("ambient"); + elemAmbient->Set("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->SetName("specular"); + elemSpecular->Set("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->SetName("emissive"); + elemEmissive->Set("-0.1 0.2 0.3 0.4"); + elem->InsertElement(elemEmissive); + + errors.clear(); + errors = material.Load(elem); + PrintErrors(errors); + + EXPECT_EQ(errors.size(), 4u); + for (auto e : errors) + EXPECT_EQ(e.Code(), sdf::ErrorCode::ELEMENT_INVALID); + + // valid diffuse, ambient, specular, emissive + elemDiffuse->Set("0 0.1 0.2"); + elemAmbient->Set("0.3 0.4 0.55 1"); + elemSpecular->Set("0 0.1 0.2 0.3"); + elemEmissive->Set("0.12 0.23 0.34 0.56"); + + errors.clear(); + errors = material.Load(elem); + 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)); +} From f4633ca1011bd63afe05cb51928a5736a52fa09c Mon Sep 17 00:00:00 2001 From: Jenn Nguyen Date: Fri, 19 Mar 2021 11:47:35 -0700 Subject: [PATCH 2/4] added error when color element is empty Signed-off-by: Jenn Nguyen --- src/Material.cc | 17 +++++++++++------ src/Material_TEST.cc | 41 ++++++++++++++++++++++++++++------------- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/src/Material.cc b/src/Material.cc index 84cd01c46..774f3512a 100644 --- a/src/Material.cc +++ b/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) diff --git a/src/Material_TEST.cc b/src/Material_TEST.cc index 12b0b1641..54f82ee20 100644 --- a/src/Material_TEST.cc +++ b/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("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("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("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("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("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("-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("0 0.1 0.2"); elemAmbient->Set("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)); } From 39c05a539f665f413d54154ce5df423073399c1e Mon Sep 17 00:00:00 2001 From: Jenn Nguyen Date: Mon, 22 Mar 2021 17:18:31 -0700 Subject: [PATCH 3/4] moved color parsing to Param Signed-off-by: Jenn Nguyen --- src/Material.cc | 90 ++--------------- src/Material_TEST.cc | 101 ------------------- src/Param.cc | 76 +++++++++++--- src/Param_TEST.cc | 9 +- test/integration/CMakeLists.txt | 1 + test/integration/material.cc | 169 ++++++++++++++++++++++++++++++++ test/sdf/material_invalid.sdf | 15 +++ test/sdf/material_valid.sdf | 14 +++ 8 files changed, 275 insertions(+), 200 deletions(-) create mode 100644 test/integration/material.cc create mode 100644 test/sdf/material_invalid.sdf create mode 100644 test/sdf/material_valid.sdf diff --git a/src/Material.cc b/src/Material.cc index 774f3512a..d4459801b 100644 --- a/src/Material.cc +++ b/src/Material.cc @@ -14,7 +14,6 @@ * limitations under the License. * */ -#include #include #include #include @@ -70,18 +69,6 @@ class sdf::Material::Implementation /// \brief The path to the file where this material was defined. 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 (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), 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 - public: ignition::math::Color GetColor(const std::string &_colorType, - Errors &_errors); }; ///////////////////////////////////////////////// @@ -181,13 +168,17 @@ Errors Material::Load(sdf::ElementPtr _sdf) this->dataPtr->renderOrder = _sdf->Get("render_order", this->dataPtr->renderOrder).first; - this->dataPtr->ambient = this->dataPtr->GetColor("ambient", errors); + this->dataPtr->ambient = _sdf->Get("ambient", + this->dataPtr->ambient).first; - this->dataPtr->diffuse = this->dataPtr->GetColor("diffuse", errors); + this->dataPtr->diffuse = _sdf->Get("diffuse", + this->dataPtr->diffuse).first; - this->dataPtr->specular = this->dataPtr->GetColor("specular", errors); + this->dataPtr->specular = _sdf->Get("specular", + this->dataPtr->specular).first; - this->dataPtr->emissive = this->dataPtr->GetColor("emissive", errors); + this->dataPtr->emissive = _sdf->Get("emissive", + this->dataPtr->emissive).first; this->dataPtr->lighting = _sdf->Get("lighting", this->dataPtr->lighting).first; @@ -367,68 +358,3 @@ void Material::SetFilePath(const std::string &_filePath) { this->dataPtr->filePath = _filePath; } - -////////////////////////////////////////////////// -ignition::math::Color Material::Implementation::GetColor( - const std::string &_colorType, Errors &_errors) -{ - ignition::math::Color color(0, 0, 0, 1); - - if (!this->sdf->HasElement(_colorType)) - return color; - - std::string colorStr = this->sdf->GetElement(_colorType) - ->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; - std::vector colors; - float c; // r,g,b,a values - bool isValidColor = true; - while (ss >> token) - { - try - { - c = std::stof(token); - colors.push_back(c); - } - catch(const std::exception &/*e*/) - { - std::cerr << "Error converting color value: can not convert [" - << token << "] to a float" << std::endl; - isValidColor = false; - break; - } - - if (c < 0.0f || c > 1.0f) - { - isValidColor = false; - break; - } - } - - size_t colorSize = colors.size(); - if (colorSize == 3u) - colors.push_back(1.0f); - else if (colorSize != 4u) - isValidColor = false; - - if (!isValidColor) - { - _errors.push_back({ErrorCode::ELEMENT_INVALID, "The value <" + _colorType + - ">" + colorStr + " is invalid. " + - "Using default values 0 0 0 1 for <" + _colorType + "> element."}); - return color; - } - - color.Set(colors[0], colors[1], colors[2], colors[3]); - - return color; -} diff --git a/src/Material_TEST.cc b/src/Material_TEST.cc index 54f82ee20..115a56849 100644 --- a/src/Material_TEST.cc +++ b/src/Material_TEST.cc @@ -289,104 +289,3 @@ TEST(DOMMaterial, InvalidSdf) sdf::Errors errors = material.Load(elem); EXPECT_EQ(sdf::ErrorCode::ELEMENT_INCORRECT_TYPE, errors[0].Code()); } - -void PrintErrors(sdf::Errors &_errors) -{ - for (const auto &e : _errors) - std::cout << e << std::endl; -} - -///////////////////////////////////////////////// -TEST(DOMMaterial, Colors) -{ - sdf::ElementPtr elem(new sdf::Element()); - elem->SetName("material"); - - // invalid diffuse (empty) - sdf::ElementPtr elemDiffuse(new sdf::Element()); - elemDiffuse->SetName("diffuse"); - elemDiffuse->AddValue("string", "", false, "description"); - elem->InsertElement(elemDiffuse); - - sdf::Material material; - sdf::Errors errors = material.Load(elem); - PrintErrors(errors); - - ASSERT_EQ(errors.size(), 1u); - EXPECT_EQ(errors[0].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)); - - // another invalid diffuse (value is > 1) - elemDiffuse->Set("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("0.1 0.2"); - 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)); - - // invalid ambient - sdf::ElementPtr elemAmbient(new sdf::Element()); - elemAmbient->AddValue("string", "", false, "description"); - elemAmbient->SetName("ambient"); - elemAmbient->Set("0.1 0.2 test"); - elem->InsertElement(elemAmbient); - - // invalid specular - sdf::ElementPtr elemSpecular(new sdf::Element()); - elemSpecular->AddValue("string", "", false, "description"); - elemSpecular->SetName("specular"); - elemSpecular->Set("0.1 0.2 0.3 0.4 0.5"); - elem->InsertElement(elemSpecular); - - // invalid emissive - sdf::ElementPtr elemEmissive(new sdf::Element()); - elemEmissive->AddValue("string", "", false, "description"); - elemEmissive->SetName("emissive"); - elemEmissive->Set("-0.1 0.2 0.3 0.4"); - elem->InsertElement(elemEmissive); - - errors.clear(); - errors = material.Load(elem); - PrintErrors(errors); - - EXPECT_EQ(errors.size(), 4u); - 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("0 0.1 0.2"); - elemAmbient->Set("0.3 0.4 0.55 1"); - elemSpecular->Set("0 0.1 0.2 0.3"); - elemEmissive->Set("0.12 0.23 0.34 0.56"); - - errors.clear(); - errors = material.Load(elem); - PrintErrors(errors); - - EXPECT_EQ(errors.size(), 0u); - 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)); -} diff --git a/src/Param.cc b/src/Param.cc index ced4922dc..190781b42 100644 --- a/src/Param.cc +++ b/src/Param.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -359,6 +360,63 @@ bool ParseUsingStringStream(const std::string &_input, const std::string &_key, return true; } +////////////////////////////////////////////////// +/// \brief Helper function for Param::ValueFromString for parsing colors +/// This checks the color components specified in _input are rgb or rgba +/// (expects 3 or 4 values) and each value is between [0,1]. When only 3 values +/// are present (rgb), then alpha is set to 1. +/// \param[in] _input Input string. +/// \param[in] _key Key of the parameter, used for error message. +/// \param[out] _value This will be set with the parsed value. +/// \return True if parsing colors succeeded. +bool ParseColorUsingStringStream(const std::string &_input, + const std::string &_key, ParamPrivate::ParamVariant &_value) +{ + StringStreamClassicLocale ss(_input); + std::string token; + std::vector colors; + float c; // r,g,b,a values + bool isValidColor = true; + while (ss >> token) + { + try + { + c = std::stof(token); + colors.push_back(c); + } + catch(const std::exception &/*e*/) + { + sdferr << "Error converting color value [" << token << "] to a float\n"; + isValidColor = false; + break; + } + + if (c < 0.0f || c > 1.0f) + { + isValidColor = false; + break; + } + } + + size_t colorSize = colors.size(); + if (isValidColor && colorSize == 3u) + colors.push_back(1.0f); + else if (colorSize != 4u) + isValidColor = false; + + if (isValidColor) + { + _value = ignition::math::Color(colors[0], colors[1], colors[2], colors[3]); + } + else + { + sdferr << "The value <" << _key << + ">" << _input << " is invalid.\n"; + } + + return isValidColor; +} + ////////////////////////////////////////////////// bool Param::ValueFromString(const std::string &_value) { @@ -448,22 +506,8 @@ bool Param::ValueFromString(const std::string &_value) else if (this->dataPtr->typeName == "ignition::math::Color" || this->dataPtr->typeName == "color") { - // The insertion operator (>>) expects 4 values, but the last value (the - // alpha) is optional. We first try to parse assuming the alpha is - // specified. If that fails, we append the default value of alpha to the - // string and try to parse again. - bool result = ParseUsingStringStream( - tmp, this->dataPtr->key, this->dataPtr->value); - - if (!result) - { - ignition::math::Color colortmp; - return ParseUsingStringStream( - tmp + " " + std::to_string(colortmp.A()), this->dataPtr->key, - this->dataPtr->value); - } - else - return true; + return ParseColorUsingStringStream( + tmp, this->dataPtr->key, this->dataPtr->value); } else if (this->dataPtr->typeName == "ignition::math::Vector2i" || this->dataPtr->typeName == "vector2i") diff --git a/src/Param_TEST.cc b/src/Param_TEST.cc index 2af855e27..37899d889 100644 --- a/src/Param_TEST.cc +++ b/src/Param_TEST.cc @@ -290,6 +290,13 @@ TEST(Param, InvalidConstructor) sdf::AssertionInternalError); } +//////////////////////////////////////////////////// +TEST(Param, InvalidColorConstructor) +{ + ASSERT_THROW(sdf::Param("key", "color", "8 20 67 23", false, "description"), + sdf::AssertionInternalError); +} + //////////////////////////////////////////////////// TEST(Param, SetDescription) { @@ -392,7 +399,7 @@ TEST(Param, GetAny) sdf::Param timeParam("key", "time", "8 20", false, "description"); EXPECT_TRUE(timeParam.GetAny(anyValue)); - sdf::Param colorParam("key", "color", "8 20 67 23", false, "description"); + sdf::Param colorParam("key", "color", "0 0.1 0.2 0.3", false, "description"); EXPECT_TRUE(colorParam.GetAny(anyValue)); sdf::Param vector3Param("key", "vector3", "8.1 20.24 67.7", false, diff --git a/test/integration/CMakeLists.txt b/test/integration/CMakeLists.txt index 0e587604a..7261f2dd7 100644 --- a/test/integration/CMakeLists.txt +++ b/test/integration/CMakeLists.txt @@ -24,6 +24,7 @@ set(tests locale_fix.cc locale_fix_cxx.cc material_pbr.cc + material.cc model_dom.cc model_versions.cc nested_model.cc diff --git a/test/integration/material.cc b/test/integration/material.cc new file mode 100644 index 000000000..ee2330da9 --- /dev/null +++ b/test/integration/material.cc @@ -0,0 +1,169 @@ +/* + * Copyright 2021 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include +#include + +#include + +#include "sdf/sdf.hh" +#include "test_config.h" + +void ExpectInvalidWithMessage(sdf::Errors &_errors, std::string _compType) +{ + for (const auto &e : _errors) + { + if (e.Message().find(_compType) != std::string::npos) + { + EXPECT_EQ(e.Code(), sdf::ErrorCode::ELEMENT_INVALID); + break; + } + } +} + +////////////////////////////////////////////////// +TEST(Material, InvalidColors) +{ + std::string testFile = + sdf::testing::TestFile("sdf", "material_invalid.sdf"); + + sdf::Root root; + sdf::Errors errors = root.Load(testFile); + std::cout << errors << std::endl; + + ASSERT_FALSE(errors.empty()); + ExpectInvalidWithMessage(errors, "ambient"); + + // since the above test will break at //ambient, it doesn't test the other + // invalid cases. Below tests these other invalid cases + + // less than 3 values + std::string testSDF = R"( + + + + + + + 0 0.1 + + + + + )"; + + errors.clear(); + errors = root.LoadSdfString(testSDF); + std::cout << errors << std::endl; + + ASSERT_FALSE(errors.empty()); + ExpectInvalidWithMessage(errors, "specular"); + + // negative value + testSDF = R"( + + + + + + + 0.1 0.2 -1 + + + + + )"; + + errors.clear(); + errors = root.LoadSdfString(testSDF); + std::cout << errors << std::endl; + + ASSERT_FALSE(errors.empty()); + ExpectInvalidWithMessage(errors, "emissive"); + + // more than 4 values + testSDF = R"( + + + + + + + 0.1 0.2 0.3 0.4 0.5 + + + + + )"; + + errors.clear(); + errors = root.LoadSdfString(testSDF); + std::cout << errors << std::endl; + + ASSERT_FALSE(errors.empty()); + ExpectInvalidWithMessage(errors, "diffuse"); + + // invalid string value + testSDF = R"( + + + + + + + 0.1 0.2 test + + + + + )"; + + errors.clear(); + errors = root.LoadSdfString(testSDF); + std::cout << errors << std::endl; + + ASSERT_FALSE(errors.empty()); + ExpectInvalidWithMessage(errors, "ambient"); +} + +////////////////////////////////////////////////// +TEST(Material, ValidColors) +{ + std::string testFile = + sdf::testing::TestFile("sdf", "material_valid.sdf"); + + sdf::Root root; + sdf::Errors errors = root.Load(testFile); + std::cout << errors << std::endl; + + ASSERT_TRUE(errors.empty()); + + sdf::ElementPtr elem = root.Element()->GetElement("model") + ->GetElement("link") + ->GetElement("visual") + ->GetElement("material"); + ASSERT_NE(elem, nullptr); + + EXPECT_EQ(elem->Get("diffuse"), + ignition::math::Color(0, 0.1f, 0.2f, 1)); + EXPECT_EQ(elem->Get("specular"), + ignition::math::Color(0, 0.1f, 0.2f, 0.3f)); + EXPECT_EQ(elem->Get("emissive"), + ignition::math::Color(0.12f, 0.23f, 0.34f, 0.56f)); + EXPECT_EQ(elem->Get("ambient"), + ignition::math::Color(0, 0, 0, 1)); +} diff --git a/test/sdf/material_invalid.sdf b/test/sdf/material_invalid.sdf new file mode 100644 index 000000000..b81fabfe7 --- /dev/null +++ b/test/sdf/material_invalid.sdf @@ -0,0 +1,15 @@ + + + + + + + 1.5 0 0 1 + 0 0.1 + 0.1 0.2 -1 + 0.1 0.2 0.3 0.4 0.5 + + + + + diff --git a/test/sdf/material_valid.sdf b/test/sdf/material_valid.sdf new file mode 100644 index 000000000..4f55c3603 --- /dev/null +++ b/test/sdf/material_valid.sdf @@ -0,0 +1,14 @@ + + + + + + + 0 0.1 0.2 + 0 0.1 0.2 0.3 + 0.12 0.23 0.34 0.56 + + + + + From dbf761434d51f8bc7bdc514a99cfc46439b304e3 Mon Sep 17 00:00:00 2001 From: Jenn Nguyen Date: Mon, 22 Mar 2021 18:16:01 -0700 Subject: [PATCH 4/4] specified exceptions Signed-off-by: Jenn Nguyen --- src/Param.cc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Param.cc b/src/Param.cc index 190781b42..4335fa890 100644 --- a/src/Param.cc +++ b/src/Param.cc @@ -384,9 +384,19 @@ bool ParseColorUsingStringStream(const std::string &_input, c = std::stof(token); colors.push_back(c); } - catch(const std::exception &/*e*/) + // Catch invalid argument exception from std::stof + catch(std::invalid_argument &) { - sdferr << "Error converting color value [" << token << "] to a float\n"; + sdferr << "Invalid argument. Unable to set value ["<< token + << "] for key [" << _key << "].\n"; + isValidColor = false; + break; + } + // Catch out of range exception from std::stof + catch(std::out_of_range &) + { + sdferr << "Out of range. Unable to set value [" << token + << "] for key [" << _key << "].\n"; isValidColor = false; break; }