Feature/add vulkan device#12
Conversation
…ling/Segfault into feature/add_vulkan_device
|
Warning Review limit reached
More reviews will be available in 54 minutes and 15 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughIntroduces two new Vulkan abstraction files ( ChangesVulkan Utility and Device Abstractions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/runtime/renderer/RHIVulkan.cpp (3)
1519-1524:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against zero physical devices before indexing
physicalDevices[0].If
vkEnumeratePhysicalDevicesreturns 0, Line 1524 is out-of-bounds and will crash. Add a count check and fail initialization cleanly when no Vulkan-capable device exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1519 - 1524, Add a bounds check after the second vkEnumeratePhysicalDevices call to verify that physicalDeviceCount is greater than 0 before accessing physicalDevices[0]. If the count is zero, log an appropriate error message and fail the initialization cleanly (e.g., return an error status or throw an exception) rather than allowing an out-of-bounds array access that would crash.
1121-1126:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate texture layout transition is still present and invalidates image-state flow.
transitionImageLayout(... TRANSFER_DST_OPTIMAL -> SHADER_READ_ONLY_OPTIMAL)is called twice consecutively. The second call repeats a transition that has already been applied and can trigger validation/runtime failures. Remove the duplicate call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1121 - 1126, The transitionImageLayout function is being called twice in succession with identical parameters (transitioning from VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL to VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL). Remove the duplicate transitionImageLayout call to maintain correct image state flow and avoid validation failures.
403-407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQueue create-info count is overwritten to 1 after preparing unique queue families.
You build
queueCreateInfosfrom unique graphics/present families, assign it tocreateInfo, then forcequeueCreateInfoCount = 1. On hardware where graphics and present families differ, logical device creation/queue retrieval can fail or be incomplete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 403 - 407, The queueCreateInfoCount is being overwritten to 1 after it has been correctly set to the size of queueCreateInfos. Remove the line that sets createInfo.queueCreateInfoCount to 1 (the second assignment in the diff block). The queueCreateInfoCount should remain equal to queueCreateInfos.size() to properly handle cases where graphics and present queue families are different, ensuring all queue create infos are passed to the logical device creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Line 1: The file RHIVulkan.cpp is missing the required MIT license header
block at the beginning. Add the standard MIT license header comment block as the
very first lines of the file, before the `#include "RHI.h"` statement, to comply
with repository source-file requirements.
In `@src/runtime/renderer/vulkandevice.cpp`:
- Around line 5-7: The method isComplete() is declared in vulkandevice.h but has
no implementation in vulkandevice.cpp. Add a definition for the isComplete()
method in vulkandevice.cpp alongside the existing isGraphicsComplete() method.
The isComplete() method should return a boolean indicating whether all required
queue family indices (graphicsFamily and presentFamily) have values, similar to
how isGraphicsComplete() works but potentially checking additional required
families if they exist in the QueueFamilyIndices struct.
- Line 1: The vulkandevice.cpp file is missing the required MIT license header
block at the top. Add the standard MIT license header block before the existing
`#include` "vulkandevice.h" statement to comply with the coding guidelines that
require all source files to include proper licensing information.
- Around line 13-21: The VulkanDevice lifecycle is incomplete: the destructor
VulkanDevice::~VulkanDevice() is empty and does not release resources, while the
init() method returns true after only storing the surface without actually
initializing the mDevice member or other critical resources. You need to
implement proper initialization logic in init() that creates and configures the
Vulkan device based on the provided surface and requirements, returning false if
initialization fails, and then implement corresponding cleanup logic in the
destructor to release those allocated Vulkan resources when the object is
destroyed.
In `@src/runtime/renderer/vulkandevice.h`:
- Line 55: The VulkanDevice constructor that takes a single VkPhysicalDevice
parameter allows implicit conversions, which can accidentally create temporary
objects. Add the explicit keyword before the VulkanDevice constructor
declaration to prevent implicit conversions and require explicit object
construction calls.
- Around line 1-55: Add the MIT license header at the top of the file before the
pragma once directive, and fix the Doxygen documentation for the VulkanDevice
constructor to use the correct parameter name. Change the `@param` documentation
from `@param` instance to `@param` physicalDevice to match the actual constructor
parameter in the VulkanDevice class.
In `@src/runtime/renderer/vulkantypes.h`:
- Line 1: The file src/runtime/renderer/vulkantypes.h is missing the required
MIT license header at the start. Add the project's MIT license header block
before the `#pragma` once directive at the beginning of the file to comply with
repository licensing requirements.
- Around line 7-11: The BufferUsage enum in vulkantypes.h is properly defined
with bit flag values, but the conversion logic in vulkanbuffer.cpp at lines
25-38 incorrectly uses equality checks (==) instead of bitwise membership
checks. Update the conversion logic to use bitwise AND operations (&) when
checking if specific BufferUsage flags are set, so that combined flag values are
properly handled instead of silently mapping to zero. For example, change
conditions like `bufferUsage == BufferUsage::VertexBuffer` to `(bufferUsage &
BufferUsage::VertexBuffer) != 0` or equivalent bitwise checks to correctly
detect which flags are present in the combined usage value.
In `@src/runtime/renderer/vulkanutils.cpp`:
- Line 1: The file vulkanutils.cpp is missing the required MIT license header at
the top. Add the MIT license header text before the first include statement (the
include "vulkanutils.h" line). The license header should be the very first
content in the file, followed by a blank line, and then the existing includes
and code.
In `@src/runtime/renderer/vulkanutils.h`:
- Around line 1-20: Add an MIT license header comment block at the very
beginning of the file (before the `#pragma` once directive). Additionally, add
Doxygen-style documentation to the Vertex struct by including a brief
description using `@brief` tags. For the two static methods
getAttributeDescriptions() and getBindingDescription(), add Doxygen comments
with `@brief` and `@return` tags to document what each method does and what it
returns. Ensure all documentation follows the header API guidelines by providing
clear, concise descriptions for the struct and its public interface.
- Around line 18-19: The public header vulkanutils.h exposes std::array in the
return type of getAttributeDescriptions() method without including the <array>
header, which can cause compilation failures. Add `#include` <array> to the
includes section at the top of the file to ensure std::array is properly
available whenever this header is included.
---
Outside diff comments:
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 1519-1524: Add a bounds check after the second
vkEnumeratePhysicalDevices call to verify that physicalDeviceCount is greater
than 0 before accessing physicalDevices[0]. If the count is zero, log an
appropriate error message and fail the initialization cleanly (e.g., return an
error status or throw an exception) rather than allowing an out-of-bounds array
access that would crash.
- Around line 1121-1126: The transitionImageLayout function is being called
twice in succession with identical parameters (transitioning from
VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL to
VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL). Remove the duplicate
transitionImageLayout call to maintain correct image state flow and avoid
validation failures.
- Around line 403-407: The queueCreateInfoCount is being overwritten to 1 after
it has been correctly set to the size of queueCreateInfos. Remove the line that
sets createInfo.queueCreateInfoCount to 1 (the second assignment in the diff
block). The queueCreateInfoCount should remain equal to queueCreateInfos.size()
to properly handle cases where graphics and present queue families are
different, ensuring all queue create infos are passed to the logical device
creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c67da1eb-593a-4b8e-9acd-70698ef703a7
📒 Files selected for processing (9)
src/runtime/CMakeLists.txtsrc/runtime/renderer/RHIVulkan.cppsrc/runtime/renderer/vulkanbuffer.cppsrc/runtime/renderer/vulkanbuffer.hsrc/runtime/renderer/vulkandevice.cppsrc/runtime/renderer/vulkandevice.hsrc/runtime/renderer/vulkantypes.hsrc/runtime/renderer/vulkanutils.cppsrc/runtime/renderer/vulkanutils.h
💤 Files with no reviewable changes (1)
- src/runtime/renderer/vulkanbuffer.h
| @@ -1,5 +1,6 @@ | |||
| #include "RHI.h" | |||
There was a problem hiding this comment.
Missing MIT license header in modified .cpp file.
Please add the required MIT header block to satisfy repository source-file requirements.
As per coding guidelines, "Include MIT license header in all source files".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/RHIVulkan.cpp` at line 1, The file RHIVulkan.cpp is
missing the required MIT license header block at the beginning. Add the standard
MIT license header comment block as the very first lines of the file, before the
`#include "RHI.h"` statement, to comply with repository source-file
requirements.
Source: Coding guidelines
| @@ -0,0 +1,30 @@ | |||
| #include "vulkandevice.h" | |||
There was a problem hiding this comment.
Missing MIT license header in new .cpp file.
Please add the required MIT header block at the top of this file.
As per coding guidelines, "Include MIT license header in all source files".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkandevice.cpp` at line 1, The vulkandevice.cpp file
is missing the required MIT license header block at the top. Add the standard
MIT license header block before the existing `#include` "vulkandevice.h" statement
to comply with the coding guidelines that require all source files to include
proper licensing information.
Source: Coding guidelines
| bool QueueFamilyIndices::isGraphicsComplete() const { | ||
| return graphicsFamily.has_value() && presentFamily.has_value(); | ||
| } |
There was a problem hiding this comment.
QueueFamilyIndices::isComplete() is declared but not defined.
isComplete() is in vulkandevice.h (Line 18) but this TU only defines isGraphicsComplete(). Any call to isComplete() will fail at link time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkandevice.cpp` around lines 5 - 7, The method
isComplete() is declared in vulkandevice.h but has no implementation in
vulkandevice.cpp. Add a definition for the isComplete() method in
vulkandevice.cpp alongside the existing isGraphicsComplete() method. The
isComplete() method should return a boolean indicating whether all required
queue family indices (graphicsFamily and presentFamily) have values, similar to
how isGraphicsComplete() works but potentially checking additional required
families if they exist in the QueueFamilyIndices struct.
| VulkanDevice::~VulkanDevice() { | ||
|
|
||
| } | ||
|
|
||
| bool VulkanDevice::init(VkSurfaceKHR surface, const DeviceRequirements& requirements) { | ||
| mSurface = surface; | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Lifecycle contract is incomplete: destructor is empty and init() reports success without initialization.
~VulkanDevice() does not release resources, and init() returns true after only storing mSurface. This exposes a “successful” object with mDevice still null and shifts cleanup burden to callers.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 13-13: Use "=default" instead of the default implementation of this special member functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkandevice.cpp` around lines 13 - 21, The VulkanDevice
lifecycle is incomplete: the destructor VulkanDevice::~VulkanDevice() is empty
and does not release resources, while the init() method returns true after only
storing the surface without actually initializing the mDevice member or other
critical resources. You need to implement proper initialization logic in init()
that creates and configures the Vulkan device based on the provided surface and
requirements, returning false if initialization fails, and then implement
corresponding cleanup logic in the destructor to release those allocated Vulkan
resources when the object is destroyed.
| #pragma once | ||
|
|
||
| #include "volk.h" | ||
| #include "core/segfault.h" | ||
| #include <vector> | ||
| #include <optional> | ||
| #include <string> | ||
| #include <cstdint> | ||
|
|
||
| namespace segfault::renderer { | ||
|
|
||
| struct QueueFamilyIndices { | ||
| std::optional<uint32_t> graphicsFamily; | ||
| std::optional<uint32_t> presentFamily; | ||
| std::optional<uint32_t> computeFamily; | ||
| std::optional<uint32_t> transferFamily; | ||
|
|
||
| bool isComplete() const; | ||
| bool isGraphicsComplete() const; | ||
| }; | ||
|
|
||
| struct DeviceRequirements { | ||
| std::vector<const char*> requiredExtensions; | ||
| std::vector<const char*> optionalExtensions; | ||
| VkPhysicalDeviceFeatures requiredFeatures{}; | ||
| VkPhysicalDeviceFeatures optionalFeatures{}; | ||
| bool requirePresentQueue{ false }; | ||
| bool requireComputeQueue{ false }; | ||
| bool requireTransferQueue{ false }; | ||
| }; | ||
|
|
||
| struct DeviceProperties { | ||
| VkPhysicalDeviceProperties properties{}; | ||
| VkPhysicalDeviceFeatures features{}; | ||
| VkPhysicalDeviceMemoryProperties memoryProperties{}; | ||
| std::vector<VkExtensionProperties> availableExtensions; | ||
| std::vector<VkQueueFamilyProperties> queueFamilies; | ||
| }; | ||
|
|
||
| //--------------------------------------------------------------------------------------------- | ||
| /// @class VulkanDevice | ||
| /// @brief Encapsulates a Vulkan physical and logical device with queue management. | ||
| /// | ||
| /// This class manages the selection, creation, and lifecycle of Vulkan devices. | ||
| /// It provides access to device properties, features, queues, and memory management. | ||
| //--------------------------------------------------------------------------------------------- | ||
| class SEGFAULT_EXPORT VulkanDevice final { | ||
| public: | ||
| // Disallow copying | ||
| VulkanDevice(const VulkanDevice&) = delete; | ||
| VulkanDevice& operator=(const VulkanDevice&) = delete; | ||
|
|
||
| /// @brief Constructs a VulkanDevice. | ||
| /// @param instance The Vulkan instance. | ||
| VulkanDevice(VkPhysicalDevice physicalDevice); |
There was a problem hiding this comment.
Header docs/compliance need cleanup (MIT header + incorrect Doxygen param).
MIT file header is missing, and Line 54 documents @param instance while the constructor parameter is physicalDevice.
As per coding guidelines, "Include MIT license header in all source files" and "**/*.h: Use Doxygen-style comments in headers with @brief, @param, and @return tags".
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 55-55: Add the "explicit" keyword to this constructor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkandevice.h` around lines 1 - 55, Add the MIT license
header at the top of the file before the pragma once directive, and fix the
Doxygen documentation for the VulkanDevice constructor to use the correct
parameter name. Change the `@param` documentation from `@param` instance to `@param`
physicalDevice to match the actual constructor parameter in the VulkanDevice
class.
Source: Coding guidelines
| @@ -0,0 +1,13 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Missing required MIT license header in new source file.
Line 1 starts directly with #pragma once; this header needs the project MIT license header block per repo rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkantypes.h` at line 1, The file
src/runtime/renderer/vulkantypes.h is missing the required MIT license header at
the start. Add the project's MIT license header block before the `#pragma` once
directive at the beginning of the file to comply with repository licensing
requirements.
Source: Coding guidelines
| enum class BufferUsage : uint32_t { | ||
| VertexBuffer = 0x1, | ||
| IndexBuffer = 0x2, | ||
| UniformBuffer = 0x4 | ||
| }; |
There was a problem hiding this comment.
BufferUsage is defined as bit flags, but current contract behaves like a single-choice enum.
BufferUsage uses bit values (0x1, 0x2, 0x4), but downstream conversion in src/runtime/renderer/vulkanbuffer.cpp:25-38 checks == instead of bitwise membership. Combined usage flags will silently map to 0, which breaks buffer creation behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkantypes.h` around lines 7 - 11, The BufferUsage enum
in vulkantypes.h is properly defined with bit flag values, but the conversion
logic in vulkanbuffer.cpp at lines 25-38 incorrectly uses equality checks (==)
instead of bitwise membership checks. Update the conversion logic to use bitwise
AND operations (&) when checking if specific BufferUsage flags are set, so that
combined flag values are properly handled instead of silently mapping to zero.
For example, change conditions like `bufferUsage == BufferUsage::VertexBuffer`
to `(bufferUsage & BufferUsage::VertexBuffer) != 0` or equivalent bitwise checks
to correctly detect which flags are present in the combined usage value.
| @@ -0,0 +1,65 @@ | |||
| #include "vulkanutils.h" | |||
There was a problem hiding this comment.
Missing MIT license header in new .cpp file.
This source file should include the required MIT license header at the top.
As per coding guidelines, "Include MIT license header in all source files".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkanutils.cpp` at line 1, The file vulkanutils.cpp is
missing the required MIT license header at the top. Add the MIT license header
text before the first include statement (the include "vulkanutils.h" line). The
license header should be the very first content in the file, followed by a blank
line, and then the existing includes and code.
Source: Coding guidelines
| #pragma once | ||
|
|
||
| #include "volk.h" | ||
|
|
||
| #include <vector> | ||
|
|
||
| #define GLM_FORCE_RADIANS | ||
| #include <glm/glm.hpp> | ||
| #include <glm/gtc/matrix_transform.hpp> | ||
|
|
||
| namespace segfault::renderer { | ||
|
|
||
| struct Vertex { | ||
| glm::vec3 pos{}; | ||
| glm::vec3 color{}; | ||
| glm::vec2 texCoord{}; | ||
|
|
||
| static std::array<VkVertexInputAttributeDescription, 3> getAttributeDescriptions(); | ||
| static VkVertexInputBindingDescription getBindingDescription(); | ||
| }; |
There was a problem hiding this comment.
Header guideline compliance is incomplete for this new file.
MIT header is missing, and Vertex/its methods are declared without Doxygen @brief/@return tags required for header APIs.
As per coding guidelines, "Include MIT license header in all source files" and "**/*.h: Use Doxygen-style comments in headers with @brief, @param, and @return tags".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkanutils.h` around lines 1 - 20, Add an MIT license
header comment block at the very beginning of the file (before the `#pragma` once
directive). Additionally, add Doxygen-style documentation to the Vertex struct
by including a brief description using `@brief` tags. For the two static methods
getAttributeDescriptions() and getBindingDescription(), add Doxygen comments
with `@brief` and `@return` tags to document what each method does and what it
returns. Ensure all documentation follows the header API guidelines by providing
clear, concise descriptions for the struct and its public interface.
Source: Coding guidelines
| static std::array<VkVertexInputAttributeDescription, 3> getAttributeDescriptions(); | ||
| static VkVertexInputBindingDescription getBindingDescription(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify public headers using std::array include <array> directly
rg -n --type=cpp --type=h '\bstd::array\b' src/runtime/renderer/vulkanutils.h -C2
rg -n --type=cpp --type=h '^`#include` <array>' src/runtime/renderer/vulkanutils.hRepository: kimkulling/Segfault
Length of output: 280
🏁 Script executed:
cat -n src/runtime/renderer/vulkanutils.hRepository: kimkulling/Segfault
Length of output: 1705
Public header uses std::array without including <array>.
Line 18 exposes std::array in the API, but this header does not directly include <array>. While <vector> is included, it does not guarantee std::array is available. This can break compilation for translation units that include this header without prior inclusion of <array> elsewhere.
18: static std::array<VkVertexInputAttributeDescription, 3> getAttributeDescriptions();
Add #include <array> to the includes section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/renderer/vulkanutils.h` around lines 18 - 19, The public header
vulkanutils.h exposes std::array in the return type of
getAttributeDescriptions() method without including the <array> header, which
can cause compilation failures. Add `#include` <array> to the includes section at
the top of the file to ensure std::array is properly available whenever this
header is included.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores