Merge fbx_io.cpp and fbx_io_openfbx_only.cpp using preprocessor directives #781
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
Summary: Created a high-level saveCharacter() function in character_io that automatically selects between FBX and GLTF export based on file extension. This achieves feature parity between the two formats for saving characters with motion and marker sequences. Key changes: - Added saveCharacter() overload that takes a filesystem path - Function automatically detects format from extension (.fbx, .glb, .gltf) - Routes to appropriate format-specific save function (saveFbx or GLTF saveCharacter) - Supports motion, offsets, and marker sequences for both formats - Provides clear error messages for unknown/unsupported formats Differential Revision: D85517572
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
Differential Revision: D85744246
Differential Revision: D86040408
meta-cla
bot
added
the
CLA Signed
Contributor
|
@jeongseok-meta has exported this pull request. If you are a Meta employee, you can view the originating Diff in D86063345. |
jeongseok-meta
force-pushed
the
export-D86063345
branch
from
938873c to
a9fc4d8
Compare
jeongseok-meta
temporarily deployed
to
pypi-cpu
GitHub Actions
Inactive
jeongseok-meta
temporarily deployed
to
pypi-cpu
GitHub Actions
Inactive
jeongseok-meta
temporarily deployed
to
pypi-gpu
GitHub Actions
Inactive
jeongseok-meta
temporarily deployed
to
pypi-gpu
GitHub Actions
Inactive
jeongseok-meta
force-pushed
the
export-D86063345
branch
from
a9fc4d8 to
39d2145
Compare
jeongseok-meta
temporarily deployed
to
pypi-cpu
GitHub Actions
Inactive
jeongseok-meta
temporarily deployed
to
pypi-cpu
GitHub Actions
Inactive
jeongseok-meta
temporarily deployed
to
pypi-gpu
GitHub Actions
Inactive
jeongseok-meta
temporarily deployed
to
pypi-gpu
GitHub Actions
Inactive
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: D86063345