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 AudioStreamSynchronized::get_bar_beats(), fix division by zero #99327

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

colinator27
Copy link
Contributor

Fixes #92453, and tested using its MRP. get_bar_beats() was unimplemented on AudioStreamSynchronized, so this implements it similarly to other methods like get_bpm(). It could also be implemented more similarly to get_beat_count(), which gets the maximum beat count (not just the first), although it's a bit unclear what the intended behavior would be if there are different bar beats for each sub-stream.

The underlying issue is that get_bar_beats() is used to calculate the end time of the current bar (in AudioStreamInteractive), and without an implementation, it returns zero. This leads to a division by zero, resulting in NaN, which completely breaks timings. Thus, this PR also adds a guard which will check if bar beats is > 0 (if not, the transition will instead occur immediately), since this can probably occur with other stream types that still do not implement get_bar_beats().

(As a side note, this division by zero guard may also need to be implemented in the new code introduced in #99188.)

@colinator27 colinator27 requested a review from a team as a code owner November 16, 2024 18:13
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 17, 2024
@adamscott adamscott changed the title Implement get_bar_beats() for AudioStreamSynchronized, fix division by zero Implement get_bar_beats() for AudioStreamSynchronized, fix division by zero Dec 4, 2024
@adamscott adamscott changed the title Implement get_bar_beats() for AudioStreamSynchronized, fix division by zero Implement AudioStreamSynchronized::get_bar_beats(), fix division by zero Dec 4, 2024
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Approving the PR. @colinator27, next time though, please create 2 PRs instead of cramming two features into one. ;)

@adamscott
Copy link
Member

So AudioStreamSynchronized::get_bar_beats() returns the first bpm available. Is this something we want though?

@adamscott adamscott self-requested a review December 4, 2024 15:56
@colinator27
Copy link
Contributor Author

Approving the PR. @colinator27, next time though, please create 2 PRs instead of cramming two features into one. ;)

Noted, thanks!

So AudioStreamSynchronized::get_bar_beats() returns the first bpm available. Is this something we want though?

Yeah, I'm not certain on this. At least in the given MRP for the original issue, it's better than returning 0, although it could be defined to not work with synchronized streams. Besides always being 0 or returning the first BPM, the only other options I can think of is returning a minimum or maximum BPM, but it seems pretty arbitrary either way. I'm not sure if it should be defined what happens if there's multiple different BPMs, especially as they're meant to be synchronized, but maybe there's a case to be made.

@Repiteo Repiteo merged commit eb51030 into godotengine:master Dec 5, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 5, 2024

Thanks! Congratulations on your first merged contribution! 🎉

@timoschwarzer
Copy link
Contributor

I rebased #99188 onto the latest master branch so it contains this fix, thanks for mentioning!

@adamscott
Copy link
Member

@colinator27 I just saw that the other methods in modules/interactive_music/audio_stream_synchronized.cpp use the same exact algorithm that the one you implemented. So no worries, it's OK then.

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