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 skeletal animation sample() does not work as intended #10006

Merged
merged 8 commits into from
Jan 18, 2022

Conversation

shrinktofit
Copy link
Contributor

@shrinktofit shrinktofit commented Jan 17, 2022

Re: cocos-creator/3d-tasks#

Changelog:

  • The mesh renderer should be child of skeletal animation.

@@ -83,19 +83,13 @@ export class SkeletalAnimationState extends AnimationState {
this._frames = frames - 1;
this._animInfo = this._animInfoMgr.getData(root.uuid);
this._bakedDuration = this._frames / samples; // last key
this.setUseBaked(baked);
Copy link
Contributor Author

@shrinktofit shrinktofit Jan 17, 2022

Choose a reason for hiding this comment

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

This ensure the bake option(true vs false) of animation state is up-to-date with skinned mesh renderer, whenever.

}

public onPlay () {
Copy link
Contributor Author

@shrinktofit shrinktofit Jan 17, 2022

Choose a reason for hiding this comment

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

Before this PR: do "sync bake option" on playing.

This PR: Sync at initialize(), sync bake option each time it changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this works for both baked & sampled animations? I think the sampled branch has some init code that still needs to be executed on each play event.

Copy link
Contributor Author

@shrinktofit shrinktofit Jan 18, 2022

Choose a reason for hiding this comment

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

Are you sure this works for both baked & sampled animations? I think the sampled branch has some init code that still needs to be executed on each play event.

The init code of baked branch was moved to setUseBaked, which is invoked at SkeletalAnimationState.initialize() and set SkeletalAnimation.useBakedAnimation(). Is there something I missed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean what about the else branch, under real-time sampled animation mode?

@github-actions
Copy link

github-actions bot commented Jan 17, 2022

Interface Check Report

! WARNING this pull request has changed these public interfaces:

@@ -4117,28 +4117,23 @@
          */
         get skinningRoot(): Node | null;
         set skinningRoot(value: Node | null);
         get model(): __private._cocos_3d_models_skinning_model__SkinningModel | __private._cocos_3d_models_baked_skinning_model__BakedSkinningModel | null;
+        /**
+         * Set associated animation.
+         * @internal This method only friends to skeletal animation component.
+         */
+        associatedAnimation: SkeletalAnimation | null;
         constructor();
+        onLoad(): void;
         onDestroy(): void;
-        __preload(): void;
         uploadAnimation(clip: AnimationClip | null): void;
         /**
          * Set if bake mode should be used.
          * @internal This method only friends to skeletal animation component.
          */
         setUseBakedAnimation(val?: boolean, force?: boolean): void;
         setMaterial(material: Material | null, index: number): void;
-        /**
-         * Notifies this renderer that an animation is usable.
-         * @internal This method only friends to skeletal animation component.
-         */
-        notifyAnimationUsable(animation: SkeletalAnimation): void;
-        /**
-         * Notifies this renderer that the used animation is no longer usable.
-         * @internal This method only friends to skeletal animation component.
-         */
-        notifyAnimationUnusable(): void;
         protected _updateModelParams(): void;
     }
     /**
      * @en The skinned mesh batch renderer component, batches multiple skeleton-sharing [[SkinnedMeshRenderer]].
@@ -4230,9 +4225,12 @@
         protected _parent: SkeletalAnimation | null;
         protected _curvesInited: boolean;
         constructor(clip: AnimationClip, name?: string);
         initialize(root: Node): void;
-        onPlay(): void;
+        /**
+         * @internal This method only friends to `SkeletalAnimation`.
+         */
+        setUseBaked(useBaked: boolean): void;
         rebuildSocketCurves(sockets: Socket[]): void;
     }
     export class Socket {
         /**
@@ -4281,8 +4279,9 @@
         get useBakedAnimation(): boolean;
         set useBakedAnimation(val: boolean);
         protected _useBakedAnimation: boolean;
         protected _sockets: Socket[];
+        onLoad(): void;
         onDestroy(): void;
         start(): void;
         pause(): void;
         resume(): void;
@@ -4290,18 +4289,15 @@
         querySockets(): string[];
         rebuildSocketAnimations(): void;
         createSocket(path: string): Node | null;
         /**
-         * Adds an user which uses this animation.
          * @internal This method only friends to skinned mesh renderer.
          */
-        addUser(user: SkinnedMeshRenderer): void;
+        notifySkinnedMeshAdded(skinnedMeshRenderer: SkinnedMeshRenderer): void;
         /**
-         * Remove specific user of this animation.
-         * The user should receive an "animation useless" notification.
          * @internal This method only friends to skinned mesh renderer.
          */
-        removeUser(user: SkinnedMeshRenderer): void;
+        notifySkinnedMeshRemoved(skinnedMeshRenderer: SkinnedMeshRenderer): void;
         /**
          * Get all users.
          * @internal This method only friends to the skeleton animation state.
          */
@@ -46917,8 +46913,9 @@
         export interface _cocos_3d_skeletal_animation_skeletal_animation_utils__IAnimInfo {
             buffer: gfx.Buffer;
             data: Float32Array;
             dirty: boolean;
+            currentClip: AnimationClip | null;
         }
         export class _cocos_3d_skeletal_animation_skeletal_animation_utils__JointAnimationInfo {
             constructor(device: gfx.Device);
             getData(nodeID?: string): _cocos_3d_skeletal_animation_skeletal_animation_utils__IAnimInfo;

this._sampleCurves = this._sampleCurvesBaked;
this.duration = this._bakedDuration;
this._animInfoMgr.switchClip(this._animInfo!, this.clip);
Copy link
Contributor Author

@shrinktofit shrinktofit Jan 17, 2022

Choose a reason for hiding this comment

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

Moved to below.

@@ -168,8 +162,24 @@ export class SkeletalAnimationState extends AnimationState {
private _sampleCurvesBaked (time: number) {
const ratio = time / this.duration;
const info = this._animInfo!;

// Ensure I'm the one on which the anim info is sampling.
if (!this._animInfoMgr.isCurrentlySampling(info, this.clip)) {
Copy link
Contributor Author

@shrinktofit shrinktofit Jan 17, 2022

Choose a reason for hiding this comment

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

@YunHsiao Please review the main fix here. I'll do the check-switch each time the animation is sampled.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very strange, previously onPlay is only invoked in the beginning, but _sampleCurvesBaked is invoked in every animation frame, it just doesn't seem elegant enough

Copy link
Contributor Author

@shrinktofit shrinktofit Jan 18, 2022

Choose a reason for hiding this comment

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

There are two methods that trigger the sampling process:

  • SkeletalAnimationState.play()/SkeletalAnimationState.resume() - Called once, do sampling every frame

  • SkeletalAnimationState.sample() - Called once, do once sampling immediately.

So I captured the sampling process.

private _tryBindAnimation () {
const { _skinningRoot: skinningRoot } = this;
if (!skinningRoot) {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cocos/3d/skeletal-animation/skeletal-animation-utils.ts Outdated Show resolved Hide resolved
cocos/3d/skeletal-animation/skeletal-animation-utils.ts Outdated Show resolved Hide resolved
@@ -168,8 +162,24 @@ export class SkeletalAnimationState extends AnimationState {
private _sampleCurvesBaked (time: number) {
const ratio = time / this.duration;
const info = this._animInfo!;

// Ensure I'm the one on which the anim info is sampling.
if (!this._animInfoMgr.isCurrentlySampling(info, this.clip)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very strange, previously onPlay is only invoked in the beginning, but _sampleCurvesBaked is invoked in every animation frame, it just doesn't seem elegant enough

@@ -507,7 +508,8 @@ export class JointAnimationInfo {
}

public switchClip (info: IAnimInfo, clip: AnimationClip | null) {
info.data[0] = 0;
info.currentClip = clip;
info.data[0] = -1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pandamicro pandamicro changed the base branch from v3.4.1 to v3.4.2 January 18, 2022 14:55
@pandamicro pandamicro merged commit 19c121d into cocos:v3.4.2 Jan 18, 2022
@shrinktofit shrinktofit deleted the v3.4.1 branch April 25, 2022 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants