Merge pull request #67 from Laguna1989/FEATURE_ReviewFindings

Feature review findings

Author: Laguna1989

CMakeLists.txt

@@ -1,5 +1,5 @@
 cmake_minimum_required(VERSION 3.19)
-project(OpenALpp)
+project(OpenALpp VERSION 0.1.0)
 
 option(OALPP_ENABLE_UNIT_TESTS "Enable unit tests" ON)
 option(OALPP_ENABLE_INTEGRATION_TESTS "Enable integration tests" ON)

README.md

@@ -25,10 +25,24 @@ Code Example
 
 SoundContext ctx;
 SoundData buffer { "audio.mp3" };
-Sound snd { buffer, ctx };
+Sound snd { buffer };
 snd.play();
+
+while (snd.isPlaying()) {
+    snd.update();
+}
 ```
 
+Common Pitfalls
+------------
+
+* Sound has a dependency to the `SoundData` that is passed in the constructor. You need to keep the `SoundData` alive as
+  long as any `Sound` might access it.
+    * You can bundle `SoundData` and `Sound` together in your implementation.
+    * Alternatively you can write your own `SoundDataManager` to avoid creating multiple `SoundData`s for the same file.
+* Your sound will stop after some seconds, even if the audio file contains a longer sound. `Sound`s do not update
+  themselves. You need to call `update()` regularly.
+
 How to include OpenALpp in your project
 ---------------------------------------
 
@@ -52,7 +66,7 @@ CMake Options
 
 * `OALPP_ENABLE_UNIT_TESTS` - Enable unit tests - default `ON`
 * `OALPP_ENABLE_INTEGRATION_TESTS` - Enable integration test - default `ON`
-* `OALPP_STATIC_LIBRARY` - Build OpenALpp and dependencies as static library - default `OFF`
+* `OALPP_STATIC_LIBRARY` - Build OpenALpp and dependencies as static library - default `ON`
 
 Compiler compatibility
 ----------------------
@@ -68,4 +82,4 @@ Dependencies
 * CMake 3.19
 * One of the compatible compilers
 
-All other dependencies (openal-soft and libnyquist) are automatically fetched via CMake.
+All other dependencies (`openal-soft` and `libnyquist`) are automatically fetched via CMake.

impl/oalpp/audio_exceptions.hpp

@@ -2,32 +2,34 @@
 #define OPENALPP_AUDIO_EXCEPTIONS_HPP
 
 #include <stdexcept>
+#include <string>
+#include <utility>
 
 namespace oalpp {
 class AudioException : public std::exception {
 public:
-    explicit AudioException(char const* const message)
-        : m_message { message }
+    explicit AudioException(std::string message)
+        : m_message { std::move(message) }
     {
     }
 
-    [[nodiscard]] char const* what() const noexcept override { return m_message; }
+    [[nodiscard]] char const* what() const noexcept override { return m_message.c_str(); }
 
 private:
-    char const* m_message;
+    std::string m_message;
 };
 
 class AudioSystemException : public std::exception {
 public:
-    explicit AudioSystemException(char const* const message)
-        : m_message { message }
+    explicit AudioSystemException(std::string message)
+        : m_message { std::move(message) }
     {
     }
 
-    [[nodiscard]] char const* what() const noexcept override { return m_message; }
+    [[nodiscard]] char const* what() const noexcept override { return m_message.c_str(); }
 
 private:
-    char const* m_message;
+    std::string m_message;
 };
 
 } // namespace oalpp

impl/oalpp/position.cpp

@@ -0,0 +1,10 @@
+#include "position.hpp"
+
+namespace oalpp {
+
+bool operator==(Position const& lhs, Position const& rhs)
+{
+    return lhs.x == rhs.x && lhs.y == rhs.y && lhs.z == rhs.z;
+}
+
+} // namespace oalpp

impl/oalpp/position.hpp

@@ -0,0 +1,16 @@
+#ifndef OPENALPP_POSITION_HPP
+#define OPENALPP_POSITION_HPP
+
+namespace oalpp {
+
+struct Position {
+    float x { 0.0f };
+    float y { 0.0f };
+    float z { 0.0f };
+};
+
+bool operator==(Position const& lhs, Position const& rhs);
+
+} // namespace oalpp
+
+#endif // OPENALPP_POSITION_HPP

impl/oalpp/sound.cpp

@@ -1,6 +1,5 @@
 #include "sound.hpp"
 #include "audio_exceptions.hpp"
-#include "sound_context.hpp"
 #include <cmath>
 #include <stdexcept>
 
@@ -13,83 +12,64 @@ Sound::Sound(SoundDataInterface const& soundData)
         m_format = AL_FORMAT_STEREO_FLOAT32;
     }
 
-    createSource();
-
-    // Create and fill buffers
-    createBuffers();
-    fillBufferFromStart();
+    initSourceAndBuffers();
 
     auto const errorIfAny = alGetError();
     if (errorIfAny != AL_NO_ERROR) {
-        auto const errorMessage
-            = "Could not create OpenAL soundData, error code: " + std::to_string(errorIfAny);
-        throw oalpp::AudioException { errorMessage.c_str() };
+        throw oalpp::AudioException { "Could not create OpenAL soundData, error code: "
+            + std::to_string(errorIfAny) };
     }
 }
-
-Sound::~Sound()
-{
-    deleteSource();
-    deleteBuffers();
-}
-
-void Sound::deleteBuffers()
+void Sound::initSourceAndBuffers()
 {
-    alDeleteBuffers(static_cast<ALsizei>(m_bufferIds.size()), m_bufferIds.data());
-}
+    // Create source
+    alGenSources(1, &m_sourceId);
+    alSourcef(m_sourceId, AL_PITCH, 1.0f);
+    alSourcef(m_sourceId, AL_GAIN, m_volume);
+    alSource3f(m_sourceId, AL_POSITION, 0.0f, 0, -1.0f);
+    alSource3f(m_sourceId, AL_VELOCITY, 0.0f, 0.0f, 0.0f);
+    alSourcei(m_sourceId, AL_LOOPING, AL_FALSE);
+    alSourcef(m_sourceId, AL_ROLLOFF_FACTOR, 0.0f);
+    alSourcei(m_sourceId, AL_SOURCE_RELATIVE, true);
 
-void Sound::deleteSource() const { alDeleteSources(1, &m_sourceId); }
+    alGenBuffers(static_cast<ALsizei>(m_bufferIds.size()), m_bufferIds.data());
 
-void Sound::fillBufferFromStart()
-{
     m_cursor = 0;
     for (auto const& bufferId : m_bufferIds) {
         selectSamplesForBuffer(bufferId);
     }
 }
 
-void Sound::createBuffers()
-{
-    alGenBuffers(static_cast<ALsizei>(m_bufferIds.size()), m_bufferIds.data());
-}
+Sound::~Sound() { deleteSourceAndBuffers(); }
 
-void Sound::createSource()
+void Sound::deleteSourceAndBuffers()
 {
-    // Create source
-    alGenSources(1, &m_sourceId);
-    alSourcef(m_sourceId, AL_PITCH, 1.0f);
-    alSourcef(m_sourceId, AL_GAIN, m_volume);
-    alSource3f(m_sourceId, AL_POSITION, 0.0f, 0, -1.0f);
-    alSource3f(m_sourceId, AL_VELOCITY, 0.0f, 0.0f, 0.0f);
-    alSourcei(m_sourceId, AL_LOOPING, AL_FALSE);
-    alSourcef(m_sourceId, AL_ROLLOFF_FACTOR, 0.0f);
-    alSourcei(m_sourceId, AL_SOURCE_RELATIVE, true);
+    alDeleteSources(1, &m_sourceId);
+    alDeleteBuffers(static_cast<ALsizei>(m_bufferIds.size()), m_bufferIds.data());
 }
 
 void Sound::play()
 {
     alSourcePlay(m_sourceId);
     auto const errorIfAny = alGetError();
     if (errorIfAny != AL_NO_ERROR) {
-        auto const errorMessage = "Could not play sound, error code: " + std::to_string(errorIfAny);
-        throw oalpp::AudioException { errorMessage.c_str() };
+        throw oalpp::AudioException { "Could not play sound, error code: "
+            + std::to_string(errorIfAny) };
     }
 }
 
 void Sound::stop()
 {
     alSourceStop(m_sourceId);
 
-    deleteSource();
-    deleteBuffers();
-    createSource();
-    createBuffers();
-    fillBufferFromStart();
+    deleteSourceAndBuffers();
+
+    initSourceAndBuffers();
 
     auto const errorIfAny = alGetError();
     if (errorIfAny != AL_NO_ERROR) {
-        auto const errorMessage = "Could not stop sound, error code: " + std::to_string(errorIfAny);
-        throw oalpp::AudioException { errorMessage.c_str() };
+        throw oalpp::AudioException { "Could not stop sound, error code: "
+            + std::to_string(errorIfAny) };
     }
 }
 
@@ -98,8 +78,8 @@ void Sound::pause()
     alSourcePause(m_sourceId);
     auto const errorIfAny = alGetError();
     if (errorIfAny != AL_NO_ERROR) {
-        auto const errorMessage = "Could not stop sound, error code: " + std::to_string(errorIfAny);
-        throw oalpp::AudioException { errorMessage.c_str() };
+        throw oalpp::AudioException { "Could not stop sound, error code: "
+            + std::to_string(errorIfAny) };
     }
 }
 
@@ -115,9 +95,7 @@ float Sound::getVolume() const { return m_volume; }
 void Sound::setVolume(float newVolume)
 {
     if (newVolume < 0 || newVolume > 1.0f) {
-        auto const errorMessage
-            = std::string { "Could not set volume value: " } + std::to_string(newVolume);
-        throw std::invalid_argument { errorMessage.c_str() };
+        throw std::invalid_argument { "Could not set volume value: " + std::to_string(newVolume) };
     }
     m_volume = newVolume;
     alSourcef(m_sourceId, AL_GAIN, newVolume);
@@ -126,35 +104,32 @@ void Sound::setVolume(float newVolume)
 void Sound::setPan(float newPan)
 {
     if (newPan < -1.0f || newPan > 1.0f) {
-        auto const errorMessage
-            = std::string { "Could not set pan value: " } + std::to_string(newPan);
-        throw std::invalid_argument { errorMessage.c_str() };
+        throw std::invalid_argument {
+            ("Could not set pan value: " + std::to_string(newPan)).c_str()
+        };
     }
 
-    setPosition(
-        std::array<float, 3> { newPan, 0, -static_cast<float>(sqrt(1.0f - newPan * newPan)) });
+    setPosition(Position { newPan, 0, -static_cast<float>(sqrt(1.0f - newPan * newPan)) });
 }
 
-std::array<float, 3> Sound::getPosition() const { return m_position; }
+Position Sound::getPosition() const { return m_position; }
 
-void Sound::setPosition(std::array<float, 3> const& newPos)
+void Sound::setPosition(Position const& newPosition)
 {
     if (m_format == AL_FORMAT_STEREO_FLOAT32) {
         throw oalpp::AudioException { "Could not set position on non-mono file" };
     }
 
-    m_position = newPos;
-    alSource3f(m_sourceId, AL_POSITION, newPos[0], newPos[1], newPos[2]);
+    m_position = newPosition;
+    alSource3f(m_sourceId, AL_POSITION, newPosition.x, newPosition.y, newPosition.z);
 }
 
 float Sound::getPitch() const { return m_pitch; }
 
 void Sound::setPitch(float const newPitch)
 {
     if (newPitch <= 0.0f) {
-        auto const errorMessage
-            = std::string { "Could not set pitch value: " } + std::to_string(newPitch);
-        throw std::invalid_argument { errorMessage.c_str() };
+        throw std::invalid_argument { "Could not set pitch value: " + std::to_string(newPitch) };
     }
     m_pitch = newPitch;
     alSourcef(m_sourceId, AL_PITCH, newPitch);
@@ -192,10 +167,12 @@ void Sound::update()
 
 void Sound::selectSamplesForBuffer(ALuint bufferId)
 {
-    if (m_cursor >= m_soundData.getSamples().size()) {
+    if (!hasDataToEnqueue()) {
         // do not queue any buffer
         return;
-    } else if (m_cursor + BUFFER_SIZE <= m_soundData.getSamples().size()) {
+    }
+
+    if (hasDataForFullBufferToEnqueue()) {
         // queue a full buffer
         enqueueSamplesToBuffer(bufferId, BUFFER_SIZE);
     } else {
@@ -210,6 +187,11 @@ void Sound::selectSamplesForBuffer(ALuint bufferId)
         }
     }
 }
+bool Sound::hasDataForFullBufferToEnqueue() const
+{
+    return m_cursor + BUFFER_SIZE <= m_soundData.getSamples().size();
+}
+bool Sound::hasDataToEnqueue() const { return m_cursor < m_soundData.getSamples().size(); }
 
 bool Sound::getIsLooping() const { return m_isLooping; }
 void Sound::setIsLooping(bool value) { m_isLooping = value; }

impl/oalpp/sound.hpp

@@ -2,17 +2,12 @@
 #define OPENALPP_SOUND_HPP
 
 #include "al.hpp"
+#include "position.hpp"
 #include "sound_data_interface.hpp"
 #include <array>
-#include <memory>
-#include <string>
-#include <vector>
 
 namespace oalpp {
 
-// fwd decl.
-class SoundContext;
-
 class Sound {
 public:
     explicit Sound(SoundDataInterface const& soundData);
@@ -26,13 +21,13 @@ class Sound {
     float getVolume() const;
     void setVolume(float newVolume);
 
-    std::array<float, 3> getPosition() const;
+    Position getPosition() const;
 
-    void setPosition(std::array<float, 3> const& newPos);
+    void setPosition(Position const& newPosition);
     void setPan(float newPan);
 
     float getPitch() const;
-    void setPitch(float const newPitch);
+    void setPitch(float newPitch);
 
     bool getIsLooping() const;
     void setIsLooping(bool value);
@@ -59,18 +54,18 @@ class Sound {
     std::size_t m_cursor { 0 };
 
     float m_volume { 1.0f };
-    std::array<float, 3> m_position { 0.0f, 0.0f, -1.0f };
+    Position m_position { 0.0f, 0.0f, -1.0f };
     float m_pitch { 1.0f };
 
     bool m_isLooping { false };
 
     void enqueueSamplesToBuffer(ALuint buffer, size_t samplesToQueue);
     void selectSamplesForBuffer(ALuint bufferId);
-    void createSource();
-    void createBuffers();
-    void fillBufferFromStart();
-    void deleteSource() const;
-    void deleteBuffers();
+    bool hasDataToEnqueue() const;
+    bool hasDataForFullBufferToEnqueue() const;
+
+    void initSourceAndBuffers();
+    void deleteSourceAndBuffers();
 };
 
 } // namespace oalpp