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

Expose panning strength parameters #58841

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

ellenhp
Copy link
Contributor

@ellenhp ellenhp commented Mar 6, 2022

Exposes tightness/panning strength as a project setting and as properties on the 2d and 3d audio player nodes.

Bugsquad edit: Follow-up to #58823 (can be merged independently). This closes godotengine/godot-proposals#3384 and closes #42358 by superseding it.

@ellenhp ellenhp requested review from a team as code owners March 6, 2022 17:24
@Calinou Calinou added this to the 4.0 milestone Mar 6, 2022
@Calinou
Copy link
Member

Calinou commented Mar 6, 2022

Testing project for 3D: test_audio_panning_strength.zip

I think setting Panning Strength to 0.5 by default would be optimal for headphone users (albeit still a little on the strong side, but not nearly as much as 1.0). Maybe the default project settings' values should be halved. It should still provide intense enough panning for speaker users.

Fun fact: Negative values can be set via script, which reverse the stereo panning effect entirely. This can't be done through the inspector due to the range hint.

@ellenhp
Copy link
Contributor Author

ellenhp commented Apr 3, 2022

Did we ever get to a resolution on how to properly cache a project setting for fast access every frame? Invalidating the cache gets very tricky.

@ellenhp ellenhp force-pushed the expose_tightness branch from 1479e6b to 6ac8b09 Compare April 3, 2022 15:43
@ellenhp ellenhp requested a review from a team as a code owner April 3, 2022 15:43
@ellenhp
Copy link
Contributor Author

ellenhp commented Apr 3, 2022

I just updated the settings to require restart, which is a bummer but not the end of the world. If we want to pursue the caching route someone else might have to tackle it because ProjectSettings is confusing me. :)

@Calinou you seemed to have some opinions on this PR. Did I resolve everything?

@@ -304,6 +304,12 @@
<member name="audio/driver/output_latency.web" type="int" setter="" getter="" default="50">
Safer override for [member audio/driver/output_latency] in the Web platform, to avoid audio issues especially on mobile devices.
</member>
<member name="audio/general/2d_panning_strength" type="float" setter="" getter="" default="0.5">
The strength of the panning effect for AudioStreamPlayer2D nodes.
Copy link
Member

@Calinou Calinou Apr 15, 2022

Choose a reason for hiding this comment

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

This project setting doesn't seem to be read when it's changed at run-time, so the effective global panning strength can't be changed at run-time (changes will be ignored). This should be done to make forcing non-positional audio output easier for accessibility. This is not critical for a first iteration of this feature, but in this case, the limitation should be documented in the property description.

See #53296 (review) which added a project_settings_changed signal in 3.x for reference (it's also available in master).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it up as a property that requires restart, because I couldn't figure out how to do runtime cache invalidation for project setting caches. The accessibility use-case can be addressed with the use of the Stereo Enhance effect as discussed in #58823

I'll address the rest of these in a bit!

Copy link
Member

Choose a reason for hiding this comment

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

You can use project_settings_changed signal for cache invalidation.

The strength of the panning effect for AudioStreamPlayer2D nodes.
</member>
<member name="audio/general/3d_panning_strength" type="float" setter="" getter="" default="2.0">
The strength of the panning effect for AudioStreamPlayer3D nodes.
Copy link
Member

Choose a reason for hiding this comment

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

Same remark as above for this project setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd prefer to leave this as-is for now. If we decide that we need to have this update in real-time we can come back and make that change later. It won't break compat so I'm not too worried. Especially considering that we have Stereo Enhancer. Let me know if you'd like me to build in the cache invalidation now though!

@@ -1207,6 +1207,8 @@ ProjectSettings::ProjectSettings() {
GLOBAL_DEF("application/config/project_settings_override", "");
GLOBAL_DEF_BASIC("audio/buses/default_bus_layout", "res://default_bus_layout.tres");
custom_prop_info["audio/buses/default_bus_layout"] = PropertyInfo(Variant::STRING, "audio/buses/default_bus_layout", PROPERTY_HINT_FILE, "*.tres");
GLOBAL_DEF_RST("audio/general/3d_panning_strength", 2.0f);
Copy link
Member

@Calinou Calinou Apr 15, 2022

Choose a reason for hiding this comment

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

Property range hints should be added with custom_prop_info[] (as done above) to specify a valid range for the setting.

I suggest allowing values between 0.0 and 4.0 and with a step of 0.01 (same for 2D and 3D).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm tweaking the ranges a bit. It's an advanced setting so I'm fine having it be a broad range with a fine adjustment step. Let me know if you're completely sure the ranges should be the same. The defaults are different so it would feel strange to me to have them adjustable over the same range.

Copy link
Member

@Calinou Calinou Apr 16, 2022

Choose a reason for hiding this comment

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

Is there a reason the 2D and 3D defaults are different? I find the 2D default pretty good, and I'd like the 3D default to be the same in 4.0. The current 3D default feels way too strong to me. Halving the value in the testing project gives me a good result on headphones, but I haven't tested it on speakers.

Over the years, I've seen a lot of complaints about 3D audio panning strength being too strong in Godot, and many of those complaints are likely coming from speaker users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2D and 3D values control fundamentally different values. I'm exposing the tightness parameter in SPCAP, and then just a scalar for panning in 2D. They're not comparable values, so I don't know how to make them the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to normalize the default values for each in ProjectSettings to 1.0 or something we could just hardcode a scalar into the places where these settings are used. Also I think I mentioned in another PR but I'm travelling right now so I'm not in a good position to pick a good default. I don't think I have access to headphones for at least another few days.

Copy link
Member

@Calinou Calinou Apr 17, 2022

Choose a reason for hiding this comment

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

If we want to normalize the default values for each in ProjectSettings to 1.0 or something we could just hardcode a scalar into the places where these settings are used.

Sounds good to me 🙂

We can decide changing the actual default 3D panning intensity in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, made a change to bake defaults into the nodes instead of into the project settings. I think you're right; it looks a bit cleaner that way.

@Calinou
Copy link
Member

Calinou commented Apr 15, 2022

I tested both 2D and 3D functionality rebased on latest master and it works as expected.

Updated testing project (with 2D and 3D test scenes): test_audio_panning_strength_updated.zip

@ellenhp ellenhp force-pushed the expose_tightness branch 3 times, most recently from 8662dcf to 0be393f Compare April 17, 2022 23:40
@ellenhp
Copy link
Contributor Author

ellenhp commented Jun 10, 2022

What's the status on this PR? Is it blocked on me or does Reduz need to look at it? It would be an enormous win for users IMO because the current stereo panning isn't great. Just want to make sure it doesn't get lost.

@akien-mga
Copy link
Member

What's the status on this PR? Is it blocked on me or does Reduz need to look at it? It would be an enormous win for users IMO because the current stereo panning isn't great. Just want to make sure it doesn't get lost.

I had this comment pending a resolution: https://github.com/godotengine/godot/pull/58841/files/0be393fcf225eb8197a38b4f6afd1ceed1a787b6#diff-425ab30a63dbbf819f27d912b74dd7cfc929681b8cf68037216b96a607ebb801

And I added a couple more.

Otherwise a quick check from @reduz would have been nice but since it's not happening, I think we can merge.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me, aside from a couple nitpicks.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 11, 2022
@akien-mga
Copy link
Member

I force pushed an update to resolve my review comments.

@ellenhp
Copy link
Contributor Author

ellenhp commented Jun 17, 2022

I force pushed an update to resolve my review comments.

Thank you! I'm on vacation right now and in general I've been very focused on another project so I didn't get to this.

@akien-mga akien-mga merged commit d2be541 into godotengine:master Jun 17, 2022
@akien-mga
Copy link
Member

Thanks!

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.

Add a property to adjust spatial stereo panning on AudioStreamPlayer2D and AudioStreamPlayer3D
4 participants