Skip to content
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

Fix animate property is not effect when reference changed #2383

Open
wants to merge 2 commits into
base: dev/1.4
Choose a base branch
from

Conversation

luzhuang
Copy link
Contributor

@luzhuang luzhuang commented Sep 20, 2024

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

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

    • Introduced an animation system for material properties and transformations in a 3D engine.
    • Enhanced configurability for animator cases with new configuration entries.
  • Bug Fixes

    • Improved efficiency in animation sampling and property reference management.
  • Refactor

    • Streamlined logic for managing animation property references and curve bindings.
  • Tests

    • Updated tests to encapsulate value retrieval for curve owners, enhancing maintainability.

@luzhuang luzhuang added bug Something isn't working animation Built-in animation system related functions labels Sep 20, 2024
Copy link

coderabbitai bot commented Sep 20, 2024

Walkthrough

This 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

Files Change Summary
e2e/case/animator-referenceAndProperty.ts Introduced an animation system for material properties and mesh transformations, utilizing an Animator component, AnimatorController, and AnimationClip for managing animations.
e2e/config.ts Added a new configuration entry for referenceAndProperty under "Animator" to enhance configurability for animator cases.
packages/core/src/animation/AnimationClip.ts Enhanced animation curve binding management by introducing a temporary reference manager and optimizing curve binding insertion logic for better performance during animation sampling.
packages/core/src/animation/AnimationClipCurveBinding.ts Modified methods to utilize AnimationPropertyReferenceManager for managing temporary curve owners, enhancing flexibility in animation property management.
packages/core/src/animation/Animator.ts Added dependency on AnimationClipCurveBinding and AnimationPropertyReferenceManager, improving the handling of animation curves and property references.
packages/core/src/animation/animationCurve/AnimationFloatArrayCurve.ts Updated initialization logic for Float32Array size determination to enhance encapsulation and consistency in target value retrieval.
packages/core/src/animation/internal/AnimationPropertyReference.ts Introduced an abstract class for managing animation property references, including various concrete subclasses for different reference types, enhancing the structure and efficiency of property management.
packages/core/src/animation/internal/AnimationPropertyReferenceManager.ts Created a new class to manage animation property references efficiently, allowing for structured addition and retrieval of references.
packages/core/src/animation/internal/animationCurveOwner/assembler/UniversalAnimationCurveOwnerAssembler.ts Refactored to simplify property reference management by utilizing AnimationPropertyReferenceManager, reducing complexity in getting and setting animation values.
tests/src/core/Animator.test.ts Modified the test to retrieve the current value of a curve owner using the new encapsulated method, enhancing maintainability of the test code.

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • GuoLei1990

Poem

🐰 In the world of 3D, we hop and play,
With colors that dance and curves that sway.
Animator's magic makes the mesh come alive,
Transforming our scene, watch the visuals thrive!
With new references and properties in tow,
Our animations now sparkle, putting on a show! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@luzhuang luzhuang changed the title fix: animate property is not effect when reference changed Fix animate property is not effect when reference changed Sep 20, 2024
@luzhuang luzhuang added this to the 1.4 milestone Sep 20, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the addReference method to enhance maintainability

Including JSDoc comments for the addReference method will improve code readability and help other developers understand the purpose, parameters, and return value of the method.


78-81: Add JSDoc comments for the clear method

Documenting the clear method with JSDoc comments will enhance clarity regarding its functionality and intent.

packages/core/src/animation/internal/AnimationPropertyReference.ts (1)

3-39: Specify explicit types for value and other any typed properties

The value property and method parameters are currently typed as any. 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 _assembler property visibility is appropriate.

The _assembler property 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 message

The 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

Commits

Files that changed from the base of the PR and between 7ca285a and 1ab4ae3.

Files ignored due to path filters (1)
  • e2e/fixtures/originImage/Animator_animator-referenceAndProperty.jpg is 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 AnimationPropertyReferenceManager is correctly added.


37-38: Skipped commenting due to insufficient context.

The purpose and usage of the _pathLength property are not clear from the provided code changes. More context is needed to provide a meaningful review.


44-58: LGTM!

The changes in the _createCurveOwner and _getTempCurveOwner methods introduce a new dependency on AnimationPropertyReferenceManager. The referenceManager is now passed as an additional parameter to these methods and is used when instantiating AnimationCurveOwner objects.

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 _initializeOwner method, retrieving the target value size using the _assembler.getTargetValue() method of the curveOwner object. The casting to Float32Array ensures type safety.

e2e/config.ts (1)

72-76: LGTM!

The new configuration entry referenceAndProperty follows the existing structure and naming convention of the E2E_CONFIG object. The category and threshold values are consistent with other entries in the Animator category. 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 the referenceTargetValue property 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 animation

Ensure that the rotation values provided correspond to the expected units (degrees or radians) expected by the Transform component. This will prevent any unexpected behavior during the animation.

packages/core/src/animation/internal/AnimationPropertyReference.ts (1)

155-159: LGTM!

The MountedParseFlag enum is well-defined and correctly uses bitwise flags for parsing states.

packages/core/src/animation/internal/animationCurveOwner/AnimationCurveOwner.ts (9)

6-6: Import AnimationPropertyReferenceManager added correctly.

The import statement for AnimationPropertyReferenceManager is necessary for the new reference management functionality and is correctly implemented.


38-38: Addition of referenceManager property enhances reference handling.

Adding the readonly referenceManager: AnimationPropertyReferenceManager; property allows the AnimationCurveOwner to manage animation property references more effectively.


150-150: Correctly saving default value using getTargetValue().

The saveDefaultValue method now retrieves the target value directly from the assembler, ensuring the default value is up-to-date.


158-158: Accurately saving fixed pose value using getTargetValue().

The saveFixedPoseValue method correctly saves the fixed pose value by obtaining it from the assembler, enhancing accuracy during pose transitions.


166-166: Handle potential null in referenceTargetValue.

The referenceTargetValue might be null when cureType._isCopyMode is false. Ensure that all methods receiving this variable handle null values appropriately to prevent runtime errors.

To confirm that methods handling referenceTargetValue are null-safe, review the implementations of the following methods:

  • cureType._additiveValue()
  • cureType._setValue()
  • cureType._lerpValue()

172-172: Verify referenceTargetValue handling in _additiveValue().

Ensure that cureType._additiveValue(value, weight, referenceTargetValue) correctly manages cases where referenceTargetValue may be null, maintaining the integrity of additive animations.


181-181: Confirm referenceTargetValue usage in _setValue().

Check that cureType._setValue(value, referenceTargetValue) can handle a null referenceTargetValue without causing exceptions, ensuring proper value setting in all scenarios.


187-187: Ensure referenceTargetValue is correctly used in _lerpValue().

Validate that cureType._lerpValue(referenceTargetValue, value, weight, referenceTargetValue) functions correctly even if referenceTargetValue is null, to maintain smooth interpolation.


51-59: Constructor signature change may introduce breaking changes.

The constructor of AnimationCurveOwner now requires an additional referenceManager parameter. This modification could break existing code that instantiates AnimationCurveOwner without this parameter. If backward compatibility is a concern, consider providing a default value or an overloaded constructor.

To identify all instantiations of AnimationCurveOwner that 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.ts has been updated to accommodate the new constructor signature of AnimationCurveOwner. The referenceManager parameter 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 _createCurveOwner method passes referenceManager as the first argument to the AnimationCurveOwner constructor.
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.ts

Length of output: 659

packages/core/src/animation/AnimationClip.ts (3)

9-9: Import statement added correctly

The import of AnimationPropertyReferenceManager is properly added, and the path appears to be correct.


199-212: Insertion logic for curveBindings is correct and maintains order

The logic for inserting curveBinding instances based on propertyPathLength ensures that the _curveBindings array 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 resources

While 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 AnimationClip accessing _tempReferenceManager:

Verification successful

To further investigate the potential for concurrent access to the _tempReferenceManager property, 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 _tempReferenceManager property is accessed concurrently or in a multi-threaded environment. The property is used locally within the AnimationClip class, and while there are asynchronous operations involving AnimationClip, they don't appear to introduce concurrent access to this specific property.

Key findings:

  • No usage of AnimationClip in Web Worker contexts was found.
  • Asynchronous operations involving AnimationClip are primarily related to loading and parsing, which are typically one-time operations.
  • No multi-threading libraries or concurrent execution contexts for AnimationClip were 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();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 11 to 13
getReference: AnimationPropertyReference;
setReference: AnimationPropertyReference;
referenceManager: AnimationPropertyReferenceManager;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
getReference: AnimationPropertyReference;
setReference: AnimationPropertyReference;
referenceManager: AnimationPropertyReferenceManager;
private getReference: AnimationPropertyReference;
private setReference: AnimationPropertyReference;
private referenceManager: AnimationPropertyReferenceManager;

Comment on lines 25 to 32
const existedReference = this._referenceIndexMap.get(component);
if (existedReference) {
reference = existedReference;
} else {
reference = new ComponentReference();
reference.manager = this;
reference.value = component;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Comment on lines +73 to +85
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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);
});

Comment on lines +131 to +133
this.property = propertyStr.slice(0, indexPos);
this.arrayIndex = parseInt(propertyStr.slice(indexPos + 1, -1));
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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}`);
}

Comment on lines +17 to +25
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;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
}
}

Comment on lines +427 to +431
const owner = (propertyOwners[property] ||= curveBinding._createCurveOwner(
this._animationPropertyReferenceManager,
targetEntity,
component
));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Comment on lines +16 to +17
private static _tempReferenceManager: AnimationPropertyReferenceManager;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 65.88235% with 58 lines in your changes missing coverage. Please review.

Please upload report for BASE (dev/1.4@7ca285a). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...c/animation/internal/AnimationPropertyReference.ts 51.28% 37 Missing and 1 partial ⚠️
...tion/internal/AnimationPropertyReferenceManager.ts 80.95% 4 Missing and 4 partials ⚠️
...assembler/UniversalAnimationCurveOwnerAssembler.ts 57.14% 3 Missing ⚠️
packages/core/src/animation/AnimationClip.ts 83.33% 2 Missing ⚠️
packages/core/src/animation/Animator.ts 88.88% 1 Missing and 1 partial ⚠️
...imation/animationCurve/AnimationFloatArrayCurve.ts 0.00% 2 Missing ⚠️
...nternal/animationCurveOwner/AnimationCurveOwner.ts 71.42% 2 Missing ⚠️
...es/core/src/animation/AnimationClipCurveBinding.ts 75.00% 1 Missing ⚠️
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           
Flag Coverage Δ
unittests 69.62% <65.88%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1ab4ae3 and d3232d6.

📒 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 tests

packages/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 Efficiency

The refactored implementation using _getReference and _setReference simplifies property management within the UniversalAnimationCurveOwnerAssembler class. By directly utilizing AnimationPropertyReference instances, 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 issue

Inconsistent use of keys in _referenceIndexMap may lead to incorrect reference retrieval

This issue was previously noted and still seems to persist. At line 25, _referenceIndexMap is accessed using instanceId as the key, whereas at line 44, uniqueKey (a combination of instanceId and 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 uniqueKey or instanceId as the key in _referenceIndexMap to ensure reliable reference retrieval and avoid potential conflicts.

Also applies to: 44-45

Comment on lines +47 to +48
reference = existReference;
continue;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 ArrayReference when the property is an array.
  • Line 58: Initializing a MethodReference when 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

Comment on lines +24 to +26
const index = this.invDependencies[i];
const reference = this.manager.animationPropertyReferences[index];
reference.dirty = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
animation Built-in animation system related functions bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants