feat(asset-gen): add animation_type rig mode to import_model_file#1262
Merged
Scriptwonder merged 3 commits intoJul 12, 2026
Merged
Conversation
A rigged/animated FBX imported through import_model_file always arrived with zero AnimationClips because the import pipeline hard-coded ModelImporterAnimationType.None. Thread an optional animation_type param (none|generic|humanoid|legacy) through the MCP tool, the CLI command, and the C# pipeline so callers can surface a model's clips; the default stays None, so existing imports are unchanged. glTF/GLB ignores the knob (glTFast imports animation itself). Also fold the tested Blender-handoff findings into the blender-to-unity skill: GLB-vs-FBX fidelity guidance (with the new references/bridge-fidelity.md matrix, force-added since .claude is gitignored), FBX emission restoration steps, and the animation_type knob. Sync uv.lock with the pyproject 10.0.0 version already on beta. Claude-Session: https://claude.ai/code/session_01Cya1SZmg7CJgJS61nhjLH4
Scriptwonder
added
safe-to-test
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds FBX/OBJ animation-type selection to the model import CLI and tool, propagates it through Unity job state and importer settings, adds routing and parsing tests, and expands Blender–Unity fidelity documentation with GLB/FBX guidance and FBX emission recovery steps. ChangesModel import fidelity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant import_model_file
participant ImportModelFile
participant ModelImportPipeline
CLI->>import_model_file: provide animation_type
import_model_file->>ImportModelFile: send animationType
ImportModelFile->>ModelImportPipeline: pass AssetGenJob.AnimationType
ModelImportPipeline->>ModelImportPipeline: apply parsed importer animation type
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an optional animation_type parameter to the import_model_file pipeline so rigged/animated FBX imports can surface AnimationClips by configuring Unity’s ModelImporter.animationType, while keeping the default behavior unchanged (None).
Changes:
- Thread
animation_typefrom Python tool + CLI down into the Unity C# import pipeline (AssetGenJob.AnimationType→ModelImportPipeline.ParseAnimationType()→ModelImporter.animationType). - Add unit tests in both Unity (rig mode parsing) and Python (wire payload/CLI flag routing).
- Expand Blender→Unity handoff skill docs with format-fidelity guidance and a new reference matrix.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ModelImportPipelineTests.cs | Adds EditMode coverage for ParseAnimationType mappings. |
| MCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.cs | Applies parsed animation type to ModelImporter and introduces ParseAnimationType. |
| MCPForUnity/Editor/Tools/AssetGen/ImportModelFile.cs | Reads animationType param into the AssetGenJob used for import. |
| MCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.cs | Extends AssetGenJob with AnimationType field. |
| Server/src/services/tools/import_model_file.py | Adds animation_type tool param and forwards animationType camelCase to Unity. |
| Server/src/cli/commands/asset_gen.py | Adds --animation-type flag and forwards animationType in the CLI payload. |
| Server/tests/test_asset_gen_import_file.py | Updates allowed payload keys and adds routing/CLI tests for animationType. |
| Server/uv.lock | Syncs locked package version to 10.0.0. |
| .claude/skills/blender-to-unity/SKILL.md | Updates skill guidance for GLB-vs-FBX fidelity and documents animation_type usage. |
| .claude/skills/blender-to-unity/references/bridge-fidelity.md | Adds a tested format-fidelity matrix for the Blender→Unity bridge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// <summary> | ||
| /// Map the caller's rig mode to a <see cref="ModelImporterAnimationType"/>. Defaults to | ||
| /// <c>None</c> (no rig) when unset — pass "generic"/"humanoid" to surface a rigged FBX/OBJ's |
| "name, output_folder (under Assets/), target_size. Returns { asset_path, asset_guid }.\n\n" | ||
| "name, output_folder (under Assets/), target_size, animation_type. " | ||
| "Returns { asset_path, asset_guid }.\n\n" | ||
| "animation_type (FBX/OBJ only): pass 'generic' or 'humanoid' for a rigged/animated mesh so " |
Comment on lines
+49
to
+50
| "FBX/OBJ only: rig/animation import mode. 'generic' or 'humanoid' surface the model's " | ||
| "AnimationClips; 'none' (default) imports no rig. Ignored for glTF/GLB.", |
| @click.option("--target-size", default=None, type=float, help="Normalize largest dimension (meters).") | ||
| @click.option("--animation-type", "animation_type", default=None, | ||
| type=click.Choice(["none", "generic", "humanoid", "legacy"]), | ||
| help="FBX/OBJ rig mode: generic/humanoid surface animation clips (glTF ignores this).") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/blender-to-unity/references/bridge-fidelity.md:
- Around line 62-65: Update the GLB export recipes around the rig/morph guidance
and the generic command to separate static geometry from rigged or shape-key
assets: use export_apply=True only for static geometry, and provide a distinct
rig/morph command using export_apply=False while preserving skin and morph
export settings.
- Around line 90-92: Update the export_animations=True guidance in the GLB
export options to state that only eligible active or stashed actions are
exported through the object’s active action or NLA tracks, rather than all
actions; mention the required Action/NLA setup while leaving the export_morph
guidance unchanged.
In @.claude/skills/blender-to-unity/SKILL.md:
- Around line 106-109: Update the GLB export example in the glTF/glTFast
guidance to include use_active_scene=True, preserving the existing export
behavior while ensuring the currently active Blender scene is exported.
- Around line 44-46: Update the rigged/animated FBX guidance to document all
supported animation_type values: none, generic, humanoid, and legacy. Clarify
that none intentionally imports the mesh without animation clips, while generic,
humanoid, and legacy select the corresponding animation import modes; keep the
GLB behavior unchanged.
- Around line 104-110: Update the “Materials & emission” guidance to state that
FBX preserves vertex-color data, while default URP Lit does not display it
without a vertex-color-reading shader. Keep the separate limitation that FBX
drops emission and node-based color, and retain the glTF/GLB and Step 5
recommendations.
- Around line 25-30: Update the format-selection guidance near the GLB/FBX
bullets to remove multi-material zones as an independent GLB trigger. Keep GLB
preferred for rigging, animation, PBR, emission, and transparency, while
directing multi-material-only models through the existing FBX fallback because
both formats preserve material slots.
- Around line 40-41: Define a distinct temporary GLB output path variable before
the glTF branch invokes bpy.ops.export_scene.gltf, then pass that variable as
filepath. Keep the existing out path for the other export branch and ensure
out_glb is initialized before use.
- Around line 76-79: Update the Principled BSDF emission handling around p to
support both Blender socket names: prefer “Emission Color” for Blender 4.0+ and
fall back to “Emission” for Blender 3.x, retrieving color and strength from the
selected sockets. Preserve the existing emissive material conversion behavior
across both versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da38f393-7bbb-44ef-bd2e-045fc495fa78
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.claude/skills/blender-to-unity/SKILL.md.claude/skills/blender-to-unity/references/bridge-fidelity.mdMCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.csMCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.csMCPForUnity/Editor/Tools/AssetGen/ImportModelFile.csServer/src/cli/commands/asset_gen.pyServer/src/services/tools/import_model_file.pyServer/tests/test_asset_gen_import_file.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ModelImportPipelineTests.cs
…eference
Review responses (CodeRabbit + Copilot):
- Document the 'legacy' animation_type value in the C# XML summary, MCP tool
description, param help, and CLI help (Copilot x4).
- SKILL.md: drop multi-material zones as a GLB-only trigger (both formats keep
slots), define out_glb in the glTF branch example, document the full
animation_type contract, probe both Principled emission socket names
('Emission Color' 4.x / 'Emission' 3.x), keep use_active_scene=True in the
Notes GLB example, and distinguish FBX vertex-color data transfer from URP
Lit shader display.
- bridge-fidelity.md: make export_apply conditional (static-only; skinned/
shape-key meshes need False) and document export_animations' active/NLA-
stashed semantics.
Regenerate website/docs/reference for the updated tool description — fixes the
"Check docs reference is fresh" CI failure ("differs: asset_gen/import_model_file.md").
Claude-Session: https://claude.ai/code/session_01Cya1SZmg7CJgJS61nhjLH4
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/docs/reference/tools/asset_gen/import_model_file.md`:
- Around line 17-19: Clarify the source tool documentation to distinguish an
omitted animation_type/animationType, which preserves existing behavior, from
explicitly passing "none", which disables rig import; remove wording that
presents "none" as the API default. Update the source description and regenerate
the auto-generated import_model_file page rather than editing the generated page
directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ea4545e-e51b-4bd4-b89b-3a616969d27f
📒 Files selected for processing (6)
.claude/skills/blender-to-unity/SKILL.md.claude/skills/blender-to-unity/references/bridge-fidelity.mdMCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.csServer/src/cli/commands/asset_gen.pyServer/src/services/tools/import_model_file.pywebsite/docs/reference/tools/asset_gen/import_model_file.md
✅ Files skipped from review due to trivial changes (1)
- .claude/skills/blender-to-unity/references/bridge-fidelity.md
🚧 Files skipped from review as they are similar to previous changes (4)
- Server/src/cli/commands/asset_gen.py
- Server/src/services/tools/import_model_file.py
- MCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.cs
- .claude/skills/blender-to-unity/SKILL.md
CodeRabbit follow-up on CoplayDev#1262: the API-level default is omitting the parameter (nothing crosses the wire); passing 'none' explicitly maps to the same no-rig import. Say "omitted or 'none'" instead of calling 'none' the default, and regenerate the reference page. Claude-Session: https://claude.ai/code/session_01Cya1SZmg7CJgJS61nhjLH4
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
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.
Problem
A rigged/animated FBX imported through
import_model_filealways arrives with zero AnimationClips: the import pipeline hard-codesModelImporterAnimationType.None, so Unity strips the rig and clips silently. Agents (and humans) conclude the export was broken when it wasn't.Change
Thread an optional
animation_typeparam —none | generic | humanoid | legacy— through all three layers:AssetGenJob.AnimationType+ModelImportPipeline.ParseAnimationType()(internal, unit-tested);ImportModelFilereadsanimationType.animation_typeas aLiteralparam onimport_model_file, forwarded camelCase; tool description documents the FBX/OBJ-only semantics.--animation-typewith a matchingclick.Choice.The default stays
None, so existing imports are byte-for-byte unchanged. glTF/GLB ignores the knob — glTFast imports animation itself.Also folds tested Blender-handoff findings into the
blender-to-unityskill: GLB-vs-FBX format-fidelity guidance with a newreferences/bridge-fidelity.mdmatrix (force-added;.claudeis gitignored), FBX emission-restoration steps, and theanimation_typeusage. SyncsServer/uv.lockwith the10.0.0version already inpyproject.tomlon beta.Testing
animationTypereaches the wire camelCase, absent when unset) and CLI tests.ParseAnimationTypeTestCases (case/whitespace-insensitive mapping,humanalias, safeNonefallback for null/empty/nonsense) all pass.animation_type="generic"surfaces its clips; without it, mesh-only import as before.https://claude.ai/code/session_01Cya1SZmg7CJgJS61nhjLH4
Summary by CodeRabbit
--animation-typeoption for model imports (none,generic,humanoid,legacy) and wired it through to the Unity importer for FBX/OBJ to select rig/animation import behavior.animationTypepayload mapping and robust case/whitespace handling.