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

Enable OpenAL-Soft extensions for non-framework builds and add config for disabling HRTF #1620

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gabeklavans
Copy link

@gabeklavans gabeklavans commented Feb 27, 2024

Issue description

There was no include for the alext.h header, meaning the engine could not use any OpenAL-Soft extensions. Additionally, HRTF was always enabled, meaning when OpenAL detected headphones as the output device, it would apply the default HRTF to the audio.

Solution description

Added the alext.h include for non-framework builds (the OpenAL framework does not include alext.h) and added a config variable audio-want-hrtf to determine how the ALC_SOFT_HRTF extension should be configured in the openal audio manager.

Checklist

I have done my best to ensure that…

  • …I have familiarized myself with the CONTRIBUTING.md file
  • …this change follows the coding style and design patterns of the codebase
  • …I own the intellectual property rights to this code
  • …the intent of this change is clearly explained
  • …existing uses of the Panda3D API are not broken
  • …the changed code is adequately covered by the test suite, where possible.

I've tested a wheel build of this branch using python3.9 on Windows, macOS, and Ubuntu 22.04

resolves #1613

@@ -2601,7 +2601,8 @@ def WriteConfigSettings():
dtool_config["PYTHON_FRAMEWORK"] = 'Python'
dtool_config["PHAVE_MALLOC_H"] = 'UNDEF'
dtool_config["PHAVE_SYS_MALLOC_H"] = '1'
dtool_config["HAVE_OPENAL_FRAMEWORK"] = '1'
if not os.path.isdir(GetThirdpartyDir() + "openal"):
dtool_config["HAVE_OPENAL_FRAMEWORK"] = '1'
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this isn't perfect, but I can't see a better way right now

Copy link
Author

Choose a reason for hiding this comment

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

I had the same thought. I couldn't find anything that specified whether openal was enabled specifically via the thirdparty directory. However, I think removing OpenAL Framework support entirely is going to be my next suggestion, so I didn't belabor the change too much.

Copy link
Member

Choose a reason for hiding this comment

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

Fine - makepanda will be deprecated in favour of CMake shortly anyway.

PRC_DESC("This determines whether OpenAL-Soft should activate HRTF in "
"certain hardware configurations. Set it to true to cause "
"OpenAL to automatically apply HRTF processing to all output "
"audio when headphones are used for playback."));
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this an openal-specific config variable?

Copy link
Author

Choose a reason for hiding this comment

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

There are almost no config variables that specify that they are for openal by name, most of the config variables that do in fact only apply to openal just contain "audio" (I experienced this with audio-preload-threshold and I'm sure there are many others).

So my way making it an "openal-specific config" was just to place it along side the other ones. Let me know if there's more I should do for that.

@rdb
Copy link
Member

rdb commented Mar 2, 2024

Emscripten build failed... maybe we need a separate dtool_config.h flag for alext presence.

@rdb
Copy link
Member

rdb commented Mar 2, 2024

Or, we need to just vendor alext.h, just like we do with glext.h... it's feature-gated at runtime anyway.

@gabeklavans
Copy link
Author

Emscripten build failed... maybe we need a separate dtool_config.h flag for alext presence.

Emscripten does actually support extensions, including the one I'm trying to use here ALC_SOFT_HRTF. Not sure how if they don't expose alext.h, since you kinda need the defines. Maybe they pack it into alc.h? I can experiment on my local.

Otherwise yeah I can look into feature gating

@rdb
Copy link
Member

rdb commented Mar 2, 2024

I am leaning towards investigating shipping our own alext.h.

@gabeklavans
Copy link
Author

gabeklavans commented Apr 9, 2024

Poking around in the PR that added support for the ALC_HRTF_SOFT extension in Emscripten, specifically in the tests added for it, it seems they require you to specify the enum's value directly, which is not quite ideal for our case.

That is to say, I agree that we could try to ship our own alext.h file for Emscripten builds to have access to the proper defines. I'll try to look into that soon, likely by looking at glext.h as you mentioned for an example.

@gabeklavans
Copy link
Author

The missing alext.h file is actually already on the radar for emscripten, but no one's made progress on it in a while. I think a better solution to this would be to just fix this upstream, since it seems to be a pretty simple fix there (just adding the alext.h header to their includes).

@gabeklavans
Copy link
Author

alext.h landed in emscripten emscripten-core/emscripten#21857 so whenever the next version releases, if we bump to that, this PR should hopefully successfully build

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

Successfully merging this pull request may close these issues.

Include alext.h header file to enable using OpenAL Extensions
2 participants