Add volume L/R controls to AudioStreamPlayer (master)

[20kdc]

This is useful for manual panning.
Manual panning otherwise requires processing the sounds to have separate L and R variants.
Processing this in GDScript is insanely inefficient, so I decided this was the neater solution.

NOTE: Due to inherent interference between Godot 4.x hardware support and my own personal policies, a version of this PR will be made targetting 3.x directly, as the PR is effectively useless to me on 4.x-only.
Testing for this PR has only been performed with the 3.x version.

The 3.x PR will be submitted first so that it can be cross-linked-to, the master PR will be submitted a few seconds later with the link added.

3.x PR: #51666

[20kdc]

Would just like to check: Are any of the requested changes not actually performed?

[Calinou]

@ellenhp Do you think the audio scale L/R should be exposed as linear scales or decibels? Exposing as decibels is more consistent with volume_db, but may be confusing. I have no strong preference for using a linear scale or decibels here.

Would just like to check: Are any of the requested changes not actually performed?

I can see the changes being pushed, so yes 🙂

[ellenhp]

I'd recommend using decibels for consistency. Do you know how this change interacts with surround sound mix targets? It seems this should work but it might be a good idea to special-case MIX_TARGET_CENTER, since if I remember right one channel of the center mix target is the center, and the other is LFE which goes directly to a subwoofer (I think?). The spatial audio player nodes show some of the logic that's used to deal with that if I remember right. I'd also recommend maybe considering doing a left-right channel balance instead of letting users throw decibel values directly into the AudioStreamPlayer.

Perhaps a value in the range -1 (full left) to 1 (full right) with zero being balanced, and the default? That's much more intuitive to deal with in the editor in my opinion, and preserving loudness automatically would be really nice for people doing stuff in scripts.

The other thing here is you are going to need to implement ramps to avoid popping.

[Calinou]

Perhaps a value in the range -1 (full left) to 1 (full right) with zero being balanced, and the default? That's much more intuitive to deal with in the editor in my opinion, and preserving loudness automatically would be really nice for people doing stuff in scripts.

I agree this would be easier for users to manage too.

[ellenhp]

Honestly I'd really prefer we get #51296 in before merging this, because it'll really simplify the code. It removes the need for ramping volume, for starters. That's all handled in the audio server now. I suppose for 3.x the volume ramps will need to be implemented though since #51296 is not a reasonable change to cherry pick IMO.

[20kdc]

Notes before I work out how to make this into a commit:
My assumption on surround is that if what I'm doing here doesn't work, then the original code never worked in the presence of a stereo file (as one might get from stereo music).
My assumption on panning is that this exists explicitly as a bypass for stuff like AudioStreamPlayer2D, and therefore that automating things is probably not the ideal approach (helper function to set them with a specific balance, maybe, but I'd need to know the specific balancing mechanism you had in mind)

Something that would resolve both concerns would be per-mix-target volume pairs + a balance function to mostly-automate if that's what the user wants, but not sure how to design the API for this... or achieve this at all.

[ellenhp]

I think that's a reasonable assumption. It's probably fine as-is without handling surround sound in a special way. The only issue IMO is it being difficult to use for dealing with stereo balance. I'd expect any sort of stereo balance control to preserve loudness as a user. I don't feel strongly either way but that's my suggestion. :)

[20kdc]

	// the 2.0 on left/right is critical to preserving loudness.
	// a balance of 0.0 ought to be a no-op, which means a scale of 1.0,1.0
	// therefore, left/right need to be scaled to 2.0 to compensate.
	volume_scale.l = MIN(2.0, MAX(0.0, 1.0 - p_balance));
	volume_scale.r = MIN(2.0, MAX(0.0, 1.0 + p_balance));

I'd like confirmation on this balancing mechanism before I actually commit it to both branches.

[ellenhp]

This looks right to me. Unless there's some psycho-acoustic or perceptual stuff that I don't know about, I think it'll sound right too.

[20kdc]

found the issue with linux builds, turns out I mis-documented a parameter, will fix

[20kdc]

Um, I didn't expect that to remove the existing request, sorry!
EDIT:
Explaination: I hit "re-request review" assuming that was the correct way to acknowledge that as far as I know (via see review) I've performed all requested changes.

[20kdc]

Just so you know, the only requested change marker still active is outdated, but a merge conflict has appeared since, so I'll have to poke this

[Calinou]

Should audio balance be exposed as a property? Right now, it's only a method, which means you can't change it in the editor.

I'm not sure if exposing the individual L/R scale makes sense, as I expect most people to be tweaking the audio balance instead.

[20kdc]

Ok, so, the good news is that by the look of it AudioServer now covers the functionality I wanted, but better: (set_playback_all_bus_volumes_linear).
The bad news: This isn't actually exposed to GDScript.

However obviously this represents a significant divergence from the 3.x solution.
With that in mind, this PR is being closed.

[ellenhp]

I'm more than happy to help walk you through how to navigate the different solutions for the master branch and 3.x, but I would strongly prefer to only expose the panning property. I don't want to expose separate left/right volumes how you were doing earlier, because then we're stacking two different volume controls on top of each other which just seems weird to me. And like Calinou said if you want to change panning in the editor you're out of luck if the left/right volumes are exposed instead of the panning property. For the master branch we can use the AudioServer method you found since that does all the volume ramping for us, but for 3.x you'll need to write some volume ramping code. It's not the end of the world but it's a lot harder than it should be, which is a big part of why I added that AudioServer method in the first place :)

…

On Wed, Oct 20, 2021 at 2:42 AM 20kdc ***@***.***> wrote: Closed #51667 <#51667>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51667 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOY265YIPTO4Q3EEBWAF7DUH2FHFANCNFSM5CFCVNWQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[20kdc]

to me it would be above and beyond enough to be able to supply a specific set of volumes to set_playback_all_bus_volumes_linear from GDScript.

It is important to note that separate left/right volumes was the whole point of the PR. If someone wanted to use whatever Godot's default balancing method is they would likely just build a small "virtual listener box" in 2D.

Right now it's not even possible to block off the left or right channel without processing the sample float-by-float. (in 3.x, anyway, AFAIK.)

[ellenhp]

Last time I tried to make the case for exposing the AudioServer's playback API, reduz didn't seem interested. For this PR though, I want to expose the panning property you made instead of the left/right volume. It seems mathematically equivalent to left/right volume but it's more intuitive and it's less clutter in the inspector. If reduz can be convinced that the AudioServer playback API is worth exposing then this is all unnecessary I suppose.

…

On Wed, Oct 20, 2021 at 8:44 AM 20kdc ***@***.***> wrote: to me it would be above and beyond enough to be able to supply a specific set of volumes to set_playback_all_bus_volumes_linear from GDScript. It is important to note that separate left/right volumes was the whole point of the PR. If someone wanted to use whatever Godot's default balancing method is they would likely just build a small "virtual listener box" in 2D. Right now it's not even possible to block off the left or right channel without processing the sample float-by-float. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51667 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOY266UBW5VG4N4KQSKGCLUH3PXJANCNFSM5CFCVNWQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[20kdc]

Eeeh. This bit only matters for the 3.x PR, but: It's mathematically equivalent if willing to pre-process the L/R volumes to get the panning ratio between them, which seems annoying.
Plus someone would have to stop anyone from changing the panning function even if the new one is more realistic to some arbitrary definition of panning, because at that point people are going to start relying on the details (the mathematical equivalence only holds so long as the panning function remains the same).

Anyway, not exposing AudioServer's playback API directly isn't that big of a concern to me.

Ultimately my general idea is that if something wants exact panning control, it should have it. Hypothetical application would be "write an XM player in Godot" kinda thing here - stuff where there's no real reason GDScript shouldn't be able to do it because the real CPU-intensive stuff is all done on the C++ side anyway, so using GDScript avoids having to deal with native compilation across platforms (read: Mac).

That in mind, now that API on the AudioServer exists direct Volume L/R properties are the wrong way to go about this on master to me, because it's less flexible. Hence I've closed this PR - it'd basically be rewritten entirely anyway.

Seeing as the underlying API supports surround sound, whatever API covers it ought to do so as well. If I was doing it I'd just add a method to apply a multiplied PoolFloatArray over the whole lot and a method to clear it.

[ellenhp]

That's a good point. Philosophically speaking though this should be exposed by the audio server not by the nodes. Godot exposes advanced use cases through the servers not by adding a bunch of properties to the nodes. Plus I wrote the audio server API to be resilient enough to be exposed to script. I'll bring this up in the developers chat again. Oct 20, 2021 9:09:17 AM 20kdc ***@***.***>:

…

Eeeh. This bit only matters for the 3.x PR, but: It's mathematically equivalent if willing to pre-process the L/R volumes to get the panning ratio between them, which seems annoying. Plus someone would have to stop anyone from changing the panning function even if the new one is more realistic to some arbitrary definition of panning, because at that point people are going to start relying on the details (the mathematical equivalence only holds so long as the panning function remains the same). Anyway, not exposing AudioServer's playback API directly isn't that big of a concern to me. Ultimately my general idea is that if something wants exact panning control, it should have it. Hypothetical application would be "write an XM player in Godot" kinda thing here - stuff where there's no real reason GDScript shouldn't be able to do it because the real CPU-intensive stuff is all done on the C++ side anyway, so using GDScript avoids having to deal with native compilation across platforms (read: Mac). That in mind, now that API on the AudioServer exists direct Volume L/R properties are the wrong way to go about this on master to me, because it's less flexible. Hence I've closed this PR - it'd basically be rewritten entirely anyway. Seeing as the underlying API supports surround sound, whatever API covers it ought to do so as well. If I was doing it I'd just add a method to apply a multiplied PoolFloatArray over the whole lot and a method to clear it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub[#51667 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAOY265VM3344447QENBVT3UH3SSZANCNFSM5CFCVNWQ]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEIAAABCCAYAAADjVADoAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAnSURBVHic7cEBDQAAAMKg909tDwcUAAAAAAAAAAAAAAAAAAAAAPBjRFIAASHKmKkAAAAASUVORK5CYII=###24x24:true###][Tracking image][https://github.com/notifications/beacon/AAOY2644MGZIBKNDHPGVZ5LUH3SSZA5CNFSM5CFCVNW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHB7I43I.gif]