Put layerIndex and layer in AnimatorLayerData to reduce code #2390
Conversation
luzhuang
added
animation
WalkthroughThe pull request includes significant refactoring of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
packages/core/src/animation/internal/AnimatorLayerData.ts (1)
Line range hint
1-31: Summary and Recommendations
- The new properties
layerIndexandlayerhave been added toAnimatorLayerDataas intended.- The import for
AnimatorControllerLayerhas been correctly added.- There are concerns about the initialization and usage of the new properties.
- It's unclear how these changes contribute to code reduction mentioned in the PR objectives.
Recommendations:
- Initialize the new properties to prevent potential undefined behavior.
- Provide more context on how these properties will be used within the class and in the broader codebase.
- Clarify how these changes contribute to code reduction, possibly by showing changes in other files that benefit from these new properties.
- Confirm that these changes don't introduce breaking changes, considering the
@internalnature of the class.- Consider adding tests and updating documentation as mentioned in the PR checklist.
These changes seem to be part of a larger refactoring effort. To ensure the changes are beneficial:
- Document the purpose of these new properties in the class.
- If these properties are meant to replace some existing code patterns, consider deprecating the old patterns to guide future development.
- Ensure that the addition of these properties doesn't violate the single responsibility principle for the
AnimatorLayerDataclass.packages/core/src/animation/Animator.ts (3)
681-681: Use explicitifstatement instead of short-circuit evaluation for clarityAt line 681, short-circuit evaluation is used to conditionally call
_updateState. While this is concise, using an explicitifstatement improves readability and makes the code's intent clearer.Refactor the code as follows:
- remainDeltaTime > 0 && this._updateState(layerData, remainDeltaTime, aniUpdate); + if (remainDeltaTime > 0) { + this._updateState(layerData, remainDeltaTime, aniUpdate); + }
798-798: Use explicitifstatement instead of short-circuit evaluation for claritySimilar to line 681, at line 798, replacing the short-circuit evaluation with an
ifstatement enhances code readability.Refactor the code as follows:
- remainDeltaTime > 0 && this._updateState(layerData, remainDeltaTime, aniUpdate); + if (remainDeltaTime > 0) { + this._updateState(layerData, remainDeltaTime, aniUpdate); + }
908-908: Use explicitifstatement instead of short-circuit evaluation for clarityAt line 908, using an explicit
ifstatement instead of short-circuit evaluation makes the code more readable and the execution flow clearer.Refactor the code as follows:
- remainDeltaTime > 0 && this._updateState(layerData, remainDeltaTime, aniUpdate); + if (remainDeltaTime > 0) { + this._updateState(layerData, remainDeltaTime, aniUpdate); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/src/animation/Animator.ts (15 hunks)
- packages/core/src/animation/internal/AnimatorLayerData.ts (2 hunks)
🧰 Additional context used
Biome
packages/core/src/animation/Animator.ts
[error] 562-562: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments not posted (4)
packages/core/src/animation/internal/AnimatorLayerData.ts (3)
1-1: LGTM: Import statement added for new property type.The import for
AnimatorControllerLayerhas been correctly added to support the newlayerproperty.
Line range hint
9-31: Clarification needed on code reduction mentioned in PR objectives.The PR objectives mention "reducing code", but the changes in this file only add new properties. While this might be part of a larger refactoring effort, it's not clear from this file alone how these changes contribute to code reduction.
Could you please provide more context on how these new properties help in reducing code elsewhere in the project? This would help in understanding the full impact of these changes.
Also, given that this class is marked as
@internal, can you confirm that these changes don't introduce any breaking changes for library users?To get a better understanding of the impact, let's check for files that import and use
AnimatorLayerData:#!/bin/bash # Search for files that import and use AnimatorLayerData rg --type typescript -e "import.*AnimatorLayerData" -A 10 | rg -e "new AnimatorLayerData" -B 10
12-13:⚠️ Potential issueNew properties added, but initialization and usage unclear.
The new properties
layerIndexandlayerhave been added to theAnimatorLayerDataclass as per the PR objective. However, there are some concerns:
- These properties are not initialized, which could lead to undefined behavior.
- The purpose and usage of these properties within the class are not clear from this change alone.
Could you please provide more context on how these properties will be used and ensure they are properly initialized?
Consider initializing these properties, either in the constructor or with default values:
layerIndex: number = -1; // or any appropriate default value layer: AnimatorControllerLayer | null = null;To understand the usage of these new properties, let's check for their occurrences in the codebase:
✅ Verification successful
New properties are properly integrated and initialized across the codebase.
After searching the codebase, it’s clear that the
layerIndexandlayerproperties in theAnimatorLayerDataclass are consistently initialized and utilized across multiple files. Their addition aligns with the project's objectives, and there are no indications of undefined behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of layerIndex and layer properties rg --type typescript -e "layerIndex" -e "\.layer\b" --glob "!**/AnimatorLayerData.ts"Length of output: 132
Script:
#!/bin/bash # Search for usages of layerIndex and layer properties in TypeScript files, excluding AnimatorLayerData.ts rg -e "layerIndex" -e "\.layer\b" --glob "*.ts" --glob "*.tsx" --glob "!**/AnimatorLayerData.ts"Length of output: 13118
packages/core/src/animation/Animator.ts (1)
198-198: Refactored_updateStatecall enhances code maintainabilityThe call to
_updateStatenow utilizeslayerDatadirectly, eliminating the need to passlayerIndexandlayerseparately. This refactoring reduces parameter redundancy and improves code readability.
| aniUpdate: boolean | ||
| ): void { | ||
| private _updateState(layerData: AnimatorLayerData, deltaTime: number, aniUpdate: boolean): void { | ||
| const { layerIndex, layer } = layerData; | ||
| let { weight } = layer; | ||
| const additive = layer.blendingMode === AnimatorLayerBlendingMode.Additive; | ||
|
|
||
| layerIndex === 0 && (weight = 1.0); |
There was a problem hiding this comment.
Avoid assignments within expressions to enhance code clarity
According to static analysis, at line 562, there is an assignment used within an expression. Assignments within expressions can be confusing and may lead to unintended side effects, as expressions are generally expected to be side-effect-free.
Refactor the code to separate the assignment from the expression. For example, if the code is:
case LayerState.Playing:
layerData.layerState = LayerState.Playing;
// Additional code...Ensure that assignments are not embedded within conditions or expression statements.
🧰 Tools
Biome
[error] 562-562: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
|
|
||
| const transition = | ||
| (anyStateTransitions.count && | ||
| this._applyTransitionsByCondition(layerIndex, layerData, layer, anyStateTransitions, aniUpdate)) || | ||
| (anyStateTransitions.count && this._applyTransitionsByCondition(layerData, anyStateTransitions, aniUpdate)) || |
There was a problem hiding this comment.
Check for transition before recursive call to _updateState
At line 970, when applying transitions, ensure that the recursive call to _updateState only occurs if a valid transition is found. This prevents potential infinite recursion or unintended behavior.
Verify that transition is not null before making the recursive call.
| @@ -1195,7 +1171,7 @@ | |||
| if (isSoloMode && !transition.solo) continue; | |||
|
|
|||
| if (this._checkConditions(transition)) { | |||
| if (this._applyTransition(layerIndex, layerData, layer, transition, aniUpdate)) { | |||
| if (this._applyTransition(layerData, transition, aniUpdate)) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate transition checks to reduce code duplication
The condition checking and application of transitions at lines 1174, 1217, and 1242 follow a similar pattern. Consider refactoring these sections into a single method to reduce code duplication and improve maintainability.
Extract the common logic into a helper function that handles the transition checking and application.
| private _checkAnyAndEntryState(layerData: AnimatorLayerData, remainDeltaTime: number, aniUpdate: boolean): void { | ||
| const { layerIndex, layer } = layerData; | ||
| const { stateMachine } = layer; | ||
| const { _anyStateTransitionCollection: anyStateTransitions, _entryTransitionCollection: entryTransitions } = | ||
| stateMachine; | ||
| let transition: AnimatorStateTransition; | ||
|
|
||
| transition = | ||
| anyStateTransitions.count && | ||
| this._applyTransitionsByCondition(layerIndex, layerData, layer, anyStateTransitions, aniUpdate); | ||
| anyStateTransitions.count && this._applyTransitionsByCondition(layerData, anyStateTransitions, aniUpdate); | ||
|
|
||
| if (!transition) { | ||
| transition = | ||
| entryTransitions.count && | ||
| this._applyTransitionsByCondition(layerIndex, layerData, layer, entryTransitions, aniUpdate); | ||
| transition = entryTransitions.count && this._applyTransitionsByCondition(layerData, entryTransitions, aniUpdate); | ||
| } | ||
|
|
||
| if (transition) { | ||
| this._updateState(layerIndex, layerData, layer, remainDeltaTime, aniUpdate); | ||
| this._updateState(layerData, remainDeltaTime, aniUpdate); | ||
| } else { | ||
| const defaultState = stateMachine.defaultState; | ||
| if (defaultState) { | ||
| this._preparePlay(defaultState, layerIndex); | ||
| this._updateState(layerIndex, layerData, layer, remainDeltaTime, aniUpdate); | ||
| this._updateState(layerData, remainDeltaTime, aniUpdate); |
There was a problem hiding this comment.
Ensure default state is properly initialized
In the _checkAnyAndEntryState method (lines 1497-1517), make sure that the default state of the state machine is correctly initialized when no transitions are applied. This ensures that the animator starts in a valid state.
Verify that defaultState is not null and handle the case where it might be undefined to prevent runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/src/animation/Animator.ts (1)
Line range hint
1-1574: Overall approval: Effective implementation of AnimatorLayerData enhancementsThe changes throughout the file consistently and effectively implement the use of
AnimatorLayerDatato store and accesslayerIndexandlayer. This refactoring aligns well with the PR objectives, resulting in reduced parameter passing, simplified method signatures, and improved code organization. The modifications enhance the maintainability and readability of the animator system.However, there's one minor issue to address:
On line 562, there's an assignment within an expression:
layerData.layerIndex === 0 && (weight = 1.0);This style can be confusing and is generally discouraged.
Consider refactoring this line to use a more explicit if statement:
if (layerData.layerIndex === 0) { weight = 1.0; }This change will improve code clarity and adhere to best practices for avoiding assignments within expressions.
🧰 Tools
Biome
[error] 562-562: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/core/src/animation/Animator.ts (15 hunks)
🧰 Additional context used
Biome
packages/core/src/animation/Animator.ts
[error] 562-562: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments not posted (7)
packages/core/src/animation/Animator.ts (7)
557-557: Simplified method signature improves code clarityThe removal of
layerIndexandlayerparameters from the_updateStatemethod signature aligns with the PR objectives. This change reduces redundant parameter passing and improves code readability by relying on theAnimatorLayerDatastructure to contain this information.
558-578: Consistent use of AnimatorLayerData improves code maintainabilityThe changes in the
_updateStatemethod body consistently apply the new approach of usingAnimatorLayerDatato accesslayerandlayerIndex. This reduces code duplication and improves maintainability. The extraction oflayerfromlayerDataand the updated condition for settingweightdemonstrate a more cohesive use of theAnimatorLayerDatastructure.🧰 Tools
Biome
[error] 562-562: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
605-605: Efficient use of AnimatorLayerData enhances code structureThe changes in the
_updatePlayingStatemethod demonstrate a consistent and efficient use of theAnimatorLayerDatastructure. The extraction ofanyStateTransitionsfromlayerData.layer.stateMachineshowcases how thelayerproperty is now accessible throughlayerData. The updated method calls, including_fireAnimationEventsAndCallScriptsand the recursive call to_updateState, now uselayerDataor its properties directly, reducing parameter passing and potentially improving performance.Also applies to: 669-676, 681-681
723-723: Improved readability through consistent use of AnimatorLayerDataThe changes in the
_updateCrossFadeStatemethod further demonstrate the consistent use of theAnimatorLayerDatastructure. The extraction ofsrcPlayData,destPlayData, andlayerIndexfromlayerDataat the beginning of the method improves readability and clearly shows that these properties are now part of theAnimatorLayerDatastructure. The updated recursive call to_updateStatemaintains consistency with the new approach, potentially reducing parameter passing and improving code maintainability.Also applies to: 798-798
897-897: Consistent application of AnimatorLayerData across methodsThe changes in the
_updateCrossFadeFromPoseStatemethod maintain consistency with the new approach of usingAnimatorLayerData. The use oflayerData.layerIndexin the_fireAnimationEventsAndCallScriptscall and the updated recursive call to_updateStatedemonstrate a uniform application of this pattern across different methods. This consistency improves code maintainability and reduces the likelihood of errors related to parameter passing.Also applies to: 908-908
1497-1517: Comprehensive application of AnimatorLayerData enhances code structureThe changes in the
_checkAnyAndEntryStatemethod demonstrate a thorough and consistent application of theAnimatorLayerDatastructure. The updated method signature and the extraction ofstateMachinefromlayerData.layershowcase how thelayerproperty is now accessible throughlayerData. All subsequent method calls within this function now uselayerData, maintaining consistency with the new approach. This comprehensive refactoring enhances code structure, reduces parameter passing, and improves overall maintainability of the animator system.
548-553: Core implementation of AnimatorLayerData enhancementThe changes in the
_getAnimatorLayerDatamethod represent the core implementation of the PR objectives. By initializinglayerIndexandlayerproperties in theAnimatorLayerDataobject, this modification enables all the code simplifications seen throughout the file. This central change improves the structure of theAnimatorLayerData, reducing the need for separate parameter passing and enhancing the overall organization of the animator system.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Refactor