-
Notifications
You must be signed in to change notification settings - Fork 24
feat: add animation event #1349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds animation event support: new event types and NotifyEvent, stores events on AnimationClip, samples and collects events in the animation graph, dispatches them from Animator during update, and re-emits them from Composition. Changes
Sequence DiagramsequenceDiagram
participant ClipNode as AnimationClipNode
participant Context as GraphContext
participant GraphInst as GraphInstance
participant Animator as Animator
participant Item as VFXItem
participant Composition as Composition
Note over ClipNode,Context: per-frame evaluation
ClipNode->>ClipNode: sampleEvents(context)
ClipNode->>Context: push AnimationEventReference to activeEvents
Context->>GraphInst: activeEvents (shared)
Note over Animator: update tick
Animator->>GraphInst: iterate activeEvents
Animator->>Item: event.onEvent(item, clip, eventRef)
Item->>Item: emit('animationevent', eventRef)
Note over Composition: propagation
Item->>Composition: 'animationevent' listener on rootItem
Composition->>Composition: emit('animationevent', eventRef)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/effects-core/src/animation/animation-clip.ts (1)
189-204: Track the spec update for event data types.The @ts-expect-error suppressions are noted as temporary while the spec is updated. Consider creating a tracking issue to ensure the spec update isn't forgotten.
packages/effects-core/src/composition.ts (1)
583-583: Minor formatting adjustment.Whitespace removed before the parameter list. This is a stylistic change with no semantic impact.
packages/effects-core/src/plugins/animation-graph/nodes/animation-clip-node.ts (1)
10-10: Use relative import path for consistency.This import uses an absolute path starting with
packages/, while other files in the codebase use relative imports (e.g.,'../../animation/animation-events').🔎 Proposed fix
-import type { AnimationEventReference } from 'packages/effects-core/src/animation/animation-events'; +import type { AnimationEventReference } from '../../../animation/animation-events';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/effects-core/src/animation/animation-clip.tspackages/effects-core/src/animation/animation-events.tspackages/effects-core/src/components/animator.tspackages/effects-core/src/composition.tspackages/effects-core/src/events/types.tspackages/effects-core/src/plugins/animation-graph/graph-context.tspackages/effects-core/src/plugins/animation-graph/graph-instance.tspackages/effects-core/src/plugins/animation-graph/nodes/animation-clip-node.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects-core/src/plugins/animation-graph/graph-context.tspackages/effects-core/src/composition.tspackages/effects-core/src/plugins/animation-graph/nodes/animation-clip-node.ts
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
packages/effects-core/src/composition.ts
🧬 Code graph analysis (6)
packages/effects-core/src/plugins/animation-graph/graph-context.ts (1)
packages/effects-core/src/animation/animation-events.ts (1)
AnimationEventReference(4-8)
packages/effects-core/src/animation/animation-clip.ts (1)
packages/effects-core/src/animation/animation-events.ts (2)
AnimationEventInfo(10-16)NotifyEvent(35-39)
packages/effects-core/src/events/types.ts (1)
packages/effects-core/src/animation/animation-events.ts (1)
AnimationEventReference(4-8)
packages/effects-core/src/plugins/animation-graph/graph-instance.ts (1)
packages/effects-core/src/animation/animation-events.ts (1)
AnimationEventReference(4-8)
packages/effects-core/src/components/animator.ts (1)
packages/effects/src/player.ts (1)
event(87-89)
packages/effects-core/src/composition.ts (1)
packages/effects-core/src/utils/index.ts (1)
Constructor(21-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/effects-core/src/plugins/animation-graph/graph-instance.ts (1)
16-16: LGTM: Clean per-frame event lifecycle.The activeEvents array is properly initialized, shared with the context, and cleared at the start of each frame evaluation to prevent event accumulation across frames.
Also applies to: 64-64, 82-83
packages/effects-core/src/components/animator.ts (1)
141-148: LGTM: Event dispatch integrates cleanly into the update cycle.Events are dispatched after applying transforms and properties, ensuring the item state is fully updated before event handlers execute.
packages/effects-core/src/events/types.ts (1)
1-1: LGTM: Event type extensions are well-structured.The animation event payload is consistently added to both ItemEvent and CompositionEvent types, enabling proper type checking throughout the event system.
Also applies to: 33-36, 82-85
packages/effects-core/src/composition.ts (1)
339-342: LGTM: Event forwarding enables external listeners.The composition correctly subscribes to and re-emits animation events from the root item, allowing external code to receive events. Cleanup is handled automatically when rootItem is disposed.
packages/effects-core/src/plugins/animation-graph/nodes/animation-clip-node.ts (1)
64-67: LGTM: Event sampling integrated into evaluation flow.The event sampling is properly invoked before pose computation, and events are initialized in the Animatable constructor from the animation clip's event data.
Also applies to: 144-144, 173-179
packages/effects-core/src/animation/animation-events.ts (1)
1-39: LGTM: Well-structured event system foundation.The animation event module provides a clean separation of concerns:
- Data interfaces (
AnimationEventInfoData,AnimationEventData) for serialization- Runtime interfaces (
AnimationEventReference,AnimationEventInfo) for execution- Extensible base class (
AnimationEvent) with concrete implementation (NotifyEvent)The design allows for future event types while providing a working implementation that integrates with the VFXItem event system.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.