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

Platform-specific Audio #599

Merged
merged 43 commits into from
Jul 29, 2024
Merged

Platform-specific Audio #599

merged 43 commits into from
Jul 29, 2024

Conversation

capnkenny
Copy link
Member

@capnkenny capnkenny commented Feb 21, 2024

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Documentation has been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Removes the legacy AudioService and adds the new AudioMixer and OS specific implementations.

What is the new behavior (if this is a feature change)?

Users should begin using the AudioMixer interface and let it choose the backing audio platform automatically.

Resolves an issue where spdlog's bundled fmt lib was using deprecated/non-standard MSFT extensions that Windows was removing soon
No audio playing due to bad handling of buffers
Kind of retrofitted to current ECS setup? but the point was to make it playback using OAL first and in a separate static lib.
Need to fix Ogg playback
Copy link

github-actions bot commented Feb 21, 2024

Test Results

   10 files  ±0     10 suites  ±0   6m 46s ⏱️ +36s
  693 tests ±0    693 ✅ ±0  0 💤 ±0  0 ❌ ±0 
6 930 runs  ±0  6 930 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit fa7b475. ± Comparison against base commit 7bd1b57.

♻️ This comment has been updated with latest results.

Need to finish standardising samplerate conversion support and formats for XAudio and OpenAL
Resolves an issue where spdlog's bundled fmt lib was using deprecated/non-standard MSFT extensions that Windows was removing soon
No audio playing due to bad handling of buffers
Kind of retrofitted to current ECS setup? but the point was to make it playback using OAL first and in a separate static lib.
Need to fix Ogg playback
Need to finish standardising samplerate conversion support and formats for XAudio and OpenAL
@capnkenny capnkenny force-pushed the feature/audio-system branch from 87d3728 to d517243 Compare July 4, 2024 11:18
@capnkenny capnkenny marked this pull request as ready for review July 4, 2024 11:19
Copy link
Member

@RubyNova RubyNova left a comment

Choose a reason for hiding this comment

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

Overall looks fine I just have some questions - I've only quickly scanned this just due to how large it is.

Comment on lines +56 to +58
alGetError();
alSourceStopv(static_cast<int>(_sources.size()), reinterpret_cast<ALuint*>(_sources.data()));
GetALError();
Copy link
Member

Choose a reason for hiding this comment

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

Can I ask what the difference is between alGetError and GetALError?

Copy link
Member Author

Choose a reason for hiding this comment

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

One gets the error code from OpenAL and clears the error register internally (from OpenAL's side) - the other (GetALError) does that plus actually parses the error code into something human-readable

{
case AL_INVALID_NAME:
{
_logger->error("A bad ID or name was passed to the OpenAL function.");
Copy link
Member

Choose a reason for hiding this comment

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

Are we directly using spdlog due to the deprecation of the old APIs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - in this case its easier to do so, and since there's some uncertainty around how the APIs will be used going forward (with the great big refactor) it was just cleaner to do so, so that I could continue to keep the static lib isolated to itself

src/NovelRT/Ecs/Audio/AudioSystem.cpp Outdated Show resolved Hide resolved
src/NovelRT/Ecs/Audio/AudioSystem.cpp Outdated Show resolved Hide resolved
Copy link
Member

@RubyNova RubyNova left a comment

Choose a reason for hiding this comment

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

LGTM

@RubyNova RubyNova merged commit 5ad1ba6 into main Jul 29, 2024
15 checks passed
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.

2 participants