From 2e8a9b8bdc2f227225240807342e4316e90ce03e Mon Sep 17 00:00:00 2001 From: Carsten Stoll Date: Wed, 4 Mar 2026 12:44:43 -0800 Subject: [PATCH 1/3] Reduce CCN in json_utils.cpp (parameterLimitsToJson/FromJson) (#1085) Summary: Pull Request resolved: https://github.com/facebookresearch/momentum/pull/1085 Refactored the `parameterLimitsToJson` and `parameterLimitsFromJson` functions to reduce their cyclomatic complexity (CCN) below the target of 20. Changes made: - Created per-type helper functions in an anonymous namespace for both serialization (toJson) and deserialization (fromJson) operations - Added helper functions: `minMaxToJson`, `minMaxJointToJson`, `linearToJson`, `linearJointToJson`, `halfPlaneToJson`, `ellipsoidToJson` for serialization - Added helper functions: `parseJointParameterIndex`, `minMaxFromJson`, `minMaxJointFromJson`, `linearFromJson`, `linearJointFromJson`, `halfPlaneFromJson`, `ellipsoidFromJson` for deserialization - The main `parameterLimitsToJson` and `parameterLimitsFromJson` functions now act as simple dispatchers calling the appropriate helper functions - Combined `MinMaxJoint` and `MinMaxJointPassive` handling using a shared helper with a type parameter - Combined `ellipsoid` and deprecated `elipsoid` (typo) handling using shared helper with key parameters - Preserved all existing comments, including the TODO for deprecated `elipsoid` type migration Reviewed By: cdtwigg Differential Revision: D95214692 fbshipit-source-id: 085397d11385d7d5a90a601e4d0a268d0803ed67 --- momentum/io/gltf/utils/json_utils.cpp | 397 ++++++++++++++------------ 1 file changed, 220 insertions(+), 177 deletions(-) diff --git a/momentum/io/gltf/utils/json_utils.cpp b/momentum/io/gltf/utils/json_utils.cpp index b7b30621df..9a149b998b 100644 --- a/momentum/io/gltf/utils/json_utils.cpp +++ b/momentum/io/gltf/utils/json_utils.cpp @@ -218,6 +218,208 @@ ParameterTransform parameterTransformFromJson(const Character& character, const return pt; } +namespace { + +void minMaxToJson(const Character& character, const ParameterLimit& lim, nlohmann::json& li) { + li["type"] = "minmax"; + MT_THROW_IF( + character.parameterTransform.name.empty() || + lim.data.minMax.parameterIndex >= character.parameterTransform.name.size(), + "MinMax parameter index {} is out of bounds (name size: {})", + lim.data.minMax.parameterIndex, + character.parameterTransform.name.size()); + li["parameter"] = character.parameterTransform.name[lim.data.minMax.parameterIndex]; + li["limits"] = lim.data.minMax.limits; +} + +void minMaxJointToJson( + const Character& character, + const ParameterLimit& lim, + nlohmann::json& li, + const char* typeName) { + li["type"] = typeName; + if (!character.skeleton.joints.empty() && + lim.data.minMaxJoint.jointIndex < character.skeleton.joints.size()) { + li["jointIndex"] = character.skeleton.joints[lim.data.minMaxJoint.jointIndex].name; + } + li["jointParameter"] = kJointParameterNames[lim.data.minMaxJoint.jointParameter]; + li["limits"] = lim.data.minMaxJoint.limits; +} + +void linearToJson(const Character& character, const ParameterLimit& lim, nlohmann::json& li) { + li["type"] = "linear"; + MT_THROW_IF( + character.parameterTransform.name.empty() || + lim.data.linear.referenceIndex >= character.parameterTransform.name.size(), + "Linear referenceIndex {} is out of bounds (name size: {})", + lim.data.linear.referenceIndex, + character.parameterTransform.name.size()); + li["referenceParameter"] = character.parameterTransform.name[lim.data.linear.referenceIndex]; + MT_THROW_IF( + character.parameterTransform.name.empty() || + lim.data.linear.targetIndex >= character.parameterTransform.name.size(), + "Linear targetIndex {} is out of bounds (name size: {})", + lim.data.linear.targetIndex, + character.parameterTransform.name.size()); + li["targetParameter"] = character.parameterTransform.name[lim.data.linear.targetIndex]; + li["scale"] = lim.data.linear.scale; + li["offset"] = lim.data.linear.offset; + if (lim.data.linear.rangeMin != -std::numeric_limits::max()) { + li["rangeMin"] = lim.data.linear.rangeMin; + } + if (lim.data.linear.rangeMax != std::numeric_limits::max()) { + li["rangeMax"] = lim.data.linear.rangeMax; + } +} + +void linearJointToJson(const Character& character, const ParameterLimit& lim, nlohmann::json& li) { + li["type"] = "linear_joint"; + if (!character.skeleton.joints.empty() && + lim.data.linearJoint.referenceJointIndex < character.skeleton.joints.size()) { + li["referenceJoint"] = character.skeleton.joints[lim.data.linearJoint.referenceJointIndex].name; + } + li["referenceJointParameter"] = lim.data.linearJoint.referenceJointParameter; + if (!character.skeleton.joints.empty() && + lim.data.linearJoint.targetJointIndex < character.skeleton.joints.size()) { + li["targetJoint"] = character.skeleton.joints[lim.data.linearJoint.targetJointIndex].name; + } + li["targetJointParameter"] = lim.data.linearJoint.targetJointParameter; + li["scale"] = lim.data.linearJoint.scale; + li["offset"] = lim.data.linearJoint.offset; + if (lim.data.linearJoint.rangeMin != -std::numeric_limits::max()) { + li["rangeMin"] = lim.data.linearJoint.rangeMin; + } + if (lim.data.linearJoint.rangeMax != std::numeric_limits::max()) { + li["rangeMax"] = lim.data.linearJoint.rangeMax; + } +} + +void halfPlaneToJson(const Character& character, const ParameterLimit& lim, nlohmann::json& li) { + li["type"] = "half_plane"; + MT_THROW_IF( + character.parameterTransform.name.empty() || + lim.data.halfPlane.param1 >= character.parameterTransform.name.size(), + "HalfPlane param1 index {} is out of bounds (name size: {})", + lim.data.halfPlane.param1, + character.parameterTransform.name.size()); + li["param1"] = character.parameterTransform.name[lim.data.halfPlane.param1]; + MT_THROW_IF( + character.parameterTransform.name.empty() || + lim.data.halfPlane.param2 >= character.parameterTransform.name.size(), + "HalfPlane param2 index {} is out of bounds (name size: {})", + lim.data.halfPlane.param2, + character.parameterTransform.name.size()); + li["param2"] = character.parameterTransform.name[lim.data.halfPlane.param2]; + li["normal"] = lim.data.halfPlane.normal; + li["offset"] = lim.data.halfPlane.offset; +} + +void ellipsoidToJson(const Character& character, const ParameterLimit& lim, nlohmann::json& li) { + li["type"] = "ellipsoid"; + if (!character.skeleton.joints.empty() && + lim.data.ellipsoid.parent < character.skeleton.joints.size()) { + li["parent"] = character.skeleton.joints[lim.data.ellipsoid.parent].name; + } + if (!character.skeleton.joints.empty() && + lim.data.ellipsoid.ellipsoidParent < character.skeleton.joints.size()) { + li["ellipsoidParent"] = character.skeleton.joints[lim.data.ellipsoid.ellipsoidParent].name; + } + li["offset"] = lim.data.ellipsoid.offset * toM(); + auto eli = lim.data.ellipsoid.ellipsoid; + eli.translation() *= toM(); + toJson(eli.matrix(), li["ellipsoid"]); +} + +size_t parseJointParameterIndex(const std::string& attribute) { + for (size_t t = 0; t < kParametersPerJoint; t++) { + if (attribute == kJointParameterNames[t]) { + return t; + } + } + return kInvalidIndex; +} + +void minMaxFromJson(const Character& character, const nlohmann::json& element, ParameterLimit& l) { + l.type = MinMax; + l.data.minMax.parameterIndex = + character.parameterTransform.getParameterIdByName(element.value("parameter", "")); + l.data.minMax.limits = fromJson(element["limits"]); +} + +void minMaxJointFromJson( + const Character& character, + const nlohmann::json& element, + ParameterLimit& l, + LimitType limitType) { + l.type = limitType; + l.data.minMaxJoint.jointIndex = + character.skeleton.getJointIdByName(element.value("jointIndex", "")); + const std::string attribute = element.value("jointParameter", ""); + l.data.minMaxJoint.jointParameter = parseJointParameterIndex(attribute); + l.data.minMaxJoint.limits = fromJson(element["limits"]); +} + +void linearFromJson(const Character& character, const nlohmann::json& element, ParameterLimit& l) { + l.type = Linear; + l.data.linear.referenceIndex = + character.parameterTransform.getParameterIdByName(element.value("referenceParameter", "")); + l.data.linear.targetIndex = + character.parameterTransform.getParameterIdByName(element.value("targetParameter", "")); + l.data.linear.scale = element["scale"]; + l.data.linear.offset = element["offset"]; + l.data.linear.rangeMin = element.value("rangeMin", -std::numeric_limits::max()); + l.data.linear.rangeMax = element.value("rangeMax", std::numeric_limits::max()); +} + +void linearJointFromJson( + const Character& character, + const nlohmann::json& element, + ParameterLimit& l) { + l.type = LinearJoint; + l.data.linearJoint.referenceJointIndex = + character.skeleton.getJointIdByName(element.value("referenceJoint", "")); + l.data.linearJoint.targetJointIndex = + character.skeleton.getJointIdByName(element.value("targetJoint", "")); + l.data.linearJoint.referenceJointParameter = element["referenceJointParameter"].get(); + l.data.linearJoint.targetJointParameter = element["targetJointParameter"].get(); + l.data.linearJoint.scale = element["scale"]; + l.data.linearJoint.offset = element["offset"]; + l.data.linearJoint.rangeMin = element.value("rangeMin", -std::numeric_limits::max()); + l.data.linearJoint.rangeMax = element.value("rangeMax", std::numeric_limits::max()); +} + +void halfPlaneFromJson( + const Character& character, + const nlohmann::json& element, + ParameterLimit& l) { + l.type = HalfPlane; + l.data.halfPlane.param1 = + character.parameterTransform.getParameterIdByName(element.value("param1", "")); + l.data.halfPlane.param2 = + character.parameterTransform.getParameterIdByName(element.value("param2", "")); + l.data.halfPlane.normal = fromJson(element["normal"]); + l.data.halfPlane.offset = element["offset"]; +} + +void ellipsoidFromJson( + const Character& character, + const nlohmann::json& element, + ParameterLimit& l, + const std::string& parentKey, + const std::string& ellipsoidKey) { + l.type = Ellipsoid; + l.data.ellipsoid.parent = character.skeleton.getJointIdByName(element.value("parent", "")); + l.data.ellipsoid.ellipsoidParent = + character.skeleton.getJointIdByName(element.value(parentKey, "")); + l.data.ellipsoid.offset = fromJson(element["offset"]); + l.data.ellipsoid.offset /= toM(); + l.data.ellipsoid.ellipsoid.matrix() = fromJson(element[ellipsoidKey]); + l.data.ellipsoid.ellipsoid.translation() /= toM(); + l.data.ellipsoid.ellipsoidInv = l.data.ellipsoid.ellipsoid.inverse(); +} + +} // namespace + void parameterLimitsToJson(const Character& character, nlohmann::json& j) { j = nlohmann::json::array(); for (const auto& lim : character.parameterLimits) { @@ -227,125 +429,32 @@ void parameterLimitsToJson(const Character& character, nlohmann::json& j) { switch (lim.type) { case MinMax: - li["type"] = "minmax"; - MT_THROW_IF( - character.parameterTransform.name.empty() || - lim.data.minMax.parameterIndex >= character.parameterTransform.name.size(), - "MinMax parameter index {} is out of bounds (name size: {})", - lim.data.minMax.parameterIndex, - character.parameterTransform.name.size()); - li["parameter"] = character.parameterTransform.name[lim.data.minMax.parameterIndex]; - li["limits"] = lim.data.minMax.limits; + minMaxToJson(character, lim, li); break; case MinMaxJoint: - li["type"] = "minmax_joint"; - if (!character.skeleton.joints.empty() && - lim.data.minMaxJoint.jointIndex < character.skeleton.joints.size()) { - li["jointIndex"] = character.skeleton.joints[lim.data.minMaxJoint.jointIndex].name; - } - li["jointParameter"] = kJointParameterNames[lim.data.minMaxJoint.jointParameter]; - li["limits"] = lim.data.minMaxJoint.limits; + minMaxJointToJson(character, lim, li, "minmax_joint"); break; case MinMaxJointPassive: - li["type"] = "minmax_joint_passive"; - if (!character.skeleton.joints.empty() && - lim.data.minMaxJoint.jointIndex < character.skeleton.joints.size()) { - li["jointIndex"] = character.skeleton.joints[lim.data.minMaxJoint.jointIndex].name; - } - li["jointParameter"] = kJointParameterNames[lim.data.minMaxJoint.jointParameter]; - li["limits"] = lim.data.minMaxJoint.limits; + minMaxJointToJson(character, lim, li, "minmax_joint_passive"); break; case Linear: - li["type"] = "linear"; - MT_THROW_IF( - character.parameterTransform.name.empty() || - lim.data.linear.referenceIndex >= character.parameterTransform.name.size(), - "Linear referenceIndex {} is out of bounds (name size: {})", - lim.data.linear.referenceIndex, - character.parameterTransform.name.size()); - li["referenceParameter"] = - character.parameterTransform.name[lim.data.linear.referenceIndex]; - MT_THROW_IF( - character.parameterTransform.name.empty() || - lim.data.linear.targetIndex >= character.parameterTransform.name.size(), - "Linear targetIndex {} is out of bounds (name size: {})", - lim.data.linear.targetIndex, - character.parameterTransform.name.size()); - li["targetParameter"] = character.parameterTransform.name[lim.data.linear.targetIndex]; - li["scale"] = lim.data.linear.scale; - li["offset"] = lim.data.linear.offset; - if (lim.data.linear.rangeMin != -std::numeric_limits::max()) { - li["rangeMin"] = lim.data.linear.rangeMin; - } - if (lim.data.linear.rangeMax != std::numeric_limits::max()) { - li["rangeMax"] = lim.data.linear.rangeMax; - } + linearToJson(character, lim, li); break; case LinearJoint: - li["type"] = "linear_joint"; - if (!character.skeleton.joints.empty() && - lim.data.linearJoint.referenceJointIndex < character.skeleton.joints.size()) { - li["referenceJoint"] = - character.skeleton.joints[lim.data.linearJoint.referenceJointIndex].name; - } - li["referenceJointParameter"] = lim.data.linearJoint.referenceJointParameter; - if (!character.skeleton.joints.empty() && - lim.data.linearJoint.targetJointIndex < character.skeleton.joints.size()) { - li["targetJoint"] = character.skeleton.joints[lim.data.linearJoint.targetJointIndex].name; - } - li["targetJointParameter"] = lim.data.linearJoint.targetJointParameter; - li["scale"] = lim.data.linearJoint.scale; - li["offset"] = lim.data.linearJoint.offset; - if (lim.data.linearJoint.rangeMin != -std::numeric_limits::max()) { - li["rangeMin"] = lim.data.linearJoint.rangeMin; - } - if (lim.data.linearJoint.rangeMax != std::numeric_limits::max()) { - li["rangeMax"] = lim.data.linearJoint.rangeMax; - } + linearJointToJson(character, lim, li); break; case HalfPlane: - li["type"] = "half_plane"; - MT_THROW_IF( - character.parameterTransform.name.empty() || - lim.data.halfPlane.param1 >= character.parameterTransform.name.size(), - "HalfPlane param1 index {} is out of bounds (name size: {})", - lim.data.halfPlane.param1, - character.parameterTransform.name.size()); - li["param1"] = character.parameterTransform.name[lim.data.halfPlane.param1]; - MT_THROW_IF( - character.parameterTransform.name.empty() || - lim.data.halfPlane.param2 >= character.parameterTransform.name.size(), - "HalfPlane param2 index {} is out of bounds (name size: {})", - lim.data.halfPlane.param2, - character.parameterTransform.name.size()); - li["param2"] = character.parameterTransform.name[lim.data.halfPlane.param2]; - li["normal"] = lim.data.halfPlane.normal; - li["offset"] = lim.data.halfPlane.offset; + halfPlaneToJson(character, lim, li); break; - case Ellipsoid: { - li["type"] = "ellipsoid"; - if (!character.skeleton.joints.empty() && - lim.data.ellipsoid.parent < character.skeleton.joints.size()) { - li["parent"] = character.skeleton.joints[lim.data.ellipsoid.parent].name; - } - if (!character.skeleton.joints.empty() && - lim.data.ellipsoid.ellipsoidParent < character.skeleton.joints.size()) { - li["ellipsoidParent"] = - character.skeleton.joints[lim.data.ellipsoid.ellipsoidParent].name; - } - li["offset"] = lim.data.ellipsoid.offset * toM(); - auto eli = lim.data.ellipsoid.ellipsoid; - eli.translation() *= toM(); - toJson(eli.matrix(), li["ellipsoid"]); + case Ellipsoid: + ellipsoidToJson(character, lim, li); break; - } - default: { + default: MT_LOGE( "Unknown parameter limit type '{}' from character name '{}'", toString(lim.type), character.name); break; - } } } } @@ -357,92 +466,26 @@ ParameterLimits parameterLimitsFromJson(const Character& character, const nlohma const std::string type = element.value("type", ""); ParameterLimit l; l.weight = element.value("weight", 0.0f); + if (type == "minmax") { - l.type = MinMax; - l.data.minMax.parameterIndex = - character.parameterTransform.getParameterIdByName(element.value("parameter", "")); - l.data.minMax.limits = fromJson(element["limits"]); + minMaxFromJson(character, element, l); } else if (type == "minmax_joint") { - l.type = MinMaxJoint; - l.data.minMaxJoint.jointIndex = - character.skeleton.getJointIdByName(element.value("jointIndex", "")); - const std::string attribute = element.value("jointParameter", ""); - size_t attributeIndex = kInvalidIndex; - for (size_t t = 0; t < kParametersPerJoint; t++) { - if (attribute == kJointParameterNames[t]) { - attributeIndex = t; - break; - } - } - l.data.minMaxJoint.jointParameter = attributeIndex; - l.data.minMaxJoint.limits = fromJson(element["limits"]); + minMaxJointFromJson(character, element, l, MinMaxJoint); } else if (type == "minmax_joint_passive") { - l.type = MinMaxJointPassive; - l.data.minMaxJoint.jointIndex = - character.skeleton.getJointIdByName(element.value("jointIndex", "")); - const std::string attribute = element.value("jointParameter", ""); - size_t attributeIndex = kInvalidIndex; - for (size_t t = 0; t < kParametersPerJoint; t++) { - if (attribute == kJointParameterNames[t]) { - attributeIndex = t; - break; - } - } - l.data.minMaxJoint.jointParameter = attributeIndex; - l.data.minMaxJoint.limits = fromJson(element["limits"]); + minMaxJointFromJson(character, element, l, MinMaxJointPassive); } else if (type == "linear") { - l.type = Linear; - l.data.linear.referenceIndex = character.parameterTransform.getParameterIdByName( - element.value("referenceParameter", "")); - l.data.linear.targetIndex = - character.parameterTransform.getParameterIdByName(element.value("targetParameter", "")); - l.data.linear.scale = element["scale"]; - l.data.linear.offset = element["offset"]; - l.data.linear.rangeMin = element.value("rangeMin", -std::numeric_limits::max()); - l.data.linear.rangeMax = element.value("rangeMax", std::numeric_limits::max()); + linearFromJson(character, element, l); } else if (type == "linear_joint") { - l.type = LinearJoint; - l.data.linearJoint.referenceJointIndex = - character.skeleton.getJointIdByName(element.value("referenceJoint", "")); - l.data.linearJoint.targetJointIndex = - character.skeleton.getJointIdByName(element.value("targetJoint", "")); - l.data.linearJoint.referenceJointParameter = element["referenceJointParameter"].get(); - l.data.linearJoint.targetJointParameter = element["targetJointParameter"].get(); - l.data.linearJoint.scale = element["scale"]; - l.data.linearJoint.offset = element["offset"]; - l.data.linearJoint.rangeMin = element.value("rangeMin", -std::numeric_limits::max()); - l.data.linearJoint.rangeMax = element.value("rangeMax", std::numeric_limits::max()); + linearJointFromJson(character, element, l); } else if (type == "half_plane") { - l.type = HalfPlane; - l.data.halfPlane.param1 = - character.parameterTransform.getParameterIdByName(element.value("param1", "")); - l.data.halfPlane.param2 = - character.parameterTransform.getParameterIdByName(element.value("param2", "")); - l.data.halfPlane.normal = fromJson(element["normal"]); - l.data.halfPlane.offset = element["offset"]; + halfPlaneFromJson(character, element, l); } else if (type == "ellipsoid") { - l.type = Ellipsoid; - l.data.ellipsoid.parent = character.skeleton.getJointIdByName(element.value("parent", "")); - l.data.ellipsoid.ellipsoidParent = - character.skeleton.getJointIdByName(element.value("ellipsoidParent", "")); - l.data.ellipsoid.offset = fromJson(element["offset"]); - l.data.ellipsoid.offset /= toM(); - l.data.ellipsoid.ellipsoid.matrix() = fromJson(element["ellipsoid"]); - l.data.ellipsoid.ellipsoid.translation() /= toM(); - l.data.ellipsoid.ellipsoidInv = l.data.ellipsoid.ellipsoid.inverse(); + ellipsoidFromJson(character, element, l, "ellipsoidParent", "ellipsoid"); } else if (type == "elipsoid") { // TODO: Remove once all the model files are migrated to ellipsoid MT_LOGW_ONCE( "Deprecated parameter limit type: {} (typo). Please use 'ellipsoid' instead.", type); - l.type = Ellipsoid; - l.data.ellipsoid.parent = character.skeleton.getJointIdByName(element.value("parent", "")); - l.data.ellipsoid.ellipsoidParent = - character.skeleton.getJointIdByName(element.value("elipsoidParent", "")); - l.data.ellipsoid.offset = fromJson(element["offset"]); - l.data.ellipsoid.offset /= toM(); - l.data.ellipsoid.ellipsoid.matrix() = fromJson(element["elipsoid"]); - l.data.ellipsoid.ellipsoid.translation() /= toM(); - l.data.ellipsoid.ellipsoidInv = l.data.ellipsoid.ellipsoid.inverse(); + ellipsoidFromJson(character, element, l, "elipsoidParent", "elipsoid"); } else { MT_THROW("Unknown parameter limit type '{}' from character name '{}'.", type, character.name); } From 91a12b9c87b7fa754fd6d6ed2c02b4cd30132150 Mon Sep 17 00:00:00 2001 From: Chris Twigg Date: Wed, 4 Mar 2026 14:15:16 -0800 Subject: [PATCH 2/3] Add polygon face support to MeshT (#1066) Summary: MeshT currently only stores triangle faces (std::vector faces). Characters can have arbitrary polygon topology (quads, n-gons) which is lost during FBX loading when polygons are fan-triangulated. This adds a redundant polygon representation alongside the existing triangle faces so that polygon topology can be preserved. The existing faces field stays unchanged and all downstream code continues to work. Three new fields are added to MeshT: - polyFaces: packed polygon vertex indices (all polygons concatenated) - polyFaceSizes: number of vertices per polygon face - polyTexcoordFaces: packed polygon texcoord indices (shares polyFaceSizes) Also updates cast()/reset(), adds tests, updates compareMeshes(), adds Python bindings with validation, and regenerates .pyi stubs. Reviewed By: nickyhe-gemini Differential Revision: D94577296 --- momentum/math/mesh.cpp | 6 ++ momentum/math/mesh.h | 20 +++++ .../character/character_helpers_gtest.cpp | 6 ++ momentum/test/math/mesh_test.cpp | 87 ++++++++++++++++++ pymomentum/geometry/mesh_pybind.cpp | 90 ++++++++++++++++++- 5 files changed, 205 insertions(+), 4 deletions(-) diff --git a/momentum/math/mesh.cpp b/momentum/math/mesh.cpp index a94b83b48f..14a33bb6c4 100644 --- a/momentum/math/mesh.cpp +++ b/momentum/math/mesh.cpp @@ -69,6 +69,9 @@ MeshT MeshT::cast() const { result.texcoords = this->texcoords; result.texcoord_faces = this->texcoord_faces; result.texcoord_lines = this->texcoord_lines; + result.polyFaces = this->polyFaces; + result.polyFaceSizes = this->polyFaceSizes; + result.polyTexcoordFaces = this->polyTexcoordFaces; return result; } @@ -83,6 +86,9 @@ void MeshT::reset() { texcoords.clear(); texcoord_faces.clear(); texcoord_lines.clear(); + polyFaces.clear(); + polyFaceSizes.clear(); + polyTexcoordFaces.clear(); } template MeshT MeshT::cast() const; diff --git a/momentum/math/mesh.h b/momentum/math/mesh.h index 01c2e54076..9eb7da9d3f 100644 --- a/momentum/math/mesh.h +++ b/momentum/math/mesh.h @@ -56,6 +56,26 @@ struct MeshT { /// Maps each line to its corresponding texture coordinates std::vector> texcoord_lines; + /// Packed polygon face vertex indices (all polygons concatenated back-to-back). + /// + /// For example, a quad followed by a triangle would be: [v0, v1, v2, v3, v4, v5, v6]. + /// This is an optional, redundant representation — the triangulated @ref faces field is always + /// required and used by all downstream code. Polygon data preserves the original topology (quads, + /// n-gons) from source files like FBX. + std::vector polyFaces; + + /// Number of vertices in each polygon face. + /// + /// For example, [4, 3] means the first polygon is a quad, the second is a triangle. + /// May be empty if polygon data is not available; see @ref polyFaces. + std::vector polyFaceSizes; + + /// Packed polygon face texture coordinate indices (same layout as polyFaces). + /// + /// Uses the same polyFaceSizes array — polygon sizes are identical for geometry and texcoords. + /// May be empty if texture coordinates are not available; see @ref polyFaces. + std::vector polyTexcoordFaces; + /// Compute vertex normals by averaging connected face normals. /// /// This method calculates normals for each vertex by averaging the normals of all diff --git a/momentum/test/character/character_helpers_gtest.cpp b/momentum/test/character/character_helpers_gtest.cpp index b726680f2a..7147603cb6 100644 --- a/momentum/test/character/character_helpers_gtest.cpp +++ b/momentum/test/character/character_helpers_gtest.cpp @@ -100,6 +100,12 @@ void compareMeshes(const Mesh_u& refMesh, const Mesh_u& mesh) { refMesh->confidence, testing::Pointwise(testing::DoubleNear(0.0001), mesh->confidence)); EXPECT_THAT( refMesh->texcoord_faces, testing::Pointwise(IntExactPointwise(), mesh->texcoord_faces)); + // Only compare polygon data when both meshes have it (e.g. glTF doesn't populate poly fields) + if (!refMesh->polyFaces.empty() && !mesh->polyFaces.empty()) { + EXPECT_EQ(refMesh->polyFaces, mesh->polyFaces); + EXPECT_EQ(refMesh->polyFaceSizes, mesh->polyFaceSizes); + EXPECT_EQ(refMesh->polyTexcoordFaces, mesh->polyTexcoordFaces); + } } void compareBlendShapes(const BlendShape_const_p& refShapes, const BlendShape_const_p& shapes) { diff --git a/momentum/test/math/mesh_test.cpp b/momentum/test/math/mesh_test.cpp index 34c3824a63..d0e64fe540 100644 --- a/momentum/test/math/mesh_test.cpp +++ b/momentum/test/math/mesh_test.cpp @@ -38,6 +38,9 @@ TYPED_TEST(MeshTest, DefaultConstructor) { EXPECT_TRUE(mesh.texcoords.empty()); EXPECT_TRUE(mesh.texcoord_faces.empty()); EXPECT_TRUE(mesh.texcoord_lines.empty()); + EXPECT_TRUE(mesh.polyFaces.empty()); + EXPECT_TRUE(mesh.polyFaceSizes.empty()); + EXPECT_TRUE(mesh.polyTexcoordFaces.empty()); } // Test updateNormals with a tetrahedron @@ -166,6 +169,11 @@ TYPED_TEST(MeshTest, Cast) { mesh.confidence = {0.5, 0.6, 0.7}; + // Add polygon data: a quad (v0,v1,v2,v3) + a triangle (v0,v1,v2) + mesh.polyFaces = {0, 1, 2, 0, 0, 1, 2}; + mesh.polyFaceSizes = {4, 3}; + mesh.polyTexcoordFaces = {10, 11, 12, 13, 14, 15, 16}; + // Cast to the same type (should be a copy) auto sameMesh = mesh.template cast(); @@ -179,6 +187,9 @@ TYPED_TEST(MeshTest, Cast) { EXPECT_TRUE(sameMesh.normals[i].isApprox(mesh.normals[i])); EXPECT_NEAR(sameMesh.confidence[i], mesh.confidence[i], Eps(1e-5f, 1e-13)); } + EXPECT_EQ(sameMesh.polyFaces, mesh.polyFaces); + EXPECT_EQ(sameMesh.polyFaceSizes, mesh.polyFaceSizes); + EXPECT_EQ(sameMesh.polyTexcoordFaces, mesh.polyTexcoordFaces); // Cast to the other type using OtherT = typename std::conditional::value, double, float>::type; @@ -195,6 +206,9 @@ TYPED_TEST(MeshTest, Cast) { // Use a more relaxed tolerance for float-to-double or double-to-float conversions EXPECT_NEAR(static_cast(otherMesh.confidence[i]), mesh.confidence[i], Eps(1e-4f, 1e-6)); } + EXPECT_EQ(otherMesh.polyFaces, mesh.polyFaces); + EXPECT_EQ(otherMesh.polyFaceSizes, mesh.polyFaceSizes); + EXPECT_EQ(otherMesh.polyTexcoordFaces, mesh.polyTexcoordFaces); } // Test reset method @@ -225,6 +239,10 @@ TYPED_TEST(MeshTest, Reset) { mesh.texcoord_lines = {{0, 1}, {1, 2}, {2, 0}}; + mesh.polyFaces = {0, 1, 2, 0, 0, 1, 2}; + mesh.polyFaceSizes = {4, 3}; + mesh.polyTexcoordFaces = {10, 11, 12, 13, 14, 15, 16}; + // Reset the mesh mesh.reset(); @@ -238,6 +256,9 @@ TYPED_TEST(MeshTest, Reset) { EXPECT_TRUE(mesh.texcoords.empty()); EXPECT_TRUE(mesh.texcoord_faces.empty()); EXPECT_TRUE(mesh.texcoord_lines.empty()); + EXPECT_TRUE(mesh.polyFaces.empty()); + EXPECT_TRUE(mesh.polyFaceSizes.empty()); + EXPECT_TRUE(mesh.polyTexcoordFaces.empty()); } // Test with a complex mesh @@ -351,6 +372,9 @@ TYPED_TEST(MeshTest, EmptyMesh) { EXPECT_TRUE(castedMesh.texcoords.empty()); EXPECT_TRUE(castedMesh.texcoord_faces.empty()); EXPECT_TRUE(castedMesh.texcoord_lines.empty()); + EXPECT_TRUE(castedMesh.polyFaces.empty()); + EXPECT_TRUE(castedMesh.polyFaceSizes.empty()); + EXPECT_TRUE(castedMesh.polyTexcoordFaces.empty()); // Reset an empty mesh EXPECT_NO_THROW(mesh.reset()); @@ -462,3 +486,66 @@ TYPED_TEST(MeshTest, Lines) { mesh.reset(); EXPECT_TRUE(mesh.lines.empty()); } + +// Test polygon face representation +TYPED_TEST(MeshTest, PolygonFaces) { + using T = TypeParam; + using MeshType = typename TestFixture::MeshType; + using Vector3 = Eigen::Vector3; + + MeshType mesh; + + // Create a mesh with 5 vertices + mesh.vertices = { + Vector3(0, 0, 0), Vector3(1, 0, 0), Vector3(1, 1, 0), Vector3(0, 1, 0), Vector3(0.5, 0.5, 1)}; + + // Polygon representation: a quad (4 verts) + a triangle (3 verts) + mesh.polyFaces = {0, 1, 2, 3, 0, 1, 4}; + mesh.polyFaceSizes = {4, 3}; + mesh.polyTexcoordFaces = {0, 1, 2, 3, 4, 5, 6}; + + // Verify sizes sum matches polyFaces length + uint32_t totalSize = 0; + for (auto s : mesh.polyFaceSizes) { + totalSize += s; + } + EXPECT_EQ(totalSize, mesh.polyFaces.size()); + EXPECT_EQ(totalSize, mesh.polyTexcoordFaces.size()); + + // Cast and verify preservation + using OtherT = typename std::conditional::value, double, float>::type; + auto otherMesh = mesh.template cast(); + EXPECT_EQ(otherMesh.polyFaces, mesh.polyFaces); + EXPECT_EQ(otherMesh.polyFaceSizes, mesh.polyFaceSizes); + EXPECT_EQ(otherMesh.polyTexcoordFaces, mesh.polyTexcoordFaces); + + // Reset and verify cleared + mesh.reset(); + EXPECT_TRUE(mesh.polyFaces.empty()); + EXPECT_TRUE(mesh.polyFaceSizes.empty()); + EXPECT_TRUE(mesh.polyTexcoordFaces.empty()); +} + +// Test polygon faces with empty texcoord faces +TYPED_TEST(MeshTest, PolygonFacesNoTexcoords) { + using T = TypeParam; + using MeshType = typename TestFixture::MeshType; + using Vector3 = Eigen::Vector3; + + MeshType mesh; + + mesh.vertices = {Vector3(0, 0, 0), Vector3(1, 0, 0), Vector3(0, 1, 0)}; + mesh.polyFaces = {0, 1, 2}; + mesh.polyFaceSizes = {3}; + // polyTexcoordFaces intentionally left empty + + EXPECT_EQ(mesh.polyFaces.size(), 3u); + EXPECT_EQ(mesh.polyFaceSizes.size(), 1u); + EXPECT_TRUE(mesh.polyTexcoordFaces.empty()); + + // Cast preserves the empty texcoord faces + auto castedMesh = mesh.template cast(); + EXPECT_EQ(castedMesh.polyFaces, mesh.polyFaces); + EXPECT_EQ(castedMesh.polyFaceSizes, mesh.polyFaceSizes); + EXPECT_TRUE(castedMesh.polyTexcoordFaces.empty()); +} diff --git a/pymomentum/geometry/mesh_pybind.cpp b/pymomentum/geometry/mesh_pybind.cpp index 6e57114331..432803e258 100644 --- a/pymomentum/geometry/mesh_pybind.cpp +++ b/pymomentum/geometry/mesh_pybind.cpp @@ -29,6 +29,52 @@ namespace pymomentum { namespace { +void validateAndSetPolyData( + mm::Mesh& mesh, + std::vector poly_faces, + std::vector poly_face_sizes, + std::vector poly_texcoord_faces, + int nVerts, + int nTextureCoords) { + MT_THROW_IF( + poly_faces.empty() != poly_face_sizes.empty(), + "poly_faces and poly_face_sizes must both be empty or both be non-empty"); + if (!poly_face_sizes.empty()) { + uint32_t totalSize = 0; + for (const auto s : poly_face_sizes) { + MT_THROW_IF(s < 3, "poly_face_sizes entries must be >= 3, got {}", s); + totalSize += s; + } + MT_THROW_IF( + totalSize != poly_faces.size(), + "poly_face_sizes sum ({}) must equal poly_faces length ({})", + totalSize, + poly_faces.size()); + for (const auto idx : poly_faces) { + MT_THROW_IF( + idx >= static_cast(nVerts), + "poly_faces index ({}) exceeded vertex count ({})", + idx, + nVerts); + } + MT_THROW_IF( + !poly_texcoord_faces.empty() && poly_texcoord_faces.size() != poly_faces.size(), + "poly_texcoord_faces must be empty or same length as poly_faces ({} vs {})", + poly_texcoord_faces.size(), + poly_faces.size()); + for (const auto idx : poly_texcoord_faces) { + MT_THROW_IF( + idx >= static_cast(nTextureCoords), + "poly_texcoord_faces index ({}) exceeded texcoord count ({})", + idx, + nTextureCoords); + } + } + mesh.polyFaces = std::move(poly_faces); + mesh.polyFaceSizes = std::move(poly_face_sizes); + mesh.polyTexcoordFaces = std::move(poly_texcoord_faces); +} + mm::Mesh createMesh( const py::array_t& vertices, const py::array_t& faces, @@ -38,7 +84,10 @@ mm::Mesh createMesh( const std::vector& confidence, std::optional> texcoords, std::optional> texcoord_faces, - const std::vector>& texcoord_lines) { + const std::vector>& texcoord_lines, + std::vector poly_faces, + std::vector poly_face_sizes, + std::vector poly_texcoord_faces) { mm::Mesh mesh; MT_THROW_IF(vertices.ndim() != 2, "vertices must be a 2D array"); MT_THROW_IF(vertices.shape(1) != 3, "vertices must have size n x 3"); @@ -110,6 +159,14 @@ mm::Mesh createMesh( mesh.texcoord_lines = texcoord_lines; + validateAndSetPolyData( + mesh, + std::move(poly_faces), + std::move(poly_face_sizes), + std::move(poly_texcoord_faces), + static_cast(nVerts), + nTextureCoords); + return mesh; } @@ -136,6 +193,9 @@ void registerMeshBindings(py::class_& meshClass) { :param texcoords: Optional n x 2 array of texture coordinates. :param texcoord_faces: Optional n x 3 array of triangles in the texture map. Each triangle corresponds to a triangle on the mesh, but indices should refer to the texcoord array. :param texcoord_lines: Optional list of lines, where each line is a list of texture coordinate indices. +:param poly_faces: Optional list of packed polygon face vertex indices (all polygons concatenated). The triangulated ``faces`` parameter is always required; polygon data is optional and preserves original topology. +:param poly_face_sizes: Optional list of vertex counts per polygon face. +:param poly_texcoord_faces: Optional list of packed polygon face texture coordinate indices (same layout as poly_faces). )", py::arg("vertices"), py::arg("faces"), @@ -146,7 +206,10 @@ void registerMeshBindings(py::class_& meshClass) { py::arg("confidence") = std::vector{}, py::arg("texcoords") = std::optional>{}, py::arg("texcoord_faces") = std::optional>{}, - py::arg("texcoord_lines") = std::vector>{}) + py::arg("texcoord_lines") = std::vector>{}, + py::arg("poly_faces") = std::vector{}, + py::arg("poly_face_sizes") = std::vector{}, + py::arg("poly_texcoord_faces") = std::vector{}) .def_property_readonly( "n_vertices", [](const mm::Mesh& mesh) { return mesh.vertices.size(); }, @@ -188,6 +251,24 @@ void registerMeshBindings(py::class_& meshClass) { "texcoord_lines", &mm::Mesh::texcoord_lines, "Texture coordinate indices for each line. ") + .def_readonly( + "poly_faces", + &mm::Mesh::polyFaces, + "Packed polygon face vertex indices (all polygons concatenated back-to-back). " + "This is optional — the triangulated :attr:`faces` field is always required.") + .def_readonly( + "poly_face_sizes", + &mm::Mesh::polyFaceSizes, + "Number of vertices in each polygon face. May be empty if polygon data is not available.") + .def_readonly( + "poly_texcoord_faces", + &mm::Mesh::polyTexcoordFaces, + "Packed polygon face texture coordinate indices (same layout as :attr:`poly_faces`). " + "May be empty if texture coordinates are not available.") + .def_property_readonly( + "n_poly_faces", + [](const mm::Mesh& mesh) { return mesh.polyFaceSizes.size(); }, + ":return: The number of polygon faces in the mesh.") .def( "self_intersections", [](const mm::Mesh& mesh) { @@ -204,12 +285,13 @@ void registerMeshBindings(py::class_& meshClass) { }) .def("__repr__", [](const mm::Mesh& m) { return fmt::format( - "Mesh(vertices={}, faces={}, has_normals={}, has_colors={}, has_texcoords={})", + "Mesh(vertices={}, faces={}, has_normals={}, has_colors={}, has_texcoords={}, poly_faces={})", m.vertices.size(), m.faces.size(), !m.normals.empty() ? "True" : "False", !m.colors.empty() ? "True" : "False", - !m.texcoords.empty() ? "True" : "False"); + !m.texcoords.empty() ? "True" : "False", + m.polyFaceSizes.size()); }); } From 336f33b8537cd236aedab79f964f37c322865e51 Mon Sep 17 00:00:00 2001 From: Chris Twigg Date: Wed, 4 Mar 2026 14:15:16 -0800 Subject: [PATCH 3/3] Populate polygon face fields in FBX I/O (#1067) Summary: Wire up the polyFaces, polyFaceSizes, and polyTexcoordFaces fields added in the previous diff so they are populated during FBX load and written during FBX save. Load: copy raw polygon topology from PolygonData into the new mesh fields alongside the existing triangulation. Save: when polygon data is available, write actual polygons (quads, n-gons) instead of only triangles; fall back to triangulated faces otherwise. Reviewed By: nickyhe-gemini Differential Revision: D94577295 --- momentum/io/fbx/fbx_io.cpp | 69 +++++++++++++++++++++++------- momentum/io/fbx/openfbx_loader.cpp | 24 +++++++++++ 2 files changed, 77 insertions(+), 16 deletions(-) diff --git a/momentum/io/fbx/fbx_io.cpp b/momentum/io/fbx/fbx_io.cpp index 5d76690e9c..2516404ac0 100644 --- a/momentum/io/fbx/fbx_io.cpp +++ b/momentum/io/fbx/fbx_io.cpp @@ -561,6 +561,56 @@ void addMetaData(::fbxsdk::FbxNode* skeletonRootNode, const Character& character } } +void writePolygonsToFbxMesh(const Mesh& mesh, ::fbxsdk::FbxMesh* lMesh) { + if (!mesh.polyFaces.empty() && !mesh.polyFaceSizes.empty()) { + // Write original polygon topology + uint32_t offset = 0; + for (const auto polySize : mesh.polyFaceSizes) { + lMesh->BeginPolygon(); + for (uint32_t i = 0; i < polySize; ++i) { + lMesh->AddPolygon(mesh.polyFaces[offset + i]); + } + lMesh->EndPolygon(); + offset += polySize; + } + } else { + // Fall back to triangulated faces + for (const auto& face : mesh.faces) { + lMesh->BeginPolygon(); + for (int i = 0; i < 3; i++) { + lMesh->AddPolygon(face[i]); + } + lMesh->EndPolygon(); + } + } +} + +void writeTextureUVIndicesToFbxMesh( + const Mesh& mesh, + ::fbxsdk::FbxMesh* lMesh, + fbxsdk::FbxLayerElement::EType uvType) { + if (!mesh.polyFaces.empty() && !mesh.polyFaceSizes.empty()) { + uint32_t offset = 0; + int polyIdx = 0; + for (const auto polySize : mesh.polyFaceSizes) { + for (uint32_t i = 0; i < polySize; ++i) { + const uint32_t texIdx = mesh.polyTexcoordFaces.empty() ? mesh.polyFaces[offset + i] + : mesh.polyTexcoordFaces[offset + i]; + lMesh->SetTextureUVIndex(polyIdx, i, texIdx, uvType); + } + offset += polySize; + polyIdx++; + } + } else { + for (int faceIdx = 0; faceIdx < static_cast(mesh.texcoord_faces.size()); ++faceIdx) { + const auto& texcoords = mesh.texcoord_faces[faceIdx]; + lMesh->SetTextureUVIndex(faceIdx, 0, texcoords[0], uvType); + lMesh->SetTextureUVIndex(faceIdx, 1, texcoords[1], uvType); + lMesh->SetTextureUVIndex(faceIdx, 2, texcoords[2], uvType); + } + } +} + void saveFbxCommon( const filesystem::path& filename, const Character& character, @@ -687,7 +737,6 @@ void saveFbxCommon( if (saveMesh && character.mesh != nullptr) { // Add the mesh const int numVertices = character.mesh.get()->vertices.size(); - const int numFaces = character.mesh.get()->faces.size(); ::fbxsdk::FbxNode* meshNode = ::fbxsdk::FbxNode::Create(scene, "body_mesh"); ::fbxsdk::FbxMesh* lMesh = ::fbxsdk::FbxMesh::Create(scene, "mesh"); lMesh->SetControlPointCount(numVertices); @@ -704,14 +753,7 @@ void saveFbxCommon( lMesh->SetControlPointAt(point, normal, i); } // Add polygons to lMesh - for (int iFace = 0; iFace < numFaces; iFace++) { - lMesh->BeginPolygon(); - for (int i = 0; i < 3; i++) { // We have tris for models. This could be extended for - // supporting Quads or npoly if needed. - lMesh->AddPolygon(character.mesh.get()->faces[iFace][i]); - } - lMesh->EndPolygon(); - } + writePolygonsToFbxMesh(*character.mesh, lMesh); lMesh->BuildMeshEdgeArray(); meshNode->SetNodeAttribute(lMesh); @@ -733,13 +775,8 @@ void saveFbxCommon( lMesh->AddTextureUV(::fbxsdk::FbxVector2(texcoords[0], 1.0f - texcoords[1]), uvType); } - // Set UV indices for each face. We only have triangles. - int faceCount = 0; - for (const auto& texcoords : character.mesh->texcoord_faces) { - lMesh->SetTextureUVIndex(faceCount, 0, texcoords[0], uvType); - lMesh->SetTextureUVIndex(faceCount, 1, texcoords[1], uvType); - lMesh->SetTextureUVIndex(faceCount++, 2, texcoords[2], uvType); - } + // Set UV indices for each face. + writeTextureUVIndicesToFbxMesh(*character.mesh, lMesh, uvType); } // --------------------------------------------- diff --git a/momentum/io/fbx/openfbx_loader.cpp b/momentum/io/fbx/openfbx_loader.cpp index e34b55d66a..d5de4fedcd 100644 --- a/momentum/io/fbx/openfbx_loader.cpp +++ b/momentum/io/fbx/openfbx_loader.cpp @@ -501,6 +501,27 @@ parseSkeleton(const ofbx::Object* sceneRoot, const std::string& skelRoot, Permis return {skeleton, jointFbxNodes, locators, collision}; } +// Note: we intentionally omit reserve() here. This function is called once per sub-mesh in a loop, +// and reserve(size() + batchSize) defeats std::vector's geometric growth, causing O(K*N) copies +// instead of O(N) amortized. Letting the default 2x growth strategy handle it is more efficient. +void populatePolyFaceData( + Mesh& mesh, + const PolygonData& vertices, + size_t vertexOffset, + size_t textureCoordOffset) { + for (const auto& idx : vertices.indices) { + mesh.polyFaces.push_back(idx + static_cast(vertexOffset)); + } + for (size_t i = 0; i + 1 < vertices.offsets.size(); ++i) { + mesh.polyFaceSizes.push_back(vertices.offsets[i + 1] - vertices.offsets[i]); + } + if (!vertices.textureIndices.empty()) { + for (const auto& idx : vertices.textureIndices) { + mesh.polyTexcoordFaces.push_back(idx + static_cast(textureCoordOffset)); + } + } +} + void parseSkinnedModel( const ofbx::Mesh* meshRoot, const std::vector& boneFbxNodes, @@ -659,6 +680,9 @@ void parseSkinnedModel( mesh.texcoord_faces.emplace_back(t + Eigen::Vector3i::Constant(textureCoordOffset)); } + // Populate polygon face data (preserves original topology) + populatePolyFaceData(mesh, vertices, vertexOffset, textureCoordOffset); + const auto* blendshapes = geometry->getBlendShape(); if (loadBlendShapes == LoadBlendShapes::Yes && blendshapes) { if (!blendShape) {