Apply suggestions from code review

Co-authored-by: Adam Dąbrowski <[email protected]>
Co-authored-by: lumberyard-employee-dm <[email protected]>

Signed-off-by: Michał Pełka <[email protected]>

Gems/ROS2/Code/Source/RobotImporter/FixURDF/FixURDF.cpp

@@ -8,11 +8,12 @@
 
 #include "FixURDF.h"
 #include <AzCore/XML/rapidxml_print.h>
-#include <AzCore/std/containers/set.h>
+#include <AzCore/std/containers/unordered_set.h>
 #include <iostream>
+
 namespace ROS2::Utils
 {
-    AZStd::vector<AZStd::string> AddMissingInertiaToLink(AZ::rapidxml::xml_node<>* urdf)
+    AZStd::vector<AZStd::string> AddMissingInertiaToLinks(AZ::rapidxml::xml_node<>* urdf)
     {
         AZStd::vector<AZStd::string> modifiedLinks;
         using namespace AZ::rapidxml;
@@ -67,7 +68,7 @@ namespace ROS2::Utils
     {
         using namespace AZ::rapidxml;
         AZStd::vector<AZStd::string> modifiedLinks;
-        AZStd::set<AZStd::string> linkAndJointsName;
+        AZStd::unordered_set<AZStd::string> linkAndJointsName;
         for (xml_node<>* link = urdf->first_node("link"); link; link = link->next_sibling("link"))
         {
             auto* name = link->first_attribute("name");
@@ -86,7 +87,7 @@ namespace ROS2::Utils
                     auto newName = AZStd::string(name->value()) + "_joint_dup";
                     name->value(urdf->document()->allocate_string(newName.c_str()));
                     linkAndJointsName.insert(newName);
-                    modifiedLinks.push_back(newName);
+                    modifiedLinks.push_back(AZStd::move(newName));
                 }
                 else
                 {
@@ -104,7 +105,7 @@ namespace ROS2::Utils
         xml_document<> doc;
         doc.parse<0>(const_cast<char*>(data.c_str()));
         xml_node<>* urdf = doc.first_node("robot");
-        auto links = AddMissingInertiaToLink(urdf);
+        auto links = AddMissingInertiaToLinks(urdf);
 
         if (links.size())
         {

Gems/ROS2/Code/Source/RobotImporter/FixURDF/FixURDF.h

@@ -16,20 +16,20 @@ namespace ROS2
 {
     namespace Utils
     {
-        //! Modifies in memory URDF to add missing inertia to links, circle navigate SDF error 19
-        //! @param urdf - the in memory URDF to modify
-        //! @returns a list of links that were modified
-        AZStd::vector<AZStd::string> AddMissingInertiaToLink(AZ::rapidxml::xml_node<>* urdf);
+        //! Modifies a parsed URDF in memory to add missing inertia to links, which prevents SDF error 19.
+        //! @param urdf URDF to modify.
+        //! @returns a list of names of links that were modified.
+        AZStd::vector<AZStd::string> AddMissingInertiaToLinks(AZ::rapidxml::xml_node<>* urdf);
 
-        //! Modifies names of links and joints to be unique, circle navigate SDF error 2
-        //! @param urdf - the in memory URDF to modify
+        //! Modifies names of links and joints to be unique, which means that a set of both contains no duplicates. This prevents SDF
+        //! error 2.
+        //! @param urdf URDF to modify.
         //! @returns a list of links that were modified
         AZStd::vector<AZStd::string> ChangeDuplications(AZ::rapidxml::xml_node<>* urdf);
 
         //! Modifies in memory URDF to add missing elements
-        //! @param urdf - the in memory URDF to modify
+        //! @param urdf URDF to modify.
         //! @returns a modified URDF and a list of XML element that were modified
         AZStd::pair<std::string, AZStd::vector<AZStd::string>> ModifyURDFInMemory(const std::string& data);
-
     } // namespace Utils
 } // namespace ROS2

Gems/ROS2/Code/Source/RobotImporter/RobotImporterWidget.cpp

@@ -153,13 +153,13 @@ namespace ROS2
                 AZ_Assert(false, "Unknown file extension : %s \n", m_urdfPath.c_str());
             }
             AZStd::string log;
-            bool urdfParsedSuccess{ parsedUrdfOutcome };
-            bool urdfParsedWithWarnings{ parsedUrdfOutcome.m_modifiedURDFContent.size() > 0};
+            const bool urdfParsedSuccess{ parsedUrdfOutcome };
+            const bool urdfParsedWithWarnings{ parsedUrdfOutcome.UrdfParsedWithModifiedContent() };
             if (urdfParsedSuccess)
             {
                 if (urdfParsedWithWarnings)
                 {
-                    report += "# " + tr("The URDF was parsed, but it was modified to be compatible") + "\n";
+                    report += "# " + tr("The URDF was parsed, though results were modified to be compatible with SDFormat") + "\n";
                     report += tr("Modified tags in URDF:") + "\n";
                     for (const auto& modifiedTag : parsedUrdfOutcome.m_modifiedURDFTags)
                     {

Gems/ROS2/Code/Source/RobotImporter/URDF/UrdfParser.cpp

@@ -14,7 +14,7 @@
 #include <AzCore/std/string/string.h>
 #include <RobotImporter/FixURDF/FixURDF.h>
 #include <RobotImporter/Utils/ErrorUtils.h>
-#include <RobotImporter/Utils/FilePath.cpp>
+#include <RobotImporter/Utils/FilePath.h>
 
 namespace ROS2::UrdfParser
 {
@@ -85,6 +85,11 @@ namespace ROS2::UrdfParser
         return m_sdfErrors.empty();
     }
 
+    bool ParseResult::UrdfParsedWithModifiedContent() const
+    {
+        return m_modifiedURDFTags.size() > 0;
+    }
+
     RootObjectOutcome Parse(AZStd::string_view xmlString, const sdf::ParserConfig& parserConfig)
     {
         return Parse(std::string(xmlString.data(), xmlString.size()), parserConfig);
@@ -126,16 +131,18 @@ namespace ROS2::UrdfParser
         }
 
         std::string xmlStr((std::istreambuf_iterator<char>(istream)), std::istreambuf_iterator<char>());
-        // modify in memory
         if (Utils::IsFileUrdf(filePath) && settings.m_fixURDF)
         {
-            const auto& [modifiedXmlStr, modifiedElements] = (ROS2::Utils::ModifyURDFInMemory(xmlStr));
+            // modify in memory
+            auto [modifiedXmlStr, modifiedElements] = (ROS2::Utils::ModifyURDFInMemory(xmlStr));
 
             auto result = Parse(modifiedXmlStr, parserConfig);
-            result.m_modifiedURDFTags = modifiedElements;
-            result.m_modifiedURDFContent = modifiedXmlStr;
+            result.m_modifiedURDFTags = AZStd::move(modifiedElements);
+            result.m_modifiedURDFContent = AZStd::move(modifiedXmlStr);
             return result;
         }
         return Parse(xmlStr, parserConfig);
     }
+
+
 } // namespace ROS2::UrdfParser

Gems/ROS2/Code/Source/RobotImporter/URDF/UrdfParser.h

@@ -9,6 +9,7 @@
 #pragma once
 
 #include <AzCore/IO/Path/Path.h>
+#include <AzCore/std/containers/vector.h>
 #include <AzCore/std/utility/expected.h>
 #include <SdfAssetBuilder/SdfAssetBuilderSettings.h>
 #include <sdf/Collision.hh>
@@ -34,8 +35,10 @@ namespace ROS2
         private:
             // Provides custom sdf::ErrorCode values when parsing is done through O3DE
             inline static constexpr auto O3DESdfErrorCodeStart = static_cast<sdf::ErrorCode>(1000);
+
         public:
-           inline static constexpr auto O3DESdfErrorParseNotStarted = static_cast<sdf::ErrorCode>(static_cast<int>(O3DESdfErrorCodeStart) + 1);
+            inline static constexpr auto O3DESdfErrorParseNotStarted =
+                static_cast<sdf::ErrorCode>(static_cast<int>(O3DESdfErrorCodeStart) + 1);
 
             //! Ref qualifier overloads for retrieving sdf::Root
             //! it supports a non-const lvalue overload to allow
@@ -62,12 +65,16 @@ namespace ROS2
             //! Returns if the parsing of the SDF file has succeeded
             explicit operator bool() const;
 
+            //! Returns if the URDF content was modified during parsing
+            bool UrdfParsedWithModifiedContent() const;
+
             sdf::Root m_root;
             AZStd::string m_parseMessages;
-            sdf::Errors m_sdfErrors{ sdf::Error{ O3DESdfErrorParseNotStarted, std::string{"No Parsing has occurred yet"}} };
+            sdf::Errors m_sdfErrors{ sdf::Error{ O3DESdfErrorParseNotStarted, std::string{ "No Parsing has occurred yet" } } };
 
             //! Stores the modified URDF content after parsing, empty if no modification occurred
             std::string m_modifiedURDFContent;
+
             //! Stores the modified URDF tags after parsing, empty if no modification occurred
             AZStd::vector<AZStd::string> m_modifiedURDFTags;
         };
@@ -91,6 +98,7 @@ namespace ROS2
         //!        AddURIPath() function to provide a mapping of package:// and model:// references to the local filesystem
         //! @paragraph settings structure that contains configuration options for the SDFAssetBuilder
         //! @return SDF root object containing parsed <world> or <model> tags
-        RootObjectOutcome ParseFromFile(AZ::IO::PathView filePath, const sdf::ParserConfig& parserConfig, const SdfAssetBuilderSettings& settings);
+        RootObjectOutcome ParseFromFile(
+            AZ::IO::PathView filePath, const sdf::ParserConfig& parserConfig, const SdfAssetBuilderSettings& settings);
     }; // namespace UrdfParser
 } // namespace ROS2

Gems/ROS2/Code/Source/SdfAssetBuilder/SdfAssetBuilderSettings.cpp

@@ -97,7 +97,7 @@ namespace ROS2
                         AZ::Edit::UIHandlers::Default,
                         &SdfAssetBuilderSettings::m_fixURDF,
                         "Fix URDF to be compatible with libsdformat",
-                        "When set, fixes the URDF file before importing it. This is useful for fixing URDF files that have missing inertials."
+                        "When set, fixes the URDF file before importing it. This is useful for fixing URDF files that have missing inertials or duplicate names within links and joints."
                         );
             }
         }

Gems/ROS2/Code/Source/SdfAssetBuilder/SdfAssetBuilderSettings.h

@@ -36,7 +36,7 @@ namespace ROS2
         bool m_urdfPreserveFixedJoints = true;
         // When true, .dae/.stl mesh files are imported into the project folder to allow the AP to process them
         bool m_importReferencedMeshFiles = true;
-        // When true URDF will be fixed to be compatible with SDF
+        // When true URDF will be fixed to be compatible with SDFormat.
         bool m_fixURDF = true;
     };
 } // namespace ROS2