Integrate Audio with XAudio2 #28
Conversation
Integrated an `AudioManager` system into the `Kerberos` namespace to enable audio management functionality. - Updated `Application.cpp` to create and initialize the `AudioManager`. - Modified `Application.h` to include `AudioManager` and prevent copying/moving of the `Application` class. - Introduced an abstract `AudioManager` interface with methods for initialization, updating, and shutdown. - Added a `Sound` interface for audio playback and control. - Implemented a Windows-specific `XAudio2AudioManager` using the XAudio2 library. - Handles initialization, resource management, and shutdown. - Ensures proper error handling and logging during initialization. - Added platform-specific preprocessor directives for Windows compatibility. - Updated the build system to include XAudio2 dependencies.
Updated `Log::Init` to use `stdout_color_sink_mt` for logging to standard output instead of standard error. Added a log message to indicate successful initialization of `XAudio2AudioManager`. Made a minor formatting adjustment in the `BuffersQueued` condition check. Updated `KerberosScriptCoreLib.dll` and `KerberosScriptCoreLib.pdb` binaries to reflect changes in the codebase.
Introduced `ComputeInfo` to manage compute shader limits in `GraphicsContext` and implemented querying of OpenGL compute shader limits in `OpenGLContext`. Enhanced logging for OpenGL information. Improved audio resource management in `XAudio2AudioManager` by fixing cleanup logic, adjusting log levels, and ensuring proper error handling. Updated `AudioFormat` enum to use `uint8_t`. Marked `AssetsPanel` constructor as `explicit` to prevent implicit conversions. Added `_isPlayingAudio` flag in `Player` to prevent redundant audio play/stop calls.
Enhanced OpenGL logging and error handling: - Added GLSL version logging in `OpenGLContext.cpp`. - Improved debug message formatting in `OpenGLRendererAPI.cpp`. - Refactored shader error handling in `OpenGLShader.cpp`: - Added assertions for unsupported shader types. - Improved shader linking error messages and fallback handling.
WalkthroughAudio system addition featuring an abstract AudioManager interface, Windows XAudio2 implementation, Sound asset type with loading/serialization, audio components for entities (AudioSource2D/3D, AudioListener), C# scripting bindings, and editor UI for audio asset management and component configuration. Changes
Sequence DiagramsequenceDiagram
participant Editor as Editor/Scene
participant App as Application
participant SI as ScriptInterface
participant AM as AudioManager
participant XA as XAudio2Backend
participant Voice as XAudio2 Voice
Editor->>App: Initialize
App->>AM: Create() & Init()
AM->>XA: Initialize XAudio2 & Master Voice
Editor->>SI: Load Sound Asset
SI->>AM: Load(filepath)
AM->>XA: ParseWAV & Create Buffer
XA-->>AM: Return Sound Object
AM-->>SI: Return Ref<Sound>
SI-->>Editor: Asset Ready
Editor->>SI: Play Audio (via Script/Component)
SI->>AM: Play(soundID)
AM->>XA: CreateSourceVoice & SubmitBuffer
XA->>Voice: Play Audio Data
Voice-->>XA: Playing
Editor->>SI: Volume Control (SetVolume)
SI->>AM: SetVolume(soundID, volume)
AM->>Voice: SetVolume(volume)
App->>AM: Update() [per frame]
AM->>XA: Update Playing Voices
XA->>Voice: Check Completion & Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
KerberosEditor/src/Editor/HierarchyPanel.cpp (1)
1458-1461: Bug: Wrong component removed for TextComponent.Line 1460 removes
EnvironmentComponentinstead ofTextComponentwhen the delete action is triggered in the Text component section.if (componentDeleted) { - entity.RemoveComponent<EnvironmentComponent>(); + entity.RemoveComponent<TextComponent>(); }
🟡 Minor comments (14)
Kerberos.sln.DotSettings.user-18-18 (1)
18-18: Check whetherOpengis a typo
Openglooks like a misspelling (maybeOpenGLorOpengl). Whitelisting it here will hide that typo in inspections. Please confirm the intended identifier and adjust the dictionary entry (and any usage) accordingly.Kerberos/src/Platform/OpenGL/OpenGLShader.cpp-681-698 (1)
681-698: Typo in error message: "OpengGL" should be "OpenGL".The enhanced error handling logic is good, but there's a typo on line 689.
else { - KBR_CORE_ERROR("Shader linking failed ({0}): No OpengGL info log was provided!", m_FilePath); + KBR_CORE_ERROR("Shader linking failed ({0}): No OpenGL info log was provided!", m_FilePath); }Kerberos/src/Platform/OpenGL/OpenGLRendererAPI.cpp-17-46 (1)
17-46: Enhanced debug formatting improves diagnostics.The refactored OpenGL message callback now provides much richer context by prefixing source and type information before the actual GL message. The implementation correctly handles all GL debug source and type enumerations with appropriate fallback assertions.
However, this OpenGL debugging enhancement appears unrelated to the audio integration PR.
Kerberos/src/Platform/OpenGL/OpenGLContext.cpp-57-91 (1)
57-91: OpenGL compute info changes unrelated to audio integration.This entire compute shader limits feature appears unrelated to the PR's stated objective of integrating XAudio2 for audio. Consider moving these OpenGL enhancements to a separate PR for better change isolation and review focus.
Kerberos/src/Platform/OpenGL/OpenGLContext.h-17-18 (1)
17-18: Declaration looks good, but unrelated to audio integration.The
QueryComputeInfo()method declaration is correctly placed in the private section. However, this OpenGL compute shader functionality appears unrelated to the audio integration described in the PR objectives.KerberosScriptCoreLib/Source/Kerberos/Player.cs-16-17 (1)
16-17: Incomplete comment - clarify or remove.The comment at line 16 appears incomplete: "Implement OnXButtonClicked methods for". Please complete the thought or remove this fragment if it's no longer relevant.
Kerberos/src/Kerberos/Utils/PlatformUtils.h-30-30 (1)
30-30: Typo: "give" should be "given".- * @brief Opens the give file with the default application associated with the file type. + * @brief Opens the given file with the default application associated with the file type.Kerberos/src/Platform/Windows/WindowsPlatformUtils.cpp-87-101 (1)
87-101: Log message inconsistency with method name.Line 91 logs
"FileOperations::DeleteFile called..."but the method is namedDelete. Update for consistency.- KBR_CORE_WARN("FileOperations::DeleteFile called with an empty path."); + KBR_CORE_WARN("FileOperations::Delete called with an empty path.");KerberosEditor/src/Editor/AssetsPanel.cpp-113-118 (1)
113-118: Unused[[nodiscard]]result fromDelete.The return value from
FileOperations::Deleteis stored but never used. SinceDeleteis marked[[nodiscard]], handle the failure case (e.g., show notification to user).if (ImGui::MenuItem("Delete")) { - const bool success = FileOperations::Delete((m_AssetsDirectory / item).string().c_str()); + if (!FileOperations::Delete((m_AssetsDirectory / item).string().c_str())) + { + KBR_CORE_ERROR("Failed to delete asset: {0}", item.string()); + } + else + { + RefreshAssetTree(); + } }Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp-14-17 (1)
14-17: Potential null pointer dereference if initialization order is not guaranteed.
GetAudioManager()can returnnullptrand is dereferenced without a check. While AudioManager is initialized during Application construction, this code assumes the audio system is always initialized before asset loading. Consider adding a null check for defensive programming, though note this pattern exists throughout the codebase (Sound.cpp, Scene.cpp).Kerberos/src/Kerberos/Scene/Scene.cpp-120-121 (1)
120-121: Consider null-checking AudioManager before calling Update.If
GetAudioManager()returnsnullptr(e.g., audio initialization failed), this will cause a null pointer dereference.- Application::Get().GetAudioManager()->Update(); + if (auto* audioManager = Application::Get().GetAudioManager()) + audioManager->Update();Kerberos/src/Kerberos/Scripting/ScriptInterface.cpp-252-262 (1)
252-262: Copy-paste error: variable namedtextComponentfor audio component.The variable on line 257 is named
textComponentbut holds anAudioSource2DComponent. This appears in multiple functions.- const AudioSource2DComponent& textComponent = entity.GetComponent<AudioSource2DComponent>(); - if (textComponent.SoundAsset) + const AudioSource2DComponent& audioComponent = entity.GetComponent<AudioSource2DComponent>(); + if (audioComponent.SoundAsset) { - textComponent.SoundAsset->Play(); + audioComponent.SoundAsset->Play(); }Same issue appears in
AudioSource2DComponent_Stop(line 269),AudioSource3DComponent_Play(line 318), andAudioSource3DComponent_Stop(line 330).Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.cpp-39-49 (1)
39-49:XAUDIO2_DEBUG_CONFIGURATIONstruct not zero-initialized.The struct may contain additional fields that are left uninitialized, potentially causing unexpected debug behavior.
#ifdef KBR_DEBUG - XAUDIO2_DEBUG_CONFIGURATION debugConfig; + XAUDIO2_DEBUG_CONFIGURATION debugConfig = {}; debugConfig.TraceMask = XAUDIO2_LOG_ERRORS;Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.cpp-8-11 (1)
8-11: Destructor callingShutdown()can cause double cleanup.If
Shutdown()is called explicitly before destruction, the destructor will call it again. While guards exist (if (m_MasteringVoice),if (m_XAudio2)),CoUninitialize()will be called unconditionally on line 92, potentially causing issues if COM wasn't initialized or was already uninitialized.Consider tracking initialization state:
+ bool m_Initialized = false; + void XAudio2AudioManager::Shutdown() { + if (!m_Initialized) + return; + m_Initialized = false; + if (m_MasteringVoice)And set
m_Initialized = trueat the end ofInit().Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (15)
Kerberos.sln.DotSettings.user (1)
1-30: Consider excluding.DotSettings.userfrom version controlThis is a JetBrains per-user settings file; teams often ignore these (e.g., via
.gitignore) to avoid noisy, user-specific diffs. If you don’t rely on sharing these settings, consider dropping it from the repo or moving shared dictionary entries into a non-.usersettings file instead.Kerberos/src/Platform/OpenGL/OpenGLShader.cpp (1)
794-802:descriptorCountis computed but never used.The
descriptorCountvariable is calculated inside the if-block but isn't included in the trace output on line 802, unlike the sampled_images section (line 774) which logs the count. Either include it in the trace for consistency, or remove the unused computation.for (const auto& resource : resources.storage_images) { uint32_t set = compiler.get_decoration(resource.id, spv::DecorationDescriptorSet); uint32_t binding = compiler.get_decoration(resource.id, spv::DecorationBinding); + uint32_t descriptorCount = 1; const spirv_cross::SPIRType& type = compiler.get_type(resource.base_type_id); if (!type.array.empty()) { - uint32_t descriptorCount = type.array[0]; + descriptorCount = type.array[0]; if (descriptorCount == 0) - //descriptorCount = VulkanContext::Get().GetCapabilities().maxStorageImageAllocationCount; // Or some max descriptorCount = 1; // Default to 1 if unsized } - KBR_CORE_TRACE(" Name: {0}, Set: {1}, Binding: {2}", resource.name, set, binding); + KBR_CORE_TRACE(" Name: {0}, Set: {1}, Binding: {2}, Count: {3}", resource.name, set, binding, descriptorCount); }Kerberos/src/Platform/OpenGL/OpenGLContext.cpp (1)
26-29: Consider using spaces instead of tabs for log alignment.The logging statements use tab characters for alignment, which may render inconsistently across different console configurations. Consider using spaces for predictable alignment.
Kerberos/src/Kerberos/Application.h (1)
65-65: Consider returning a reference or using smart pointer semantics.Returning a raw pointer for
GetAudioManager()exposes ownership ambiguity. Since callers should not own or delete this pointer, consider returning a reference (if AudioManager is guaranteed to exist) or documenting ownership clearly.If AudioManager is always guaranteed to be initialized after construction:
- AudioManager* GetAudioManager() const { return m_AudioManager; } + AudioManager& GetAudioManager() const { return *m_AudioManager; }This makes the API clearer that callers get a non-owning reference, though it requires ensuring
m_AudioManageris never null when accessed.Kerberos/src/Kerberos/Audio/AudioManager.cpp (1)
11-19: Clarify ownership semantics for raw pointer.The
Create()factory returns a raw pointer, which transfers ownership to the caller. Consider documenting this explicitly or usingstd::unique_ptr<AudioManager>to make ownership transfer clear and prevent potential memory leaks.Additionally, the implementation only supports Windows (XAudio2). Ensure other platforms are tracked in your roadmap or add a compile-time error for clarity.
Consider this alternative using smart pointers:
- AudioManager* AudioManager::Create() + std::unique_ptr<AudioManager> AudioManager::Create() { #ifdef KBR_PLATFORM_WINDOWS - return new XAudio2AudioManager(); + return std::make_unique<XAudio2AudioManager>(); #endif KBR_CORE_ASSERT(false, "No AudioManager implementation for this platform!"); return nullptr; }KerberosEditor/src/EditorLayer.cpp (1)
32-32: LGTM - testing disabled for release.Disabling the
TESTINGmacro is appropriate for the release build. However, consider using a CMake/build configuration option instead of a source-level#defineto make it easier for developers to toggle testing mode without modifying source files.Kerberos/src/Kerberos/Scene/SceneSerializer.cpp (1)
789-809: Add asset handle validation for audio component deserialization to match the StaticMeshComponent pattern.Audio components are loaded without validating the asset handle (lines 789-809), unlike
StaticMeshComponentdeserialization (lines 762-771) which checksmeshHandle.IsValid()before loading. While audio components do handle nullSoundAssetgracefully—as confirmed by null checks inHierarchyPanel.cppandScriptInterface.cpp—adding validation would ensure consistency across the codebase and provide clearer intent.Apply the same pattern to both
AudioSource3DComponentandAudioSource2DComponent:if (auto audioSource3DComponent = entity["AudioSource3DComponent"]) { auto& audioSource3D = deserializedEntity.AddComponent<AudioSource3DComponent>(); - audioSource3D.SoundAsset = AssetManager::GetAsset<Sound>(AssetHandle(audioSource3DComponent["SoundAsset"].as<uint64_t>())); + const AssetHandle soundHandle = AssetHandle(audioSource3DComponent["SoundAsset"].as<uint64_t>()); + if (soundHandle.IsValid()) + audioSource3D.SoundAsset = AssetManager::GetAsset<Sound>(soundHandle); audioSource3D.Loop = audioSource3DComponent["Loop"].as<bool>(); audioSource3D.Volume = audioSource3DComponent["Volume"].as<float>(); }Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp (1)
9-12: UnusedAssetHandleparameter.The first parameter is unnamed and unused. If intentional (for interface conformance), consider adding
[[maybe_unused]]or a comment to clarify.- Ref<Sound> SoundImporter::ImportSound(AssetHandle, const AssetMetadata& metadata) + Ref<Sound> SoundImporter::ImportSound([[maybe_unused]] AssetHandle handle, const AssetMetadata& metadata)KerberosEditor/src/Editor/AssetsPanel.cpp (1)
377-405: Consider unifying asset type detection.The code mixes extension-based detection (textures, meshes) with
AssetTypeenum detection (sound). Since you're already queryingassetTypeon line 377, consider using it consistently for all types to simplify maintenance.- if (extension == ".jpg" || extension == ".png" || extension == ".svg") + if (assetType == AssetType::Texture2D) { ImGui::SetDragDropPayload(ASSET_BROWSER_TEXTURE, &handle, sizeof(AssetHandle), ImGuiCond_Once); // ... } - else if (extension == ".kbrcubemap") + else if (assetType == AssetType::TextureCube) { // ... } - else if (extension == ".obj") + else if (assetType == AssetType::Mesh) { // ... }KerberosScriptCoreLib/Source/Kerberos/Scene/Components.cs (2)
116-121: Consider exposing Volume and Looping properties.The
InternalCallsclass definesAudioSource2DComponent_SetVolume,AudioSource2DComponent_GetVolume,AudioSource2DComponent_SetLooping, andAudioSource2DComponent_IsLoopingmethods (perInternalCalls.cslines 89-98), but these are not exposed in the C# component. This limits scripting capabilities.public class AudioSource2DComponent : Component { public void Play() => InternalCalls.AudioSource2DComponent_Play(Entity.ID); public void Stop() => InternalCalls.AudioSource2DComponent_Stop(Entity.ID); + + public float Volume + { + get => InternalCalls.AudioSource2DComponent_GetVolume(Entity.ID); + set => InternalCalls.AudioSource2DComponent_SetVolume(Entity.ID, value); + } + + public bool Looping + { + get => InternalCalls.AudioSource2DComponent_IsLooping(Entity.ID); + set => InternalCalls.AudioSource2DComponent_SetLooping(Entity.ID, value); + } }
123-128: Same properties missing for AudioSource3DComponent.Similar to
AudioSource2DComponent, the 3D variant should also exposeVolumeandLoopingproperties to match the available internal calls.public class AudioSource3DComponent : Component { public void Play() => InternalCalls.AudioSource3DComponent_Play(Entity.ID); public void Stop() => InternalCalls.AudioSource3DComponent_Stop(Entity.ID); + + public float Volume + { + get => InternalCalls.AudioSource3DComponent_GetVolume(Entity.ID); + set => InternalCalls.AudioSource3DComponent_SetVolume(Entity.ID, value); + } + + public bool Looping + { + get => InternalCalls.AudioSource3DComponent_IsLooping(Entity.ID); + set => InternalCalls.AudioSource3DComponent_SetLooping(Entity.ID, value); + } }KerberosEditor/src/Editor/HierarchyPanel.cpp (2)
1522-1580: AudioSource3DComponent UI is nearly identical to 2D - consider extracting common logic.Both AudioSource2DComponent and AudioSource3DComponent UI blocks share nearly identical code. Consider extracting a helper template or function to reduce duplication.
1605-1605: Typo in variable name.
listenerCOmpshould belistenerCompfor consistency with naming conventions used elsewhere in the codebase.- auto& listenerCOmp = entity.GetComponent<AudioListenerComponent>(); + auto& listenerComp = entity.GetComponent<AudioListenerComponent>(); - ImGui::DragFloat("Volume", &listenerCOmp.Volume, 0.01f, 0.0f, 1.0f); + ImGui::DragFloat("Volume", &listenerComp.Volume, 0.01f, 0.0f, 1.0f);Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.cpp (2)
206-254: Volume controls don't clamp values to valid range.
DecreaseVolumecan result in negative volume (phase inversion in XAudio2), andIncreaseVolumehas no upper bound. While XAudio2 accepts these values, they're likely unintended.float currentVolume = 0; audio->second->GetVolume(¤tVolume); - if (const HRESULT res = audio->second->SetVolume(currentVolume + delta); FAILED(res)) + const float newVolume = std::clamp(currentVolume + delta, 0.0f, 2.0f); + if (const HRESULT res = audio->second->SetVolume(newVolume); FAILED(res))Apply similar clamping in
DecreaseVolumeandSetVolume.
361-372: LargechunkSizein data chunk could cause allocation issues.Line 363 resizes the buffer to
chunkSizewithout validating the value. A malformed WAV file with a huge chunk size could cause memory exhaustion or crash.Consider adding a reasonable size limit:
else if (strncmp(chunkId, "data", 4) == 0) { // Read audio data + constexpr DWORD maxAudioSize = 100 * 1024 * 1024; // 100 MB limit + if (chunkSize > maxAudioSize) { + KBR_CORE_ERROR("WAV file data chunk too large: {0}", filepath.string()); + return; + } soundData.buffer.resize(chunkSize);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
KerberosEditor/Resources/Scripts/KerberosScriptCoreLib.dllis excluded by!**/*.dllKerberosEditor/assets/models/donut/donut_csanad.fbxis excluded by!**/*.fbxKerberosEditor/assets/sounds/Technoshock.wavis excluded by!**/*.wav
📒 Files selected for processing (40)
Kerberos.sln.DotSettings.user(2 hunks)Kerberos/src/Kerberos/Application.cpp(1 hunks)Kerberos/src/Kerberos/Application.h(4 hunks)Kerberos/src/Kerberos/Assets/Asset.h(3 hunks)Kerberos/src/Kerberos/Assets/EditorAssetManager.cpp(2 hunks)Kerberos/src/Kerberos/Assets/Importers/AssetImporter.cpp(2 hunks)Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp(1 hunks)Kerberos/src/Kerberos/Assets/Importers/SoundImporter.h(1 hunks)Kerberos/src/Kerberos/Audio/AudioManager.cpp(1 hunks)Kerberos/src/Kerberos/Audio/AudioManager.h(1 hunks)Kerberos/src/Kerberos/Audio/Sound.cpp(1 hunks)Kerberos/src/Kerberos/Audio/Sound.h(1 hunks)Kerberos/src/Kerberos/Log.cpp(1 hunks)Kerberos/src/Kerberos/Renderer/GraphicsContext.h(1 hunks)Kerberos/src/Kerberos/Scene/Components/AudioComponents.h(1 hunks)Kerberos/src/Kerberos/Scene/Scene.cpp(6 hunks)Kerberos/src/Kerberos/Scene/SceneSerializer.cpp(3 hunks)Kerberos/src/Kerberos/Scripting/ScriptInterface.cpp(4 hunks)Kerberos/src/Kerberos/Utils/PlatformUtils.h(3 hunks)Kerberos/src/Platform/OpenGL/OpenGLContext.cpp(2 hunks)Kerberos/src/Platform/OpenGL/OpenGLContext.h(1 hunks)Kerberos/src/Platform/OpenGL/OpenGLRendererAPI.cpp(1 hunks)Kerberos/src/Platform/OpenGL/OpenGLShader.cpp(7 hunks)Kerberos/src/Platform/Vulkan/VulkanContext.cpp(1 hunks)Kerberos/src/Platform/Vulkan/VulkanContext.h(1 hunks)Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.cpp(1 hunks)Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.h(1 hunks)Kerberos/src/Platform/Windows/WindowsPlatformUtils.cpp(1 hunks)KerberosEditor/assets/AssetRegistry.kbrar(1 hunks)KerberosEditor/assets/scenes/Example.kerberos(3 hunks)KerberosEditor/imgui.ini(2 hunks)KerberosEditor/src/Editor/AssetConstants.h(1 hunks)KerberosEditor/src/Editor/AssetsPanel.cpp(6 hunks)KerberosEditor/src/Editor/AssetsPanel.h(1 hunks)KerberosEditor/src/Editor/HierarchyPanel.cpp(8 hunks)KerberosEditor/src/EditorLayer.cpp(3 hunks)KerberosScriptCoreLib/Source/InternalCalls.cs(1 hunks)KerberosScriptCoreLib/Source/Kerberos/Player.cs(3 hunks)KerberosScriptCoreLib/Source/Kerberos/Scene/Components.cs(1 hunks)premake5.lua(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (21)
Kerberos/src/Kerberos/Renderer/GraphicsContext.h (2)
Kerberos/src/Platform/OpenGL/OpenGLContext.h (1)
Kerberos(7-24)Kerberos/src/Platform/OpenGL/OpenGLContext.cpp (4)
Init(15-48)Init(15-15)SwapBuffers(50-55)SwapBuffers(50-50)
KerberosEditor/src/Editor/AssetsPanel.h (1)
KerberosEditor/src/Editor/AssetsPanel.cpp (1)
AssetsPanel(17-27)
Kerberos/src/Kerberos/Assets/Importers/SoundImporter.h (3)
Kerberos/src/Kerberos/Audio/AudioManager.h (1)
Kerberos(9-39)Kerberos/src/Kerberos/Audio/Sound.h (2)
Kerberos(6-43)Sound(8-42)Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp (4)
ImportSound(9-12)ImportSound(9-9)ImportSound(14-17)ImportSound(14-14)
Kerberos/src/Kerberos/Application.h (1)
Kerberos/src/Kerberos/Application.cpp (2)
Application(15-50)Application(52-56)
KerberosScriptCoreLib/Source/Kerberos/Scene/Components.cs (2)
KerberosScriptCoreLib/Source/InternalCalls.cs (1)
InternalCalls(8-124)Kerberos/src/Kerberos/Scripting/ScriptInterface.cpp (8)
AudioSource2DComponent_Play(252-262)AudioSource2DComponent_Play(252-252)AudioSource2DComponent_Stop(264-274)AudioSource2DComponent_Stop(264-264)AudioSource3DComponent_Play(313-323)AudioSource3DComponent_Play(313-313)AudioSource3DComponent_Stop(325-335)AudioSource3DComponent_Stop(325-325)
Kerberos/src/Kerberos/Utils/PlatformUtils.h (1)
Kerberos/src/Platform/Windows/WindowsPlatformUtils.cpp (8)
OpenFile(13-34)OpenFile(13-13)OpenFile(59-85)OpenFile(59-59)Delete(87-101)Delete(87-87)RevealInFileExplorer(103-130)RevealInFileExplorer(103-103)
Kerberos/src/Kerberos/Assets/Importers/AssetImporter.cpp (2)
Kerberos/src/Kerberos/Audio/Sound.h (1)
Sound(8-42)Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp (4)
ImportSound(9-12)ImportSound(9-9)ImportSound(14-17)ImportSound(14-14)
Kerberos/src/Platform/Vulkan/VulkanContext.h (1)
Kerberos/src/Platform/Vulkan/VulkanContext.cpp (2)
GetBufferDeviceAddress(1104-1111)GetBufferDeviceAddress(1104-1104)
Kerberos/src/Kerberos/Application.cpp (2)
Kerberos/src/Kerberos/Audio/AudioManager.cpp (2)
Create(11-19)Create(11-11)Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.cpp (2)
Init(13-52)Init(13-13)
KerberosScriptCoreLib/Source/Kerberos/Player.cs (3)
KerberosScriptCoreLib/Source/Kerberos/Scene/Components.cs (5)
AudioSource2DComponent(116-121)Play(118-118)Play(125-125)Stop(120-120)Stop(127-127)KerberosScriptCoreLib/Source/Kerberos/Scene/Entity.cs (1)
HasComponent(33-36)KerberosScriptCoreLib/Source/Kerberos/Core/Input.cs (2)
Input(3-9)IsKeyDown(5-8)
Kerberos/src/Kerberos/Scene/Components/AudioComponents.h (3)
Kerberos/src/Kerberos/Audio/Sound.h (2)
Kerberos(6-43)Sound(8-42)Kerberos/src/Kerberos/Assets/Asset.h (1)
Kerberos(8-79)KerberosScriptCoreLib/Source/Kerberos/Scene/Components.cs (1)
AudioSource3DComponent(123-128)
Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp (1)
Kerberos/src/Kerberos/Application.h (1)
Get(61-61)
Kerberos/src/Kerberos/Audio/AudioManager.h (4)
Kerberos/src/Kerberos/Audio/Sound.h (3)
Kerberos(6-43)Sound(8-42)Play(22-32)Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.h (1)
Kerberos(11-72)Kerberos/src/Kerberos/Audio/Sound.cpp (14)
Play(8-11)Play(8-8)Stop(13-16)Stop(13-13)IncreaseVolume(18-21)IncreaseVolume(18-18)DecreaseVolume(23-26)DecreaseVolume(23-23)SetVolume(28-31)SetVolume(28-28)ResetVolume(33-36)ResetVolume(33-33)Mute(38-41)Mute(38-38)Kerberos/src/Kerberos/Audio/AudioManager.cpp (2)
Create(11-19)Create(11-11)
KerberosEditor/src/Editor/AssetsPanel.cpp (2)
Kerberos/src/Platform/Windows/WindowsPlatformUtils.cpp (2)
Delete(87-101)Delete(87-87)Kerberos/src/Kerberos/Audio/Sound.h (1)
Sound(8-42)
Kerberos/src/Platform/OpenGL/OpenGLContext.h (1)
Kerberos/src/Platform/OpenGL/OpenGLContext.cpp (2)
QueryComputeInfo(57-91)QueryComputeInfo(57-57)
Kerberos/src/Kerberos/Assets/EditorAssetManager.cpp (2)
Kerberos/src/Kerberos/Audio/Sound.h (1)
Sound(8-42)Kerberos/src/Kerberos/Renderer/TextureCube.h (1)
TextureCube(20-28)
Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.h (4)
Kerberos/src/Kerberos/Audio/AudioManager.h (2)
Kerberos(9-39)AudioManager(11-38)Kerberos/src/Kerberos/Audio/Sound.h (3)
Kerberos(6-43)Sound(8-42)Play(22-32)Kerberos/src/Kerberos/Assets/Asset.h (1)
Kerberos(8-79)Kerberos/src/Kerberos/Audio/Sound.cpp (14)
Play(8-11)Play(8-8)Stop(13-16)Stop(13-13)IncreaseVolume(18-21)IncreaseVolume(18-18)DecreaseVolume(23-26)DecreaseVolume(23-23)SetVolume(28-31)SetVolume(28-28)ResetVolume(33-36)ResetVolume(33-33)Mute(38-41)Mute(38-38)
KerberosScriptCoreLib/Source/InternalCalls.cs (1)
Kerberos/src/Kerberos/Scripting/ScriptInterface.cpp (24)
AudioSource2DComponent_Play(252-262)AudioSource2DComponent_Play(252-252)AudioSource2DComponent_Stop(264-274)AudioSource2DComponent_Stop(264-264)AudioSource2DComponent_SetVolume(285-292)AudioSource2DComponent_SetVolume(285-285)AudioSource2DComponent_GetVolume(276-283)AudioSource2DComponent_GetVolume(276-276)AudioSource2DComponent_SetLooping(294-301)AudioSource2DComponent_SetLooping(294-294)AudioSource2DComponent_IsLooping(303-310)AudioSource2DComponent_IsLooping(303-303)AudioSource3DComponent_Play(313-323)AudioSource3DComponent_Play(313-313)AudioSource3DComponent_Stop(325-335)AudioSource3DComponent_Stop(325-325)AudioSource3DComponent_SetVolume(346-354)AudioSource3DComponent_SetVolume(346-346)AudioSource3DComponent_GetVolume(337-344)AudioSource3DComponent_GetVolume(337-337)AudioSource3DComponent_SetLooping(356-363)AudioSource3DComponent_SetLooping(356-356)AudioSource3DComponent_IsLooping(365-372)AudioSource3DComponent_IsLooping(365-365)
Kerberos/src/Kerberos/Scene/SceneSerializer.cpp (2)
Kerberos/src/Kerberos/Core/UUID.h (1)
Invalid(33-36)Kerberos/src/Kerberos/Assets/EditorAssetManager.cpp (2)
GetAsset(44-72)GetAsset(44-44)
Kerberos/src/Kerberos/Scripting/ScriptInterface.cpp (2)
Kerberos/src/Kerberos/Scripting/ScriptEngine.cpp (2)
GetSceneContext(230-233)GetSceneContext(230-230)Kerberos/src/Kerberos/Physics/PhysicsSystem.cpp (3)
entity(168-168)entity(189-189)entity(224-224)
Kerberos/src/Kerberos/Assets/Asset.h (1)
Kerberos/src/Kerberos/Audio/Sound.h (1)
Sound(8-42)
🪛 Clang (14.0.6)
Kerberos/src/Kerberos/Renderer/GraphicsContext.h
[error] 3-3: 'glm/glm.hpp' file not found
(clang-diagnostic-error)
Kerberos/src/Kerberos/Audio/AudioManager.cpp
[error] 1-1: 'kbrpch.h' file not found
(clang-diagnostic-error)
Kerberos/src/Kerberos/Assets/Importers/SoundImporter.h
[error] 3-3: 'Kerberos/Assets/Asset.h' file not found
(clang-diagnostic-error)
KerberosEditor/src/Editor/AssetConstants.h
[error] 3-3: unknown type name 'namespace'
(clang-diagnostic-error)
[error] 3-3: expected ';' after top level declarator
(clang-diagnostic-error)
Kerberos/src/Kerberos/Audio/Sound.cpp
[error] 1-1: 'kbrpch.h' file not found
(clang-diagnostic-error)
Kerberos/src/Kerberos/Scene/Components/AudioComponents.h
[error] 3-3: 'Kerberos/Audio/Sound.h' file not found
(clang-diagnostic-error)
Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp
[error] 1-1: 'kbrpch.h' file not found
(clang-diagnostic-error)
Kerberos/src/Kerberos/Audio/AudioManager.h
[error] 3-3: 'Kerberos/Core.h' file not found
(clang-diagnostic-error)
Kerberos/src/Kerberos/Audio/Sound.h
[error] 3-3: 'Kerberos/Core.h' file not found
(clang-diagnostic-error)
Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.cpp
[error] 1-1: 'kbrpch.h' file not found
(clang-diagnostic-error)
Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.h
[error] 3-3: 'Kerberos/Audio/AudioManager.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build on Windows (Debug)
- GitHub Check: Build on Windows (Release)
🔇 Additional comments (45)
KerberosEditor/imgui.ini (1)
12-12: UI layout adjustments are expected for audio feature integration.The imgui.ini file is auto-generated by ImGui to store window layout configuration, including positions, sizes, and docking state. The changes reflect reasonable adjustments to the editor UI layout to accommodate the new audio asset management and component configuration panels introduced in this PR. The layout modifications appear consistent and appropriately scaled.
Also applies to: 17-18, 24-24, 29-30, 35-35, 42-42, 48-48, 57-57, 64-65
Kerberos.sln.DotSettings.user (2)
8-8: ADPCM dictionary entry is appropriate
ADPCMis a valid audio-related acronym; adding it to the user dictionary is reasonable and non-impactful.
22-22:shadercdictionary entry makes sense
shadercis a common library/tool name; adding it to the dictionary is fine and keeps inspections clean.Kerberos/src/Platform/OpenGL/OpenGLShader.cpp (4)
41-44: LGTM!Good addition of the default case to handle unexpected shader types with an assertion. This is consistent with the error handling pattern used elsewhere in the file.
54-57: LGTM!Consistent handling of unsupported shader types with the previous function.
458-473: LGTM!Good use of C++17 init-statement to scope the
ifstreamto its usage block.
583-598: LGTM!Consistent use of C++17 init-statement pattern.
Kerberos/src/Kerberos/Renderer/GraphicsContext.h (1)
3-3: Static analysis warning can be ignored.The
'glm/glm.hpp' file not foundwarning from Clang is a false positive. GLM is a standard graphics math library and the include path is configured correctly at build time.Kerberos/src/Platform/OpenGL/OpenGLContext.cpp (1)
31-47: LGTM with dependency on fixing MaxWorkGroupInvocations.The initialization flow correctly calls
QueryComputeInfo()and logs compute shader limits for diagnostics. However, the logging ofMaxWorkGroupInvocationsat lines 44-47 will need to be updated to log a single value rather than a vec3 once the critical issue inGraphicsContext.his addressed.Kerberos/src/Kerberos/Log.cpp (1)
17-17: The sink configuration is already consistent between core and client loggers.Both the core logger (line 17) and client logger (line 25) use
stdout_color_sink_mt(), making them uniform. The original concern about switching from stderr to stdout appears to be based on a misunderstanding of the code state. Both loggers output to stdout, which is the intended design in this codebase.If this logging configuration is unrelated to the audio integration described in the PR title, that concern may still warrant clarification—consider moving unrelated changes to a separate PR.
Likely an incorrect or invalid review comment.
KerberosEditor/assets/scenes/Example.kerberos (2)
25-30: Verify the script class naming convention.The
ClassNamevalueKerberos.Source.Kerberos.Cameraappears to have a redundant or incorrect namespace pattern. Typically, C# script class names follow a simpler convention likeKerberos.CameraorMyProject.Camera. TheSource.Kerberossegment seems unusual.Please verify this is the intended namespace structure for your C# scripts, or if this should be simplified to something like
Kerberos.Camera.
159-164: Same naming convention concern for Player script.Same observation as above -
Kerberos.Source.Kerberos.Playerhas an unusual namespace pattern.Kerberos/src/Kerberos/Assets/Importers/AssetImporter.cpp (1)
37-38: LGTM!The Sound asset import case follows the established pattern used by other asset types like Texture2D and TextureCube, properly delegating to the specialized
SoundImporter.KerberosEditor/assets/AssetRegistry.kbrar (1)
47-49: LGTM!The Sound asset entry follows the established registry format and is properly positioned among other assets.
KerberosEditor/src/Editor/AssetsPanel.h (1)
19-19: LGTM!Adding
explicitto the single-argument constructor prevents unintended implicit conversions, which is a good practice.Kerberos/src/Kerberos/Application.h (1)
46-49: LGTM!Explicitly deleting copy/move constructors and assignment operators for the Application singleton is the correct approach to prevent accidental copies or moves.
Kerberos/src/Kerberos/Audio/Sound.cpp (1)
8-41:m_SoundIDis properly initialized via UUID's default constructor.The
m_SoundIDmember, while not explicitly listed in the Sound constructor's member initializer list, is automatically initialized when each Sound object is constructed. The UUID class provides a default constructor that generates a random 64-bit value, and C++ implicitly invokes this constructor form_SoundIDduring Sound object construction. Each Sound instance receives a unique random UUID, which is then used to track the sound in the AudioManager's internal mappings.Kerberos/src/Kerberos/Assets/EditorAssetManager.cpp (2)
22-22: Limited audio format support - verify roadmap.Currently only WAV format is supported. Ensure the TODO for additional audio formats (e.g., MP3, OGG, FLAC) is tracked properly for future implementation.
222-226: LGTM - transitional implementation.The explicit type check for Sound is appropriate as a transitional approach. The TODO correctly identifies this as temporary until comprehensive asset type support is implemented.
KerberosScriptCoreLib/Source/Kerberos/Player.cs (1)
36-37: LGTM!The initialization pattern is consistent with other component retrievals in this method.
Kerberos/src/Kerberos/Scene/SceneSerializer.cpp (1)
423-452: LGTM!The serialization logic for audio components follows the established pattern used by other asset-referencing components (Mesh, Texture). The null-safety check using
UUID::Invalid()for missing SoundAssets is consistent with the codebase conventions.KerberosEditor/src/EditorLayer.cpp (1)
657-657: LGTM!Using the
ASSET_BROWSER_ITEMconstant fromAssetConstants.himproves maintainability and reduces the risk of typos compared to magic strings.Kerberos/src/Kerberos/Assets/Importers/SoundImporter.h (1)
9-14: LGTM!The
SoundImporterinterface follows the established pattern used by other asset importers in the codebase. The two static overloads provide flexibility for both asset-system-driven and direct file imports.Kerberos/src/Kerberos/Assets/Asset.h (2)
18-19: LGTM!The
Soundasset type is properly added to theAssetTypeenum and maintains the sequential pattern.
53-54: LGTM!The
Soundcase is correctly added toAssetTypeToString.Kerberos/src/Kerberos/Utils/PlatformUtils.h (1)
37-50: LGTM - new file operation methods.The declarations are well-documented. Consider adding
[[nodiscard]]toRevealInFileExplorerfor consistency with the other methods, though it's not strictly necessary if fire-and-forget usage is acceptable.KerberosEditor/src/Editor/AssetConstants.h (1)
1-12: LGTM - centralized asset type constants.Good refactor to eliminate magic strings for drag-and-drop payloads. The static analysis errors are false positives from the analyzer not recognizing C++
namespacesyntax.KerberosEditor/src/Editor/AssetsPanel.cpp (1)
8-8: Good addition of centralized constants.Using
AssetConstants.heliminates magic strings and improves maintainability.KerberosScriptCoreLib/Source/Kerberos/Scene/Components.cs (1)
130-132: AudioListenerComponent is empty.This component has no exposed functionality. If this is intentional (just a marker component), that's fine. Otherwise, consider adding a
Volumeproperty similar to the nativeAudioListenerComponent.Volumefield shown in the editor UI.Kerberos/src/Kerberos/Scene/Scene.cpp (3)
302-313: Audio component duplication looks correct.The duplication pattern for
AudioSource2DComponent,AudioSource3DComponent, andAudioListenerComponentfollows the established convention used by other components in this function.
539-550: Audio component copying in Scene::Copy is consistent.The copying pattern matches the approach used for other components in this function.
987-1001: Empty OnComponentAdded specializations are appropriate.These empty stubs follow the pattern established by other simple components (e.g.,
TagComponent,TransformComponent). If audio-specific initialization is needed later, these hooks are already in place.KerberosEditor/src/Editor/HierarchyPanel.cpp (3)
225-258: Physics submenu restructuring looks good.The reorganization into a Physics submenu improves UI organization and follows a logical grouping pattern.
273-294: Audio submenu implementation is consistent with the codebase patterns.The new Audio submenu properly adds the three audio component types and follows the same pattern used for other menu items.
1463-1521: AudioSource2DComponent UI implementation looks correct.The UI follows the established pattern with proper popup handling, drag-and-drop for sound assets, and controls for Loop and Volume.
Kerberos/src/Kerberos/Scene/Components/AudioComponents.h (3)
23-37: AudioSource2DComponent mirrors 3D variant appropriately.The 2D component has the same structure as 3D, which makes sense for consistent API. The distinction between 2D and 3D audio will be in the audio backend behavior.
39-45: AudioListenerComponent is minimal but functional.For a listener component, having just a volume control is reasonable. In the future, you may want to add properties like doppler factor, distance model, etc., but this is sufficient for initial implementation.
7-21: AudioSource3DComponent structure is well-defined.The component includes all necessary fields for audio playback. The
IsPlayingfield is correctly handled as runtime-only state—it is intentionally excluded from serialization and defaults tofalseupon deserialization, preventing stale state after scene load.Kerberos/src/Kerberos/Audio/Sound.h (2)
32-32: The baseAsset::GetType()is declared as non-const (virtual AssetType GetType() = 0), and all derived classes consistently implement it without const qualification (Texture, TextureCube, Material, Mesh, Scene). Adding const to Sound's override would break the method signature contract with the base class and cause compilation errors. The current implementation is correct.Likely an incorrect or invalid review comment.
36-41: m_SoundID initialization depends on UUID's default constructor.The concern about uninitialized
m_SoundIDis valid only ifUUID's default constructor does not provide reliable initialization. IfUUIDhas a proper default constructor (as is typical in standard UUID libraries like Boost), this is not an issue.Kerberos/src/Kerberos/Audio/AudioManager.h (1)
11-38: Clean abstract interface design.The interface provides a solid foundation for platform-specific audio implementations. The static factory pattern via
Create()aligns with the platform abstraction approach visible inAudioManager.cpp.One consideration: the defaulted copy/move constructors on lines 17-20 are somewhat unusual for an abstract base class (since derived classes like
XAudio2AudioManagerdelete copy operations anyway). This is harmless but could be simplified by just declaring virtual destructor.Kerberos/src/Kerberos/Scripting/ScriptInterface.cpp (1)
408-420: Audio scripting bindings properly registered.The internal calls and component type registrations are correctly added and align with the C# declarations.
Also applies to: 463-466
Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.h (2)
21-31:AudioDatastruct design looks good.The struct properly initializes
wfxwithmemsetand uses sensible defaults. Usingstd::vector<uint8_t>for the buffer is appropriate for managing audio data lifetime.
68-70: No hash specialization issue exists.
std::filesystem::pathhas a standardstd::hashspecialization available in C++17 and later. The project usesstdcpplatest(latest C++ standard), sostd::unordered_map<std::filesystem::path, ...>compiles and works correctly without any custom hash implementation. The implementation file confirms this pattern is already in use and functioning in production code.Likely an incorrect or invalid review comment.
KerberosScriptCoreLib/Source/InternalCalls.cs (1)
84-122: Audio component internal call declarations are complete and correct.The extern declarations match the native C++ implementations registered in
ScriptInterface.cpp. Parameter types and naming conventions follow the established patterns in this file.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @EasySouls. * #28 (comment) The following files were modified: * `Kerberos/src/Kerberos/Application.cpp` * `Kerberos/src/Kerberos/Application.h` * `Kerberos/src/Kerberos/Assets/Asset.h` * `Kerberos/src/Kerberos/Assets/EditorAssetManager.cpp` * `Kerberos/src/Kerberos/Assets/Importers/AssetImporter.cpp` * `Kerberos/src/Kerberos/Assets/Importers/SoundImporter.cpp` * `Kerberos/src/Kerberos/Assets/Importers/SoundImporter.h` * `Kerberos/src/Kerberos/Audio/AudioManager.cpp` * `Kerberos/src/Kerberos/Audio/AudioManager.h` * `Kerberos/src/Kerberos/Audio/Sound.cpp` * `Kerberos/src/Kerberos/Audio/Sound.h` * `Kerberos/src/Kerberos/Log.cpp` * `Kerberos/src/Kerberos/Renderer/GraphicsContext.h` * `Kerberos/src/Kerberos/Scene/Scene.cpp` * `Kerberos/src/Kerberos/Scene/SceneSerializer.cpp` * `Kerberos/src/Kerberos/Scripting/ScriptInterface.cpp` * `Kerberos/src/Kerberos/Utils/PlatformUtils.h` * `Kerberos/src/Platform/OpenGL/OpenGLContext.cpp` * `Kerberos/src/Platform/OpenGL/OpenGLContext.h` * `Kerberos/src/Platform/OpenGL/OpenGLRendererAPI.cpp` * `Kerberos/src/Platform/OpenGL/OpenGLShader.cpp` * `Kerberos/src/Platform/Vulkan/VulkanContext.cpp` * `Kerberos/src/Platform/Vulkan/VulkanContext.h` * `Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.cpp` * `Kerberos/src/Platform/Windows/Audio/XAudio2AudioManager.h` * `Kerberos/src/Platform/Windows/WindowsPlatformUtils.cpp` * `KerberosEditor/src/Editor/AssetConstants.h` * `KerberosEditor/src/Editor/AssetsPanel.cpp` * `KerberosEditor/src/Editor/AssetsPanel.h` * `KerberosEditor/src/Editor/HierarchyPanel.cpp` * `KerberosEditor/src/EditorLayer.cpp` * `KerberosScriptCoreLib/Source/InternalCalls.cs` * `KerberosScriptCoreLib/Source/Kerberos/Player.cs` * `KerberosScriptCoreLib/Source/Kerberos/Scene/Components.cs`
Added volume synchronization for `SoundAsset` in `AudioSource2DComponent`. Enhanced `XAudio2AudioManager` to stop and clean up existing sounds before replaying. Updated `VulkanContext` to retrieve buffer device addresses and added a TODO for extension checks. Marked move constructors/assignments as `noexcept` in `XAudio2AudioManager` and added a TODO for proper move behavior with COM pointers.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.