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

Check for NaN in set_volume_db functions #90861

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

aaronp64
Copy link
Contributor

Added check in AudioStreamPlayer, AudioStreamPlayer2D, and AudioStreamPlayer3D set_volume_db functions to prevent setting volume to NaN, and give an error. Using NaN for volume and playing the AudioStreamPlayer could prevent all audio from playing, even from other AudioStreamPlayers.

Fixes #88133

Added check in AudioStreamPlayer, AudioStreamPlayer2D, and AudioStreamPlayer3D set_volume_db functions to prevent setting volume to NaN, and give an error.  Using NaN for volume and playing the AudioStreamPlayer could prevent all audio from playing, even from other AudioStreamPlayers.

Fixes godotengine#88133
@aaronp64 aaronp64 requested review from a team as code owners April 18, 2024 15:36
@KoBeWi KoBeWi added this to the 4.3 milestone Apr 18, 2024
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

What about is_finite()? Like would you also want to block infinities?

@AThousandShips
Copy link
Member

Agreed, should check !is_finite otherwise you'll get NaN in several cases

@KoBeWi
Copy link
Member

KoBeWi commented Apr 18, 2024

Using infinite numbers does not break anything, only NaNs.

@AThousandShips
Copy link
Member

AThousandShips commented Apr 18, 2024

Using infinite numbers does not break anything, only NaNs.

Are you sure? There's no maths there that generates NaN with infinities? I haven't looked at it all but infinities breeds NaN, remember that 0 * inf is NaN as is inf - inf and -inf + inf

@KoBeWi
Copy link
Member

KoBeWi commented Apr 18, 2024

I mean I tested what happens when you set volume_db to INF/-INF and it works in all AudioStreamPlayers (i.e. does not permanently break audio).

@AThousandShips
Copy link
Member

Then let's keep it NaN and see if something breaks down the line, though I'd prefer to keep it to just finites as it doesn't make sense to have infinites

@akien-mga akien-mga merged commit e34399c into godotengine:master Apr 19, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronp64 aaronp64 deleted the audio_stream_player_nan branch May 13, 2024 13:48
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.

If AudioStream is playing with NaN VolumeDb value, whole audio doesn't work. No errors displayed.
5 participants