Add tests to python binding (#752)

Summary:
Pull Request resolved: #752

Add python tests for marker sequence io in fbx and the unified save function.

Reviewed By: jeongseok-meta

Differential Revision: D85739418

Author: yutingye

momentum/io/fbx/fbx_io.cpp

@@ -318,6 +318,9 @@ std::vector<::fbxsdk::FbxNode*> createMarkerNodes(
     return markerNodes;
   }
 
+  // Set the framerate for the scene
+  setFrameRate(scene, framerate);
+
   // Create a root node for all markers
   ::fbxsdk::FbxNode* markersRootNode = ::fbxsdk::FbxNode::Create(scene, "Markers");
   ::fbxsdk::FbxNull* markersRootAttr = ::fbxsdk::FbxNull::Create(scene, "MarkersRootNull");

momentum/io/fbx/openfbx_loader.cpp

@@ -1058,6 +1058,7 @@ parseMarkerSequence(const ofbx::IScene* scene, const ofbx::Object* root, const f
 
   markersRoot = findMarkersRoot(root);
   if (!markersRoot) {
+    // Return empty sequence with no frames
     return result;
   }
 
@@ -1076,12 +1077,14 @@ parseMarkerSequence(const ofbx::IScene* scene, const ofbx::Object* root, const f
   }
 
   if (markerNodes.empty()) {
+    // Return empty sequence with no frames
     return result;
   }
 
   // Get animation data for all marker nodes
   const ofbx::AnimationStack* animStack = scene->getAnimationStack(0);
   if (!animStack) {
+    // Return empty sequence with no frames
     return result;
   }
 
@@ -1105,6 +1108,7 @@ parseMarkerSequence(const ofbx::IScene* scene, const ofbx::Object* root, const f
   }
 
   if (markerAnimCurves.empty()) {
+    // Return empty sequence with no frames
     return result;
   }
 
@@ -1131,35 +1135,46 @@ parseMarkerSequence(const ofbx::IScene* scene, const ofbx::Object* root, const f
     }
 
     if (translationCurve) {
-      // Collect all unique keyframe times across X, Y, Z channels
+      // Get the individual animation curves for X, Y, Z components
+      const ofbx::AnimationCurve* curveX = translationCurve->getCurve(0);
+      const ofbx::AnimationCurve* curveY = translationCurve->getCurve(1);
+      const ofbx::AnimationCurve* curveZ = translationCurve->getCurve(2);
+
+      // Collect all unique keyframe times from all three curves
       std::set<ofbx::i64> keyframeTimes;
-      for (int iChannel = 0; iChannel < 3; ++iChannel) {
-        const ofbx::AnimationCurve* channel = translationCurve->getCurve(iChannel);
-        if (channel == nullptr) {
-          continue;
+      if (curveX) {
+        const int keyCount = curveX->getKeyCount();
+        const ofbx::i64* times = curveX->getKeyTime();
+        for (int i = 0; i < keyCount; ++i) {
+          keyframeTimes.insert(times[i]);
         }
-        const int keyCount = channel->getKeyCount();
-        if (keyCount <= 0) {
-          continue;
+      }
+      if (curveY) {
+        const int keyCount = curveY->getKeyCount();
+        const ofbx::i64* times = curveY->getKeyTime();
+        for (int i = 0; i < keyCount; ++i) {
+          keyframeTimes.insert(times[i]);
         }
-        const ofbx::i64* times = channel->getKeyTime();
+      }
+      if (curveZ) {
+        const int keyCount = curveZ->getKeyCount();
+        const ofbx::i64* times = curveZ->getKeyTime();
         for (int i = 0; i < keyCount; ++i) {
           keyframeTimes.insert(times[i]);
         }
       }
 
-      // For each unique keyframe time, add a marker at that frame
-      // This matches GLTF behavior: only add markers at explicitly keyed frames
-      for (ofbx::i64 fbxTime : keyframeTimes) {
+      // Add markers only at frames where keyframes exist
+      for (const ofbx::i64 fbxTime : keyframeTimes) {
         const double timeInSeconds = ofbx::fbxTimeToSeconds(fbxTime);
-        const auto frameIndex = static_cast<size_t>(std::lround(timeInSeconds * fps));
+        const size_t frameIndex = std::round(timeInSeconds * fps);
 
         // Skip if frame index is out of bounds
         if (frameIndex >= numFrames) {
           continue;
         }
 
-        // Evaluate position at this specific keyframe time
+        // Evaluate position at this keyframe time
         const ofbx::DVec3 position = translationCurve->getNodeLocalTransform(timeInSeconds);
 
         Marker marker;

momentum/io/marker/marker_io.cpp

@@ -87,12 +87,9 @@ int findMainSubjectIndex(std::span<const MarkerSequence> markerSequences) {
   }
 
   // Special case when there's only one actor.
+  // Return the actor even if it has no markers (empty sequence is valid).
   if (numActors == 1) {
-    if (maxMarkers.at(0) > 0) {
-      return 0;
-    } else {
-      return -1;
-    }
+    return 0;
   }
 
   // find the max marker count with a name

pymomentum/geometry/momentum_io.cpp

@@ -167,26 +167,18 @@ void saveFBXCharacterToFile(
     const std::optional<const std::vector<std::vector<momentum::Marker>>>& markers,
     const std::optional<const momentum::FBXCoordSystemInfo>& coordSystemInfo,
     std::string_view fbxNamespace) {
-  if (motion.has_value() && offsets.has_value()) {
-    momentum::saveFbx(
-        path,
-        character,
-        motion.value().transpose(),
-        offsets.value(),
-        fps,
-        true, /*saveMesh*/
-        coordSystemInfo.value_or(momentum::FBXCoordSystemInfo()),
-        false, /*permissive*/
-        markers.value_or(std::vector<std::vector<momentum::Marker>>{}),
-        fbxNamespace);
-  } else {
-    momentum::saveFbxModel(
-        path,
-        character,
-        coordSystemInfo.value_or(momentum::FBXCoordSystemInfo()),
-        false, /*permissive*/
-        fbxNamespace);
-  }
+  // Always use saveFbx to support markers even without motion
+  momentum::saveFbx(
+      path,
+      character,
+      motion.has_value() ? motion.value().transpose() : Eigen::MatrixXf(),
+      offsets.has_value() ? offsets.value() : Eigen::VectorXf(),
+      fps,
+      true, /*saveMesh*/
+      coordSystemInfo.value_or(momentum::FBXCoordSystemInfo()),
+      false, /*permissive*/
+      markers.value_or(std::vector<std::vector<momentum::Marker>>{}),
+      fbxNamespace);
 }
 
 void saveFBXCharacterToFileWithJointParams(
@@ -197,25 +189,17 @@ void saveFBXCharacterToFileWithJointParams(
     const std::optional<const std::vector<std::vector<momentum::Marker>>>& markers,
     const std::optional<const momentum::FBXCoordSystemInfo>& coordSystemInfo,
     std::string_view fbxNamespace) {
-  if (jointParams.has_value()) {
-    momentum::saveFbxWithJointParams(
-        path,
-        character,
-        jointParams.value().transpose(),
-        fps,
-        true, /*saveMesh*/
-        coordSystemInfo.value_or(momentum::FBXCoordSystemInfo()),
-        false, /*permissive*/
-        markers.value_or(std::vector<std::vector<momentum::Marker>>{}),
-        fbxNamespace);
-  } else {
-    momentum::saveFbxModel(
-        path,
-        character,
-        coordSystemInfo.value_or(momentum::FBXCoordSystemInfo()),
-        false, /*permissive*/
-        fbxNamespace);
-  }
+  // Always use saveFbxWithJointParams to support markers even without motion
+  momentum::saveFbxWithJointParams(
+      path,
+      character,
+      jointParams.has_value() ? jointParams.value().transpose() : Eigen::MatrixXf(),
+      fps,
+      true, /*saveMesh*/
+      coordSystemInfo.value_or(momentum::FBXCoordSystemInfo()),
+      false, /*permissive*/
+      markers.value_or(std::vector<std::vector<momentum::Marker>>{}),
+      fbxNamespace);
 }
 
 void saveCharacterToFile(