-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1004 Add Audio support to the engine #1321
base: main
Are you sure you want to change the base?
Conversation
|
8ee607d
to
362bfac
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1321 +/- ##
==========================================
- Coverage 42.45% 42.19% -0.26%
==========================================
Files 410 416 +6
Lines 29260 29442 +182
==========================================
+ Hits 12421 12422 +1
- Misses 16839 17020 +181 ☔ View full report in Codecov by Sentry. |
e72f419
to
9a7bb18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
31b51bd
to
a7bae01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Reviewed the audio device part
b480460
to
52949f6
Compare
2b9a02d
to
d30fa15
Compare
d30fa15
to
9c844c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/2)
CUBOS_REFLECT; | ||
|
||
private: | ||
cubos::core::al::Source source; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid case style for private member source
cubos::core::al::Source source; | |
cubos::core::al::Source mSource; |
bool active = false; //< Is the listener active? | ||
|
||
private: | ||
cubos::core::al::Listener listener; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid case style for private member listener
cubos::core::al::Listener listener; | |
cubos::core::al::Listener mListener; |
|
||
#include <cubos/engine/assets/bridges/file.hpp> | ||
#include <cubos/engine/audio/audio.hpp> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass by value and use std::move
#include <utility> | |
/// @brief Constructs a bridge. | ||
AudioBridge(std::shared_ptr<cubos::core::al::AudioContext> context) | ||
: FileBridge(core::reflection::reflect<Audio>()) | ||
, mContext(context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass by value and use std::move
, mContext(context) | |
, mContext(std::move(context)) |
|
||
#include <cubos/engine/assets/bridges/file.hpp> | ||
#include <cubos/engine/audio/audio.hpp> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter context
is passed by value and only copied once; consider moving it to avoid unnecessary copies
#include <utility> | |
/// @brief Constructs a bridge. | ||
AudioBridge(std::shared_ptr<cubos::core::al::AudioContext> context) | ||
: FileBridge(core::reflection::reflect<Audio>()) | ||
, mContext(context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter context
is passed by value and only copied once; consider moving it to avoid unnecessary copies
, mContext(context) | |
, mContext(std::move(context)) |
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use = default
to define a trivial destructor
{ | |
} | |
= default; |
public: | ||
MiniaudioDevice(ma_context& context, const std::string& deviceName, ma_uint32 listenerCount) | ||
: mContext(context) | ||
, mListeners() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initializer for member mListeners
is redundant
, mListeners() | |
, |
|
||
for (ma_uint32 i = 0; i < playbackDeviceCount; i++) | ||
{ | ||
if (pPlaybackDeviceInfos[i].isDefault) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implicit conversion ma_bool32
(aka unsigned int
) -> bool
if (pPlaybackDeviceInfos[i].isDefault) | |
if (pPlaybackDeviceInfos[i].isDefault != 0u) |
/// @brief Audio buffer. | ||
cubos::core::al::Buffer buffer; | ||
|
||
explicit Audio(std::shared_ptr<cubos::core::al::AudioContext>, core::memory::Stream& stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all parameters should be named in a function
explicit Audio(std::shared_ptr<cubos::core::al::AudioContext>, core::memory::Stream& stream); | |
explicit Audio(std::shared_ptr<cubos::core::al::AudioContext> /*context*/, core::memory::Stream& stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (2/2)
return core::reflection::Type::create("cubos::engine::Audio"); | ||
} | ||
|
||
cubos::engine::Audio::Audio(std::shared_ptr<cubos::core::al::AudioContext> context, core::memory::Stream& stream) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the parameter context
is copied for each invocation but only used as a const reference; consider making it a const reference
cubos::engine::Audio::Audio(std::shared_ptr<cubos::core::al::AudioContext> context, core::memory::Stream& stream) | |
cubos::engine::Audio::Audio(const std::shared_ptr<cubos::core::al::AudioContext>& context, core::memory::Stream& stream) |
/// @brief Audio buffer. | ||
cubos::core::al::Buffer buffer; | ||
|
||
explicit Audio(std::shared_ptr<cubos::core::al::AudioContext>, core::memory::Stream& stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the parameter context
is copied for each invocation but only used as a const reference; consider making it a const reference
explicit Audio(std::shared_ptr<cubos::core::al::AudioContext>, core::memory::Stream& stream); | |
explicit Audio(const std::shared_ptr<cubos::core::al::AudioContext>&, core::memory::Stream& stream); |
operator delete(contents); | ||
} | ||
|
||
cubos::engine::Audio::~Audio() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use = default
to define a trivial destructor
} | ||
|
||
cubos::engine::Audio::Audio(Audio&& other) noexcept | ||
: buffer(other.buffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move constructor initializes class member by calling a copy constructor
Co-Authored-By: João Miguel Nogueira <[email protected]>
Co-Authored-By: João Miguel Nogueira <[email protected]>
Co-Authored-By: João Miguel Nogueira <[email protected]>
9c844c0
to
813e49e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
@@ -0,0 +1,26 @@ | |||
#include <cubos/core/log.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubos/core/log.hpp
file not found
if (audio.buffer == nullptr) | ||
{ | ||
return false; | ||
} | ||
assets.store(handle, std::move(audio)); | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant boolean literal in conditional return statement
if (audio.buffer == nullptr) | |
{ | |
return false; | |
} | |
assets.store(handle, std::move(audio)); | |
return true; | |
return static_cast<bool>(audio.buffer != nullptr); |
@@ -1,7 +1,7 @@ | |||
#define MINIAUDIO_IMPLEMENTATION | |||
#include <cubos/core/al/miniaudio_context.hpp> | |||
#include <cubos/core/log.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubos/core/log.hpp
file not found
@@ -0,0 +1,43 @@ | |||
#include <cubos/core/log.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubos/core/log.hpp
file not found
Description
Include a summary of the changes here.
Checklist