Sound improvements #1450

kcat · 2017-09-13T11:08:19Z

Cleans up some unnecessary use of shared_ptr with related micro-allocations.
Removes excessive checks for changing the 'near water' sound effect when near water.
Increases the default min/max buffer cache size to 56/64 (should help avoid log spam with sound mods).
Applies an "underwater" reverb preset and low-pass filter when underwater, replacing the pitch shift effect. Requires the ALC_EXT_EFX extension.
Ensures all 3D sounds are properly spatialized. Requires the AL_SOFT_source_spatialize extension.

The website is currently down, so I'm unfortunately not able to provide links to related issues.

akortunov · 2017-09-13T11:47:09Z

apps/openmw/mwsound/openal_output.cpp


 #define MAKE_PTRID(id) ((void*)(uintptr_t)id)
 #define GET_PTRID(ptr) ((ALuint)(uintptr_t)ptr)

 namespace
 {

+// The game uses 64 units per yard, or approximately 69.99125109 units per meter.
+// Should this be defined publically somewhere?
+constexpr float UnitsPerMeter = 69.99125109f;


Looks like Visual Studio 2013 does not support constexpr.

Do we want to keep support for MSVC 2013? Our win builds are the only official ones...

According to this page, constexpr should be (mostly) supported in the November 2013 CTP which can be used with VS2013. Is it possible to update AppVeyor? Might need to ask @Ace13 and whoever else builds on Windows if that's a good idea.

Ah, the 2013 CTP. I've used that for personal projects before, not the nicest of things to either install or use.
I'd rather see us drop 2013 support than tell people to install something that's literally named "Preview" to be able to compile.

Though dropping 2013 also probably means dropping Windows XP support, since 2015 and forward use the UCRT runtime.

We should not be supporting Windows XP either. In linux land, if the distro is EoL, then we don't support it. Same for Microsoft, if they EoL WinXP (and hopefully Vista), then we don't officially support it either.

While I'm open to the idea of upgrading, is it worth it purely for 'constexpr'? Is there anything else we would gain? If we do want to go ahead, best take it to the forums to ask who's using 2013.

We should not be supporting Windows XP either. In linux land, if the distro is EoL, then we don't support it. Same for Microsoft, if they EoL WinXP (and hopefully Vista), then we don't officially support it either.

Generally agreed, but there's two types of support - we can allow things to run, but that doesn't mean we need to be accountable if someone can't get it working or reports a bug. Something that isn't officially supported can still happen to work, for a while anyway.

Sure, there's not much usecase for XP but if you happened to have a really old computer that is only used offline, there's no harm in not upgrading it.

Agreed, but if the machine is used offline then you're not likely to be running OpenMW now then are you? ;)

If 'constexpr' is the straw that broke the camel's back for WindowsXP support, then so be it. We shouldn't be held behind for that.

BTW: We still support mingw right? So we still have WindowsXP compatible builds anyway.

This instance of using constexpr is far from necessary, so if it's that much of an issue I can just use plain const. But it would still be a good idea to discuss it on the forums for when constant expressions come up elsewhere in the code base.

In fact this exact issue was already discussed in this PR a few weeks ago but no decision was made there. Personally I don't think XP support should be dropped just for constexpr but there's always the option of using
if (MSVC_VERSION LESS 1900) add_definitions(-Dconstexpr) ;)

ghost · 2017-09-13T12:27:57Z

apps/openmw/mwsound/soundmanagerimp.cpp

@@ -1021,7 +1021,7 @@ namespace MWSound

                    if(sound->getDistanceCull())
                    {
-                        if((mListenerPos - objpos).length2() > 2000*2000)
+                        if((mListenerPos - objpos).length2() > sfx->mMaxDist*sfx->mMaxDist)


Are you sure this is fine? IIRC the original game uses 2000, regardless of what's in the sound record. See bug 244 about loud ghostgate sounds, and the comment next to the definition of Play_RemoveAtDistance.

I was testing things around Seyda Neen and saw that sounds that were being culled had a max distance of 2000, so it seemed having the hardcoded cull distance at 2000 was unnecessary and potentially problematic if modded sounds wanted to be audible farther away.

In regards to Ghostgate, I checked and it doesn't seem to be obnoxiously loud like this. Although that may be due to OpenAL Soft now having a volume limiter (it reduces the overall gain when the output samples would clip). The video in bug 244 showcases what the sound is like in vanilla's software audio mode, which can have extra limitations due to needing to run on weaker CPUs. Unfortunately, there doesn't seem to be any examples of it in hardware/DirectSound3D mode for comparison.

I'll set the vanilla game up to run in Wine with emulated hardware audio to see how it behaves.

For now I've reverted it back to a hardcoded 2000, because I ran into another place where sounds became unnecessarily loud ('dagoth ur, outer facility'). However, I ran into a separate issue with sound sources of projectiles sometimes getting "lost" (intermittently the sound sources for the projectiles don't stop when they're supposed to, but are no longer being tracked either). I need to figure out what's going on with that before I update the PR.

ghost · 2017-09-13T12:38:47Z

apps/openmw/mwsound/openal_output.cpp

@@ -622,6 +622,7 @@ void OpenAL_Output::init(const std::string &devname)
    }

    ALC.EXT_EFX = !!alcIsExtensionPresent(mDevice, "ALC_EXT_EFX");
+    AL.SOFT_source_spatialize = !!alIsExtensionPresent("AL_SOFT_source_spatialize");


The !! stopped me for a second... that's a funny way to write static_cast<bool>

Anyway great change. This fixes the overly-loud riekling scream sounds, doesn't it?

The !! is something I picked up in earlier days using C (which didn't have an explicit bool type readily available). And it just seems more lean and elegant to me than static_cast<bool>(...). If it's an issue, I can actually change the flags to bools for the cast to happen automatically (I just wasn't sure if the size specifier worked properly with bools, but they do).

It does fix the riekling sounds, yes. cr/riek/moan.wav is stereo for some reason (other sounds may be too), and this will properly handle it.

To avoid being able to accidentally cast a Stream* to a Sound*, or vice-versa.

Rather than checking every frame you're near the water, only check when the current cell changed (the sfx will only change when moving between interior and exterior). It also doesn't need to look through all playing sounds, as it's a local one not attached to a Ptr.

This replaces the pitch-shift effect when available.

Standard OpenAL does not spatialize non-mono sounds, although the game has some stereo sounds meant to play in 3D. The desired behavior can be achieved with the AL_SOFT_source_spatialize extension.

Also shorten them by putting them in the MWSound namespace

kcat · 2017-09-15T09:44:06Z

Updated. Removed constexpr, restored the hardcoded 2000 unit culling distance, and also cleaned up some enums and a bunch of unnecessary exceptions. I believe I got the cause of the stuck sounds I mentioned... at least I haven't heard it anymore when testing since.

There is one thing to note about this changeset, namely that returned Sound objects no longer have an indefinite lifetime. Once the associated sound stops, the Sound object is no longer 'valid' as it's returned to the internal pool for later reuse. I've checked over the code and all current uses are fine with this (most places ignore the returned Sound object, and anything that actually holds onto it plays a looping sound which won't automatically stop until explicitly called to, at which point it knows to not use it anymore). To the point though, it would be beneficial if nothing outside of the SoundManager depends on the returned Sound object, and there's actually only two places that use it: projectiles and ambient weather. All other sounds are either fire-and-forget, or are referenced by using a Ptr and/or SoundID.

In theory, the weather manager would just need to tell the sound manager what the current ambient sound is and its volume, and the sound manager can manage the Sound object on its own. This would be relatively easy to do.

Projectiles are a different story. Despite being things in the world they don't seem to have an associated Ptr, which leaves no easy way to reference individual instances other than the returned Sound object. Rather than having a Ptr to track the sound position and lifetime like every other world object, the projectile manager wants to manually handle the sounds. This doesn't seem right to me, but I wouldn't know where to begin on fixing that (it seems to be a bit of a hack, directly touching the rendering and physics systems and having deeper control of sounds than anything else outside of the sound manager needs).

psi29a · 2017-09-15T12:03:10Z

And msvc 2013 failed again with:

C:/projects/openmw/apps/openmw/mwsound/openal_output.cpp(398): error C2536: 'MWSound::OpenAL_SoundStream::MWSound::OpenAL_SoundStream::mBuffers' : cannot specify explicit initializer for arrays (C:\projects\openmw\MSVC2013_32\apps\openmw\ub_mwsound.cpp) [C:\projects\openmw\MSVC2013_32\apps\openmw\openmw.vcxpr

ananace · 2017-09-15T12:13:10Z

I'd suggest just replacing the mBuffers{0} with memset(mBuffers, 0, sNumBuffers), single-value initialization of plain C-arrays isn't that well defined even with C++11.

ghost · 2017-09-15T21:33:48Z

I think this PR should be fine as-is, but it would be nice to have another independent person test it, due to the scope of the changes. If anyone's tested, please tell us :)

it would be beneficial if nothing outside of the SoundManager depends on the returned Sound object,
...
In theory, the weather manager would just need to tell the sound manager what the current ambient sound is and its volume, and the sound manager can manage the Sound object on its own. This would be relatively easy to do.

The current SoundManager design feels a little weird to me. On the one hand it deals with world objects and their environment, on the other hand it also deals with actually playing/updating and buffers for all those sounds, talk to the OpenAL backend and other implementation details. Maybe it should do only do the backend and leave the managing part to World, Scene or another class, i.e. the sound subsystem no longer depends on World or another subsystem. In the event we wanted to refactor in that direction in the future, it might be beneficial to still support Sound objects being handled by the caller.

Projectiles are a different story. Despite being things in the world they don't seem to have an associated Ptr, which leaves no easy way to reference individual instances other than the returned Sound object.

They're not handled as world objects because the original game doesn't handle them as such, and if they were then they'd have incorrect collision, you could grab a projectile to your inventory while it's in air, etc.

MiroslavR · 2017-09-16T00:43:13Z

I think this PR should be fine as-is, but it would be nice to have another independent person test it, due to the scope of the changes. If anyone's tested, please tell us :)

I have played a bit and no obvious issues showed up. The new water filters work fine as far as I can tell. Didn't notice any change in the riekling moan loudness, though.

kcat · 2017-09-16T01:27:51Z

On the one hand it deals with world objects and their environment, on the other hand it also deals with actually playing/updating and buffers for all those sounds, talk to the OpenAL backend and other implementation details. Maybe it should do only do the backend and leave the managing part to World, Scene or another class, i.e. the sound subsystem no longer depends on World or another subsystem.

It makes sense to me that the sound manager handles the sound lifetimes and their associations with world objects. It's the glue between the sound renderer and world state. That way we only have one place to worry about sound duplication (the same object playing the same sound multiple times), and making sure sounds are cleaned up when their associated Ptr goes away. Whenever (multiple) sound zones become a thing in the future, having a single place to manage those and how they interact with sounds would also be preferable.

Granted, I'm hardly an expert on sound system design. Most of the things I'm planning for I've not attempted to do in "production code" before, but this is how I would initially plan for them. Though I wouldn't necessarily discount alternative design ideas as long as it doesn't over-complicate things.

They're not handled as world objects because the original game doesn't handle them as such, and if they were then they'd have incorrect collision, you could grab a projectile to your inventory while it's in air, etc.

I don't mean to have an actual arrow object be the Ptr. I mean a live projectile would be its own class (where models and sounds and such are specified) and have a Ptr to reference live instances of it.

The new water filters work fine as far as I can tell. Didn't notice any change in the riekling moan loudness, though.

The riekling fix needs the AL_SOFT_source_spatialize extension to work (added in OpenAL Soft 1.18). Older versions of OpenAL Soft, and Creative's drivers, don't have that extension so the loudness won't be any different. It would be nice to have a proper console logging where this information can be reported, rather than relying on stdout and stderr. Maybe I'm too used to something like ZDoom's start-up and in-game console.

MiroslavR · 2017-09-16T09:36:52Z

The riekling fix needs the AL_SOFT_source_spatialize extension to work (added in OpenAL Soft 1.18).

Okay, that explains it. The latest Debian provides is 1.17 currently.

ghost · 2017-09-16T15:57:46Z

I don't mean to have an actual arrow object be the Ptr. I mean a live projectile would be its own class (where models and sounds and such are specified) and have a Ptr to reference live instances of it.

I remember considering this idea back then, for some reason we discarded it but I don't remember why.

psi29a · 2017-09-16T17:30:54Z

@MiroslavR 1.18 is ready to ship on Debian, waiting for an uploader...

akortunov reviewed Sep 13, 2017

View reviewed changes

ghost reviewed Sep 13, 2017

View reviewed changes

kcat added 18 commits September 14, 2017 16:00

Replace some shared_ptrs with pointers to deque entries

1fe60dd

Include alext.h to get OpenAL extension definitions

9e7a49f

Make a note that stopTrack needs to be called for a stopping track

9e45f6d

Make Sound and Stream sibling types

617c05f

To avoid being able to accidentally cast a Stream* to a Sound*, or vice-versa.

Load EFX functions when available

3d37cb3

Load an effect and filter for underwater

c790fed

Set the appropriate meter/unit scale for sound

0b720cd

Properly update the near water sound volume

033303b

Increase the default buffer cache sizes

ec01b89

Use unordered_map for the music playlist and sound buffer caches

27eeaf9

Apply reverb and a low-pass filter when underwater

edfba68

This replaces the pitch-shift effect when available.

Avoid copying the same Ptr with each iteration

6f57233

Ensure 3D sources are spatialized

605c937

Standard OpenAL does not spatialize non-mono sounds, although the game has some stereo sounds meant to play in 3D. The desired behavior can be achieved with the AL_SOFT_source_spatialize extension.

Move the soundlist when updating a Ptr instead of copying

abe80f5

Don't be so throw-happy in the sound manager

c17edfd

Set HRTF when initializing the device

3757571

Avoid some explicit loops

1e123a2

kcat force-pushed the master branch from 774d444 to 4601e17 Compare September 15, 2017 08:09

kcat added 2 commits September 15, 2017 02:36

Replace a few more explicit loops

0c1ad7c

Make the PlayMode and PlayType enums scoped

780e824

Also shorten them by putting them in the MWSound namespace

kcat force-pushed the master branch from 4601e17 to 780e824 Compare September 15, 2017 09:38

Use an std::array for the OpenAL stream buffers

d68e158

ghost merged commit 5dd2e87 into OpenMW:master Sep 16, 2017

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sound improvements #1450

Sound improvements #1450

kcat commented Sep 13, 2017

akortunov Sep 13, 2017

psi29a Sep 13, 2017

kcat Sep 14, 2017

ananace Sep 14, 2017 •

edited

Loading

psi29a Sep 14, 2017

ghost Sep 14, 2017

psi29a Sep 14, 2017 •

edited

Loading

kcat Sep 14, 2017

PlutonicOverkill Sep 15, 2017 •

edited

Loading

ghost Sep 13, 2017

kcat Sep 13, 2017

kcat Sep 14, 2017

ghost Sep 13, 2017

kcat Sep 13, 2017

kcat commented Sep 15, 2017

psi29a commented Sep 15, 2017 •

edited

Loading

ananace commented Sep 15, 2017

ghost commented Sep 15, 2017

MiroslavR commented Sep 16, 2017

kcat commented Sep 16, 2017

MiroslavR commented Sep 16, 2017

ghost commented Sep 16, 2017

psi29a commented Sep 16, 2017

Sound improvements #1450

Sound improvements #1450

Conversation

kcat commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananace Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psi29a Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlutonicOverkill Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcat commented Sep 15, 2017

psi29a commented Sep 15, 2017 • edited Loading

ananace commented Sep 15, 2017

ghost commented Sep 15, 2017

MiroslavR commented Sep 16, 2017

kcat commented Sep 16, 2017

MiroslavR commented Sep 16, 2017

ghost commented Sep 16, 2017

psi29a commented Sep 16, 2017

ananace Sep 14, 2017 •

edited

Loading

psi29a Sep 14, 2017 •

edited

Loading

PlutonicOverkill Sep 15, 2017 •

edited

Loading

psi29a commented Sep 15, 2017 •

edited

Loading