search

lairworks / nas2d-core

NAS2D is an open source, object oriented 2D game development framework written in portable C++.
http://nas2d.lairworks.com
zlib License
10 stars 5 forks source link

Re-introduced audio bug #1103

Closed DanRStevens closed 1 year ago

DanRStevens commented 1 year ago

While experimenting with a Windows VM, I saw we may have re-introduced a bug loading the background music, as seen in:

I believe the bug may have been introduced by the removal of vcpkg install flags for SDL2-mixer here:

Without those install flags, vcpkg may build a version of SDL2-mixer without support for the needed audio format.

Also, since we did some manual copying of DLLs related to that audio format, it's possible this bug may be hidden by old files still being around on developer's machines. It could be that cleaning the solution output folder may be needed to reproduce the error.

As I was working on other vcpkg changes at the time, it's possible my changes in progress were related, so this may need some verification.

This may be easier to address after switching to vcpkg Manifests (Issue #373).

ldicker83 commented 1 year ago

Running in a VM often causes this as an invalid or null audio device is provided to SDL and NAS2D assumes with MixerSDL that it will always get a valid audio device. There used to be (may still exist) a Null Mixer that could be used in its place if an invalid audio mixer is provided but there is no automatic fallback at this point, the end user needs to do that and OPHD doesn't check for problems with the mixer.

DanRStevens commented 1 year ago

I'm pretty sure that's not the problem. All the VMs I've ever used have always had default audio hardware. And I vaguely remember you saying basically the same thing last time we had a problem with the audio, and it turned out to be the vcpkg feature flags to enable that format needed to be passed when building SDL2-mixer.

Couple that with a fairly recent change that removed the SDL2-mixer flags, I think that's the problem. There never really was much of an explanation for why the feature flags were removed. Supposedly because of problems, and removing the flags made the errors go away, though the audio format error was a runtime issue, so could easily have been missed during that change. At any rate, I never saw the original error that removing the flags supposedly addressed, nor any documentation suggesting there had been a change.

The vcpkg port file still defines optional features, and none of them are default: https://github.com/microsoft/vcpkg/blob/2718aa52a31977b792af7e9c2f83eaf4ddcd932d/ports/sdl2-mixer/vcpkg.json#L4

Pretty sure we simply removed the feature flag needed for the OGG audio format.

Also, we already do have fallback code to use MixerNull:

        try
        {
            Utility<Mixer>::init<MixerSDL>();
        }
        catch (...)
        {
            Utility<Mixer>::init<MixerNull>();
        }
ldicker83 commented 1 year ago

Huh. Well then.

It's weird that it's a problem, I thought I'd tried a full build on a clean Windows install afterward. Will have to look further into it.

DanRStevens commented 1 year ago

So you may have been right about the VM lacking audio support. I double checked the VM, and it indeed lacked audio support. Adding audio hardware to the VM allowed the game to play sound.

There were a few interesting findings while looking into this. First off is that the PostBuildEvent used in OPHD to copy OGG related DLL files is no longer needed (PR https://github.com/OutpostUniverse/OPHD/pull/1392). It seems vcpkg will copy these files on it's own now.

Perhaps related to that, is the removal of the vcpkg flag for libvorbis when installing sdl2-mixer. It seems the core install of sdl2-mixer with vcpkg now includes support for OGG files.

The two flags that vcpkg removed for sdl2-mixer were:

Additionally, the sdl2-mixer package has a feature flag which was possibly added since then:

That means the following flags are potentially now usable: "features": ["opusfile", "mpg123", "libmodplug", "libflac", "fluidsynth"]

To investigate, I branched off the point before the flags were removed, so I could run a CI build and see any errors concerning the flags. The flags were removed in commit 7405f6ec3bdd68df54c815bd40a3b073b45c7780, so need to branch off of commit 0c6a9a51f2f08c5e04df6df941e51f4867d51b18.

Errors were:

Build Logs:

Not sure if we want to add back any extra feature flags. We probably don't need them, and only experimented with them to get OGG support. Though it may be useful to have that extra support if people want to mod the game.