Repository navigation
Add native skeletal animation authoring, curves, and 3D IK - #209
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1e5a7b2a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let rig, libraryIDs.insert(library.id).inserted else { | ||
| throw AssetError.message("Animation libraries need a rig and unique library IDs.") |
There was a problem hiding this comment.
For a glTF with neither skins nor imported animations, creating a rest-pose animation library succeeds because EditorAnimationAuthoringService.importLibrary builds a hierarchy rig unconditionally, and scene binding validates against that rig. However, ModelAsset3D sets animationRig to nil for precisely these models, so scene instantiation subsequently throws here instead of playing the authored node tracks. Preserve or reconstruct the compatible hierarchy rig when installing authored animations so a successfully created and bound library can actually run.
Useful? React with 👍 / 👎.
| textureSettingsModels.removeValue(forKey: documentID) | ||
| shaderGraphModels.removeValue(forKey: documentID) | ||
| animationWorkspaces.removeValue(forKey: documentID)?.cancel() | ||
| if case let .asset(asset) = closingDocument, asset.kind == .animationLibrary { try? animationService?.close(asset.relativePath) } |
There was a problem hiding this comment.
If a model workspace has an animation library attached, closing that library's saved resource tab calls service.close, which discards the shared document even though the model workspace still references it. EditorAnimationTimelineModel.isEditable then becomes false, and subsequent refreshes detach the library, interrupting editing in the still-open model tab. Check workspace ownership before closing the shared document, as releaseUnreferencedModifierDocuments already does for modifier resources.
Useful? React with 👍 / 👎.
| } else if workspace.sourceLibrary != nil || workspace.library != nil { | ||
| EditorAnimationTimeline(workspace: workspace) | ||
| } else { | ||
| Text(L10n.text("Choose an imported clip to copy, or create an animation from the rig's rest pose.")) |
There was a problem hiding this comment.
Opening an animated .vox now leaves its preview permanently paused: the workspace initializes preview.playsAnimation to false, while voxel assets have no animationRig, so neither sourceLibrary nor library is populated and this branch never displays the timeline's Play control. The previous model preview explicitly enabled playback for hasVoxelAnimation; repository-wide inspection of the replacement animation views finds no equivalent voxel transport. Retain a voxel playback control rather than routing these assets into the skeletal-library empty state.
Useful? React with 👍 / 👎.
| icon("⏮", title: "Stop", id: "Stop") { model.seek(0) } | ||
| icon("‹◆", title: "Previous Key", id: "PreviousKey") { model.step(-1, byKey: true) } | ||
| icon("‹", title: "Previous Frame", id: "PreviousFrame") { model.step(-1, byKey: false) } | ||
| icon(workspace.preview.playsAnimation ? "Ⅱ" : "▶", title: "Play", id: "Play") { togglePlayback() } |
There was a problem hiding this comment.
The new transport renders arbitrary Unicode strings in the system font, and its icon helper supplies only an accessibility identifier and hover text, not an accessible action name. Consequently, assistive technology receives glyph labels such as ‹◆ rather than “Previous Key,” and appearance depends on font fallback instead of the bundled icon system. AGENTS.md explicitly prohibits substituting arbitrary Unicode interface icons and requires localized tooltips and accessible names; use the existing Material Symbols helper and label each action.
AGENTS.md reference: AGENTS.md:L55-L59
Useful? React with 👍 / 👎.
Imported skeletal clips can now be copied into project-owned animation libraries, edited on the native timeline, and combined with an ordered 3D IK stack. Studio, the editor agent and developer Tools share revision-checked edits, explicit saving and Undo/Redo. Imported GLB/glTF files remain unchanged.
Implemented:
.animlibauthoring, bone/morph tracks, grouped key editing, scrubbing, clip/duration/loop controls and production curve previews..animmodresources, current scene-marker bindings, viewport gizmos and shared agent/Tool operations.Validation:
git diff --checkand the unmodified DocC temporal-curve example passed their focused checks.This is a draft delivery of the implemented stages, not completion of the full animation roadmap. Native 2D rigs/weighted deformation/IK, spatial splines, typed animation/Auto Key of IK parameters, additional tangent modes and canvas handle dragging remain outstanding. State-machine/blend-space and remaining property/playback stages are also not included.
Full local review validation has not passed: retained attempts hit a Windows SPIRV-Cross/MSVC link failure, missing SwiftLint/analyzer prerequisites, an Editor build timeout, Windows DocC conversion failure and unavailable platform/render checks. Independent review found no remaining concrete defect in the reviewed 3D IK and temporal-curve slices but withheld full-task approval for these validation/completion gaps. CI results must be assessed before merge.
Local Git hooks were bypassed once for this draft because SwiftLint is unavailable on this host and complete local validation has not passed. GitHub CI is retained; this draft is not ready to merge.
Initial CI result: SwiftPM Test on macOS stops in
Test Studio installationbefore the Swift tests. Five installer tests reportPermissionError: Operation not permittedwhile preparing a temporary.appfixture. The installer/test files are unchanged by this PR. Other workflows were still pending at publication; no overall CI pass is claimed.