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 1 commit into
base: dev/1.4
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions e2e/case/animator-referenceAndProperty.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/**
* @title Animation Reference And Property
* @category Animation
*/

import {
AnimationClip,
AnimationColorCurve,
AnimationFloatCurve,
AnimationRefCurve,
Animator,
AnimatorController,
AnimatorControllerLayer,
BlinnPhongMaterial,
Camera,
Color,
DirectLight,
Keyframe,
Logger,
Material,
MeshRenderer,
PBRMaterial,
PrimitiveMesh,
RenderFace,
Transform,
Vector3,
WebGLEngine
} from "@galacean/engine";
import { OrbitControl } from "@galacean/engine-toolkit";
import { initScreenshot, updateForE2E } from "./.mockForE2E";

Logger.enable();
WebGLEngine.create({ canvas: "canvas" }).then((engine) => {
engine.canvas.resizeByClientSize(2);
const scene = engine.sceneManager.activeScene;
const rootEntity = scene.createRootEntity();

// camera
const cameraEntity = rootEntity.createChild("camera_node");
cameraEntity.transform.position = new Vector3(0, 1, 5);
const camera = cameraEntity.addComponent(Camera);
cameraEntity.addComponent(OrbitControl).target = new Vector3(0, 1, 0);

const lightNode = rootEntity.createChild("light_node");
lightNode.addComponent(DirectLight).intensity = 0.6;
lightNode.transform.lookAt(new Vector3(0, 0, 1));
lightNode.transform.rotate(new Vector3(0, 90, 0));

const material = new BlinnPhongMaterial(engine);
material.renderFace = RenderFace.Double;
material.baseColor = new Color(1, 0, 0, 1);

const material2 = new PBRMaterial(engine);
material2.renderFace = RenderFace.Double;
material2.baseColor = new Color(0, 1, 0, 1);

const entity = rootEntity.createChild("mesh");
const { transform } = entity;
transform.setPosition(0, 1, 0);
transform.setRotation(0, 0, 0);
const meshRenderer = entity.addComponent(MeshRenderer);
meshRenderer.mesh = PrimitiveMesh.createCuboid(engine);
meshRenderer.setMaterial(material);

const animator = entity.addComponent(Animator);
const controller = new AnimatorController(engine);
const layer = new AnimatorControllerLayer("base");
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)


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


const colorCurve = new AnimationColorCurve();
const key6 = new Keyframe<Color>();
key6.time = 0;
key6.value = new Color(1, 0, 0, 1);
const key7 = new Keyframe<Color>();
key7.time = 1;
key7.value = new Color(0.5, 0.5, 0.5, 1);
const key8 = new Keyframe<Color>();
key8.time = 2;
key8.value = new Color(1, 0, 0, 1);
colorCurve.addKey(key6);
colorCurve.addKey(key7);
colorCurve.addKey(key8);

const rotateCurve = new AnimationFloatCurve();
const key9 = new Keyframe<number>();
key9.time = 0;
key9.value = 0;
const key10 = new Keyframe<number>();
key10.time = 2;
key10.value = 360;
rotateCurve.addKey(key9);
rotateCurve.addKey(key10);

cubeClip.addCurveBinding("", MeshRenderer, "getMaterial().baseColor", colorCurve);
cubeClip.addCurveBinding("", MeshRenderer, "setMaterial($value)", "getMaterial()", materialCurve);
Copy link

Choose a reason for hiding this comment

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

Verify the correctness of material property binding

The method names setMaterial($value) and getMaterial() used in addCurveBinding might not correctly reference the material property for animation. Please ensure that the property paths align with the API and that the animation affects the material as intended.

Consider updating the binding to directly reference the material property:

- cubeClip.addCurveBinding("", MeshRenderer, "setMaterial($value)", "getMaterial()", materialCurve);
+ cubeClip.addCurveBinding("", MeshRenderer, "material", materialCurve);

Committable suggestion was skipped due to low confidence.

cubeClip.addCurveBinding("", Transform, "rotation.y", rotateCurve);
animator.animatorController = controller;
animator.play("material");
animator.speed = 0.5;

updateForE2E(engine, 130);

initScreenshot(engine, camera);
});
5 changes: 5 additions & 0 deletions e2e/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ export const E2E_CONFIG = {
category: "Animator",
caseFileName: "animator-stateMachine",
threshold: 0.1
},
referenceAndProperty: {
category: "Animator",
caseFileName: "animator-referenceAndProperty",
threshold: 0.1
}
},
GLTF: {
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
26 changes: 24 additions & 2 deletions packages/core/src/animation/AnimationClip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@ import { AnimationClipCurveBinding } from "./AnimationClipCurveBinding";
import { AnimationCurve } from "./animationCurve/AnimationCurve";
import { AnimationEvent } from "./AnimationEvent";
import { AnimationCurveOwner } from "./internal/animationCurveOwner/AnimationCurveOwner";
import { AnimationPropertyReferenceManager } from "./internal/AnimationPropertyReferenceManager";
import { KeyframeValueType } from "./Keyframe";

/**
* Stores keyframe based animations.
*/
export class AnimationClip extends EngineObject {
private static _tempReferenceManager: AnimationPropertyReferenceManager;

Comment on lines +16 to +17
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

/** @internal */
_curveBindings: AnimationClipCurveBinding[] = [];

Expand Down Expand Up @@ -193,7 +196,20 @@ export class AnimationClip extends EngineObject {
}

this._length = Math.max(this._length, curveBinding.curve.length);
this._curveBindings.push(curveBinding);

const curveBindings = this._curveBindings;
const count = curveBindings.length;
const propertyPathLength = curveBinding.property.split(".").length;
curveBinding._pathLength = propertyPathLength;

const maxPropertyPathLength = count ? curveBindings[count - 1]._pathLength : 0;
if (propertyPathLength > maxPropertyPathLength) {
curveBindings.push(curveBinding);
} else {
let index = count;
while (--index >= 0 && propertyPathLength < curveBindings[index]._pathLength);
curveBindings.splice(index + 1, 0, curveBinding);
}
}

/**
Expand All @@ -211,6 +227,12 @@ export class AnimationClip extends EngineObject {
* @param time - The time to sample an animation
*/
_sampleAnimation(entity: Entity, time: number): void {
if (!AnimationClip._tempReferenceManager) {
AnimationClip._tempReferenceManager = new AnimationPropertyReferenceManager();
}

AnimationClip._tempReferenceManager.clear();

const { _curveBindings: curveBindings } = this;
for (let i = curveBindings.length - 1; i >= 0; i--) {
const curve = curveBindings[i];
Expand All @@ -223,7 +245,7 @@ export class AnimationClip extends EngineObject {
if (!component) {
continue;
}
const curveOwner = curve._getTempCurveOwner(targetEntity, component);
const curveOwner = curve._getTempCurveOwner(AnimationClip._tempReferenceManager, targetEntity, component);
if (curveOwner && curve.curve.keys.length) {
const value = curveOwner.evaluateValue(curve.curve, time, false);
curveOwner.applyValue(value, 1, false);
Expand Down
27 changes: 23 additions & 4 deletions packages/core/src/animation/AnimationClipCurveBinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { KeyframeValueType } from "./Keyframe";
import { AnimationCurve } from "./animationCurve";
import { IAnimationCurveCalculator } from "./animationCurve/interfaces/IAnimationCurveCalculator";
import { AnimationCurveLayerOwner } from "./internal/AnimationCurveLayerOwner";
import { AnimationPropertyReferenceManager } from "./internal/AnimationPropertyReferenceManager";
import { AnimationCurveOwner } from "./internal/animationCurveOwner/AnimationCurveOwner";

/**
Expand Down Expand Up @@ -33,14 +34,28 @@ export class AnimationClipCurveBinding {
/** The animation curve. */
curve: AnimationCurve<KeyframeValueType>;

/** @internal */
_pathLength: number;
private _tempCurveOwner: Record<number, AnimationCurveOwner<KeyframeValueType>> = {};

/**
* @internal
*/
_createCurveOwner(entity: Entity, component: Component): AnimationCurveOwner<KeyframeValueType> {
_createCurveOwner(
referenceManager: AnimationPropertyReferenceManager,
entity: Entity,
component: Component
): AnimationCurveOwner<KeyframeValueType> {
const curveType = (<unknown>this.curve.constructor) as IAnimationCurveCalculator<KeyframeValueType>;
const owner = new AnimationCurveOwner(entity, this.type, component, this.property, this.getProperty, curveType);
const owner = new AnimationCurveOwner(
referenceManager,
entity,
this.type,
component,
this.property,
this.getProperty,
curveType
);
curveType._initializeOwner(owner);
owner.saveDefaultValue();
return owner;
Expand All @@ -62,10 +77,14 @@ export class AnimationClipCurveBinding {
/**
* @internal
*/
_getTempCurveOwner(entity: Entity, component: Component): AnimationCurveOwner<KeyframeValueType> {
_getTempCurveOwner(
referenceManager: AnimationPropertyReferenceManager,
entity: Entity,
component: Component
): AnimationCurveOwner<KeyframeValueType> {
const { instanceId } = entity;
if (!this._tempCurveOwner[instanceId]) {
this._tempCurveOwner[instanceId] = this._createCurveOwner(entity, component);
this._tempCurveOwner[instanceId] = this._createCurveOwner(referenceManager, entity, component);
}
return this._tempCurveOwner[instanceId];
}
Expand Down
Loading
Loading