Apply suggestions from code review

Co-authored-by: lumberyard-employee-dm <[email protected]>
Co-authored-by: Steve Pham <[email protected]>
Signed-off-by: Michał Pełka <[email protected]>

Gems/ROS2/.github/ISSUE_TEMPLATE/bug_report.md

@@ -1,32 +0,0 @@
----
-name: Bug report
-about: Create a report to help us improve
-title: ''
-labels: ''
-assignees: ''
-
----
-
-**Bug description**
-A clear and concise description of what the bug is.
-
-**To Reproduce**
-Steps to reproduce the behavior:
-1. Go to '...'
-2. Click on '....'
-3. Scroll down to '....'
-4. See error
-
-**Expected behavior**
-A clear and concise description of what you expected to happen.
-
-**Screenshots / videos**
-If applicable, add screenshots/videos to help explain your problem.
-
-**Test environment**
- - OS: [e.g. Ubuntu 20.04]
- - ROS: [e.g. Galactic]
-
-
-**Additional context**
-Add any other context about the problem here.

Gems/ROS2/.github/ISSUE_TEMPLATE/feature_request.md

@@ -1,20 +0,0 @@
----
-name: Feature request
-about: Suggest an idea for this project
-title: ''
-labels: ''
-assignees: ''
-
----
-
-**Is your feature request related to a problem? Please describe.**
-A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
-
-**Describe the solution you'd like**
-A clear and concise description of what you want to happen.
-
-**Describe alternatives you've considered**
-A clear and concise description of any alternative solutions or features you've considered.
-
-**Additional context**
-Add any other context or screenshots about the feature request here.

Gems/ROS2/.github/workflows/Build.yml

@@ -1,76 +0,0 @@
-name: Build
-
-on:
-  pull_request:
-    branches:
-      - development
-    paths:
-      - ".github/workflows/Build.yml"
-      - "Code/**"
-      - "CMakeLists.txt"
-      - "gem.json"
-      
-permissions:
-  contents: read
-
-jobs:
-  build-with-empty-project:
-    name: Build the Gem with an empty Project
-    runs-on: self-hosted
-    timeout-minutes: 60
-    env:
-#     vv Change the o3de hash (LONG FORMAT!) here to fit the build vv
-      O3DE_SHA: 932a0327a58580028bb4a314b2d464dd974edaef
-      ROS_DISTRO: galactic
-    steps:
-      - name: O3DE ROS 2 Gem checkout
-        uses: actions/checkout@v3
-        with:
-          path: o3de-ros2-gem
-      - name: O3DE checkout
-        uses: actions/checkout@v3
-        with:
-          repository: o3de/o3de
-          ref: ${{ env.O3DE_SHA }}
-          path: o3de
-      - name: O3DE git-lfs pull
-        run: |
-          cd o3de
-          git lfs pull
-          cd ..
-      - name: Set RUNNER_HOME
-        run: |
-          RUNNER_WORK=$(pwd)
-          cd ../../..
-          echo "RUNNER_HOME=$(pwd)" >> $GITHUB_ENV
-          cd $RUNNER_WORK
-      - name: Register the Engine
-        run: |
-          mkdir -p ${{ env.RUNNER_HOME }}/o3de-packages
-          o3de/python/get_python.sh
-          o3de/scripts/o3de.sh register --this-engine --force
-      - name: Create an empty Project
-        run: |
-          o3de/scripts/o3de.sh create-project \
-            --project-path ${{ env.RUNNER_HOME }}/o3de-projects/EmptyProject
-      - name: Add the O3DE ROS2 Gem to the Project
-        run: |
-          o3de/scripts/o3de.sh register --gem-path o3de-ros2-gem
-          o3de/scripts/o3de.sh enable-gem -gn ROS2 \
-            -pp ${{ env.RUNNER_HOME }}/o3de-projects/EmptyProject
-      - name: Create the Linux build project for the Project
-        run: |
-          cd ${{ env.RUNNER_HOME }}/o3de-projects/EmptyProject
-          cmake -B build/linux -S . -G "Ninja Multi-Config" \
-            -DLY_3RDPARTY_PATH=${{ env.RUNNER_HOME }}/o3de-packages
-      - name: Build the Project
-        run: >
-          cmake
-          --build ${{ env.RUNNER_HOME }}/o3de-projects/EmptyProject/build/linux
-          --config release
-          --target EmptyProject Editor
-          -j $(($(nproc) / 2))
-      - name: Project Cleanup
-        if: always()
-        run: |
-          rm -r ${{ env.RUNNER_HOME }}/o3de-projects/EmptyProject 2> /dev/null

Gems/ROS2/.github/workflows/CodeChecks.yml

@@ -1,49 +0,0 @@
-name: CodeChecks
-
-on:
-  pull_request:
-    branches:
-      - development
-    paths:
-      - ".github/workflows/CodeChecks.yml"
-      - "Code/**"
-      - "CMakeLists.txt"
-      - ".licenserc.yaml"
-
-permissions:
-  contents: read
-
-jobs:
-  clang-format:
-    name: Check code formatting with clang-format-13
-    runs-on: ubuntu-20.04
-    steps:
-      - name: Clone the Gem repository
-        uses: actions/checkout@v3
-      - name: Clone the run-clang-format repository
-        uses: actions/checkout@v3
-        with:
-          repository: Sarcasm/run-clang-format
-          ref: 39081c9c42768ab5e8321127a7494ad1647c6a2f
-          path: run-clang-format
-      - name: Install clang-format-13
-        run: |
-          wget https://apt.llvm.org/llvm.sh
-          chmod +x llvm.sh
-          sudo ./llvm.sh 13
-          sudo apt update && sudo apt install clang-format-13
-      - name: Run the 'run-clang-format.py' file
-        run: |
-          cp run-clang-format/run-clang-format.py run-clang-format.py
-          sudo rm -r run-clang-format
-          ./run-clang-format.py -r --style=file --clang-format-executable=clang-format-13 Code
-
-  license:
-    name: Check license headers
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@v3
-      - uses: apache/[email protected]
-        with:
-          mode: check
-          config: .licenserc.yaml

Gems/ROS2/.github/workflows/Documentation.yml

@@ -1,48 +0,0 @@
-name: Documentation
-
-on:
-  push:
-    branches:
-      - development
-    paths:
-      - ".github/workflows/Documentation.yml"
-      - "Code/**"
-  workflow_run:
-# DISCLAIMER: This trigger will only work if this workflow is located on the default branch!
-    workflows: ["Documentation Configuration Changed"]
-    branches:
-      - "gh-pages"
-    types:
-      - completed
-
-permissions:
-  contents: write
-
-jobs:
-  docsgen:
-    name: Generate documentation and publish it to Github Pages
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout gh-pages branch
-        uses: actions/checkout@v3
-        with:
-          ref: gh-pages
-      - name: Checkout development branch
-        uses: actions/checkout@v3
-        with:
-          repository: RobotecAI/o3de-ros2-gem
-          ref: development
-          path: o3de-ros2-gem
-      - name: Setup doxygen and graphviz
-        run: |
-          sudo apt update
-          sudo apt install -y doxygen wget
-          sudo apt install graphviz
-      - name: Generate documentation
-        run: doxygen Doxyfile
-      - name: Remove the unused development branch folder
-        run: rm -r o3de-ros2-gem
-      - name: Deploy to GitHub Pages
-        uses: JamesIves/[email protected]
-        with:
-          folder: .

Gems/ROS2/Assets/Scripts/SimpleTwistControl.lua

@@ -1,37 +0,0 @@
---[[
-    This is an example of control implementation which sets the desired velocity on a single body.
-    To imitate the steering, current linear and angular velocities of a single rigidbody are forcefully overwritten
-    with the desired control.
-    TODO: Control the robot with forces applied to the wheels instead of directly setting up body velocity.
---]]
-
-local SimpleTwistControl = {
-    Properties = {}
-}
-
-function SimpleTwistControl:OnActivate()
-    self.twistNotificationBus = TwistNotificationBus.Connect(self);
-end
-
-function SimpleTwistControl:TwistReceived(linear, angular)
-    -- Get current linear velocity
-    local currentLinearVelocity = RigidBodyRequestBus.Event.GetLinearVelocity(self.entityId)
-
-    -- Convert local steering to world frame
-    local worldTM = TransformBus.Event.GetWorldTM(self.entityId)
-    local linearVelocityTransformed = Transform.TransformVector(worldTM, linear)
-
-    -- Overwrite control velocities on two axis
-    currentLinearVelocity["x"] = linearVelocityTransformed["x"]
-    currentLinearVelocity["y"] = linearVelocityTransformed["y"]
-
-    -- Reapply altered velocities for the rigidbody
-    RigidBodyRequestBus.Event.SetAngularVelocity(self.entityId, angular)
-    RigidBodyRequestBus.Event.SetLinearVelocity(self.entityId, currentLinearVelocity)
-end
-
-function SimpleTwistControl:OnDeactivate()
-    self.twistNotificationBus:Disconnect()
-end
-
-return SimpleTwistControl

Gems/ROS2/CMakeLists.txt

@@ -3,6 +3,16 @@
 #
 # SPDX-License-Identifier: Apache-2.0 OR MIT
 
+# Query the gem name from the gem.json file if possible
+# otherwise fallback to using ${Name}
+o3de_find_ancestor_gem_root(gempath gem_name "${CMAKE_CURRENT_SOURCE_DIR}")
+# Query the gem name from the gem.json file if possible
+# otherwise fallback to using ${Name}
+o3de_find_ancestor_gem_root(gempath gem_name "${CMAKE_CURRENT_SOURCE_DIR}")
+if (NOT gem_name)
+    set(gem_name "ROS2")
+endif()
+
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/Code")
 set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} PARENT_SCOPE)
 

Gems/ROS2/Code/CMakeLists.txt

@@ -22,7 +22,7 @@ endif()
 # Note: We include the common files and the platform specific files which are set in ros2_common_files.cmake
 # and in ${pal_dir}/ros2_${PAL_PLATFORM_NAME_LOWERCASE}_files.cmake
 ly_add_target(
-    NAME ROS2.Static STATIC
+    NAME ${gem_name}.Static STATIC
     NAMESPACE Gem
     FILES_CMAKE
         ros2_header_files.cmake
@@ -42,10 +42,10 @@ ly_add_target(
             Gem::StartingPointInput
 )
 
-target_depends_on_ros2_packages(ROS2.Static rclcpp builtin_interfaces std_msgs sensor_msgs nav_msgs urdfdom tf2_ros ackermann_msgs gazebo_msgs control_toolbox)
+target_depends_on_ros2_packages(${gem_name}.Static rclcpp builtin_interfaces std_msgs sensor_msgs nav_msgs urdfdom tf2_ros ackermann_msgs gazebo_msgs control_toolbox)
 
 ly_add_target(
-    NAME ROS2.API HEADERONLY
+    NAME ${gem_name}.API HEADERONLY
     NAMESPACE Gem
     FILES_CMAKE
         ros2_header_files.cmake
@@ -56,7 +56,7 @@ ly_add_target(
 
 # Here add ROS2 target, it depends on the ROS2.Static
 ly_add_target(
-    NAME ROS2 ${PAL_TRAIT_MONOLITHIC_DRIVEN_MODULE_TYPE}
+    NAME ${gem_name} ${PAL_TRAIT_MONOLITHIC_DRIVEN_MODULE_TYPE}
     NAMESPACE Gem
     FILES_CMAKE
         ros2_shared_files.cmake
@@ -67,21 +67,21 @@ ly_add_target(
             Source
     BUILD_DEPENDENCIES
         PRIVATE
-            Gem::ROS2.Static
+            Gem::${gem_name}.Static
             Gem::Atom_Feature_Common.Static
 )
 
 # By default, we will specify that the above target ROS2 would be used by
 # Client and Server type targets when this gem is enabled.  If you don't want it
 # active in Clients or Servers by default, delete one of both of the following lines:
-ly_create_alias(NAME ROS2.Clients NAMESPACE Gem TARGETS Gem::ROS2)
-ly_create_alias(NAME ROS2.Servers NAMESPACE Gem TARGETS Gem::ROS2)
+ly_create_alias(NAME ${gem_name}.Clients NAMESPACE Gem TARGETS Gem::${gem_name})
+ly_create_alias(NAME ${gem_name}.Servers NAMESPACE Gem TARGETS Gem::${gem_name})
 
-# If we are on a host platform, we want to add the host tools targets like the ROS2.Editor target which
-# will also depend on ROS2.Static
+# If we are on a host platform, we want to add the host tools targets like the ${gem_name}.Editor target which
+# will also depend on ${gem_name}.Static
 if(PAL_TRAIT_BUILD_HOST_TOOLS)
     ly_add_target(
-        NAME ROS2.Editor.Static STATIC
+        NAME ${gem_name}.Editor.Static STATIC
         NAMESPACE Gem
         FILES_CMAKE
             ros2_editor_files.cmake
@@ -101,11 +101,11 @@ if(PAL_TRAIT_BUILD_HOST_TOOLS)
                 Gem::AtomLyIntegration_CommonFeatures.Editor.Static
                 Gem::LmbrCentral.API
                 Gem::PhysX.Editor.Static
-                Gem::ROS2.Static
+                Gem::${gem_name}.Static
     )
 
     ly_add_target(
-        NAME ROS2.Editor GEM_MODULE
+        NAME ${gem_name}.Editor GEM_MODULE
         NAMESPACE Gem
         AUTOMOC
         AUTORCC
@@ -124,15 +124,15 @@ if(PAL_TRAIT_BUILD_HOST_TOOLS)
                 Include
         BUILD_DEPENDENCIES
             PUBLIC
-                Gem::ROS2.Editor.Static
+                Gem::${gem_name}.Editor.Static
                 Gem::Atom_Feature_Common.Static
     )
 
     # By default, we will specify that the above target ROS2 would be used by
     # Tool and Builder type targets when this gem is enabled.  If you don't want it
     # active in Tools or Builders by default, delete one of both of the following lines:
-    ly_create_alias(NAME ROS2.Tools    NAMESPACE Gem TARGETS Gem::ROS2.Editor)
-    ly_create_alias(NAME ROS2.Builders NAMESPACE Gem TARGETS Gem::ROS2.Editor)
+    ly_create_alias(NAME ${gem_name}.Tools    NAMESPACE Gem TARGETS Gem::${gem_name}.Editor)
+    ly_create_alias(NAME ${gem_name}.Builders NAMESPACE Gem TARGETS Gem::${gem_name}.Editor)
 endif()
 
 ################################################################################
@@ -143,7 +143,7 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
     if(PAL_TRAIT_TEST_GOOGLE_TEST_SUPPORTED)
         # We support ROS2.Tests on this platform, add ROS2.Tests target which depends on ROS2.Static
         ly_add_target(
-            NAME ROS2.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
+            NAME ${gem_name}.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
             NAMESPACE Gem
             FILES_CMAKE
                 ros2_tests_files.cmake
@@ -154,12 +154,12 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
             BUILD_DEPENDENCIES
                 PRIVATE
                     AZ::AzTest
-                    Gem::ROS2.Static
+                    Gem::${gem_name}.Static
         )
 
         # Add ROS2.Tests to googletest
         ly_add_googletest(
-            NAME Gem::ROS2.Tests
+            NAME Gem::${gem_name}.Tests
         )
     endif()
 
@@ -168,7 +168,7 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
         if(PAL_TRAIT_TEST_GOOGLE_TEST_SUPPORTED)
             # We support ROS2.Editor.Tests on this platform, add ROS2.Editor.Tests target which depends on ROS2.Editor
             ly_add_target(
-                NAME ROS2.Editor.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
+                NAME ${gem_name}.Editor.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
                 NAMESPACE Gem
                 FILES_CMAKE
                     ros2_editor_tests_files.cmake
@@ -181,12 +181,12 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
                 BUILD_DEPENDENCIES
                     PRIVATE
                         AZ::AzTest
-                        Gem::ROS2.Editor
+                        Gem::${gem_name}.Editor
             )
 
             # Add ROS2.Editor.Tests to googletest
             ly_add_googletest(
-                NAME Gem::ROS2.Editor.Tests
+                NAME Gem::${gem_name}.Editor.Tests
             )
         endif()
     endif()

Gems/ROS2/Code/Include/ROS2/Communication/QoS.h

@@ -23,14 +23,13 @@ namespace ROS2
         QoS(const rclcpp::QoS& qos = rclcpp::QoS(rmw_qos_profile_default.depth));
         static void Reflect(AZ::ReflectContext* context);
 
-        //! Create and return QoS based on member values (Editor combos selection).
+        //! Convert and return a rclcpp::QoS instance based on member values (Editor combos selection).
         //! @return a <a href="https://docs.ros2.org/latest/api/rclcpp/classrclcpp_1_1QoS.html">ROS2 QoS struct</a>.
         rclcpp::QoS GetQoS() const;
 
     private:
-        AZ::Crc32 OnQoSSelected();
+        AZ::Crc32 OnQoSSelected() const;
 
-        // TODO - only for Editor component
         //! If necessary, extend to also expose history and liveliness.
         rclcpp::ReliabilityPolicy m_reliabilityPolicy;
         rclcpp::DurabilityPolicy m_durabilityPolicy;

Gems/ROS2/Code/Include/ROS2/Frame/NamespaceConfiguration.h

@@ -25,20 +25,20 @@ namespace ROS2
 
         //! A choice of methods to handle namespaces.
         //! @note Top level ROS2FrameComponent will likely be associated with an interesting object (robot). For multi-robot
-        //! simulation, namespaces are often derived from the robot name itself. For this reason, the default behavior
-        //! for top level ROS2FrameComponent is to generate namespace from entity name.
-        enum NamespaceStrategy
+        //! simulations, namespaces are often derived from the robot name itself. For this reason, the default behavior
+        //! for top level ROS2FrameComponent is to generate the namespace from entity name.
+        enum class NamespaceStrategy
         {
             Default, //!< FromEntityName for top-level frames, Empty otherwise.
-            Empty,
+            Empty, //!< An empty, blank namespace
             FromEntityName, //!< Generate from Entity name, but substitute disallowed characters through RosifyName.
             Custom //!< Non-empty and based on user-provided value.
         };
 
         //! Set namespace based on context.
-        //! @param isRoot Whether namespace belongs to top-level entity in the entity hierarchy.
+        //! @param isRoot Whether or not the namespace belongs to top-level entity in the entity hierarchy.
         //! @param entityName Raw (not ros-ified) name of the entity to which the namespace belongs.
-        void PopulateNamespace(bool isRoot, AZStd::string entityName);
+        void PopulateNamespace(bool isRoot, const AZStd::string& entityName);
         AZStd::string GetNamespace(const AZStd::string& parentNamespace) const;
 
     private:
@@ -47,8 +47,13 @@ namespace ROS2
         bool m_isRoot;
         AZStd::string m_entityName;
 
+        //! Determine if namespace is using the Custom namespace strategy
         bool IsNamespaceCustom() const;
+        //! Update the namespace based on the current attributes
+
+        //! Update the namespace based on the current attributes
         void UpdateNamespace();
+
         AZ::Crc32 OnNamespaceStrategySelected();
     };
 } // namespace ROS2

Gems/ROS2/Code/Include/ROS2/Frame/ROS2FrameComponent.h

@@ -7,12 +7,12 @@
  */
 #pragma once
 
-#include "ROS2/Frame/NamespaceConfiguration.h"
-#include "ROS2/Frame/ROS2Transform.h"
-#include "ROS2/ROS2GemUtilities.h"
 #include <AzCore/Component/Component.h>
 #include <AzCore/std/smart_ptr/unique_ptr.h>
 #include <AzFramework/Components/TransformComponent.h>
+#include <ROS2/Frame/NamespaceConfiguration.h>
+#include <ROS2/Frame/ROS2Transform.h>
+#include <ROS2/ROS2GemUtilities.h>
 
 namespace ROS2
 {
@@ -29,10 +29,14 @@ namespace ROS2
         AZ_COMPONENT(ROS2FrameComponent, "{EE743472-3E25-41EA-961B-14096AC1D66F}");
 
         ROS2FrameComponent();
-        ROS2FrameComponent(AZStd::string frameId);
+        //! Initialize to a specific frame id
+        ROS2FrameComponent(const AZStd::string& frameId);
 
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
 
         static void Reflect(AZ::ReflectContext* context);
         static void GetProvidedServices(AZ::ComponentDescriptor::DependencyArrayType& provided);
@@ -57,10 +61,13 @@ namespace ROS2
 
         //! Global frame name in ros2 ecosystem.
         //! @return The name of the global frame with namespace attached. It is typically "odom", "map", "world".
-        AZStd::string GetGlobalFrameName() const; // TODO - allow to configure global frame in a specialized component
+        AZStd::string GetGlobalFrameName() const;
 
     private:
+        //////////////////////////////////////////////////////////////////////////
+        // AZ::TickBus::Handler overrides
         void OnTick(float deltaTime, AZ::ScriptTimePoint time) override;
+        //////////////////////////////////////////////////////////////////////////
 
         bool IsTopLevel() const; //!< True if this entity does not have a parent entity with ROS2.
 
@@ -73,9 +80,8 @@ namespace ROS2
         //! @see GetGlobalFrameName().
         AZStd::string GetParentFrameID() const;
 
-        // TODO - Editor component: validation of fields, constraints between values and so on
         NamespaceConfiguration m_namespaceConfiguration;
-        AZStd::string m_frameName = "sensor_frame"; // TODO - option to fill from entity name
+        AZStd::string m_frameName = "sensor_frame";
 
         bool m_publishTransform = true;
         AZStd::unique_ptr<ROS2Transform> m_ros2Transform;

Gems/ROS2/Code/Include/ROS2/Frame/ROS2Transform.h

@@ -15,7 +15,6 @@ namespace ROS2
 {
     //! Publishes transforms as standard ROS2 tf2 messages. Static transforms are published once.
     //! @note This class is already used through ROS2FrameComponent.
-    // TODO - Rework this class (name, function). Separate broadcaster out of ROS2SystemComponent
     class ROS2Transform
     {
     public:

Gems/ROS2/Code/Include/ROS2/Manipulator/MotorizedJointBus.h

@@ -38,7 +38,7 @@ namespace ROS2
         virtual float GetSetpoint() = 0;
 
         //! Retrieve current measurement.
-        //! Measurement is the value of the movement - e.g. protrusion of an actuator.
+        //! Measurement is the value of the movement - eg protrusion of an actuator.
         //! For linear actuators, measurement is given in meters.
         //! For the rotational actuators, measurement is given in radians.
         //! When the setpoint is reached this should be close to setpoint.

Gems/ROS2/Code/Include/ROS2/Manipulator/MotorizedJointComponent.h

@@ -8,11 +8,11 @@
 #pragma once
 
 #include "MotorizedJointBus.h"
-#include "ROS2/VehicleDynamics/DriveModels/PidConfiguration.h"
 #include <AzCore/Component/Component.h>
 #include <AzCore/Component/TickBus.h>
 #include <AzCore/Math/Transform.h>
 #include <AzCore/Math/Vector2.h>
+#include <ROS2/VehicleDynamics/DriveModels/PidConfiguration.h>
 
 namespace ROS2
 {
@@ -21,7 +21,6 @@ namespace ROS2
     //! TransformBus mode, called `AnimationMode` changes local transform. In this mode, you cannot have a rigid body
     //! controller enabled. With RigidBodyBus it applies forces and torque according to PID control.
     //! @note This class is already used through ROS2FrameComponent.
-    // TODO This is a prototype. Tasks: refactor, add bus interface, rotation, ramps, cascading controllers, tests.
     class MotorizedJointComponent
         : public AZ::Component
         , public AZ::TickBus::Handler
@@ -32,24 +31,20 @@ namespace ROS2
 
         MotorizedJointComponent() = default;
         ~MotorizedJointComponent() = default;
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
         static void Reflect(AZ::ReflectContext* context);
+        //////////////////////////////////////////////////////////////////////////
 
-        //! Set a setpoint (e.g. desired local position). The controller will follow it.
+        ////////////////////////////////////////////////////////////////////////
+        // MotorizedJointRequestBus::Handler overrides
         void SetSetpoint(float setpoint) override;
-
-        //! Get a setpoint
         float GetSetpoint() override;
-
-        //! Get current control error. It is the difference between control value and measurement.
-        //! When the setpoint is reached this should be close to zero.
-        //! @returns control error, in meters for linear joints and in radians for angular joints.
         float GetError() override;
-
-        //! Get current position from measurement.
-        //! @returns current position, in meters for linear joints and radians for angular joints.
         float GetCurrentMeasurement() override;
+        ////////////////////////////////////////////////////////////////////////
 
         //! Get a degree of freedom direction.
         //! @returns direction of joint movement in global coordinates.
@@ -64,7 +59,10 @@ namespace ROS2
         void ApplyLinVelAnimation(float velocity, float deltaTime);
         void ApplyLinVelRigidBodyImpulse(float velocity, float deltaTime);
         void ApplyLinVelRigidBody(float velocity, float deltaTime);
+        //////////////////////////////////////////////////////////////////////////
+        // AZ::TickBus::Handler overrides
         void OnTick(float deltaTime, AZ::ScriptTimePoint time) override;
+        //////////////////////////////////////////////////////////////////////////
 
         AZ::Vector3 m_jointDir{ 0.f, 0.f, 1.f }; //!< Direction of joint movement in parent frame of reference, used to compute measurement.
         AZ::Vector3 m_effortAxis{ 0.f, 0.f, 1.f }; //!< Direction of force or torque application in owning entity frame of reference.
@@ -74,7 +72,6 @@ namespace ROS2
         bool m_linear{ true }; //!< Linear mode. The force is applied through RigidBodyBus.
         bool m_animationMode{ true }; //!< Use TransformBus (animation mode, no physics) instead of RigidBodyBus.
 
-        // TODO - remove test signal?
         bool m_testSinusoidal{ true }; //!< Enable sinusoidal signal generator to setpoint (for tuning).
         float m_sinAmplitude{ 0.5 }; //!< Amplitude of test signal generator.
         float m_sinDC{ 0.25 }; //!< DC of test signal generator.
@@ -85,9 +82,8 @@ namespace ROS2
         float m_error{ 0 }; //!< Current error (difference between control value and measurement).
         float m_currentPosition{ 0 }; //!< Last measured position.
         float m_currentVelocity{ 0 }; //!< Last measured velocity.
-        double m_lastMeasurementTime; //!< Last measurement time in seconds.
+        double m_lastMeasurementTime{ 0 }; //!< Last measurement time in seconds.
 
-        // TODO - remove/replace with proper API use (EntityDebugDisplayEventBus)
         AZ::EntityId m_debugDrawEntity; //!< Optional Entity that allows to visualize desired setpoint value.
         AZ::Transform m_debugDrawEntityInitialTransform; //!< Initial transform of m_debugDrawEntity.
         bool m_debugPrint{ false }; //!< Print debug info to the console.

Gems/ROS2/Code/Include/ROS2/ROS2GemUtilities.h

@@ -7,25 +7,25 @@
  */
 #pragma once
 
-#include "AzCore/Component/ComponentBus.h"
-#include "AzCore/Component/Entity.h"
+#include <AzCore/Component/ComponentBus.h>
+#include <AzCore/Component/Entity.h>
 #ifdef ROS2_EDITOR
-#include "AzToolsFramework/ToolsComponents/GenericComponentWrapper.h"
+#include <AzToolsFramework/ToolsComponents/GenericComponentWrapper.h>
 #endif
 namespace ROS2
 {
     namespace Utils
     {
         /// Create component for a given entity in safe way.
-        /// \param entityId entity that will own component
-        /// \param componentType Uuid of component to create
-        /// \return created componentId, if it fails, it returns invalid id
+        /// @param entityId entity that will own component
+        /// @param componentType Uuid of component to create
+        /// @return The created componentId if successful, otherwise returns an invalid id
         AZ::ComponentId CreateComponent(const AZ::EntityId entityId, const AZ::Uuid componentType);
 
         /// Retrieve component from entity given by a pointer. It is a way to get game components and wrapped components.
-        /// We should use that that we are not sure if we access e.g. ROS2FrameComponent in game mode or from Editor
-        /// \param entity pointer to entity e.g. with GetEntity()
-        /// \return pointer to component with type T
+        /// We should use that that we are not sure if we access eg ROS2FrameComponent in game mode or from Editor
+        /// @param entity pointer to entity eg with GetEntity()
+        /// @return pointer to component with type T
 
         template<class ComponentType>
         ComponentType* GetGameOrEditorComponent(const AZ::Entity* entity)

Gems/ROS2/Code/Include/ROS2/RobotControl/Ackermann/AckermannBus.h

@@ -23,7 +23,7 @@ namespace ROS2
 
         //! Handle Ackermann command
         //! @param ackermannCommand A structure with AckermannDrive message fields
-        virtual void AckermannReceived(const AckermannCommandStruct& angular) = 0;
+        virtual void AckermannReceived(const AckermannCommandStruct& ackermannCommand) = 0;
     };
 
     using AckermannNotificationBus = AZ::EBus<AckermannNotifications>;

Gems/ROS2/Code/Include/ROS2/RobotControl/Ackermann/AckermannCommandStruct.h

@@ -13,9 +13,9 @@
 struct AckermannCommandStruct
 {
     AZ_TYPE_INFO(AckermannCommandStruct, "{6D03C30F-F06B-4CEE-8AD1-DDCCCB57C4B5}");
-    float m_steeringAngle = 0; //!< radians
-    float m_steeringAngleVelocity = 0; //!< radians/s
-    float m_speed = 0; //!< m/s
-    float m_acceleration = 0; //!< m/s^2
-    float m_jerk = 0; //!< m/s^3
+    float m_steeringAngle = 0; //!< desired virtual angle (radians)
+    float m_steeringAngleVelocity = 0; //!< desired rate of change (radians/s)
+    float m_speed = 0; //!< desired forward speed (m/s)
+    float m_acceleration = 0; //!< desired acceleration (m/s^2)
+    float m_jerk = 0; //!< desired jerk (m/s^3)
 };

Gems/ROS2/Code/Include/ROS2/RobotControl/ControlConfiguration.h

@@ -23,7 +23,7 @@ namespace ROS2
         //! Type of control for the robot.
         //! Different types of steering can fit different platforms,
         //! depending on the type their mobile base (four wheels, omniwheels, ..).
-        enum Steering
+        enum class Steering
         {
             Twist, //!< @see <a href="https://docs.ros2.org/latest/api/geometry_msgs/msg/Twist.html">Twist</a>.
             Ackermann //!< @see <a href="https://github.com/ros-drivers/ackermann_msgs/blob/ros2/msg/AckermannDrive.msg">AckermannDrive</a>.

Gems/ROS2/Code/Include/ROS2/RobotControl/ControlSubscriptionHandler.h

@@ -7,10 +7,10 @@
  */
 #pragma once
 
-#include "ROS2/Communication/TopicConfiguration.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/Utilities/ROS2Names.h"
+#include <ROS2/Communication/TopicConfiguration.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/Utilities/ROS2Names.h>
 #include <rclcpp/rclcpp.hpp>
 
 namespace ROS2
@@ -24,6 +24,7 @@ namespace ROS2
         //! @param entity Activation context for the owning Component - the entity it belongs to.
         //! @param subscriberConfiguration configuration with topic and qos
         virtual void Activate(const AZ::Entity* entity, const TopicConfiguration& subscriberConfiguration) = 0;
+        //! Interface handling component deactivation
         virtual void Deactivate() = 0;
         virtual ~IControlSubscriptionHandler() = default;
     };
@@ -34,7 +35,7 @@ namespace ROS2
     class ControlSubscriptionHandler : public IControlSubscriptionHandler
     {
     public:
-        void Activate(const AZ::Entity* entity, const TopicConfiguration& subscriberConfiguration) final
+        void Activate(const AZ::Entity* entity, const TopicConfiguration& subscriberConfiguration) override final
         {
             m_active = true;
             m_entityId = entity->GetId();
@@ -54,7 +55,7 @@ namespace ROS2
             }
         };
 
-        void Deactivate() final
+        void Deactivate() override final
         {
             m_active = false;
             m_controlSubscription.reset(); // Note: topic and qos can change, need to re-subscribe
@@ -72,7 +73,9 @@ namespace ROS2
         void OnControlMessage(const T& message)
         {
             if (!m_active)
+            {
                 return;
+            }
 
             SendToBus(message);
         };

Gems/ROS2/Code/Include/ROS2/Sensor/ROS2SensorComponent.h

@@ -7,11 +7,11 @@
  */
 #pragma once
 
-#include "ROS2/ROS2GemUtilities.h"
 #include "SensorConfiguration.h"
 #include <AzCore/Component/Component.h>
 #include <AzCore/Component/TickBus.h>
 #include <AzCore/std/smart_ptr/unique_ptr.h>
+#include <ROS2/ROS2GemUtilities.h>
 
 namespace ROS2
 {
@@ -20,15 +20,19 @@ namespace ROS2
     //! Derive this Component to implement a new ROS2 sensor. Each sensor Component requires ROS2FrameComponent.
     class ROS2SensorComponent
         : public AZ::Component
-        , public AZ::TickBus::Handler // TODO - high resolution tick source?
+        , public AZ::TickBus::Handler
     {
     public:
         ROS2SensorComponent() = default;
         virtual ~ROS2SensorComponent() = default;
         AZ_COMPONENT(ROS2SensorComponent, "{91BCC1E9-6D93-4466-9CDB-E73D497C6B5E}");
 
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
+
         static void Reflect(AZ::ReflectContext* context);
         void OnTick(float deltaTime, AZ::ScriptTimePoint time) override;
         static void GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required);

Gems/ROS2/Code/Include/ROS2/Sensor/SensorConfiguration.h

@@ -7,11 +7,11 @@
  */
 #pragma once
 
-#include "ROS2/Communication/TopicConfiguration.h"
 #include <AzCore/RTTI/RTTI.h>
 #include <AzCore/Serialization/SerializeContext.h>
 #include <AzCore/std/containers/map.h>
 #include <AzCore/std/string/string.h>
+#include <ROS2/Communication/TopicConfiguration.h>
 
 namespace ROS2
 {
@@ -26,13 +26,11 @@ namespace ROS2
 
         //! ROS2 Publishers of this sensor.
         //! Some sensors can have more than one publisher (example: Camera).
-        //! This map will typically hold 1-3 elements.
+        //! @note This map will typically hold 1-3 elements.
         AZStd::map<AZStd::string, TopicConfiguration> m_publishersConfigurations;
 
         //! Frequency in Hz (1/s).
         //! Applies both to data acquisition and publishing.
-        // TODO - consider moving frequency, publishingEnabled to publisherConfiguration if any sensor has
-        // a couple of publishers for which we want different values of these fields
         float m_frequency = 10;
 
         bool m_publishingEnabled = true;

Gems/ROS2/Code/Include/ROS2/Utilities/ROS2Conversions.h

@@ -17,16 +17,15 @@
 namespace ROS2
 {
     //! Utility class for conversions between ROS2 types and O3DE (AZ::) types.
-    class ROS2Conversions
+    namespace ROS2Conversions
     {
-    public:
-        static AZ::Vector3 FromROS2Vector3(const geometry_msgs::msg::Vector3& ros2vector);
-        static geometry_msgs::msg::Vector3 ToROS2Vector3(const AZ::Vector3& azvector);
-        static geometry_msgs::msg::Point ToROS2Point(const AZ::Vector3& azvector);
-        static geometry_msgs::msg::Quaternion ToROS2Quaternion(const AZ::Quaternion& azquaternion);
-        static geometry_msgs::msg::Pose ToROS2Pose(const AZ::Transform& aztransform);
-        static AZ::Transform FromROS2Pose(const geometry_msgs::msg::Pose& ros2pose);
-        static AZ::Vector3 FromROS2Point(const geometry_msgs::msg::Point& ros2point);
-        static AZ::Quaternion FromROS2Quaternion(const geometry_msgs::msg::Quaternion& ros2quaternion);
-    };
+        AZ::Vector3 FromROS2Vector3(const geometry_msgs::msg::Vector3& ros2vector);
+        geometry_msgs::msg::Vector3 ToROS2Vector3(const AZ::Vector3& azvector);
+        geometry_msgs::msg::Point ToROS2Point(const AZ::Vector3& azvector);
+        geometry_msgs::msg::Quaternion ToROS2Quaternion(const AZ::Quaternion& azquaternion);
+        geometry_msgs::msg::Pose ToROS2Pose(const AZ::Transform& aztransform);
+        AZ::Transform FromROS2Pose(const geometry_msgs::msg::Pose& ros2pose);
+        AZ::Vector3 FromROS2Point(const geometry_msgs::msg::Point& ros2point);
+        AZ::Quaternion FromROS2Quaternion(const geometry_msgs::msg::Quaternion& ros2quaternion);
+    }; // namespace ROS2Conversions
 } // namespace ROS2

Gems/ROS2/Code/Include/ROS2/Utilities/ROS2Names.h

@@ -14,26 +14,25 @@
 namespace ROS2
 {
     //! Utility class for handling ROS2 naming rules.
-    class ROS2Names
+    namespace ROS2Names
     {
-    public:
         //! Joins namespace and the given name.
-        static AZStd::string GetNamespacedName(const AZStd::string& ns, const AZStd::string& name);
+        AZStd::string GetNamespacedName(const AZStd::string& ns, const AZStd::string& name);
 
         //! Converts input to a ROS2-acceptable name for topics and namespaces.
         //! Any characters not fitting ROS2 naming specification are replaced with underscores.
-        static AZStd::string RosifyName(const AZStd::string& input);
+        AZStd::string RosifyName(const AZStd::string& input);
 
         //! Validates namespace adherence to ROS2 specification. Delegates validation to ROS2 layers.
-        static AZ::Outcome<void, AZStd::string> ValidateNamespace(const AZStd::string& ros2Namespace);
+        AZ::Outcome<void, AZStd::string> ValidateNamespace(const AZStd::string& ros2Namespace);
 
         //! Validate namespace field. Fits ChangeValidate for Editor fields.
-        static AZ::Outcome<void, AZStd::string> ValidateNamespaceField(void* newValue, const AZ::Uuid& valueType);
+        AZ::Outcome<void, AZStd::string> ValidateNamespaceField(void* newValue, const AZ::Uuid& valueType);
 
         //! Validate topic adherence to ROS2 specification.
-        static AZ::Outcome<void, AZStd::string> ValidateTopic(const AZStd::string& topic);
+        AZ::Outcome<void, AZStd::string> ValidateTopic(const AZStd::string& topic);
 
         //! Validate topic field. Fits ChangeValidate for Editor fields.
-        static AZ::Outcome<void, AZStd::string> ValidateTopicField(void* newValue, const AZ::Uuid& valueType);
-    };
+        AZ::Outcome<void, AZStd::string> ValidateTopicField(void* newValue, const AZ::Uuid& valueType);
+    }; // namespace ROS2Names
 } // namespace ROS2

Gems/ROS2/Code/Include/ROS2/VehicleDynamics/VehicleInputControlBus.h

@@ -30,26 +30,26 @@ namespace VehicleDynamics
 
         //! Steer in a direction given in relative coordinate system (current direction is 0).
         //! @param steering is angle in radians, positive to the right and negative to the left.
-        //! Note that the actual angle applied is subject to limits and implementation (e.g. smoothing).
+        //! @note The actual angle applied is subject to limits and implementation (eg smoothing).
         virtual void SetTargetSteering(float steering) = 0;
 
         //! Accelerate without target speed, relative to the limits.
-        //! @param acceleration is relative to limits of possible acceleration.
+        //! @param accelerationFraction is relative to limits of possible acceleration.
         //! 1 - accelerate as much as possible, -1 - brake as much as possible.
         virtual void SetTargetAccelerationFraction(float accelerationFraction) = 0;
 
         //! Steer input version which is relative to limits.
-        //! @param steering is -1 to 1, which applies as a fraction of vehicle model steering limits.
-        //! Note that the actual angle applied is subject to limits and implementation (e.g. smoothing).
+        //! @param steeringFraction is -1 to 1, which applies as a fraction of vehicle model steering limits.
+        //! @note The actual angle applied is subject to limits and implementation (eg smoothing).
         virtual void SetTargetSteeringFraction(float steeringFraction) = 0;
 
         //! Speed input version which is relative to limits.
-        //! @param speedMps is -1 to 1, which applies as a fraction of vehicle model speed limits.
+        //! @param speedFraction is -1 to 1, which applies as a fraction of vehicle model speed limits.
         virtual void SetTargetLinearSpeedFraction(float speedFraction) = 0;
 
-        //! Disable vehicle dynamics
-        //! @param is_disable if set true no torque will be applied
-        virtual void SetDisableVehicleDynamics(bool is_disable) = 0;
+        //! Disables (or enables) the vehicle dynamics
+        //! @param disable if set true no torque will be applied
+        virtual void SetDisableVehicleDynamics(bool disable) = 0;
     };
 
     using VehicleInputControlRequestBus = AZ::EBus<VehicleInputControlRequests>;

Gems/ROS2/Code/Platform/Common/Clang/ros2_static_editor_clang.cmake

@@ -7,5 +7,5 @@
 
 set(LY_COMPILE_OPTIONS
         PRIVATE
-            -fexceptions -frtti
+            -fexceptions
 )

Gems/ROS2/Code/Platform/Common/GCC/ros2_static_editor_gcc.cmake

@@ -7,5 +7,5 @@
 
 set(LY_COMPILE_OPTIONS
         PRIVATE
-            -fexceptions -frtti
+            -fexceptions
 )

Gems/ROS2/Code/Platform/Common/msvc/ros2_static_editor_msvc.cmake

@@ -7,5 +7,5 @@
 
 set(LY_COMPILE_OPTIONS
         PRIVATE
-            /EHsc /GR
+            /EHsc
 )

Gems/ROS2/Code/Source/Camera/CameraSensor.cpp

@@ -29,7 +29,7 @@ namespace ROS2
     {
 
         /// @FormatMappings - contains the mapping from RHI to ROS image encodings. List of supported
-        /// ROS image encodings lives in `sensor_msgs/image_encodings.hpp'
+        /// ROS image encodings lives in `sensor_msgs/image_encodings.hpp`
         /// We are not including `image_encodings.hpp` since it uses exceptions.
         AZStd::unordered_map<AZ::RHI::Format, const char*> FormatMappings{
             { AZ::RHI::Format::R8G8B8A8_UNORM, "rgba8" },     { AZ::RHI::Format::R16G16B16A16_UNORM, "rgba16" },
@@ -52,7 +52,6 @@ namespace ROS2
     } // namespace Internal
     CameraSensorDescription::CameraSensorDescription(const AZStd::string& cameraName, float verticalFov, int width, int height)
         : m_verticalFieldOfViewDeg(verticalFov)
-        , m_verticalFieldOfViewRad(AZ::DegToRad(m_verticalFieldOfViewDeg))
         , m_width(width)
         , m_height(height)
         , m_cameraName(cameraName)
@@ -60,7 +59,7 @@ namespace ROS2
         , m_viewToClipMatrix(MakeViewToClipMatrix())
         , m_cameraIntrinsics(MakeCameraIntrinsics())
     {
-        validateParameters();
+        ValidateParameters();
     }
 
     AZ::Matrix4x4 CameraSensorDescription::MakeViewToClipMatrix() const
@@ -72,7 +71,7 @@ namespace ROS2
         return localViewToClipMatrix;
     }
 
-    void CameraSensorDescription::validateParameters() const
+    void CameraSensorDescription::ValidateParameters() const
     {
         AZ_Assert(
             m_verticalFieldOfViewDeg > 0.0f && m_verticalFieldOfViewDeg < 180.0f,
@@ -93,9 +92,10 @@ namespace ROS2
         // (cx, cy).
         const auto w = static_cast<double>(m_width);
         const auto h = static_cast<double>(m_height);
-        const double horizontalFoV = 2.0 * AZStd::atan(AZStd::tan(m_verticalFieldOfViewRad / 2.0) * m_aspectRatio);
+        const double verticalFieldOfView = AZ::DegToRad(m_verticalFieldOfViewDeg);
+        const double horizontalFoV = 2.0 * AZStd::atan(AZStd::tan(verticalFieldOfView / 2.0) * m_aspectRatio);
         const double focalLengthX = w / (2.0 * AZStd::tan(horizontalFoV / 2.0));
-        const double focalLengthY = h / (2.0 * AZStd::tan(m_verticalFieldOfViewRad / 2.0));
+        const double focalLengthY = h / (2.0 * AZStd::tan(verticalFieldOfView / 2.0));
         return { focalLengthX, 0.0, w / 2.0, 0.0, focalLengthY, h / 2.0, 0.0, 0.0, 1.0 };
     }
 
@@ -104,22 +104,22 @@ namespace ROS2
     {
     }
 
-    void CameraSensor::setupPasses()
+    void CameraSensor::SetupPasses()
     {
-        AZ_TracePrintf("CameraSensor", "Initializing pipeline for %s", m_cameraSensorDescription.m_cameraName.c_str());
+        AZ_TracePrintf("CameraSensor", "Initializing pipeline for %s\n", m_cameraSensorDescription.m_cameraName.c_str());
 
         const AZ::Name viewName = AZ::Name("MainCamera");
         m_view = AZ::RPI::View::CreateView(viewName, AZ::RPI::View::UsageCamera);
         m_view->SetViewToClipMatrix(m_cameraSensorDescription.m_viewToClipMatrix);
         m_scene = AZ::RPI::RPISystemInterface::Get()->GetSceneByName(AZ::Name("Main"));
 
-        const AZStd::string pipelineName = m_cameraSensorDescription.m_cameraName + "Pipeline" + getPipelineTypeName();
+        const AZStd::string pipelineName = m_cameraSensorDescription.m_cameraName + "Pipeline" + GetPipelineTypeName();
 
         AZ::RPI::RenderPipelineDescriptor pipelineDesc;
         pipelineDesc.m_mainViewTagName = "MainCamera";
         pipelineDesc.m_name = pipelineName;
 
-        pipelineDesc.m_rootPassTemplate = getPipelineTemplateName();
+        pipelineDesc.m_rootPassTemplate = GetPipelineTemplateName();
 
         pipelineDesc.m_renderSettings.m_multisampleState = AZ::RPI::RPISystemInterface::Get()->GetApplicationMultisampleState();
         m_pipeline = AZ::RPI::RenderPipeline::CreateRenderPipeline(pipelineDesc);
@@ -162,7 +162,7 @@ namespace ROS2
     void CameraSensor::RequestFrame(
         const AZ::Transform& cameraPose, AZStd::function<void(const AZ::RPI::AttachmentReadback::ReadbackResult& result)> callback)
     {
-        const AZ::Transform inverse = (cameraPose * kAtomToRos).GetInverse();
+        const AZ::Transform inverse = (cameraPose * AtomToRos).GetInverse();
         m_view->SetWorldToViewMatrix(AZ::Matrix4x4::CreateFromQuaternionAndTranslation(inverse.GetRotation(), inverse.GetTranslation()));
 
         AZ::Render::FrameCaptureId captureId = AZ::Render::InvalidFrameCaptureId;
@@ -182,7 +182,7 @@ namespace ROS2
         return m_cameraSensorDescription;
     }
 
-    void CameraSensor::publishMassage(
+    void CameraSensor::RequestMessagePublication(
         std::shared_ptr<rclcpp::Publisher<sensor_msgs::msg::Image>> publisher,
         const AZ::Transform& cameraPose,
         const std_msgs::msg::Header& header)
@@ -208,15 +208,15 @@ namespace ROS2
     CameraDepthSensor::CameraDepthSensor(const CameraSensorDescription& cameraSensorDescription)
         : CameraSensor(cameraSensorDescription)
     {
-        setupPasses();
+        SetupPasses();
     }
 
-    AZStd::string CameraDepthSensor::getPipelineTemplateName()
+    AZStd::string CameraDepthSensor::GetPipelineTemplateName() const
     {
         return "PipelineRenderToTextureROSDepth";
     };
 
-    AZStd::string CameraDepthSensor::getPipelineTypeName()
+    AZStd::string CameraDepthSensor::GetPipelineTypeName() const
     {
         return "Depth";
     };
@@ -224,15 +224,15 @@ namespace ROS2
     CameraColorSensor::CameraColorSensor(const CameraSensorDescription& cameraSensorDescription)
         : CameraSensor(cameraSensorDescription)
     {
-        setupPasses();
+        SetupPasses();
     }
 
-    AZStd::string CameraColorSensor::getPipelineTemplateName()
+    AZStd::string CameraColorSensor::GetPipelineTemplateName() const
     {
         return "PipelineRenderToTextureROSColor";
     };
 
-    AZStd::string CameraColorSensor::getPipelineTypeName()
+    AZStd::string CameraColorSensor::GetPipelineTypeName() const
     {
         return "Color";
     };

Gems/ROS2/Code/Source/Camera/CameraSensor.h

@@ -7,15 +7,13 @@
  */
 #pragma once
 
-#include "ROS2/ROS2GemUtilities.h"
 #include <Atom/Feature/Utils/FrameCaptureBus.h>
+#include <ROS2/ROS2GemUtilities.h>
 #include <chrono>
 #include <rclcpp/publisher.hpp>
 #include <sensor_msgs/msg/image.hpp>
 #include <std_msgs/msg/header.hpp>
 
-class Entity;
-
 namespace ROS2
 {
 
@@ -30,7 +28,6 @@ namespace ROS2
         CameraSensorDescription(const AZStd::string& cameraName, float verticalFov, int width, int height);
 
         const float m_verticalFieldOfViewDeg; //!< camera vertical field of view
-        double m_verticalFieldOfViewRad;
         const int m_width; //!< camera image width in pixels
         const int m_height; //!< camera image height in pixels
         const AZStd::string m_cameraName; //!< camera name to differentiate cameras in a multi-camera setup
@@ -44,7 +41,7 @@ namespace ROS2
 
         AZStd::array<double, 9> MakeCameraIntrinsics() const;
 
-        void validateParameters() const;
+        void ValidateParameters() const;
     };
 
     //! Class to create camera sensor using Atom renderer
@@ -59,20 +56,20 @@ namespace ROS2
         //! Deinitializes rendering pipeline for the camera sensor
         virtual ~CameraSensor();
 
-        //! Function publish Image Message frame from rendering pipeline
+        //! Publish Image Message frame from rendering pipeline
         //! @param publisher - ROS2 publisher to publish image in future
         //! @param header - header with filled message information (frame, timestamp, seq)
         //! @param cameraPose - current camera pose from which the rendering should take place
-        void publishMassage(
+        void RequestMessagePublication(
             std::shared_ptr<rclcpp::Publisher<sensor_msgs::msg::Image>> publisher,
             const AZ::Transform& cameraPose,
             const std_msgs::msg::Header& header);
 
-        //! Function to get camera sensor description
+        //! Get the camera sensor description
         [[nodiscard]] const CameraSensorDescription& GetCameraSensorDescription() const;
 
     private:
-        //! Function requesting frame from rendering pipeline
+        //! Request a frame from the rendering pipeline
         //! @param cameraPose - current camera pose from which the rendering should take place
         //! @param callback - callback function object that will be called when capture is ready
         //!                   it's argument is readback structure containing, among other thins, captured image
@@ -84,34 +81,36 @@ namespace ROS2
         AZ::RPI::RenderPipelinePtr m_pipeline;
         AZ::RPI::ViewPtr m_view;
         AZ::RPI::Scene* m_scene = nullptr;
-        const AZ::Transform kAtomToRos{ AZ::Transform::CreateFromQuaternion(
+        const AZ::Transform AtomToRos{ AZ::Transform::CreateFromQuaternion(
             AZ::Quaternion::CreateFromMatrix3x3(AZ::Matrix3x3::CreateFromRows({ 1, 0, 0 }, { 0, -1, 0 }, { 0, 0, -1 }))) };
-        virtual AZStd::string getPipelineTemplateName() = 0;
-        virtual AZStd::string getPipelineTypeName() = 0;
+        virtual AZStd::string GetPipelineTemplateName() const = 0; //! Returns name of pass template to use in pipeline
+        virtual AZStd::string GetPipelineTypeName() const = 0; //! Type of returned data eg Color, Depth, Optical flow
 
     protected:
         //! Read and setup Atom Passes
-        void setupPasses();
+        void SetupPasses();
     };
 
+    //! Implementation of camera sensors that runs pipeline which produces depth image
     class CameraDepthSensor : public CameraSensor
     {
     public:
         CameraDepthSensor(const CameraSensorDescription& cameraSensorDescription);
 
     private:
-        virtual AZStd::string getPipelineTemplateName() override;
-        virtual AZStd::string getPipelineTypeName() override;
+        AZStd::string GetPipelineTemplateName() const override;
+        AZStd::string GetPipelineTypeName() const override;
     };
 
+    //! Implementation of camera sensors that runs pipeline which produces color image
     class CameraColorSensor : public CameraSensor
     {
     public:
         CameraColorSensor(const CameraSensorDescription& cameraSensorDescription);
 
     private:
-        virtual AZStd::string getPipelineTemplateName() override;
-        virtual AZStd::string getPipelineTypeName() override;
+        AZStd::string GetPipelineTemplateName() const override;
+        AZStd::string GetPipelineTypeName() const override;
     };
 
 } // namespace ROS2

Gems/ROS2/Code/Source/Camera/ROS2CameraSensorComponent.cpp

@@ -6,11 +6,11 @@
  *
  */
 
-#include "Camera/ROS2CameraSensorComponent.h"
-#include "ROS2/Communication/TopicConfiguration.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/Utilities/ROS2Names.h"
+#include "ROS2CameraSensorComponent.h"
+#include <ROS2/Communication/TopicConfiguration.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 #include <AzCore/Component/Entity.h>
 #include <AzCore/Component/TransformBus.h>
@@ -78,7 +78,7 @@ namespace ROS2
                 ec->Class<ROS2CameraSensorComponent>("ROS2 Camera Sensor", "[Camera component]")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->DataElement(
                         AZ::Edit::UIHandlers::Default,
                         &ROS2CameraSensorComponent::m_VerticalFieldOfViewDeg,
@@ -100,7 +100,7 @@ namespace ROS2
 
         const auto cameraInfoPublisherConfig = m_sensorConfiguration.m_publishersConfigurations[Internal::kInfoConfig];
         AZStd::string cameraInfoFullTopic = ROS2Names::GetNamespacedName(GetNamespace(), cameraInfoPublisherConfig.m_topic);
-        AZ_TracePrintf("ROS2", "Creating publisher for camera info on topic %s", cameraInfoFullTopic.data());
+        AZ_TracePrintf("ROS2", "Creating publisher for camera info on topic %s\n", cameraInfoFullTopic.data());
 
         m_cameraInfoPublisher =
             ros2Node->create_publisher<sensor_msgs::msg::CameraInfo>(cameraInfoFullTopic.data(), cameraInfoPublisherConfig.GetQoS());
@@ -114,7 +114,7 @@ namespace ROS2
             AZStd::string cameraImageFullTopic = ROS2Names::GetNamespacedName(GetNamespace(), cameraImagePublisherConfig.m_topic);
             auto publisher =
                 ros2Node->create_publisher<sensor_msgs::msg::Image>(cameraImageFullTopic.data(), cameraImagePublisherConfig.GetQoS());
-            m_cameraSensorsWithPublihsers.emplace_back(createPair<CameraColorSensor>(publisher, description));
+            m_cameraSensorsWithPublihsers.emplace_back(CreatePair<CameraColorSensor>(publisher, description));
         }
         if (m_depthCamera)
         {
@@ -122,7 +122,7 @@ namespace ROS2
             AZStd::string cameraImageFullTopic = ROS2Names::GetNamespacedName(GetNamespace(), cameraImagePublisherConfig.m_topic);
             auto publisher =
                 ros2Node->create_publisher<sensor_msgs::msg::Image>(cameraImageFullTopic.data(), cameraImagePublisherConfig.GetQoS());
-            m_cameraSensorsWithPublihsers.emplace_back(createPair<CameraDepthSensor>(publisher, description));
+            m_cameraSensorsWithPublihsers.emplace_back(CreatePair<CameraDepthSensor>(publisher, description));
         }
         const auto* component = Utils::GetGameOrEditorComponent<ROS2FrameComponent>(GetEntity());
         AZ_Assert(component, "Entity has no ROS2FrameComponent");
@@ -151,15 +151,16 @@ namespace ROS2
             cameraInfo.width = m_width;
             cameraInfo.height = m_height;
             cameraInfo.distortion_model = sensor_msgs::distortion_models::PLUMB_BOB;
-            AZ_Assert(cameraIntrinsics.size() == cameraInfo.k.size(), "should be 9");
-            std::copy_n(cameraIntrinsics.data(), cameraIntrinsics.size(), cameraInfo.k.begin());
+            AZ_Assert(cameraIntrinsics.size() == 9, "camera matrix should have 9 elements");
+            AZ_Assert(cameraInfo.k.size() == 9, "camera matrix should have 9 elements");
+            AZStd::copy(cameraIntrinsics.begin(), cameraIntrinsics.end(), cameraInfo.k.begin());
             cameraInfo.p = { cameraInfo.k[0], cameraInfo.k[1], cameraInfo.k[2], 0, cameraInfo.k[3], cameraInfo.k[4], cameraInfo.k[5], 0,
                              cameraInfo.k[6], cameraInfo.k[7], cameraInfo.k[8], 0 };
             m_cameraInfoPublisher->publish(cameraInfo);
         }
         for (auto& [publisher, sensor] : m_cameraSensorsWithPublihsers)
         {
-            sensor->publishMassage(publisher, transform, ros_header);
+            sensor->RequestMessagePublication(publisher, transform, ros_header);
         }
     }
 } // namespace ROS2

Gems/ROS2/Code/Source/Camera/ROS2CameraSensorComponent.h

@@ -11,7 +11,7 @@
 #include <sensor_msgs/msg/camera_info.hpp>
 #include <sensor_msgs/msg/image.hpp>
 
-#include "ROS2/Sensor/ROS2SensorComponent.h"
+#include <ROS2/Sensor/ROS2SensorComponent.h>
 
 #include <AzCore/Component/Component.h>
 
@@ -41,14 +41,22 @@ namespace ROS2
         void Deactivate() override;
 
     private:
-        //! Type aliases for pointer used in this component
+        //! Pointer to ROS2 image publisher type
         using ImagePublisherPtrType = std::shared_ptr<rclcpp::Publisher<sensor_msgs::msg::Image>>;
+
+        //! Pointer to ROS2 camera sensor publisher type
         using CameraInfoPublisherPtrType = std::shared_ptr<rclcpp::Publisher<sensor_msgs::msg::CameraInfo>>;
+
+        //! Type that combines pointer to ROS2 publisher and CameraSensor
         using PublisherSensorPtrPair = AZStd::pair<ImagePublisherPtrType, AZStd::shared_ptr<CameraSensor>>;
 
-        //! Helper to construct PublisherSensorPtrPair with give Sensor type
+        //! Helper to construct a PublisherSensorPtrPair with a pointer to ROS2 publisher and intrinsic calibration
+        //! @tparam CameraType type of camera sensor (eg 'CameraColorSensor')
+        //! @param publisher pointer to ROS2 image publisher
+        //! @param description CameraSensorDescription with intrinsic calibration
+        //! @return PublisherSensorPtrPair with all provided parameters
         template<typename CameraType>
-        PublisherSensorPtrPair createPair(ImagePublisherPtrType publisher, const CameraSensorDescription& description) const
+        PublisherSensorPtrPair CreatePair(ImagePublisherPtrType publisher, const CameraSensorDescription& description) const
         {
             return { publisher, AZStd::make_shared<CameraType>(description) };
         }

Gems/ROS2/Code/Source/Clock/SimulationClock.cpp

@@ -7,8 +7,8 @@
  */
 
 #include "SimulationClock.h"
-#include "ROS2/ROS2Bus.h"
 #include <AzCore/Time/ITime.h>
+#include <ROS2/ROS2Bus.h>
 #include <rclcpp/qos.hpp>
 
 namespace ROS2

Gems/ROS2/Code/Source/Clock/SimulationClock.h

@@ -21,11 +21,10 @@ namespace ROS2
         //! @see ROS2Requests::GetROSTimestamp() for more details.
         builtin_interfaces::msg::Time GetROSTimestamp() const;
 
-        // TODO - consider having it called internally, also perhaps in a thread with a given frequency
         void Tick();
 
     private:
-        // Time since start of sim, scaled with t_simulationTickScale
+        //! Get the time since start of sim, scaled with t_simulationTickScale
         int64_t GetElapsedTimeMicroseconds() const;
 
         rclcpp::Publisher<rosgraph_msgs::msg::Clock>::SharedPtr m_clockPublisher;

Gems/ROS2/Code/Source/Communication/QoS.cpp

@@ -6,20 +6,20 @@
  *
  */
 
-#include "ROS2/Communication/QoS.h"
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <ROS2/Communication/QoS.h>
 
 namespace ROS2
 {
     QoS::QoS(const rclcpp::QoS& qos)
+        : m_reliabilityPolicy(qos.reliability())
+        , m_durabilityPolicy(qos.durability())
+        , m_depth(qos.depth())
     {
-        m_reliabilityPolicy = qos.reliability();
-        m_durabilityPolicy = qos.durability();
-        m_depth = qos.depth();
     }
 
-    AZ::Crc32 QoS::OnQoSSelected()
+    AZ::Crc32 QoS::OnQoSSelected() const
     {
         return AZ::Edit::PropertyRefreshLevels::AttributesAndValues;
     }

Gems/ROS2/Code/Source/Communication/TopicConfiguration.cpp

@@ -6,9 +6,9 @@
  *
  */
 
-#include "ROS2/Communication/TopicConfiguration.h"
-#include "ROS2/Utilities/ROS2Names.h"
 #include <AzCore/Serialization/EditContext.h>
+#include <ROS2/Communication/TopicConfiguration.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 namespace ROS2
 {

Gems/ROS2/Code/Source/Frame/NamespaceConfiguration.cpp

@@ -6,17 +6,17 @@
  *
  */
 
-#include "ROS2/Frame/NamespaceConfiguration.h"
-#include "ROS2/Utilities/ROS2Names.h"
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
+#include <ROS2/Frame/NamespaceConfiguration.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 namespace ROS2
 {
-    void NamespaceConfiguration::PopulateNamespace(bool isRoot, AZStd::string entityName)
+    void NamespaceConfiguration::PopulateNamespace(bool isRoot, const AZStd::string& entityName)
     {
         m_isRoot = isRoot;
-        m_entityName = AZStd::move(entityName);
+        m_entityName = entityName;
         OnNamespaceStrategySelected();
     }
 
@@ -92,9 +92,7 @@ namespace ROS2
                     ->EnumAttribute(NamespaceConfiguration::NamespaceStrategy::Custom, "Custom")
                     ->DataElement(AZ::Edit::UIHandlers::Default, &NamespaceConfiguration::m_namespace, "Namespace", "Namespace")
                     ->Attribute(AZ::Edit::Attributes::Visibility, &NamespaceConfiguration::IsNamespaceCustom)
-                    ->Attribute(AZ::Edit::Attributes::ChangeValidate, &ROS2Names::ValidateNamespaceField)
-                    // TODO - hide for now, but desired Editor component behavior would be to show a read only value
-                    ;
+                    ->Attribute(AZ::Edit::Attributes::ChangeValidate, &ROS2Names::ValidateNamespaceField);
             }
         }
     }

Gems/ROS2/Code/Source/Frame/ROS2FrameComponent.cpp

@@ -6,21 +6,20 @@
  *
  */
 
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/ROS2GemUtilities.h"
-#include "ROS2/Utilities/ROS2Names.h"
 #include <AzCore/Component/Entity.h>
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/ROS2GemUtilities.h>
+#include <ROS2/Utilities/ROS2Names.h>
 namespace ROS2
 {
     namespace Internal
     {
         AZ::TransformInterface* GetEntityTransformInterface(const AZ::Entity* entity)
         {
-            // TODO - instead, use EditorFrameComponent to handle Editor-context queries and here only use the "Game" version
             if (!entity)
             {
                 AZ_Error("GetEntityTransformInterface", false, "Invalid entity!");
@@ -68,7 +67,7 @@ namespace ROS2
         {
             AZ_TracePrintf(
                 "ROS2FrameComponent",
-                "Setting up %s transfrom between parent %s and child %s to be published %s",
+                "Setting up %s transfrom between parent %s and child %s to be published %s\n",
                 IsDynamic() ? "dynamic" : "static",
                 GetParentFrameID().data(),
                 GetFrameID().data(),
@@ -105,7 +104,6 @@ namespace ROS2
 
     AZStd::string ROS2FrameComponent::GetGlobalFrameName() const
     {
-        // TODO - parametrize this (typically: "odom", "world" and sometimes "map")
         return ROS2Names::GetNamespacedName(GetNamespace(), AZStd::string("odom"));
     }
 
@@ -115,7 +113,7 @@ namespace ROS2
     }
 
     bool ROS2FrameComponent::IsDynamic() const
-    { // TODO - determine by joint type
+    {
         return IsTopLevel();
     }
 
@@ -156,7 +154,7 @@ namespace ROS2
     AZStd::string ROS2FrameComponent::GetNamespace() const
     {
         auto parentFrame = GetParentROS2FrameComponent();
-        AZStd::string parentNamespace("");
+        AZStd::string parentNamespace;
         if (parentFrame != nullptr)
         {
             parentNamespace = parentFrame->GetNamespace();
@@ -180,7 +178,7 @@ namespace ROS2
                 ec->Class<ROS2FrameComponent>("ROS2 Frame", "[ROS2 Frame component]")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->DataElement(
                         AZ::Edit::UIHandlers::Default,
                         &ROS2FrameComponent::m_namespaceConfiguration,
@@ -205,13 +203,13 @@ namespace ROS2
 
     void ROS2FrameComponent::GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required)
     {
-        required.push_back(AZ_CRC("TransformService"));
+        required.push_back(AZ_CRC_CE("TransformService"));
     }
 
     ROS2FrameComponent::ROS2FrameComponent() = default;
 
-    ROS2FrameComponent::ROS2FrameComponent(AZStd::string frameId)
-        : m_frameName(AZStd::move(frameId))
+    ROS2FrameComponent::ROS2FrameComponent(const AZStd::string& frameId)
+        : m_frameName(frameId)
     {
     }
 } // namespace ROS2

Gems/ROS2/Code/Source/Frame/ROS2Transform.cpp

@@ -6,9 +6,9 @@
  *
  */
 
-#include "ROS2/Frame/ROS2Transform.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/Utilities/ROS2Conversions.h"
+#include <ROS2/Frame/ROS2Transform.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/Utilities/ROS2Conversions.h>
 #include <tf2_ros/qos.hpp>
 #include <tf2_ros/transform_broadcaster.h>
 

Gems/ROS2/Code/Source/GNSS/GNSSFormatConversions.cpp

@@ -18,20 +18,11 @@ constexpr double secondEccentrictySquared =
 // Based on http://wiki.gis.com/wiki/index.php/Geodetic_system
 namespace ROS2::GNSS
 {
-    float Rad2Deg(float rad)
-    {
-        return rad * 180.0f / AZ::Constants::Pi;
-    }
-
-    float Deg2Rad(float deg)
-    {
-        return deg * AZ::Constants::Pi / 180.0f;
-    }
 
     AZ::Vector3 WGS84ToECEF(const AZ::Vector3& latitudeLongitudeAltitude)
     {
-        const double latitudeRad = Deg2Rad(latitudeLongitudeAltitude.GetX());
-        const double longitudeRad = Deg2Rad(latitudeLongitudeAltitude.GetY());
+        const double latitudeRad = AZ::DegToRad(latitudeLongitudeAltitude.GetX());
+        const double longitudeRad = AZ::DegToRad(latitudeLongitudeAltitude.GetY());
         const double altitude = latitudeLongitudeAltitude.GetZ();
 
         const double helper = AZStd::sqrt(1.0f - firstEccentricitySquared * AZStd::sin(latitudeRad) * AZStd::sin(latitudeRad));
@@ -48,8 +39,8 @@ namespace ROS2::GNSS
         const AZ::Vector3 referencePointInECEF = WGS84ToECEF(referenceLatitudeLongitudeAltitude);
         const AZ::Vector3 pointToReferencePointECEF = ECEFPoint - referencePointInECEF;
 
-        const float referenceLatitudeRad = Deg2Rad(referenceLatitudeLongitudeAltitude.GetX());
-        const float referenceLongitudeRad = Deg2Rad(referenceLatitudeLongitudeAltitude.GetY());
+        const float referenceLatitudeRad = AZ::DegToRad(referenceLatitudeLongitudeAltitude.GetX());
+        const float referenceLongitudeRad = AZ::DegToRad(referenceLatitudeLongitudeAltitude.GetY());
 
         return {
             -AZStd::sin(referenceLongitudeRad) * pointToReferencePointECEF.GetX() +
@@ -67,8 +58,8 @@ namespace ROS2::GNSS
     {
         AZ::Vector3 referenceECEF = WGS84ToECEF(referenceLatitudeLongitudeAltitude);
 
-        const float referenceLatitudeRad = Deg2Rad(referenceLatitudeLongitudeAltitude.GetX());
-        const float referenceLongitudeRad = Deg2Rad(referenceLatitudeLongitudeAltitude.GetY());
+        const float referenceLatitudeRad = AZ::DegToRad(referenceLatitudeLongitudeAltitude.GetX());
+        const float referenceLongitudeRad = AZ::DegToRad(referenceLatitudeLongitudeAltitude.GetY());
         const float e = ENUPoint.GetX();
         const float n = ENUPoint.GetY();
         const float u = ENUPoint.GetZ();
@@ -110,7 +101,7 @@ namespace ROS2::GNSS
         const double longitude = AZStd::atan2(y, x);
         const double altitude = U * (1.0f - earthSemiminorAxis * earthSemiminorAxis / (earthSemimajorAxis * V));
 
-        return { Rad2Deg(static_cast<float>(latitude)), Rad2Deg(static_cast<float>(longitude)), static_cast<float>(altitude) };
+        return { AZ::RadToDeg(static_cast<float>(latitude)), AZ::RadToDeg(static_cast<float>(longitude)), static_cast<float>(altitude) };
     }
 
 } // namespace ROS2::GNSS

Gems/ROS2/Code/Source/GNSS/GNSSFormatConversions.h

@@ -8,15 +8,10 @@
 
 #pragma once
 
-#include "AzCore/Math/Matrix4x4.h"
+#include <AzCore/Math/Matrix4x4.h>
 
 namespace ROS2::GNSS
 {
-    //! Converts radians to degrees
-    float Rad2Deg(float rad);
-
-    //! Converts degrees to radians
-    float Deg2Rad(float deg);
 
     //! Converts point in 1984 World Geodetic System (GS84) to Earth Centred Earth Fixed (ECEF)
     //! @param latitudeLongitudeAltitude - point's latitude, longitude and altitude as 3d vector.

Gems/ROS2/Code/Source/GNSS/ROS2GNSSSensorComponent.cpp

@@ -6,12 +6,12 @@
  *
  */
 
-#include "GNSS/ROS2GNSSSensorComponent.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/ROS2GemUtilities.h"
-#include "ROS2/Utilities/ROS2Conversions.h"
-#include "ROS2/Utilities/ROS2Names.h"
+#include "ROS2GNSSSensorComponent.h"
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/ROS2GemUtilities.h>
+#include <ROS2/Utilities/ROS2Conversions.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 #include <AzCore/Math/Matrix4x4.h>
 #include <AzCore/Serialization/EditContext.h>
@@ -41,7 +41,7 @@ namespace ROS2
                 ec->Class<ROS2GNSSSensorComponent>("ROS2 GNSS Sensor", "GNSS sensor component")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->DataElement(
                         AZ::Edit::UIHandlers::Default,
                         &ROS2GNSSSensorComponent::m_gnssOriginLatitudeDeg,

Gems/ROS2/Code/Source/GNSS/ROS2GNSSSensorComponent.h

@@ -7,9 +7,9 @@
  */
 #pragma once
 
-#include "ROS2/Sensor/ROS2SensorComponent.h"
 #include <AzCore/Math/Transform.h>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <ROS2/Sensor/ROS2SensorComponent.h>
 #include <rclcpp/publisher.hpp>
 #include <sensor_msgs/msg/nav_sat_fix.hpp>
 
@@ -26,15 +26,21 @@ namespace ROS2
         ROS2GNSSSensorComponent();
         ~ROS2GNSSSensorComponent() = default;
         static void Reflect(AZ::ReflectContext* context);
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
 
     private:
         float m_gnssOriginLatitudeDeg = 0.0f;
         float m_gnssOriginLongitudeDeg = 0.0f;
         float m_gnssOriginAltitude = 0.0f;
 
+        //////////////////////////////////////////////////////////////////////////
+        // ROS2SensorComponent overrides
         void FrequencyTick() override;
+        //////////////////////////////////////////////////////////////////////////
 
         AZ::Transform GetCurrentPose() const;
 

Gems/ROS2/Code/Source/Imu/ROS2ImuSensorComponent.cpp

@@ -6,11 +6,11 @@
  *
  */
 
-#include "Imu/ROS2ImuSensorComponent.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/Utilities/ROS2Conversions.h"
-#include "ROS2/Utilities/ROS2Names.h"
+#include "ROS2ImuSensorComponent.h"
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/Utilities/ROS2Conversions.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 #include <AzCore/Component/Entity.h>
 #include <AzCore/Script/ScriptTimePoint.h>
@@ -36,7 +36,7 @@ namespace ROS2
                 ec->Class<ROS2ImuSensorComponent>("ROS2 Imu Sensor", "Imu sensor component")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"));
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"));
             }
         }
     }

Gems/ROS2/Code/Source/Imu/ROS2ImuSensorComponent.h

@@ -7,16 +7,16 @@
  */
 #pragma once
 
-#include "ROS2/Sensor/ROS2SensorComponent.h"
 #include <AzCore/Math/Transform.h>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <ROS2/Sensor/ROS2SensorComponent.h>
 #include <rclcpp/publisher.hpp>
 #include <sensor_msgs/msg/imu.hpp>
 
 namespace ROS2
 {
     //! An IMU (Inertial Measurement Unit) sensor Component.
-    //! IMUs typically include gyroscopes, accelerometers and magnetometers. This Component encapsulates data
+    //! IMUs typically include gyroscopes, accelerometers and magnetometers. This component encapsulates data
     //! acquisition and its publishing to ROS2 ecosystem. IMU Component requires ROS2FrameComponent.
     class ROS2ImuSensorComponent : public ROS2SensorComponent
     {
@@ -25,11 +25,17 @@ namespace ROS2
         ROS2ImuSensorComponent();
         ~ROS2ImuSensorComponent() = default;
         static void Reflect(AZ::ReflectContext* context);
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
 
     private:
+        //////////////////////////////////////////////////////////////////////////
+        // ROS2SensorComponent overrides
         void FrequencyTick() override;
+        //////////////////////////////////////////////////////////////////////////
 
         void InitializeImuMessage();
         double GetCurrentTimeInSec() const;

Gems/ROS2/Code/Source/Lidar/LidarRaycaster.cpp

@@ -21,7 +21,6 @@ namespace ROS2
         m_sceneHandle = handle;
     }
 
-    // A simplified, non-optimized first version. TODO - generalize results (fields)
     AZStd::vector<AZ::Vector3> LidarRaycaster::PerformRaycast(
         const AZ::Vector3& start,
         const AZStd::vector<AZ::Vector3>& directions,
@@ -66,9 +65,9 @@ namespace ROS2
         auto requestResults = sceneInterface->QuerySceneBatch(m_sceneHandle, requests);
         AZ_Assert(requestResults.size() == directions.size(), "request size should be equal to directions size");
         for (int i = 0; i < requestResults.size(); i++)
-        { // TODO - check flag for SceneQuery::ResultFlags::Position
+        {
             const auto& requestResult = requestResults[i];
-            if (requestResult.m_hits.size() > 0)
+            if (!requestResult.m_hits.empty())
             {
                 auto globalHitPoint = requestResult.m_hits[0].m_position;
                 results.push_back(globalToLidarTM.TransformPoint(globalHitPoint)); // Transform back to local frame

Gems/ROS2/Code/Source/Lidar/LidarRaycaster.h

@@ -13,7 +13,6 @@
 #include <AzCore/std/containers/vector.h>
 #include <AzFramework/Physics/PhysicsScene.h>
 
-// TODO - switch to interface
 namespace ROS2
 {
     //! A simple implementation of Lidar operation in terms of raycasting.
@@ -25,7 +24,7 @@ namespace ROS2
         //! @code
         //! auto sceneHandle = AZ::RPI::Scene::GetSceneForEntityId(GetEntityId());
         //! @endcode
-        //! @param scene that will be subject to ray-casting.
+        //! @param handle Scene that will be subject to ray-casting.
         void SetRaycasterScene(const AzPhysics::SceneHandle& handle);
 
         //! Perform raycast against the current scene.
@@ -38,8 +37,6 @@ namespace ROS2
         //! @param ignoredLayerIndex Index of collision layer to be ignored
         //! @return Hits of raycast. The returned vector size can be anything between zero and size of directions.
         //! No hits further than distance will be reported.
-        // TODO - different starting points for rays, distance from reference point, noise models, rotating mirror sim, other
-        // TODO - customized settings. Encapsulate in lidar definition and pass in constructor, update transform.
         AZStd::vector<AZ::Vector3> PerformRaycast(
             const AZ::Vector3& start,
             const AZStd::vector<AZ::Vector3>& directions,

Gems/ROS2/Code/Source/Lidar/LidarTemplate.h

@@ -23,21 +23,29 @@ namespace ROS2
         AZ_TYPE_INFO(LidarTemplate, "{9E9EF583-733D-4450-BBA0-ADD4D1BEFBF2}");
         static void Reflect(AZ::ReflectContext* context);
 
-        // TODO - implement more models. Add at least one realistic model.
-        enum LidarModel
+        enum class LidarModel
         {
             Generic3DLidar
         };
 
         LidarModel m_model;
+        //! Name of lidar template
         AZStd::string m_name;
+        //! Minimum horizontal angle (altitude of the ray), in degrees
         float m_minHAngle = 0.0f;
+        //! Maximum horizontal angle (altitude of the ray), in degrees
         float m_maxHAngle = 0.0f;
+        //! Minimum vertical angle (azimuth of the ray), in degrees
         float m_minVAngle = 0.0f;
+        //! Maximum vertical angle (azimuth of the ray), in degrees
         float m_maxVAngle = 0.0f;
+        //! Number of lasers layers (resolution in horizontal direction)
         unsigned int m_layers = 0;
+        //! Resolution in vertical direction
         unsigned int m_numberOfIncrements = 0;
+        //! Maximum range of simulated LiDAR
         float m_maxRange = 0.0f;
+        //! Adds point with maximum range when ray does not hit obstacle
         bool m_addPointsAtMax = false;
     };
 } // namespace ROS2

Gems/ROS2/Code/Source/Lidar/LidarTemplateUtils.cpp

@@ -18,11 +18,11 @@ namespace ROS2
 {
     LidarTemplate LidarTemplateUtils::GetTemplate(LidarTemplate::LidarModel model)
     {
-        static std::unordered_map<LidarTemplate::LidarModel, LidarTemplate> templates;
+        static AZStd::unordered_map<LidarTemplate::LidarModel, LidarTemplate> templates;
 
         if (templates.empty())
         {
-            LidarTemplate generic3DLidar = { /*.m_model = */ LidarTemplate::Generic3DLidar,
+            LidarTemplate generic3DLidar = { /*.m_model = */ LidarTemplate::LidarModel::Generic3DLidar,
                                              /*.m_name = */ "GenericLidar",
                                              /*.m_minHAngle = */ -180.0f,
                                              /*.m_maxHAngle = */ 180.0f,
@@ -31,13 +31,13 @@ namespace ROS2
                                              /*.m_layers = */ 24,
                                              /*.m_numberOfIncrements = */ 924,
                                              /*.m_maxRange = */ 100.0f };
-            templates[LidarTemplate::Generic3DLidar] = generic3DLidar;
+            templates[LidarTemplate::LidarModel::Generic3DLidar] = generic3DLidar;
         }
 
         auto it = templates.find(model);
         if (it == templates.end())
         {
-            return LidarTemplate(); // TODO - handle it
+            return LidarTemplate();
         }
 
         return it->second;
@@ -48,7 +48,6 @@ namespace ROS2
         return t.m_layers * t.m_numberOfIncrements;
     }
 
-    // TODO - lidars in reality do not have uniform distributions - populating needs to be defined per model
     AZStd::vector<AZ::Vector3> LidarTemplateUtils::PopulateRayDirections(
         const LidarTemplate& lidarTemplate, const AZ::Vector3& rootRotation)
     {
@@ -66,7 +65,6 @@ namespace ROS2
         {
             for (int layer = 0; layer < lidarTemplate.m_layers; layer++)
             {
-                // TODO: also include roll. Move to quaternions to avoid abnormalities
                 const float pitch = minVertAngle + layer * verticalStep + rootRotation.GetY();
                 const float yaw = minHorAngle + incr * horizontalStep + rootRotation.GetZ();
 

Gems/ROS2/Code/Source/Lidar/LidarTemplateUtils.h

@@ -14,16 +14,15 @@
 namespace ROS2
 {
     //! Utility class for Lidar model computations.
-    class LidarTemplateUtils
+    namespace LidarTemplateUtils
     {
-    public:
-        static LidarTemplate GetTemplate(LidarTemplate::LidarModel model);
-        static size_t TotalPointCount(const LidarTemplate& t);
+        LidarTemplate GetTemplate(LidarTemplate::LidarModel model);
+        size_t TotalPointCount(const LidarTemplate& t);
 
         //! Compute ray directions based on lidar model and rotation.
         //! @param model Lidar model to use. Note that different models will produce different number of rays.
         //! @param rootRotation Root rotation as Euler angles in radians.
         //! @return All ray directions which can be used to perform ray-casting simulation of lidar operation.
-        static AZStd::vector<AZ::Vector3> PopulateRayDirections(const LidarTemplate& lidarTemplate, const AZ::Vector3& rootRotation);
-    };
+        AZStd::vector<AZ::Vector3> PopulateRayDirections(const LidarTemplate& lidarTemplate, const AZ::Vector3& rootRotation);
+    }; // namespace LidarTemplateUtils
 } // namespace ROS2

Gems/ROS2/Code/Source/Lidar/ROS2LidarSensorComponent.cpp

@@ -6,11 +6,6 @@
  *
  */
 
-#include "Lidar/ROS2LidarSensorComponent.h"
-#include "Lidar/LidarTemplateUtils.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/Utilities/ROS2Names.h"
 #include <Atom/RPI.Public/AuxGeom/AuxGeomFeatureProcessorInterface.h>
 #include <Atom/RPI.Public/RPISystemInterface.h>
 #include <Atom/RPI.Public/Scene.h>
@@ -18,6 +13,11 @@
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
 #include <AzFramework/Physics/PhysicsSystem.h>
+#include <Lidar/LidarTemplateUtils.h>
+#include <Lidar/ROS2LidarSensorComponent.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 namespace ROS2
 {
@@ -43,11 +43,10 @@ namespace ROS2
                 ec->Class<ROS2LidarSensorComponent>("ROS2 Lidar Sensor", "Lidar sensor component")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->DataElement(AZ::Edit::UIHandlers::ComboBox, &ROS2LidarSensorComponent::m_lidarModel, "Lidar Model", "Lidar model")
                     ->Attribute(AZ::Edit::Attributes::ChangeNotify, &ROS2LidarSensorComponent::OnLidarModelSelected)
                     ->EnumAttribute(LidarTemplate::LidarModel::Generic3DLidar, "Generic Lidar")
-                    // TODO - show lidar template field values (read only) - see Reflect for LidarTemplate
                     ->DataElement(
                         AZ::Edit::UIHandlers::EntityId,
                         &ROS2LidarSensorComponent::m_lidarParameters,
@@ -70,7 +69,7 @@ namespace ROS2
 
     bool ROS2LidarSensorComponent::IsConfigurationVisible() const
     {
-        return m_lidarModel == LidarTemplate::Generic3DLidar;
+        return m_lidarModel == LidarTemplate::LidarModel::Generic3DLidar;
     }
 
     AZ::Crc32 ROS2LidarSensorComponent::OnLidarModelSelected()
@@ -85,7 +84,7 @@ namespace ROS2
         AZStd::string type = Internal::kPointCloudType;
         pc.m_type = type;
         pc.m_topic = "pc";
-        m_sensorConfiguration.m_frequency = 10; // TODO - dependent on lidar type
+        m_sensorConfiguration.m_frequency = 10;
         m_sensorConfiguration.m_publishersConfigurations.insert(AZStd::make_pair(type, pc));
     }
 
@@ -153,7 +152,7 @@ namespace ROS2
     void ROS2LidarSensorComponent::FrequencyTick()
     {
         float distance = m_lidarParameters.m_maxRange;
-        auto entityTransform = GetEntity()->FindComponent<AzFramework::TransformComponent>(); // TODO - go through ROS2Frame
+        auto entityTransform = GetEntity()->FindComponent<AzFramework::TransformComponent>();
         const auto directions =
             LidarTemplateUtils::PopulateRayDirections(m_lidarParameters, entityTransform->GetWorldTM().GetEulerRadians());
         AZ::Vector3 start = entityTransform->GetWorldTM().GetTranslation();
@@ -174,7 +173,6 @@ namespace ROS2
         { // Store points for visualisation purposes, in global frame
             auto localToWorldTM = entityTransform->GetWorldTM();
 
-            // TODO - improve performance
             m_visualisationPoints = m_lastScanResults;
             for (AZ::Vector3& point : m_visualisationPoints)
             {
@@ -188,13 +186,12 @@ namespace ROS2
         message.header.stamp = ROS2Interface::Get()->GetROSTimestamp();
         message.height = 1;
         message.width = m_lastScanResults.size();
-        message.point_step = sizeof(AZ::Vector3); // TODO - Point Fields can be custom
+        message.point_step = sizeof(AZ::Vector3);
         message.row_step = message.width * message.point_step;
 
-        // TODO - a list of supported fields should be returned by lidar implementation
-        std::vector<std::string> point_field_names = { "x", "y", "z" };
+        AZStd::array<const char*, 3> point_field_names = { "x", "y", "z" };
         for (int i = 0; i < point_field_names.size(); i++)
-        { // TODO - placeholder impl
+        {
             sensor_msgs::msg::PointField pf;
             pf.name = point_field_names[i];
             pf.offset = i * 4;

Gems/ROS2/Code/Source/Lidar/ROS2LidarSensorComponent.h

@@ -7,12 +7,12 @@
  */
 #pragma once
 
-#include "Lidar/LidarRaycaster.h"
-#include "Lidar/LidarTemplate.h"
-#include "Lidar/LidarTemplateUtils.h"
-#include "ROS2/Sensor/ROS2SensorComponent.h"
+#include "LidarRaycaster.h"
+#include "LidarTemplate.h"
+#include "LidarTemplateUtils.h"
 #include <Atom/RPI.Public/AuxGeom/AuxGeomDraw.h>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <ROS2/Sensor/ROS2SensorComponent.h>
 #include <rclcpp/publisher.hpp>
 #include <sensor_msgs/msg/point_cloud2.hpp>
 
@@ -20,9 +20,8 @@ namespace ROS2
 {
     //! Lidar sensor Component.
     //! Lidars (Light Detection and Ranging) emit laser light and measure it after reflection.
-    //! Lidar Component allows customization of lidar type and behavior and encapsulates both simulation.
-    //! and data publishing. Lidar Component requires ROS2FrameComponent.
-    // TODO - Add selection of implementation choice (PhysX, GPU, other), noise
+    //! Lidar Component allows customization of lidar type and behavior and encapsulates both simulation
+    //! and data publishing. It requires ROS2FrameComponent.
     class ROS2LidarSensorComponent : public ROS2SensorComponent
     {
     public:
@@ -30,19 +29,25 @@ namespace ROS2
         ROS2LidarSensorComponent();
         ~ROS2LidarSensorComponent() = default;
         static void Reflect(AZ::ReflectContext* context);
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
 
     private:
+        //////////////////////////////////////////////////////////////////////////
+        // ROS2SensorComponent overrides
         void FrequencyTick() override;
         void Visualise() override;
+        //////////////////////////////////////////////////////////////////////////
         void SetPhysicsScene();
 
         AZ::Crc32 OnLidarModelSelected();
         bool IsConfigurationVisible() const;
 
-        LidarTemplate::LidarModel m_lidarModel = LidarTemplate::Generic3DLidar;
-        LidarTemplate m_lidarParameters = LidarTemplateUtils::GetTemplate(LidarTemplate::Generic3DLidar);
+        LidarTemplate::LidarModel m_lidarModel = LidarTemplate::LidarModel::Generic3DLidar;
+        LidarTemplate m_lidarParameters = LidarTemplateUtils::GetTemplate(LidarTemplate::LidarModel::Generic3DLidar);
         LidarRaycaster m_lidarRaycaster;
         std::shared_ptr<rclcpp::Publisher<sensor_msgs::msg::PointCloud2>> m_pointCloudPublisher;
 
@@ -52,7 +57,6 @@ namespace ROS2
 
         AZStd::vector<AZ::Vector3> m_lastScanResults;
 
-        // TODO - change to AzPhysics::CollisionLayer, use mask instead of single layer
         unsigned int m_ignoredLayerIndex = 0;
         bool m_ignoreLayer = false;
     };

Gems/ROS2/Code/Source/Manipulator/MotorizedJointComponent.cpp

@@ -6,12 +6,12 @@
  *
  */
 
-#include "ROS2/Manipulator/MotorizedJointComponent.h"
-#include "AzFramework/Physics/Components/SimulatedBodyComponentBus.h"
 #include <AzCore/Component/Entity.h>
 #include <AzCore/Component/TransformBus.h>
 #include <AzCore/Serialization/EditContext.h>
+#include <AzFramework/Physics/Components/SimulatedBodyComponentBus.h>
 #include <AzFramework/Physics/RigidBodyBus.h>
+#include <ROS2/Manipulator/MotorizedJointComponent.h>
 
 namespace ROS2
 {
@@ -57,7 +57,7 @@ namespace ROS2
             {
                 ec->Class<MotorizedJointComponent>("MotorizedJointComponent", "MotorizedJointComponent")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "MotorizedJointComponent")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->Attribute(AZ::Edit::Attributes::Category, "MotorizedJointComponent")
                     ->DataElement(
                         AZ::Edit::UIHandlers::Default,
@@ -130,7 +130,7 @@ namespace ROS2
             m_setpoint = m_sinDC + m_sinAmplitude * AZ::Sin(m_sinFreq * time.GetSeconds());
         }
         const float control_position_error = (m_setpoint + m_zeroOffset) - measurement;
-        m_error = control_position_error; // TODO decide if we want to expose this control error.
+        m_error = control_position_error;
 
         if (m_debugDrawEntity.IsValid())
         {
@@ -165,7 +165,7 @@ namespace ROS2
         {
             AZ_Printf(
                 "MotorizedJointComponent",
-                " %s | pos: %f | err: %f | cntrl : %f | set : %f |",
+                " %s | pos: %f | err: %f | cntrl : %f | set : %f |\n",
                 GetEntity()->GetName().c_str(),
                 measurement,
                 control_position_error,
@@ -217,9 +217,7 @@ namespace ROS2
             deltaTime = AZStd::min(deltaTime, 0.1f); // limit max force for small FPS
             if (m_linear)
             {
-                // TODO decide which approach is better here.
                 ApplyLinVelRigidBodyImpulse(velocity, deltaTime);
-                // ApplyLinVelRigidBody(velocity, deltaTime);
             }
         }
     }

Gems/ROS2/Code/Source/Odometry/ROS2OdometrySensorComponent.cpp

@@ -7,13 +7,13 @@
  */
 
 #include "ROS2OdometrySensorComponent.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/Utilities/ROS2Conversions.h"
-#include "ROS2/Utilities/ROS2Names.h"
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
 #include <AzFramework/Physics/RigidBodyBus.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/Utilities/ROS2Conversions.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 namespace ROS2
 {
@@ -33,7 +33,7 @@ namespace ROS2
                 ec->Class<ROS2OdometrySensorComponent>("ROS2 Odometry Sensor", "Odometry sensor component")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"));
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"));
             }
         }
     }

Gems/ROS2/Code/Source/Odometry/ROS2OdometrySensorComponent.h

@@ -7,9 +7,9 @@
  */
 #pragma once
 
-#include "ROS2/Sensor/ROS2SensorComponent.h"
 #include <AzCore/Math/Transform.h>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <ROS2/Sensor/ROS2SensorComponent.h>
 #include <nav_msgs/msg/odometry.hpp>
 #include <rclcpp/publisher.hpp>
 
@@ -27,11 +27,17 @@ namespace ROS2
         ~ROS2OdometrySensorComponent() = default;
         static void GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required);
         static void Reflect(AZ::ReflectContext* context);
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
 
     private:
+        //////////////////////////////////////////////////////////////////////////
+        // ROS2SensorComponent overrides
         void FrequencyTick() override;
+        //////////////////////////////////////////////////////////////////////////
 
         std::shared_ptr<rclcpp::Publisher<nav_msgs::msg::Odometry>> m_odometryPublisher;
         nav_msgs::msg::Odometry m_odometryMsg;

Gems/ROS2/Code/Source/ROS2EditorModule.cpp

@@ -5,16 +5,16 @@
  * SPDX-License-Identifier: Apache-2.0 OR MIT
  *
  */
-#include "RobotImporter/ROS2RobotImporterEditorSystemComponent.h"
 #include <ROS2EditorSystemComponent.h>
 #include <ROS2ModuleInterface.h>
+#include <RobotImporter/ROS2RobotImporterEditorSystemComponent.h>
 
 namespace ROS2
 {
     class ROS2EditorModule : public ROS2ModuleInterface
     {
     public:
-        AZ_RTTI(ROS2EditorModule, "{e23a1379-787c-481e-ad83-c0e04a3d06fe}", ROS2ModuleInterface);
+        AZ_RTTI(ROS2EditorModule, "{3DDFC98F-D1CC-4658-BAF8-2CC34A9D39F3}", ROS2ModuleInterface);
         AZ_CLASS_ALLOCATOR(ROS2EditorModule, AZ::SystemAllocator, 0);
 
         ROS2EditorModule()

Gems/ROS2/Code/Source/ROS2EditorSystemComponent.h

@@ -31,8 +31,10 @@ namespace ROS2
         static void GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required);
         static void GetDependentServices(AZ::ComponentDescriptor::DependencyArrayType& dependent);
 
-        // AZ::Component
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
     };
 } // namespace ROS2

Gems/ROS2/Code/Source/ROS2GemUtilities.cpp

@@ -6,9 +6,9 @@
  *
  */
 
-#include "ROS2/ROS2GemUtilities.h"
 #include <AzCore/std/string/regex.h>
 #include <AzToolsFramework/API/EntityCompositionRequestBus.h>
+#include <ROS2/ROS2GemUtilities.h>
 
 namespace ROS2
 {
@@ -31,7 +31,7 @@ namespace ROS2
                 addComponentsOutcome.GetError().c_str());
         }
         const auto& added = addComponentsOutcome.GetValue().at(entityId).m_componentsAdded;
-        if (added.size())
+        if (!added.empty())
         {
             return added.front()->GetId();
         }

Gems/ROS2/Code/Source/ROS2ModuleInterface.h

@@ -7,24 +7,24 @@
  */
 #pragma once
 
-#include "Camera/ROS2CameraSensorComponent.h"
-#include "GNSS/ROS2GNSSSensorComponent.h"
-#include "Imu/ROS2ImuSensorComponent.h"
-#include "Lidar/ROS2LidarSensorComponent.h"
-#include "Odometry/ROS2OdometrySensorComponent.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/Manipulator/MotorizedJointComponent.h"
 #include "ROS2SystemComponent.h"
-#include "RobotControl/Controllers/AckermannController/AckermannControlComponent.h"
-#include "RobotControl/Controllers/RigidBodyController/RigidBodyTwistControlComponent.h"
-#include "RobotControl/ROS2RobotControlComponent.h"
-#include "RobotImporter/ROS2RobotImporterSystemComponent.h"
-#include "Spawner/ROS2SpawnPointComponent.h"
-#include "Spawner/ROS2SpawnerComponent.h"
-#include "VehicleDynamics/VehicleModelComponent.h" // TODO - separate out
-#include "VehicleDynamics/WheelControllerComponent.h" // TODO - separate out
 #include <AzCore/Memory/SystemAllocator.h>
 #include <AzCore/Module/Module.h>
+#include <Camera/ROS2CameraSensorComponent.h>
+#include <GNSS/ROS2GNSSSensorComponent.h>
+#include <Imu/ROS2ImuSensorComponent.h>
+#include <Lidar/ROS2LidarSensorComponent.h>
+#include <Odometry/ROS2OdometrySensorComponent.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/Manipulator/MotorizedJointComponent.h>
+#include <RobotControl/Controllers/AckermannController/AckermannControlComponent.h>
+#include <RobotControl/Controllers/RigidBodyController/RigidBodyTwistControlComponent.h>
+#include <RobotControl/ROS2RobotControlComponent.h>
+#include <RobotImporter/ROS2RobotImporterSystemComponent.h>
+#include <Spawner/ROS2SpawnPointComponent.h>
+#include <Spawner/ROS2SpawnerComponent.h>
+#include <VehicleDynamics/VehicleModelComponent.h>
+#include <VehicleDynamics/WheelControllerComponent.h>
 
 namespace ROS2
 {

Gems/ROS2/Code/Source/ROS2SystemComponent.cpp

@@ -7,8 +7,8 @@
  */
 #include <ROS2SystemComponent.h>
 
-#include "ROS2/Communication/QoS.h"
-#include "ROS2/Communication/TopicConfiguration.h"
+#include <ROS2/Communication/QoS.h>
+#include <ROS2/Communication/TopicConfiguration.h>
 
 #include <Atom/RPI.Public/Pass/PassSystemInterface.h>
 
@@ -33,7 +33,7 @@ namespace ROS2
             {
                 ec->Class<ROS2SystemComponent>("ROS2 System Component", "[Description of functionality provided by this System Component]")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("System"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("System"))
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
                     ->Attribute(AZ::Edit::Attributes::AutoExpand, true);
             }
@@ -140,9 +140,6 @@ namespace ROS2
         if (rclcpp::ok())
         {
             m_simulationClock.Tick();
-
-            // TODO - this can be in another thread and done with a higher resolution for less latency.
-            // TODO - callbacks will be called in the spinning thread (here, the main thread).
             m_executor->spin_some();
         }
     }

Gems/ROS2/Code/Source/ROS2SystemComponent.h

@@ -7,12 +7,12 @@
  */
 #pragma once
 
-#include "Clock/SimulationClock.h"
-#include "ROS2/ROS2Bus.h"
 #include <Atom/RPI.Public/Pass/PassSystemInterface.h>
 #include <AzCore/Component/Component.h>
 #include <AzCore/Component/TickBus.h>
 #include <AzCore/std/smart_ptr/unique_ptr.h>
+#include <Clock/SimulationClock.h>
+#include <ROS2/ROS2Bus.h>
 #include <builtin_interfaces/msg/time.hpp>
 #include <memory>
 #include <rclcpp/rclcpp.hpp>
@@ -40,14 +40,12 @@ namespace ROS2
         ROS2SystemComponent();
         ~ROS2SystemComponent();
 
-        //! @see ROS2Requests::GetNode()
+        //////////////////////////////////////////////////////////////////////////
+        // ROS2RequestBus::Handler overrides
         std::shared_ptr<rclcpp::Node> GetNode() const override;
-
-        //! @see ROS2Requests::GetROSTimestamp()
         builtin_interfaces::msg::Time GetROSTimestamp() const override;
-
-        // TODO - rethink ownership of this one. It needs to be a singleton-like behavior, but not necessarily here
         void BroadcastTransform(const geometry_msgs::msg::TransformStamped& t, bool isDynamic) const override;
+        //////////////////////////////////////////////////////////////////////////
 
     protected:
         ////////////////////////////////////////////////////////////////////////
@@ -68,7 +66,7 @@ namespace ROS2
         AZStd::unique_ptr<tf2_ros::TransformBroadcaster> m_dynamicTFBroadcaster;
         AZStd::unique_ptr<tf2_ros::StaticTransformBroadcaster> m_staticTFBroadcaster;
         SimulationClock m_simulationClock;
-        //! Used for loading the pass templates of the ROS2 gem.
+        //! Load the pass templates of the ROS2 gem.
         void LoadPassTemplateMappings();
         AZ::RPI::PassSystemInterface::OnReadyLoadTemplatesEvent::Handler m_loadTemplatesHandler;
     };

Gems/ROS2/Code/Source/RobotControl/Ackermann/AckermannSubscriptionHandler.cpp

@@ -7,8 +7,8 @@
  */
 
 #include "AckermannSubscriptionHandler.h"
-#include "ROS2/RobotControl/Ackermann/AckermannBus.h"
-#include "ROS2/RobotControl/Ackermann/AckermannCommandStruct.h"
+#include <ROS2/RobotControl/Ackermann/AckermannBus.h>
+#include <ROS2/RobotControl/Ackermann/AckermannCommandStruct.h>
 
 namespace ROS2
 {

Gems/ROS2/Code/Source/RobotControl/Ackermann/AckermannSubscriptionHandler.h

@@ -7,7 +7,7 @@
  */
 #pragma once
 
-#include "ROS2/RobotControl/ControlSubscriptionHandler.h"
+#include <ROS2/RobotControl/ControlSubscriptionHandler.h>
 #include <ackermann_msgs/msg/ackermann_drive.hpp>
 
 namespace ROS2

Gems/ROS2/Code/Source/RobotControl/ControlConfiguration.cpp

@@ -6,8 +6,8 @@
  *
  */
 
-#include "ROS2/RobotControl/ControlConfiguration.h"
 #include <AzCore/Serialization/EditContext.h>
+#include <ROS2/RobotControl/ControlConfiguration.h>
 
 namespace ROS2
 {

Gems/ROS2/Code/Source/RobotControl/Controllers/AckermannController/AckermannControlComponent.cpp

@@ -7,10 +7,10 @@
  */
 
 #include "AckermannControlComponent.h"
-#include "ROS2/VehicleDynamics/VehicleInputControlBus.h"
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
 #include <AzFramework/Physics/RigidBodyBus.h>
+#include <ROS2/VehicleDynamics/VehicleInputControlBus.h>
 
 namespace ROS2
 {
@@ -23,7 +23,7 @@ namespace ROS2
             {
                 ec->Class<AckermannControlComponent>("Ackermann Control", "Relays Ackermann commands to vehicle inputs")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2");
             }
         }
@@ -40,8 +40,8 @@ namespace ROS2
 
     void AckermannControlComponent::GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required)
     {
-        required.push_back(AZ_CRC("ROS2RobotControl"));
-        required.push_back(AZ_CRC("VehicleModelService"));
+        required.push_back(AZ_CRC_CE("ROS2RobotControl"));
+        required.push_back(AZ_CRC_CE("VehicleModelService"));
     }
 
     void AckermannControlComponent::AckermannReceived(const AckermannCommandStruct& acs)

Gems/ROS2/Code/Source/RobotControl/Controllers/AckermannController/AckermannControlComponent.h

@@ -7,8 +7,8 @@
  */
 #pragma once
 
-#include "ROS2/RobotControl/Ackermann/AckermannBus.h"
 #include <AzCore/Component/Component.h>
+#include <ROS2/RobotControl/Ackermann/AckermannBus.h>
 
 namespace ROS2
 {
@@ -21,13 +21,18 @@ namespace ROS2
         AZ_COMPONENT(AckermannControlComponent, "{16EC2F18-F579-414C-8B3B-DB47078729BC}", AZ::Component);
         AckermannControlComponent() = default;
 
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
         static void GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required);
         static void Reflect(AZ::ReflectContext* context);
 
     private:
-        //! Simply relay received commands to vehicle dynamics input system
+        //////////////////////////////////////////////////////////////////////////
+        // AckermannNotificationBus::Handler overrides
         void AckermannReceived(const AckermannCommandStruct& angular) override;
+        //////////////////////////////////////////////////////////////////////////
     };
 } // namespace ROS2

Gems/ROS2/Code/Source/RobotControl/Controllers/RigidBodyController/RigidBodyTwistControlComponent.cpp

@@ -24,7 +24,7 @@ namespace ROS2
             {
                 ec->Class<RigidBodyTwistControlComponent>("Rigid Body Twist Control", "Simple control through RigidBody")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2");
             }
         }
@@ -42,7 +42,7 @@ namespace ROS2
 
     void RigidBodyTwistControlComponent::GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required)
     {
-        required.push_back(AZ_CRC("ROS2RobotControl"));
+        required.push_back(AZ_CRC_CE("ROS2RobotControl"));
         required.push_back(AZ_CRC_CE("PhysicsRigidBodyService"));
     }
 

Gems/ROS2/Code/Source/RobotControl/Controllers/RigidBodyController/RigidBodyTwistControlComponent.h

@@ -7,8 +7,8 @@
  */
 #pragma once
 
-#include "ROS2/RobotControl/Twist/TwistBus.h"
 #include <AzCore/Component/Component.h>
+#include <ROS2/RobotControl/Twist/TwistBus.h>
 
 namespace ROS2
 {
@@ -22,13 +22,19 @@ namespace ROS2
         AZ_COMPONENT(RigidBodyTwistControlComponent, "{D994FE1A-AA6A-42B9-8B8E-B3B375891F5B}", AZ::Component);
         RigidBodyTwistControlComponent() = default;
 
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
+
         static void GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required);
         static void Reflect(AZ::ReflectContext* context);
 
     private:
-        //! Simplest approach: To imitate the steering, current linear and angular velocities of a rigid body are overwritten with inputs
+        //////////////////////////////////////////////////////////////////////////
+        // TwistNotificationBus::Handler overrides
         void TwistReceived(const AZ::Vector3& linear, const AZ::Vector3& angular) override;
+        //////////////////////////////////////////////////////////////////////////
     };
 } // namespace ROS2

Gems/ROS2/Code/Source/RobotControl/ROS2RobotControlComponent.cpp

@@ -6,14 +6,15 @@
  *
  */
 
-#include "RobotControl/ROS2RobotControlComponent.h"
-#include "Ackermann/AckermannSubscriptionHandler.h"
-#include "Twist/TwistSubscriptionHandler.h"
+#include "ROS2RobotControlComponent.h"
 #include <AzCore/Component/Entity.h>
 #include <AzCore/Debug/Trace.h>
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <RobotControl/Ackermann/AckermannSubscriptionHandler.h>
+#include <RobotControl/ROS2RobotControlComponent.h>
+#include <RobotControl/Twist/TwistSubscriptionHandler.h>
 
 namespace ROS2
 {
@@ -21,11 +22,10 @@ namespace ROS2
     {
         switch (m_controlConfiguration.m_steering)
         {
-        case ControlConfiguration::Twist:
+        case ControlConfiguration::Steering::Twist:
             m_subscriptionHandler = AZStd::make_unique<TwistSubscriptionHandler>();
             break;
-        case ControlConfiguration::Ackermann:
-            // TODO add ackermann
+        case ControlConfiguration::Steering::Ackermann:
             m_subscriptionHandler = AZStd::make_unique<AckermannSubscriptionHandler>();
             break;
         default:
@@ -64,7 +64,7 @@ namespace ROS2
                 ec->Class<ROS2RobotControlComponent>("ROS2 Robot control", "Customizable robot control component")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game")) // TODO - "Simulation"?
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->DataElement(
                         AZ::Edit::UIHandlers::Default,
                         &ROS2RobotControlComponent::m_controlConfiguration,
@@ -81,8 +81,7 @@ namespace ROS2
 
     void ROS2RobotControlComponent::GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required)
     {
-        // TODO - also, dependent on current/selected RobotControl implementation for what components are required
-        required.push_back(AZ_CRC("ROS2Frame"));
+        required.push_back(AZ_CRC_CE("ROS2Frame"));
     }
 
     const ControlConfiguration& ROS2RobotControlComponent::GetControlConfiguration() const

Gems/ROS2/Code/Source/RobotControl/ROS2RobotControlComponent.h

@@ -7,11 +7,11 @@
  */
 #pragma once
 
-#include "ROS2/RobotControl/ControlConfiguration.h"
-#include "ROS2/RobotControl/ControlSubscriptionHandler.h"
 #include <AzCore/Component/Component.h>
 #include <AzCore/std/smart_ptr/unique_ptr.h>
 #include <ROS2/Communication/TopicConfiguration.h>
+#include <ROS2/RobotControl/ControlConfiguration.h>
+#include <ROS2/RobotControl/ControlSubscriptionHandler.h>
 
 namespace ROS2
 {
@@ -36,9 +36,11 @@ namespace ROS2
 
         void SetSubscriberConfiguration(const TopicConfiguration& subscriberConfiguration);
 
-        // AZ::Component interface implementation.
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
         static void GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required);
         static void GetProvidedServices(AZ::ComponentDescriptor::DependencyArrayType& provided);
 

Gems/ROS2/Code/Source/RobotControl/Twist/TwistSubscriptionHandler.cpp

@@ -7,8 +7,8 @@
  */
 
 #include "TwistSubscriptionHandler.h"
-#include "ROS2/RobotControl/Twist/TwistBus.h"
-#include "ROS2/Utilities/ROS2Conversions.h"
+#include <ROS2/RobotControl/Twist/TwistBus.h>
+#include <ROS2/Utilities/ROS2Conversions.h>
 
 namespace ROS2
 {

Gems/ROS2/Code/Source/RobotControl/Twist/TwistSubscriptionHandler.h

@@ -7,7 +7,7 @@
  */
 #pragma once
 
-#include "ROS2/RobotControl/ControlSubscriptionHandler.h"
+#include <ROS2/RobotControl/ControlSubscriptionHandler.h>
 #include <geometry_msgs/msg/twist.hpp>
 
 namespace ROS2

Gems/ROS2/Code/Source/RobotImporter/Pages/PrefabMakerPage.cpp

@@ -7,7 +7,7 @@
  */
 
 #include "PrefabMakerPage.h"
-#include "RobotImporter/RobotImporterWidget.h"
+#include <RobotImporter/RobotImporterWidget.h>
 
 namespace ROS2
 {

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

@@ -8,7 +8,7 @@
 
 #pragma once
 
-#include "RobotImporter/ROS2RobotImporterSystemComponent.h"
+#include "ROS2RobotImporterSystemComponent.h"
 #include <AzToolsFramework/Entity/EditorEntityContextBus.h>
 
 namespace ROS2
@@ -29,8 +29,15 @@ namespace ROS2
         ~ROS2RobotImporterEditorSystemComponent() = default;
 
     private:
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
         void Deactivate() override;
+        //////////////////////////////////////////////////////////////////////////
+
+        //////////////////////////////////////////////////////////////////////////
+        // AzToolsFramework::EditorEvents::Bus::Handler overrides
         void NotifyRegisterViews() override;
+        //////////////////////////////////////////////////////////////////////////
     };
 } // namespace ROS2

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

@@ -24,7 +24,7 @@ namespace ROS2
                 ec->Class<ROS2RobotImporterSystemComponent>(
                       "Robot Importer", "Use this component to import robot definition from supported formats")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("System"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("System"))
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
                     ->Attribute(AZ::Edit::Attributes::AutoExpand, true);
             }

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

@@ -27,8 +27,11 @@ namespace ROS2
         virtual ~ROS2RobotImporterSystemComponent() = default;
 
     protected:
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Init() override{};
         void Activate() override{};
         void Deactivate() override{};
+        //////////////////////////////////////////////////////////////////////////
     };
 } // namespace ROS2

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

@@ -10,11 +10,10 @@
 #include <AzCore/IO/Path/Path.h>
 #include <AzCore/Utils/Utils.h>
 
-#include "RobotImporter/RobotImporterWidget.h"
-#include "RobotImporter/RobotImporterWidgetUtils.h"
-#include "RobotImporter/URDF/URDFPrefabMaker.h"
-#include "RobotImporter/URDF/UrdfParser.h"
-#include "RobotImporter/Utils/RobotImporterUtils.h"
+#include "RobotImporterWidget.h"
+#include "URDF/URDFPrefabMaker.h"
+#include "URDF/UrdfParser.h"
+#include "Utils/RobotImporterUtils.h"
 #include <QApplication>
 #include <QScreen>
 #include <QTranslator>
@@ -130,21 +129,21 @@ namespace ROS2
         if (m_parsedUrdf)
         {
             m_urdfAssetsMapping = AZStd::make_shared<Utils::UrdfAssetMap>(Utils::FindAssetsForUrdf(m_meshNames, m_urdfPath));
-            auto colliders_names = Utils::GetMeshesFilenames(m_parsedUrdf->getRoot(), false, true);
-            auto visual_names = Utils::GetMeshesFilenames(m_parsedUrdf->getRoot(), true, false);
-            for (const AZStd::string& mesh_path : m_meshNames)
+            auto collidersNames = Utils::GetMeshesFilenames(m_parsedUrdf->getRoot(), false, true);
+            auto visualNames = Utils::GetMeshesFilenames(m_parsedUrdf->getRoot(), true, false);
+            for (const AZStd::string& meshPath : m_meshNames)
             {
-                const QString mesh_pathqs = QString::fromUtf8(mesh_path.data(), mesh_path.size());
+                const QString meshPathqs = QString::fromUtf8(meshPath.data(), meshPath.size());
                 const QString kNotFound = tr("not found");
 
-                if (m_urdfAssetsMapping->contains(mesh_path))
+                if (m_urdfAssetsMapping->contains(meshPath))
                 {
                     QString type = kNotFound;
-                    QString source_path = kNotFound;
+                    QString sourcePath = kNotFound;
                     auto crc = AZ::Crc32();
                     QString tooltip = kNotFound;
-                    bool visual = visual_names.contains(mesh_path);
-                    bool collider = colliders_names.contains(mesh_path);
+                    bool visual = visualNames.contains(meshPath);
+                    bool collider = collidersNames.contains(meshPath);
                     if (visual && collider)
                     {
                         type = tr("Visual and Collider");
@@ -153,26 +152,26 @@ namespace ROS2
                     {
                         type = tr("Visual");
                     }
-                    else if (visual)
+                    else if (collider)
                     {
                         type = tr("Collider");
                     }
 
-                    if (m_urdfAssetsMapping->contains(mesh_path))
+                    if (m_urdfAssetsMapping->contains(meshPath))
                     {
-                        const auto& asset = m_urdfAssetsMapping->at(mesh_path);
-                        const AZStd::string& product_path = asset.m_availableAssetInfo.m_productAssetRelativePath;
-                        const AZStd::string& resolved_path = asset.m_resolvedUrdfPath.data();
+                        const auto& asset = m_urdfAssetsMapping->at(meshPath);
+                        const AZStd::string& productPath = asset.m_availableAssetInfo.m_productAssetRelativePath;
+                        const AZStd::string& resolvedPath = asset.m_resolvedUrdfPath.data();
 
-                        source_path = QString::fromUtf8(product_path.data(), product_path.size());
+                        sourcePath = QString::fromUtf8(productPath.data(), productPath.size());
                         crc = asset.m_urdfFileCRC;
-                        tooltip = QString::fromUtf8(resolved_path.data(), resolved_path.size());
+                        tooltip = QString::fromUtf8(resolvedPath.data(), resolvedPath.size());
                     }
-                    m_assetPage->ReportAsset(mesh_pathqs, type, source_path, crc, tooltip);
+                    m_assetPage->ReportAsset(meshPathqs, type, sourcePath, crc, tooltip);
                 }
                 else
                 {
-                    m_assetPage->ReportAsset(mesh_pathqs, kNotFound, kNotFound, AZ::Crc32(), kNotFound);
+                    m_assetPage->ReportAsset(meshPathqs, kNotFound, kNotFound, AZ::Crc32(), kNotFound);
                 };
             }
         }
@@ -277,30 +276,29 @@ namespace ROS2
 
     bool RobotImporterWidget::CheckCyclicalDependency(AZ::IO::Path importedPrefabPath)
     {
-        AzFramework::EntityContextId context_id;
-        EBUS_EVENT_RESULT(context_id, AzFramework::EntityIdContextQueryBus, GetOwningContextId);
+        AzFramework::EntityContextId contextId;
+        AzFramework::EntityIdContextQueryBus::BroadcastResult(contextId, &AzFramework::EntityIdContextQueryBus::Events::GetOwningContextId);
 
-        AZ_Printf("CheckCyclicalDependency", "CheckCyclicalDependency %s", importedPrefabPath.Native().data());
-        auto focus_interface = AZ::Interface<AzToolsFramework::Prefab::PrefabFocusInterface>::Get();
+        AZ_Printf("CheckCyclicalDependency", "CheckCyclicalDependency %s\n", importedPrefabPath.Native().c_str());
+        auto focusInterface = AZ::Interface<AzToolsFramework::Prefab::PrefabFocusInterface>::Get();
 
-        if (!focus_interface)
+        if (!focusInterface)
         {
             ReportError(tr("Imported prefab could not be validated.\nImport aborted."));
             return true;
         }
 
-        auto focus_prefab_instance = focus_interface->GetFocusedPrefabInstance(context_id);
+        auto focusedPrefabInstance = focusInterface->GetFocusedPrefabInstance(contextId);
 
-        if (!focus_prefab_instance)
+        if (!focusedPrefabInstance)
         {
             ReportError(tr("Imported prefab could not be validated.\nImport aborted."));
             return true;
         }
 
-        auto focus_prefab_filename = focus_prefab_instance.value().get().GetTemplateSourcePath();
+        auto focusPrefabFilename = focusedPrefabInstance.value().get().GetTemplateSourcePath();
 
-        AZ_Printf("CheckCyclicalDependency", "focus_prefab_filename %s", focus_prefab_filename.Native().data());
-        if (focus_prefab_filename == importedPrefabPath)
+        if (focusPrefabFilename == importedPrefabPath)
         {
             ReportError(
                 tr("Cyclical dependency detected.\nSelected URDF model is currently being edited. Exit prefab edit mode and try again."));
@@ -313,6 +311,6 @@ namespace ROS2
     void RobotImporterWidget::ReportError(const QString& errorMessage)
     {
         QMessageBox::critical(this, QObject::tr("Error"), errorMessage);
-        AZ_Error("RobotImporterWidget", false, errorMessage.toStdString().c_str());
+        AZ_Error("RobotImporterWidget", false, "%s", errorMessage.toUtf8().constData());
     }
 } // namespace ROS2

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

@@ -1,109 +0,0 @@
-/*
- * Copyright (c) Contributors to the Open 3D Engine Project.
- * For complete copyright and license terms please see the LICENSE at the root of this distribution.
- *
- * SPDX-License-Identifier: Apache-2.0 OR MIT
- *
- */
-
-#include "RobotImporterWidgetUtils.h"
-#include <QFileDialog>
-#include <QMessageBox>
-#include <QWidget>
-
-#include <AzCore/Utils/Utils.h>
-
-namespace ROS2::RobotImporterWidgetUtils
-{
-    namespace Internal
-    {
-        AZStd::optional<QString> QueryUserForPath(
-            const AZStd::string& extensionDescription, QFileDialog::FileMode mode, QWidget* parent = nullptr)
-        {
-            QFileDialog importFileDialog(parent);
-            importFileDialog.setDirectory(AZ::Utils::GetProjectPath().c_str());
-            importFileDialog.setFileMode(mode);
-            importFileDialog.setNameFilter(QObject::tr(extensionDescription.c_str()));
-            importFileDialog.setViewMode(QFileDialog::Detail);
-
-            int result = importFileDialog.exec();
-            if (result != QDialog::DialogCode::Accepted)
-            {
-                return AZStd::nullopt;
-            }
-
-            return importFileDialog.selectedFiles().first();
-        }
-
-        ExistingPrefabAction QueryUserForExistingPrefabAction(QWidget* parent = nullptr)
-        {
-            QMessageBox msgBox(parent);
-            msgBox.setWindowTitle(QObject::tr("Prefab file exists"));
-            msgBox.setText(QObject::tr("The prefab file already exists."));
-            msgBox.setInformativeText(QObject::tr("Do you want to overwrite it or save it with another file name?"));
-            msgBox.setStandardButtons(QMessageBox::Save | QMessageBox::Discard | QMessageBox::Cancel);
-            msgBox.setDefaultButton(QMessageBox::Discard);
-            msgBox.setButtonText(QMessageBox::Save, QObject::tr("Overwrite"));
-            msgBox.setButtonText(QMessageBox::Discard, QObject::tr("Save As..."));
-
-            switch (msgBox.exec())
-            {
-            case QMessageBox::Save:
-                return ExistingPrefabAction::Overwrite;
-            case QMessageBox::Discard:
-                return ExistingPrefabAction::CreateWithNewName;
-            default:
-                return ExistingPrefabAction::Cancel;
-            }
-        }
-    } // namespace Internal
-
-    AZStd::optional<AZStd::string> QueryUserForURDFPath(QWidget* parent)
-    {
-        const auto path = Internal::QueryUserForPath("Unified Robot Description Format (*.urdf)", QFileDialog::ExistingFiles);
-        if (!path)
-        {
-            return AZStd::nullopt;
-        }
-
-        if (path->isEmpty())
-        {
-            QMessageBox::critical(parent, QObject::tr("Empty path provided"), QObject::tr("No path was provided. Please try again"));
-            return QueryUserForURDFPath();
-        }
-
-        if (!QFile::exists(path.value()))
-        {
-            QMessageBox::critical(parent, QObject::tr("Does not exist"), QObject::tr("File does not exist. Please try again"));
-            return QueryUserForURDFPath();
-        }
-
-        return path->toUtf8().constData();
-    }
-
-    AZStd::optional<AZ::IO::Path> ValidatePrefabPathExistenceAndQueryUserForNewIfNecessary(const AZ::IO::Path& path, QWidget* parent)
-    {
-        if (!QFile::exists(path.c_str()))
-        {
-            return path;
-        }
-
-        switch (Internal::QueryUserForExistingPrefabAction(parent))
-        {
-        case ExistingPrefabAction::Cancel:
-            return AZStd::nullopt;
-        case ExistingPrefabAction::Overwrite:
-            return path;
-        case ExistingPrefabAction::CreateWithNewName:
-            // I am aware that similar functionality might be available by QFileDialog::setAcceptMode
-            // However, the prompt to confirm the overwrite showed up under the file selection dialog, which made a terrible UX
-            // TODO: It should be fixed at some point in the future
-            AZStd::optional<QString> newPathCandidate = Internal::QueryUserForPath("Prefab (*.prefab)", QFileDialog::AnyFile);
-            if (!newPathCandidate || newPathCandidate->isEmpty())
-            {
-                return AZStd::nullopt;
-            }
-            return ValidatePrefabPathExistenceAndQueryUserForNewIfNecessary(newPathCandidate.value().toStdString().c_str());
-        }
-    }
-} // namespace ROS2::RobotImporterWidgetUtils

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

@@ -1,35 +0,0 @@
-/*
- * Copyright (c) Contributors to the Open 3D Engine Project.
- * For complete copyright and license terms please see the LICENSE at the root of this distribution.
- *
- * SPDX-License-Identifier: Apache-2.0 OR MIT
- *
- */
-
-#pragma once
-
-#include <AzCore/IO/Path/Path.h>
-#include <QWidget>
-#include <optional>
-
-namespace ROS2::RobotImporterWidgetUtils
-{
-    enum ExistingPrefabAction
-    {
-        Overwrite,
-        CreateWithNewName,
-        Cancel
-    };
-
-    //! Get valid path to the existing URDF file from the user
-    //! @return valid path to the existing URDF file or empty optional if the user canceled the operation
-    //! @param parent - parent widget for the widgets used inside this function
-    AZStd::optional<AZStd::string> QueryUserForURDFPath(QWidget* parent = nullptr);
-
-    //! Validate whether a path exists. If yes, ask user to take a proper action to provide correct path.
-    //! @param path - path to validate
-    //! @param parent - parent widget for the widgets used inside this function
-    //! @return Valid path or an empty optional if it was not possible or user cancelled.
-    AZStd::optional<AZ::IO::Path> ValidatePrefabPathExistenceAndQueryUserForNewIfNecessary(
-        const AZ::IO::Path& path, QWidget* parent = nullptr);
-} // namespace ROS2::RobotImporterWidgetUtils

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

@@ -6,16 +6,16 @@
  *
  */
 
-#include "RobotImporter/URDF/CollidersMaker.h"
-#include "RobotImporter/URDF/PrefabMakerUtils.h"
-#include "RobotImporter/Utils/RobotImporterUtils.h"
-#include "RobotImporter/Utils/SourceAssetsStorage.h"
-#include "RobotImporter/Utils/TypeConversions.h"
+#include "CollidersMaker.h"
+#include "PrefabMakerUtils.h"
 #include <AzCore/Asset/AssetManagerBus.h>
 #include <AzCore/Serialization/Json/JsonUtils.h>
 #include <AzCore/StringFunc/StringFunc.h>
 #include <AzToolsFramework/API/EditorAssetSystemAPI.h>
 #include <AzToolsFramework/Entity/EditorEntityHelpers.h>
+#include <RobotImporter/Utils/RobotImporterUtils.h>
+#include <RobotImporter/Utils/SourceAssetsStorage.h>
+#include <RobotImporter/Utils/TypeConversions.h>
 #include <SceneAPI/SceneCore/Containers/Scene.h>
 #include <SceneAPI/SceneCore/Containers/Utilities/Filters.h>
 #include <SceneAPI/SceneCore/DataTypes/Groups/ISceneNodeGroup.h>
@@ -28,11 +28,11 @@ namespace ROS2
 {
     namespace Internal
     {
-        static const char* collidersMakerLoggingTag = "CollidersMaker";
+        static const char* CollidersMakerLoggingTag = "CollidersMaker";
 
         AZStd::optional<AZ::IO::Path> GetMeshProductPathFromSourcePath(const AZ::IO::Path& sourcePath)
         {
-            AZ_TracePrintf(Internal::collidersMakerLoggingTag, "GetMeshProductPathFromSourcePath: %s", sourcePath.c_str());
+            AZ_TracePrintf(Internal::CollidersMakerLoggingTag, "GetMeshProductPathFromSourcePath: %s\n", sourcePath.c_str());
             AZ::Data::AssetInfo assetInfo;
 
             AZStd::string watchDir;
@@ -46,7 +46,7 @@ namespace ROS2
 
             if (!assetFound)
             {
-                AZ_Error(Internal::collidersMakerLoggingTag, false, "Could not find asset %s", sourcePath.c_str());
+                AZ_Error(Internal::CollidersMakerLoggingTag, false, "Could not find asset %s", sourcePath.c_str());
                 return {};
             }
 
@@ -61,7 +61,7 @@ namespace ROS2
 
             if (!productsFound)
             {
-                AZ_Error(Internal::collidersMakerLoggingTag, false, "Could not find products for asset %s", sourcePath.c_str());
+                AZ_Error(Internal::CollidersMakerLoggingTag, false, "Could not find products for asset %s", sourcePath.c_str());
                 return {};
             }
 
@@ -133,12 +133,12 @@ namespace ROS2
         {
             m_wheelMaterial =
                 AZ::Data::Asset<Physics::MaterialAsset>(assetId, Physics::MaterialAsset::TYPEINFO_Uuid(), physicsMaterialAssetRelPath);
-            AZ_TracePrintf(Internal::collidersMakerLoggingTag, "Waiting for asset load\n");
+            AZ_TracePrintf(Internal::CollidersMakerLoggingTag, "Waiting for asset load\n");
             m_wheelMaterial.BlockUntilLoadComplete();
         }
         else
         {
-            AZ_Warning(Internal::collidersMakerLoggingTag, false, "Cannot load wheel material");
+            AZ_Warning(Internal::CollidersMakerLoggingTag, false, "Cannot load wheel material");
         }
     }
 
@@ -184,7 +184,7 @@ namespace ROS2
             if (!scene)
             {
                 AZ_Error(
-                    Internal::collidersMakerLoggingTag,
+                    Internal::CollidersMakerLoggingTag,
                     false,
                     "Error loading collider. Invalid scene: %s, URDF path: %s",
                     azMeshPath.c_str(),
@@ -197,14 +197,14 @@ namespace ROS2
             if (valueStorage.empty())
             {
                 AZ_Error(
-                    Internal::collidersMakerLoggingTag, false, "Error loading collider. Invalid value storage: %s", azMeshPath.c_str());
+                    Internal::CollidersMakerLoggingTag, false, "Error loading collider. Invalid value storage: %s", azMeshPath.c_str());
                 return;
             }
 
             auto view = AZ::SceneAPI::Containers::MakeDerivedFilterView<AZ::SceneAPI::DataTypes::ISceneNodeGroup>(valueStorage);
             if (view.empty())
             {
-                AZ_Error(Internal::collidersMakerLoggingTag, false, "Error loading collider. Invalid node views: %s", azMeshPath.c_str());
+                AZ_Error(Internal::CollidersMakerLoggingTag, false, "Error loading collider. Invalid node views: %s", azMeshPath.c_str());
                 return;
             }
 
@@ -225,13 +225,13 @@ namespace ROS2
 
             if (result.GetResult() != AZ::SceneAPI::Events::ProcessingResult::Success)
             {
-                AZ_TracePrintf(Internal::collidersMakerLoggingTag, "Scene updated\n");
+                AZ_TracePrintf(Internal::CollidersMakerLoggingTag, "Scene updated\n");
                 return;
             }
 
             auto assetInfoFilePath = AZ::IO::Path{ azMeshPath };
             assetInfoFilePath.Native() += ".assetinfo";
-            AZ_Printf(Internal::collidersMakerLoggingTag, "Saving collider manifest to %s", assetInfoFilePath.c_str());
+            AZ_Printf(Internal::CollidersMakerLoggingTag, "Saving collider manifest to %s\n", assetInfoFilePath.c_str());
             scene->GetManifest().SaveToFile(assetInfoFilePath.c_str());
 
             bool assetFound = false;
@@ -249,7 +249,7 @@ namespace ROS2
             if (!readOutcome.IsSuccess())
             {
                 AZ_Error(
-                    Internal::collidersMakerLoggingTag,
+                    Internal::CollidersMakerLoggingTag,
                     false,
                     "Could not read %s with %s",
                     assetInfoFilePath.c_str(),
@@ -262,7 +262,7 @@ namespace ROS2
             if (valuesIterator == manifestObject.MemberEnd())
             {
                 AZ_Error(
-                    Internal::collidersMakerLoggingTag, false, "Invalid json file: %s (Missing 'values' node)", assetInfoFilePath.c_str());
+                    Internal::CollidersMakerLoggingTag, false, "Invalid json file: %s (Missing 'values' node)", assetInfoFilePath.c_str());
                 return;
             }
 
@@ -283,7 +283,7 @@ namespace ROS2
             if (!saveOutcome.IsSuccess())
             {
                 AZ_Error(
-                    Internal::collidersMakerLoggingTag,
+                    Internal::CollidersMakerLoggingTag,
                     false,
                     "Could not save %s with %s",
                     assetInfoFilePath.c_str(),
@@ -306,7 +306,7 @@ namespace ROS2
         const bool isWheelEntity = Utils::IsWheelURDFHeuristics(link);
         if (isWheelEntity)
         {
-            AZ_Printf(Internal::collidersMakerLoggingTag, "Due to its name, %s is considered a wheel entity", link->name.c_str());
+            AZ_Printf(Internal::CollidersMakerLoggingTag, "Due to its name, %s is considered a wheel entity\n", link->name.c_str());
         }
         const AZ::Data::Asset<Physics::MaterialAsset> materialAsset =
             isWheelEntity ? m_wheelMaterial : AZ::Data::Asset<Physics::MaterialAsset>();
@@ -334,12 +334,12 @@ namespace ROS2
         { // it is ok not to have collision in a link
             return;
         }
-        AZ_TracePrintf(Internal::collidersMakerLoggingTag, "Processing collisions for entity id:%s\n", entityId.ToString().c_str());
+        AZ_TracePrintf(Internal::CollidersMakerLoggingTag, "Processing collisions for entity id:%s\n", entityId.ToString().c_str());
 
         auto geometry = collision->geometry;
         if (!geometry)
         { // non-empty visual should have a geometry
-            AZ_Warning(Internal::collidersMakerLoggingTag, false, "No Geometry for a collider of entity %s", entityId.ToString().c_str());
+            AZ_Warning(Internal::CollidersMakerLoggingTag, false, "No Geometry for a collider of entity %s", entityId.ToString().c_str());
             return;
         }
 
@@ -349,9 +349,6 @@ namespace ROS2
     void CollidersMaker::AddColliderToEntity(
         urdf::CollisionSharedPtr collision, AZ::EntityId entityId, const AZ::Data::Asset<Physics::MaterialAsset>& materialAsset) const
     {
-        // TODO - we are unable to set collider origin. Sub-entities don't work since they would need to parent visuals etc.
-        // TODO - solution: once Collider Component supports Cylinder Shape, switch to it from Shape Collider Component.
-
         AZ::Entity* entity = AzToolsFramework::GetEntityById(entityId);
         AZ_Assert(entity, "AddColliderToEntity called with invalid entityId");
         auto geometry = collision->geometry;
@@ -364,13 +361,12 @@ namespace ROS2
         colliderConfig.m_rotation = URDF::TypeConversions::ConvertQuaternion(collision->origin.rotation);
         if (!isPrimitiveShape)
         {
-            // TODO move setting mesh with ebus here - othervise material is not assigned
             Physics::PhysicsAssetShapeConfiguration shapeConfiguration;
             shapeConfiguration.m_useMaterialsFromAsset = false;
             entity->CreateComponent<PhysX::EditorColliderComponent>(colliderConfig, shapeConfiguration);
             entity->Activate();
 
-            AZ_Printf(Internal::collidersMakerLoggingTag, "Adding mesh collider to %s\n", entityId.ToString().c_str());
+            AZ_Printf(Internal::CollidersMakerLoggingTag, "Adding mesh collider to %s\n", entityId.ToString().c_str());
             auto meshGeometry = std::dynamic_pointer_cast<urdf::Mesh>(geometry);
             AZ_Assert(meshGeometry, "geometry is not meshGeometry");
 
@@ -385,7 +381,7 @@ namespace ROS2
             AZStd::optional<AZ::IO::Path> pxmodelPath = Internal::GetMeshProductPathFromSourcePath(azMeshPath);
             if (!pxmodelPath)
             {
-                AZ_Error(Internal::collidersMakerLoggingTag, false, "Could not find pxmodel for %s", azMeshPath.c_str());
+                AZ_Error(Internal::CollidersMakerLoggingTag, false, "Could not find pxmodel for %s", azMeshPath.c_str());
                 entity->Deactivate();
                 return;
             }
@@ -402,7 +398,7 @@ namespace ROS2
             return;
         }
 
-        AZ_Printf(Internal::collidersMakerLoggingTag, "URDF geometry type: %d\n", geometry->type);
+        AZ_Printf(Internal::CollidersMakerLoggingTag, "URDF geometry type: %d\n", geometry->type);
         switch (geometry->type)
         {
         case urdf::Geometry::SPHERE:
@@ -425,7 +421,6 @@ namespace ROS2
             {
                 auto cylinderGeometry = std::dynamic_pointer_cast<urdf::Cylinder>(geometry);
                 AZ_Assert(cylinderGeometry, "geometry is not cylinderGeometry");
-                // TODO HACK Underlying API of O3DE  does not have Physic::CylinderShapeConfiguration implementation
                 Physics::BoxShapeConfiguration cfg;
                 auto* component = entity->CreateComponent<PhysX::EditorColliderComponent>(colliderConfig, cfg);
                 entity->Activate();
@@ -449,7 +444,7 @@ namespace ROS2
             }
             break;
         default:
-            AZ_Warning(Internal::collidersMakerLoggingTag, false, "Unsupported collider geometry type: %d", geometry->type);
+            AZ_Warning(Internal::CollidersMakerLoggingTag, false, "Unsupported collider geometry type: %d", geometry->type);
             break;
         }
     }
@@ -459,7 +454,7 @@ namespace ROS2
         m_buildThread = AZStd::thread(
             [this, notifyBuildReadyCb]()
             {
-                AZ_Printf(Internal::collidersMakerLoggingTag, "Waiting for URDF assets\n");
+                AZ_Printf(Internal::CollidersMakerLoggingTag, "Waiting for URDF assets\n");
 
                 while (!m_meshesToBuild.empty() && !m_stopBuildFlag)
                 {
@@ -477,7 +472,7 @@ namespace ROS2
                     }
                 }
 
-                AZ_Printf(Internal::collidersMakerLoggingTag, "All URDF assets are ready!\n");
+                AZ_Printf(Internal::CollidersMakerLoggingTag, "All URDF assets are ready!\n");
                 // Notify the caller that we can continue with constructing the prefab.
                 notifyBuildReadyCb();
             });

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

@@ -8,7 +8,6 @@
 
 #pragma once
 
-#include "RobotImporter/Utils/SourceAssetsStorage.h"
 #include "UrdfParser.h"
 #include <AzCore/Component/EntityId.h>
 #include <AzCore/IO/Path/Path.h>
@@ -21,6 +20,7 @@
 #include <AzCore/std/smart_ptr/shared_ptr.h>
 #include <AzFramework/Physics/Material/PhysicsMaterialId.h>
 #include <AzFramework/Physics/Material/PhysicsMaterialManager.h>
+#include <RobotImporter/Utils/SourceAssetsStorage.h>
 
 namespace ROS2
 {

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

@@ -6,10 +6,10 @@
  *
  */
 
-#include "RobotImporter/URDF/InertialsMaker.h"
-#include "RobotImporter/Utils/TypeConversions.h"
+#include "InertialsMaker.h"
 #include <AzCore/Component/EntityId.h>
 #include <AzToolsFramework/Entity/EditorEntityHelpers.h>
+#include <RobotImporter/Utils/TypeConversions.h>
 #include <Source/EditorRigidBodyComponent.h>
 
 namespace ROS2
@@ -38,13 +38,11 @@ namespace ROS2
         rigidBodyConfiguration.m_mass = inertial->mass;
         rigidBodyConfiguration.m_computeMass = false;
 
-        // TODO - Check whether this is a correct offset for every case (consider entity transform and collider origin)
         rigidBodyConfiguration.m_centerOfMassOffset = URDF::TypeConversions::ConvertVector3(inertial->origin.position);
         rigidBodyConfiguration.m_computeCenterOfMass = false;
 
         if (!URDF::TypeConversions::ConvertQuaternion(inertial->origin.rotation).IsIdentity())
         { // There is a rotation component in URDF that we are not able to apply
-            // TODO - determine a solution to also include rotation in import
             AZ_Warning("AddInertial", false, "Ignoring URDF inertial origin rotation (no such field in rigid body configuration)");
         }
 

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

@@ -6,15 +6,16 @@
  *
  */
 
-#include "RobotImporter/URDF/JointsMaker.h"
-#include "RobotImporter/URDF/PrefabMakerUtils.h"
-#include "RobotImporter/Utils/TypeConversions.h"
+#include "JointsMaker.h"
+#include "PrefabMakerUtils.h"
 #include <AzToolsFramework/Entity/EditorEntityHelpers.h>
+#include <RobotImporter/Utils/TypeConversions.h>
 #include <Source/EditorColliderComponent.h>
 #include <Source/EditorFixedJointComponent.h>
 #include <Source/EditorHingeJointComponent.h>
 #include <Source/EditorPrismaticJointComponent.h>
 #include <Source/EditorRigidBodyComponent.h>
+
 namespace ROS2
 {
 
@@ -23,8 +24,6 @@ namespace ROS2
     {
         AZ::Entity* followColliderEntity = AzToolsFramework::GetEntityById(followColliderEntityId);
         PhysX::EditorJointComponent* jointComponent = nullptr;
-        // TODO - ATM, there is no support withing Joint Components for the following:
-        // TODO <calibration> <dynamics> <mimic>, friction, effort, velocity, joint safety and several joint types
 
         // URDF has a joint axis configurable by a normalized vector - that is given by the 'axis' sub-element in the joint element.
         // The o3de has a slightly different way of configuring the axis of the joint. The o3de has an axis around positive `X` and rotation
@@ -46,7 +45,7 @@ namespace ROS2
         const AZ::Vector3 rotation = quaternion.GetEulerDegrees();
 
         switch (joint->type)
-        { // TODO - replace with a generic member function
+        {
         case urdf::Joint::FIXED:
             {
                 jointComponent = followColliderEntity->CreateComponent<PhysX::EditorFixedJointComponent>();

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

@@ -22,7 +22,7 @@ namespace ROS2
     public:
         using JointsMakerResult = AZ::Outcome<AZ::ComponentId, AZStd::string>;
 
-        //! Adds joint to entity and sets it accordingly to urdf::Joint
+        //! Add a joint to an entity and sets it accordingly to urdf::Joint
         //! @param joint Joint data
         //! @param followColliderEntityId A non-active entity which will be populated with Joint components.
         //! @param leadColliderEntityId An entity higher in hierarchy which is connected through the joint with the child entity.

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

@@ -103,8 +103,7 @@ namespace ROS2::PrefabMakerUtils
     }
 
     bool HasCollider(AZ::EntityId entityId)
-    { // TODO - actually, we want EditorColliderComponent specifically, but the change can be applied only after moving to ECC
-        // TODO - which will happen when Cylinder shape is supported. Until then, we check for either ECC or ESCC.
+    {
         AZ::Entity* entity = AzToolsFramework::GetEntityById(entityId);
         AZ_Assert(entity, "Unknown entity %s", entityId.ToString().c_str());
         return entity->FindComponent<PhysX::EditorColliderComponent>() != nullptr ||

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

@@ -8,11 +8,11 @@
 
 #pragma once
 
-#include "RobotImporter/Utils/SourceAssetsStorage.h"
 #include "UrdfParser.h"
 #include <AzCore/IO/Path/Path.h>
 #include <AzCore/std/optional.h>
 #include <AzCore/std/string/string.h>
+#include <RobotImporter/Utils/SourceAssetsStorage.h>
 
 #include <AzToolsFramework/Prefab/PrefabPublicInterface.h>
 

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

@@ -6,14 +6,9 @@
  *
  */
 
-#include "RobotImporter/URDF/URDFPrefabMaker.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2GemUtilities.h"
-#include "ROS2/Spawner/SpawnerBus.h"
-#include "RobotControl/ROS2RobotControlComponent.h"
-#include "RobotImporter/URDF/CollidersMaker.h"
-#include "RobotImporter/URDF/PrefabMakerUtils.h"
-#include "RobotImporter/Utils/RobotImporterUtils.h"
+#include "URDFPrefabMaker.h"
+#include "CollidersMaker.h"
+#include "PrefabMakerUtils.h"
 #include <API/EditorAssetSystemAPI.h>
 #include <AzCore/IO/FileIO.h>
 #include <AzToolsFramework/Entity/EditorEntityHelpers.h>
@@ -21,6 +16,11 @@
 #include <AzToolsFramework/Prefab/PrefabSystemComponentInterface.h>
 #include <AzToolsFramework/ToolsComponents/GenericComponentWrapper.h>
 #include <AzToolsFramework/ToolsComponents/TransformComponent.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2GemUtilities.h>
+#include <ROS2/Spawner/SpawnerBus.h>
+#include <RobotControl/ROS2RobotControlComponent.h>
+#include <RobotImporter/Utils/RobotImporterUtils.h>
 
 namespace ROS2
 {
@@ -65,11 +65,10 @@ namespace ROS2
             AZStd::lock_guard<AZStd::mutex> lck(m_statusLock);
             m_status.clear();
         }
-        // TODO - this is PoC code, restructure when developing semantics of URDF->Prefab/Entities/Components mapping
-        AZStd::unordered_map<AZStd::string, AzToolsFramework::Prefab::PrefabEntityResult> created_links;
+        AZStd::unordered_map<AZStd::string, AzToolsFramework::Prefab::PrefabEntityResult> createdLinks;
         AzToolsFramework::Prefab::PrefabEntityResult createEntityRoot = AddEntitiesForLink(m_model->root_link_, AZ::EntityId());
-        AZStd::string root_name(m_model->root_link_->name.c_str(), m_model->root_link_->name.size());
-        created_links[root_name] = createEntityRoot;
+        AZStd::string rootName(m_model->root_link_->name.c_str(), m_model->root_link_->name.size());
+        createdLinks[rootName] = createEntityRoot;
         if (!createEntityRoot.IsSuccess())
         {
             return AZ::Failure(AZStd::string(createEntityRoot.GetError()));
@@ -78,16 +77,16 @@ namespace ROS2
         auto links = Utils::GetAllLinks(m_model->root_link_->child_links);
 
         // create links
-        for (const auto& [name, link_ptr] : links)
+        for (const auto& [name, linkPtr] : links)
         {
-            created_links[name] = AddEntitiesForLink(link_ptr, createEntityRoot.GetValue());
+            createdLinks[name] = AddEntitiesForLink(linkPtr, createEntityRoot.GetValue());
         }
 
-        for (const auto& [name, result] : created_links)
+        for (const auto& [name, result] : createdLinks)
         {
             AZ_TracePrintf(
                 "CreatePrefabFromURDF",
-                "Link with name %s was created as: %s",
+                "Link with name %s was created as: %s\n",
                 name.c_str(),
                 result.IsSuccess() ? (result.GetValue().ToString().c_str()) : ("[Failed]"));
             AZStd::lock_guard<AZStd::mutex> lck(m_statusLock);
@@ -102,12 +101,12 @@ namespace ROS2
         }
 
         // set transforms of links
-        for (const auto& [name, link_ptr] : links)
+        for (const auto& [name, linkPtr] : links)
         {
-            const auto this_entry = created_links.at(name);
+            const auto this_entry = createdLinks.at(name);
             if (this_entry.IsSuccess())
             {
-                AZ::Transform tf = Utils::GetWorldTransformURDF(link_ptr);
+                AZ::Transform tf = Utils::GetWorldTransformURDF(linkPtr);
                 auto* entity = AzToolsFramework::GetEntityById(this_entry.GetValue());
                 if (entity)
                 {
@@ -116,7 +115,7 @@ namespace ROS2
                     {
                         AZ_TracePrintf(
                             "CreatePrefabFromURDF",
-                            "Setting transform %s %s to [%f %f %f] [%f %f %f %f]",
+                            "Setting transform %s %s to [%f %f %f] [%f %f %f %f]\n",
                             name.c_str(),
                             this_entry.GetValue().ToString().c_str(),
                             tf.GetTranslation().GetX(),
@@ -131,71 +130,71 @@ namespace ROS2
                     else
                     {
                         AZ_TracePrintf(
-                            "CreatePrefabFromURDF", "Setting transform failed: %s does not have transform interface", name.c_str());
+                            "CreatePrefabFromURDF", "Setting transform failed: %s does not have transform interface\n", name.c_str());
                     }
                 }
             }
         }
 
         // set hierarchy
-        for (const auto& [name, link_ptr] : links)
+        for (const auto& [name, linkPtr] : links)
         {
-            const auto this_entry = created_links.at(name);
-            if (!this_entry.IsSuccess())
+            const auto thisEntry = createdLinks.at(name);
+            if (!thisEntry.IsSuccess())
             {
-                AZ_TracePrintf("CreatePrefabFromURDF", "Link %s creation failed", name.c_str());
+                AZ_TracePrintf("CreatePrefabFromURDF", "Link %s creation failed\n", name.c_str());
                 continue;
             }
-            auto parent_ptr = link_ptr->getParent();
-            if (!parent_ptr)
+            auto parentPtr = linkPtr->getParent();
+            if (!parentPtr)
             {
-                AZ_TracePrintf("CreatePrefabFromURDF", "Link %s has no parents", name.c_str());
+                AZ_TracePrintf("CreatePrefabFromURDF", "Link %s has no parents\n", name.c_str());
                 continue;
             }
-            AZStd::string parent_name(parent_ptr->name.c_str(), parent_ptr->name.size());
-            const auto parent_entry = created_links.find(parent_name);
-            if (parent_entry == created_links.end())
+            AZStd::string parentName(parentPtr->name.c_str(), parentPtr->name.size());
+            const auto parentEntry = createdLinks.find(parentName);
+            if (parentEntry == createdLinks.end())
             {
-                AZ_TracePrintf("CreatePrefabFromURDF", "Link %s has invalid parent name %s", name.c_str(), parent_name.c_str());
+                AZ_TracePrintf("CreatePrefabFromURDF", "Link %s has invalid parent name %s\n", name.c_str(), parentName.c_str());
                 continue;
             }
-            if (!parent_entry->second.IsSuccess())
+            if (!parentEntry->second.IsSuccess())
             {
                 AZ_TracePrintf(
-                    "CreatePrefabFromURDF", "Link %s has parent %s which has failed to create", name.c_str(), parent_name.c_str());
+                    "CreatePrefabFromURDF", "Link %s has parent %s which has failed to create\n", name.c_str(), parentName.c_str());
                 continue;
             }
             AZ_TracePrintf(
                 "CreatePrefabFromURDF",
-                "Link %s setting parent to %s",
-                this_entry.GetValue().ToString().c_str(),
-                parent_entry->second.GetValue().ToString().c_str());
-            AZ_TracePrintf("CreatePrefabFromURDF", "Link %s setting parent to %s", name.c_str(), parent_name.c_str());
-            auto* entity = AzToolsFramework::GetEntityById(this_entry.GetValue());
+                "Link %s setting parent to %s\n",
+                thisEntry.GetValue().ToString().c_str(),
+                parentEntry->second.GetValue().ToString().c_str());
+            AZ_TracePrintf("CreatePrefabFromURDF", "Link %s setting parent to %s\n", name.c_str(), parentName.c_str());
+            auto* entity = AzToolsFramework::GetEntityById(thisEntry.GetValue());
             entity->Activate();
-            AZ::TransformBus::Event(this_entry.GetValue(), &AZ::TransformBus::Events::SetParent, parent_entry->second.GetValue());
+            AZ::TransformBus::Event(thisEntry.GetValue(), &AZ::TransformBus::Events::SetParent, parentEntry->second.GetValue());
             entity->Deactivate();
         }
 
         // create joint
         auto joints = Utils::GetAllJoints(m_model->root_link_->child_links);
-        for (const auto& [name, joint_ptr] : joints)
+        for (const auto& [name, jointPtr] : joints)
         {
-            AZ_Assert(joint_ptr, "joint %s is null", name.c_str());
+            AZ_Assert(jointPtr, "joint %s is null", name.c_str());
             AZ_TracePrintf(
                 "CreatePrefabFromURDF",
-                "Creating joint %s : %s -> %s",
+                "Creating joint %s : %s -> %s\n",
                 name.c_str(),
-                joint_ptr->parent_link_name.c_str(),
-                joint_ptr->child_link_name.c_str());
+                jointPtr->parent_link_name.c_str(),
+                jointPtr->child_link_name.c_str());
 
-            auto lead_entity = created_links.at(joint_ptr->parent_link_name.c_str());
-            auto child_entity = created_links.at(joint_ptr->child_link_name.c_str());
+            auto leadEntity = createdLinks.at(jointPtr->parent_link_name.c_str());
+            auto childEntity = createdLinks.at(jointPtr->child_link_name.c_str());
             // check if both has RigidBody
-            if (lead_entity.IsSuccess() && child_entity.IsSuccess())
+            if (leadEntity.IsSuccess() && childEntity.IsSuccess())
             {
                 AZStd::lock_guard<AZStd::mutex> lck(m_statusLock);
-                auto result = m_jointsMaker.AddJointComponent(joint_ptr, child_entity.GetValue(), lead_entity.GetValue());
+                auto result = m_jointsMaker.AddJointComponent(jointPtr, childEntity.GetValue(), leadEntity.GetValue());
                 if (result.IsSuccess())
                 {
                     m_status.emplace(name, AZStd::string::format("created as %llu", result.GetValue()));
@@ -262,8 +261,6 @@ namespace ROS2
         AZ::EntityId entityId = createEntityResult.GetValue();
         AZ::Entity* entity = AzToolsFramework::GetEntityById(entityId);
 
-        // Add ROS2FrameComponent - TODO: only for top level and joints
-        // TODO - add unique namespace to the robot's top level frame
         const auto frameCompontentId = Utils::CreateComponent(entityId, ROS2FrameComponent::TYPEINFO_Uuid());
         if (frameCompontentId)
         {
@@ -303,7 +300,7 @@ namespace ROS2
 
         if (spawner == nullptr)
         {
-            AZ_TracePrintf("URDF Importer", "Spawner not found - creating entity in (0,0,0)") return;
+            AZ_TracePrintf("URDF Importer", "Spawner not found - creating entity in (0,0,0)\n") return;
         }
 
         auto entity_ = AzToolsFramework::GetEntityById(rootEntityId);
@@ -311,7 +308,7 @@ namespace ROS2
 
         if (transformInterface_ == nullptr)
         {
-            AZ_TracePrintf("URDF Importer", "TransformComponent not found in created entity") return;
+            AZ_TracePrintf("URDF Importer", "TransformComponent not found in created entity\n") return;
         }
 
         auto pose = spawner->GetDefaultSpawnPose();
@@ -323,9 +320,9 @@ namespace ROS2
     {
         AZStd::string str;
         AZStd::lock_guard<AZStd::mutex> lck(m_statusLock);
-        for (const auto& [entry, entry_status] : m_status)
+        for (const auto& [entry, entryStatus] : m_status)
         {
-            str += entry + " " + entry_status + "\n";
+            str += entry + " " + entryStatus + "\n";
         }
         return str;
     }

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

@@ -8,16 +8,16 @@
 
 #pragma once
 
-#include "RobotImporter/URDF/CollidersMaker.h"
-#include "RobotImporter/URDF/InertialsMaker.h"
-#include "RobotImporter/URDF/JointsMaker.h"
-#include "RobotImporter/URDF/VisualsMaker.h"
-#include "RobotImporter/Utils/SourceAssetsStorage.h"
+#include "CollidersMaker.h"
+#include "InertialsMaker.h"
+#include "JointsMaker.h"
 #include "UrdfParser.h"
+#include "VisualsMaker.h"
 #include <AzCore/Component/EntityId.h>
 #include <AzCore/std/smart_ptr/make_shared.h>
 #include <AzCore/std/smart_ptr/shared_ptr.h>
 #include <AzToolsFramework/Prefab/PrefabPublicInterface.h>
+#include <RobotImporter/Utils/SourceAssetsStorage.h>
 
 namespace ROS2
 {

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

@@ -17,9 +17,9 @@
 
 namespace ROS2
 {
-    namespace Internal
+    namespace UrdfParser::Internal
     {
-        void checkIfCurrentLocaleHasDotAsADecimalSeparator()
+        void CheckIfCurrentLocaleHasDotAsADecimalSeparator()
         {
             // Due to the fact that URDF parser takes into account the locale information, incompatibility between URDF file locale and
             // system locale might lead to incorrect URDF parsing. Mainly it affects floating point numbers, and the decimal separator. When
@@ -33,13 +33,45 @@ namespace ROS2
                     "UrdfParser", false, "Locale %s might be incompatible with the URDF file content.\n", currentLocale.name().c_str());
             }
         }
-    } // namespace Internal
+
+        class CustomConsoleHandler : public console_bridge::OutputHandler
+        {
+        private:
+            std::stringstream console_ss;
+
+        public:
+            void log(const std::string& text, console_bridge::LogLevel level, const char* filename, int line) override final;
+
+            //! Clears accumulated log
+            void Clear();
+
+            //! Read accumulated log to file
+            AZStd::string GetLog();
+        };
+
+        void CustomConsoleHandler::log(const std::string& text, console_bridge::LogLevel level, const char* filename, int line)
+        {
+            AZ_Printf("UrdfParser", "%s\n", text.c_str());
+            console_ss << text << "\n";
+        }
+
+        void CustomConsoleHandler::Clear()
+        {
+            console_ss = std::stringstream();
+        }
+
+        AZStd::string CustomConsoleHandler::GetLog()
+        {
+            return AZStd::string(console_ss.str().c_str(), console_ss.str().size());
+        }
+
+        CustomConsoleHandler customConsoleHandler;
+    } // namespace UrdfParser::Internal
 
     urdf::ModelInterfaceSharedPtr UrdfParser::Parse(const AZStd::string& xmlString)
     {
-        m_customConsoleHandler.console_ss = std::stringstream();
-        console_bridge::useOutputHandler(&m_customConsoleHandler);
-        Internal::checkIfCurrentLocaleHasDotAsADecimalSeparator();
+        console_bridge::useOutputHandler(&Internal::customConsoleHandler);
+        Internal::CheckIfCurrentLocaleHasDotAsADecimalSeparator();
         const auto ret = urdf::parseURDF(xmlString.c_str());
         console_bridge::restorePreviousOutputHandler();
         return ret;
@@ -60,15 +92,7 @@ namespace ROS2
 
     AZStd::string UrdfParser::GetUrdfParsingLog()
     {
-        return AZStd::string(
-            UrdfParser::m_customConsoleHandler.console_ss.str().c_str(), UrdfParser::m_customConsoleHandler.console_ss.str().size());
-    }
-
-    void UrdfParser::CustomConsoleHandler::log(const std::string& text, console_bridge::LogLevel level, const char* filename, int line)
-    {
-        AZ_Printf("UrdfParser", "%s", text.c_str());
-        console_ss << text << "\n";
+        return Internal::customConsoleHandler.GetLog();
     }
 
-    UrdfParser::CustomConsoleHandler UrdfParser::m_customConsoleHandler;
 } // namespace ROS2

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

@@ -16,29 +16,21 @@
 namespace ROS2
 {
     //! Class for parsing URDF data.
-    class UrdfParser
+    namespace UrdfParser
     {
-    public:
         //! Parse string with URDF data and generate model.
         //! @param xmlString a string that contains URDF data (XML format).
         //! @return model represented as a tree of parsed links.
-        static urdf::ModelInterfaceSharedPtr Parse(const AZStd::string& xmlString);
+        urdf::ModelInterfaceSharedPtr Parse(const AZStd::string& xmlString);
 
         //! Parse file with URDF data and generate model.
         //! @param filePath is a path to file with URDF data that will be loaded and parsed.
         //! @return model represented as a tree of parsed links.
-        static urdf::ModelInterfaceSharedPtr ParseFromFile(const AZStd::string& filePath);
+        urdf::ModelInterfaceSharedPtr ParseFromFile(const AZStd::string& filePath);
 
-        static AZStd::string GetUrdfParsingLog();
+        //! Retrieve console log from URDF parsing
+        //! @return a log with output from urdf_parser
+        AZStd::string GetUrdfParsingLog();
 
-    private:
-        class CustomConsoleHandler : private console_bridge::OutputHandler
-        {
-        private:
-            friend UrdfParser;
-            void log(const std::string& text, console_bridge::LogLevel level, const char* filename, int line) override;
-            std::stringstream console_ss;
-        };
-        static CustomConsoleHandler m_customConsoleHandler;
-    };
+    }; // namespace UrdfParser
 } // namespace ROS2

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

@@ -201,8 +201,6 @@ namespace ROS2
             return;
         }
 
-        // Mesh visual - we can have either filename or default material with a given color
-        // TODO - handle texture_filename - file materials
         entity->CreateComponent(AZ::Render::EditorMaterialComponentTypeId);
         AZ_Printf("AddVisual", "Setting color for material %s\n", visual->material->name.c_str());
         entity->Activate();

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

@@ -8,7 +8,6 @@
 
 #pragma once
 
-#include "RobotImporter/Utils/SourceAssetsStorage.h"
 #include "UrdfParser.h"
 #include <AzCore/Component/EntityId.h>
 #include <AzCore/IO/Path/Path.h>

Gems/ROS2/Code/Source/RobotImporter/Utils/RobotImporterUtils.cpp

@@ -59,7 +59,7 @@ namespace ROS2
     AZStd::unordered_map<AZStd::string, urdf::LinkSharedPtr> Utils::GetAllLinks(const std::vector<urdf::LinkSharedPtr>& childLinks)
     {
         AZStd::unordered_map<AZStd::string, urdf::LinkSharedPtr> pointers;
-        std::function<void(const std::vector<urdf::LinkSharedPtr>&)> link_visitor =
+        AZStd::function<void(const std::vector<urdf::LinkSharedPtr>&)> link_visitor =
             [&](const std::vector<urdf::LinkSharedPtr>& child_links) -> void
         {
             for (const urdf::LinkSharedPtr& child_link : child_links)
@@ -76,7 +76,7 @@ namespace ROS2
     AZStd::unordered_map<AZStd::string, urdf::JointSharedPtr> Utils::GetAllJoints(const std::vector<urdf::LinkSharedPtr>& childLinks)
     {
         AZStd::unordered_map<AZStd::string, urdf::JointSharedPtr> joints;
-        std::function<void(const std::vector<urdf::LinkSharedPtr>&)> link_visitor =
+        AZStd::function<void(const std::vector<urdf::LinkSharedPtr>&)> link_visitor =
             [&](const std::vector<urdf::LinkSharedPtr>& child_links) -> void
         {
             for (auto child_link : child_links)
@@ -124,7 +124,7 @@ namespace ROS2
             }
         };
 
-        std::function<void(const std::vector<urdf::LinkConstSharedPtr>&)> linkVisitor =
+        AZStd::function<void(const std::vector<urdf::LinkConstSharedPtr>&)> linkVisitor =
             [&](const std::vector<urdf::LinkConstSharedPtr>& child_links) -> void
         {
             for (auto link : child_links)
@@ -154,15 +154,15 @@ namespace ROS2
         {
             AZ::StringFunc::Replace(unresolvedPath, "package://", "", true, true);
             AZ::IO::Path urdfProperPath(urdfFilePath);
-            AZ::IO::Path package_path;
+            AZ::IO::Path packagePath;
             for (auto it = urdfProperPath.begin(); it != urdfProperPath.end(); it++)
             {
-                package_path /= *it;
-                AZStd::string package_xml_candite = (package_path / "package.xml").String();
-                if (fileExists(package_xml_candite))
+                packagePath /= *it;
+                AZStd::string packageXmlCandite = (packagePath / "package.xml").String();
+                if (fileExists(packageXmlCandite))
                 {
                     // package.xml has been found
-                    return (package_path / unresolvedPath).String();
+                    return (packagePath / unresolvedPath).String();
                 }
             }
             // we have nothing
@@ -175,10 +175,10 @@ namespace ROS2
             return unresolvedPath;
         }
         // seems to be relative path
-        AZ::IO::Path relative_path(unresolvedPath);
-        AZ::IO::Path urdf_proper_Path(urdfFilePath);
-        AZ::IO::Path urdf_parent_path = urdf_proper_Path.ParentPath();
-        return (urdf_parent_path / relative_path).String();
+        AZ::IO::Path relativePath(unresolvedPath);
+        AZ::IO::Path urdfProperPath(urdfFilePath);
+        AZ::IO::Path urdfParentPath = urdfProperPath.ParentPath();
+        return (urdfParentPath / relativePath).String();
     }
 
 } // namespace ROS2

Gems/ROS2/Code/Source/RobotImporter/Utils/RobotImporterUtils.h

@@ -7,50 +7,50 @@
  */
 #pragma once
 
-#include "AzCore/Component/ComponentBus.h"
-#include "AzCore/std/string/string.h"
-#include "RobotImporter/URDF/UrdfParser.h"
+#include <AzCore/Component/ComponentBus.h>
 #include <AzCore/IO/SystemFile.h>
 #include <AzCore/Math/Transform.h>
 #include <AzCore/std/containers/unordered_map.h>
 #include <AzCore/std/function/function_template.h>
+#include <AzCore/std/string/string.h>
+#include <RobotImporter/URDF/UrdfParser.h>
 
 namespace ROS2
 {
     namespace
     {
-        const AZStd::function<bool(const AZStd::string&)> fileExistsCall = [](const AZStd::string& filename) -> bool
+        static inline bool FileExistsCall(const AZStd::string& filename)
         {
             return AZ::IO::SystemFile::Exists(filename.c_str());
         };
-    }
+    } // namespace
 
     namespace Utils
     {
         bool IsWheelURDFHeuristics(const urdf::LinkConstSharedPtr& link);
 
         //! The recursive function for the given link goes through URDF and finds world-to-entity transformation for us.
-        //! It traverses URDF from the given link to the root.
-        //! @param t - should be default (identity).
+        //! @param link pointer to URDF link that root of robot description
+        //! @param t initial transform, should be identity for non-recursive call.
         //! @returns root to entity transform
         AZ::Transform GetWorldTransformURDF(const urdf::LinkSharedPtr& link, AZ::Transform t = AZ::Transform::Identity());
 
-        //! Retrieve all child links in urdf.
-        //! @param child links list of links in a query
+        //! Retrieve all links in URDF as a map, where a key is link's name and a value is a pointer to link.
+        //! Allows to retrieve a pointer to a link given it name.
+        //! @param childLinks list of links in a query
         //! @returns mapping from link name to link pointer
         AZStd::unordered_map<AZStd::string, urdf::LinkSharedPtr> GetAllLinks(const std::vector<urdf::LinkSharedPtr>& childLinks);
 
         //! Retrieve all joints in URDF.
-        //! @param child links list of links in a query
-        //! @returns mapping from link name to link pointer
+        //! @param childLinks list of links in a query
+        //! @returns mapping from joint name to joint pointer
         AZStd::unordered_map<AZStd::string, urdf::JointSharedPtr> GetAllJoints(const std::vector<urdf::LinkSharedPtr>& childLinks);
 
         //! Retrieve all meshes referenced in URDF as unresolved URDF patches.
-        //! Function traverse URDF in recursive manner.
-        //! It obtains referenced meshes' filenames.
         //! Note that returned filenames are unresolved URDF patches.
         //! @param visual - search for visual meshes.
         //! @param colliders - search for collider meshes.
+        //! @param rootLink - pointer to URDF link that is a root of robot description
         //! @returns set of meshes' filenames.
         AZStd::unordered_set<AZStd::string> GetMeshesFilenames(const urdf::LinkConstSharedPtr& rootLink, bool visual, bool colliders);
 
@@ -62,7 +62,7 @@ namespace ROS2
         AZStd::string ResolveURDFPath(
             AZStd::string unresolvedPath,
             const AZStd::string& urdfFilePath,
-            const AZStd::function<bool(const AZStd::string&)>& fileExists = fileExistsCall);
+            const AZStd::function<bool(const AZStd::string&)>& fileExists = FileExistsCall);
 
     } // namespace Utils
 } // namespace ROS2

Gems/ROS2/Code/Source/RobotImporter/Utils/SourceAssetsStorage.cpp

@@ -6,7 +6,7 @@
  *
  */
 #include "SourceAssetsStorage.h"
-#include "RobotImporter/Utils/RobotImporterUtils.h"
+#include "RobotImporterUtils.h"
 
 namespace ROS2::Utils
 {

Gems/ROS2/Code/Source/RobotImporter/Utils/SourceAssetsStorage.h

@@ -19,29 +19,29 @@
 namespace ROS2::Utils
 {
     //! Structure contains essential information about the source and product assets in O3DE.
-    //! It is designed to provide necessary information for other classes in URDF converter, e.g. CollidersMaker or VisualsMaker.
+    //! It is designed to provide necessary information for other classes in URDF converter, eg CollidersMaker or VisualsMaker.
     struct AvailableAsset
     {
-        //! Relative path to source asset e.g. `Assets/foo_robot/meshes/bar_link.dae`.
+        //! Relative path to source asset eg `Assets/foo_robot/meshes/bar_link.dae`.
         AZStd::string m_sourceAssetRelativePath;
 
-        //! Relative path to source asset e.g. `/home/user/project/Assets/foo_robot/meshes/bar_link.dae`.
+        //! Relative path to source asset eg `/home/user/project/Assets/foo_robot/meshes/bar_link.dae`.
         AZStd::string m_sourceAssetGlobalPath;
 
-        //! Relative path to source asset e.g. `foo_robot/meshes/bar_link.azmodel`.
+        //! Relative path to source asset eg `foo_robot/meshes/bar_link.azmodel`.
         AZStd::string m_productAssetRelativePath;
 
-        /// Product asset ID @see AZ::Data::AssetInfo.
+        //! Product asset ID @see AZ::Data::AssetInfo.
         AZ::Data::AssetId m_assetId;
     };
 
     //! The structure contains a mapping between URDF's path to O3DE asset information.
     struct UrdfAsset
     {
-        //! Unresolved URDF path to mesh, e.g. `package://meshes/bar_link.dae`.
+        //! Unresolved URDF path to mesh, eg `package://meshes/bar_link.dae`.
         AZStd::string m_urdfPath;
 
-        //! Resolved URDF path, points to the valid mesh in the filestystem, e.g. `/home/user/ros_ws/src/foo_robot/meshes/bar_link.dae'
+        //! Resolved URDF path, points to the valid mesh in the filestystem, eg `/home/user/ros_ws/src/foo_robot/meshes/bar_link.dae'
         AZStd::string m_resolvedUrdfPath;
 
         //! Checksum of the file located pointed by `m_resolvedUrdfPath`.
@@ -57,13 +57,11 @@ namespace ROS2::Utils
     //! Function computes CRC32 on first kilobyte of file.
     AZ::Crc32 GetFileCRC(const AZStd::string& filename);
 
-    //! Function takes assets catalog and compute CRC for every source mesh.
-    //! ToDo consider limit scope to only sub-directory.
-    //! ToDo consider use filesystem instead of AssetCatalogRequestBus::EnumerateAssets.
+    //! Compute CRC for every source mesh from the assets catalog.
     //! @returns map where key is crc of source file and value is AvailableAsset.
     AZStd::unordered_map<AZ::Crc32, AvailableAsset> GetInterestingSourceAssetsCRC();
 
-    //! The function is to discover an association between meshes in URDF and O3DE source and product assets.
+    //! Discover an association between meshes in URDF and O3DE source and product assets.
     //! The @param meshesFilenames contains the list of unresolved URDF filenames that are to be found as assets.
     //! Steps:
     //! - Functions resolves URDF filenames with `ResolveURDFPath`.
@@ -72,7 +70,7 @@ namespace ROS2::Utils
     //! - Suitable mapping to the O3DE asset is found by comparing the checksum of the file pointed by the URDF path and source asset.
     //! @param meshesFilenames - list of the unresolved path from the URDF file
     //! @param urdFilename - filename of URDF file, used for resolvement
-    //! @returns map where key is unresolved URDF path to AvailableAsset
+    //! @returns a URDF Asset map where the key is unresolved URDF path to AvailableAsset
     UrdfAssetMap FindAssetsForUrdf(const AZStd::unordered_set<AZStd::string>& meshesFilenames, const AZStd::string& urdFilename);
 
 } // namespace ROS2::Utils

Gems/ROS2/Code/Source/RobotImporter/Utils/TypeConversions.h

@@ -8,21 +8,20 @@
 
 #pragma once
 
-#include "AzCore/Math/Transform.h"
-#include "RobotImporter/URDF/UrdfParser.h"
 #include <AzCore/Math/Color.h>
 #include <AzCore/Math/Quaternion.h>
+#include <AzCore/Math/Transform.h>
 #include <AzCore/Math/Vector3.h>
+#include <RobotImporter/URDF/UrdfParser.h>
 
 namespace ROS2::URDF
 {
     //! Common types conversion between urdf and AZ formats
-    class TypeConversions
+    namespace TypeConversions
     {
-    public:
-        static AZ::Vector3 ConvertVector3(const urdf::Vector3& urdfVector);
-        static AZ::Quaternion ConvertQuaternion(const urdf::Rotation& urdfQuaternion);
-        static AZ::Color ConvertColor(const urdf::Color& color);
-        static AZ::Transform ConvertPose(const urdf::Pose& pose);
-    };
+        AZ::Vector3 ConvertVector3(const urdf::Vector3& urdfVector);
+        AZ::Quaternion ConvertQuaternion(const urdf::Rotation& urdfQuaternion);
+        AZ::Color ConvertColor(const urdf::Color& color);
+        AZ::Transform ConvertPose(const urdf::Pose& pose);
+    }; // namespace TypeConversions
 } // namespace ROS2::URDF

Gems/ROS2/Code/Source/Sensor/ROS2SensorComponent.cpp

@@ -6,15 +6,15 @@
  *
  */
 
-#include "ROS2/Sensor/ROS2SensorComponent.h"
-#include "ROS2/Frame/ROS2FrameComponent.h"
-#include "ROS2/ROS2Bus.h"
-#include "ROS2/ROS2GemUtilities.h"
-#include "ROS2/Utilities/ROS2Names.h"
 #include <AzCore/Component/Entity.h>
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <ROS2/Frame/ROS2FrameComponent.h>
+#include <ROS2/ROS2Bus.h>
+#include <ROS2/ROS2GemUtilities.h>
+#include <ROS2/Sensor/ROS2SensorComponent.h>
+#include <ROS2/Utilities/ROS2Names.h>
 
 namespace ROS2
 {
@@ -51,7 +51,6 @@ namespace ROS2
 
     AZStd::string ROS2SensorComponent::GetNamespace() const
     {
-        // TODO - hold frame?
         auto* ros2Frame = Utils::GetGameOrEditorComponent<ROS2FrameComponent>(GetEntity());
         return ros2Frame->GetNamespace();
     };
@@ -64,7 +63,7 @@ namespace ROS2
 
     void ROS2SensorComponent::GetRequiredServices(AZ::ComponentDescriptor::DependencyArrayType& required)
     {
-        required.push_back(AZ_CRC("ROS2Frame"));
+        required.push_back(AZ_CRC_CE("ROS2Frame"));
     }
 
     void ROS2SensorComponent::OnTick([[maybe_unused]] float deltaTime, [[maybe_unused]] AZ::ScriptTimePoint time)
@@ -77,7 +76,6 @@ namespace ROS2
 
         auto frequency = m_sensorConfiguration.m_frequency;
 
-        // TODO - add range validation (Attributes?)
         auto frameTime = frequency == 0 ? 1 : 1 / frequency;
 
         m_timeElapsedSinceLastTick += deltaTime;

Gems/ROS2/Code/Source/Sensor/SensorConfiguration.cpp

@@ -6,9 +6,9 @@
  *
  */
 
-#include "ROS2/Sensor/SensorConfiguration.h"
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/EditContextConstants.inl>
+#include <ROS2/Sensor/SensorConfiguration.h>
 
 namespace ROS2
 {

Gems/ROS2/Code/Source/Spawner/ROS2SpawnPointComponent.cpp

@@ -38,7 +38,7 @@ namespace ROS2
             {
                 ec->Class<ROS2SpawnPointComponent>("ROS2 Spawn Point", "Spawn Point")
                     ->ClassElement(AZ::Edit::ClassElements::EditorData, "Stores information about available spawn point")
-                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("Game"))
+                    ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC_CE("Game"))
                     ->Attribute(AZ::Edit::Attributes::Category, "ROS2")
                     ->DataElement(AZ::Edit::UIHandlers::EntityId, &ROS2SpawnPointComponent::m_name, "Name", "Name")
                     ->DataElement(

Gems/ROS2/Code/Source/Spawner/ROS2SpawnPointComponent.h

@@ -27,11 +27,11 @@ namespace ROS2
         ROS2SpawnPointComponent() = default;
 
         ~ROS2SpawnPointComponent() = default;
-
+        //////////////////////////////////////////////////////////////////////////
+        // Component overrides
         void Activate() override;
-
         void Deactivate() override;
-
+        //////////////////////////////////////////////////////////////////////////
         static void Reflect(AZ::ReflectContext* context);
 
         AZStd::pair<AZStd::string, SpawnPointInfo> GetInfo() const;