Update from task a1ce759e-0b74-4564-9c2c-b6370e14c669#27
Open
tronpis wants to merge 1 commit into
Open
Conversation
- Refactored RenderGraph::compile() to validate dependency indices and distinguish between pass-level and resource-specific dependencies, generating appropriate VkMemoryBarrier or VkImageMemoryBarrier objects - Extended PassDependency struct and RenderGraphBuilder API to support resource-specific synchronization with layout transitions and aspect masks - Added defensive guards in RenderGraph::execute() to prevent submission of invalid Vulkan commands when renderPass or framebuffer are null - Updated all example usage files to call the new graph.build() method ensuring proper Vulkan object initialization - Enhanced ResourceRegistry with additional bounds checking to prevent out-of-bounds access with stale indices - Improved error reporting with descriptive messages for invalid dependencies and missing Vulkan objects
Review Summary by QodoFix Vulkan validation errors and enhance RenderGraph dependency system
WalkthroughsDescription• Fix Vulkan validation errors with defensive guards and bounds checking • Distinguish between pass-level and resource-specific dependencies with proper barriers • Add resource-specific synchronization with layout transitions and aspect masks • Require explicit build() call for Vulkan object initialization in all examples • Improve error reporting with descriptive messages for invalid dependencies Diagramflowchart LR
A["PassDependency struct"] -->|"extended with resource fields"| B["Resource-specific dependencies"]
C["RenderGraphBuilder API"] -->|"new addResourceDependency method"| B
B -->|"generates VkImageMemoryBarrier"| D["RenderGraph::compile"]
E["Pass-level dependencies"] -->|"generates VkMemoryBarrier"| D
D -->|"validates indices"| F["Error handling"]
G["ResourceRegistry"] -->|"bounds checking"| H["Prevent out-of-bounds access"]
I["RenderGraph::execute"] -->|"defensive guards"| J["Validate Vulkan objects"]
K["Example usage files"] -->|"call graph.build"| L["Proper initialization"]
File Changes1. FarmEngine/src/rendering/graph/RenderGraph.cpp
|
Code Review by Qodo
1. Wrong subpass dependency indices
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The changes successfully address Vulkan RenderGraph barrier handling and API consistency. All modifications are correctly implemented and improve the codebase:
Key Improvements:
- Added missing
graph.build()calls in example functions and renderer implementations - Implemented resource-specific dependency API with proper VkImageMemoryBarrier support
- Added defensive bounds checking in ResourceRegistry methods
- Enhanced barrier recording with proper pipeline stage synchronization
All changes follow Vulkan best practices and maintain API consistency across the codebase.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the project's .gitignore and significantly enhances the RenderGraph system by introducing resource-specific dependencies and more robust synchronization. Key changes include the implementation of addResourceDependency in the RenderGraphBuilder, the addition of defensive bounds checks in the ResourceRegistry, and the enforcement of a build step across various renderers and examples to ensure Vulkan objects are properly initialized before execution. Review feedback identifies critical issues regarding synchronization efficiency, specifically the lack of fine-grained stage masks in barriers, potential invalid image handles during the compilation phase, and the incorrect application of subpass dependency indices for inter-render-pass synchronization.
Comment on lines
+247
to
269
| if (!dep.resourceName.empty()) { | ||
| // Resource-specific dependency: create VkImageMemoryBarrier | ||
| const ResourceHandle* res = compiledRegistry.getResource(dep.resourceName); | ||
| if (!res) { | ||
| throw std::runtime_error("Resource dependency references unknown resource: " + dep.resourceName); | ||
| } | ||
|
|
||
| VkImageMemoryBarrier barrier{}; | ||
| barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER; | ||
| barrier.srcAccessMask = dep.srcAccessMask; | ||
| barrier.dstAccessMask = dep.dstAccessMask; | ||
| barrier.oldLayout = VK_IMAGE_LAYOUT_UNDEFINED; | ||
| barrier.newLayout = VK_IMAGE_LAYOUT_UNDEFINED; | ||
| barrier.oldLayout = dep.oldLayout; | ||
| barrier.newLayout = dep.newLayout; | ||
| barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | ||
| barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | ||
| barrier.image = VK_NULL_HANDLE; // Se establecerá durante la ejecución | ||
| barrier.subresourceRange = {VK_IMAGE_ASPECT_COLOR_BIT, 0, 1, 0, 1}; | ||
| barrier.image = res->image; | ||
| barrier.subresourceRange = { | ||
| dep.aspectMask, | ||
| 0, res->mipLevels, | ||
| 0, res->arrayLayers | ||
| }; | ||
|
|
||
| compiledPasses[dep.to].prePassBarriers.push_back(barrier); |
Contributor
There was a problem hiding this comment.
When processing a resource-specific dependency, the srcStageMask and dstStageMask from the dep object are not stored. These masks are essential for the vkCmdPipelineBarrier call in recordBarriers. Currently, recordBarriers (lines 372-373) defaults to VK_PIPELINE_STAGE_ALL_COMMANDS_BIT, which is inefficient and causes unnecessary GPU stalls. You should store these stage masks in the CompiledPass structure alongside the barriers to allow for fine-grained synchronization.
| barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | ||
| barrier.image = VK_NULL_HANDLE; // Se establecerá durante la ejecución | ||
| barrier.subresourceRange = {VK_IMAGE_ASPECT_COLOR_BIT, 0, 1, 0, 1}; | ||
| barrier.image = res->image; |
Contributor
There was a problem hiding this comment.
The barrier.image is assigned res->image during the compile phase. However, for internal resources (Frame/Persistent), the image handles are typically not created until the build phase or later. If res->image is VK_NULL_HANDLE during compilation, the recorded barrier will be invalid and won't function during execution. Consider resolving the image handle at execution time or ensuring allocation happens before the graph is compiled.
Comment on lines
+272
to
+280
| compiledPasses[dep.to].dependencies.push_back({ | ||
| dep.from, | ||
| dep.to, | ||
| dep.srcStageMask, | ||
| dep.dstStageMask, | ||
| dep.srcAccessMask, | ||
| dep.dstAccessMask, | ||
| VK_DEPENDENCY_BY_REGION_BIT | ||
| }); |
Contributor
There was a problem hiding this comment.
The VkSubpassDependency is being populated with global pass indices (dep.from, dep.to). However, VkSubpassDependency is designed for synchronization between subpasses within the same VkRenderPass. Since each CompiledPass creates its own separate VkRenderPass with a single subpass (index 0), these indices are invalid and will likely cause Vulkan validation errors. For inter-render-pass synchronization, use vkCmdPipelineBarrier or set the subpass indices to VK_SUBPASS_EXTERNAL.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+270
to
+280
| } else { | ||
| // Pass-level execution dependency: store in CompiledPass for use with VkMemoryBarrier | ||
| compiledPasses[dep.to].dependencies.push_back({ | ||
| dep.from, | ||
| dep.to, | ||
| dep.srcStageMask, | ||
| dep.dstStageMask, | ||
| dep.srcAccessMask, | ||
| dep.dstAccessMask, | ||
| VK_DEPENDENCY_BY_REGION_BIT | ||
| }); |
There was a problem hiding this comment.
1. Wrong subpass dependency indices 🐞 Bug ≡ Correctness
RenderGraph::compile() converts addDependency(from,to,...) into a VkSubpassDependency with srcSubpass=from and dstSubpass=to, but each VkRenderPass created has subpassCount=1 so dstSubpass must be 0 (or VK_SUBPASS_EXTERNAL). Any dependency with to!=0 (e.g., ShadowMappingExample’s 0->1) can make vkCreateRenderPass fail or trigger validation errors.
Agent Prompt
## Issue description
`RenderGraphBuilder::addDependency()` is a pass-to-pass dependency API, but `RenderGraph::compile()` currently encodes it into `VkSubpassDependency` fields (`srcSubpass`, `dstSubpass`) using *pass indices*. Each `VkRenderPass` created by `createRenderPasses()` has exactly one subpass (index 0), so `dstSubpass = to` becomes invalid when `to != 0`.
## Issue Context
This breaks real usage (e.g. `ShadowMappingExample` adds dependency 0->1). These dependencies must be enforced between *separate render passes*, not as subpass dependencies.
## Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[235-282]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[291-314]
- FarmEngine/src/rendering/graph/RenderGraph.h[160-176]
## Suggested fix
1. Change the representation for pass-level dependencies to something that can be emitted via `vkCmdPipelineBarrier` before executing pass `to` (e.g. store a list of `{srcStageMask,dstStageMask, VkMemoryBarrier}` on `CompiledPass`).
2. In `recordBarriers()`, emit those pass-level `VkMemoryBarrier`s using the stored stage masks.
3. Keep `VkSubpassDependency` only for within-renderpass subpass dependencies (if/when you have multiple subpasses).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+247
to
269
| if (!dep.resourceName.empty()) { | ||
| // Resource-specific dependency: create VkImageMemoryBarrier | ||
| const ResourceHandle* res = compiledRegistry.getResource(dep.resourceName); | ||
| if (!res) { | ||
| throw std::runtime_error("Resource dependency references unknown resource: " + dep.resourceName); | ||
| } | ||
|
|
||
| VkImageMemoryBarrier barrier{}; | ||
| barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER; | ||
| barrier.srcAccessMask = dep.srcAccessMask; | ||
| barrier.dstAccessMask = dep.dstAccessMask; | ||
| barrier.oldLayout = VK_IMAGE_LAYOUT_UNDEFINED; | ||
| barrier.newLayout = VK_IMAGE_LAYOUT_UNDEFINED; | ||
| barrier.oldLayout = dep.oldLayout; | ||
| barrier.newLayout = dep.newLayout; | ||
| barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | ||
| barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | ||
| barrier.image = VK_NULL_HANDLE; // Se establecerá durante la ejecución | ||
| barrier.subresourceRange = {VK_IMAGE_ASPECT_COLOR_BIT, 0, 1, 0, 1}; | ||
| barrier.image = res->image; | ||
| barrier.subresourceRange = { | ||
| dep.aspectMask, | ||
| 0, res->mipLevels, | ||
| 0, res->arrayLayers | ||
| }; | ||
|
|
||
| compiledPasses[dep.to].prePassBarriers.push_back(barrier); |
There was a problem hiding this comment.
2. Barriers capture vk_null images 🐞 Bug ≡ Correctness
Resource-specific dependencies bake VkImageMemoryBarrier.image from compiledRegistry at compile time, but addColorTarget/addDepthTarget never populate VkImage so this often becomes VK_NULL_HANDLE and cannot be updated for per-frame external/swapchain images. recordBarriers() then submits vkCmdPipelineBarrier with these baked (potentially null/wrong) images, so synchronization and layout transitions can be invalid or ineffective.
Agent Prompt
## Issue description
Resource barriers are created during `compile()` with `barrier.image = res->image`, but for most resources built via `addColorTarget()` / `addDepthTarget()` the `VkImage` is never set (defaults to `VK_NULL_HANDLE`). Also, external/swapchain images can vary per-frame, so baking `VkImage` into the compiled barrier is not safe.
## Issue Context
`execute()` is passed a runtime `ResourceRegistry& registry`, but `recordBarriers()` currently ignores it and uses pre-baked `VkImageMemoryBarrier`s. This prevents correct per-frame image resolution and can submit invalid barriers.
## Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[235-270]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[367-380]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[383-409]
- FarmEngine/src/rendering/graph/RenderGraph.h[33-51]
## Suggested fix
1. Don’t store `VkImageMemoryBarrier` with a concrete `image` in `CompiledPass`.
2. Store a small "pending barrier" struct that includes `resourceName`, `oldLayout/newLayout`, access masks, aspectMask, and stage masks.
3. In `recordBarriers(cmd, pass, registry)` resolve `VkImage image = registry.getImage(resourceName)` (or other runtime source) and build the `VkImageMemoryBarrier` just-in-time.
4. If `image == VK_NULL_HANDLE`, throw with a clear message (or skip execution) so the failure is explicit.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
This PR was created by qwen-chat coder for task a1ce759e-0b74-4564-9c2c-b6370e14c669.