feat: support additive transform clip mixing#1497
Open
ChengYi996 wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces contribution-based transform mixing for timeline clips. ChangesAdditive Transform Clip Mixing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![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: 3
🧹 Nitpick comments (2)
packages/effects-core/src/plugins/timeline/playables/transform-playable.ts (1)
153-174: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInitialize before sampling contributions.
getContribution()can currently return empty/stale contributions if it is called beforeprocessFrame()has runstart(). Since this is now the public sampling API used by the mixer, share the lazy-start guard between both paths.Suggested refactor
+ private ensureStarted (): void { + if (!this.started) { + this.start(); + this.started = true; + } + } + override processFrame (context: FrameContext): void { - if (!this.started) { - this.start(); - this.started = true; - } + this.ensureStarted(); const boundObject = context.output.getUserData(); if (!this.originalTransform && boundObject instanceof VFXItem) { this.captureOriginalTransform(boundObject); @@ getContribution (basePosition?: Vector3): TransformContribution { + this.ensureStarted(); this.sampleAnimation(basePosition); return this.contribution; }🤖 Prompt for 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. In `@packages/effects-core/src/plugins/timeline/playables/transform-playable.ts` around lines 153 - 174, getContribution() can sample before the playable has been started, so it may return stale or empty data. Move the lazy-start initialization logic out of processFrame() into a shared guard used by both processFrame() and getContribution() in TransformPlayable, ensuring start() and started are handled consistently before sampleAnimation() runs.web-packages/test/unit/src/effects-core/plugins/cal/transform-clip-mix.spec.ts (1)
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the scene fixture and rename
comp
buildScene()can be typed asScene.LoadTypeinstead of usingscene as anyat the four callsites, which preserves shape checking on the hand-authored fixture. Renamecomptocompositionfor readability.🤖 Prompt for 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. In `@web-packages/test/unit/src/effects-core/plugins/cal/transform-clip-mix.spec.ts` around lines 30 - 34, Update the test fixture helper buildScene to use Scene.LoadType for the scene object so the hand-authored fixture is type-checked instead of casting scene as any at each callsite. Also rename the local variable comp to composition in the transform-clip-mix spec for clarity, and update any references in the helper or assertions to match.Source: Coding guidelines
🤖 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
`@packages/effects-core/src/plugins/timeline/playables/transform-mixer-playable.ts`:
- Around line 25-55: The evaluate() path in TransformMixerPlayable currently
only updates the bound transform when there is at least one active clip, so
frames with no contributing clips leave item.transform stuck on the previous
pose. Update the no-active-clip branch in evaluate() to explicitly reset the
transform to the captured base pose (or otherwise clear the mixed result) before
returning, and keep the existing
captureBasePose/resetFrame/addContribution/flush flow for active clips.
In `@packages/effects-core/src/plugins/timeline/transform-clip-mixer.ts`:
- Around line 89-101: Reset transform channels when their contributions
disappear: flush() in TransformClipMixer currently only applies channels that
are active, so stale position/rotation/scale values can linger after a clip
ends. Update TransformClipMixer.flush() to restore each channel to the base pose
when its contribution is no longer present, and add a restore/flush path in
TransformMixerPlayable.evaluate() for the case where no transform clips are
active but a transform was previously applied.
In `@web-packages/demo/src/transform-clip-mix.ts`:
- Around line 206-216: The async IIFE around Player setup is still vulnerable to
an unhandled rejection because player.loadScene(scene) is awaited without
explicit rejection handling, and the onError callback only covers
Player-reported errors. Update the async wrapper in transform-clip-mix so the
loadScene call is wrapped in explicit error handling (using the surrounding
async function and its await path), and log or surface the failure from
player.loadScene(scene) in the same style as the existing onError handler.
---
Nitpick comments:
In `@packages/effects-core/src/plugins/timeline/playables/transform-playable.ts`:
- Around line 153-174: getContribution() can sample before the playable has been
started, so it may return stale or empty data. Move the lazy-start
initialization logic out of processFrame() into a shared guard used by both
processFrame() and getContribution() in TransformPlayable, ensuring start() and
started are handled consistently before sampleAnimation() runs.
In
`@web-packages/test/unit/src/effects-core/plugins/cal/transform-clip-mix.spec.ts`:
- Around line 30-34: Update the test fixture helper buildScene to use
Scene.LoadType for the scene object so the hand-authored fixture is type-checked
instead of casting scene as any at each callsite. Also rename the local variable
comp to composition in the transform-clip-mix spec for clarity, and update any
references in the helper or assertions to match.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7464f0c-eec9-46fe-be4b-9982698eea48
📒 Files selected for processing (9)
packages/effects-core/src/plugins/timeline/playables/transform-mixer-playable.tspackages/effects-core/src/plugins/timeline/playables/transform-playable.tspackages/effects-core/src/plugins/timeline/track-instance.tspackages/effects-core/src/plugins/timeline/transform-clip-mixer.tsweb-packages/demo/html/transform-clip-mix.htmlweb-packages/demo/index.htmlweb-packages/demo/src/transform-clip-mix.tsweb-packages/test/unit/src/effects-core/index.tsweb-packages/test/unit/src/effects-core/plugins/cal/transform-clip-mix.spec.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/effects-core/src/plugins/timeline/transform-clip-mixer.ts (1)
130-145: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCapture the base Z scale from
scale.z.Line 137 seeds the cached base scale as
(x, y, x), so any item withscale.z !== scale.xwill mix and restore against the wrong Z baseline. The current scale tests only use uniform values, so this slips through.Proposed fix
this.basePose = { position: item.transform.position.clone(), rotation: item.transform.getRotation().clone(), - scale: new Vector3(scale.x, scale.y, scale.x), + scale: new Vector3(scale.x, scale.y, scale.z), };🤖 Prompt for 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. In `@packages/effects-core/src/plugins/timeline/transform-clip-mixer.ts` around lines 130 - 145, The cached base pose in ensureBasePose is seeding scale with the wrong Z value, so items with non-uniform scaling restore against an incorrect baseline. Update the basePose construction in transform-clip-mixer.ts to preserve the item’s actual scale.z when cloning the scale, keeping the existing position/rotation caching and boundInstanceId reset logic unchanged.
🤖 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.
Outside diff comments:
In `@packages/effects-core/src/plugins/timeline/transform-clip-mixer.ts`:
- Around line 130-145: The cached base pose in ensureBasePose is seeding scale
with the wrong Z value, so items with non-uniform scaling restore against an
incorrect baseline. Update the basePose construction in transform-clip-mixer.ts
to preserve the item’s actual scale.z when cloning the scale, keeping the
existing position/rotation caching and boundInstanceId reset logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00ff01a3-1b60-4e99-bba7-46ec22556089
📒 Files selected for processing (5)
packages/effects-core/src/plugins/timeline/playables/transform-mixer-playable.tspackages/effects-core/src/plugins/timeline/playables/transform-playable.tspackages/effects-core/src/plugins/timeline/transform-clip-mixer.tsweb-packages/demo/src/transform-clip-mix.tsweb-packages/test/unit/src/effects-core/plugins/cal/transform-clip-mix.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web-packages/demo/src/transform-clip-mix.ts
- packages/effects-core/src/plugins/timeline/playables/transform-playable.ts
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
1 participant
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.
Summary
Details
This changes Transform animation evaluation from direct per-clip transform writes to contribution-based mixing:
The Timeline core remains generic. FrameContext, TimelineAsset, and TimelineInstance do not need to know about Transform-specific mixing.
Compatibility
Summary by CodeRabbit