Feature/add depth texture#10
Merged
Merged
Conversation
WalkthroughThis PR upgrades the Vulkan renderer with 3D vertex position support and depth rendering. Vertex input moves from 2D to 3D; the renderer gains depth image resources, depth format selection, and depth attachment integration into render passes and pipelines. Initialization is wired to create depth resources and command buffers. RHI initialization errors are now propagated to the application. Documentation and code cleanup round out the changes. ChangesDepth Rendering and 3D Vertex Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/renderer/RHIVulkan.cpp (1)
1423-1450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the unconditional pre-initialized vkCmdPipelineBarrier in transitionImageLayout()
RHIImpl::transitionImageLayout()records a firstvkCmdPipelineBarrier()before the depth/stencilaspectMask, the layout-specificsrcAccessMask/dstAccessMask, and the computedsourceStage/destinationStageare set—using0 /* TODO */stage masks and the initialVK_IMAGE_ASPECT_COLOR_BITaspectMask. ForVK_IMAGE_LAYOUT_UNDEFINED -> VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL, this causes an extra (and potentially invalid) barrier before the correct second depth barrier is submitted.🤖 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 1423 - 1450, The initial unconditional vkCmdPipelineBarrier call in RHIImpl::transitionImageLayout() is executed before aspectMask, src/dst access masks and source/destination stages are computed, causing an extra/invalid barrier (notably for VK_IMAGE_LAYOUT_UNDEFINED -> VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL); remove that early vkCmdPipelineBarrier and only call vkCmdPipelineBarrier after you have set barrier.subresourceRange.aspectMask, barrier.srcAccessMask, barrier.dstAccessMask and computed sourceStage/destinationStage (i.e., move or defer the pipeline barrier invocation to after the layout-specific if/else blocks inside transitionImageLayout()) so only the correctly-configured barrier is recorded.
🤖 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/application/app.cpp`:
- Around line 107-113: When RHI::init(appName, mSdlWindow) returns false you
must roll back the partial initialization: delete the allocated mRHI, set mRHI
to nullptr and restore mState out of ModuleState::Init (e.g.
ModuleState::Stopped or ModuleState::Uninitialized) so retries can proceed and
no leak remains; perform this cleanup inside the failure branch that currently
only logs the error before returning ret.
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 206-227: Change findSupportedFormat to a non-throwing function
that returns bool and writes the chosen VkFormat via an out-parameter (e.g.,
bool findSupportedFormat(RHIImpl* rhi, const std::vector<VkFormat>& candidates,
VkImageTiling tiling, VkFormatFeatureFlags features, VkFormat& outFormat));
replace the throw with returning false when no candidate matches. Likewise
change findDepthFormat to return bool and call the new findSupportedFormat,
propagating false on failure and only returning true with the depth format set
on success. Update any callers of findDepthFormat/findSupportedFormat to handle
the bool failure result accordingly.
- Around line 1068-1070: In cleanupSwapChain, the code currently destroys
depthImageView/depthImage/depthImageMemory before destroying
swapChainFramebuffers, which is invalid because createFramebuffers uses
depthImageView as an attachment; change cleanupSwapChain so it first iterates
over and destroys all VkFramebuffer objects in swapChainFramebuffers
(vkDestroyFramebuffer) and clears that container, and only after that call
vkDestroyImageView(device, depthImageView, nullptr), vkDestroyImage(device,
depthImage, nullptr) and vkFreeMemory(device, depthImageMemory, nullptr); ensure
references to swapChainFramebuffers, cleanupSwapChain, createFramebuffers,
depthImageView, depthImage, and depthImageMemory are correctly updated.
---
Outside diff comments:
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 1423-1450: The initial unconditional vkCmdPipelineBarrier call in
RHIImpl::transitionImageLayout() is executed before aspectMask, src/dst access
masks and source/destination stages are computed, causing an extra/invalid
barrier (notably for VK_IMAGE_LAYOUT_UNDEFINED ->
VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL); remove that early
vkCmdPipelineBarrier and only call vkCmdPipelineBarrier after you have set
barrier.subresourceRange.aspectMask, barrier.srcAccessMask,
barrier.dstAccessMask and computed sourceStage/destinationStage (i.e., move or
defer the pipeline barrier invocation to after the layout-specific if/else
blocks inside transitionImageLayout()) so only the correctly-configured barrier
is recorded.
🪄 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: b320271a-821c-42f5-b961-f487c8671b79
📒 Files selected for processing (6)
assets/shaders/default.vertsrc/runtime/application/app.cppsrc/runtime/application/app.hsrc/runtime/renderer/RHI.hsrc/runtime/renderer/RHIVulkan.cppsrc/runtime/renderer/rendercore.h
Comment on lines
+1068
to
+1070
| vkDestroyImageView(device, depthImageView, nullptr); | ||
| vkDestroyImage(device, depthImage, nullptr); | ||
| vkFreeMemory(device, depthImageMemory, nullptr); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '841,863p;1067,1080p' src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 1709
Vulkan lifetime ordering in cleanupSwapChain
createFramebuffers() builds each VkFramebuffer with depthImageView as an attachment, but cleanupSwapChain() destroys depthImageView/depthImage/depthImageMemory before destroying the swapChainFramebuffers. This breaks Vulkan object-lifetime ordering—destroy the framebuffers first, then tear down the shared depth resources.
🤖 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 1068 - 1070, In
cleanupSwapChain, the code currently destroys
depthImageView/depthImage/depthImageMemory before destroying
swapChainFramebuffers, which is invalid because createFramebuffers uses
depthImageView as an attachment; change cleanupSwapChain so it first iterates
over and destroys all VkFramebuffer objects in swapChainFramebuffers
(vkDestroyFramebuffer) and clears that container, and only after that call
vkDestroyImageView(device, depthImageView, nullptr), vkDestroyImage(device,
depthImage, nullptr) and vkFreeMemory(device, depthImageMemory, nullptr); ensure
references to swapChainFramebuffers, cleanupSwapChain, createFramebuffers,
depthImageView, depthImage, and depthImageMemory are correctly updated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation