Implement methods for sfx, and refactor Sound system #558

ghost · 2018-07-26T21:30:02Z

SoundBuffer and SoundSource got their own files.
Also has been added hardcoded metadata of sfx. (Like in origin)

It's continuation of backporting code from #420.

madebr

Looks good in general.

madebr · 2018-07-27T01:28:08Z

rwengine/src/audio/SfxParameters.hpp

+/// @todo some of them should return
+/// random(?) number in range
+/// see comment in second column
+constexpr SoundInstanceData sfxData[] = {


Can this be moved to a cpp file?

madebr · 2018-07-27T01:28:31Z

rwengine/src/audio/Sound.hpp

+        buffer->stop();
+    }
+
+    void setPosition(const glm::vec3 position) {


const glm::vec3 &position (same remark for other arguments)
Or is this more efficient? (I would let the compiler decide and use references)

madebr · 2018-07-27T01:29:31Z

rwengine/src/audio/Sound.hpp

+        buffer->setLooping(looping);
+    }
+
+    void setPitch(const float  pitch) {


Is the const really necessary? Same remark goes to all const arguments in this pr.

madebr · 2018-07-27T01:31:18Z

rwengine/src/audio/SoundManager.cpp

+    Sound* sound = nullptr;
+    auto soundRef = sfx.find(index);
+
+    //Let's try reuse buffor


// Try to reuse the buffer (buffor -> buffer: general remark for this pr)

madebr · 2018-07-27T01:36:16Z

rwengine/src/audio/SfxParameters.hpp

+    {45, 413, 80},
+    {46, 414, 30},
+    {47, 414, 30},
+    {48, 415, 30},                  ///	Marco's Bistro (no sound)


The comments do not have consistent layout + should not contain tabs.

I forgot to reformat it. (shame)

madebr · 2018-07-27T01:38:45Z

rwengine/src/audio/SfxParameters.hpp

+#define _RWENGINE_SFX_PARAMETERS_HPP_
+
+/// Nr of existing sfx sounds
+constexpr size_t kNrOfAllSfx = 3032;


What is the source of this number? Isn't this data dependent?

Taken from gtamodding.

ghost · 2018-07-27T19:01:34Z

Done. ;)

Lihis

Nit: Is commenting with /// allowed? Personally I would find it better to use /* */ when multiline comment is needed.

Lihis · 2018-08-01T15:29:27Z

rwengine/src/audio/SfxParameters.cpp

+    {106, 176, 50},            /// bullet hit ground
+    {107, 177, 50},
+    {108, 178, 50},
+    {110, 389, 80},  /// silent, replace the SFX or edit the memory to a


nit: inconsistent alignment of comments in this array.

Lihis · 2018-08-01T15:43:31Z

rwengine/src/audio/SoundManager.cpp

+    auto ref = buffers.find(name);
+    if (ref != buffers.end()) {
+        return ref->second;
+    } else {  // Hmm, we should reload this


nit: I think this comment is unnecessary

Lihis · 2018-08-01T15:50:42Z

rwengine/src/audio/SoundManager.cpp

+    Sound* sound = nullptr;
+    auto soundRef = sfx.find(index);
+
+    // Let's try reuse buffer


nit: I would rather phrase this something like Try to reuse first available buffer and remove the comment from the loop below.

Lihis · 2018-08-01T15:53:07Z

rwengine/src/audio/SoundManager.cpp

    }
+    // Hmmm, nothing, we should create new buffer


nit: rephrase to something that indicates that there was no available buffer to be reused so that's why new one is created.

Lihis · 2018-08-01T16:14:13Z

rwengine/src/audio/SoundManager.hpp

+#include "audio/SfxParameters.hpp"
+
+/// Game's sound manager.
+/// It handles all staff conntected with sounds.


Typos (stuff and connected)

Lihis · 2018-08-01T16:19:44Z

rwengine/src/audio/SoundSource.cpp

+
+                if (len >= 0 && gotFrame) {
+                    // Write samples to audio buffer
+


Unnecessary empty line

Lihis · 2018-08-01T16:29:00Z

rwengine/src/audio/SfxParameters.hpp

+#include <cstddef>
+
+/// Nr of existing sfx sounds
+constexpr size_t kNrOfAllSfx = 3032;


Like @madebr noted previously this looks to depend of the game and this is correct only for GTA III?

Yeah. I'll rename it.

I think Todo might be enough at this point (as also the SfxData array in SfxParameters.cpp is valid only for GTAIII?)

Lihis · 2018-08-04T08:02:48Z

rwengine/src/audio/SfxParameters.cpp

+    {34, 405, 30},
+    {35, 405, 30},
+    {36, 407 /*-408*/,
+     30},                     /// 407 plays continuously


This can be on same line (at line 94).

Lihis · 2018-08-04T08:04:22Z

rwengine/src/audio/SfxParameters.cpp

+    {60, 406, 30},            /// Salvatore's place
+    {61, 406, 80},
+    {62, 432 /*-434*/,
+     20},                     /// South of Sex Club Seven


This could fit into line 123.

Lihis · 2018-08-04T08:04:47Z

rwengine/src/audio/SfxParameters.cpp

+                              /// and intermittently
+    {63, 432 /*-434*/, 80},
+    {64, 435 /*-437*/,
+     20},                     /// East of Portland


This could fit into line 130.

ghost · 2018-08-05T12:05:19Z

rwengine/src/audio/SfxParameters.cpp

+                                         return data.id == scriptId;
+                                     });
+    if (finded != std::end(sfxData)) {
+        return finded->sfx;


Should be range.

ghost · 2018-08-05T19:53:08Z

Up. :)

danhedron · 2018-08-05T23:40:06Z

rwengine/src/audio/SoundManager.cpp

-        }
-        av_free_packet(&readingPacket);
+        sound->source = std::make_shared<SoundSource>();
+        sound->source->loadSfx(path, i);


loadSfx has too much responsibility.

Since it's called in a loop here it would make more sense to create a LoaderSDT here and pass it in.

danhedron · 2018-08-05T23:50:05Z

rwengine/src/audio/SoundSource.cpp

+                if (receiveFrame == 0 && sendPacket == 0) {
+                    // Write samples to audio buffer
+
+                    for (size_t i = 0;


Is there any way to extract this inner loop so it can be shared between all of the frame handling paths?

Discussed on IRC: task to do in later pull.

ghost · 2018-08-06T19:58:45Z

Small up. Reusing instance of LoaderSDT and cleanup of headers.

ghost · 2018-08-06T20:09:10Z

rwengine/src/audio/SoundSource.cpp

+    }
+
+
+    sdt.load(path / "audio/sfx");


It should be done only once in ctor of SoundManager. @madebr can/should we keep path in SoundManager?

To do later.

ghost · 2018-08-06T20:13:32Z

rwengine/src/audio/SoundManager.cpp

+
+size_t SoundManager::createSfxInstance(size_t index) {
+    Sound* sound = nullptr;
+    auto soundRef = sfx.find(index);


Here we can check, if sound is loaded, and if there's need load it. But it would require storing game path in SoundManager.

To do later.

danhedron · 2018-08-07T21:40:40Z

@ShFil119 Do you intend to take care of those comments in this PR?

ghost · 2018-08-07T21:57:19Z

I'll take care about these features later.

danhedron · 2018-08-07T22:04:17Z

rwengine/src/audio/SfxParameters.cpp

+};
+}  // namespace
+
+int convertScriptIndexIntoSfx(int scriptId) {


Does SoundInstanceData need to be hidden? A simpler interface could just be const SoundInstanceData& getScriptSoundInstanceData(int scriptId).

Not sure if we need/want this interface in script. If you want, I can change.

Yeah. This seems to be unused in this PR, could you move it into the next PR?

SoundBuffer and SoundSource got their own files.

madebr reviewed Jul 27, 2018

View reviewed changes

ghost changed the title ~~Implement methods for sfx, and refactor Sound system~~ [To rebase]Implement methods for sfx, and refactor Sound system Jul 29, 2018

ghost changed the title ~~[To rebase]Implement methods for sfx, and refactor Sound system~~ Implement methods for sfx, and refactor Sound system Jul 29, 2018

Lihis reviewed Aug 1, 2018

View reviewed changes

Lihis suggested changes Aug 4, 2018

View reviewed changes

ghost commented Aug 5, 2018

View reviewed changes

danhedron reviewed Aug 5, 2018

View reviewed changes

ghost commented Aug 6, 2018

View reviewed changes

danhedron reviewed Aug 7, 2018

View reviewed changes

Implement methods for sfx, and refactor Sound system

11bc35c

SoundBuffer and SoundSource got their own files.

danhedron approved these changes Aug 8, 2018

View reviewed changes

Lihis approved these changes Aug 8, 2018

View reviewed changes

danhedron merged commit 844fdf8 into rwengine:master Aug 8, 2018

ghost deleted the soundRefactor branch August 8, 2018 23:20

ghost mentioned this pull request Aug 11, 2018

State of implementing sfx sounds #581

Open

13 tasks

Implement methods for sfx, and refactor Sound system #558

Implement methods for sfx, and refactor Sound system #558

Conversation

ghost commented Jul 26, 2018

madebr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 27, 2018 • edited by ghost 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 27, 2018

Lihis left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 1, 2018 • edited by ghost 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

ghost commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 6, 2018

ghost Aug 6, 2018 • edited by ghost 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

danhedron commented Aug 7, 2018

ghost commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 27, 2018 •

edited by ghost

Loading

Lihis left a comment •

edited

Loading

ghost Aug 1, 2018 •

edited by ghost

Loading

ghost Aug 6, 2018 •

edited by ghost

Loading