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

Replace audio backend with miniaudio #2749

Merged
merged 3 commits into from
Apr 25, 2024
Merged

Replace audio backend with miniaudio #2749

merged 3 commits into from
Apr 25, 2024

Conversation

binary1248
Copy link
Member

By popular demand.

Work in progress.

Here be dragons.

@ChrisThrasher
Copy link
Member

master...miniaudio_rebase

I took a shot at rebasing and fixing merge conflicts. Some of the conflicts were easily fixed but I'm still not sure I correctly fixed the conflicts in src/SFML/Audio/Sound.cpp. I can't get it to compile.

@ChrisThrasher
Copy link
Member

ChrisThrasher commented Oct 24, 2023

I have most of the build errors fixed and removed all the imgui and imgui-sfml source code. I still need to fix compiler warnings and clang-tidy errors but this PR is at least in a state where it passes most of CI which means more people should be able to test it out.

The iOS jobs fail for reasons I don't totally understand. It's hopefully a matter of ensuring that Obj-C headers are being compiled as Obj-C instead of C or C++. Hopefully miniaudio actually works on iOS or else this PR is going to have a pretty tough decision to make.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: Patch coverage is 3.61174% with 1281 lines in your changes are missing coverage. Please review.

Project coverage is 42.76%. Comparing base (6eaf300) to head (fe5e54d).
Report is 15 commits behind head on master.

❗ Current head fe5e54d differs from pull request most recent head 13490da. Consider uploading reports for the commit 13490da to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2749      +/-   ##
==========================================
- Coverage   43.65%   42.76%   -0.90%     
==========================================
  Files         253      253              
  Lines       20940    22019    +1079     
  Branches     5139     5440     +301     
==========================================
+ Hits         9142     9417     +275     
- Misses      10785    11717     +932     
+ Partials     1013      885     -128     
Files Coverage Δ
include/SFML/Audio/InputSoundFile.hpp 71.42% <ø> (-3.58%) ⬇️
include/SFML/Audio/OutputSoundFile.hpp 0.00% <ø> (ø)
include/SFML/Audio/SoundFileFactory.inl 100.00% <ø> (ø)
include/SFML/Audio/SoundFileReader.hpp 100.00% <100.00%> (ø)
include/SFML/Audio/SoundFileWriter.hpp 100.00% <ø> (ø)
include/SFML/Audio/SoundRecorder.hpp 0.00% <ø> (ø)
src/SFML/Audio/SoundFileWriterFlac.hpp 100.00% <100.00%> (ø)
src/SFML/Audio/SoundFileWriterOgg.hpp 100.00% <100.00%> (ø)
src/SFML/Audio/SoundFileWriterWav.hpp 100.00% <100.00%> (ø)
include/SFML/Audio/AudioResource.hpp 0.00% <0.00%> (ø)
... and 24 more

... and 61 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebf916e...13490da. Read the comment docs.

@eXpl0it3r eXpl0it3r added this to the 3.0 milestone Oct 24, 2023
@binary1248
Copy link
Member Author

miniaudio officially supports iOS and building for it even has a dedicated subsection in its documentation.

@ChrisThrasher
Copy link
Member

Screenshot 2023-10-24 at 11 35 22 PM

I think my hunch was right. This should be fixable by telling CMake what specific files need to be compiled as Objective-C++.

@ChrisThrasher ChrisThrasher force-pushed the feature/miniaudio branch 2 times, most recently from 25b0fc0 to 28ddb70 Compare October 26, 2023 21:12
@ChrisThrasher
Copy link
Member

I've fixed almost all of the compiler warnings. The last one to address is here where MinGW appears to have found a null dereference inside of mini audio. I'm not sure if this warning is triggered by code we wrote or what.

We'll probably just pragma around this but I wanted to highlight it in case someone had an idea for a different fix.

Lastly I still have to fix those iOS jobs and then'll have 100% green CI :)

@ChrisThrasher ChrisThrasher force-pushed the feature/miniaudio branch 2 times, most recently from 2b60477 to 3785c81 Compare October 27, 2023 23:55
@ChrisThrasher
Copy link
Member

I was able to fix the iOS build errors and added and workaround to deal with that MinGW warning that I didn't otherwise know how to fix. CI should be 100% green now!

@ChrisThrasher ChrisThrasher marked this pull request as ready for review October 27, 2023 23:56
@ChrisThrasher ChrisThrasher force-pushed the feature/miniaudio branch 2 times, most recently from b53a5c5 to c09a017 Compare October 28, 2023 01:32
src/SFML/Audio/SoundBuffer.cpp Outdated Show resolved Hide resolved
src/SFML/Audio/SoundFileReaderWav.cpp Outdated Show resolved Hide resolved
src/SFML/Audio/Sound.cpp Outdated Show resolved Hide resolved
////////////////////////////////////////////////////////////
void* Sound::getSound() const
{
return &m_impl->m_sound;
Copy link
Member

Choose a reason for hiding this comment

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

#2640 made sure that a sound always had an associated sound buffer but that was merged after this PR's branch was originally started. We need to make sure that this new implementation maintains that same property. I'd expect the underlying pointers within the implementation to always be non-null no matter how a user may use the API.

include/SFML/Audio/Sound.hpp Show resolved Hide resolved
extlibs/headers/miniaudio/miniaudio.h Outdated Show resolved Hide resolved
src/SFML/Audio/CMakeLists.txt Outdated Show resolved Hide resolved
src/SFML/Audio/CMakeLists.txt Show resolved Hide resolved
include/SFML/Audio/SoundBuffer.hpp Show resolved Hide resolved
@ChrisThrasher
Copy link
Member

Rebased on master and applied a few changes recommended by Kimci.

src/SFML/Audio/CMakeLists.txt Outdated Show resolved Hide resolved
src/SFML/Audio/CMakeLists.txt Outdated Show resolved Hide resolved
include/SFML/Audio/Listener.hpp Outdated Show resolved Hide resolved
include/SFML/Audio/Music.hpp Outdated Show resolved Hide resolved
src/SFML/Audio/SoundFileWriterWav.cpp Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
src/SFML/Audio/SoundBuffer.cpp Outdated Show resolved Hide resolved
include/SFML/Audio/SoundSource.hpp Outdated Show resolved Hide resolved
src/SFML/Audio/SoundSource.cpp Outdated Show resolved Hide resolved
@ChrisThrasher
Copy link
Member

Rebased on master and fixed minor merge conflict.

@ChrisThrasher
Copy link
Member

I reenabled the clang-tidy check which is catching this pure virtual function call bug so that we can get better feedback once we start trying to fix it.

@binary1248
Copy link
Member Author

@danieljpetersen The pure virtual method call issue should be fixed now.

@ChrisThrasher
Copy link
Member

ChrisThrasher commented Apr 22, 2024

The unit tests in #2927 are still crashing on the copy construction of sf::SoundSource when I rebase them on this PR. This time UBSan is stating a pure virtual function is being called.

@ChrisThrasher
Copy link
Member

ChrisThrasher commented Apr 22, 2024

My Audio unit tests are now passing without any crashes. There is however this oddity in the sf::SoundSource tests

    SECTION("Set/get pitch")
    {
        SoundSource soundSource;
        soundSource.setPitch(42);
        CHECK(soundSource.getPitch() == 0); // Why not 42 here?
    }

    SECTION("Set/get volume")
    {
        SoundSource soundSource;
        soundSource.setVolume(0.5f);
        CHECK(soundSource.getVolume() == 0);
    }

    SECTION("Set/get position")
    {
        SoundSource soundSource;
        soundSource.setPosition({1, 2, 3});
        CHECK(soundSource.getPosition() == sf::Vector3f());
    }

    SECTION("Set/get relative to listener")
    {
        SoundSource soundSource;
        soundSource.setRelativeToListener(true);
        CHECK(!soundSource.isRelativeToListener());
    }

    SECTION("Set/get min distance")
    {
        SoundSource soundSource;
        soundSource.setMinDistance(12.34f);
        CHECK(soundSource.getMinDistance() == 0);
    }

    SECTION("Set/get attenuation")
    {
        SoundSource soundSource;
        soundSource.setAttenuation(10);
        CHECK(soundSource.getAttenuation() == 0);
    }

The setters and getters seem to just not work. Setting a value does not result in being about to get the old value. The setters and getters behave as one would expect with the OpenAL backend.

@binary1248 binary1248 force-pushed the feature/miniaudio branch 2 times, most recently from 655c36c to 7839348 Compare April 23, 2024 01:28
@binary1248
Copy link
Member Author

SoundSource was downgraded from being a semi-concrete class managing the actual OpenAL sound source to being an abstract interface with common functionality that is used by Sound and SoundStream. If you work around the fact that you shouldn't even be able to instantiate a SoundSource object by defining an empty derived class you can't expect it to work properly because there is no backing implementation behind it. If you want to test the SoundSource interface instantiate a proper object (Sound or Music) and test through a SoundSource& to the object.

@ChrisThrasher
Copy link
Member

CI is failing due to Homebrew updating clang-tidy which includes some new checks. I fixed that in #2953.

@danieljpetersen
Copy link
Contributor

Thank you Binary! That did indeed fix it. And thank you Bambo, thank you Thrasher!

Yesterday I wrote a little sound interpolator for the audio section of my codebase, where you give a duration and an easing formula for any given music/sound property(s). I've got it working in my game now to transition music on scene changes, it's especially nice when going from high tempo to low tempo music.

I'm looking forward to whenever this gets merged into master.

Copy link
Member

@eXpl0it3r eXpl0it3r left a comment

Choose a reason for hiding this comment

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

The assets of the new example need to be referenced in the asset_licenses.md file.

It's a really cool PR! 😎

Beyond getting rid or OpenAL and its LGPL implications, it's awesome to see the natural use of SFML 3 features like Angle and C++17(-ish). 🎉

src/SFML/Audio/Sound.cpp Show resolved Hide resolved
src/SFML/Audio/SoundBuffer.cpp Outdated Show resolved Hide resolved
binary1248 and others added 3 commits April 24, 2024 23:33
…s to multiple modules.

Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>
Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>
Co-authored-by: kimci86 <kimci86@hotmail.fr>
Co-authored-by: vittorioromeo <mail@vittorioromeo.com>
@binary1248
Copy link
Member Author

Rebased and updated.

Copy link
Member

@eXpl0it3r eXpl0it3r left a comment

Choose a reason for hiding this comment

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

The asset_licenses.md file still needs updating, but we can do that later as well. 😊

@eXpl0it3r eXpl0it3r merged commit c0ca39e into master Apr 25, 2024
209 checks passed
@eXpl0it3r eXpl0it3r deleted the feature/miniaudio branch April 25, 2024 08:24
@eXpl0it3r
Copy link
Member

Big thanks to @binary1248 for taking on this work and everyone for reviewing! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet