Update audio engine by frabert · Pull Request #366 · REGoth-project/REGoth

frabert · 2018-09-12T19:39:59Z

Provides a more backend-independent audio system
Also fixes some sounds being played at half speed (chapter introductions)

Provides a more backend-independent audio system Also fixes some sounds being played at half speed (chapter introductions)

ghost · 2018-09-13T20:00:14Z

src/audio/AudioWorld.cpp

-            alGenBuffers(RE_NUM_MUSIC_BUFFERS, m_musicBuffers);
-            alGenSources(1, &m_musicSource);
+            m_MusicSource = m_Engine.getAudioEngine().createSound(
+                [&](std::int16_t* buf, std::size_t len) -> int {


nit: as types of buf and len aren't directly needed in lambda, you can use auto.

ghost · 2018-09-13T20:00:57Z

src/audio/AudioWorld.cpp

-                return Handle::SfxHandle::makeInvalidHandle();
-            }
-            return Handle::SfxHandle::makeInvalidHandle();
+            const std::int16_t* dataPtr = (std::int16_t*)(wav.getData());


I'd try to avoid old style cast.

I'd try to avoid old style cast.

why?

You don't really know what is happening there. And it's quite hard to refactor.

ghost · 2018-09-13T20:01:43Z

src/audio/AudioEngine.h

+        virtual void stop() = 0;
+    };
+
+    using SoundRef = std::shared_ptr<Sound>;


SoundPtr?

ghost · 2018-09-13T20:05:24Z

src/audio/OpenALAudioEngine.cpp

+            , m_engine(engine)
+        {}
+    public:
+        OpenALSound(OpenALAudioEngine* engine, const std::int16_t* buf, std::size_t len, Format fmt, std::size_t samplingFreq)


line is way too long

ghost · 2018-09-13T20:06:31Z

src/audio/OpenALAudioEngine.cpp

+    captureContext();
+    // Remove all expired sounds
+    std::remove_if(std::begin(m_bufferedSounds), std::end(m_bufferedSounds),
+                   [](const std::shared_ptr<OpenALSound>& sound) { return sound.use_count() == 1; });


auto for parametr/arg

ghost · 2018-09-13T20:10:45Z

src/audio/OpenALAudioEngine.cpp

+    std::remove_if(std::begin(m_bufferedSounds), std::end(m_bufferedSounds),
+                   [](const std::shared_ptr<OpenALSound>& sound) { return sound.use_count() == 1; });
+    std::remove_if(std::begin(m_streamingSounds), std::end(m_streamingSounds),
+                   [](const std::shared_ptr<OpenALSound>& sound) { return sound.use_count() == 1; });


lamda is needlessly written twice.

ghost · 2018-09-13T20:12:06Z

src/audio/OpenALAudioEngine.cpp

+        {
+            std::array<std::int16_t, BUFFER_SIZE> dataBuffer{};
+
+            for(auto buf : m_buffers)


If you are referring to auto buf, m_buffers is a vector of integers, it should not make any difference here

ghost · 2018-09-13T20:13:45Z

src/audio/AudioWorld.h

            Daedalus::GEngineClasses::C_SFX sfx;
            std::vector<Handle::SfxHandle> variants;  // Instances ending with "_Ax"
-            unsigned m_Handle = 0;
+            Audio::SoundRef sound = nullptr;


= nullptris redundant

ghost · 2018-09-13T20:17:24Z

src/audio/OpenALAudioEngine.cpp

+
+OpenALAudioEngine::~OpenALAudioEngine()
+{
+    m_bufferedSounds.erase(std::begin(m_bufferedSounds), std::end(m_bufferedSounds));


m_bufferedSounds.clear(); will do the same // I'd add comment why you do it

tomedi · 2018-09-13T21:41:25Z

Tested your build. The chapter music works fine, but it seems that musiczones got somehow messed up. When you enter Khorinis, for example, music doesn't switch until you reach the place in front of Vatras. Also, I had the impression that just by turning the Hero, it had an impact on the music (directly in front of Lobart, there was a weird switching in between the lobart and the regular world theme).

frabert · 2018-09-14T16:24:40Z

@shfil119 I addressed most of the criticisms, thanks.
@tomedi I don't think those issues could be introduced with this PR. If you toggle the debug music zones draw, you can see that indeed the city zone starts in front of Vatras's place

frabert · 2018-09-14T17:48:37Z

I'm marking this as WIP, I'd like to make some more updates like making the AudioWorld live in the BaseEngine instead of the World (there are no ties between it and the AudioWorld), to allow stuff like playing music and sound effects before a game is started

ghost · 2018-09-14T22:01:06Z

src/audio/OpenALAudioEngine.cpp

+
+using namespace Audio;
+
+static std::vector<std::string> enumeratedDevices;


maybe instead of global static use anonymous namespace?

I don't really see the need, it's just going to be used in this file only, I think. Actually, it could even be declared as static inside the function that uses it 🤔

Even better, we could also stick to the YAGNI principle and remove the functionality outright...!

ghost · 2018-09-14T22:02:08Z

src/audio/OpenALAudioEngine.cpp

+// can be accessed in a thread-safe manner
+static std::mutex mtx_enumeratedDevices;
+
+constexpr std::size_t OPENAL_SOURCES_POOL = 256;


Don't know regoth naming convection, but it looks like macro name.

Yeah the rationale is that that variable is essentially to be considered a macro, but let the compiler treat it as a variable. If that makes sense anyway.

ghost · 2018-09-14T22:03:30Z

src/audio/OpenALAudioEngine.cpp

+        }
+        else
+        {
+            enumeratedDevices.push_back("");


You can emplace_back()

ghost · 2018-09-14T22:05:14Z

src/audio/OpenALAudioEngine.cpp

+
+OpenALAudioEngine::~OpenALAudioEngine()
+{
+    m_bufferedSounds.clear();


IIRC buffers of openAL should be removed earlier than "closing device". I guess comment explaining why it is here will be helpful.

ghost · 2018-09-14T22:06:05Z

src/audio/OpenALAudioEngine.cpp

+    {
+    protected:
+        OpenALSound(OpenALAudioEngine* engine)
+            : m_buffer(0)


You can use default member initializer and make ctors simpler.

ghost · 2018-09-14T22:07:34Z

src/engine/BaseEngine.h

        EngineArgs m_Args;

-        Audio::AudioEngine* m_AudioEngine = nullptr;
+        std::shared_ptr<Audio::AudioEngine> m_AudioEngine;


Can it be unique_ptr?

Yes, I think so 👍

ghost · 2018-09-14T22:09:45Z

src/audio/OpenALAudioEngine.h

+
+#include <string>
+
+typedef struct ALCdevice_struct ALCdevice;


It's going to interfere with #include <al/al.h> when it's included in OpenALAudioEngine

frabert · 2018-09-14T23:06:20Z

@shfil119 Done addressing the new reviews.

During testing Gothic 1 I noticed that some sounds are played even though they should not be heard, like In Extremo (which is now playing correctly 👍 ) that starts right as the player is spawned. Maybe it's got something to do with the maximumDistance. More investigation is needed

frabert · 2018-09-15T10:21:50Z

Last commit should fix the issue, marking as ready for review & merge

ghost · 2018-09-15T12:00:22Z

src/audio/OpenALAudioEngine.cpp

+    {
+    protected:
+        OpenALSound(OpenALAudioEngine* engine)
+            : m_hasBuffer(false)


Also can used default member initializer.

ghost · 2018-09-15T12:00:45Z

src/audio/OpenALAudioEngine.cpp

+            , m_stream(stream)
+            , m_format(fmt == Format::Mono ? AL_FORMAT_MONO16 : AL_FORMAT_STEREO16)
+            , m_sampleRate(sampleRate)
+            , m_stop(false)


Also can used default member initializer.

… large music zones

src/engine/BaseEngine.cpp

ghost · 2018-09-18T19:29:33Z

src/logic/MusicController.cpp

-    "_NGT_STD",
-    "_NGT_THR",
-    "_NGT_FGT",
+    {


also unwanted spaces?

Yep, this one is unwanted 😨

ghost · 2018-09-18T19:34:15Z

src/audio/OpenALAudioEngine.cpp

+    auto lambda = [](const auto& sound) { return sound.use_count() == 1; };
+
+    // Remove all expired sounds
+    std::remove_if(std::begin(m_bufferedSounds), std::end(m_bufferedSounds), lambda);


Btw Is this needed? Actually it just moves objects in vector.

What do you mean if it is needed? The shared_ptrs are never going to get deleted if they stay inside the array, this allows them to be freed

I think about missing erase, https://cpppatterns.com/patterns/remove-elements-from-container.html

Doh! 🤦‍♂️ Yeah my C++-fu is still not very strong

ghost · 2018-09-18T19:37:08Z

src/audio/OpenALAudioEngine.cpp

+
+            if(!renderBuffer(dataBuffer)) return;
+
+            while(true)


Without further reading it's hard to determine what's the purpose of loop. Some boolean with name explaining what's going on will be useful. ;)

ghost · 2018-09-18T19:37:38Z

src/audio/OpenALAudioEngine.cpp

+                ALint processedBuffers;
+                alGetSourcei(m_source, AL_BUFFERS_PROCESSED, &processedBuffers);
+                if(processedBuffers <= 0) continue;
+                while(processedBuffers--)


Also readability. Can be used range loop?

I think at most a standard for loop could be used

ghost · 2018-09-18T19:43:16Z

src/audio/AudioWorld.h

-        };
+        Daedalus::DaedalusVM *m_SoundVM = nullptr, *m_MusicVM = nullptr;

        struct Sound : public Handle::HandleTypeDescriptor<Handle::SfxHandle>


I haven't seen virtual dtor of HandleTypeDescriptor, isn't there problem with leaking memory? (by vector and string)

I don't know the details of the memory management inside the engine, this is something @ataulien or @markusobi know better than me

ghost · 2018-09-18T19:46:18Z

src/audio/NullAudioEngine.cpp

+    : public Sound
+{
+public:
+    void pitch(float p) override {}


As some parts of interface aren't implemented you can think about splitting interface into smaller ones. Idea from SOLID. ;)

I'm not usre I understand what do you mean? The interface is fully implemented, it just doesn't do anything :)

Well im not cpp guy but he means that having methods from contract which are not doing anything in class which implements this contract(like no body at all or throwing exeption) is overall a bad concept and you should rethink your interfaces. It should be separated interface then which this class just shouldn't implement.

It's pretty much Liskov Subtitution Principle and also ISP.

The current way this is implemented is to allow compiling for platforms that don't support audio, or for running the game without any audio, without having to change anything about the frontend code. The NullAudioEngine provides a way to let the game engine use an AudioEngine without actually doing anything.

This is the best I could come up with, suggestions are welcome!

src/content/AnimationLibrary.cpp

ghost · 2018-09-18T19:50:46Z

src/logic/MusicController.cpp

 MusicController::MusicController(World::WorldInstance& world, Handle::EntityHandle entity)
    : Controller(world, entity)
-    , m_isPlaying(false) {}
+    , m_isPlaying(false)


Also can be default member initializer.

ghost · 2018-09-18T19:51:52Z

src/logic/MusicController.cpp

    // threatened or fighting to play the correct music.
-    if ((!m_isPlaying && isInBoundingBox())
-        || (m_isPlaying && m_currentTime != time))
+    if (((!m_isPlaying || currentTheme.find(m_instancePrefix) != 0) && isInBoundingBox()) || (m_isPlaying && m_currentTime != time))


Too long, I'd try to reformat all your code with clang-format. ;)

src/audio/AudioEngine.h

tomedi · 2018-09-22T15:08:27Z

The game crashes when you load a savegame after you started a new game.
crashlog_music.txt

frabert · 2018-09-22T16:11:30Z

Can you confirm this is not present in master? I cannot reproduce this on my machine

Also this looks like it's got something to do with bgfx/3d rendering, I'm not sure what could have introduced it from this PR

tomedi · 2018-09-22T16:46:52Z

@frabert Oh, I'm sorry... This crash is indeed present at the latest master-build. Just stumbled over it because I was testing your branch.

frabert · 2018-09-22T16:55:25Z

That's a bug on its own right, anyway. I remember seeing something similar some time ago but could never pinpoint it to something precise, it happens sometimes when sometimes calls btVector3::normalize apparently

ghost · 2018-10-06T18:52:38Z

src/audio/AudioEngine.h

+        virtual void stop() = 0;
+    };
+
+    using SoundPtr = std::shared_ptr<Sound>;


nit: it should be doable with unique_ptr

I wasn't able to do it with a unique_ptr because OpenGLAudioEngine keeps reference of the sounds that have been created

Is the destruction order of Sound and OpenGLAudioEngine unspecified?

Considering that OpenGLAudioEngine is basically guaranteed to never be destroyed (it should be destroyed when the application closes), Sounds are always going to die before the engine. The issue is that the Engine gives out Sounds to its users, and these objects must stay alive even if they go out of scope, otherwise the sounds would stop playing. So the Engine keeps reference of all created sounds and destroys them when they are not tracked anymore by anyone else and they are not playing

ghost

Don't have assets to test behavior, but code looks good to me.

ghost · 2018-10-26T17:19:13Z

src/audio/OpenALAudioEngine.cpp

+        {
+            if (!m_hasSource) return State::Stopped;
+            ALint s;
+            alGetSourcei(m_source, AL_SOURCE_STATE, &s);


nit: as it returns number, you can assign these numbers in enum and cast value of s to State.

I'm not sure I particularly like this idea, because it ties it specifically to the states that OpenAL returns, which is the reason I wrote a wrapper around it...

ghost · 2018-10-26T17:22:43Z

src/audio/OpenALAudioEngine.cpp

+        std::size_t m_sampleRate;
+        std::atomic_bool m_stop;
+        std::array<ALuint, BUFFER_NUM> m_buffers{};
+        std::thread m_thread;


nit: Probably one thread for sounds is enough.

// But probably (also) it's not convenient to implement this.

Indeed it is a bit wasteful to have a separate thread for each streaming sound, it would have be nice to have a single thread that manages all streaming sounds at once, but it complicates the design as you said.

However, there should be very few streaming sounds going on generally (one for music, one for video sound playback when it will get implemented), so I don't think it is too bad of a perf hit.

frabert added 3 commits September 12, 2018 21:35

Update audio engine

aea0e26

Provides a more backend-independent audio system Also fixes some sounds being played at half speed (chapter introductions)

Fix disabling OpenAL audio system if needed

f773798

Fix no-sound command line flag having wrong short form

f2d84e4

ghost reviewed Sep 13, 2018

View reviewed changes

Address various issues in the code

5979f79

frabert changed the title ~~Update audio engine~~ [WIP] Update audio engine Sep 14, 2018

frabert added 2 commits September 14, 2018 21:37

Move AudioWorld from World to BaseEngine

4306e72

Play music during game start and loading screens

cd5e440

ghost reviewed Sep 14, 2018

View reviewed changes

frabert added 6 commits September 15, 2018 00:22

Cleanup usage of std::transform instead of Utils::uppered/lowered

7ed92f6

Make m_AudioEngine a unique_ptr

c3e0385

Add some comments

1ab3fbe

Put static members directly inside function definition

ce3042c

Simplify constructors

d3a0348

Use emplace_back instead of push_back

dac612d

Fix sounds being played even when out of range

e10c5aa

frabert changed the title ~~[WIP] Update audio engine~~ Update audio engine Sep 15, 2018

ghost reviewed Sep 15, 2018

View reviewed changes

frabert added 5 commits September 15, 2018 14:22

Use default initializers where possible

6651301

Revert default initializer for m_stop

69574b0

Fix that intersecting music zones could cause issues

e820843

Use player's position instead of camera's to detect music zones

54c13a3

Increase MusicController's draw distance factor to disable pruning of…

3cbb8d9

… large music zones

ghost reviewed Sep 18, 2018

View reviewed changes

Remove superfluous whitespace

c671961

frabert added 2 commits September 19, 2018 17:41

Clarify audio buffering loop

c4b4588

_Actually_ remove items from container

e0f423a

mdrost reviewed Sep 20, 2018

View reviewed changes

src/audio/AudioEngine.h Show resolved Hide resolved

frabert added 6 commits September 20, 2018 17:27

Rewrite music zone system

f0691c0

Fix random typo

d5f9367

Add virtual destructor for Sound

2220b89

Play loading music also on savegame load

0285540

Move MusicZoneManager, enable (de)serialization for savegames

84b22ce

Update CMake version

e5e1124

Update Gradle plugin

7ec8c7d

Change apk path

f93d45c

ghost reviewed Oct 6, 2018

View reviewed changes

ghost approved these changes Oct 6, 2018

View reviewed changes

ghost reviewed Oct 26, 2018

View reviewed changes


		using namespace Audio;

		static std::vector<std::string> enumeratedDevices;

Conversation

frabert commented Sep 12, 2018

Uh oh!

ghost Sep 13, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Sep 13, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomedi commented Sep 13, 2018

Uh oh!

frabert commented Sep 14, 2018

Uh oh!

frabert commented Sep 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frabert commented Sep 14, 2018

Uh oh!

frabert commented Sep 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Sep 19, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ghost Sep 13, 2018 •

edited by ghost

Loading

ghost Sep 13, 2018 •

edited by ghost

Loading

ghost Sep 19, 2018 •

edited by ghost

Loading

Jurigag Sep 22, 2018 •

edited

Loading