review feedback

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

Author: adlarkin

test/sdf/double_pendulum.sdf

@@ -198,7 +198,7 @@
       <axis>
         <xyz>1.0 0 0</xyz>
         <dynamics>
-          <damping>35.000000</damping>
+          <damping>35</damping>
         </dynamics>
         <limit>
           <stiffness>350</stiffness>
@@ -212,7 +212,7 @@
       <axis>
         <xyz>1.0 0 0</xyz>
         <dynamics>
-          <damping>35.000000</damping>
+          <damping>35</damping>
         </dynamics>
         <limit>
           <stiffness>350</stiffness>

usd/src/sdf_parser/Joint.cc

@@ -17,6 +17,8 @@
 
 #include "sdf/usd/sdf_parser/Joint.hh"
 
+#include <sstream>
+
 #include <ignition/math/Angle.hh>
 #include <ignition/math/Pose3.hh>
 #include <ignition/math/Vector3.hh>
@@ -89,43 +91,29 @@ namespace usd
     {
       auto poseResolutionErrors =
         _joint.SemanticPose().Resolve(parentToJoint, _joint.ParentLinkName());
-      std::vector<UsdError> poseResolutionUSDErrors;
-      for (const auto & error : poseResolutionErrors)
-      {
-        poseResolutionUSDErrors.emplace_back(error);
-      }
       if (!poseResolutionErrors.empty())
       {
         errors.push_back(UsdError(
               sdf::Error(sdf::ErrorCode::POSE_RELATIVE_TO_INVALID,
               "Unable to get the pose of joint [" + _joint.Name() +
               "] w.r.t. its parent link [" + _joint.ParentLinkName() + "].")));
-        errors.insert(
-          errors.end(),
-          poseResolutionUSDErrors.begin(),
-          poseResolutionUSDErrors.end());
+        for (const auto &e : poseResolutionErrors)
+          errors.push_back(UsdError(e));
         return errors;
       }
     }
 
     ignition::math::Pose3d childToJoint;
     auto poseResolutionErrors = _joint.SemanticPose().Resolve(childToJoint,
         _joint.ChildLinkName());
-    std::vector<UsdError> poseResolutionUSDErrors;
-    for (const auto & error : poseResolutionErrors)
-    {
-      poseResolutionUSDErrors.emplace_back(error);
-    }
     if (!poseResolutionErrors.empty())
     {
       errors.push_back(UsdError(
           sdf::Error(sdf::ErrorCode::POSE_RELATIVE_TO_INVALID,
             "Unable to get the pose of joint [" + _joint.Name() +
             "] w.r.t. its child [" + _joint.ChildLinkName() + "].")));
-      errors.insert(
-        errors.end(),
-        poseResolutionUSDErrors.begin(),
-        poseResolutionUSDErrors.end());
+      for (const auto &e : poseResolutionErrors)
+        errors.push_back(UsdError(e));
       return errors;
     }
 
@@ -142,7 +130,7 @@ namespace usd
           childToJoint.Rot().Z());
 
     const auto axis = _joint.Axis();
-    // TODO(anyone): Review this logic which convert a Y axis into a X axis.
+    // TODO(anyone) Review this logic which converts a Y axis into a X axis.
     if (axis && (axis->Xyz() == ignition::math::Vector3d::UnitY))
     {
       if (auto jointRevolute = pxr::UsdPhysicsRevoluteJoint(_jointPrim))
@@ -229,10 +217,12 @@ namespace usd
     }
     else
     {
+      std::stringstream axisStr;
+      axisStr << axis->Xyz();
       errors.push_back(UsdError(sdf::Error(sdf::ErrorCode::ELEMENT_INVALID,
-        "Revolute joint [" + _joint.Name() + "] has specified an axis "
-        "[x y z], but USD only supports integer values of [0, 1] when"
-        "specifying joint axis unit vectors.")));
+        "Revolute joint [" + _joint.Name() + "] has specified an axis ["
+        + axisStr.str() + "], but USD only supports integer values of [0, 1] "
+        "when specifying joint axis unit vectors.")));
       return errors;
     }
 
@@ -256,10 +246,8 @@ namespace usd
     }
 
     // TODO(ahcorde) Review damping and stiffness values
-    double damping = axis->Damping();
-    drive.CreateDampingAttr().Set(static_cast<float>(damping));
-    double stiffness = axis->Stiffness();
-    drive.CreateStiffnessAttr().Set(static_cast<float>(stiffness));
+    drive.CreateDampingAttr().Set(static_cast<float>(axis->Damping()));
+    drive.CreateStiffnessAttr().Set(static_cast<float>(axis->Stiffness()));
     drive.CreateMaxForceAttr().Set(static_cast<float>(axis->Effort()));
 
     return errors;
@@ -302,10 +290,12 @@ namespace usd
     }
     else
     {
+      std::stringstream axisStr;
+      axisStr << axis->Xyz();
       errors.push_back(UsdError(sdf::Error(sdf::ErrorCode::ELEMENT_INVALID,
-        "Prismatic joint [" + _joint.Name() + "] has specified an axis "
-        "[x y z], but USD only supports integer values of [0, 1] when"
-        "specifying joint axis unit vectors.")));
+        "Prismatic joint [" + _joint.Name() + "] has specified an axis ["
+        + axisStr.str() + "], but USD only supports integer values of [0, 1] "
+        "when specifying joint axis unit vectors.")));
       return errors;
     }
 

usd/src/sdf_parser/Joint_Sdf2Usd_TEST.cc

@@ -309,21 +309,17 @@ TEST_F(UsdJointStageFixture, RevoluteJoints)
     ASSERT_TRUE(dampingAttr);
     float usdDamping;
     EXPECT_TRUE(dampingAttr.Get(&usdDamping));
-    const auto sdfDamping =
-      ignition::math::equal(0.0, sdfJoint->Axis()->Damping()) ?
-      35.0f : static_cast<float>(sdfJoint->Axis()->Damping());
-    EXPECT_FLOAT_EQ(usdDamping, sdfDamping);
+    EXPECT_FLOAT_EQ(usdDamping,
+        static_cast<float>(sdfJoint->Axis()->Damping()));
 
     // check stiffness
     const auto stiffnessAttr =
       prim.GetAttribute(pxr::TfToken(driveApiAttrPrefix + "stiffness"));
     ASSERT_TRUE(stiffnessAttr);
     float usdStiffness;
     EXPECT_TRUE(stiffnessAttr.Get(&usdStiffness));
-    const auto sdfStiffness =
-      ignition::math::equal(1e8, sdfJoint->Axis()->Stiffness()) ?
-      350 : static_cast<float>(sdfJoint->Axis()->Stiffness());
-    EXPECT_FLOAT_EQ(usdStiffness, sdfStiffness);
+    EXPECT_FLOAT_EQ(usdStiffness,
+        static_cast<float>(sdfJoint->Axis()->Stiffness()));
 
     // check max force/effort
     const auto maxForceAttr =