Refactorings - #16
Conversation
WalkthroughThe PR reorganizes Vulkan renderer state around ChangesVulkan renderer
Behavior tree declarations
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/runtime/renderer/RHIVulkan.cpp (4)
1116-1148: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate (and stale) texture layout transition call.
Lines 1142 and 1144 issue the identical
transitionImageLayout(mTextureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL)call back-to-back. The second call is not just redundant — it also submits a staleoldLayout(the image is alreadySHADER_READ_ONLY_OPTIMALafter the first call), and each call performs a synchronousvkQueueWaitIdleviaendSingleTimeCommands.🐛 Proposed fix
transitionImageLayout(mTextureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL); - - transitionImageLayout(mTextureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);🤖 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 1116 - 1148, Remove the duplicate second transitionImageLayout call in RHIImpl::createTextureImage, keeping the first transition from VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL to VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL before staging-buffer cleanup.
382-439: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
createLogicalDeviceinconsistently reads frommQueueFamilyIndicesinstead of its ownqfIndicesparameter.Line 383 refreshes the parameter
qfIndices, but lines 386 and 400 readmQueueFamilyIndices(the struct member) instead, while lines 435-436 correctly useqfIndices. This only produces correct results today because the sole call site (mImpl->createLogicalDevice(mImpl->mEnableValidationLayers, mImpl->mPhysicalDevice, mImpl->mQueueFamilyIndices)) happens to passmQueueFamilyIndicesby reference. If ever called with a differentQueueFamilyIndicesinstance, lines 386/400 would silently use stale/unrelated data frommQueueFamilyIndices, risking astd::bad_optional_accessif it's still empty.🐛 Proposed fix
- std::set<uint32_t> uniqueQueueFamilies = { mQueueFamilyIndices.graphicsFamily.value(), mQueueFamilyIndices.presentFamily.value() }; + std::set<uint32_t> uniqueQueueFamilies = { qfIndices.graphicsFamily.value(), qfIndices.presentFamily.value() }; ... - queueCreateInfo.queueFamilyIndex = mQueueFamilyIndices.graphicsFamily.value(); + queueCreateInfo.queueFamilyIndex = qfIndices.graphicsFamily.value();🤖 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 382 - 439, Use the refreshed qfIndices parameter consistently throughout createLogicalDevice: replace the mQueueFamilyIndices reads when constructing uniqueQueueFamilies and configuring the queue create info with qfIndices. Preserve the existing queue retrieval and device creation behavior.
1491-1530: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winInverted validation-layer support check.
if (mImpl->mEnableValidationLayers && !mImpl->checkValidationLayerSupport())enables the validation-layercreateInfofields exactly when support checking fails, and takes theelse(disabling layers) when support checking succeeds — the opposite of the intended behavior. If validation is requested butVK_LAYER_KHRONOS_validationisn't installed, this forcesvkCreateInstanceto request it anyway (riskingVK_ERROR_LAYER_NOT_PRESENT), while a system that does have it installed never gets it enabled.🐛 Proposed fix
- if (mImpl->mEnableValidationLayers && !mImpl->checkValidationLayerSupport()) { + if (mImpl->mEnableValidationLayers && mImpl->checkValidationLayerSupport()) {🤖 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 1491 - 1530, Correct the condition in the validation-layer setup around mImpl->checkValidationLayerSupport() so validation layers are enabled only when mImpl->mEnableValidationLayers is true and support checking succeeds. Preserve the existing fallback that clears enabled layers and pNext when validation is disabled or unsupported.
648-773: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
mVertShaderModule/mFragShaderModuleare dead fields — the shutdown destroy calls on them are permanent no-ops.
createGraphicsPipelinecreates localvertShaderModule/fragShaderModule(lines 652-653) and destroys those locals immediately after pipeline creation (lines 771-772). The struct'smVertShaderModule/mFragShaderModulemembers are never assigned, soRHI::shutdown()'svkDestroyShaderModule(mImpl->mDevice, mImpl->mFragShaderModule, nullptr)/...mVertShaderModule...calls always operate on null handles. No leak occurs (locals are already cleaned up), but the members are misleading dead state.♻️ Proposed fix (remove the dead members and no-op shutdown calls)
- VkShaderModule mVertShaderModule{}; - VkShaderModule mFragShaderModule{};And in
RHI::shutdown():- vkDestroyShaderModule(mImpl->mDevice, mImpl->mFragShaderModule, nullptr); - vkDestroyShaderModule(mImpl->mDevice, mImpl->mVertShaderModule, nullptr);🤖 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 648 - 773, Remove the unused mVertShaderModule and mFragShaderModule members from the RHI implementation, and remove their corresponding vkDestroyShaderModule calls from RHI::shutdown(). Keep the local shader module creation and destruction in RHIImpl::createGraphicsPipeline unchanged.
🧹 Nitpick comments (3)
src/runtime/renderer/vulkantypes.h (1)
14-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Doxygen comments to the new
QueueFamilyIndicespublic API.The struct and
isComplete()have no@brief/documentation, unlikeVulkanDeviceelsewhere in this PR which is properly documented.As per coding guidelines, "Use Doxygen-style comments in header files for public APIs (
@brief,@param,@return, etc.)."🤖 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 14 - 23, Add Doxygen documentation to the public QueueFamilyIndices struct and its isComplete() method in vulkantypes.h, including `@brief` descriptions and an `@return` description for isComplete().Source: Coding guidelines
src/runtime/renderer/vulkandevice.h (1)
36-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
DeviceRequirements/DevicePropertiespublic structs.Neither struct has Doxygen comments, unlike
VulkanDevicejust below them.As per coding guidelines, "Use Doxygen-style comments in header files for public APIs (
@brief,@param,@return, etc.)."🤖 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 36 - 52, Document the public DeviceRequirements and DeviceProperties structs in Doxygen style, adding concise `@brief` descriptions and documenting their public fields, including extension lists, feature requirements, queue requirements, device properties, available extensions, and queue families.Source: Coding guidelines
src/runtime/renderer/vulkanbuffer.h (1)
39-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic methods still lack Doxygen
@brief/@param/@returntags.Only the class-level summary was added;
init,cleanup,map,unmap,bind,copyTo, and the getters remain undocumented.As per coding guidelines, "Use Doxygen-style comments in header files for public APIs (
@brief,@param,@return, etc.)."🤖 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/vulkanbuffer.h` around lines 39 - 52, Add Doxygen comments for each public method in VulkanBuffer: init, cleanup, map, unmap, bind, copyTo, getBuffer, getMemory, and getSize. Include an appropriate `@brief`, document every parameter with `@param`, and describe return values with `@return` for non-void methods, while preserving the existing declarations and behavior.Source: Coding guidelines
🤖 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`:
- Around line 1569-1602: Remove the redundant vkDestroySwapchainKHR call from
RHI::shutdown because mImpl->cleanupSwapChain() already destroys the swapchain.
Keep cleanupSwapChain() as the single owner of swapchain destruction and leave
the remaining shutdown resource cleanup unchanged.
- Around line 1532-1553: Update the Vulkan initialization flow after
vkEnumeratePhysicalDevices to handle enumeration failures or zero devices before
accessing physicalDevices[0], then select only a device accepted by
isDeviceSuitable(). Check the return value of SDL_Vulkan_CreateSurface and the
result of createLogicalDevice, returning false immediately when either fails;
preserve the existing setup sequence only after a valid surface and suitable
device are established.
In `@src/runtime/renderer/vulkanbuffer.h`:
- Line 38: Add SEGFAULT_EXPORT to the VulkanBuffer class declaration so it is
exported from the shared runtime library and remains usable through
VulkanDevice::createBuffer() for external consumers.
In `@src/runtime/renderer/vulkandevice.h`:
- Around line 67-69: Update the Doxygen comment for the VulkanDevice constructor
to document the actual physicalDevice parameter, replacing the stale instance
description while preserving the existing `@brief` and public API documentation
style.
---
Outside diff comments:
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 1116-1148: Remove the duplicate second transitionImageLayout call
in RHIImpl::createTextureImage, keeping the first transition from
VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL to VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL
before staging-buffer cleanup.
- Around line 382-439: Use the refreshed qfIndices parameter consistently
throughout createLogicalDevice: replace the mQueueFamilyIndices reads when
constructing uniqueQueueFamilies and configuring the queue create info with
qfIndices. Preserve the existing queue retrieval and device creation behavior.
- Around line 1491-1530: Correct the condition in the validation-layer setup
around mImpl->checkValidationLayerSupport() so validation layers are enabled
only when mImpl->mEnableValidationLayers is true and support checking succeeds.
Preserve the existing fallback that clears enabled layers and pNext when
validation is disabled or unsupported.
- Around line 648-773: Remove the unused mVertShaderModule and mFragShaderModule
members from the RHI implementation, and remove their corresponding
vkDestroyShaderModule calls from RHI::shutdown(). Keep the local shader module
creation and destruction in RHIImpl::createGraphicsPipeline unchanged.
---
Nitpick comments:
In `@src/runtime/renderer/vulkanbuffer.h`:
- Around line 39-52: Add Doxygen comments for each public method in
VulkanBuffer: init, cleanup, map, unmap, bind, copyTo, getBuffer, getMemory, and
getSize. Include an appropriate `@brief`, document every parameter with `@param`,
and describe return values with `@return` for non-void methods, while preserving
the existing declarations and behavior.
In `@src/runtime/renderer/vulkandevice.h`:
- Around line 36-52: Document the public DeviceRequirements and DeviceProperties
structs in Doxygen style, adding concise `@brief` descriptions and documenting
their public fields, including extension lists, feature requirements, queue
requirements, device properties, available extensions, and queue families.
In `@src/runtime/renderer/vulkantypes.h`:
- Around line 14-23: Add Doxygen documentation to the public QueueFamilyIndices
struct and its isComplete() method in vulkantypes.h, including `@brief`
descriptions and an `@return` description for isComplete().
🪄 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: e1a39763-4f15-4de0-838c-c5c1bab44a3a
📒 Files selected for processing (7)
src/runtime/ai/behavior_tree.hsrc/runtime/renderer/RHIVulkan.cppsrc/runtime/renderer/vulkanbuffer.cppsrc/runtime/renderer/vulkanbuffer.hsrc/runtime/renderer/vulkandevice.cppsrc/runtime/renderer/vulkandevice.hsrc/runtime/renderer/vulkantypes.h
| result = vkCreateInstance(&createInfo, nullptr, &mImpl->mInstance); | ||
| if (result != VK_SUCCESS) { | ||
| return false; | ||
| } | ||
| volkLoadInstance(mImpl->instance); | ||
| volkLoadInstance(mImpl->mInstance); | ||
|
|
||
| uint32_t physicalDeviceCount = 0; | ||
| vkEnumeratePhysicalDevices(mImpl->instance, &physicalDeviceCount, nullptr); | ||
| uint32_t physicalDeviceCount{ 0 }; | ||
| vkEnumeratePhysicalDevices(mImpl->mInstance, &physicalDeviceCount, nullptr); | ||
| std::vector<VkPhysicalDevice> physicalDevices(physicalDeviceCount); | ||
| vkEnumeratePhysicalDevices(mImpl->instance, &physicalDeviceCount, physicalDevices.data()); | ||
| mImpl->physicalDevice = physicalDevices[0]; | ||
| vkEnumeratePhysicalDevices(mImpl->mInstance, &physicalDeviceCount, physicalDevices.data()); | ||
| mImpl->mPhysicalDevice = physicalDevices[0]; | ||
|
|
||
| SDL_Vulkan_CreateSurface(mImpl->window, mImpl->instance, &mImpl->surface); | ||
| SDL_Vulkan_CreateSurface(mImpl->mWindow, mImpl->mInstance, &mImpl->mSurface); | ||
|
|
||
| mImpl->createLogicalDevice(mImpl->enableValidationLayers, mImpl->physicalDevice, mImpl->queueFamilyIndices); | ||
| mImpl->createLogicalDevice(mImpl->mEnableValidationLayers, mImpl->mPhysicalDevice, mImpl->mQueueFamilyIndices); | ||
|
|
||
| mImpl->createSwapChain(); | ||
| mImpl->createImageViews(); | ||
| mImpl->createRenderPass(); | ||
| mImpl->createDescriptorSetLayout(); | ||
| mImpl->createGraphicsPipeline(); | ||
| mImpl->createCommandPool(mImpl->queueFamilyIndices); | ||
| mImpl->createCommandPool(mImpl->mQueueFamilyIndices); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline src/runtime/renderer/RHIVulkan.cpp --view expanded || true
printf '\n== relevant symbols ==\n'
rg -n "isDeviceSuitable|checkDeviceExtensionSupport|createLogicalDevice|SDL_Vulkan_CreateSurface|vkEnumeratePhysicalDevices|createTextureSampler|samplerInfo.anisotropyEnable" src/runtime/renderer/RHIVulkan.cpp
printf '\n== surrounding init lines ==\n'
sed -n '1500,1585p' src/runtime/renderer/RHIVulkan.cpp
printf '\n== helper implementations ==\n'
sed -n '1,700p' src/runtime/renderer/RHIVulkan.cpp | rg -n "isDeviceSuitable|checkDeviceExtensionSupport|createLogicalDevice|createTextureSampler|findQueueFamilies|pickPhysicalDevice|createSurface|SDL_Vulkan_CreateSurface" -n -A40 -B20
printf '\n== all call sites of createLogicalDevice ==\n'
rg -n "createLogicalDevice\\(" src/runtime/rendererRepository: kimkulling/Segfault
Length of output: 32462
🏁 Script executed:
set -euo pipefail
ast-grep outline src/runtime/renderer/RHIVulkan.cpp --view expanded || true
rg -n "isDeviceSuitable|checkDeviceExtensionSupport|createLogicalDevice|SDL_Vulkan_CreateSurface|vkEnumeratePhysicalDevices|createTextureSampler|samplerInfo.anisotropyEnable" src/runtime/renderer/RHIVulkan.cpp
sed -n '1500,1585p' src/runtime/renderer/RHIVulkan.cpp
sed -n '1,700p' src/runtime/renderer/RHIVulkan.cpp | rg -n "isDeviceSuitable|checkDeviceExtensionSupport|createLogicalDevice|createTextureSampler|findQueueFamilies|pickPhysicalDevice|createSurface|SDL_Vulkan_CreateSurface" -n -A40 -B20
rg -n "createLogicalDevice\\(" src/runtime/rendererRepository: kimkulling/Segfault
Length of output: 32313
🌐 Web query:
Vulkan vkEnumeratePhysicalDevices may return zero devices SDL_Vulkan_CreateSurface return value documentation bool
💡 Result:
When using vkEnumeratePhysicalDevices, receiving a count of zero physical devices typically indicates that no Vulkan-compatible hardware or drivers were detected by the Vulkan loader [1][2]. It does not necessarily mean the function failed; it can return VK_SUCCESS with a count of zero if the system has no Vulkan-capable devices, or it may return an error (such as VK_ERROR_INITIALIZATION_FAILED) if the loader itself encountered a problem during discovery [2][3]. Regarding SDL_Vulkan_CreateSurface, the return type has changed across different versions of the Simple Directmedia Layer (SDL) library: In SDL 2.x, SDL_Vulkan_CreateSurface returns an SDL_bool type, where SDL_TRUE indicates success and SDL_FALSE indicates failure [4][5]. In SDL 3.x, the function signature was updated to return an integer (int). Specifically, it returns 0 on success and -1 on failure [6][7][8]. If an error occurs, you can use SDL_GetError to retrieve detailed information [6][7][8]. Always check the specific documentation for the version of SDL you are using to ensure correct implementation [6][4].
Citations:
- 1: https://stackoverflow.com/questions/39275836/vkenumeratephysicaldevices-assigning-0-to-variable-gpu-count
- 2: vkEnumeratePhysicalDevices returns an error if there are no devices KhronosGroup/Vulkan-Loader#104
- 3: https://stackoverflow.com/questions/63274891/vulkan-cannot-find-physical-devices-on-windows-10
- 4: https://wiki.libsdl.org/SDL2/SDL_Vulkan_CreateSurface
- 5: https://github.com/libsdl-org/SDL/blob/SDL2/include/SDL_vulkan.h
- 6: https://wiki.libsdl.org/SDL3/SDL_Vulkan_CreateSurface
- 7: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_vulkan.h
- 8: https://discourse.libsdl.org/t/sdl-vulkan-sdl-vulkan-createsurface-now-returns-the-usual-int-0-ok-1-error/52318
Guard Vulkan init before selecting the adapter
physicalDevices[0]can go out of bounds whenvkEnumeratePhysicalDevicesreturns 0.- Check
SDL_Vulkan_CreateSurface(...)andcreateLogicalDevice(...)here, and select only a device that passesisDeviceSuitable(); otherwise the init path can continue with an invalid surface/device and fail later increateTextureSampler().
🤖 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 1532 - 1553, Update the
Vulkan initialization flow after vkEnumeratePhysicalDevices to handle
enumeration failures or zero devices before accessing physicalDevices[0], then
select only a device accepted by isDeviceSuitable(). Check the return value of
SDL_Vulkan_CreateSurface and the result of createLogicalDevice, returning false
immediately when either fails; preserve the existing setup sequence only after a
valid surface and suitable device are established.
| bool RHI::shutdown() { | ||
| mImpl->cleanupSwapChain(); | ||
| vkDestroyImage(mImpl->device, mImpl->textureImage, nullptr); | ||
| vkDestroySampler(mImpl->device, mImpl->textureSampler, nullptr); | ||
| vkDestroyImageView(mImpl->device, mImpl->textureImageView, nullptr); | ||
| vkDestroyImage(mImpl->mDevice, mImpl->mTextureImage, nullptr); | ||
| vkDestroySampler(mImpl->mDevice, mImpl->mTextureSampler, nullptr); | ||
| vkDestroyImageView(mImpl->mDevice, mImpl->mTextureImageView, nullptr); | ||
|
|
||
| vkFreeMemory(mImpl->device, mImpl->textureImageMemory, nullptr); | ||
| vkFreeMemory(mImpl->mDevice, mImpl->mTextureImageMemory, nullptr); | ||
| for (size_t i = 0; i < RHIImpl::MAX_FRAMES_IN_FLIGHT; i++) { | ||
| vkDestroyBuffer(mImpl->device, mImpl->uniformBuffers[i], nullptr); | ||
| vkFreeMemory(mImpl->device, mImpl->uniformBuffersMemory[i], nullptr); | ||
| vkDestroyBuffer(mImpl->mDevice, mImpl->mUniformBuffers[i], nullptr); | ||
| vkFreeMemory(mImpl->mDevice, mImpl->mUniformBuffersMemory[i], nullptr); | ||
| } | ||
| vkDestroyDescriptorPool(mImpl->device, mImpl->descriptorPool, nullptr); | ||
| vkDestroyDescriptorSetLayout(mImpl->device, mImpl->descriptorSetLayout, nullptr); | ||
| vkDestroyBuffer(mImpl->device, mImpl->vertexBuffer, nullptr); | ||
| vkFreeMemory(mImpl->device, mImpl->vertexBufferMemory, nullptr); | ||
| vkDestroyDescriptorPool(mImpl->mDevice, mImpl->mDescriptorPool, nullptr); | ||
| vkDestroyDescriptorSetLayout(mImpl->mDevice, mImpl->mDescriptorSetLayout, nullptr); | ||
| vkDestroyBuffer(mImpl->mDevice, mImpl->mVertexBuffer, nullptr); | ||
| vkFreeMemory(mImpl->mDevice, mImpl->mVertexBufferMemory, nullptr); | ||
|
|
||
| vkDestroyBuffer(mImpl->device, mImpl->indexBuffer, nullptr); | ||
| vkFreeMemory(mImpl->device, mImpl->indexBufferMemory, nullptr); | ||
| vkDestroyBuffer(mImpl->mDevice, mImpl->mIndexBuffer, nullptr); | ||
| vkFreeMemory(mImpl->mDevice, mImpl->mIndexBufferMemory, nullptr); | ||
|
|
||
| for (size_t i = 0; i < RHIImpl::MAX_FRAMES_IN_FLIGHT; i++) { | ||
| vkDestroySemaphore(mImpl->device, mImpl->renderFinishedSemaphores[i], nullptr); | ||
| vkDestroySemaphore(mImpl->device, mImpl->imageAvailableSemaphores[i], nullptr); | ||
| vkDestroyFence(mImpl->device, mImpl->inFlightFences[i], nullptr); | ||
| vkDestroySemaphore(mImpl->mDevice, mImpl->mRenderFinishedSemaphores[i], nullptr); | ||
| vkDestroySemaphore(mImpl->mDevice, mImpl->mImageAvailableSemaphores[i], nullptr); | ||
| vkDestroyFence(mImpl->mDevice, mImpl->mInFlightFences[i], nullptr); | ||
| } | ||
|
|
||
| vkDestroyCommandPool(mImpl->device, mImpl->commandPool, nullptr); | ||
| vkDestroyCommandPool(mImpl->mDevice, mImpl->mCommandPool, nullptr); | ||
|
|
||
| vkDestroyShaderModule(mImpl->device, mImpl->fragShaderModule, nullptr); | ||
| vkDestroyShaderModule(mImpl->device, mImpl->vertShaderModule, nullptr); | ||
| vkDestroyPipelineLayout(mImpl->device, mImpl->pipelineLayout, nullptr); | ||
| vkDestroyRenderPass(mImpl->device, mImpl->renderPass, nullptr); | ||
| vkDestroyShaderModule(mImpl->mDevice, mImpl->mFragShaderModule, nullptr); | ||
| vkDestroyShaderModule(mImpl->mDevice, mImpl->mVertShaderModule, nullptr); | ||
| vkDestroyPipelineLayout(mImpl->mDevice, mImpl->mPipelineLayout, nullptr); | ||
| vkDestroyRenderPass(mImpl->mDevice, mImpl->mRenderPass, nullptr); | ||
|
|
||
| vkDestroySwapchainKHR(mImpl->device, mImpl->swapChain, nullptr); | ||
| vkDestroyDevice(mImpl->device, nullptr); | ||
| vkDestroySwapchainKHR(mImpl->mDevice, mImpl->mSwapChain, nullptr); | ||
| vkDestroyDevice(mImpl->mDevice, nullptr); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Swapchain is destroyed twice in RHI::shutdown.
Line 1570 calls mImpl->cleanupSwapChain(), whose body already calls vkDestroySwapchainKHR(mDevice, mSwapChain, nullptr) without resetting mSwapChain to VK_NULL_HANDLE. Line 1601 then calls vkDestroySwapchainKHR(mImpl->mDevice, mImpl->mSwapChain, nullptr) again on the same (already-destroyed) handle — Vulkan's valid-usage rules require the handle to be either valid or VK_NULL_HANDLE, so this double-destroy is undefined behavior and a known source of loader/driver crashes.
🐛 Proposed fix
vkDestroyCommandPool(mImpl->mDevice, mImpl->mCommandPool, nullptr);
vkDestroyShaderModule(mImpl->mDevice, mImpl->mFragShaderModule, nullptr);
vkDestroyShaderModule(mImpl->mDevice, mImpl->mVertShaderModule, nullptr);
vkDestroyPipelineLayout(mImpl->mDevice, mImpl->mPipelineLayout, nullptr);
vkDestroyRenderPass(mImpl->mDevice, mImpl->mRenderPass, nullptr);
- vkDestroySwapchainKHR(mImpl->mDevice, mImpl->mSwapChain, nullptr);
vkDestroyDevice(mImpl->mDevice, nullptr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool RHI::shutdown() { | |
| mImpl->cleanupSwapChain(); | |
| vkDestroyImage(mImpl->device, mImpl->textureImage, nullptr); | |
| vkDestroySampler(mImpl->device, mImpl->textureSampler, nullptr); | |
| vkDestroyImageView(mImpl->device, mImpl->textureImageView, nullptr); | |
| vkDestroyImage(mImpl->mDevice, mImpl->mTextureImage, nullptr); | |
| vkDestroySampler(mImpl->mDevice, mImpl->mTextureSampler, nullptr); | |
| vkDestroyImageView(mImpl->mDevice, mImpl->mTextureImageView, nullptr); | |
| vkFreeMemory(mImpl->device, mImpl->textureImageMemory, nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mTextureImageMemory, nullptr); | |
| for (size_t i = 0; i < RHIImpl::MAX_FRAMES_IN_FLIGHT; i++) { | |
| vkDestroyBuffer(mImpl->device, mImpl->uniformBuffers[i], nullptr); | |
| vkFreeMemory(mImpl->device, mImpl->uniformBuffersMemory[i], nullptr); | |
| vkDestroyBuffer(mImpl->mDevice, mImpl->mUniformBuffers[i], nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mUniformBuffersMemory[i], nullptr); | |
| } | |
| vkDestroyDescriptorPool(mImpl->device, mImpl->descriptorPool, nullptr); | |
| vkDestroyDescriptorSetLayout(mImpl->device, mImpl->descriptorSetLayout, nullptr); | |
| vkDestroyBuffer(mImpl->device, mImpl->vertexBuffer, nullptr); | |
| vkFreeMemory(mImpl->device, mImpl->vertexBufferMemory, nullptr); | |
| vkDestroyDescriptorPool(mImpl->mDevice, mImpl->mDescriptorPool, nullptr); | |
| vkDestroyDescriptorSetLayout(mImpl->mDevice, mImpl->mDescriptorSetLayout, nullptr); | |
| vkDestroyBuffer(mImpl->mDevice, mImpl->mVertexBuffer, nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mVertexBufferMemory, nullptr); | |
| vkDestroyBuffer(mImpl->device, mImpl->indexBuffer, nullptr); | |
| vkFreeMemory(mImpl->device, mImpl->indexBufferMemory, nullptr); | |
| vkDestroyBuffer(mImpl->mDevice, mImpl->mIndexBuffer, nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mIndexBufferMemory, nullptr); | |
| for (size_t i = 0; i < RHIImpl::MAX_FRAMES_IN_FLIGHT; i++) { | |
| vkDestroySemaphore(mImpl->device, mImpl->renderFinishedSemaphores[i], nullptr); | |
| vkDestroySemaphore(mImpl->device, mImpl->imageAvailableSemaphores[i], nullptr); | |
| vkDestroyFence(mImpl->device, mImpl->inFlightFences[i], nullptr); | |
| vkDestroySemaphore(mImpl->mDevice, mImpl->mRenderFinishedSemaphores[i], nullptr); | |
| vkDestroySemaphore(mImpl->mDevice, mImpl->mImageAvailableSemaphores[i], nullptr); | |
| vkDestroyFence(mImpl->mDevice, mImpl->mInFlightFences[i], nullptr); | |
| } | |
| vkDestroyCommandPool(mImpl->device, mImpl->commandPool, nullptr); | |
| vkDestroyCommandPool(mImpl->mDevice, mImpl->mCommandPool, nullptr); | |
| vkDestroyShaderModule(mImpl->device, mImpl->fragShaderModule, nullptr); | |
| vkDestroyShaderModule(mImpl->device, mImpl->vertShaderModule, nullptr); | |
| vkDestroyPipelineLayout(mImpl->device, mImpl->pipelineLayout, nullptr); | |
| vkDestroyRenderPass(mImpl->device, mImpl->renderPass, nullptr); | |
| vkDestroyShaderModule(mImpl->mDevice, mImpl->mFragShaderModule, nullptr); | |
| vkDestroyShaderModule(mImpl->mDevice, mImpl->mVertShaderModule, nullptr); | |
| vkDestroyPipelineLayout(mImpl->mDevice, mImpl->mPipelineLayout, nullptr); | |
| vkDestroyRenderPass(mImpl->mDevice, mImpl->mRenderPass, nullptr); | |
| vkDestroySwapchainKHR(mImpl->device, mImpl->swapChain, nullptr); | |
| vkDestroyDevice(mImpl->device, nullptr); | |
| vkDestroySwapchainKHR(mImpl->mDevice, mImpl->mSwapChain, nullptr); | |
| vkDestroyDevice(mImpl->mDevice, nullptr); | |
| bool RHI::shutdown() { | |
| mImpl->cleanupSwapChain(); | |
| vkDestroyImage(mImpl->mDevice, mImpl->mTextureImage, nullptr); | |
| vkDestroySampler(mImpl->mDevice, mImpl->mTextureSampler, nullptr); | |
| vkDestroyImageView(mImpl->mDevice, mImpl->mTextureImageView, nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mTextureImageMemory, nullptr); | |
| for (size_t i = 0; i < RHIImpl::MAX_FRAMES_IN_FLIGHT; i++) { | |
| vkDestroyBuffer(mImpl->mDevice, mImpl->mUniformBuffers[i], nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mUniformBuffersMemory[i], nullptr); | |
| } | |
| vkDestroyDescriptorPool(mImpl->mDevice, mImpl->mDescriptorPool, nullptr); | |
| vkDestroyDescriptorSetLayout(mImpl->mDevice, mImpl->mDescriptorSetLayout, nullptr); | |
| vkDestroyBuffer(mImpl->mDevice, mImpl->mVertexBuffer, nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mVertexBufferMemory, nullptr); | |
| vkDestroyBuffer(mImpl->mDevice, mImpl->mIndexBuffer, nullptr); | |
| vkFreeMemory(mImpl->mDevice, mImpl->mIndexBufferMemory, nullptr); | |
| for (size_t i = 0; i < RHIImpl::MAX_FRAMES_IN_FLIGHT; i++) { | |
| vkDestroySemaphore(mImpl->mDevice, mImpl->mRenderFinishedSemaphores[i], nullptr); | |
| vkDestroySemaphore(mImpl->mDevice, mImpl->mImageAvailableSemaphores[i], nullptr); | |
| vkDestroyFence(mImpl->mDevice, mImpl->mInFlightFences[i], nullptr); | |
| } | |
| vkDestroyCommandPool(mImpl->mDevice, mImpl->mCommandPool, nullptr); | |
| vkDestroyShaderModule(mImpl->mDevice, mImpl->mFragShaderModule, nullptr); | |
| vkDestroyShaderModule(mImpl->mDevice, mImpl->mVertShaderModule, nullptr); | |
| vkDestroyPipelineLayout(mImpl->mDevice, mImpl->mPipelineLayout, nullptr); | |
| vkDestroyRenderPass(mImpl->mDevice, mImpl->mRenderPass, nullptr); | |
| vkDestroyDevice(mImpl->mDevice, nullptr); |
🤖 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 1569 - 1602, Remove the
redundant vkDestroySwapchainKHR call from RHI::shutdown because
mImpl->cleanupSwapChain() already destroys the swapchain. Keep
cleanupSwapChain() as the single owner of swapchain destruction and leave the
remaining shutdown resource cleanup unchanged.
| /// mapping, binding, and copying of data to the buffer, as well as cleanup of | ||
| /// resources when they are no longer needed. | ||
| //--------------------------------------------------------------------------------------------- | ||
| class VulkanBuffer { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files | rg 'src/runtime/renderer/vulkan(buffer|device)\.(h|cpp)$|src/runtime/renderer/.*vulkan.*\.(h|cpp)$'
printf '\n== Outline: vulkanbuffer.h ==\n'
ast-grep outline src/runtime/renderer/vulkanbuffer.h --view expanded || true
printf '\n== Outline: vulkandevice.h ==\n'
ast-grep outline src/runtime/renderer/vulkandevice.h --view expanded || true
printf '\n== vulkanbuffer.h (selected lines) ==\n'
cat -n src/runtime/renderer/vulkanbuffer.h | sed -n '1,220p'
printf '\n== vulkandevice.h (selected lines) ==\n'
cat -n src/runtime/renderer/vulkandevice.h | sed -n '1,220p'
printf '\n== vulkandevice.cpp references ==\n'
rg -n "createBuffer|VulkanBuffer" src/runtime/renderer/vulkandevice.cpp src/runtime/renderer/vulkanbuffer.cpp src/runtime/renderer/vulkanbuffer.h src/runtime/renderer/vulkandevice.h
printf '\n== export macro usage ==\n'
rg -n "SEGFAULT_EXPORT" src/runtime/renderer | sed -n '1,200p'Repository: kimkulling/Segfault
Length of output: 11862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/runtime/renderer/vulkanbuffer.h
printf '\n---\n'
sed -n '1,220p' src/runtime/renderer/vulkandevice.h
printf '\n---\n'
sed -n '1,220p' src/runtime/renderer/vulkandevice.cpp
printf '\n---\n'
sed -n '1,220p' src/runtime/renderer/vulkanbuffer.cppRepository: kimkulling/Segfault
Length of output: 16719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== core/segfault.h ==\n'
sed -n '1,220p' src/core/segfault.h
printf '\n== build files mentioning renderer/core library targets ==\n'
rg -n "add_library|SHARED|STATIC|SEGFAULT_EXPORT|renderer|core" CMakeLists.txt cmake src -g 'CMakeLists.txt' -g '*.cmake' -g 'meson.build' -g 'Makefile' | sed -n '1,240p'
printf '\n== VulkanBuffer usages outside its own files ==\n'
rg -n "\bVulkanBuffer\b" src | sed -n '1,240p'Repository: kimkulling/Segfault
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate segfault.h ==\n'
fd -a 'segfault.h' .
printf '\n== locate build files ==\n'
fd -a 'CMakeLists.txt|*.cmake|meson.build|Makefile' .
printf '\n== search for SEGFAULT_EXPORT definition ==\n'
rg -n "define\s+SEGFAULT_EXPORT|SEGFAULT_EXPORT" . -g '!**/build/**' -g '!**/dist/**' | sed -n '1,240p'Repository: kimkulling/Segfault
Length of output: 593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/runtime/core/segfault.h ==\n'
sed -n '1,220p' src/runtime/core/segfault.h
printf '\n== build files ==\n'
fd -a --glob 'CMakeLists.txt' --glob '*.cmake' --glob 'meson.build' --glob 'Makefile' .
printf '\n== SEGFAULT_EXPORT definition/search ==\n'
rg -n "SEGFAULT_EXPORT|define[[:space:]]+SEGFAULT_EXPORT" src CMakeLists.txt cmake -g '!**/build/**' -g '!**/dist/**' | sed -n '1,240p'
printf '\n== library target declarations ==\n'
rg -n "add_library|set_target_properties|target_compile_definitions|target_include_directories|SHARED|STATIC" CMakeLists.txt src cmake -g 'CMakeLists.txt' -g '*.cmake' | sed -n '1,260p'Repository: kimkulling/Segfault
Length of output: 5689
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== top-level CMakeLists.txt ==\n'
sed -n '1,260p' CMakeLists.txt
printf '\n== src/runtime/CMakeLists.txt ==\n'
sed -n '1,260p' src/runtime/CMakeLists.txt
printf '\n== visibility / shared-library settings ==\n'
rg -n "CMAKE_CXX_VISIBILITY_PRESET|VISIBILITY_INLINES_HIDDEN|add_library|SHARED|STATIC|POSITION_INDEPENDENT_CODE|SEGFAULT_BUILD_EXPORT|target_compile_definitions|target_compile_options" CMakeLists.txt src/runtime/CMakeLists.txt src -g 'CMakeLists.txt' | sed -n '1,260p'Repository: kimkulling/Segfault
Length of output: 3317
Export VulkanBuffer from the shared runtime library. segfault_runtime is built as SHARED, and VulkanDevice::createBuffer() exposes VulkanBuffer* in its public API. Without SEGFAULT_EXPORT on VulkanBuffer, external consumers can hit unresolved symbol/visibility failures on the DLL boundary.
🛠️ Proposed fix
- class VulkanBuffer {
+ class SEGFAULT_EXPORT VulkanBuffer {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class VulkanBuffer { | |
| class SEGFAULT_EXPORT VulkanBuffer { |
🤖 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/vulkanbuffer.h` at line 38, Add SEGFAULT_EXPORT to the
VulkanBuffer class declaration so it is exported from the shared runtime library
and remains usable through VulkanDevice::createBuffer() for external consumers.
Source: Coding guidelines
| /// @brief Constructs a VulkanDevice. | ||
| /// @param instance The Vulkan instance. | ||
| VulkanDevice(VkPhysicalDevice physicalDevice); | ||
| explicit VulkanDevice(VkPhysicalDevice physicalDevice); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale @param doc on the constructor.
@param instance The Vulkan instance. doesn't match the actual parameter physicalDevice (VkPhysicalDevice). Likely leftover from an earlier signature.
As per coding guidelines, "Use Doxygen-style comments in header files for public APIs (@brief, @param, @return, etc.)."
📝 Proposed fix
/// `@brief` Constructs a VulkanDevice.
- /// `@param` instance The Vulkan instance.
+ /// `@param` physicalDevice The Vulkan physical device to wrap.
explicit VulkanDevice(VkPhysicalDevice physicalDevice);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @brief Constructs a VulkanDevice. | |
| /// @param instance The Vulkan instance. | |
| VulkanDevice(VkPhysicalDevice physicalDevice); | |
| explicit VulkanDevice(VkPhysicalDevice physicalDevice); | |
| /// `@brief` Constructs a VulkanDevice. | |
| /// `@param` physicalDevice The Vulkan physical device to wrap. | |
| explicit VulkanDevice(VkPhysicalDevice physicalDevice); |
🤖 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 67 - 69, Update the Doxygen
comment for the VulkanDevice constructor to document the actual physicalDevice
parameter, replacing the stale instance description while preserving the
existing `@brief` and public API documentation style.
Source: Coding guidelines
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation