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.

Sign up for GitHub

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 animation time error because of speed and animatorParameter rename bug #2273

Merged
merged 9 commits into from
Jul 29, 2024
Merged

Fix animation time error because of speed and animatorParameter rename bug #2273

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ad70775
fix: animation time error because of speed and precision
luzhuang Jul 28, 2024
3435cdf
feat: opt code
luzhuang Jul 29, 2024
e40be2d
feat: opt code
luzhuang Jul 29, 2024
e6a170e
feat: opt code
luzhuang Jul 29, 2024
6731c21
feat: opt code
luzhuang Jul 29, 2024
692b6af
feat: opt code
luzhuang Jul 29, 2024
dc9a18a
feat: opt code
luzhuang Jul 29, 2024
50fe5e9
feat: opt code
luzhuang Jul 29, 2024
6729113
feat: opt code
luzhuang Jul 29, 2024
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
138 changes: 63 additions & 75 deletions packages/core/src/animation/Animator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { AnimatorLayerData } from "./internal/AnimatorLayerData";
import { AnimatorStateData } from "./internal/AnimatorStateData";
import { AnimatorStatePlayData } from "./internal/AnimatorStatePlayData";
import { AnimationCurveOwner } from "./internal/animationCurveOwner/AnimationCurveOwner";
import { MathUtil } from "@galacean/engine-math";

/**
* The controller of the animation system.
Expand Down Expand Up @@ -536,15 +537,15 @@ export class Animator extends Component {
const { srcPlayData } = layerData;
const { state } = srcPlayData;

const actualSpeed = state.speed * this.speed;
const actualDeltaTime = actualSpeed * deltaTime;
const playSpeed = state.speed * this.speed;
const playDeltaTime = playSpeed * deltaTime;

srcPlayData.updateOrientation(actualDeltaTime);
srcPlayData.updateOrientation(playDeltaTime);

const { clipTime: lastClipTime, playState: lastPlayState } = srcPlayData;

// Precalculate to get the transition
srcPlayData.update(actualDeltaTime);
srcPlayData.update(playDeltaTime);

const { clipTime, isForwards } = srcPlayData;
const transition =
Expand All @@ -565,51 +566,47 @@ export class Animator extends Component {
state.transitions,
lastClipTime,
clipTime,
actualDeltaTime,
playDeltaTime,
aniUpdate
);

let costTime: number;
let playCostTime: number;
if (transition) {
const clipDuration = state.clip.length;
const clipEndTime = state.clipEndTime * clipDuration;
const exitTime = transition.exitTime * state._getDuration();

if (isForwards) {
if (exitTime < lastClipTime) {
costTime = exitTime + clipEndTime - lastClipTime;
playCostTime = exitTime + clipEndTime - lastClipTime;
} else {
costTime = exitTime - lastClipTime;
playCostTime = exitTime - lastClipTime;
}
} else {
const startTime = state.clipStartTime * clipDuration;
if (lastClipTime < exitTime) {
costTime = clipEndTime - exitTime + lastClipTime - startTime;
playCostTime = clipEndTime - exitTime + lastClipTime - startTime;
} else {
costTime = lastClipTime - exitTime;
playCostTime = lastClipTime - exitTime;
}
costTime = -costTime;
playCostTime = -playCostTime;
}
// Revert actualDeltaTime and update costTime
srcPlayData.update(costTime - actualDeltaTime);
// Revert actualDeltaTime and update playCostTime
srcPlayData.update(playCostTime - playDeltaTime);
} else {
costTime = actualDeltaTime;
}

const needSwitchLayerState = !!transition;
const layerFinished = !needSwitchLayerState && srcPlayData.playState === AnimatorStatePlayState.Finished;
if (layerFinished) {
layerData.layerState = LayerState.Finished;
playCostTime = playDeltaTime;
if (srcPlayData.playState === AnimatorStatePlayState.Finished) {
layerData.layerState = LayerState.Finished;
}
}

costTime = Math.abs(costTime);

this._evaluatePlayingState(srcPlayData, weight, additive, aniUpdate);
this._fireAnimationEventsAndCallScripts(layerIndex, srcPlayData, state, lastClipTime, lastPlayState, costTime);
this._fireAnimationEventsAndCallScripts(layerIndex, srcPlayData, state, lastClipTime, lastPlayState, playCostTime);

if (needSwitchLayerState) {
const remainDeltaTime = deltaTime - costTime;
this._updateState(layerIndex, layerData, layer, remainDeltaTime, aniUpdate);
if (transition) {
// actualCostTime = playCostTime / playSpeed
Copy link
Member

Choose a reason for hiding this comment

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

// Remove speed factor, use actual time

const remainDeltaTime = deltaTime - playCostTime / playSpeed;
remainDeltaTime > 0 && this._updateState(layerIndex, layerData, layer, remainDeltaTime, aniUpdate);
}
}

Expand Down Expand Up @@ -660,51 +657,51 @@ export class Animator extends Component {
const destStateDuration = destState._getDuration();
const transitionDuration = destStateDuration * layerData.crossFadeTransition.duration;

const actualSrcSpeed = srcState.speed * speed;
const actualDestSpeed = destState.speed * speed;
const actualDestDeltaTime = actualDestSpeed * deltaTime;
const srcPlaySpeed = srcState.speed * speed;
const dstPlaySpeed = destState.speed * speed;
const dstPlayDeltaTime = dstPlaySpeed * deltaTime;

srcPlayData && srcPlayData.updateOrientation(actualSrcSpeed * deltaTime);
destPlayData && destPlayData.updateOrientation(actualDestDeltaTime);
srcPlayData && srcPlayData.updateOrientation(srcPlaySpeed * deltaTime);
destPlayData && destPlayData.updateOrientation(dstPlayDeltaTime);
Comment on lines +664 to +665
Copy link

Choose a reason for hiding this comment

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

Use optional chaining for safety.

The static analysis tool suggests using optional chaining for better safety.

- srcPlayData && srcPlayData.updateOrientation(srcPlaySpeed * deltaTime);
- destPlayData && destPlayData.updateOrientation(dstPlayDeltaTime);
+ srcPlayData?.updateOrientation(srcPlaySpeed * deltaTime);
+ destPlayData?.updateOrientation(dstPlayDeltaTime);
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
srcPlayData && srcPlayData.updateOrientation(srcPlaySpeed * deltaTime);
destPlayData && destPlayData.updateOrientation(dstPlayDeltaTime);
srcPlayData?.updateOrientation(srcPlaySpeed * deltaTime);
destPlayData?.updateOrientation(dstPlayDeltaTime);
Tools
Biome

[error] 667-667: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 668-668: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

Comment on lines +660 to +665
Copy link

Choose a reason for hiding this comment

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

Use optional chaining for safety.

The static analysis tool suggests using optional chaining for better safety.

- srcPlayData && srcPlayData.updateOrientation(srcPlaySpeed * deltaTime);
- destPlayData && destPlayData.updateOrientation(dstPlayDeltaTime);
+ srcPlayData?.updateOrientation(srcPlaySpeed * deltaTime);
+ destPlayData?.updateOrientation(dstPlayDeltaTime);
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 srcPlaySpeed = srcState.speed * speed;
const dstPlaySpeed = destState.speed * speed;
const dstPlayDeltaTime = dstPlaySpeed * deltaTime;
srcPlayData && srcPlayData.updateOrientation(actualSrcSpeed * deltaTime);
destPlayData && destPlayData.updateOrientation(actualDestDeltaTime);
srcPlayData && srcPlayData.updateOrientation(srcPlaySpeed * deltaTime);
destPlayData && destPlayData.updateOrientation(dstPlayDeltaTime);
const srcPlaySpeed = srcState.speed * speed;
const dstPlaySpeed = destState.speed * speed;
const dstPlayDeltaTime = dstPlaySpeed * deltaTime;
srcPlayData?.updateOrientation(srcPlaySpeed * deltaTime);
destPlayData?.updateOrientation(dstPlayDeltaTime);
Tools
Biome

[error] 664-664: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 665-665: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


const { clipTime: lastSrcClipTime, playState: lastSrcPlayState } = srcPlayData;
const { clipTime: lastDestClipTime, playState: lastDstPlayState } = destPlayData;

let destCostTime: number;
let dstPlayCostTime: number;
if (destPlayData.isForwards) {
destCostTime =
lastDestClipTime + actualDestDeltaTime > transitionDuration
dstPlayCostTime =
lastDestClipTime + dstPlayDeltaTime > transitionDuration
? transitionDuration - lastDestClipTime
: actualDestDeltaTime;
: dstPlayDeltaTime;
} else {
// The time that has been played
const playedTime = destStateDuration - lastDestClipTime;
destCostTime =
dstPlayCostTime =
// -actualDestDeltaTime: The time that will be played, negative are meant to make ite be a periods
// > transition: The time that will be played is enough to finish the transition
playedTime - actualDestDeltaTime > transitionDuration
playedTime - dstPlayDeltaTime > transitionDuration
? // Negative number is used to convert a time period into a reverse deltaTime.
// -(transitionDuration - playedTime)
playedTime - transitionDuration
: actualDestDeltaTime;
: dstPlayDeltaTime;
}

const costTime = actualDestSpeed === 0 ? 0 : destCostTime / actualDestSpeed;
const srcCostTime = costTime * actualSrcSpeed;
const actualCostTime = dstPlaySpeed === 0 ? 0 : dstPlayCostTime / dstPlaySpeed;
const srcPlayCostTime = actualCostTime * srcPlaySpeed;

srcPlayData.update(srcCostTime);
destPlayData.update(destCostTime);
srcPlayData.update(srcPlayCostTime);
destPlayData.update(dstPlayCostTime);

let crossWeight = Math.abs(destPlayData.frameTime) / transitionDuration;
(crossWeight >= 1.0 || transitionDuration === 0) && (crossWeight = 1.0);
(crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) && (crossWeight = 1.0);
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.

The use of assignments within expressions can be confusing. Refactor the code for better readability.

- (crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) && (crossWeight = 1.0);
+ if (crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) {
+   crossWeight = 1.0;
+ }
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
(crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) && (crossWeight = 1.0);
if (crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) {
crossWeight = 1.0;
}
Tools
Biome

[error] 696-696: 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 crossFadeFinished = crossWeight === 1.0;

if (crossFadeFinished) {
this._preparePlayOwner(layerData, destState);
this._evaluatePlayingState(destPlayData, weight, additive, aniUpdate);
} else {
this._evaluateCrossFadeState(layerData, srcPlayData, destPlayData, weight, additive, aniUpdate);
this._evaluateCrossFadeState(layerData, srcPlayData, destPlayData, weight, crossWeight, additive, aniUpdate);
}

this._fireAnimationEventsAndCallScripts(
Expand All @@ -713,7 +710,7 @@ export class Animator extends Component {
srcState,
lastSrcClipTime,
lastSrcPlayState,
Math.abs(srcCostTime)
srcPlayCostTime
);

this._fireAnimationEventsAndCallScripts(
Expand All @@ -722,12 +719,12 @@ export class Animator extends Component {
destState,
lastDestClipTime,
lastDstPlayState,
Math.abs(destCostTime)
dstPlayCostTime
);

if (crossFadeFinished) {
this._updateCrossFadeData(layerData);
const remainDeltaTime = deltaTime - costTime;
const remainDeltaTime = deltaTime - actualCostTime;
remainDeltaTime > 0 && this._updateState(layerIndex, layerData, layer, remainDeltaTime, aniUpdate);
}
}
Expand All @@ -737,6 +734,7 @@ export class Animator extends Component {
srcPlayData: AnimatorStatePlayData,
destPlayData: AnimatorStatePlayData,
weight: number,
crossWeight: number,
additive: boolean,
aniUpdate: boolean
) {
Expand All @@ -745,10 +743,6 @@ export class Animator extends Component {
const { state: destState } = destPlayData;
const { _curveBindings: destCurves } = destState.clip;

const transitionDuration = destState._getDuration() * layerData.crossFadeTransition.duration;
let crossWeight = Math.abs(destPlayData.frameTime) / transitionDuration;
(crossWeight >= 1.0 || transitionDuration === 0) && (crossWeight = 1.0);

const finished = destPlayData.playState === AnimatorStatePlayState.Finished;

if (aniUpdate || finished) {
Expand Down Expand Up @@ -792,46 +786,44 @@ export class Animator extends Component {
const stateDuration = state._getDuration();
const transitionDuration = stateDuration * layerData.crossFadeTransition.duration;

const actualSpeed = state.speed * this.speed;
const actualDeltaTime = actualSpeed * deltaTime;
const playSpeed = state.speed * this.speed;
const playDeltaTime = playSpeed * deltaTime;

destPlayData.updateOrientation(actualDeltaTime);
destPlayData.updateOrientation(playDeltaTime);

const { clipTime: lastDestClipTime, playState: lastPlayState } = destPlayData;

let destCostTime: number;
let dstPlayCostTime: number;
if (destPlayData.isForwards) {
destCostTime =
lastDestClipTime + actualDeltaTime > transitionDuration
? transitionDuration - lastDestClipTime
: actualDeltaTime;
dstPlayCostTime =
lastDestClipTime + playDeltaTime > transitionDuration ? transitionDuration - lastDestClipTime : playDeltaTime;
} else {
// The time that has been played
const playedTime = stateDuration - lastDestClipTime;
destCostTime =
dstPlayCostTime =
// -actualDestDeltaTime: The time that will be played, negative are meant to make ite be a periods
// > transition: The time that will be played is enough to finish the transition
playedTime - actualDeltaTime > transitionDuration
playedTime - playDeltaTime > transitionDuration
? // Negative number is used to convert a time period into a reverse deltaTime.
// -(transitionDuration - playedTime)
playedTime - transitionDuration
: actualDeltaTime;
: playDeltaTime;
}

const costTime = actualSpeed === 0 ? 0 : destCostTime / actualSpeed;
const actualCostTime = playSpeed === 0 ? 0 : dstPlayCostTime / playSpeed;

destPlayData.update(destCostTime);
destPlayData.update(dstPlayCostTime);

let crossWeight = Math.abs(destPlayData.frameTime) / transitionDuration;
(crossWeight >= 1.0 || transitionDuration === 0) && (crossWeight = 1.0);
(crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) && (crossWeight = 1.0);
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.

The use of assignments within expressions can be confusing. Refactor the code for better readability.

- (crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) && (crossWeight = 1.0);
+ if (crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) {
+   crossWeight = 1.0;
+ }
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
(crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) && (crossWeight = 1.0);
if (crossWeight >= 1.0 - MathUtil.zeroTolerance || transitionDuration === 0) {
crossWeight = 1.0;
}
Tools
Biome

[error] 818-818: 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 crossFadeFinished = crossWeight === 1.0;

if (crossFadeFinished) {
this._preparePlayOwner(layerData, state);
this._evaluatePlayingState(destPlayData, weight, additive, aniUpdate);
} else {
this._evaluateCrossFadeFromPoseState(layerData, destPlayData, weight, additive, aniUpdate);
this._evaluateCrossFadeFromPoseState(layerData, destPlayData, weight, crossWeight, additive, aniUpdate);
}

this._fireAnimationEventsAndCallScripts(
Expand All @@ -840,12 +832,12 @@ export class Animator extends Component {
state,
lastDestClipTime,
lastPlayState,
Math.abs(destCostTime)
dstPlayCostTime
);

if (crossFadeFinished) {
this._updateCrossFadeData(layerData);
const remainDeltaTime = deltaTime - costTime;
const remainDeltaTime = deltaTime - actualCostTime;
remainDeltaTime > 0 && this._updateState(layerIndex, layerData, layer, remainDeltaTime, aniUpdate);
}
}
Expand All @@ -854,17 +846,14 @@ export class Animator extends Component {
layerData: AnimatorLayerData,
destPlayData: AnimatorStatePlayData,
weight: number,
crossWeight: number,
additive: boolean,
aniUpdate: boolean
) {
const { crossLayerOwnerCollection } = layerData;
const { state } = destPlayData;
const { _curveBindings: curveBindings } = state.clip;

const duration = state._getDuration() * layerData.crossFadeTransition.duration;
let crossWeight = Math.abs(destPlayData.frameTime) / duration;
(crossWeight >= 1.0 || duration === 0) && (crossWeight = 1.0);

const { clipTime: destClipTime, playState } = destPlayData;
const finished = playState === AnimatorStatePlayState.Finished;

Expand Down Expand Up @@ -1321,18 +1310,17 @@ export class Animator extends Component {
const clipDuration = state.clip.length;
const startTime = state.clipStartTime * clipDuration;
const endTime = state.clipEndTime * clipDuration;
const actualDeltaTime = this.speed * state.speed * deltaTime;

if (isForwards) {
if (lastClipTime + actualDeltaTime >= endTime) {
if (lastClipTime + deltaTime >= endTime) {
this._fireSubAnimationEvents(playData, eventHandlers, lastClipTime, state.clipEndTime * clipDuration);
playData.currentEventIndex = 0;
this._fireSubAnimationEvents(playData, eventHandlers, state.clipStartTime * clipDuration, clipTime);
} else {
this._fireSubAnimationEvents(playData, eventHandlers, lastClipTime, clipTime);
}
} else {
if (lastClipTime + actualDeltaTime <= startTime) {
if (lastClipTime + deltaTime <= startTime) {
this._fireBackwardSubAnimationEvents(playData, eventHandlers, lastClipTime, state.clipStartTime * clipDuration);
playData.currentEventIndex = eventHandlers.length - 1;
this._fireBackwardSubAnimationEvents(playData, eventHandlers, state.clipEndTime * clipDuration, clipTime);
Expand Down
34 changes: 16 additions & 18 deletions packages/core/src/animation/AnimatorController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,31 +54,29 @@ export class AnimatorController extends ReferResource {
* @param name - The name of the parameter
* @param defaultValue - The default value of the parameter
*/
addParameter(name: string, defaultValue?: AnimatorControllerParameterValue): AnimatorControllerParameter;

/**
* Add a parameter to the controller.
* @param parameter - The parameter
*/
addParameter(parameter: AnimatorControllerParameter): AnimatorControllerParameter;

addParameter(param: AnimatorControllerParameter | string, defaultValue?: AnimatorControllerParameterValue) {
if (typeof param === "string") {
const name = param;
param = new AnimatorControllerParameter();
param.name = name;
param.defaultValue = defaultValue;
addParameter(name: string, defaultValue?: AnimatorControllerParameterValue) {
if (this._parametersMap[name]) {
console.warn(`Parameter ${name} already exists.`);
return null;
}
this._parametersMap[param.name] = param;
const param = new AnimatorControllerParameter();
param.name = name;
param.defaultValue = defaultValue;
param._onNameChanged = (oldName, newName) => {
delete this._parametersMap[oldName];
this._parametersMap[newName] = param as AnimatorControllerParameter;
};
this._parametersMap[name] = param;
this._parameters.push(param);
return param;
}

/**
* Remove a parameter from the controller.
* @param parameter - The parameter
* Remove a parameter from the controller by name.
* @param name - The parameter name
*/
removeParameter(parameter: AnimatorControllerParameter) {
removeParameter(name: string) {
const parameter = this._parametersMap[name];
const index = this._parameters.indexOf(parameter);
if (index !== -1) {
this._parameters.splice(index, 1);
Expand Down
Loading
Loading