Skip to content
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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

diogomsmiranda
Copy link
Contributor

Description

Include a summary of the changes here.

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.
  • Add entry to the changelog's unreleased section.

Copy link
Contributor

github-actions bot commented Sep 24, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/preview/pr-1321/
on branch gh-pages at 2024-10-22 21:40 UTC

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

@diogomsmiranda diogomsmiranda force-pushed the 1004-add-audio-support-to-the-engine branch from 8ee607d to 362bfac Compare September 24, 2024 11:30
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 205 lines in your changes missing coverage. Please review.

Project coverage is 42.19%. Comparing base (7205c59) to head (9c844c0).

Files with missing lines Patch % Lines
core/src/al/miniaudio_context.cpp 0.00% 137 Missing ⚠️
engine/src/audio/audio.cpp 0.00% 20 Missing ⚠️
engine/src/audio/plugin.cpp 0.00% 18 Missing ⚠️
core/include/cubos/core/al/audio_context.hpp 0.00% 10 Missing ⚠️
engine/src/audio/bridge.cpp 0.00% 9 Missing ⚠️
engine/src/audio/listener.cpp 0.00% 4 Missing ⚠️
engine/include/cubos/engine/audio/bridge.hpp 0.00% 3 Missing ⚠️
core/src/al/audio_context.cpp 0.00% 2 Missing ⚠️
engine/src/audio/source.cpp 0.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@diogomsmiranda diogomsmiranda force-pushed the 1004-add-audio-support-to-the-engine branch 3 times, most recently from e72f419 to 9a7bb18 Compare September 24, 2024 12:17
Copy link
Contributor

@github-actions github-actions bot left a 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)

core/src/al/miniaudio_device.cpp Outdated Show resolved Hide resolved
core/src/al/miniaudio_device.cpp Outdated Show resolved Hide resolved
@diogomsmiranda diogomsmiranda force-pushed the 1004-add-audio-support-to-the-engine branch 5 times, most recently from 31b51bd to a7bae01 Compare September 29, 2024 21:28
Copy link
Member

@RiscadoA RiscadoA left a 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

core/include/cubos/core/al/miniaudio_device.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_device.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_device.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_device.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_device.hpp Outdated Show resolved Hide resolved
core/src/al/miniaudio_device.cpp Outdated Show resolved Hide resolved
core/src/al/miniaudio_device.cpp Outdated Show resolved Hide resolved
core/src/al/miniaudio_device.cpp Outdated Show resolved Hide resolved
core/src/al/miniaudio_device.cpp Outdated Show resolved Hide resolved
core/src/al/miniaudio_device.cpp Outdated Show resolved Hide resolved
@diogomsmiranda diogomsmiranda force-pushed the 1004-add-audio-support-to-the-engine branch 2 times, most recently from b480460 to 52949f6 Compare October 2, 2024 14:13
core/include/cubos/core/al/audio_context.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_context.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_context.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_context.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/al/audio_context.hpp Show resolved Hide resolved
engine/include/cubos/engine/audio/source.hpp Outdated Show resolved Hide resolved
engine/src/audio/audio.cpp Outdated Show resolved Hide resolved
engine/src/audio/plugin.cpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/audio/plugin.hpp Outdated Show resolved Hide resolved
engine/src/audio/plugin.cpp Outdated Show resolved Hide resolved
@diogomsmiranda diogomsmiranda force-pushed the 1004-add-audio-support-to-the-engine branch from 2b9a02d to d30fa15 Compare October 2, 2024 18:38
@diogomsmiranda diogomsmiranda force-pushed the 1004-add-audio-support-to-the-engine branch from d30fa15 to 9c844c0 Compare October 3, 2024 15:54
Copy link
Contributor

@github-actions github-actions bot left a 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for private member source

Suggested change
cubos::core::al::Source source;
cubos::core::al::Source mSource;

bool active = false; //< Is the listener active?

private:
cubos::core::al::Listener listener;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for private member listener

Suggested change
cubos::core::al::Listener listener;
cubos::core::al::Listener mListener;


#include <cubos/engine/assets/bridges/file.hpp>
#include <cubos/engine/audio/audio.hpp>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ modernize-pass-by-value ⚠️
pass by value and use std::move

Suggested change
#include <utility>

/// @brief Constructs a bridge.
AudioBridge(std::shared_ptr<cubos::core::al::AudioContext> context)
: FileBridge(core::reflection::reflect<Audio>())
, mContext(context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ modernize-pass-by-value ⚠️
pass by value and use std::move

Suggested change
, mContext(context)
, mContext(std::move(context))


#include <cubos/engine/assets/bridges/file.hpp>
#include <cubos/engine/audio/audio.hpp>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ performance-unnecessary-value-param ⚠️
parameter context is passed by value and only copied once; consider moving it to avoid unnecessary copies

Suggested change
#include <utility>

/// @brief Constructs a bridge.
AudioBridge(std::shared_ptr<cubos::core::al::AudioContext> context)
: FileBridge(core::reflection::reflect<Audio>())
, mContext(context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ performance-unnecessary-value-param ⚠️
parameter context is passed by value and only copied once; consider moving it to avoid unnecessary copies

Suggested change
, mContext(context)
, mContext(std::move(context))

Comment on lines +158 to +159
{
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ modernize-use-equals-default ⚠️
use = default to define a trivial destructor

Suggested change
{
}
= default;

public:
MiniaudioDevice(ma_context& context, const std::string& deviceName, ma_uint32 listenerCount)
: mContext(context)
, mListeners()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-redundant-member-init ⚠️
initializer for member mListeners is redundant

Suggested change
, mListeners()
,


for (ma_uint32 i = 0; i < playbackDeviceCount; i++)
{
if (pPlaybackDeviceInfos[i].isDefault)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-implicit-bool-conversion ⚠️
implicit conversion ma_bool32 (aka unsigned int) -> bool

Suggested change
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change
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);

Copy link
Contributor

@github-actions github-actions bot left a 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ performance-unnecessary-value-param ⚠️
the parameter context is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ performance-unnecessary-value-param ⚠️
the parameter context is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ modernize-use-equals-default ⚠️
use = default to define a trivial destructor

}

cubos::engine::Audio::Audio(Audio&& other) noexcept
: buffer(other.buffer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ performance-move-constructor-init ⚠️
move constructor initializes class member by calling a copy constructor

@diogomsmiranda diogomsmiranda force-pushed the 1004-add-audio-support-to-the-engine branch from 9c844c0 to 813e49e Compare October 22, 2024 21:39
Copy link
Contributor

@github-actions github-actions bot left a 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-diagnostic-error ⚠️
cubos/core/log.hpp file not found

Comment on lines +12 to +17
if (audio.buffer == nullptr)
{
return false;
}
assets.store(handle, std::move(audio));
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-simplify-boolean-expr ⚠️
redundant boolean literal in conditional return statement

Suggested change
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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-diagnostic-error ⚠️
cubos/core/log.hpp file not found

@@ -0,0 +1,43 @@
#include <cubos/core/log.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-diagnostic-error ⚠️
cubos/core/log.hpp file not found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add audio support to the engine
3 participants