Fix animate property is not effect when reference changed #2383
Conversation
luzhuang
added
bug
WalkthroughThis pull request introduces a comprehensive animation system enhancement within a 3D engine. It includes the implementation of an animation script for material properties and mesh transformations, along with updates to various core animation classes. New abstractions for managing animation property references are introduced, improving the organization and efficiency of the animation system. Additionally, configuration settings for the animation cases are updated, and tests are modified to align with the new architecture. 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 (
|
luzhuang
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range and nitpick comments (5)
packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts (2)
20-76: Add JSDoc comments for theaddReferencemethod to enhance maintainabilityIncluding JSDoc comments for the
addReferencemethod will improve code readability and help other developers understand the purpose, parameters, and return value of the method.
78-81: Add JSDoc comments for theclearmethodDocumenting the
clearmethod with JSDoc comments will enhance clarity regarding its functionality and intent.packages/core/src/animation/internal/AnimationPropertyReference.ts (1)
3-39: Specify explicit types forvalueand otheranytyped propertiesThe
valueproperty and method parameters are currently typed asany. To improve type safety and leverage TypeScript's benefits, consider specifying more precise types based on the expected data these properties will hold.packages/core/src/animation/internal/animationCurveOwner/AnimationCurveOwner.ts (1)
47-48: Ensure the_assemblerproperty visibility is appropriate.The
_assemblerproperty is marked with the/** @internal */comment. Verify that this aligns with the intended API exposure and that external modules do not rely on this property.packages/core/src/animation/Animator.ts (1)
446-446: Correct grammatical error in warning messageThe log message contains a grammatical error. It should read "doesn't have" instead of "don't have" for proper subject-verb agreement.
Apply the following diff to fix the message:
- Logger.warn(`The entity don\'t have the child entity which path is ${relativePath}.`); + Logger.warn(`The entity doesn't have a child entity with the path ${relativePath}.`);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
e2e/fixtures/originImage/Animator_animator-referenceAndProperty.jpgis excluded by!**/*.jpg
Files selected for processing (11)
- e2e/case/animator-referenceAndProperty.ts (1 hunks)
- e2e/config.ts (1 hunks)
- packages/core/src/animation/AnimationClip.ts (4 hunks)
- packages/core/src/animation/AnimationClipCurveBinding.ts (3 hunks)
- packages/core/src/animation/Animator.ts (13 hunks)
- packages/core/src/animation/animationCurve/AnimationFloatArrayCurve.ts (2 hunks)
- packages/core/src/animation/internal/AnimationPropertyReference.ts (1 hunks)
- packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts (1 hunks)
- packages/core/src/animation/internal/animationCurveOwner/AnimationCurveOwner.ts (4 hunks)
- packages/core/src/animation/internal/animationCurveOwner/assembler/UniversalAnimationCurveOwnerAssembler.ts (1 hunks)
- tests/src/core/Animator.test.ts (1 hunks)
Additional context used
Biome
e2e/case/animator-referenceAndProperty.ts
[error] 71-71: 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)
packages/core/src/animation/Animator.ts
[error] 424-426: 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)
[error] 427-431: 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)
[error] 434-436: 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)
[error] 437-437: 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 (21)
packages/core/src/animation/AnimationClipCurveBinding.ts (3)
7-7: LGTM!The import statement for
AnimationPropertyReferenceManageris correctly added.
37-38: Skipped commenting due to insufficient context.The purpose and usage of the
_pathLengthproperty are not clear from the provided code changes. More context is needed to provide a meaningful review.
44-58: LGTM!The changes in the
_createCurveOwnerand_getTempCurveOwnermethods introduce a new dependency onAnimationPropertyReferenceManager. ThereferenceManageris now passed as an additional parameter to these methods and is used when instantiatingAnimationCurveOwnerobjects.This adjustment allows the methods to utilize the reference manager for managing animation property references, potentially improving the flexibility and maintainability of the animation system.
The changes are consistent across both methods, and there are no apparent issues with the code.
Also applies to: 80-87
packages/core/src/animation/animationCurve/AnimationFloatArrayCurve.ts (2)
22-22: LGTM!The change encapsulates the logic for obtaining the target value size within the
_assembler.getTargetValue()method, improving flexibility and maintainability. The initialization of related properties remains consistent.
33-33: LGTM!The change aligns with the updated approach in the
_initializeOwnermethod, retrieving the target value size using the_assembler.getTargetValue()method of thecurveOwnerobject. The casting toFloat32Arrayensures type safety.e2e/config.ts (1)
72-76: LGTM!The new configuration entry
referenceAndPropertyfollows the existing structure and naming convention of theE2E_CONFIGobject. Thecategoryandthresholdvalues are consistent with other entries in theAnimatorcategory. This addition enhances the configurability of the Animator category by allowing for an additional case scenario to be specified.tests/src/core/Animator.test.ts (1)
140-140: LGTM!The change to retrieve the current value of the curve owner using
_assembler.getTargetValue()instead of directly accessing thereferenceTargetValueproperty is a good refactor. It encapsulates the value retrieval process, allowing for additional logic or processing if needed in the future, while maintaining the same test expectation.e2e/case/animator-referenceAndProperty.ts (1)
107-107: Confirm rotation units for animationEnsure that the rotation values provided correspond to the expected units (degrees or radians) expected by the
Transformcomponent. This will prevent any unexpected behavior during the animation.packages/core/src/animation/internal/AnimationPropertyReference.ts (1)
155-159: LGTM!The
MountedParseFlagenum is well-defined and correctly uses bitwise flags for parsing states.packages/core/src/animation/internal/animationCurveOwner/AnimationCurveOwner.ts (9)
6-6: ImportAnimationPropertyReferenceManageradded correctly.The import statement for
AnimationPropertyReferenceManageris necessary for the new reference management functionality and is correctly implemented.
38-38: Addition ofreferenceManagerproperty enhances reference handling.Adding the
readonly referenceManager: AnimationPropertyReferenceManager;property allows theAnimationCurveOwnerto manage animation property references more effectively.
150-150: Correctly saving default value usinggetTargetValue().The
saveDefaultValuemethod now retrieves the target value directly from the assembler, ensuring the default value is up-to-date.
158-158: Accurately saving fixed pose value usinggetTargetValue().The
saveFixedPoseValuemethod correctly saves the fixed pose value by obtaining it from the assembler, enhancing accuracy during pose transitions.
166-166: Handle potentialnullinreferenceTargetValue.The
referenceTargetValuemight benullwhencureType._isCopyModeisfalse. Ensure that all methods receiving this variable handlenullvalues appropriately to prevent runtime errors.To confirm that methods handling
referenceTargetValuearenull-safe, review the implementations of the following methods:
cureType._additiveValue()cureType._setValue()cureType._lerpValue()
172-172: VerifyreferenceTargetValuehandling in_additiveValue().Ensure that
cureType._additiveValue(value, weight, referenceTargetValue)correctly manages cases wherereferenceTargetValuemay benull, maintaining the integrity of additive animations.
181-181: ConfirmreferenceTargetValueusage in_setValue().Check that
cureType._setValue(value, referenceTargetValue)can handle anullreferenceTargetValuewithout causing exceptions, ensuring proper value setting in all scenarios.
187-187: EnsurereferenceTargetValueis correctly used in_lerpValue().Validate that
cureType._lerpValue(referenceTargetValue, value, weight, referenceTargetValue)functions correctly even ifreferenceTargetValueisnull, to maintain smooth interpolation.
51-59: Constructor signature change may introduce breaking changes.The constructor of
AnimationCurveOwnernow requires an additionalreferenceManagerparameter. This modification could break existing code that instantiatesAnimationCurveOwnerwithout this parameter. If backward compatibility is a concern, consider providing a default value or an overloaded constructor.To identify all instantiations of
AnimationCurveOwnerthat may be affected, run the following script:Verification successful
To gather more information about the instantiation of AnimationCurveOwner, I'll generate a script to view the relevant part of AnimationClipCurveBinding.ts.
Constructor signature change does not introduce breaking changes.
The code in
AnimationClipCurveBinding.tshas been updated to accommodate the new constructor signature ofAnimationCurveOwner. ThereferenceManagerparameter is correctly provided as the first argument in the instantiation, indicating that the change has been properly propagated.
- In
packages/core/src/animation/AnimationClipCurveBinding.ts, the_createCurveOwnermethod passesreferenceManageras the first argument to theAnimationCurveOwnerconstructor.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances where AnimationCurveOwner is instantiated. # Search for instantiations of AnimationCurveOwner in TypeScript files rg --type ts 'new AnimationCurveOwner\('Length of output: 142
Script:
#!/bin/bash # Description: View the context of AnimationCurveOwner instantiation in AnimationClipCurveBinding.ts rg --type ts -C 10 'new AnimationCurveOwner\(' packages/core/src/animation/AnimationClipCurveBinding.tsLength of output: 659
packages/core/src/animation/AnimationClip.ts (3)
9-9: Import statement added correctlyThe import of
AnimationPropertyReferenceManageris properly added, and the path appears to be correct.
199-212: Insertion logic for curveBindings is correct and maintains orderThe logic for inserting
curveBindinginstances based onpropertyPathLengthensures that the_curveBindingsarray is maintained in order of increasing path length. This should optimize the sampling process by processing shorter paths first.
230-235: Ensure thread safety when using shared resourcesWhile JavaScript is single-threaded in most environments, if your application uses Web Workers or runs in environments with concurrent execution, accessing shared static properties may lead to race conditions. Confirm that this implementation does not introduce concurrency issues.
To verify, check for concurrent instances of
AnimationClipaccessing_tempReferenceManager:Verification successful
To further investigate the potential for concurrent access to the
_tempReferenceManagerproperty, we need to gather more information. Let's run some additional scripts to search for asynchronous usage of AnimationClip and any multi-threading libraries.
Static property usage appears safe in current context
Based on a thorough analysis of the codebase, there's no evidence to suggest that the static
_tempReferenceManagerproperty is accessed concurrently or in a multi-threaded environment. The property is used locally within theAnimationClipclass, and while there are asynchronous operations involvingAnimationClip, they don't appear to introduce concurrent access to this specific property.Key findings:
- No usage of
AnimationClipin Web Worker contexts was found.- Asynchronous operations involving
AnimationClipare primarily related to loading and parsing, which are typically one-time operations.- No multi-threading libraries or concurrent execution contexts for
AnimationClipwere identified.While it's always good to be cautious about shared resources, the current implementation appears to be safe for single-threaded environments, which is the typical case for most JavaScript applications. If the application's architecture changes in the future to include true concurrent execution, it would be wise to revisit this implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of AnimationClip in Web Workers or concurrent contexts. # Test: Look for imports or references to AnimationClip in worker scripts. fd --type f --extension js --extension ts --exec grep -E 'import.*AnimationClip|require\(".*AnimationClip.*"\)' {} # Check for Web Worker instantiation. grep -r 'new Worker' .Length of output: 857
Script:
#!/bin/bash # Search for asynchronous usage of AnimationClip and multi-threading libraries # Look for AnimationClip used with async/await or Promises echo "Searching for async usage of AnimationClip:" rg -i "async.*animationclip|animationclip.*promise|promise.*animationclip" # Search for multi-threading or parallel processing libraries echo "Searching for multi-threading libraries:" rg -i "worker_threads|cluster|child_process|parallel|concurrent" # Look for direct uses of _tempReferenceManager echo "Searching for direct uses of _tempReferenceManager:" rg "_tempReferenceManager"Length of output: 2885
| case HandleType.Property: | ||
| return this._getMounted[this._getValueName] as KeyframeValueType; | ||
| } | ||
| return this.getReference.getValue(); |
There was a problem hiding this comment.
Ensure references are initialized before use
The methods getTargetValue() and setTargetValue() rely on this.getReference and this.setReference being initialized by the initialize() method. If initialize() is not called before these methods, it could lead to runtime errors due to accessing undefined properties. Consider adding checks or safeguards to ensure that these references are initialized before use.
Apply this diff to add checks before accessing the references:
getTargetValue(): KeyframeValueType {
+ if (!this.getReference) {
+ throw new Error("getReference is not initialized. Ensure initialize() has been called.");
+ }
return this.getReference.getValue();
}
setTargetValue(value: KeyframeValueType): void {
+ if (!this.setReference) {
+ throw new Error("setReference is not initialized. Ensure initialize() has been called.");
+ }
this.setReference.setValue(value);
}Also applies to: 37-37
| getReference: AnimationPropertyReference; | ||
| setReference: AnimationPropertyReference; | ||
| referenceManager: AnimationPropertyReferenceManager; |
There was a problem hiding this comment.
Specify access modifiers for class properties
The class properties getReference, setReference, and referenceManager do not have explicit access modifiers. For better encapsulation and to adhere to TypeScript best practices, consider specifying their access levels (private, protected, or public).
Apply this diff to specify access modifiers:
+ private getReference: AnimationPropertyReference;
+ private setReference: AnimationPropertyReference;
+ private referenceManager: AnimationPropertyReferenceManager;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getReference: AnimationPropertyReference; | |
| setReference: AnimationPropertyReference; | |
| referenceManager: AnimationPropertyReferenceManager; | |
| private getReference: AnimationPropertyReference; | |
| private setReference: AnimationPropertyReference; | |
| private referenceManager: AnimationPropertyReferenceManager; |
| const existedReference = this._referenceIndexMap.get(component); | ||
| if (existedReference) { | ||
| reference = existedReference; | ||
| } else { | ||
| reference = new ComponentReference(); | ||
| reference.manager = this; | ||
| reference.value = component; | ||
| } |
There was a problem hiding this comment.
Inconsistent use of keys in _referenceIndexMap may lead to incorrect reference retrieval
At line 25, _referenceIndexMap is accessed using component as the key, whereas elsewhere, like at line 44, uniqueKey (a string) is used as the key. This inconsistent key usage can cause potential issues because object references are compared by reference in JavaScript/TypeScript Maps. If a different instance of component is used, it may lead to incorrect retrieval or cache misses.
Consider consistently using instanceId or uniqueKey as the key for _referenceIndexMap to ensure reliable retrieval of references.
Apply this diff to fix the inconsistent key usage:
- const existedReference = this._referenceIndexMap.get(component);
+ const existedReference = this._referenceIndexMap.get(instanceId);Ensure that when setting the initial ComponentReference, you use:
- this._referenceIndexMap.set(uniqueKey, reference);
+ this._referenceIndexMap.set(instanceId, reference);Committable suggestion was skipped due to low confidence.
| controller.addLayer(layer); | ||
| const stateMachine = layer.stateMachine; | ||
| const cubeState = stateMachine.addState("material"); | ||
| const cubeClip = (cubeState.clip = new AnimationClip("material")); |
There was a problem hiding this comment.
Avoid assignments within expressions for clarity
Assigning a value within an expression can make the code harder to read and maintain. It's better to separate the assignments into individual statements for improved readability.
Apply this diff to separate the assignments:
- const cubeClip = (cubeState.clip = new AnimationClip("material"));
+ const cubeClip = new AnimationClip("material");
+ cubeState.clip = cubeClip;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cubeClip = (cubeState.clip = new AnimationClip("material")); | |
| const cubeClip = new AnimationClip("material"); | |
| cubeState.clip = cubeClip; |
Tools
Biome
[error] 71-71: 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 materialCurve = new AnimationRefCurve(); | ||
| const key1 = new Keyframe<Material>(); | ||
| key1.time = 0; | ||
| key1.value = material; | ||
| const key2 = new Keyframe<Material>(); | ||
| key2.time = 1; | ||
| key2.value = material2; | ||
| const key3 = new Keyframe<Material>(); | ||
| key3.time = 2; | ||
| key3.value = material; | ||
| materialCurve.addKey(key1); | ||
| materialCurve.addKey(key2); | ||
| materialCurve.addKey(key3); |
There was a problem hiding this comment.
Simplify Keyframe creation for cleaner code
You can streamline the creation of keyframes by initializing them within a loop or using an array, which reduces repetitive code and enhances readability.
An example refactor:
const materialCurve = new AnimationRefCurve();
- const key1 = new Keyframe<Material>();
- key1.time = 0;
- key1.value = material;
- const key2 = new Keyframe<Material>();
- key2.time = 1;
- key2.value = material2;
- const key3 = new Keyframe<Material>();
- key3.time = 2;
- key3.value = material;
- materialCurve.addKey(key1);
- materialCurve.addKey(key2);
- materialCurve.addKey(key3);
+ const keyframes = [
+ { time: 0, value: material },
+ { time: 1, value: material2 },
+ { time: 2, value: material },
+ ];
+ keyframes.forEach(({ time, value }) => {
+ const key = new Keyframe<Material>();
+ key.time = time;
+ key.value = value;
+ materialCurve.addKey(key);
+ });Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const materialCurve = new AnimationRefCurve(); | |
| const key1 = new Keyframe<Material>(); | |
| key1.time = 0; | |
| key1.value = material; | |
| const key2 = new Keyframe<Material>(); | |
| key2.time = 1; | |
| key2.value = material2; | |
| const key3 = new Keyframe<Material>(); | |
| key3.time = 2; | |
| key3.value = material; | |
| materialCurve.addKey(key1); | |
| materialCurve.addKey(key2); | |
| materialCurve.addKey(key3); | |
| const materialCurve = new AnimationRefCurve(); | |
| const keyframes = [ | |
| { time: 0, value: material }, | |
| { time: 1, value: material2 }, | |
| { time: 2, value: material }, | |
| ]; | |
| keyframes.forEach(({ time, value }) => { | |
| const key = new Keyframe<Material>(); | |
| key.time = time; | |
| key.value = value; | |
| materialCurve.addKey(key); | |
| }); |
| this.property = propertyStr.slice(0, indexPos); | ||
| this.arrayIndex = parseInt(propertyStr.slice(indexPos + 1, -1)); | ||
| } |
There was a problem hiding this comment.
Add validation for array index parsing to prevent runtime errors
The code assumes that propertyStr contains a correctly formatted array index (e.g., "property[0]"). If propertyStr does not contain a '[' character or does not end with ']', the index parsing may fail, leading to incorrect behavior or NaN. Consider adding validation to ensure propertyStr is properly formatted.
Apply this diff to add validation:
const indexPos = propertyStr.indexOf("[");
+ if (indexPos === -1 || !propertyStr.endsWith("]")) {
+ throw new Error(`Invalid array property format: ${propertyStr}`);
+ }
this.property = propertyStr.slice(0, indexPos);
const indexStr = propertyStr.slice(indexPos + 1, -1);
this.arrayIndex = parseInt(indexStr, 10);
+ if (isNaN(this.arrayIndex)) {
+ throw new Error(`Invalid array index '${indexStr}' in property string: ${propertyStr}`);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.property = propertyStr.slice(0, indexPos); | |
| this.arrayIndex = parseInt(propertyStr.slice(indexPos + 1, -1)); | |
| } | |
| const indexPos = propertyStr.indexOf("["); | |
| if (indexPos === -1 || !propertyStr.endsWith("]")) { | |
| throw new Error(`Invalid array property format: ${propertyStr}`); | |
| } | |
| this.property = propertyStr.slice(0, indexPos); | |
| const indexStr = propertyStr.slice(indexPos + 1, -1); | |
| this.arrayIndex = parseInt(indexStr, 10); | |
| if (isNaN(this.arrayIndex)) { | |
| throw new Error(`Invalid array index '${indexStr}' in property string: ${propertyStr}`); | |
| } |
| set dirty(value: boolean) { | ||
| this._dirty = value; | ||
| if (value) { | ||
| for (let i = 0; i < this.invDependencies.length; i++) { | ||
| const index = this.invDependencies[i]; | ||
| const reference = this.manager.animationPropertyReferences[index]; | ||
| reference.dirty = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure manager is initialized before accessing properties to prevent runtime errors
In the dirty setter, the code accesses this.manager.animationPropertyReferences without verifying that this.manager is defined. If manager is uninitialized, this could lead to a runtime error. Consider adding a check to ensure manager is properly initialized before use.
Apply this diff to add a null check:
if (value) {
+ if (!this.manager || !this.manager.animationPropertyReferences) {
+ throw new Error('AnimationPropertyReference manager is not initialized.');
+ }
for (let i = 0; i < this.invDependencies.length; i++) {
const index = this.invDependencies[i];
const reference = this.manager.animationPropertyReferences[index];
reference.dirty = true;
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set dirty(value: boolean) { | |
| this._dirty = value; | |
| if (value) { | |
| for (let i = 0; i < this.invDependencies.length; i++) { | |
| const index = this.invDependencies[i]; | |
| const reference = this.manager.animationPropertyReferences[index]; | |
| reference.dirty = true; | |
| } | |
| } | |
| set dirty(value: boolean) { | |
| this._dirty = value; | |
| if (value) { | |
| if (!this.manager || !this.manager.animationPropertyReferences) { | |
| throw new Error('AnimationPropertyReference manager is not initialized.'); | |
| } | |
| for (let i = 0; i < this.invDependencies.length; i++) { | |
| const index = this.invDependencies[i]; | |
| const reference = this.manager.animationPropertyReferences[index]; | |
| reference.dirty = true; | |
| } | |
| } |
| const owner = (propertyOwners[property] ||= curveBinding._createCurveOwner( | ||
| this._animationPropertyReferenceManager, | ||
| targetEntity, | ||
| component | ||
| )); |
There was a problem hiding this comment.
Avoid assignments within expressions for clarity
Using assignments within expressions can make the code harder to read and maintain. It's better to separate the assignment from the expression to improve readability.
Apply the following diff to refactor the code:
- const owner = (propertyOwners[property] ||= curveBinding._createCurveOwner(
- this._animationPropertyReferenceManager,
- targetEntity,
- component
- ));
+ if (!propertyOwners[property]) {
+ propertyOwners[property] = curveBinding._createCurveOwner(
+ this._animationPropertyReferenceManager,
+ targetEntity,
+ component
+ );
+ }
+ const owner = propertyOwners[property];Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const owner = (propertyOwners[property] ||= curveBinding._createCurveOwner( | |
| this._animationPropertyReferenceManager, | |
| targetEntity, | |
| component | |
| )); | |
| if (!propertyOwners[property]) { | |
| propertyOwners[property] = curveBinding._createCurveOwner( | |
| this._animationPropertyReferenceManager, | |
| targetEntity, | |
| component | |
| ); | |
| } | |
| const owner = propertyOwners[property]; |
Tools
Biome
[error] 427-431: 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)
|
|
||
| // Get layer owner | ||
| const layerPropertyOwners = (layerCurveOwnerPool[instanceId] ||= <Record<string, AnimationCurveLayerOwner>>( | ||
| Object.create(null) | ||
| )); | ||
| const layerOwner = (layerPropertyOwners[property] ||= curve._createCurveLayerOwner(owner)); | ||
| const layerOwner = (layerPropertyOwners[property] ||= curveBinding._createCurveLayerOwner(owner)); |
There was a problem hiding this comment.
Avoid assignments within expressions for clarity
Similar to the previous comment, assigning within an expression can reduce code clarity. Refactoring will enhance maintainability.
Apply the following diff:
- const layerOwner = (layerPropertyOwners[property] ||= curveBinding._createCurveLayerOwner(owner));
+ if (!layerPropertyOwners[property]) {
+ layerPropertyOwners[property] = curveBinding._createCurveLayerOwner(owner);
+ }
+ const layerOwner = layerPropertyOwners[property];Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const layerOwner = (layerPropertyOwners[property] ||= curveBinding._createCurveLayerOwner(owner)); | |
| if (!layerPropertyOwners[property]) { | |
| layerPropertyOwners[property] = curveBinding._createCurveLayerOwner(owner); | |
| } | |
| const layerOwner = layerPropertyOwners[property]; |
Tools
Biome
[error] 437-437: 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)
| private static _tempReferenceManager: AnimationPropertyReferenceManager; | ||
|
|
There was a problem hiding this comment.
Consider changing _tempReferenceManager to an instance property to avoid shared state
Using a static _tempReferenceManager may cause shared state issues if multiple AnimationClip instances are sampled concurrently. Changing it to an instance property ensures that each AnimationClip maintains its own reference manager, avoiding potential conflicts.
Apply this diff to fix the issue:
- private static _tempReferenceManager: AnimationPropertyReferenceManager;
+ private _tempReferenceManager: AnimationPropertyReferenceManager;
...
- if (!AnimationClip._tempReferenceManager) {
- AnimationClip._tempReferenceManager = new AnimationPropertyReferenceManager();
- }
- AnimationClip._tempReferenceManager.clear();
+ if (!this._tempReferenceManager) {
+ this._tempReferenceManager = new AnimationPropertyReferenceManager();
+ }
+ this._tempReferenceManager.clear();
...
- const curveOwner = curve._getTempCurveOwner(AnimationClip._tempReferenceManager, targetEntity, component);
+ const curveOwner = curve._getTempCurveOwner(this._tempReferenceManager, targetEntity, component);Also applies to: 230-235, 248-248
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2383 +/- ##
==========================================
Coverage ? 69.62%
==========================================
Files ? 525
Lines ? 27510
Branches ? 4131
==========================================
Hits ? 19155
Misses ? 6854
Partials ? 1501
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/animation/internal/AnimationPropertyReference.ts (1 hunks)
- packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts (1 hunks)
- packages/core/src/animation/internal/animationCurveOwner/assembler/UniversalAnimationCurveOwnerAssembler.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/animation/internal/AnimationPropertyReference.ts
[warning] 24-26: packages/core/src/animation/internal/AnimationPropertyReference.ts#L24-L26
Added lines #L24 - L26 were not covered by tests
[warning] 53-53: packages/core/src/animation/internal/AnimationPropertyReference.ts#L53
Added line #L53 was not covered by tests
[warning] 85-85: packages/core/src/animation/internal/AnimationPropertyReference.ts#L85
Added line #L85 was not covered by tests
[warning] 103-103: packages/core/src/animation/internal/AnimationPropertyReference.ts#L103
Added line #L103 was not covered by tests
[warning] 107-109: packages/core/src/animation/internal/AnimationPropertyReference.ts#L107-L109
Added lines #L107 - L109 were not covered by tests
[warning] 113-113: packages/core/src/animation/internal/AnimationPropertyReference.ts#L113
Added line #L113 was not covered by tests
[warning] 115-115: packages/core/src/animation/internal/AnimationPropertyReference.ts#L115
Added line #L115 was not covered by tests
[warning] 117-119: packages/core/src/animation/internal/AnimationPropertyReference.ts#L117-L119
Added lines #L117 - L119 were not covered by tests
[warning] 123-123: packages/core/src/animation/internal/AnimationPropertyReference.ts#L123
Added line #L123 was not covered by tests
[warning] 125-125: packages/core/src/animation/internal/AnimationPropertyReference.ts#L125
Added line #L125 was not covered by tests
[warning] 127-127: packages/core/src/animation/internal/AnimationPropertyReference.ts#L127
Added line #L127 was not covered by tests
[warning] 129-129: packages/core/src/animation/internal/AnimationPropertyReference.ts#L129
Added line #L129 was not covered by tests
[warning] 131-132: packages/core/src/animation/internal/AnimationPropertyReference.ts#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 145-147: packages/core/src/animation/internal/AnimationPropertyReference.ts#L145-L147
Added lines #L145 - L147 were not covered by tests
[warning] 151-151: packages/core/src/animation/internal/AnimationPropertyReference.ts#L151
Added line #L151 was not covered by tests
[warning] 153-153: packages/core/src/animation/internal/AnimationPropertyReference.ts#L153
Added line #L153 was not covered by tests
[warning] 155-157: packages/core/src/animation/internal/AnimationPropertyReference.ts#L155-L157
Added lines #L155 - L157 were not covered by tests
[warning] 161-161: packages/core/src/animation/internal/AnimationPropertyReference.ts#L161
Added line #L161 was not covered by tests
[warning] 163-163: packages/core/src/animation/internal/AnimationPropertyReference.ts#L163
Added line #L163 was not covered by tests
[warning] 165-166: packages/core/src/animation/internal/AnimationPropertyReference.ts#L165-L166
Added lines #L165 - L166 were not covered by testspackages/core/src/animation/internal/AnimationPropertyReferenceManager.ts
[warning] 47-48: packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 55-55: packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts#L55
Added line #L55 was not covered by tests
[warning] 58-58: packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts#L58
Added line #L58 was not covered by tests
🔇 Additional comments (2)
packages/core/src/animation/internal/animationCurveOwner/assembler/UniversalAnimationCurveOwnerAssembler.ts (1)
11-12: Refactoring Enhances Clarity and EfficiencyThe refactored implementation using
_getReferenceand_setReferencesimplifies property management within theUniversalAnimationCurveOwnerAssemblerclass. By directly utilizingAnimationPropertyReferenceinstances, the code becomes more readable and maintainable. The private access modifiers provide proper encapsulation, aligning with TypeScript best practices.Also applies to: 15-26, 31-35
packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts (1)
25-33:⚠️ Potential issueInconsistent use of keys in
_referenceIndexMapmay lead to incorrect reference retrievalThis issue was previously noted and still seems to persist. At line 25,
_referenceIndexMapis accessed usinginstanceIdas the key, whereas at line 44,uniqueKey(a combination ofinstanceIdand sub-properties) is used as the key. This inconsistent key usage may result in potential issues like incorrect retrieval or cache misses.Consider consistently using
uniqueKeyorinstanceIdas the key in_referenceIndexMapto ensure reliable reference retrieval and avoid potential conflicts.Also applies to: 44-45
| reference = existReference; | ||
| continue; |
There was a problem hiding this comment.
Increase test coverage for untested code paths
Static analysis indicates that certain code paths are not covered by tests:
- Lines 47-48: Handling existing references in
_referenceIndexMap. - Line 55: Initializing an
ArrayReferencewhen the property is an array. - Line 58: Initializing a
MethodReferencewhen the property is a method.
Adding unit tests for these scenarios will enhance test coverage and ensure the reliability and correctness of these code paths.
Would you like assistance in creating unit tests for these scenarios or opening a GitHub issue to track this task?
Also applies to: 55-55, 58-58
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-48: packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts#L47-L48
Added lines #L47 - L48 were not covered by tests
| const index = this.invDependencies[i]; | ||
| const reference = this.manager.animationPropertyReferences[index]; | ||
| reference.dirty = true; |
There was a problem hiding this comment.
Increase test coverage for the dirty setter
Lines 24-26 in the dirty setter method are not covered by tests, as indicated by the static analysis tools. This logic is crucial for propagating the dirty state to dependent references. Adding unit tests for this setter will help ensure that state changes correctly trigger updates in dependencies.
Would you like assistance in creating unit tests for the dirty setter to improve test coverage?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-26: packages/core/src/animation/internal/AnimationPropertyReference.ts#L24-L26
Added lines #L24 - L26 were not covered by tests
| if (dependence.dirty) { | ||
| dependence.getValue(); | ||
| } | ||
| dependence.value[this.property][this.arrayIndex] = value; |
There was a problem hiding this comment.
Add checks before setting array element to prevent runtime errors
In ArrayReference.setValue() at line 165, the code assigns a value to dependence.value[this.property][this.arrayIndex] without ensuring that dependence.value[this.property] is an array and that this.arrayIndex is within valid bounds. This could result in runtime errors.
Apply this diff to add checks before setting the array element:
setValue(value: any) {
const dependence = this.parent;
if (dependence.dirty) {
dependence.getValue();
}
+ const array = dependence.value[this.property];
+ if (!Array.isArray(array)) {
+ throw new Error(`Property '${this.property}' is not an array on the dependence value`);
+ }
+ if (this.arrayIndex < 0 || this.arrayIndex >= array.length) {
+ throw new Error(`Array index ${this.arrayIndex} is out of bounds for property '${this.property}'`);
+ }
- dependence.value[this.property][this.arrayIndex] = value;
+ array[this.arrayIndex] = value;
this.dirty = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dependence.value[this.property][this.arrayIndex] = value; | |
| setValue(value: any) { | |
| const dependence = this.parent; | |
| if (dependence.dirty) { | |
| dependence.getValue(); | |
| } | |
| const array = dependence.value[this.property]; | |
| if (!Array.isArray(array)) { | |
| throw new Error(`Property '${this.property}' is not an array on the dependence value`); | |
| } | |
| if (this.arrayIndex < 0 || this.arrayIndex >= array.length) { | |
| throw new Error(`Array index ${this.arrayIndex} is out of bounds for property '${this.property}'`); | |
| } | |
| array[this.arrayIndex] = value; | |
| this.dirty = true; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-166: packages/core/src/animation/internal/AnimationPropertyReference.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
| if (dependence.dirty) { | ||
| dependence.getValue(); | ||
| } | ||
| this.value = dependence.value[this.methodName].apply(dependence.value, this.args); |
There was a problem hiding this comment.
Add null check before method invocation to prevent runtime errors
In MethodReference.getValue() at line 117, the code invokes dependence.value[this.methodName].apply(dependence.value, this.args) without verifying if dependence.value[this.methodName] is a valid function. If this.methodName does not correspond to a method on dependence.value, this could lead to a runtime error.
Apply this diff to add a null check before invoking the method:
getValue() {
const dependence = this.parent;
if (dependence.dirty) {
dependence.getValue();
}
+ const method = dependence.value[this.methodName];
+ if (typeof method !== 'function') {
+ throw new Error(`Method ${this.methodName} is not a function on the dependence value`);
+ }
- this.value = dependence.value[this.methodName].apply(dependence.value, this.args);
+ this.value = method.apply(dependence.value, this.args);
this.dirty = false;
return this.value;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.value = dependence.value[this.methodName].apply(dependence.value, this.args); | |
| getValue() { | |
| const dependence = this.parent; | |
| if (dependence.dirty) { | |
| dependence.getValue(); | |
| } | |
| const method = dependence.value[this.methodName]; | |
| if (typeof method !== 'function') { | |
| throw new Error(`Method ${this.methodName} is not a function on the dependence value`); | |
| } | |
| this.value = method.apply(dependence.value, this.args); | |
| this.dirty = false; | |
| return this.value; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 117-119: packages/core/src/animation/internal/AnimationPropertyReference.ts#L117-L119
Added lines #L117 - L119 were not covered by tests
| if (dependence.dirty) { | ||
| dependence.getValue(); | ||
| } | ||
| this.value = dependence.value[this.property][this.arrayIndex]; |
There was a problem hiding this comment.
Add checks for array existence and bounds to prevent runtime errors
In ArrayReference.getValue() at line 155, the code accesses dependence.value[this.property][this.arrayIndex] without verifying if dependence.value[this.property] is an array and if this.arrayIndex is within valid bounds. This could lead to runtime errors if the property is undefined or the index is out of range.
Apply this diff to add checks before accessing the array element:
getValue() {
const dependence = this.parent;
if (dependence.dirty) {
dependence.getValue();
}
+ const array = dependence.value[this.property];
+ if (!Array.isArray(array)) {
+ throw new Error(`Property '${this.property}' is not an array on the dependence value`);
+ }
+ if (this.arrayIndex < 0 || this.arrayIndex >= array.length) {
+ throw new Error(`Array index ${this.arrayIndex} is out of bounds for property '${this.property}'`);
+ }
- this.value = dependence.value[this.property][this.arrayIndex];
+ this.value = array[this.arrayIndex];
this.dirty = false;
return this.value;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.value = dependence.value[this.property][this.arrayIndex]; | |
| getValue() { | |
| const dependence = this.parent; | |
| if (dependence.dirty) { | |
| dependence.getValue(); | |
| } | |
| const array = dependence.value[this.property]; | |
| if (!Array.isArray(array)) { | |
| throw new Error(`Property '${this.property}' is not an array on the dependence value`); | |
| } | |
| if (this.arrayIndex < 0 || this.arrayIndex >= array.length) { | |
| throw new Error(`Array index ${this.arrayIndex} is out of bounds for property '${this.property}'`); | |
| } | |
| this.value = array[this.arrayIndex]; | |
| this.dirty = false; | |
| return this.value; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 155-157: packages/core/src/animation/internal/AnimationPropertyReference.ts#L155-L157
Added lines #L155 - L157 were not covered by tests
| if (this.replaceValueIndex >= 0) { | ||
| args[this.replaceValueIndex] = value; | ||
| } | ||
| dependence.value[this.methodName].apply(dependence.value, args); |
There was a problem hiding this comment.
Add null check before method invocation in setValue to prevent runtime errors
In MethodReference.setValue() at line 131, the code calls dependence.value[this.methodName].apply(dependence.value, args) without checking if dependence.value[this.methodName] is a valid function. This could cause a runtime error if this.methodName does not exist on dependence.value.
Apply this diff to add a null check before invoking the method:
setValue(value: any) {
const dependence = this.parent;
if (dependence.dirty) {
dependence.getValue();
}
const args = this.args;
if (this.replaceValueIndex >= 0) {
args[this.replaceValueIndex] = value;
}
+ const method = dependence.value[this.methodName];
+ if (typeof method !== 'function') {
+ throw new Error(`Method ${this.methodName} is not a function on the dependence value`);
+ }
- dependence.value[this.methodName].apply(dependence.value, args);
+ method.apply(dependence.value, args);
this.dirty = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dependence.value[this.methodName].apply(dependence.value, args); | |
| setValue(value: any) { | |
| const dependence = this.parent; | |
| if (dependence.dirty) { | |
| dependence.getValue(); | |
| } | |
| const args = this.args; | |
| if (this.replaceValueIndex >= 0) { | |
| args[this.replaceValueIndex] = value; | |
| } | |
| const method = dependence.value[this.methodName]; | |
| if (typeof method !== 'function') { | |
| throw new Error(`Method ${this.methodName} is not a function on the dependence value`); | |
| } | |
| method.apply(dependence.value, args); | |
| this.dirty = true; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 131-132: packages/core/src/animation/internal/AnimationPropertyReference.ts#L131-L132
Added lines #L131 - L132 were not covered by tests
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
Bug Fixes
Refactor
Tests