Tree-shakable AnimationManager - #840
Open
jfboeve wants to merge 3 commits into
Open
Conversation
jfboeve
changed the title
wouterlucas
requested changes
Jul 27, 2026
wouterlucas
left a comment
wouterlucas
left a comment
Contributor
There was a problem hiding this comment.
ok - stopped reviewing
Think your agent did this silly thing with extending the animation manager class which makes it very messy.
RendererMain is its own class
Animation Manager can be an abstract type
The old animation manager should implement said abstract class
You can provide the animation manager, or not if you chose not to use one, on renderer main init.
| maxRetryCount?: number; | ||
| }; | ||
| export type RendererMainSettings<A extends AnimationManager> = | ||
| RendererRuntimeSettings & |
Contributor
There was a problem hiding this comment.
do we need this A extends AnimationManager ?
| readonly root: INode; | ||
| export class RendererMain< | ||
| A extends AnimationManager = AnimationManager, | ||
| > extends EventEmitter { |
Contributor
There was a problem hiding this comment.
same here - why are we extending?
|
|
||
| assertTruthy( | ||
| settings.animationManager, | ||
| 'An animation manager must be provided in settings.animationManager', |
Contributor
There was a problem hiding this comment.
does it? can be null to right? if we dont want to use an animation manager
| this.inspector = new inspector( | ||
| this.canvas, | ||
| settings as RendererMainSettings, | ||
| settings as RendererMainSettings<A>, |
|
|
||
| // Extract the root node | ||
| this.root = this.stage.root as unknown as INode; | ||
| this.root = this.stage.root as unknown as INode<A>; |
| createNode<ShNode extends CoreShaderNode<any>>( | ||
| props: Partial<INodeProps<ShNode>>, | ||
| ): INode<ShNode> { | ||
| props: Partial<INodeProps<A, ShNode>>, |
Contributor
There was a problem hiding this comment.
stahhpp its leaking everywhere 🫣
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
2 participants
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.
This pull request refactors the animation system to introduce a new
AnimationManagerinterface and a concrete implementation calledCoreAnimationManager. It standardizes how animations are created and managed, updates the API for animating nodes, and ensures consistent usage across the codebase and tests. The changes also update type signatures and dependency injection for improved flexibility and testability.Animation System Refactor
AnimationManagerinterface and implemented it asCoreAnimationManager, clarifying animation management responsibilities and exposing a consistent API (animate,animateNode, andupdate). (src/core/animations/AnimationManager.ts) [1] [2]createAnimationmethod to the newanimatemethod in the codebase and tests, ensuring consistency. (src/core/animations/AnimationManager.test.ts,src/core/animations/CoreAnimationController.test.ts) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]CoreNode Animation API Changes
animatemethod fromCoreNodeand replaced it with a property that delegates tostage.animationManager.animateNode, aligning with the new animation manager API. (src/core/CoreNode.ts) [1] [2]CoreNode. (src/core/CoreNode.ts)Renderer and Stage Integration
Stageand renderer initialization to accept ananimationManagerinstance via options, improving dependency injection and flexibility. (src/core/Stage.ts,examples/index.ts) [1] [2] [3] [4] [5] [6] [7]Type and Dependency Updates
CoreAnimationManagerwhere appropriate, ensuring type safety and clarity throughout the codebase. (src/core/animations/CoreAnimationController.ts) [1] [2]These changes collectively modernize the animation system, improve code clarity, and make the animation API more flexible and consistent. This also allows you to implement your own animation manager.