godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.13k stars 21.19k forks source link

`AnimationPlayer.advance()` doesn't advance audio #48982

Open Jayman2000 opened 3 years ago

Jayman2000 commented 3 years ago

Godot version:

v3.3.1.stable.official

OS/device including version:

OS: Arch Linux (I don't know how to provide a version for this)

Issue description:

I have an AnimationPlayer with multiple tracks. One of them is an audio playback track. When I call the advance() method, I expect the audio to jump ahead. It doesn't do that.

Workaround Call AudioStreamPlayer.seek() after calling AnimationPlayer.advance().

Steps to reproduce:

  1. Create an AnimationPlayer.
  2. Give it an animation named "example".
  3. Add an audio playback track to that animation.
  4. Add a 2 minute long sound clip to that audio playback track.
  5. Make sure that the animation is at least 2 minutes long.
  6. Attach the following script to the AnimationPlayer:
    
    extends AnimationPlayer

func _ready(): play("example") advance(60)

# Workaround:
#$AudioStreamPlayer.seek(60)

7. Run the scene.

**Minimal reproduction project:**
<!-- A small Godot project which reproduces the issue. Drag and drop a zip archive to upload it. -->
[animation_player_does_not_advance_audio.zip](https://github.com/godotengine/godot/files/6527228/animation_player_does_not_advance_audio.zip)
alessandrofama commented 3 years ago

Very new to Godot's source but trying to get started and sharing my observations. I think the relevant code (master) is here:

https://github.com/godotengine/godot/blob/db028ac7001cca42c9f61cc231e2363ae884e70a/scene/animation/animation_player.cpp#L611-L712

I noticed that when you call AnimationPlayer.seek() it works correctly (p_seeked will be true) and it starts playing the audio again at the right start_ofs. When calling AnimationPlayer.advance(), p_seeked is false and it runs the code inside the else statement below. The size of the to_play list is 0 and so it skips that part and checks if there no audio at the position you shifted to and stops the stream in that case (I guess).

If you put another audio file on the same track and call advance(130) (example) it will play the second stream:

grafik

The size of the to_play list will be 1 in this case, so it doesn't skip this code inside the else statement.

I noticed that the value of p_delta is like 0.016666667535901070 (this is from Node::get_process_delta_time ( ) ?) when randomly setting a breakpoint, but will be set to the delta argument of AnimationPlayer.advance() when that function is called.

As a test, by doing something like

if (p_seeked || p_delta > 0.1) {
//find whatever should be playing
[...]

it will correctly jump to the right audio position, as p_delta > 0.1 will only be true when you call AnimationPlayer.advance(delta) with delta > 0.1, but p_delta will be smaller all the other times. But that does not seem like a clean solution to me.

It feels to me AnimationPlayer.advance() should behave as AnimationPlayer.seek() for audio, but the only value that differs in the code to check if advance() was called seems to be p_delta. Not sure what the optimal solution to this problem would be right now. In any case looking forward to read and understand more of the AnimationPlayer code :-)

alessandrofama commented 3 years ago

One thing I've tried to do is adding a advanced bool to the Playback struct:

    struct Playback {
        List<Blend> blend;
        PlaybackData current;
        StringName assigned;
        bool seeked = false;
        bool started = false;
        bool advanced = false;
    } playback;

and setting it to true when calling AnimationPlayer::advance():

void AnimationPlayer::advance(float p_time) {
    playback.advanced = true;
    _animation_process(p_time);
}

then passing it to AnimationPlayer::_animation_process_data() in AnimationPlayer::_animation_process2():

_animation_process_data(c.current, p_delta, 1.0f, c.seeked && p_delta != 0, p_started, c.advanced);

and consequently also to AnimationPlayer::_animation_process_animation():

_animation_process_animation(cd.from, cd.pos, delta, p_blend, &cd == &playback.current, p_seeked, p_started, p_advanced);

In the if statement inside the Animation::TYPE_AUDIO switch case, where it is checking if it seeked, then also checking if it advanced:

if (p_seeked || p_advanced) {
    //find whatever should be playing
 [...]

That's similar to how Playback.seeked is being set in AnimationPlayer::seek(). Works fine and solves the issue, but not sure if it is the right approach, since introducing Playback.advanced would only be useful for this particular audio problem. Testing it here: Changes, branch.