From 8fca2f1409fca6e4867178346590b1c628745315 Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Fri, 9 Jul 2021 14:37:16 -0500 Subject: [PATCH 01/10] Add Element::FindElement as an alternative to Element::GetElement (#620) `Element::GetElement` may create a child element if it doesn't exist. This makes it a non-const function. Instead of changing this behavior, a new function `Element::FindElement` is added that returns a nullptr if the child element is not found instead of creating a new element. Signed-off-by: Addisu Z. Taddese --- Migration.md | 7 +++ include/sdf/Element.hh | 29 ++++++++++- src/Element.cc | 12 +++++ src/Element_TEST.cc | 110 +++++++++++++++++++++++++++++++++++------ 4 files changed, 141 insertions(+), 17 deletions(-) diff --git a/Migration.md b/Migration.md index 29a828bb7..f28003154 100644 --- a/Migration.md +++ b/Migration.md @@ -12,6 +12,13 @@ forward programmatically. This document aims to contain similar information to those files but with improved human-readability.. +## libsdformat 9.3 to 9.5 + +### Additions + +1. **sdf/Element.hh** + + sdf::ElementPtr FindElement() const + ## libsdformat 9.3 to 9.4 ### Modifications diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index 27f3cc239..4850cd289 100644 --- a/include/sdf/Element.hh +++ b/include/sdf/Element.hh @@ -52,6 +52,10 @@ namespace sdf /// \brief Shared pointer to an SDF Element typedef std::shared_ptr ElementPtr; + /// \def ElementConstPtr + /// \brief Shared pointer to a const SDF Element + typedef std::shared_ptr ElementConstPtr; + /// \def ElementWeakPtr /// \brief Weak pointer to an SDF Element typedef std::weak_ptr ElementWeakPtr; @@ -359,7 +363,8 @@ namespace sdf /// \brief Return a pointer to the child element with the provided name. /// /// A new child element, with the provided name, is added to this element - /// if there is no existing child element. + /// if there is no existing child element. If this is not desired see \ref + /// FindElement /// \remarks If there are multiple elements with the given tag, it returns /// the first one. /// \param[in] _name Name of the child element to retreive. @@ -367,6 +372,28 @@ namespace sdf /// element if an existing child element did not exist. public: ElementPtr GetElement(const std::string &_name); + /// \brief Return a pointer to the child element with the provided name. + /// + /// Unlike \ref GetElement, this does not create a new child element if it + /// fails to find an existing element. + /// \remarks If there are multiple elements with the given tag, it returns + /// the first one. + /// \param[in] _name Name of the child element to retreive. + /// \return Pointer to the existing child element, or nullptr + /// if the child element was not found. + public: ElementPtr FindElement(const std::string &_name); + + /// \brief Return a pointer to the child element with the provided name. + /// + /// Unlike \ref GetElement, this does not create a new child element if it + /// fails to find an existing element. + /// \remarks If there are multiple elements with the given tag, it returns + /// the first one. + /// \param[in] _name Name of the child element to retreive. + /// \return Pointer to the existing child element, or nullptr + /// if the child element was not found. + public: ElementConstPtr FindElement(const std::string &_name) const; + /// \brief Add a named element. /// \param[in] _name the name of the element to add. /// \return A pointer to the newly created Element object. diff --git a/src/Element.cc b/src/Element.cc index 4f3326215..852be9e24 100644 --- a/src/Element.cc +++ b/src/Element.cc @@ -827,6 +827,18 @@ ElementPtr Element::GetElement(const std::string &_name) return result; } +///////////////////////////////////////////////// +ElementPtr Element::FindElement(const std::string &_name) +{ + return this->GetElementImpl(_name); +} + +///////////////////////////////////////////////// +ElementConstPtr Element::FindElement(const std::string &_name) const +{ + return this->GetElementImpl(_name); +} + ///////////////////////////////////////////////// void Element::InsertElement(ElementPtr _elem) { diff --git a/src/Element_TEST.cc b/src/Element_TEST.cc index 046843004..04bb9591c 100644 --- a/src/Element_TEST.cc +++ b/src/Element_TEST.cc @@ -825,6 +825,25 @@ TEST(Element, GetNextElementMultiple) ASSERT_EQ(child2->GetNextElement(""), nullptr); } +///////////////////////////////////////////////// +/// Helper function to add child elements without having to create descriptions +sdf::ElementPtr addChildElement(sdf::ElementPtr _parent, + const std::string &_elementName, + const bool _addNameAttribute, + const std::string &_childName) +{ + sdf::ElementPtr child = std::make_shared(); + child->SetParent(_parent); + child->SetName(_elementName); + _parent->InsertElement(child); + + if (_addNameAttribute) + { + child->AddAttribute("name", "string", _childName, false, "description"); + } + return child; +} + ///////////////////////////////////////////////// TEST(Element, CountNamedElements) { @@ -838,22 +857,6 @@ TEST(Element, CountNamedElements) EXPECT_TRUE(parent->HasUniqueChildNames()); EXPECT_TRUE(parent->HasUniqueChildNames("child")); - auto addChildElement = []( - sdf::ElementPtr _parent, - const std::string &_elementName, - const bool _addNameAttribute, - const std::string &_childName) - { - sdf::ElementPtr child = std::make_shared(); - child->SetParent(_parent); - child->SetName(_elementName); - _parent->InsertElement(child); - - if (_addNameAttribute) - { - child->AddAttribute("name", "string", _childName, false, "description"); - } - }; // The following calls should make the following child elements: // @@ -914,6 +917,81 @@ TEST(Element, CountNamedElements) EXPECT_EQ(allMap.at("child3"), 1u); } +TEST(Element, FindElement) +{ + // + // + // + // + // + // + // + // + // + // + sdf::ElementPtr root = std::make_shared(); + root->SetName("root"); + + // Create elements + { + auto elemA = addChildElement(root, "elem_A", false, ""); + addChildElement(elemA, "child_elem_A", false, ""); + + auto elemB = addChildElement(root, "elem_B", false, ""); + auto firstChildElemB = + addChildElement(elemB, "child_elem_B", true, "first_child"); + + addChildElement(elemB, "child_elem_B", false, ""); + } + + { + auto elemA = root->FindElement("elem_A"); + ASSERT_NE(nullptr, elemA); + EXPECT_NE(nullptr, elemA->FindElement("child_elem_A")); + EXPECT_EQ(nullptr, elemA->FindElement("non_existent_elem")); + + auto elemB = root->FindElement("elem_B"); + ASSERT_NE(nullptr, elemB); + // This should find the first "child_elem_B" element, which has the name + // attribute + auto childElemB = elemB->FindElement("child_elem_B"); + ASSERT_TRUE(childElemB->HasAttribute("name")); + EXPECT_EQ("first_child", childElemB->GetAttribute("name")->GetAsString()); + } + + // Check that it works with const pointers + { + sdf::ElementConstPtr rootConst = root; + sdf::ElementConstPtr elemA = rootConst->FindElement("elem_A"); + ASSERT_NE(nullptr, elemA); + EXPECT_NE(nullptr, elemA->FindElement("child_elem_A")); + EXPECT_EQ(nullptr, elemA->FindElement("non_existent_elem")); + + sdf::ElementConstPtr elemB = root->FindElement("elem_B"); + ASSERT_NE(nullptr, elemB); + // This should find the first "child_elem_B" element, which has the name + // attribute + sdf::ElementConstPtr childElemB = elemB->FindElement("child_elem_B"); + ASSERT_TRUE(childElemB->HasAttribute("name")); + EXPECT_EQ("first_child", childElemB->GetAttribute("name")->GetAsString()); + } + { + sdf::ElementConstPtr rootConst = root; + sdf::ElementConstPtr elemA = rootConst->FindElement("elem_A"); + ASSERT_NE(nullptr, elemA); + EXPECT_NE(nullptr, elemA->FindElement("child_elem_A")); + EXPECT_EQ(nullptr, elemA->FindElement("non_existent_elem")); + + sdf::ElementConstPtr elemB = root->FindElement("elem_B"); + ASSERT_NE(nullptr, elemB); + // This should find the first "child_elem_B" element, which has the name + // attribute + sdf::ElementConstPtr childElemB = elemB->FindElement("child_elem_B"); + ASSERT_TRUE(childElemB->HasAttribute("name")); + EXPECT_EQ("first_child", childElemB->GetAttribute("name")->GetAsString()); + } +} + ///////////////////////////////////////////////// /// Main int main(int argc, char **argv) From 8a5cd487f3bc966289d405f8f80096ec8a27cf66 Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Tue, 13 Jul 2021 17:16:17 -0500 Subject: [PATCH 02/10] Update build system to allow overriding CXX flags and using clang on Linux (#621) This makes it impossible to pass additional flags at build time, such as the ones needed for building with ASAN Signed-off-by: Addisu Z. Taddese --- CMakeLists.txt | 1 - cmake/DefaultCFlags.cmake | 12 +++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 25044fd50..7bc1796bd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -161,7 +161,6 @@ endif() ##################################### # Handle CFlags unset (CMAKE_C_FLAGS_ALL CACHE) -unset (CMAKE_CXX_FLAGS CACHE) # USE_HOST_CFLAGS (default TRUE) # Will check building host machine for proper cflags diff --git a/cmake/DefaultCFlags.cmake b/cmake/DefaultCFlags.cmake index 610116d9f..be57d0a3d 100644 --- a/cmake/DefaultCFlags.cmake +++ b/cmake/DefaultCFlags.cmake @@ -40,12 +40,12 @@ endif() ##################################### # Set all the global build flags -set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") -set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") +set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") +set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") set (CMAKE_CXX_EXTENSIONS off) -set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_LINK_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") -set (CMAKE_SHARED_LINKER_FLAGS "${CMAKE_LINK_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") -set (CMAKE_MODULE_LINKER_FLAGS "${CMAKE_LINK_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") +set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${CMAKE_LINK_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") +set (CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${CMAKE_LINK_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") +set (CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${CMAKE_LINK_FLAGS_${CMAKE_BUILD_TYPE_UPPERCASE}}") set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) @@ -67,7 +67,9 @@ if ("${CMAKE_CXX_COMPILER_ID} " MATCHES "GNU ") message(FATAL_ERROR "${PROJECT_NAME} requires g++ 7.0 or greater.") endif () elseif ("${CMAKE_CXX_COMPILER_ID} " MATCHES "Clang ") + if(APPLE) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++") + endif() elseif ("${CMAKE_CXX_COMPILER_ID} " STREQUAL "MSVC ") if (MSVC_VERSION LESS 1914) message(FATAL_ERROR "${PROJECT_NAME} requires VS 2017 or greater.") From 66a8c2157ebff8f2a831a60445823667297781e0 Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Wed, 14 Jul 2021 12:48:12 -0700 Subject: [PATCH 03/10] Error: move << operator from .hh to .cc file (#625) * Error: move << operator from .hh to .cc file Signed-off-by: Steven Peters * Fix visibility Signed-off-by: Addisu Z. Taddese Co-authored-by: Addisu Z. Taddese --- include/sdf/Error.hh | 10 ++-------- src/Error.cc | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/include/sdf/Error.hh b/include/sdf/Error.hh index 9ae088e9c..3c059563e 100644 --- a/include/sdf/Error.hh +++ b/include/sdf/Error.hh @@ -171,14 +171,8 @@ namespace sdf /// \param[in,out] _out The output stream. /// \param[in] _err The error to output. /// \return Reference to the given output stream - public: friend std::ostream &operator<<(std::ostream &_out, - const sdf::Error &_err) - { - _out << "Error Code " - << static_cast::type>(_err.Code()) - << " Msg: " << _err.Message(); - return _out; - } + public: friend SDFORMAT_VISIBLE std::ostream &operator<<( + std::ostream &_out, const sdf::Error &_err); /// \brief The error code value. private: ErrorCode code = ErrorCode::NONE; diff --git a/src/Error.cc b/src/Error.cc index 4a262e833..1793ff238 100644 --- a/src/Error.cc +++ b/src/Error.cc @@ -51,3 +51,20 @@ bool Error::operator==(const bool _value) const return ((this->code != ErrorCode::NONE) && _value) || ((this->code == ErrorCode::NONE) && !_value); } + +namespace sdf +{ +// Inline bracket to help doxygen filtering. +inline namespace SDF_VERSION_NAMESPACE { + +///////////////////////////////////////////////// +// cppcheck-suppress unusedFunction +std::ostream &operator<<(std::ostream &_out, const sdf::Error &_err) +{ + _out << "Error Code " + << static_cast::type>(_err.Code()) + << " Msg: " << _err.Message(); + return _out; +} +} +} From 3a8d345567f8595a4344bd1af91ad6c760ea9d32 Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Thu, 22 Jul 2021 15:25:15 -0700 Subject: [PATCH 04/10] Minor fix to Migration guide (#630) Signed-off-by: Steven Peters --- Migration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Migration.md b/Migration.md index f28003154..e5bdd1076 100644 --- a/Migration.md +++ b/Migration.md @@ -12,7 +12,7 @@ forward programmatically. This document aims to contain similar information to those files but with improved human-readability.. -## libsdformat 9.3 to 9.5 +## libsdformat 9.4 to 9.5 ### Additions From e4c658f14b73ab839d730c7a402a82a7dfa6a16e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Wallk=C3=B6tter?= Date: Fri, 23 Jul 2021 20:37:37 +0200 Subject: [PATCH 05/10] BUG: add missing sdf files to CMakeLists (#631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * BUG: add missing sdf files to CMakeLists Closes: https://github.com/ignitionrobotics/sdformat/issues/629 Signed-off-by: Sebastian Wallkötter Signed-off-by: FirefoxMetzger --- sdf/1.6/CMakeLists.txt | 3 +++ sdf/1.7/CMakeLists.txt | 3 +++ sdf/1.8/CMakeLists.txt | 3 +++ 3 files changed, 9 insertions(+) diff --git a/sdf/1.6/CMakeLists.txt b/sdf/1.6/CMakeLists.txt index c77c738ec..fe972e4c5 100644 --- a/sdf/1.6/CMakeLists.txt +++ b/sdf/1.6/CMakeLists.txt @@ -1,5 +1,6 @@ set (sdfs actor.sdf + air_pressure.sdf altimeter.sdf atmosphere.sdf audio_source.sdf @@ -21,6 +22,7 @@ set (sdfs imu.sdf inertial.sdf joint.sdf + lidar.sdf light.sdf light_state.sdf link.sdf @@ -32,6 +34,7 @@ set (sdfs model.sdf model_state.sdf noise.sdf + particle_emitter.sdf physics.sdf plane_shape.sdf plugin.sdf diff --git a/sdf/1.7/CMakeLists.txt b/sdf/1.7/CMakeLists.txt index c170977c9..516a7874b 100644 --- a/sdf/1.7/CMakeLists.txt +++ b/sdf/1.7/CMakeLists.txt @@ -1,5 +1,6 @@ set (sdfs actor.sdf + air_pressure.sdf altimeter.sdf atmosphere.sdf audio_source.sdf @@ -21,6 +22,7 @@ set (sdfs imu.sdf inertial.sdf joint.sdf + lidar.sdf light.sdf light_state.sdf link.sdf @@ -32,6 +34,7 @@ set (sdfs model.sdf model_state.sdf noise.sdf + particle_emitter.sdf physics.sdf plane_shape.sdf plugin.sdf diff --git a/sdf/1.8/CMakeLists.txt b/sdf/1.8/CMakeLists.txt index a1a70717b..d074806f5 100644 --- a/sdf/1.8/CMakeLists.txt +++ b/sdf/1.8/CMakeLists.txt @@ -1,5 +1,6 @@ set (sdfs actor.sdf + air_pressure.sdf altimeter.sdf atmosphere.sdf audio_source.sdf @@ -23,6 +24,7 @@ set (sdfs imu.sdf inertial.sdf joint.sdf + lidar.sdf light.sdf light_state.sdf link.sdf @@ -34,6 +36,7 @@ set (sdfs model.sdf model_state.sdf noise.sdf + particle_emitter.sdf physics.sdf plane_shape.sdf plugin.sdf From 99d025049ff6ec04812d2b1d1e06506973b57d38 Mon Sep 17 00:00:00 2001 From: Jenn Nguyen Date: Wed, 28 Jul 2021 17:51:44 -0700 Subject: [PATCH 06/10] Updated material spec (#644) Signed-off-by: Jenn Nguyen --- sdf/1.6/material.sdf | 4 ++++ sdf/1.7/material.sdf | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/sdf/1.6/material.sdf b/sdf/1.6/material.sdf index 7cd011598..6c2696a1a 100644 --- a/sdf/1.6/material.sdf +++ b/sdf/1.6/material.sdf @@ -111,6 +111,10 @@ Material glossiness in the range of [0-1], where 0 represents a rough surface and 1 represents a smooth surface. This is the inverse of a roughness map in a PBR metal workflow. + + Filename of the environment / reflection map, typically in the form of a cubemap + + Filename of the ambient occlusion map. The map defines the amount of ambient lighting on the surface. diff --git a/sdf/1.7/material.sdf b/sdf/1.7/material.sdf index 00ed20924..4b222f61c 100644 --- a/sdf/1.7/material.sdf +++ b/sdf/1.7/material.sdf @@ -116,6 +116,10 @@ Material glossiness in the range of [0-1], where 0 represents a rough surface and 1 represents a smooth surface. This is the inverse of a roughness map in a PBR metal workflow. + + Filename of the environment / reflection map, typically in the form of a cubemap + + Filename of the ambient occlusion map. The map defines the amount of ambient lighting on the surface. From 4ca726fa655750c5507c5ca4cc412786c7bb2afb Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Wed, 4 Aug 2021 23:51:14 +0800 Subject: [PATCH 07/10] Fix unreported invalid model when reference frame is unavailable (#636) Introduced a step before calling readXml in readDoc, called checkXml, which allows us to check the XML statically for any undesired behavior. This should ideally allow us to add on more rules and restrictions on the XML files moving forwards. Signed-off-by: Aaron Chong --- src/ign_TEST.cc | 11 ++++ src/parser.cc | 69 +++++++++++++++++++++- src/parser_private.hh | 10 ++++ test/integration/frame.cc | 26 +++++--- test/sdf/model_invalid_top_level_frame.sdf | 12 ++++ 5 files changed, 118 insertions(+), 10 deletions(-) create mode 100644 test/sdf/model_invalid_top_level_frame.sdf diff --git a/src/ign_TEST.cc b/src/ign_TEST.cc index d221051aa..9757a307c 100644 --- a/src/ign_TEST.cc +++ b/src/ign_TEST.cc @@ -810,6 +810,17 @@ TEST(check, IGN_UTILS_TEST_DISABLED_ON_WIN32(SDF)) custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); EXPECT_EQ("Valid.\n", output) << output; } + // Check an SDF with an invalid relative frame at the top level model + { + std::string path = pathBase + "/model_invalid_top_level_frame.sdf"; + + std::string output = + custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + EXPECT_NE( + output.find("Attribute //pose[@relative_to] of top level model must be " + "left empty, found //pose[@relative_to='some_frame']."), + std::string::npos) << output; + } } ///////////////////////////////////////////////// diff --git a/src/parser.cc b/src/parser.cc index 9e0e9f6d6..8501d8d27 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -703,8 +704,18 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, SDFPtr _sdf, Converter::Convert(_xmlDoc, SDF::Version()); } - // parse new sdf xml auto *elemXml = _xmlDoc->FirstChildElement(_sdf->Root()->GetName().c_str()); + + // Perform all the pre-checks necessary for the XML elements before reading + if (!checkXmlFromRoot(elemXml, _source, _errors)) + { + _errors.push_back({ErrorCode::ELEMENT_INVALID, + "Errors were found when checking the XML of element<" + + _sdf->Root()->GetName() + ">."}); + return false; + } + + // parse new sdf xml if (!readXml(elemXml, _sdf->Root(), _config, _source, _errors)) { _errors.push_back({ErrorCode::ELEMENT_INVALID, @@ -796,6 +807,15 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, ElementPtr _sdf, elemXml = sdfNode->FirstChildElement(_sdf->GetName().c_str()); } + // Perform all the pre-checks necessary for the XML elements before reading + if (!checkXmlFromRoot(elemXml, _source, _errors)) + { + _errors.push_back({ErrorCode::ELEMENT_INVALID, + "Errors were found when checking the XML of element[" + + _sdf->GetName() + "]."}); + return false; + } + // parse new sdf xml if (!readXml(elemXml, _sdf, _config, _source, _errors)) { @@ -832,6 +852,53 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, ElementPtr _sdf, return true; } +////////////////////////////////////////////////// +bool checkXmlFromRoot(tinyxml2::XMLElement *_xmlRoot, + const std::string &_source, Errors &_errors) +{ + // A null XML Root element is still valid as it might not be a mandatory + // element. Further errors will be deciphered by calling readXml with its + // SDF ptr. + if (!_xmlRoot) + return true; + + std::string errorSourcePath = _source; + if (_source == kSdfStringSource || _source == kUrdfStringSource) + errorSourcePath = "<" + _source + ">"; + + // Top level models must have an empty relative_to frame on the top level + // pose. + { + if (tinyxml2::XMLElement *topLevelElem = + _xmlRoot->FirstChildElement("model")) + { + if (tinyxml2::XMLElement *topLevelPose = + topLevelElem->FirstChildElement("pose")) + { + if (const char *relativeTo = topLevelPose->Attribute("relative_to")) + { + const std::string relativeToStr(relativeTo); + if (!relativeToStr.empty()) + { + std::stringstream sstream; + sstream << "Attribute //pose[@relative_to] of top level model " + << "must be left empty, found //pose[@relative_to='" + << relativeToStr << "'].\n"; + _errors.push_back({ + ErrorCode::ATTRIBUTE_INVALID, + sstream.str(), + errorSourcePath, + topLevelPose->GetLineNum()}); + return false; + } + } + } + } + } + + return true; +} + ////////////////////////////////////////////////// std::string getBestSupportedModelVersion(tinyxml2::XMLElement *_modelXML, std::string &_modelFileName) diff --git a/src/parser_private.hh b/src/parser_private.hh index da2c5a99b..c1b8d963f 100644 --- a/src/parser_private.hh +++ b/src/parser_private.hh @@ -76,6 +76,16 @@ namespace sdf const std::string &_source, bool _convert, const ParserConfig &_config, Errors &_errors); + /// \brief Perform a series of checks to determine the validity of this XML + /// document from the root level element in the context of an SDF Element. + /// This is called before readXml. + /// \remark For internal use only. Do not use this function. + /// \param[in] _xmlRoot Pointer to the root level TinyXML element. + /// \param[in] _source Source of the XML document. + /// \param[in] _errors Captures errors found during the checks. + bool checkXmlFromRoot(tinyxml2::XMLElement *_xmlRoot, + const std::string &_source, Errors &_errors); + /// \brief Populate an SDF Element from the XML input. The XML input here is /// an actual SDFormat file or string, not the description of the SDFormat /// spec. diff --git a/test/integration/frame.cc b/test/integration/frame.cc index c5ae76113..a1fd535a5 100644 --- a/test/integration/frame.cc +++ b/test/integration/frame.cc @@ -42,13 +42,15 @@ TEST(Frame, ModelFrame) std::string version = SDF_VERSION; stream << "" - << "" - << " " - << " 1 1 0 0 0 0" - << " " - << " 1 0 0 0 0 0" - << " " - << "" + << "" + << " " + << " " + << " 1 1 0 0 0 0" + << " " + << " 1 0 0 0 0 0" + << " " + << " " + << "" << ""; sdf::SDFPtr sdfParsed(new sdf::SDF()); @@ -57,9 +59,15 @@ TEST(Frame, ModelFrame) // Verify correct parsing + // world + ASSERT_TRUE(sdfParsed->Root()->HasElement("world")); + sdf::ElementPtr worldElem = sdfParsed->Root()->GetElement("world"); + ASSERT_NE(worldElem, nullptr); + // model - EXPECT_TRUE(sdfParsed->Root()->HasElement("model")); - sdf::ElementPtr modelElem = sdfParsed->Root()->GetElement("model"); + ASSERT_TRUE(worldElem->HasElement("model")); + sdf::ElementPtr modelElem = worldElem->GetElement("model"); + ASSERT_TRUE(modelElem); EXPECT_TRUE(modelElem->HasAttribute("name")); EXPECT_EQ(modelElem->Get("name"), "my_model"); diff --git a/test/sdf/model_invalid_top_level_frame.sdf b/test/sdf/model_invalid_top_level_frame.sdf new file mode 100644 index 000000000..a53c19d4c --- /dev/null +++ b/test/sdf/model_invalid_top_level_frame.sdf @@ -0,0 +1,12 @@ + + + + + + + + From 57300dfd8890559069f081fc1e8b010de4abf402 Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Wed, 4 Aug 2021 13:14:58 -0700 Subject: [PATCH 08/10] Fix urdf link extension tags (#628) * parser_urdf: no empty velocity_decay elements This fixes the parser_urdf code to only add elements into links if a parameter is specified in a tag. Currently there there can be multiple empty tags added to a link. * parser_urdf: only add to links if set This fixes the parser_urdf code to only add elements to links if the parameter has been specified in a tag. * Add test for link extension tags Signed-off-by: Steven Peters --- src/SDFExtension.cc | 2 ++ src/SDFExtension.hh | 1 + src/parser_urdf.cc | 13 +++---- test/integration/fixed_joint_reduction.cc | 4 +++ ..._reduction_collision_visual_empty_root.sdf | 2 -- ...t_reduction_collision_visual_extension.sdf | 4 --- test/integration/urdf_gazebo_extensions.cc | 36 +++++++++++++++++++ test/integration/urdf_gazebo_extensions.urdf | 13 +++++++ 8 files changed, 61 insertions(+), 14 deletions(-) diff --git a/src/SDFExtension.cc b/src/SDFExtension.cc index d2d7d9d44..cd72f41fb 100644 --- a/src/SDFExtension.cc +++ b/src/SDFExtension.cc @@ -26,6 +26,7 @@ SDFExtension::SDFExtension() this->visual_blobs.clear(); this->collision_blobs.clear(); this->setStaticFlag = false; + this->isGravity = false; this->gravity = true; this->isDampingFactor = false; this->isMaxContacts = false; @@ -74,6 +75,7 @@ SDFExtension::SDFExtension(const SDFExtension &_ge) this->visual_blobs = _ge.visual_blobs; this->collision_blobs = _ge.collision_blobs; this->setStaticFlag = _ge.setStaticFlag; + this->isGravity = _ge.isGravity; this->gravity = _ge.gravity; this->isDampingFactor = _ge.isDampingFactor; this->isMaxContacts = _ge.isMaxContacts; diff --git a/src/SDFExtension.hh b/src/SDFExtension.hh index a8c3d6120..b48d104f0 100644 --- a/src/SDFExtension.hh +++ b/src/SDFExtension.hh @@ -88,6 +88,7 @@ namespace sdf // body, default off public: bool setStaticFlag; + public: bool isGravity; public: bool gravity; public: bool isDampingFactor; public: double dampingFactor; diff --git a/src/parser_urdf.cc b/src/parser_urdf.cc index 85ddafa9d..1dd1f2d62 100644 --- a/src/parser_urdf.cc +++ b/src/parser_urdf.cc @@ -1343,6 +1343,7 @@ void URDF2SDF::ParseSDFExtension(TiXmlDocument &_urdfXml) else if (childElem->ValueStr() == "turnGravityOff") { std::string valueStr = GetKeyValueAsString(childElem); + sdf->isGravity = true; // default of gravity is true if (lowerStr(valueStr) == "false" || lowerStr(valueStr) == "no" || @@ -2069,26 +2070,22 @@ void InsertSDFExtensionLink(TiXmlElement *_elem, const std::string &_linkName) sdfIt->second.begin(); ge != sdfIt->second.end(); ++ge) { // insert gravity - if ((*ge)->gravity) + if ((*ge)->isGravity) { - AddKeyValue(_elem, "gravity", "true"); - } - else - { - AddKeyValue(_elem, "gravity", "false"); + AddKeyValue(_elem, "gravity", (*ge)->gravity ? "true" : "false"); } // damping factor - TiXmlElement *velocityDecay = new TiXmlElement("velocity_decay"); if ((*ge)->isDampingFactor) { + TiXmlElement *velocityDecay = new TiXmlElement("velocity_decay"); /// @todo separate linear and angular velocity decay AddKeyValue(velocityDecay, "linear", Values2str(1, &(*ge)->dampingFactor)); AddKeyValue(velocityDecay, "angular", Values2str(1, &(*ge)->dampingFactor)); + _elem->LinkEndChild(velocityDecay); } - _elem->LinkEndChild(velocityDecay); // selfCollide tag if ((*ge)->isSelfCollide) { diff --git a/test/integration/fixed_joint_reduction.cc b/test/integration/fixed_joint_reduction.cc index 33323ea39..5fe3cd7a8 100644 --- a/test/integration/fixed_joint_reduction.cc +++ b/test/integration/fixed_joint_reduction.cc @@ -134,7 +134,9 @@ void FixedJointReductionCollisionVisualExtension(const std::string &_urdfFile, for (sdf::ElementPtr link = urdfModel->GetElement("link"); link; link = link->GetNextElement("link")) { + EXPECT_FALSE(link->HasElement("gravity")); EXPECT_FALSE(link->HasElement("self_collide")); + EXPECT_FALSE(link->HasElement("velocity_decay")); for (sdf::ElementPtr col = link->GetElement("collision"); col; col = col->GetNextElement("collision")) { @@ -184,7 +186,9 @@ void FixedJointReductionCollisionVisualExtension(const std::string &_urdfFile, for (sdf::ElementPtr link = sdfModel->GetElement("link"); link; link = link->GetNextElement("link")) { + EXPECT_FALSE(link->HasElement("gravity")); EXPECT_FALSE(link->HasElement("self_collide")); + EXPECT_FALSE(link->HasElement("velocity_decay")); for (sdf::ElementPtr col = link->GetElement("collision"); col; col = col->GetNextElement("collision")) { diff --git a/test/integration/fixed_joint_reduction_collision_visual_empty_root.sdf b/test/integration/fixed_joint_reduction_collision_visual_empty_root.sdf index 192134688..001d07ac3 100644 --- a/test/integration/fixed_joint_reduction_collision_visual_empty_root.sdf +++ b/test/integration/fixed_joint_reduction_collision_visual_empty_root.sdf @@ -51,8 +51,6 @@ - 1 - 0 diff --git a/test/integration/fixed_joint_reduction_collision_visual_extension.sdf b/test/integration/fixed_joint_reduction_collision_visual_extension.sdf index 37d798beb..2b1639373 100644 --- a/test/integration/fixed_joint_reduction_collision_visual_extension.sdf +++ b/test/integration/fixed_joint_reduction_collision_visual_extension.sdf @@ -134,10 +134,6 @@ - - - 1 - diff --git a/test/integration/urdf_gazebo_extensions.cc b/test/integration/urdf_gazebo_extensions.cc index 7e4d9ff82..6f2298dfb 100644 --- a/test/integration/urdf_gazebo_extensions.cc +++ b/test/integration/urdf_gazebo_extensions.cc @@ -100,4 +100,40 @@ TEST(SDFParser, UrdfGazeboExtensionURDFTest) EXPECT_TRUE(physics->Get("implicit_spring_damper")); } } + + for (sdf::ElementPtr link = model->GetElement("link"); link; + link = link->GetNextElement("link")) + { + std::string linkName = link->Get("name"); + if (linkName == "link1" || linkName == "link2") + { + EXPECT_TRUE(link->HasElement("enable_wind")); + + EXPECT_TRUE(link->HasElement("gravity")); + EXPECT_FALSE(link->Get("gravity")); + + EXPECT_TRUE(link->HasElement("velocity_decay")); + auto velocityDecay = link->GetElement("velocity_decay"); + EXPECT_DOUBLE_EQ(0.1, velocityDecay->Get("linear")); + + if (linkName == "link1") + { + EXPECT_TRUE(link->Get("enable_wind")); + EXPECT_DOUBLE_EQ(0.2, velocityDecay->Get("angular")); + } + else + { + // linkName == "link2" + EXPECT_FALSE(link->Get("enable_wind")); + EXPECT_DOUBLE_EQ(0.1, velocityDecay->Get("angular")); + } + } + else + { + // No gazebo tags added + EXPECT_FALSE(link->HasElement("enable_wind")); + EXPECT_FALSE(link->HasElement("gravity")); + EXPECT_FALSE(link->HasElement("velocity_decay")); + } + } } diff --git a/test/integration/urdf_gazebo_extensions.urdf b/test/integration/urdf_gazebo_extensions.urdf index 4703907db..fc7216a16 100644 --- a/test/integration/urdf_gazebo_extensions.urdf +++ b/test/integration/urdf_gazebo_extensions.urdf @@ -56,6 +56,14 @@ + + 1 + 0 + + 0.1 + 0.2 + + @@ -81,6 +89,11 @@ + + 0 + 1 + 0.1 + From 89be29bda65f706906add253ddcdecfddf572c99 Mon Sep 17 00:00:00 2001 From: Ian Chen Date: Mon, 14 Dec 2020 22:30:01 -0800 Subject: [PATCH 09/10] Add lightmap to 1.7 spec and PBR material DOM (#429) Signed-off-by: Ian Chen Co-authored-by: Steve Peters --- include/sdf/Pbr.hh | 15 +++++++++++ sdf/1.7/material.sdf | 15 +++++++++++ src/Pbr.cc | 34 +++++++++++++++++++++++++ src/Pbr_TEST.cc | 34 +++++++++++++++++++++++++ test/integration/material_pbr.cc | 43 ++++++++++++++++++++++++++++++-- test/sdf/material_pbr.sdf | 4 +++ 6 files changed, 143 insertions(+), 2 deletions(-) diff --git a/include/sdf/Pbr.hh b/include/sdf/Pbr.hh index 564315d85..f1b5a0498 100644 --- a/include/sdf/Pbr.hh +++ b/include/sdf/Pbr.hh @@ -179,6 +179,21 @@ namespace sdf /// \param[in] _map Filename of the emissive map. public: void SetEmissiveMap(const std::string &_map); + /// \brief Get the light map filename. This will be an empty string + /// if an light map has not been set. + /// \return Filename of the light map, or empty string if a light + /// map has not been specified. + public: std::string LightMap() const; + + /// \brief Set the light map filename. + /// \param[in] _map Filename of the light map. + /// \param[in] _uvSet Index of the light map texture coordinate set + public: void SetLightMap(const std::string &_map, unsigned int _uvSet = 0u); + + /// \brief Get the light map texture coordinate set. + /// \return Index of the texture coordinate set + public: unsigned int LightMapTexCoordSet() const; + /// \brief Get the metalness value of the material for metal workflow /// \return metalness value of the material public: double Metalness() const; diff --git a/sdf/1.7/material.sdf b/sdf/1.7/material.sdf index 4b222f61c..90feccfab 100644 --- a/sdf/1.7/material.sdf +++ b/sdf/1.7/material.sdf @@ -95,6 +95,14 @@ Filename of the emissive map. + + + + Index of the texture coordinate set to use. + + Filename of the light map. The light map is a prebaked light texture that is applied over the albedo map + + @@ -135,6 +143,13 @@ Filename of the emissive map. + + + + Index of the texture coordinate set to use. + + Filename of the light map. The light map is a prebaked light texture that is applied over the albedo map + diff --git a/src/Pbr.cc b/src/Pbr.cc index dbfd06041..63fd5800c 100644 --- a/src/Pbr.cc +++ b/src/Pbr.cc @@ -55,6 +55,12 @@ class sdf::PbrWorkflowPrivate /// \brief Emissive map public: std::string emissiveMap = ""; + /// \brief Light map + public: std::string lightMapFilename; + + /// \brief Light map texture coordinate set + public: unsigned int lightMapUvSet = 0u; + /// \brief Roughness value (metal workflow only) public: double roughness = 0.5; @@ -140,6 +146,7 @@ bool PbrWorkflow::operator==(const PbrWorkflow &_workflow) const && (this->dataPtr->glossinessMap == _workflow.dataPtr->glossinessMap) && (this->dataPtr->environmentMap == _workflow.dataPtr->environmentMap) && (this->dataPtr->emissiveMap == _workflow.dataPtr->emissiveMap) + && (this->dataPtr->lightMapFilename == _workflow.dataPtr->lightMapFilename) && (this->dataPtr->ambientOcclusionMap == _workflow.dataPtr->ambientOcclusionMap) && (ignition::math::equal( @@ -212,6 +219,14 @@ Errors PbrWorkflow::Load(sdf::ElementPtr _sdf) this->dataPtr->emissiveMap = _sdf->Get("emissive_map", this->dataPtr->emissiveMap).first; + if (_sdf->HasElement("light_map")) + { + sdf::ElementPtr lightMapElem = _sdf->GetElement("light_map"); + this->dataPtr->lightMapFilename = lightMapElem->Get(); + this->dataPtr->lightMapUvSet = lightMapElem->Get("uv_set", + this->dataPtr->lightMapUvSet).first; + } + return errors; } @@ -366,6 +381,25 @@ void PbrWorkflow::SetEmissiveMap(const std::string &_map) this->dataPtr->emissiveMap = _map; } +////////////////////////////////////////////////// +std::string PbrWorkflow::LightMap() const +{ + return this->dataPtr->lightMapFilename; +} + +////////////////////////////////////////////////// +void PbrWorkflow::SetLightMap(const std::string &_map, unsigned int _uvSet) +{ + this->dataPtr->lightMapFilename = _map; + this->dataPtr->lightMapUvSet = _uvSet; +} + +////////////////////////////////////////////////// +unsigned int PbrWorkflow::LightMapTexCoordSet() const +{ + return this->dataPtr->lightMapUvSet; +} + ////////////////////////////////////////////////// sdf::ElementPtr PbrWorkflow::Element() const { diff --git a/src/Pbr_TEST.cc b/src/Pbr_TEST.cc index 39d3892b4..5b4c95da8 100644 --- a/src/Pbr_TEST.cc +++ b/src/Pbr_TEST.cc @@ -35,6 +35,8 @@ TEST(DOMPbr, Construction) EXPECT_EQ(std::string(), workflow.RoughnessMap()); EXPECT_EQ(std::string(), workflow.MetalnessMap()); EXPECT_EQ(std::string(), workflow.EmissiveMap()); + EXPECT_EQ(std::string(), workflow.LightMap()); + EXPECT_EQ(0u, workflow.LightMapTexCoordSet()); EXPECT_DOUBLE_EQ(0.5, workflow.Roughness()); EXPECT_DOUBLE_EQ(0.5, workflow.Metalness()); EXPECT_EQ(std::string(), workflow.SpecularMap()); @@ -70,6 +72,7 @@ TEST(DOMPbr, MoveConstructor) workflow.SetEnvironmentMap("metal_env_map.png"); workflow.SetAmbientOcclusionMap("metal_ambient_occlusion_map.png"); workflow.SetEmissiveMap("metal_emissive_map.png"); + workflow.SetLightMap("metal_light_map.png", 1u); workflow.SetRoughnessMap("roughness_map.png"); workflow.SetMetalnessMap("metalness_map.png"); workflow.SetRoughness(0.8); @@ -84,6 +87,8 @@ TEST(DOMPbr, MoveConstructor) EXPECT_EQ("metal_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("metal_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("metal_light_map.png", workflow2.LightMap()); + EXPECT_EQ(1u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("roughness_map.png", workflow2.RoughnessMap()); EXPECT_EQ("metalness_map.png", workflow2.MetalnessMap()); EXPECT_DOUBLE_EQ(0.8, workflow2.Roughness()); @@ -104,6 +109,7 @@ TEST(DOMPbr, MoveConstructor) workflow.SetEnvironmentMap("specular_env_map.png"); workflow.SetAmbientOcclusionMap("specular_ambient_occlusion_map.png"); workflow.SetEmissiveMap("specular_emissive_map.png"); + workflow.SetLightMap("specular_light_map.png", 2u); workflow.SetGlossinessMap("glossiness_map.png"); workflow.SetSpecularMap("specular_map.png"); workflow.SetGlossiness(0.1); @@ -117,6 +123,8 @@ TEST(DOMPbr, MoveConstructor) EXPECT_EQ("specular_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("specular_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("specular_light_map.png", workflow2.LightMap()); + EXPECT_EQ(2u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("specular_map.png", workflow2.SpecularMap()); EXPECT_EQ("glossiness_map.png", workflow2.GlossinessMap()); EXPECT_DOUBLE_EQ(0.1, workflow2.Glossiness()); @@ -155,6 +163,7 @@ TEST(DOMPbr, MoveAssignmentOperator) workflow.SetEnvironmentMap("metal_env_map.png"); workflow.SetAmbientOcclusionMap("metal_ambient_occlusion_map.png"); workflow.SetEmissiveMap("metal_emissive_map.png"); + workflow.SetLightMap("metal_light_map.png", 3u); workflow.SetRoughnessMap("roughness_map.png"); workflow.SetMetalnessMap("metalness_map.png"); workflow.SetRoughness(0.8); @@ -170,6 +179,8 @@ TEST(DOMPbr, MoveAssignmentOperator) EXPECT_EQ("metal_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("metal_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("metal_light_map.png", workflow2.LightMap()); + EXPECT_EQ(3u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("roughness_map.png", workflow2.RoughnessMap()); EXPECT_EQ("metalness_map.png", workflow2.MetalnessMap()); EXPECT_DOUBLE_EQ(0.8, workflow2.Roughness()); @@ -190,6 +201,7 @@ TEST(DOMPbr, MoveAssignmentOperator) workflow.SetEnvironmentMap("specular_env_map.png"); workflow.SetAmbientOcclusionMap("specular_ambient_occlusion_map.png"); workflow.SetEmissiveMap("specular_emissive_map.png"); + workflow.SetLightMap("specular_light_map.png", 1u); workflow.SetGlossinessMap("glossiness_map.png"); workflow.SetSpecularMap("specular_map.png"); workflow.SetGlossiness(0.1); @@ -204,6 +216,8 @@ TEST(DOMPbr, MoveAssignmentOperator) EXPECT_EQ("specular_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("specular_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("specular_light_map.png", workflow2.LightMap()); + EXPECT_EQ(1u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("specular_map.png", workflow2.SpecularMap()); EXPECT_EQ("glossiness_map.png", workflow2.GlossinessMap()); EXPECT_DOUBLE_EQ(0.1, workflow2.Glossiness()); @@ -241,6 +255,7 @@ TEST(DOMPbr, CopyConstructor) workflow.SetEnvironmentMap("metal_env_map.png"); workflow.SetAmbientOcclusionMap("metal_ambient_occlusion_map.png"); workflow.SetEmissiveMap("metal_emissive_map.png"); + workflow.SetLightMap("metal_light_map.png", 2u); workflow.SetRoughnessMap("roughness_map.png"); workflow.SetMetalnessMap("metalness_map.png"); workflow.SetRoughness(0.8); @@ -255,6 +270,8 @@ TEST(DOMPbr, CopyConstructor) EXPECT_EQ("metal_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("metal_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("metal_light_map.png", workflow2.LightMap()); + EXPECT_EQ(2u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("roughness_map.png", workflow2.RoughnessMap()); EXPECT_EQ("metalness_map.png", workflow2.MetalnessMap()); EXPECT_DOUBLE_EQ(0.8, workflow2.Roughness()); @@ -274,6 +291,7 @@ TEST(DOMPbr, CopyConstructor) workflow.SetEnvironmentMap("specular_env_map.png"); workflow.SetAmbientOcclusionMap("specular_ambient_occlusion_map.png"); workflow.SetEmissiveMap("specular_emissive_map.png"); + workflow.SetLightMap("specular_light_map.png", 1u); workflow.SetGlossinessMap("glossiness_map.png"); workflow.SetSpecularMap("specular_map.png"); workflow.SetGlossiness(0.1); @@ -287,6 +305,8 @@ TEST(DOMPbr, CopyConstructor) EXPECT_EQ("specular_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("specular_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("specular_light_map.png", workflow2.LightMap()); + EXPECT_EQ(1u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("specular_map.png", workflow2.SpecularMap()); EXPECT_EQ("glossiness_map.png", workflow2.GlossinessMap()); EXPECT_DOUBLE_EQ(0.1, workflow2.Glossiness()); @@ -325,6 +345,7 @@ TEST(DOMPbr, AssignmentOperator) workflow.SetEnvironmentMap("metal_env_map.png"); workflow.SetAmbientOcclusionMap("metal_ambient_occlusion_map.png"); workflow.SetEmissiveMap("metal_emissive_map.png"); + workflow.SetLightMap("metal_light_map.png", 1u); workflow.SetRoughnessMap("roughness_map.png"); workflow.SetMetalnessMap("metalness_map.png"); workflow.SetRoughness(0.8); @@ -339,6 +360,8 @@ TEST(DOMPbr, AssignmentOperator) EXPECT_EQ("metal_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("metal_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("metal_light_map.png", workflow2.LightMap()); + EXPECT_EQ(1u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("roughness_map.png", workflow2.RoughnessMap()); EXPECT_EQ("metalness_map.png", workflow2.MetalnessMap()); EXPECT_DOUBLE_EQ(0.8, workflow2.Roughness()); @@ -358,6 +381,7 @@ TEST(DOMPbr, AssignmentOperator) workflow.SetEnvironmentMap("specular_env_map.png"); workflow.SetAmbientOcclusionMap("specular_ambient_occlusion_map.png"); workflow.SetEmissiveMap("specular_emissive_map.png"); + workflow.SetLightMap("specular_light_map.png", 2u); workflow.SetGlossinessMap("glossiness_map.png"); workflow.SetSpecularMap("specular_map.png"); workflow.SetGlossiness(0.1); @@ -370,6 +394,8 @@ TEST(DOMPbr, AssignmentOperator) EXPECT_EQ("specular_ambient_occlusion_map.png", workflow2.AmbientOcclusionMap()); EXPECT_EQ("specular_emissive_map.png", workflow2.EmissiveMap()); + EXPECT_EQ("specular_light_map.png", workflow2.LightMap()); + EXPECT_EQ(2u, workflow2.LightMapTexCoordSet()); EXPECT_EQ("specular_map.png", workflow2.SpecularMap()); EXPECT_EQ("glossiness_map.png", workflow2.GlossinessMap()); EXPECT_DOUBLE_EQ(0.1, workflow2.Glossiness()); @@ -443,6 +469,10 @@ TEST(DOMPbr, Set) workflow.SetEmissiveMap("metal_emissive_map.png"); EXPECT_EQ("metal_emissive_map.png", workflow.EmissiveMap()); + workflow.SetLightMap("metal_light_map.png", 1u); + EXPECT_EQ("metal_light_map.png", workflow.LightMap()); + EXPECT_EQ(1u, workflow.LightMapTexCoordSet()); + workflow.SetRoughnessMap("roughness_map.png"); EXPECT_EQ("roughness_map.png", workflow.RoughnessMap()); @@ -491,6 +521,10 @@ TEST(DOMPbr, Set) workflow.SetEmissiveMap("specular_emissive_map.png"); EXPECT_EQ("specular_emissive_map.png", workflow.EmissiveMap()); + workflow.SetLightMap("specular_light_map.png", 1u); + EXPECT_EQ("specular_light_map.png", workflow.LightMap()); + EXPECT_EQ(1u, workflow.LightMapTexCoordSet()); + workflow.SetGlossinessMap("glossiness_map.png"); EXPECT_EQ("glossiness_map.png", workflow.GlossinessMap()); diff --git a/test/integration/material_pbr.cc b/test/integration/material_pbr.cc index 4eb8f6b3d..1d684deea 100644 --- a/test/integration/material_pbr.cc +++ b/test/integration/material_pbr.cc @@ -97,6 +97,10 @@ TEST(Material, PbrDOM) // emissive map EXPECT_EQ("emissive_map.png", workflow->EmissiveMap()); + + // light map + EXPECT_EQ("light_map.png", workflow->LightMap()); + EXPECT_EQ(1u, workflow->LightMapTexCoordSet()); } // visual specular workflow @@ -148,6 +152,10 @@ TEST(Material, PbrDOM) // emissive map EXPECT_EQ("emissive_map.png", workflow->EmissiveMap()); + + // light map + EXPECT_EQ("light_map.png", workflow->LightMap()); + EXPECT_EQ(2u, workflow->LightMapTexCoordSet()); } // visual all @@ -203,6 +211,10 @@ TEST(Material, PbrDOM) // emissive map EXPECT_EQ("emissive_map.png", workflow->EmissiveMap()); + + // light map + EXPECT_EQ("light_map.png", workflow->LightMap()); + EXPECT_EQ(3u, workflow->LightMapTexCoordSet()); } // metal { @@ -237,6 +249,10 @@ TEST(Material, PbrDOM) // emissive map EXPECT_EQ("emissive_map.png", workflow->EmissiveMap()); + + // light map + EXPECT_EQ("light_map.png", workflow->LightMap()); + EXPECT_EQ(0u, workflow->LightMapTexCoordSet()); } } } @@ -343,6 +359,13 @@ TEST(Material, MaterialPBR) EXPECT_TRUE(metalElem->HasElement("emissive_map")); sdf::ElementPtr emissiveMapElem = metalElem->GetElement("emissive_map"); EXPECT_EQ("emissive_map.png", emissiveMapElem->Get()); + + + // light map + EXPECT_TRUE(metalElem->HasElement("light_map")); + sdf::ElementPtr lightMapElem = metalElem->GetElement("light_map"); + EXPECT_EQ("light_map.png", lightMapElem->Get()); + EXPECT_EQ(1u, lightMapElem->Get("uv_set")); } // visual specular workflow @@ -412,6 +435,12 @@ TEST(Material, MaterialPBR) EXPECT_TRUE(specularElem->HasElement("emissive_map")); sdf::ElementPtr emissiveMapElem = specularElem->GetElement("emissive_map"); EXPECT_EQ("emissive_map.png", emissiveMapElem->Get()); + + // light map + EXPECT_TRUE(specularElem->HasElement("light_map")); + sdf::ElementPtr lightMapElem = specularElem->GetElement("light_map"); + EXPECT_EQ("light_map.png", lightMapElem->Get()); + EXPECT_EQ(2u, lightMapElem->Get("uv_set")); } // visual all @@ -484,6 +513,12 @@ TEST(Material, MaterialPBR) sdf::ElementPtr emissiveMapElem = specularElem->GetElement("emissive_map"); EXPECT_EQ("emissive_map.png", emissiveMapElem->Get()); + + // light map + EXPECT_TRUE(specularElem->HasElement("light_map")); + sdf::ElementPtr lightMapElem = specularElem->GetElement("light_map"); + EXPECT_EQ("light_map.png", lightMapElem->Get()); + EXPECT_EQ(3u, lightMapElem->Get("uv_set")); } { @@ -538,8 +573,12 @@ TEST(Material, MaterialPBR) EXPECT_TRUE(metalElem->HasElement("emissive_map")); sdf::ElementPtr emissiveMapElem = metalElem->GetElement("emissive_map"); EXPECT_EQ("emissive_map.png", emissiveMapElem->Get()); + + // light map + EXPECT_TRUE(metalElem->HasElement("light_map")); + sdf::ElementPtr lightMapElem = metalElem->GetElement("light_map"); + EXPECT_EQ("light_map.png", lightMapElem->Get()); + EXPECT_EQ(0u, lightMapElem->Get("uv_set")); } } } - - diff --git a/test/sdf/material_pbr.sdf b/test/sdf/material_pbr.sdf index b7987dd99..8ee2604aa 100644 --- a/test/sdf/material_pbr.sdf +++ b/test/sdf/material_pbr.sdf @@ -16,6 +16,7 @@ environment_map.png ambient_occlusion_map.png emissive_map.png + light_map.png @@ -35,6 +36,7 @@ environment_map.png ambient_occlusion_map.png emissive_map.png + light_map.png @@ -56,6 +58,7 @@ environment_map.png ambient_occlusion_map.png emissive_map.png + light_map.png albedo_map.png @@ -67,6 +70,7 @@ environment_map.png ambient_occlusion_map.png emissive_map.png + light_map.png From 42f8fad0d3f9a13ae8d17fb13692acdc80fff966 Mon Sep 17 00:00:00 2001 From: Jenn Nguyen Date: Wed, 4 Aug 2021 17:07:20 -0700 Subject: [PATCH 10/10] Support parsing elements that are not part of the schema (#638) * support parsing inf Signed-off-by: Jenn Nguyen * made ValueFromStringImpl visible Signed-off-by: Jenn Nguyen * type to string mapping Signed-off-by: Jenn Nguyen * added test Signed-off-by: Jenn Nguyen * readded bool test case Signed-off-by: Jenn Nguyen * added comment Signed-off-by: Jenn Nguyen --- include/sdf/Param.hh | 122 +++++++++++++++++++++++--------- src/Param.cc | 113 +++++++++++++++++------------ src/Param_TEST.cc | 39 +++++++++- test/integration/CMakeLists.txt | 2 - 4 files changed, 190 insertions(+), 86 deletions(-) diff --git a/include/sdf/Param.hh b/include/sdf/Param.hh index 175ef521a..bcb74cdc4 100644 --- a/include/sdf/Param.hh +++ b/include/sdf/Param.hh @@ -213,6 +213,7 @@ namespace sdf /// \brief Private method to set the Element from a passed-in string. /// \param[in] _value Value to set the parameter to. + /// \return True if the parameter was successfully set, false otherwise. private: bool ValueFromString(const std::string &_value); /// \brief Private data @@ -258,8 +259,63 @@ namespace sdf /// \brief This parameter's default value public: ParamVariant defaultValue; + + /// \brief Method used to set the Param from a passed-in string + /// \param[in] _typeName The data type of the value to set + /// \param[in] _valueStr The value as a string + /// \param[out] _valueToSet The value to set + /// \return True if the value was successfully set, false otherwise + public: bool SDFORMAT_VISIBLE ValueFromStringImpl( + const std::string &_typeName, + const std::string &_valueStr, + ParamVariant &_valueToSet) const; + + /// \brief Data type to string mapping + /// \return The type as a string, empty string if unknown type + public: template + std::string TypeToString() const; }; + /////////////////////////////////////////////// + template + std::string ParamPrivate::TypeToString() const + { + if constexpr (std::is_same_v) + return "bool"; + else if constexpr (std::is_same_v) + return "char"; + else if constexpr (std::is_same_v) + return "string"; + else if constexpr (std::is_same_v) + return "int"; + else if constexpr (std::is_same_v) + return "uint64_t"; + else if constexpr (std::is_same_v) + return "unsigned int"; + else if constexpr (std::is_same_v) + return "double"; + else if constexpr (std::is_same_v) + return "float"; + else if constexpr (std::is_same_v) + return "time"; + else if constexpr (std::is_same_v) + return "angle"; + else if constexpr (std::is_same_v) + return "color"; + else if constexpr (std::is_same_v) + return "vector2i"; + else if constexpr (std::is_same_v) + return "vector2d"; + else if constexpr (std::is_same_v) + return "vector3"; + else if constexpr (std::is_same_v) + return "quaternion"; + else if constexpr (std::is_same_v) + return "pose"; + else + return ""; + } + /////////////////////////////////////////////// template void Param::SetUpdateFunc(T _updateFunc) @@ -291,50 +347,48 @@ namespace sdf template bool Param::Get(T &_value) const { - try + T *value = std::get_if(&this->dataPtr->value); + if (value) { - if (typeid(T) == typeid(bool) && this->dataPtr->typeName == "string") + _value = *value; + } + else + { + std::string typeStr = this->dataPtr->TypeToString(); + if (typeStr.empty()) + { + sdferr << "Unknown parameter type[" << typeid(T).name() << "]\n"; + return false; + } + + std::string valueStr = this->GetAsString(); + ParamPrivate::ParamVariant pv; + bool success = this->dataPtr->ValueFromStringImpl(typeStr, valueStr, pv); + + if (success) + { + _value = std::get(pv); + } + else if (typeStr == "bool" && this->dataPtr->typeName == "string") { - std::string strValue = std::get(this->dataPtr->value); - std::transform(strValue.begin(), strValue.end(), strValue.begin(), - [](unsigned char c) - { - return static_cast(std::tolower(c)); - }); + // this section for handling bool types is to keep backward behavior + // TODO(anyone) remove for Fortress. For more details: + // https://github.com/ignitionrobotics/sdformat/pull/638 + valueStr = lowercase(valueStr); std::stringstream tmp; - if (strValue == "true" || strValue == "1") - { + if (valueStr == "true" || valueStr == "1") tmp << "1"; - } else - { tmp << "0"; - } + tmp >> _value; + return true; } - else - { - T *value = std::get_if(&this->dataPtr->value); - if (value) - _value = *value; - else - { - std::stringstream ss; - ss << ParamStreamer{this->dataPtr->value}; - ss >> _value; - } - } - } - catch(...) - { - sdferr << "Unable to convert parameter[" - << this->dataPtr->key << "] " - << "whose type is[" - << this->dataPtr->typeName << "], to " - << "type[" << typeid(T).name() << "]\n"; - return false; + + return success; } + return true; } diff --git a/src/Param.cc b/src/Param.cc index 0ab69a43a..c808db885 100644 --- a/src/Param.cc +++ b/src/Param.cc @@ -273,14 +273,24 @@ std::string Param::GetDefaultAsString() const ////////////////////////////////////////////////// bool Param::ValueFromString(const std::string &_value) +{ + return this->dataPtr->ValueFromStringImpl(this->dataPtr->typeName, + _value, + this->dataPtr->value); +} + +////////////////////////////////////////////////// +bool ParamPrivate::ValueFromStringImpl(const std::string &_typeName, + const std::string &_valueStr, + ParamVariant &_valueToSet) const { // Under some circumstances, latin locales (es_ES or pt_BR) will return a // comma for decimal position instead of a dot, making the conversion // to fail. See bug #60 for more information. Force to use always C setlocale(LC_NUMERIC, "C"); - std::string tmp(_value); - std::string lowerTmp = lowercase(_value); + std::string tmp(_valueStr); + std::string lowerTmp = lowercase(_valueStr); // "true" and "false" doesn't work properly if (lowerTmp == "true") @@ -304,15 +314,15 @@ bool Param::ValueFromString(const std::string &_value) numericBase = 16; } - if (this->dataPtr->typeName == "bool") + if (_typeName == "bool") { if (lowerTmp == "true" || lowerTmp == "1") { - this->dataPtr->value = true; + _valueToSet = true; } else if (lowerTmp == "false" || lowerTmp == "0") { - this->dataPtr->value = false; + _valueToSet = false; } else { @@ -320,107 +330,116 @@ bool Param::ValueFromString(const std::string &_value) return false; } } - else if (this->dataPtr->typeName == "char") + else if (_typeName == "char") { - this->dataPtr->value = tmp[0]; + _valueToSet = tmp[0]; } - else if (this->dataPtr->typeName == "std::string" || - this->dataPtr->typeName == "string") + else if (_typeName == "std::string" || + _typeName == "string") { - this->dataPtr->value = tmp; + _valueToSet = tmp; } - else if (this->dataPtr->typeName == "int") + else if (_typeName == "int") { - this->dataPtr->value = std::stoi(tmp, nullptr, numericBase); + _valueToSet = std::stoi(tmp, nullptr, numericBase); } - else if (this->dataPtr->typeName == "uint64_t") + else if (_typeName == "uint64_t") { StringStreamClassicLocale ss(tmp); std::uint64_t u64tmp; ss >> u64tmp; - this->dataPtr->value = u64tmp; + _valueToSet = u64tmp; } - else if (this->dataPtr->typeName == "unsigned int") + else if (_typeName == "unsigned int") { - this->dataPtr->value = static_cast( + _valueToSet = static_cast( std::stoul(tmp, nullptr, numericBase)); } - else if (this->dataPtr->typeName == "double") + else if (_typeName == "double") { - this->dataPtr->value = std::stod(tmp); + _valueToSet = std::stod(tmp); } - else if (this->dataPtr->typeName == "float") + else if (_typeName == "float") { - this->dataPtr->value = std::stof(tmp); + _valueToSet = std::stof(tmp); } - else if (this->dataPtr->typeName == "sdf::Time" || - this->dataPtr->typeName == "time") + else if (_typeName == "sdf::Time" || + _typeName == "time") { StringStreamClassicLocale ss(tmp); sdf::Time timetmp; ss >> timetmp; - this->dataPtr->value = timetmp; + _valueToSet = timetmp; + } + else if (_typeName == "ignition::math::Angle" || + _typeName == "angle") + { + StringStreamClassicLocale ss(tmp); + ignition::math::Angle angletmp; + + ss >> angletmp; + _valueToSet = angletmp; } - else if (this->dataPtr->typeName == "ignition::math::Color" || - this->dataPtr->typeName == "color") + else if (_typeName == "ignition::math::Color" || + _typeName == "color") { StringStreamClassicLocale ss(tmp); ignition::math::Color colortmp; ss >> colortmp; - this->dataPtr->value = colortmp; + _valueToSet = colortmp; } - else if (this->dataPtr->typeName == "ignition::math::Vector2i" || - this->dataPtr->typeName == "vector2i") + else if (_typeName == "ignition::math::Vector2i" || + _typeName == "vector2i") { StringStreamClassicLocale ss(tmp); ignition::math::Vector2i vectmp; ss >> vectmp; - this->dataPtr->value = vectmp; + _valueToSet = vectmp; } - else if (this->dataPtr->typeName == "ignition::math::Vector2d" || - this->dataPtr->typeName == "vector2d") + else if (_typeName == "ignition::math::Vector2d" || + _typeName == "vector2d") { StringStreamClassicLocale ss(tmp); ignition::math::Vector2d vectmp; ss >> vectmp; - this->dataPtr->value = vectmp; + _valueToSet = vectmp; } - else if (this->dataPtr->typeName == "ignition::math::Vector3d" || - this->dataPtr->typeName == "vector3") + else if (_typeName == "ignition::math::Vector3d" || + _typeName == "vector3") { StringStreamClassicLocale ss(tmp); ignition::math::Vector3d vectmp; ss >> vectmp; - this->dataPtr->value = vectmp; + _valueToSet = vectmp; } - else if (this->dataPtr->typeName == "ignition::math::Pose3d" || - this->dataPtr->typeName == "pose" || - this->dataPtr->typeName == "Pose") + else if (_typeName == "ignition::math::Pose3d" || + _typeName == "pose" || + _typeName == "Pose") { StringStreamClassicLocale ss(tmp); ignition::math::Pose3d posetmp; ss >> posetmp; - this->dataPtr->value = posetmp; + _valueToSet = posetmp; } - else if (this->dataPtr->typeName == "ignition::math::Quaterniond" || - this->dataPtr->typeName == "quaternion") + else if (_typeName == "ignition::math::Quaterniond" || + _typeName == "quaternion") { StringStreamClassicLocale ss(tmp); ignition::math::Quaterniond quattmp; ss >> quattmp; - this->dataPtr->value = quattmp; + _valueToSet = quattmp; } else { - sdferr << "Unknown parameter type[" << this->dataPtr->typeName << "]\n"; + sdferr << "Unknown parameter type[" << _typeName << "]\n"; return false; } } @@ -428,16 +447,16 @@ bool Param::ValueFromString(const std::string &_value) catch(std::invalid_argument &) { sdferr << "Invalid argument. Unable to set value [" - << _value << " ] for key[" - << this->dataPtr->key << "].\n"; + << _valueStr << " ] for key[" + << this->key << "].\n"; return false; } // Catch out of range exception from std::stoi/stoul/stod/stof catch(std::out_of_range &) { sdferr << "Out of range. Unable to set value [" - << _value << " ] for key[" - << this->dataPtr->key << "].\n"; + << _valueStr << " ] for key[" + << this->key << "].\n"; return false; } diff --git a/src/Param_TEST.cc b/src/Param_TEST.cc index 24f587c20..9604b595d 100644 --- a/src/Param_TEST.cc +++ b/src/Param_TEST.cc @@ -22,6 +22,8 @@ #include #include +#include +#include #include "sdf/Exception.hh" #include "sdf/Param.hh" @@ -101,6 +103,27 @@ TEST(SetFromString, Decimals) ASSERT_TRUE(check_double("0.2345")); } +//////////////////////////////////////////////////// +/// Test setting param as a string but getting it as different type +TEST(Param, StringTypeGet) +{ + sdf::Param stringParam("key", "string", "", false, "description"); + + // pose type + ignition::math::Pose3d pose; + EXPECT_TRUE(stringParam.SetFromString("1 1 1 0 0 0")); + EXPECT_TRUE(stringParam.Get(pose)); + EXPECT_EQ(ignition::math::Pose3d(1, 1, 1, 0, 0, 0), pose); + EXPECT_EQ("string", stringParam.GetTypeName()); + + // color type + ignition::math::Color color; + EXPECT_TRUE(stringParam.SetFromString("0 0 1 1")); + EXPECT_TRUE(stringParam.Get(color)); + EXPECT_EQ(ignition::math::Color(0, 0, 1, 1), color); + EXPECT_EQ("string", stringParam.GetTypeName()); +} + //////////////////////////////////////////////////// /// Test Inf TEST(SetFromString, DoublePositiveInf) @@ -112,10 +135,15 @@ TEST(SetFromString, DoublePositiveInf) { sdf::Param doubleParam("key", "double", "0", false, "description"); double value = 0.; - EXPECT_TRUE(doubleParam.SetFromString(infString)); doubleParam.Get(value); EXPECT_DOUBLE_EQ(std::numeric_limits::infinity(), value); + + sdf::Param stringParam("key", "string", "0", false, "description"); + value = 0; + EXPECT_TRUE(stringParam.SetFromString(infString)); + EXPECT_TRUE(stringParam.Get(value)); + EXPECT_DOUBLE_EQ(std::numeric_limits::infinity(), value); } } @@ -130,10 +158,15 @@ TEST(SetFromString, DoubleNegativeInf) { sdf::Param doubleParam("key", "double", "0", false, "description"); double value = 0.; - EXPECT_TRUE(doubleParam.SetFromString(infString)); doubleParam.Get(value); - EXPECT_DOUBLE_EQ(- std::numeric_limits::infinity(), value); + EXPECT_DOUBLE_EQ(-std::numeric_limits::infinity(), value); + + sdf::Param stringParam("key", "string", "0", false, "description"); + value = 0; + EXPECT_TRUE(stringParam.SetFromString(infString)); + EXPECT_TRUE(stringParam.Get(value)); + EXPECT_DOUBLE_EQ(-std::numeric_limits::infinity(), value); } } diff --git a/test/integration/CMakeLists.txt b/test/integration/CMakeLists.txt index 9dcd39b55..f0d7cce9c 100644 --- a/test/integration/CMakeLists.txt +++ b/test/integration/CMakeLists.txt @@ -71,5 +71,3 @@ if (UNIX AND NOT APPLE) add_test(NAME INTEGRATION_versioned_symbols COMMAND bash ${CMAKE_CURRENT_BINARY_DIR}/all_symbols_have_version.bash ${CMAKE_BINARY_DIR}/src/libsdformat${SDF_MAJOR_VERSION}.so) endif() - -