Update convert_model to use new fbx features #755
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
meta-cla
bot
added
the
CLA Signed
Contributor
yutingye
added a commit
that referenced
this pull request
Nov 2, 2025
Summary: Pull Request resolved: #755 * Add a `--fbx-namespace` to specify namespace when saving an fbx file. * Support `--save-markers` for fbx output file. * For glb, default to save marker data as unit cubes. It's easier to know that they are there with visuals. It was too clustered before, but we have better visualization tool now. * Remove .mmo format support. Nobody should be using it. * Use the unified save function for both fbx and glb, yay! Differential Revision: D85744246
yutingye
force-pushed
the
export-D85744246
branch
from
f092adc to
616fad0
Compare
Summary: Pull Request resolved: #753 After D85530152 added marker sequence support to fbx, we have functionality parity between fbx and gltf. This diff created a unified `saveCharacter()` function in `character_io` that automatically selects between FBX and GLTF export based on file extension. Added .fbx support to `loadMarkers()` as the unified function for loading marker data from any supported format. Added an additional unified `saveCharacter()` function in `character_io` that takes skeleton states as input instead of model parameters. Rename `saveCharacter()` in `gltf_io` to `saveGltfCharacter()`, but keeping the existing ones for backwards compatibility for now. It will be removed in a later diff when downstreams are updated. Differential Revision: D85517572
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
Differential Revision: D86017756
Summary:
This change consolidates scattered FBX and GLTF save options into a unified `FileSaveOptions` struct, simplifying the API and making the codebase more maintainable.
## Key Changes
**1. Created Unified Options Struct (`file_save_options.h`)**
- Consolidated FBX coordinate system enums (FBXUpVector, FBXFrontVector, FBXCoordSystem, FBXCoordSystemInfo)
- Moved GltfFileFormat enum from gltf_file_format.h (which was deleted)
- Created FileSaveOptions struct with common and format-specific options:
- Common: mesh, locators, collisions, blendShapes, permissive
- FBX-specific: coordSystemInfo, fbxNamespace
- GLTF-specific: extensions, gltfFileFormat
**2. Updated Function Signatures**
- Refactored all save functions to use FileSaveOptions as the last parameter:
- saveFbx(), saveFbxWithJointParams(), saveFbxModel()
- saveCharacter() (GLTF versions)
- saveCharacterToBytes()
- saveCharacterToFile() (unified IO)
- Simplified saveFbxCommon() to take FileSaveOptions directly instead of individual parameters
**3. Dependency Management**
- Created new header-only BUCK target: io_file_save_options
- Updated io_fbx, io_gltf, and io targets to depend on io_file_save_options
- Removed gltf_file_format.h (consolidated into file_save_options.h)
- Clean dependency graph with no circular dependencies
**4. Updated All Callsites**
- Updated test files:
- test/io/io_fbx_test.cpp
- test/io/io_fbx_gltf_roundtrip_test.cpp
- test/io/io_gltf_test.cpp
- test/io/character_io.cpp
- All tests pass successfully
## Benefits
- **Single source of truth**: All save options in one struct
- **Consistent API**: Same options pattern for FBX, GLTF, and unified save
- **Extensible**: Easy to add new options without changing function signatures
- **Type safety**: Compile-time checking of option values
- **Better documentation**: All options documented in one place
- **Cleaner code**: Reduced parameter count in function signatures
## Migration
Code changes are mostly straightforward. Example:
**Before:**
```cpp
saveFbx(path, character, motion, offsets, fps, true, FBXCoordSystemInfo(), false);
```
**After:**
```cpp
FileSaveOptions opts;
opts.mesh = true;
saveFbx(path, character, motion, offsets, fps, {}, opts);
```
Updated momentum internal files to use the new unified `FileSaveOptions` API after the core refactoring in the previous commit.
## Changes
**1. Updated convert_model example (`examples/convert_model/convert_model.cpp`)**
- Migrated from old parameter-based API to `FileSaveOptions`
- Now uses `FileSaveOptions` struct with `mesh` option set based on command-line flag
- Maintains backward compatibility with existing CLI interface
**2. Updated marker tracking utilities (`marker_tracking/app_utils.cpp`)**
- Migrated `saveMotion()` function to use `FileSaveOptions`
- Now uses `FileSaveOptions` struct with `mesh` option for marker mesh saving
- Maintains existing behavior for both FBX and GLTF output formats
## Migration Pattern
Both files follow the same migration pattern:
**Before:**
```cpp
saveFbx(output, character, motion, offsets, fps, saveMeshFlag);
```
**After:**
```cpp
FileSaveOptions opts;
opts.mesh = saveMeshFlag;
saveFbx(output, character, motion, offsets, fps, {}, opts);
```
This completes the migration of all momentum internal files to the unified `FileSaveOptions` API introduced in the previous commit.
Completed the Python binding migration to use `FileSaveOptions` instead of individual parameters for FBX and GLTF save functions. This brings the Python API in line with the unified C++ FileSaveOptions introduced in the previous commits.
## Changes
**1. Updated Python C++ binding layer (`momentum_io.h/cpp`)**
- Migrated `saveFBXCharacterToFile()` to use `FileSaveOptions` instead of `FBXCoordSystemInfo` and `fbxNamespace` parameters
- Migrated `saveFBXCharacterToFileWithJointParams()` to use `FileSaveOptions`
- Migrated `saveGLTFCharacterToFile()` to use `FileSaveOptions` instead of `GltfOptions`
- Migrated `saveGLTFCharacterToFileFromSkelStates()` to use `FileSaveOptions`
- Removed redundant `GltfFileFormat` parameter (now part of `FileSaveOptions`)
**2. Updated Python bindings (`character_pybind.cpp`)**
- Updated `save_fbx()` to accept `options` parameter of type `FileSaveOptions` (replaces `coord_system_info` and `fbx_namespace`)
- Updated `save_fbx_with_joint_params()` to accept `options` parameter of type `FileSaveOptions`
- Updated `save_gltf()` to accept `options` parameter of type `FileSaveOptions` (replaces `GltfOptions`)
- Updated `save_gltf_from_skel_states()` to accept `options` parameter of type `FileSaveOptions`
**3. Exposed FileSaveOptions class to Python (`geometry_pybind.cpp`)**
- Added `FileSaveOptions` class binding with all properties:
- Common options: `mesh`, `locators`, `collisions`, `blend_shapes`, `permissive`
- FBX-specific: `coord_system_info`, `fbx_namespace`
- GLTF-specific: `extensions`, `gltf_file_format`
- Added `GltfFileFormat` enum binding (Extension, Embedded)
- Added `__repr__` for better debugging
**4. Updated Python tests (`test/test_fbx_io.py`)**
- Migrated all test cases to use `FileSaveOptions` API
- Tests now create options objects and set properties instead of passing individual parameters
- Maintains test coverage for namespace, coord system, and marker sequence features
## Migration Pattern for Python
**Before:**
```python
Character.save_fbx(
path="output.fbx",
character=character,
fps=120,
motion=motion,
offsets=offsets,
coord_system_info=FBXCoordSystemInfo(...),
fbx_namespace="ns"
)
```
**After:**
```python
options = FileSaveOptions()
options.coord_system_info = FBXCoordSystemInfo(...)
options.fbx_namespace = "ns"
Character.save_fbx(
path="output.fbx",
character=character,
fps=120,
motion=motion,
offsets=offsets,
options=options
)
```
## Benefits
- **API Consistency**: Python API now matches C++ FileSaveOptions pattern
- **Type Safety**: All options properties are strongly typed
- **Extensibility**: New options can be added without changing function signatures
- **Backward Compatible**: Default `FileSaveOptions()` maintains existing behavior
- **Better Documentation**: All options documented in one place
Completed the deprecation of the legacy `GltfOptions` struct by migrating all remaining uses to `FileSaveOptions` and removing the deprecated class entirely.
## Changes
**1. Removed GltfOptions from C++ code (`momentum/io/gltf/`)**
- Updated `gltf_builder.h/cpp`: Changed `addCharacter()` to accept `FileSaveOptions` instead of `GltfOptions`
- Updated `gltf_io.cpp`:
- Removed internal conversions from `FileSaveOptions` to `GltfOptions`
- Removed unused `makeCharacterDocument()` helper function that took `GltfOptions`
- Updated `makeCharacterDocument()` and `saveCharacter()` functions to use `FileSaveOptions` directly
**2. Removed GltfOptions from Python bindings (`pymomentum/geometry/`)**
- Updated `gltf_builder_pybind.cpp`: Changed `add_character()` binding to accept `FileSaveOptions` parameter
- Updated `geometry_pybind.cpp`:
- **Removed entire `GltfOptions` class binding** - no longer exposed to Python
- Only `FileSaveOptions` class is now available in Python
- Updated `geometry.pyi`:
- Removed `GltfOptions` class definition from type stubs
- Updated all function signatures to use `FileSaveOptions` instead of `GltfOptions`
- Removed `GltfOptions` from `__all__` export list
**3. Updated test files**
- Updated `test_gltf.py`: Changed test to use `FileSaveOptions(mesh=False)` instead of `GltfOptions(mesh=False)`
## Migration Pattern
**Before (Python):**
```python
builder.add_character(
character,
options=pym_geometry.GltfOptions(mesh=False)
)
```
**After (Python):**
```python
builder.add_character(
character,
options=pym_geometry.FileSaveOptions(mesh=False)
)
```
## Benefits
- **API Consistency**: Both FBX and GLTF now use the same `FileSaveOptions` struct
- **Single Source of Truth**: All save options are unified in one place
- **Cleaner Codebase**: Removed deprecated class and all conversion code
- **Better Documentation**: All options documented together in `FileSaveOptions`
- **Type Safety**: Compile-time checking with single options type
## Deprecation Complete
This completes the deprecation roadmap started in earlier commits:
1. ✅ Created unified `FileSaveOptions` struct
2. ✅ Migrated all C++ callsites to use `FileSaveOptions`
3. ✅ Migrated Python bindings to use `FileSaveOptions`
4. ✅ **Removed deprecated `GltfOptions` class entirely**
Differential Revision: D86034154
Summary:
Added comprehensive Python bindings for the `FileSaveOptions` struct in pymomentum, making the unified file save options API accessible from Python. This complements the C++ FileSaveOptions API introduced in D86034154.
## Changes
**1. Added FileSaveOptions Python binding (`geometry_pybind.cpp`)**
- Created Python class binding for `FileSaveOptions` with all fields exposed as read/write properties
- Fields exposed include:
- **Common options**: `mesh`, `locators`, `collisions`, `blend_shapes`, `permissive`
- **FBX-specific**: `coord_system_info`, `fbx_namespace`
- **GLTF-specific**: `extensions`, `gltf_file_format`
- Added `__repr__` method for debugging and inspection
- Added comprehensive docstrings for all properties
- Used snake_case naming convention for Python (e.g., `blend_shapes` maps to C++ `blendShapes`)
**2. Added comprehensive test coverage (`test_geometry.py`)**
- Created `test_file_save_options()` test method with full coverage:
- Default value validation for all fields
- Field modification tests for all field types
- Integration tests with `FBXCoordSystemInfo` and `GltfFileFormat` enums
- `__repr__` functionality validation
- Instance independence verification
## Benefits
- **API Parity**: Python API now has access to the same unified options struct as C++
- **Type Safety**: All options are strongly typed Python properties
- **Documentation**: Each field has clear docstrings explaining purpose and defaults
- **Extensibility**: New options can be added without breaking existing code
- **Consistency**: Follows same patterns as existing bindings (`GltfOptions`, `FBXCoordSystemInfo`)
## Example Usage
```python
import pymomentum.geometry as pym_geometry
# Create options with defaults
options = pym_geometry.FileSaveOptions()
# Customize common options
options.mesh = True
options.blend_shapes = True
options.permissive = False
# Set FBX-specific options
options.coord_system_info = pym_geometry.FBXCoordSystemInfo(
pym_geometry.FBXUpVector.YAxis,
pym_geometry.FBXFrontVector.ParityOdd,
pym_geometry.FBXCoordSystem.RightHanded
)
options.fbx_namespace = "my_namespace"
# Set GLTF-specific options
options.extensions = True
options.gltf_file_format = pym_geometry.GltfFileFormat.GltfBinary
# Use with save functions (future work - will replace GltfOptions parameter)
# character.save_gltf(path, options=options)
```
Differential Revision: D86034396
Differential Revision: D86039901
Summary: Pull Request resolved: #755 * Add a `--fbx-namespace` to specify namespace when saving an fbx file. * Support `--save-markers` for fbx output file. * For glb, default to save marker data as unit cubes. It's easier to know that they are there with visuals. It was too clustered before, but we have better visualization tool now. * Remove .mmo format support. Nobody should be using it. * Use the unified save function for both fbx and glb, yay! Reviewed By: jeongseok-meta Differential Revision: D85744246
yutingye
force-pushed
the
export-D85744246
branch
from
616fad0 to
8e544eb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Differential Revision: D85744246