Test - #236
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances the SMPL-X demo script with new functionality for loading, modifying, and exporting 3D body models. The changes transform the demo from a simple visualization tool into a more comprehensive model manipulation utility.
Key changes:
- Adds
.smplfile loading capability with parameter extraction and conversion - Implements experimental body attribute modification (body fat, lean mass for different body parts)
- Adds model export functionality to
.glbformat with organized output directory structure
Reviewed changes
Copilot reviewed 1 out of 10 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
.DS_Store |
macOS system file that should not be in version control |
examples/demo.py |
Major enhancements including new functions for loading SMPL files, modifying body attributes, extended command-line arguments, export functionality, and updated default parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # paste this to run the script | ||
| # python examples/demo.py --model-folder "../../smpl_edit_experiment_v1/models" --gender="neutral" --output model6.glb --plotting-module none |
There was a problem hiding this comment.
[nitpick] This comment contains a hardcoded command with specific paths that appear to be from the developer's local machine. Comments like this should either be removed or generalized with placeholder values for documentation purposes. Consider removing this line or moving it to a README with appropriate placeholders.
| # paste this to run the script | |
| # python examples/demo.py --model-folder "../../smpl_edit_experiment_v1/models" --gender="neutral" --output model6.glb --plotting-module none | |
| # Example usage: | |
| # python examples/demo.py --model-folder "<MODEL_FOLDER_PATH>" --gender="neutral" --output <OUTPUT_FILE> --plotting-module <PLOTTING_MODULE> |
| - transl: torch.Tensor, shape [1, 3] or None | ||
| - gender: str | ||
| """ | ||
| data = np.load(file_path, allow_pickle=True) |
There was a problem hiding this comment.
Using allow_pickle=True with np.load() is a security risk as it can execute arbitrary code if the file contains malicious pickled objects. Since the expected format is .npz with numeric arrays, consider using allow_pickle=False (the default in newer NumPy versions) or add explicit validation of the file source. If pickled objects are truly necessary, document the security implications and recommend loading only from trusted sources.
| data = np.load(file_path, allow_pickle=True) | |
| data = np.load(file_path) |
| # Body pose (22 joints - need to convert to 21 for SMPL-X) | ||
| if 'bodyPose' in data: | ||
| body_pose = torch.from_numpy(data['bodyPose']).float() # (1, 22, 3) | ||
| # SMPL-X expects 21 body joints (excluding root) | ||
| # The first joint in bodyPose might be root, so we take joints 1-21 | ||
| if body_pose.shape[1] == 22: | ||
| # Skip first, take next 21 | ||
| params['body_pose'] = body_pose[:, 1:22, :] | ||
| elif body_pose.shape[1] == 21: | ||
| params['body_pose'] = body_pose | ||
| else: | ||
| # Reshape if needed: (1, 63) -> (1, 21, 3) | ||
| if len(body_pose.shape) == 2 and body_pose.shape[1] == 63: | ||
| params['body_pose'] = body_pose.reshape(1, 21, 3) | ||
| else: | ||
| params['body_pose'] = body_pose |
There was a problem hiding this comment.
The comment "Body pose (22 joints - need to convert to 21 for SMPL-X)" and the subsequent logic are somewhat confusing. The docstring doesn't explain why the conversion is necessary or what happens when the input has different dimensions. Consider adding a brief explanation in the docstring about SMPL vs SMPL-X joint count differences.
| # If no file loaded, use defaults | ||
| if betas is None: | ||
| if sample_shape: | ||
| betas = torch.zeros([1, model.num_betas], dtype=torch.float32) |
There was a problem hiding this comment.
Both branches of this conditional create the same tensor with torch.zeros(). The if sample_shape: condition is redundant since both branches have identical behavior. Either simplify to a single line or implement different behavior for when sample_shape=True (e.g., use torch.randn() as in the original code).
| betas = torch.zeros([1, model.num_betas], dtype=torch.float32) | |
| betas = torch.randn([1, model.num_betas], dtype=torch.float32) |
| if not filename.endswith('.glb'): | ||
| filename = filename + '.glb' if '.' not in filename else filename |
There was a problem hiding this comment.
This logic for handling file extensions is unclear and potentially buggy. The condition filename + '.glb' if '.' not in filename else filename will not add .glb if the filename contains any dot (e.g., file.txt would remain file.txt instead of becoming file.txt.glb). Consider using osp.splitext() to properly check for and handle file extensions.
| if not filename.endswith('.glb'): | |
| filename = filename + '.glb' if '.' not in filename else filename | |
| root, ext = osp.splitext(filename) | |
| if ext == '': | |
| filename = filename + '.glb' |
| trace = R[0, 0] + R[1, 1] + R[2, 2] | ||
| angle = torch.acos(torch.clamp((trace - 1) / 2, -1, 1)) | ||
| if angle < 1e-6: | ||
| # No rotation | ||
| params['global_orient'] = torch.zeros( | ||
| 1, 3, dtype=torch.float32) | ||
| else: | ||
| # Extract axis | ||
| axis = torch.stack([ | ||
| R[2, 1] - R[1, 2], | ||
| R[0, 2] - R[2, 0], | ||
| R[1, 0] - R[0, 1] | ||
| ]) / (2 * torch.sin(angle)) | ||
| params['global_orient'] = (angle * axis).unsqueeze(0) |
There was a problem hiding this comment.
The Rodrigues formula implementation has potential numerical instability issues. When angle is very close to 0, division by 2 * torch.sin(angle) at line 100 will cause issues even though there's a check at line 90. Additionally, when angle is close to π, the trace-based calculation becomes unstable. Consider using a more robust rotation matrix to axis-angle conversion function like cv2.Rodrigues() or PyTorch3D's implementation.
| # Experimental: betas[0] and betas[1] often control weight/thickness | ||
| if body_fat is not None: | ||
| # Positive values = more body fat, negative = less body fat | ||
| modified_betas[0, 0] += body_fat * 0.5 # Primary weight parameter | ||
| if modified_betas.shape[1] > 1: | ||
| modified_betas[0, 1] += body_fat * \ | ||
| 0.3 # Secondary weight parameter | ||
|
|
||
| # Lean Mass Arms: Experimental - may affect arm muscle mass | ||
| # Try betas[2] or betas[3] for arm-related shape | ||
| if lean_mass_arms is not None: | ||
| if modified_betas.shape[1] > 2: | ||
| modified_betas[0, 2] += lean_mass_arms * 0.4 | ||
| if modified_betas.shape[1] > 3: | ||
| modified_betas[0, 3] += lean_mass_arms * 0.2 | ||
|
|
||
| # Lean Mass Torso: Experimental - may affect torso muscle mass | ||
| # Try betas[4] or betas[5] for torso-related shape | ||
| if lean_mass_torso is not None: | ||
| if modified_betas.shape[1] > 4: | ||
| modified_betas[0, 4] += lean_mass_torso * 0.4 | ||
| if modified_betas.shape[1] > 5: | ||
| modified_betas[0, 5] += lean_mass_torso * 0.2 | ||
|
|
||
| # Lean Mass Legs: Experimental - may affect leg muscle mass | ||
| # Try betas[6] or betas[7] for leg-related shape | ||
| if lean_mass_legs is not None: | ||
| if modified_betas.shape[1] > 6: | ||
| modified_betas[0, 6] += lean_mass_legs * 0.4 | ||
| if modified_betas.shape[1] > 7: | ||
| modified_betas[0, 7] += lean_mass_legs * 0.2 |
There was a problem hiding this comment.
The comment states these are experimental mappings, but the implementation uses hardcoded magic numbers (0.5, 0.3, 0.4, 0.2) and beta indices (0-7) without justification. Consider extracting these as named constants with documentation explaining their experimental nature, or providing a configuration mechanism. This would make it easier to tune these values without modifying the code directly.
| if loaded_left_hand is not None and loaded_left_hand.shape[1] != expected_pca_size: | ||
| print(f'Warning: Model uses PCA for hands ({expected_pca_size} components), ' | ||
| f'but loaded hand poses are axis-angle format ({loaded_left_hand.shape[1]} values). ' | ||
| f'Skipping hand poses.') |
There was a problem hiding this comment.
This warning message could be more actionable. Consider suggesting what the user should do (e.g., "Consider exporting the hand poses in PCA format or using a model without PCA") or explaining the implications of skipping hand poses on the output model.
| f'Skipping hand poses.') | |
| f'Skipping hand poses. Consider exporting the hand poses in PCA format or using a model without PCA for hands. As a result, the hand pose will be set to the model\'s default (e.g., open hand) in the output.') |
No description provided.