Add beat and bar getters to AudioStreamPlayer[2D/3D]

[lemilonkh]

Implements godotengine/godot-proposals#6166.

Video demo

BeatTools.mp4

API

It adds the following functions and signals:

• AudioStreamPlayer.get_current_beat() -> int
• AudioStreamPlayer.get_current_bar() -> int
• AudioStreamPlayer.get_beat_progress() -> float in range [0,1]
• AudioStreamPlayer.get_bar_progress() -> float in range [0,1]
  - Signal AudioStreamPlayback.beat_changed(beat: int)
  - Signal AudioStreamPlayback.bar_changed(bar: int)

Edit: Signals were removed, see below.

This allows syncing gameplay or visual effects to the BPM (beats per minute) of music.
It is implemented for MP3 and OGG Vorbis streams (since those are the only ones that have BPM information attached, although it could be implemented for WAV if desired and the bpm/ bar_beats fields are added to that).
This works for AudioStreamPlayer, AudioStreamPlayer2D and AudioStreamPlayer3D.

Test project

BeatToolsTestProject_v2.zip

Update: New version that uses the signals on AudioStreamPlayer and adds tweens for label colors when the signals are triggered to demonstrate their usage.

BeatToolsTestProject_v3.zip

This project contains two music files in ogg and mp3 formats with different BPM (to test both implementations and show that it syncs correctly to different speeds).

The contained music files are CC0 licensed and can be found here.

If desired, I can polish this project a bit and add it to the godot-demo-projects repository/ Godot Asset Library, so that we can show a working reference implementation for the BPM features.

Outstanding work

Ideally the events from AudioStreamPlayback would be exposed on AudioStreamPlayer as well (I have defined them there too, but haven't found a good way to re-emit a signal emitted by AudioStreamPlayback from AudioStreamPlayer, since AudioStreamPlayback doesn't have a reference to the player and the player can have multiple playback instances. If someone has an idea how this signal could be exposed, please let me know and I'll adjust the implementation. Otherwise I will remove the signal definitions from AudioStreamPlayer[2D/3D].

Another option would be to emit the signals from the AudioStream instead, which the AudioStreamPlayback has a reference to and could call a method on. This is slightly less elegant to work with in GDScript than signals emitted on the AudioStreamPlayer, since the stream might be changed and then the signals need to be reconnected from code. It also means that you can't connect the signal from the editor UI on the node as usual. It's much nicer than having to connect to signals on the AudioStreamPlayback object though, since that changes often (e.g. when seeking it seems) and isn't available until you play the stream (see the test project for this being tricky to work around).

Another issue is that the signals are emitted from the audio thread, so you have to use call_deferred or set_deferred to interact with UI or other nodes from a function connected to one of the signals (as seen in the example code). Is there a way to emit the signal on the main thread instead?

Update: The signals are now emitted from AudioStreamPlayer[2D/3D] directly, so the GDScript interface is much nicer to use now. Thanks for the feedback and help in getting this to work! ✨

[MJacred]

And in all three AudioStreamPlayer* classes, you need to reset prev_beat and prev_bar inside play() and stop()

Alternatively to the if (current_bar >= 0) suggestion, you can cache a variant of beat_loop (temp variable calculated in _mix_internal in ogg vorbis and mp3), just without the has_loop-check, and expose it to the AudioStreamPlayer* classes (and cache it there).

[lemilonkh]

Thanks for the reviews!
I implemented the feedback.
The only thing I'm not 100% sure about is resetting prev_beat and prev_bar in start() and stop() yet...
This means that if you "pause" an audio player by storing the current time (progress) in a variable and then pass that to play(), it would trigger a new bar and beat immediately when you start playing (even though you might have paused it in the middle of a bar). If we keep the current state for these variables, it would only emit the events when the next beat or bar is reached, which seems more correct.

This is of course an edge case, but pausing music is relatively common when you e.g. go into a pause menu or inventory screen in the middle of the game, so it's worth considering imo.
It can be worked around by storing the current beat or bar in GDScript and comparing that to the event value, so not sure which behaviour is preferable.

[MJacred]

You're right, you need to reset them in

https://github.com/godotengine/godot/blob/5a895b71028d257e55d35b2368142010c022dc4a/scene/2d/audio_stream_player_2d.cpp#L95

And the very same if-clause in AudioStreamPlayer3D and AudioStreamPlayer.

Plus in case NOTIFICATION_PREDELETE:

https://github.com/godotengine/godot/blob/5a895b71028d257e55d35b2368142010c022dc4a/scene/audio/audio_stream_player.cpp#L92C14-L92C14

And you must still reset it in stop() because of stream_playbacks.clear(); -> this prevents the code referenced above to be executed.

This way pausing should be no issue.

[MJacred]

And the very same if-clause in AudioStreamPlayer3D and AudioStreamPlayer.

It seems you overlooked that:

https://github.com/godotengine/godot/blob/c0b189f02fe88d5a5f34ec0176d9bab19abefb0e/scene/3d/audio_stream_player_3d.cpp#L306

https://github.com/godotengine/godot/blob/c0b189f02fe88d5a5f34ec0176d9bab19abefb0e/scene/audio/audio_stream_player.cpp#L57

[lemilonkh]

Yes, thanks!
Fixed ✨

[MJacred]

Looking quite good : )

Last thing I can think of is that you should move all 3 lines with int current_beat = playback->get_current_beat(); four lines lower into the if (current_bar >= 0)-clause. That eliminates this function call for everybody who does not use the beats and bars logic.

[lemilonkh]

Thanks, applying that.
An issue that I have now is that when playing an MP3/ OGG audio stream without bpm information attached, we get tons of errors like this:

E 0:00:04:0044   get_frames_per_beat: bpm needs to be assigned in import settings.
  <C++ Error>    Condition "mp3_stream->get_bpm() <= 0" is true. Returning: -1
  <C++ Source>   modules/minimp3/audio_stream_mp3.cpp:148 @ get_frames_per_beat()

Is there a way to conditionally disable these error messages? Otherwise I guess I need a method for internal use on AudioStream that checks if there is bpm information and returns a boolean, which can then be used in the AudioStreamPlayer*::_notification logic?

Edit: I'll just call stream->get_bpm() beforehand 😅

[MJacred]

Edit: I'll just call stream->get_bpm() beforehand 😅

I thought the same, but it looks like you cannot reach the necessary stream class from the players. That's why I made that suggestion: #81542 (review)

[lemilonkh]

I can reach the class. AudioStream itself defines the get_bpm() method, and the players have a stream member.

[MJacred]

Indeed, I completely missed that and only saw Vector<Ref<AudioStreamPlayback>> stream_playbacks;!

[lemilonkh]

@AThousandShips @MJacred do you think this is ready to be merged, or do I need to make further changes? Thanks ✨

[AThousandShips]

Other than this it looks good style wise

[lemilonkh]

Applied the changes and squashed the commits again 👍
Thanks for that, I tried to do ranges with [ ] but didn't know how to escape them properly 😅

[IntangibleMatter]

Considering that this seems to be a complete pr with no remaining notes, will this be merged for 4.2?

[lemilonkh]

I was told that 4.2 is entering a feature freeze, so this will likely only get merged for 4.3 sadly.

[MJacred]

@lemilonkh: This needs to be approved first. Could you please check as well @ellenhp? (The code looked good to me at least).

If she's unavailable, we can ping Juan afterwards, as it's not about audio containers.

[ellenhp]

I'm a bit out of the loop with beat/bar stuff and dynamic audio. I made some proposals that made sense to me but Juan wasn't as convinced and I think we ended up going a different way. I haven't kept up with progress.

The only things I'll note here are:

• You shouldn't emit any signals from the audio thread, and it looks like you're not doing that. So you're good there.
• Lock-free thread safety on any shared data between the audio thread and main thread(s). As long as you're just using primitives I think that's fine because IIRC most platforms do 64-bit writes atomically. Could definitely be wrong there though.

Others may be better at reviewing for style than me and I think Juan should review this for audio.

[lemilonkh]

@reduz could you maybe give this PR a review?

I've heard from multiple people who would like to use the functionality in official builds to make e.g. rhythm games or music visualizations, so I think it would be nice if it could make it in for 4.3.

[lemilonkh]

Based on the feedback from @reduz on Twitter I removed the signals from this PR, since they were basically also polling (just with a sugar coated interface), which can be done in GDScript if required.

The previous state of the PR is preserved on the beat-helpers-signals branch in case anyone needs it.

I'd say the PR is ready for a review ✨

[supernovafiles]

@reduz any chance to take a look a this PR?

[Mickeon]

This is scheduled for 4.3 but it is far from requiring a proper assessment and it cannot be in the incoming version. Is there any progress on this PR? It should be moved to 4.4 just to be sure.

[lemilonkh]

From my end it does what it needs to do, but I agree it makes sense to move it to 4.4.

Let me know if there are any required changes, as this already went through the review process back when I originally made it, it was just never picked up by a core reviewer that could approve it...