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

Implement "looped" signal for AudioStreamPlayer, 2D and 3D #71088

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vaartis
Copy link
Contributor

@vaartis vaartis commented Jan 8, 2023

Track the number of loops on the player and if it changes, emit a singal.

Closes godotengine/godot-proposals#1641

@vaartis
Copy link
Contributor Author

vaartis commented Jan 10, 2023

Also added the same signal to the animation player, as suggested in the linked issue.

@vaartis vaartis changed the title Implement "looped" signal for AudioStreamPlayer, 2D and 3D Implement "looped" signal for AnimationPlayer and AudioStreamPlayers Jan 10, 2023
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

These codes looks messed up in totality.

It makes no sense to count the number of loops on the signal side. Also, it may cause an implicit overflow. Signal should only notify the fact that a loop has occurred and should be counted on the user script side.

Moreover, the looped signal should fire only when a loop occurs, so should not fire at the same time as finished.

@TokageItLab
Copy link
Member

Ah, I misunderstood a bit. The looped count is something that already exists, and the user determines the upper limit, right? But I think there may be a cleaner way to fire signal.

Also I remember there was some problem with the AnimationPlayer looped flag and I didn't implement it. I'll take a look at it later...

@vaartis
Copy link
Contributor Author

vaartis commented Jan 10, 2023

It makes no sense to count the number of loops on the signal side. Also, it may cause an implicit overflow. Signal should only notify the fact that a loop has occurred and should be counted on the user script side.

The overflow here does not matter, even if it overflows the number still changes and this is what matters to fire the signal. I just used the type that get_loop_count() returns.

Moreover, the looped signal should fire only when a loop occurs, so should not fire at the same time as finished.

It only fires if the stream did not end, hence the else if on the condition that checks if it ended.

Don't know about the AnimationPlayer problem, I only tested the simplest case of making a looping animation and subscribing to the signal.

scene/2d/audio_stream_player_2d.h Outdated Show resolved Hide resolved
scene/animation/animation_player.cpp Outdated Show resolved Hide resolved
@vaartis vaartis changed the title Implement "looped" signal for AnimationPlayer and AudioStreamPlayers Implement "looped" signal for AudioStreamPlayer, 2D and 3D Jan 10, 2023
Emit a signal from a playback whenever a loop occurs, the player
connects to that signal and forwards it.
@reduz
Copy link
Member

reduz commented Jan 19, 2023

I oppose the merging of this PR on the following grounds:

  • Emitting a signal from the audio thread is wrong in many accounts 1) Audio thread must never run complex logic that can allocate memory or run game logic as a result of a signal, because its time (callback) constrained. 2) A signal emitted from the audio thread is useless and dangerous.
  • You can already query the loop count, so all you have to do is check every frame whether the audio has looped.

@TokageItLab
Copy link
Member

This PR originally tried to fire a signal by storing the loop counts of all streams in a map, is this the correct way to do it? Considering the many possibilities of not using the signal, it seemed to me to be expensive.

Perhaps the correct way to do it is to set up a script that monitors only certain Streams when a signal is needed on the user's side.

@vaartis
Copy link
Contributor Author

vaartis commented Jan 27, 2023

Perhaps the correct way to do it is to set up a script that monitors only certain Streams when a signal is needed on the user's side.

Is there a way to check if a signal is needed at all? And if so, how should it track the looping? If emitting a signal on the audio thread is not a good idea, maybe just using the old approach with the dictionary is fine, as long as I can somehow know if I need to track only certain streams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a signal to AudioStreamPlayer that indicates that the loop has started again
4 participants