Reset animation on playback stop

[KoBeWi]

Fixes #27499

[fire]

@akien-mga do you see any danger in merging this?

[akien-mga]

do you see any danger in merging this?

Yes, two things I'm not sure about:

• seek(0, true) calls _animation_process(0), which does a whole lot of update logic. I'm not sure if it's good per se to have so much logic running when the user just wants to stop the animation from playing.
• More importantly, I think there are two conflicting use cases and we can't support both. Currently stop(true) will reset the position so that the next play() starts from 0, but it will not change the displayed frame back to frame 0. It's actually wanted in various situations if you want to stop the animation while staying on the stop frame, and when you play() again it should restart from 0. If we merge this PR, you'd have to pause the animation, seek to 0, and then play.

[aaronfranke]

@KoBeWi Is this still desired? If so, it needs to be rebased on the latest master branch.

[akien-mga]

Needs a rebase to fix CI. But note that this is potentially superseded by #44345, we need a design decision on how we want the API to be like for animation playback (both AnimationPlayer and AnimatedSprite).

From what I remember #44345 was discussed in a PR review meeting and had mostly consensus, so we might want to re-review that and possibly close this PR while merging #44345.

This PR might still be relevant as a simpler fix for 3.2, but I feel like any change to this API will be perceived by some as a regression, so we need to make sure we can stand by this change :)

Dismissing review because, although at first this looked good to me, I realized it's not mature yet.

[golddotasksquestions]

More importantly, I think there are two conflicting use cases and we can't support both. Currently stop(true) will reset the position so that the next play() starts from 0, but it will not change the displayed frame back to frame 0. It's actually wanted in various situations if you want to stop the animation while staying on the stop frame, and when you play() again it should restart from 0. If we merge this PR, you'd have to pause the animation, seek to 0, and then play.

@akien-mga , no, this is never wanted. This is the dirtiest behaviour I can imagine. You are mixing to very different commands here: Pause and Stop. Pause, pauses (timestamp stays where it is) stop stops playback (resets timestamp).
Godot is the only thing in existence that does this weird mixed behaviour! Why, really?
If a user wants to pause the playback, we need a .pause() for them. If they want to stop playback (It means reset timestamp to 0), they should get a .stop(). Stop is not Pause and Pause is not stop. Don't confuse people with what could be a "true" or a "false" stop. This is completely unnecessary. There are already two very well know icons, behaviour and expectations in place: ⏹ ⏸
This is the behaviour is the same in literally ever music, video or animation playing or editing device (analog or digital) I ever got my sticky sausage fingers on since early childhood. Please let's not come up with totally unique novel solutions for a worldwide, incredibly well established and solved UI solution.

[TokageItLab]

I assume that something like _internal_seek() is needed, since there is a risk that seek(0, true) may playback the method track, audio track, etc. at frame 0.

So, it should be ensured that only certain types (Position3D/Rotation3D/Scale3D/BlendShape/Value(except Discrete)/Bezier) of tracks are updated when reset.

[KoBeWi]

Rebased. The animation process code is so complex that I couldn't think of how to reasonably implement "internal seeking", so I just added a bool called is_stopping. Hope it's acceptable.

[TokageItLab]

Playback avoidance is needed not only in TYPE_METHOD but also in TYPE_AUDIO and TYPE_ANIMATION. So, it is better to do if (is_stopping) { continue; } at the beginning of those case statements rather than propagate it to can_call.

[KoBeWi]

I'm already stopping TYPE_AUDIO, forgot about TYPE_ANIMATION.

[TokageItLab]

The reset option should be propagated to the SubAnimationPlayer of the AnimationPlaybackTrack.

void AnimationPlayer::_stop_playing_caches() {
	for (TrackNodeCache *E : playing_caches) {
		if (E->node && E->audio_playing) {
			E->node->call(SNAME("stop"));
		}
		if (E->node && E->animation_playing) {
			AnimationPlayer *player = Object::cast_to<AnimationPlayer>(E->node);
			if (!player) {
				continue;
			}
			player->stop();
		}
	}

	playing_caches.clear();
}

So, it should be fixed to like this:

void AnimationPlayer::_stop_playing_caches(bool p_reset) {
~~
			if (p_reset) {
				player->stop();
			} else {
				player->pause();
			}
~~
}

[TokageItLab]

Tested, looks fine except for the above issue. I sent a PR to apply the pause() and stop() separately to AnimationPlayerEditor as #71321. I will rebase that PR as soon as this PR is merged first.

This is a note to myself: considering this fix, I will need to divide the type of seek into three types in godotengine/godot-proposals#5972.

[TokageItLab]

LGTM. I agree with this change, also it is consistent with tween.

[akien-mga]

Thanks!

[LakshayaG73]

Hi. Does this fix imply that property values tracks will have their 0 time values reapplied when an animation is stopped?

How does it make sense to update tracks when the animation player is 'stopped'?

How is the end user supposed to guess which tracks will be updated when we 'stop'?

What is being 'fixed' here? For a prettier editor scenario, you are breaking user scenarios in code.

stop() is used extensively in code, reliant on the behavior that the animation player has stopped updating.

stop() should simply NOT cause any updates. conceptually, how can it?