EFX environment and reverb effect. Closes #697. #702

ArtemPopof · 2019-02-16T11:33:02Z

Some classes to support EFX and realisation of a reverb.
Closes #697

ArtemPopof · 2019-02-16T11:39:39Z

It can be used like this:

// load random sound
size_t index = game->getWorld()->sound.createSfxInstance(555);

auto reverb = std::shared_ptr<SoundEffect>(new ReverbEffect);
auto slot = std::shared_ptr<EffectSlot>(new EffectSlot());
Sound& sound = game->getWorld()->sound.getSoundRef(index);
slot->attachEffect(reverb);
sound.attachToEffectSlot(slot);

game->getWorld()->sound.playSfx(index, {0.0f, 0.0f, 0.0f});

Multiple Sound instances can be attached to one effect slot. Effect slot can be binded to one effect.

codecov · 2019-02-16T11:45:09Z

Codecov Report

Merging #702 into master will increase coverage by 0.10%.
The diff coverage is 36.64%.

@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
+ Coverage   18.11%   18.22%   +0.10%     
==========================================
  Files         247      253       +6     
  Lines       22633    22765     +132     
  Branches     5808     5824      +16     
==========================================
+ Hits         4101     4149      +48     
- Misses      17395    17478      +83     
- Partials     1137     1138       +1

Impacted Files	Coverage Δ
rwengine/src/audio/EffectSlot.cpp	`0.00% <0.00%> (ø)`
rwengine/src/audio/Sound.hpp	`0.00% <ø> (-25.00%)`	⬇️
rwengine/src/audio/SoundBuffer.cpp	`44.61% <0.00%> (-4.54%)`	⬇️
rwengine/src/audio/SoundEffect.hpp	`0.00% <0.00%> (ø)`
rwengine/src/render/ObjectRenderer.cpp	`11.29% <0.00%> (ø)`
rwgame/states/IngameState.cpp	`0.00% <ø> (ø)`
rwengine/src/audio/ReverbEffect.cpp	`6.89% <6.89%> (ø)`
rwengine/src/audio/Sound.cpp	`46.34% <44.44%> (-0.54%)`	⬇️
rwengine/src/audio/SoundManager.cpp	`29.08% <57.89%> (+2.04%)`	⬆️
rwengine/src/audio/SoundEffect.cpp	`73.33% <71.42%> (ø)`
... and 10 more

Continue to review full report at Codecov.

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

JorgenPo · 2019-02-16T11:44:40Z

rwengine/src/audio/EffectSlot.cpp

+#include "SoundEffect.hpp"
+#include "OpenAlExtensions.hpp"
+
+EffectSlot::EffectSlot() {


Consider using an initialization list

JorgenPo · 2019-02-16T11:53:17Z

rwengine/src/audio/EffectSlot.cpp

+}
+
+void EffectSlot::setGain(float gain) {
+    alAuxiliaryEffectSlotf(slotId, AL_EFFECTSLOT_GAIN, this->gain = gain);


Split a variable assigning and a function invocation

JorgenPo · 2019-02-16T11:56:02Z

rwengine/src/audio/EffectSlot.hpp

+     */
+    std::shared_ptr<SoundEffect> effect;
+
+    ALuint slotId;


Or you can set members to defaults right here

JorgenPo · 2019-02-16T12:10:56Z

rwengine/src/audio/SoundEffect.hpp

+     * @param type OpenAl specific effect type.
+     */
+    SoundEffect(ALint type);
+    ~SoundEffect();


You should make the destructor virtual:
https://stackoverflow.com/questions/461203/when-to-use-virtual-destructors

JorgenPo · 2019-02-16T12:11:47Z

rwengine/src/audio/ReverbEffect.hpp

+public:
+    ReverbEffect();
+
+    void	setDensity             ( float density = 1.0f   );


Remove table-like formatting?

I think there should be some comments describing parameters that functions are setting

JorgenPo · 2019-02-16T12:14:14Z

rwengine/src/audio/ReverbEffect.cpp

+    alEffectf(id, AL_REVERB_LATE_REVERB_GAIN, g);
+}
+
+void	ReverbEffect :: setLateReverbDelay (float t) {


Something wrong with a formatting

JorgenPo · 2019-02-16T12:19:33Z

rwengine/src/audio/Sound.hpp

@@ -42,6 +43,8 @@ struct Sound {

    void setMaxDistance(float maxDist);

+    void attachToEffectSlot(const std::shared_ptr<EffectSlot> effectSlot);


You should pass a shared_ptr by const reference or by value and use std::move

JorgenPo · 2019-02-16T12:20:37Z

rwengine/src/audio/SoundBuffer.cpp

@@ -83,3 +86,11 @@ void SoundBuffer::setGain(float gain) {
 void SoundBuffer::setMaxDistance(float maxDist) {
    alCheck(alSourcef(source, AL_MAX_DISTANCE, maxDist));
 }
+
+void SoundBuffer::attachToEffectSlot(const std::shared_ptr<EffectSlot> slot) {
+    alCheck(alSource3i(source, AL_AUXILIARY_SEND_FILTER, slot->getSlotId(), slot->getSlotNumber(), AL_FILTER_NULL));


Use const reference here

JorgenPo · 2019-02-16T12:21:43Z

rwengine/src/audio/SoundBuffer.cpp

+}
+
+void SoundBuffer::detachFromEffectSlot(const std::shared_ptr<EffectSlot> slot) {
+    alCheck(alSource3i (source, AL_AUXILIARY_SEND_FILTER, AL_EFFECTSLOT_NULL, slot->getSlotNumber(), AL_FILTER_NULL));


See comment above

JorgenPo · 2019-02-16T12:23:51Z

rwengine/src/audio/SoundManager.cpp

+
+    initEfxFunctionPointers();
+
+    SoundEffect effect(0);


Have no idea what zero mean... Maybe define some constants?

It's just a test code, deleted.

ghost · 2019-02-16T12:30:15Z

rwengine/src/audio/ReverbEffect.h

@@ -0,0 +1,11 @@
+#ifndef REVERBEFFECT_H


I guess this file is unneeded.

ghost · 2019-02-16T12:31:50Z

rwengine/src/audio/EffectSlot.cpp

+EffectSlot::EffectSlot() {
+    alGenAuxiliaryEffectSlots(1, &slotId);
+
+    effect = nullptr;


default member initializer

ghost · 2019-02-16T22:07:05Z

I've kept possibility to manually operate on basic blocks Sound, SoundSource, SoundBuffer. It's useful to create simple tidy tests. But engine should use neat interface of SoundManager.

It (should) make usage of Sounds simpler and prevent code duplication.

So I think SoundEffect, EffectSlot should be squashed together. Instances of that class stored in vector (which is member of SoundManager).

So code would become something:

// Somewhere in header defined
enum class SoundEffectType {
    None,
    Reverb
};

// load random sound
size_t index = game->getWorld()->sound.createSfxInstance(555);

game->getWorld()->sound.playSfx(index, SoundEffectType::None, {0.0f, 0.0f, 0.0f}); // All logic are done behind

I look forward to your opinion about this idea. ;)

minor refactoring

ArtemPopof · 2019-02-18T21:19:41Z

Suggested improvements look good to me.

SoundEffect and SoundSlot is merged SoundManager contains all pre-initialized effects

ghost · 2019-02-21T21:48:09Z

rwengine/src/audio/Sound.hpp

@@ -18,8 +21,10 @@ struct Sound {
    std::shared_ptr<SoundSource> source;
    std::unique_ptr<SoundBuffer> buffer;

+    std::shared_ptr<SoundEffect> effect;


raw ptr is enough

ghost · 2019-02-21T21:49:04Z

rwengine/src/audio/Sound.cpp

@@ -2,6 +2,13 @@

 #include "audio/SoundBuffer.hpp"

+Sound::~Sound()
+{


Formatting. Use clang-format with our config file.

ghost · 2019-02-21T21:49:14Z

rwengine/src/audio/Sound.cpp

@@ -46,6 +53,12 @@ void Sound::setMaxDistance(float maxDist) {
    buffer->setMaxDistance(maxDist);
 }

+void Sound::enableEffect(std::shared_ptr<SoundEffect> effect)
+{


ghost · 2019-02-21T21:50:09Z

rwengine/src/render/ObjectRenderer.cpp

@@ -307,10 +307,11 @@ void ObjectRenderer::renderVehicle(VehicleObject* vehicle,

 void ObjectRenderer::renderPickup(PickupObject* pickup, RenderList& outList) {
    if (!pickup->isEnabled()) return;
-
+
+    static constexpr float kRotationSpeedCoeff = 3.0f;


imho it's better to avoid unnecessary code in this pull.

ghost · 2019-02-21T21:52:14Z

rwengine/src/audio/SoundManager.hpp

@@ -117,6 +123,9 @@ class SoundManager {

    /// Multiplier for music and cutscenes audio
    float _musicVolume = 1.f;
+
+    /// Available sound effects for sfx
+    std::vector<std::shared_ptr<SoundEffect>> soundEffects;


std::vector<std::unique_ptr<SoundEffect>> soundEffects; should be enough. (pass raw ptr to Sounds)

ghost · 2019-02-21T21:52:36Z

rwengine/src/audio/SoundManager.cpp

+    playSfx(name, position, SoundEffect::Type::Reverb, looping, maxDist);
+}
+
+void SoundManager::playSfx(size_t name, const glm::vec3& position, SoundEffect::Type effectType, bool looping, int maxDist) {


ghost · 2019-02-21T21:58:53Z

rwengine/src/audio/SoundManager.cpp

+
+    // first effect is placeholder
+    // it allows index to be like in enum
+    soundEffects.push_back(nullptr);


Maybe it's better to remove None from enum and add bool enabled. I think there are too many parameters for sfx to pass them around. Probably we should add some abstraction/wrapper later.

ghost · 2019-02-21T22:00:38Z

rwengine/src/audio/SoundManager.cpp

+    // first effect is placeholder
+    // it allows index to be like in enum
+    soundEffects.push_back(nullptr);
+    soundEffects.push_back(std::make_shared<ReverbEffect>());


nit(not need to do in this pull): lazy initialization / do when there's need

ghost · 2019-02-21T22:01:35Z

rwengine/src/audio/SoundBuffer.cpp

@@ -83,3 +85,11 @@ void SoundBuffer::setGain(float gain) {
 void SoundBuffer::setMaxDistance(float maxDist) {
    alCheck(alSourcef(source, AL_MAX_DISTANCE, maxDist));
 }
+
+void SoundBuffer::enableEffect(std::shared_ptr<SoundEffect> effect) {
+    alCheck(alSource3i(source, AL_AUXILIARY_SEND_FILTER, (ALint) effect->getSlotId(), effect->getSlotNumber(), AL_FILTER_NULL));


ghost · 2019-02-21T22:01:43Z

rwengine/src/audio/SoundBuffer.cpp

+}
+
+void SoundBuffer::disableEffect(std::shared_ptr<SoundEffect> effect) {
+    alCheck(alSource3i (source, AL_AUXILIARY_SEND_FILTER, AL_EFFECTSLOT_NULL, effect->getSlotNumber(), AL_FILTER_NULL));


ghost · 2019-02-21T22:09:19Z

rwengine/src/audio/Sound.hpp

@@ -1,9 +1,12 @@
 #ifndef _RWENGINE_SOUND_HPP_
 #define _RWENGINE_SOUND_HPP_

+#include <audio/SoundEffect.hpp>


forward declaration

ghost · 2019-02-21T22:19:33Z

rwengine/src/audio/EffectSlot.hpp

+ * Many sound sources can be attached to one slot.
+ * Different effects can be binded to this (e.g reverb, delay).
+ */
+class EffectSlot {


I have problems with understanding connection between this class and SoundEffect. I mean where is EffectSlot created, used, deleted. Because instances of EffectSlot and SoundEffect are in relation 1 to 1. I think solution when one of them contains/owns instances of second would be good enough.

ghost · 2019-02-27T20:16:36Z

Do you need some help with changes? You can catch me on IRC. ;)

ghost · 2019-03-17T23:17:45Z

Do you plan to continue work on this pull?

ghost · 2019-05-25T12:42:29Z

Bump?

ArtemPopof and others added 5 commits February 13, 2019 16:11

Pickups speed adjusted

be920d3

Environment for OpenAL EFX added reverb effect added

8bd8e17

requested changes

cb40cc3

Variable name adjusted

958b7f7

Merge remote-tracking branch 'origin/issue_697'

7a22c06

Removed some extra spaces

c464756

JorgenPo suggested changes Feb 16, 2019

View reviewed changes

ArtemPopof added 2 commits February 16, 2019 07:27

Minor formatting improvements

99429f3

Remove duplication

519536d

ghost reviewed Feb 16, 2019

View reviewed changes

Deleted useless line

8496f6a

ArtemPopof added 6 commits February 18, 2019 14:50

Minor format and refactor

7e2f208

Merge branch 'issue_697'

60ac44a

Extracted variable assigning from fucntion call

b673ab6

memory cleanup improvements

1dbabd1

minor refactoring

made SoundEffect constructor virtual

dd0dd44

argument pass method improvements

ce7c45f

ArtemPopof and others added 2 commits February 20, 2019 11:26

Refactor of EFX classes

1b4cc61

SoundEffect and SoundSlot is merged SoundManager contains all pre-initialized effects

Deleted test lines that don't belong to this commit.

960d1a1

ghost reviewed Feb 21, 2019

View reviewed changes

		@@ -42,6 +43,8 @@ struct Sound {

		void setMaxDistance(float maxDist);

		void attachToEffectSlot(const std::shared_ptr<EffectSlot> effectSlot);

EFX environment and reverb effect. Closes #697. #702

Are you sure you want to change the base?

EFX environment and reverb effect. Closes #697. #702

Conversation

ArtemPopof commented Feb 16, 2019 • edited

ArtemPopof commented Feb 16, 2019 • edited

codecov bot commented Feb 16, 2019 • edited

Codecov Report

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

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 Feb 16, 2019 • edited by ghost

Choose a reason for hiding this comment

ghost commented Feb 16, 2019

ArtemPopof commented Feb 18, 2019 • edited

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

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 Feb 27, 2019

ghost commented Mar 17, 2019

ghost commented May 25, 2019

ArtemPopof commented Feb 16, 2019 •

edited

ArtemPopof commented Feb 16, 2019 •

edited

codecov bot commented Feb 16, 2019 •

edited

ghost Feb 16, 2019 •

edited by ghost

ArtemPopof commented Feb 18, 2019 •

edited