review feedback

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

Author: adlarkin

usd/src/usd_parser/USDJoints.cc

@@ -203,13 +203,7 @@ namespace sdf
         jointAxis.SetMaxVelocity(vel);
       }
 
-      if (_prim.IsA<pxr::UsdPhysicsFixedJoint>())
-      {
-        _joint.SetType(sdf::JointType::FIXED);
-
-        return errors;
-      }
-      else if (_prim.IsA<pxr::UsdPhysicsPrismaticJoint>())
+      if (_prim.IsA<pxr::UsdPhysicsPrismaticJoint>())
       {
         auto variant_physics_prismatic_joint =
           pxr::UsdPhysicsPrismaticJoint(_prim);
@@ -220,9 +214,9 @@ namespace sdf
         if (!errorsAxis.empty())
         {
           errors.emplace_back(UsdError(
-            sdf::usd::UsdErrorCode::SDF_TO_USD_PARSING_ERROR,
-              std::string("Errors encountered when setting xyz of prismatic") +
-              "_joint axis: [" + std::string(_prim.GetName()) + "]"));
+            sdf::usd::UsdErrorCode::USD_TO_SDF_PARSING_ERROR,
+              "Errors encountered when setting xyz of prismatic "
+              "joint axis: [" + std::string(_prim.GetName()) + "]"));
           for (const auto & error : errorsAxis)
             errors.emplace_back(error);
           return errors;
@@ -250,9 +244,9 @@ namespace sdf
         if (!errorsAxis.empty())
         {
           errors.emplace_back(UsdError(
-            sdf::usd::UsdErrorCode::SDF_TO_USD_PARSING_ERROR,
-              std::string("Errors encountered when setting xyz of revolute") +
-              "_joint axis: [" + std::string(_prim.GetName()) + "]"));
+            sdf::usd::UsdErrorCode::USD_TO_SDF_PARSING_ERROR,
+              "Errors encountered when setting xyz of revolute "
+              "joint axis: [" + std::string(_prim.GetName()) + "]"));
           for (const auto & error : errorsAxis)
             errors.emplace_back(error);
           return errors;
@@ -268,10 +262,20 @@ namespace sdf
 
         return errors;
       }
-      else if (_prim.IsA<pxr::UsdPhysicsJoint>())
+      else if (_prim.IsA<pxr::UsdPhysicsFixedJoint>() ||
+          _prim.IsA<pxr::UsdPhysicsJoint>())
       {
         _joint.SetType(sdf::JointType::FIXED);
       }
+      else
+      {
+        errors.emplace_back(UsdError(
+              sdf::usd::UsdErrorCode::USD_TO_SDF_PARSING_ERROR,
+              "Unable to create a SDF joint from USD prim [" +
+              std::string(_prim.GetName()) +
+              "] because the prim is not a USD joint."));
+      }
+
       return errors;
     }
   }

usd/src/usd_parser/USDJoints.hh

@@ -41,7 +41,6 @@ namespace sdf
     /// a joint
     ///
     /// \param[in] _prim The USD prim that holds the USD joint
-    /// \param[in] _path The path to _prim
     /// \param[in] _usdData Object that holds data about the USD stage
     /// \param[out] _joint SDF joint to return
     /// \return UsdErrors, which is a list of UsdError objects. An empty list

usd/src/usd_parser/USDJoints_TEST.cc

@@ -59,13 +59,15 @@ TEST(USDJointTest, JointTest)
     upperJoint, usdData, joint1);
 
   EXPECT_EQ(0u, errors.size());
+  EXPECT_EQ(sdf::JointType::REVOLUTE, joint1.Type());
   EXPECT_EQ("upper_joint", joint1.Name());
   EXPECT_EQ("upper_link", joint1.ChildLinkName());
   EXPECT_EQ("base", joint1.ParentLinkName());
   EXPECT_EQ(ignition::math::Pose3d(0, 0, 0.021, -1.5708, 0, 0),
             joint1.RawPose());
 
   auto axis = joint1.Axis(0);
+  ASSERT_NE(nullptr, axis);
   EXPECT_EQ(ignition::math::Vector3d(1, 0, 0), axis->Xyz());
 
   const auto lowerJoint = stage->GetPrimAtPath(pxr::SdfPath(
@@ -77,11 +79,13 @@ TEST(USDJointTest, JointTest)
     lowerJoint, usdData, joint2);
 
   EXPECT_EQ(0u, errors.size());
+  EXPECT_EQ(sdf::JointType::REVOLUTE, joint2.Type());
   EXPECT_EQ("lower_joint", joint2.Name());
   EXPECT_EQ("lower_link", joint2.ChildLinkName());
   EXPECT_EQ("upper_link", joint2.ParentLinkName());
   EXPECT_EQ(ignition::math::Pose3d(0.0025, -0, 0.01, -0.4292, 0, 0),
             joint2.RawPose());
   axis = joint2.Axis(0);
+  ASSERT_NE(nullptr, axis);
   EXPECT_EQ(ignition::math::Vector3d(1, 0, 0), axis->Xyz());
 }

usd/src/usd_parser/USDLinks_TEST.cc

@@ -70,11 +70,11 @@ TEST(USDLinksTest, LinksNameMassAndDiagonalMoments)
         ASSERT_NE(nullptr, visual);
         auto geom = visual->Geom();
         ASSERT_NE(nullptr, geom);
+        EXPECT_EQ(_type, geom->Type());
         auto collision = _link.CollisionByIndex(0);
         ASSERT_NE(nullptr, collision);
         auto geomCol = collision->Geom();
         ASSERT_NE(nullptr, geomCol);
-        EXPECT_EQ(_type, geom->Type());
       }
     };
 

usd/src/usd_parser/USDWorld.cc

@@ -203,16 +203,15 @@ namespace usd
 
       if (prim.IsA<pxr::UsdPhysicsJoint>())
       {
-        sdf::Joint joint;
-        modelPtr = _world.ModelByName(currentModelName);
         if (!modelPtr)
         {
           errors.push_back(UsdError(UsdErrorCode::USD_TO_SDF_PARSING_ERROR,
-            "Unable to find a sdf::Model named [" + currentModelName +
-            "] in world named [" + _world.Name() +
-            "], but a sdf::Model with this name should exist."));
+            "Unable to parse joint corresponding to USD prim [" +
+            std::string(prim.GetName()) +
+            "] because the corresponding sdf::Model object wasn't found."));
           return errors;
         }
+        sdf::Joint joint;
         auto errorsJoint = ParseJoints(prim, usdData, joint);
         if (!errorsJoint.empty())
         {