Silence AudioPlayer2D if transform NaNs would poison audio system.

[jjmontesl]

This cheap check prevents using NaN values for panning and volume in AudioStreamPlayer2D, which would result in NaN values being mixed as a result of multiplying samples by those values.

This NaN volume and panning would appear if the Transform has NaN values (this should not happen, but if it does it may poison the audio system through effects).

I thought of adding ERR_PRINT_ONCE to inform about this, but I'm not sure if that's appropriate. If so, please feel free to add a message (showing the offending node path might be interesting too but might potentially create a lot of output).

I think this is a safe and useful fix to backport to 3.2, especially now that other audio fixes are being applied.

[akien-mga]

What bug does it fix exactly? Do you have a reproduction project that can be used to confirm?

@ellenhp has been fixing issues with NaN wrongly ending up in the audio system, maybe this is not needed anymore?
This will hide potential bugs which would likely need to be solved instead (it can still be considered as a temporary workaround but as it is now, it would likely lead to never fixing the bugs that actually cause NaNs to be in the audio system).

[jjmontesl]

I don't have a reproduction project for this :(, but it can be reproduced by placing an AudioStreamPlayer2D with a NaN position on the scene. This was part of my workaround for NaNs prior to @ellenhp changes.

I have been diagnosing the NaN bugs and then working a bit with @ellenhp, hence I decided to submit this one separately. I detected and fixed this issue in my codebase, and this seems to not be related to the "uninitialized buffers" problem. (Note this never tried to fix the root cause of the "serious" NaN issues, but is a different source of NaNs which I also discovered while researching #28110).

In this case, we are just protecting the AudioSystem from bogus input from the user (setting NaN values to transforms). This doesn't mask any error (the sample would not play anyway) but at least prevents the audio system from being poisoned. Also allows the user to better find the bogus Transform since only that sound will be missing, instead of the entire audio system.

(Despite the lack of a reproduction case, it's (relatively) easy to follow that NaN values in the object transform will cause NaN values to be mixed).

NOTE: In this case the NaN GlobalPosition was set by my code briefly during startup (which would poison the audio effects), there's no other root cause behind it. Hence this PR attempting to protect AudioStreamPlayer2D from that.

[ellenhp]

I agree that we should make sure that this is still necessary. I suspect that #46151 may have fixed the issue this aims to solve, but there could definitely be other issues.

Edit: If we do end up merging this, I would maybe recommend doing a WARN_PRINT_ONCE in the body of the conditional, to signal that this did occur in the wild. And it might be better to switch to using isnan? Or documenting that NaNs specifically are what you're looking for.

[jjmontesl]

I switched to isfinite() from checking NaN, as infinites would cause the same problem. We are not looking for NaNs, we are looking for finite numbers, hence the isfinite().

I also don't understand how #46151 could help in this case.

Anyway I have just tested this without my fix and indeed it causes all audio to stop:

1. Load the WoobleBooble test project from Audio stops working after scene reload. #28110.
2. Disable the existing Music node (set autoplay to false). Test music doesn't play during game (other sounds do).
3. Open scene Level.tscn. Add an AudioStreamPlayer2D node called AudioStreamPlayerNaN using the Trance song.ogg set to autoplay. Move it to the center of the level. Test that music plays.
4. Add to level.gd:21 at the end of the _ready function:

$AudioStreamPlayerNaN.global_position = Vector2.ZERO / 0.0

5. Run the game. No sounds are played at all.

With the requested fix only the bogus song player is silenced, and the audio engine is not poisoned.

[ellenhp]

Yeah, I definitely agree that NaNs in the transform will kill the audio, and maybe that makes sense to fix just for the sake of handling it. But NaNs in a node's transform might be a bug that should be addressed separately too. I bet that breaks lots of other things, not just audio.

[jjmontesl]

Well, In my case it seems to just break audio. Anyway, it does kill the audio. I agree that the NaNs in transforms or as result of operations would better be (if at all) be addressed separately. This fix is about preventing that from poisoning the audio buffers.

I was impacted by this and I'd still be if I hadn't debugged it, so I believe this is a very real problem indeed. Without this fix any operation that happens to yield NaN at any point during runtime (like in my case) may kill all audio.

[akien-mga]

I thought of adding ERR_PRINT_ONCE to inform about this, but I'm not sure if that's appropriate. If so, please feel free to add a message (showing the offending node path might be interesting too but might potentially create a lot of output).

If we end up merging this to work around the issue, I agree that we should have an ERR_PRINT_ONCE (or at least of print_verbose, but let's start with the more obnoxious one and change it if there's a need). Otherwise we're hiding the issue and won't ever fix it, while ERR_PRINT_ONCE serves as a reminder that there's a bug and we're simply working it around.

[akien-mga]

Is this still needed / is the issue that this aims to solve still reproducible in latest 4.x releases?

If it is, it would be really good to have a proper bug report / reproduction test project so it can be confirmed and investigated. As discussed above, we don't expect NaNs to happen and if they do, that's an engine bug that we need to investigate.

We can add a safety check like here but it should print an error that enables engine contributors to notice and fix the source of such bugs.

[ellenhp]

As far as I know all of the NaN issues were fixed, and I believe the fixes are all backported too. If anyone is aware of any remaining issues that cause NaNs to propagate into the audio buffer that would be good to have in the tracker.

[akien-mga]

Closing then, let's follow up with bug reports first if those issues are still reproducible.