Update from task de872a38-c33d-412a-94b3-c7df7ffebd4e#24
Open
tronpis wants to merge 3 commits into
Open
Conversation
…tion Key features implemented: - New RenderGraph.h/cpp: Core render graph implementation with resource management, pass compilation, and execution - New RenderPipeline.h/cpp: Forward and deferred renderer examples with GBuffer setup and lighting passes - New ExampleUsage.h: Multiple render graph examples including chunk rendering, shadow mapping, and SSAO - New README.md: Comprehensive documentation covering render graph architecture, advantages, and usage - Updated CMakeLists.txt: Added new render graph source files to FarmEngineRenderer library - Modified ChunkSystem.h/cpp: Updated to 64 height and fixed mesh building vertex indexing - Updated .gitignore: Refined ignore patterns for build artifacts and temporary files The render graph system provides automatic synchronization, TBDR optimization, and memory aliasing as requested. The implementation includes comprehensive examples showing integration with the chunk system and advanced rendering techniques.
Review Summary by QodoImplement Render Graph system with ChunkSystem fixes and performance improvements
WalkthroughsDescription• Implement comprehensive Render Graph system for Vulkan rendering - Core RenderGraph with resource management and pass compilation - Forward and Deferred renderer implementations with GBuffer setup - Automatic synchronization and TBDR optimization support • Add multiple render graph usage examples for chunk rendering, shadows, SSAO • Fix ChunkSystem mesh building vertex indexing calculation errors - Correct vertex stride from 3 to 6 components per vertex - Update chunk height to configurable parameter - Optimize distance calculations using squared length • Improve ChunkManager threading with reduced lock contention - Refactor chunk generation to minimize mutex hold time - Fix chunk coordinate key packing for proper uint64 handling Diagramflowchart LR
RG["RenderGraph<br/>Core System"]
RGB["RenderGraphBuilder<br/>Configuration"]
RP["RenderPass<br/>Definitions"]
RR["ResourceRegistry<br/>Management"]
FWD["ForwardRenderer<br/>Example"]
DEF["DeferredRenderer<br/>Example"]
CS["ChunkSystem<br/>Updates"]
CM["ChunkManager<br/>Optimization"]
RGB -->|builds| RG
RP -->|configures| RGB
RR -->|manages| RG
FWD -->|uses| RG
DEF -->|uses| RG
CS -->|fixed indexing| CM
CM -->|improved threading| RG
File Changes1. FarmEngine/src/rendering/graph/RenderGraph.h
|
Code Review by Qodo
1. Wrong CMake paths
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Summary
This PR adds a comprehensive render graph system and enhances the chunk system with improved performance optimizations. However, there are critical defects that must be fixed before merge.
Critical Issues (Must Fix)
Chunk Key Generation Bug: The chunk coordinate hashing in ChunkManager::getChunk() and ChunkManager::processPendingGenerations() has a logic error that causes hash collisions with negative chunk coordinates. When negative int32_t values are cast to uint32_t, they become large positive values (e.g., -1 becomes 0xFFFFFFFF), producing incorrect hash keys. This will cause chunks at negative world positions to collide, overwrite each other, or become inaccessible, breaking world generation for any player movement into negative coordinate spaces.
Changes Overview
Positive Changes:
- Added modern render graph system with proper resource management and synchronization
- Fixed hardcoded chunk height values throughout the codebase, now using configurable
chunkHeightparameter - Corrected vertex buffer stride calculation bug in mesh generation (changed from
/3to/6to match position+normal data) - Improved chunk generation performance by reducing lock contention (generation now happens outside mutex)
- Optimized distance calculations using
glm::length2instead ofglm::lengthto avoid expensive sqrt operations - Added LOD and culling systems for performance optimization
Areas of Concern:
- The chunk key generation must be fixed to handle negative coordinates correctly
- Verify that
TerrainConfigandBiometypes are properly defined in the included headers
Files Changed: 10 files with 1,183 additions and 79 deletions
Please address the critical chunk key generation issue before merging.
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 introduces a Render Graph system to FarmEngine for automated Vulkan synchronization and resource management, alongside significant improvements to the Chunk System. The Chunk System now supports configurable heights, optimized distance calculations using squared lengths, and reduced mutex contention during chunk generation. However, the Render Graph implementation contains several critical Vulkan synchronization and state management issues, including invalid subpass dependencies between separate render passes, incorrect attachment layout transitions, and hardcoded render area extents. Additionally, the .gitignore file requires cleanup of markdown delimiters.
| @@ -1,69 +1,51 @@ | |||
| ``` | |||
Contributor
There was a problem hiding this comment.
The .gitignore file contains markdown code block delimiters (```) at the beginning and end of the file. These should be removed as they are not valid gitignore syntax and were likely included by mistake during code generation.
| desc.stencilLoadOp = VK_ATTACHMENT_LOAD_OP_DONT_CARE; | ||
| desc.stencilStoreOp = VK_ATTACHMENT_STORE_OP_DONT_CARE; | ||
| desc.initialLayout = res->currentLayout; | ||
| desc.finalLayout = res->currentLayout; // Se actualizará con barreras |
Contributor
There was a problem hiding this comment.
Setting desc.finalLayout to res->currentLayout is incorrect. The finalLayout specifies the layout the attachment will transition to automatically at the end of the render pass. For a color attachment, this should typically be VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL or the layout required by the next pass. Using the current layout (which might be UNDEFINED) will lead to incorrect state transitions.
Comment on lines
+206
to
+208
| VkSubpassDependency vkDep{}; | ||
| vkDep.srcSubpass = dep.from; | ||
| vkDep.dstSubpass = dep.to; |
Contributor
There was a problem hiding this comment.
Each CompiledPass is being created as a separate VkRenderPass (line 223), but you are adding VkSubpassDependency objects to them using the graph pass indices as srcSubpass and dstSubpass. In Vulkan, subpass dependencies only synchronize subpasses within the same VkRenderPass. Since your passes are in different VkRenderPass objects, these dependencies are invalid. You should use VK_SUBPASS_EXTERNAL for transitions between different render passes, or use VkMemoryBarrier/VkImageMemoryBarrier between vkCmdEndRenderPass and vkCmdBeginRenderPass calls.
Comment on lines
+314
to
+317
| rpBegin.renderArea.extent = {compiled.attachments.empty() ? 0 : | ||
| registry.resources[0].extent.width, | ||
| compiled.attachments.empty() ? 0 : | ||
| registry.resources[0].extent.height}; |
Contributor
There was a problem hiding this comment.
Accessing registry.resources[0].extent to set the render area for every pass is a major bug. 1) The registry might be empty. 2) The first resource in the registry is not necessarily the one this pass is rendering to. 3) Different passes can have different extents (e.g., shadow maps or half-res buffers). You should use the extent of the framebuffer associated with the current compiled pass.
Comment on lines
+289
to
+290
| VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, | ||
| VK_PIPELINE_STAGE_ALL_COMMANDS_BIT, |
Contributor
There was a problem hiding this comment.
Using VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT and VK_PIPELINE_STAGE_ALL_COMMANDS_BIT for all barriers is extremely inefficient. It essentially flushes the entire GPU pipeline and prevents any overlapping execution of passes. You should use the specific stages where the resources are actually produced and consumed.
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
96
to
104
| add_library(FarmEngineRenderer STATIC | ||
| renderer/Renderer.cpp | ||
| renderer/Renderer.h | ||
| # Render Graph System | ||
| rendering/graph/RenderGraph.h | ||
| rendering/graph/RenderGraph.cpp | ||
| rendering/graph/RenderPipeline.h | ||
| rendering/graph/RenderPipeline.cpp | ||
| ) |
There was a problem hiding this comment.
1. Wrong cmake paths 🐞 Bug ≡ Correctness
FarmEngineRenderer lists new RenderGraph sources under rendering/graph/... but the added files are under src/rendering/graph/..., so the build will fail when CMake can’t find the sources.
Agent Prompt
### Issue description
`FarmEngine/CMakeLists.txt` adds RenderGraph sources using paths that don’t match where the files were added in this PR (`FarmEngine/src/rendering/graph/...`). This breaks the build because CMake won’t find the referenced files.
### Issue Context
The new files are located under `FarmEngine/src/rendering/graph`, but CMake lists them under `rendering/graph`.
### Fix Focus Areas
- FarmEngine/CMakeLists.txt[96-104]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+203
to
+216
| // Añadir dependencias explícitas | ||
| for (const auto& dep : builder.explicitDependencies) { | ||
| if (dep.from < compiledPasses.size() && dep.to < compiledPasses.size()) { | ||
| VkSubpassDependency vkDep{}; | ||
| vkDep.srcSubpass = dep.from; | ||
| vkDep.dstSubpass = dep.to; | ||
| vkDep.srcStageMask = dep.srcStageMask; | ||
| vkDep.dstStageMask = dep.dstStageMask; | ||
| vkDep.srcAccessMask = dep.srcAccessMask; | ||
| vkDep.dstAccessMask = dep.dstAccessMask; | ||
| vkDep.dependencyFlags = VK_DEPENDENCY_BY_REGION_BIT; | ||
|
|
||
| compiledPasses[dep.to].dependencies.push_back(vkDep); | ||
| } |
There was a problem hiding this comment.
2. Invalid subpass indices 🐞 Bug ≡ Correctness
RenderGraph compiles explicit dependencies by setting VkSubpassDependency.srcSubpass/dstSubpass to pass indices, but each created render pass has only one subpass, making dependencies with dstSubpass > 0 invalid and likely causing vkCreateRenderPass to fail when dependencies are used.
Agent Prompt
### Issue description
`RenderGraphBuilder::addDependency()` is translated into `VkSubpassDependency` with `srcSubpass/dstSubpass` set to *pass indices*, but each `CompiledPass` is turned into a separate `VkRenderPass` with exactly one subpass. This makes any dependency where `to != 0` invalid.
### Issue Context
`VkSubpassDependency` subpass indices must refer to subpasses within the *same* `VkRenderPass`. The current architecture (one `VkRenderPass` per pass) cannot express cross-pass dependencies via `VkSubpassDependency`.
### Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[203-216]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[223-239]
### Suggested direction
Either:
- Build a single `VkRenderPass` with multiple subpasses (so subpass indices match passes), **or**
- Keep separate render passes but express dependencies via `vkCmdPipelineBarrier` / `VkImageMemoryBarrier` recorded between passes (and remove/replace the `VkSubpassDependency` logic).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+246
to
+280
| void RenderGraph::createFramebuffers(VkDevice dev, VkExtent2D swapchainExtent) { | ||
| for (auto& compiled : compiledPasses) { | ||
| std::vector<VkImageView> attachments; | ||
| VkExtent2D extent = swapchainExtent; | ||
|
|
||
| // Obtener views de todos los attachments | ||
| for (const auto& colorName : compiled.definition.colorAttachments) { | ||
| const ResourceHandle* res = compiledRegistry.getResource(colorName); | ||
| if (res) { | ||
| attachments.push_back(res->imageView); | ||
| if (!res->isSwapchain) { | ||
| extent = res->extent; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!compiled.definition.depthAttachment.empty()) { | ||
| const ResourceHandle* res = compiledRegistry.getResource(compiled.definition.depthAttachment); | ||
| if (res) { | ||
| attachments.push_back(res->imageView); | ||
| } | ||
| } | ||
|
|
||
| VkFramebufferCreateInfo fbInfo{}; | ||
| fbInfo.sType = VK_STRUCTURE_TYPE_FRAMEBUFFER_CREATE_INFO; | ||
| fbInfo.renderPass = compiled.vkRenderPass; | ||
| fbInfo.attachmentCount = static_cast<uint32_t>(attachments.size()); | ||
| fbInfo.pAttachments = attachments.data(); | ||
| fbInfo.width = extent.width; | ||
| fbInfo.height = extent.height; | ||
| fbInfo.layers = 1; | ||
|
|
||
| if (vkCreateFramebuffer(device, &fbInfo, nullptr, &compiled.framebuffer) != VK_SUCCESS) { | ||
| throw std::runtime_error("Failed to create framebuffer: " + compiled.definition.name); | ||
| } |
There was a problem hiding this comment.
3. Null framebuffer handles 🐞 Bug ≡ Correctness
Framebuffers are built from ResourceHandle.imageView without validation, but typical graph setup (e.g., ForwardRenderer) declares an External color target via addColorTarget which leaves imageView as VK_NULL_HANDLE, causing framebuffer creation to fail.
Agent Prompt
### Issue description
`createFramebuffers()` uses `ResourceHandle.imageView` for all attachments, but graph construction can produce resources with `imageView == VK_NULL_HANDLE` (notably `External` targets created via `addColorTarget`). This will fail framebuffer creation.
### Issue Context
`ForwardRenderer::buildGraph()` uses `addColorTarget(..., ResourceLifetime::External)`, but `addColorTarget()` never sets `image/imageView`. Only `addExternalImage()` populates them.
### Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderPipeline.cpp[14-19]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[50-62]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[246-280]
### Suggested direction
- For `ResourceLifetime::External`, require callers to use `addExternalImage()` (and/or make `addColorTarget` reject `External`).
- Add explicit validation in `createFramebuffers()` to throw with a clear message if any required attachment view is `VK_NULL_HANDLE`.
- Optionally track per-resource `isSwapchain` / external-ness consistently and enforce it at compile-time.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+300
to
+334
| void RenderGraph::execute(VkCommandBuffer cmd, ResourceRegistry& registry, uint32_t frameIndex) { | ||
| if (compiledPasses.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| for (const auto& compiled : compiledPasses) { | ||
| // 1. Record barreras de transición | ||
| recordBarriers(cmd, compiled); | ||
|
|
||
| // 2. Begin render pass | ||
| VkRenderPassBeginInfo rpBegin{}; | ||
| rpBegin.sType = VK_STRUCTURE_TYPE_RENDER_PASS_BEGIN_INFO; | ||
| rpBegin.renderPass = compiled.vkRenderPass; | ||
| rpBegin.framebuffer = compiled.framebuffer; | ||
| rpBegin.renderArea.extent = {compiled.attachments.empty() ? 0 : | ||
| registry.resources[0].extent.width, | ||
| compiled.attachments.empty() ? 0 : | ||
| registry.resources[0].extent.height}; | ||
|
|
||
| // Clear values | ||
| std::vector<VkClearValue> clearValues; | ||
| if (!compiled.definition.colorAttachments.empty()) { | ||
| VkClearValue colorClear{}; | ||
| colorClear.color = compiled.definition.clearColorValue; | ||
| clearValues.push_back(colorClear); | ||
| } | ||
| if (!compiled.definition.depthAttachment.empty()) { | ||
| VkClearValue depthClear{}; | ||
| depthClear.depthStencil = compiled.definition.clearDepthValue; | ||
| clearValues.push_back(depthClear); | ||
| } | ||
|
|
||
| rpBegin.clearValueCount = static_cast<uint32_t>(clearValues.size()); | ||
| rpBegin.pClearValues = clearValues.data(); | ||
|
|
There was a problem hiding this comment.
4. Wrong renderarea clears 🐞 Bug ≡ Correctness
RenderGraph::execute sets renderArea.extent from registry.resources[0] for every pass and pushes only one VkClearValue for all color attachments, which can access out-of-bounds and break render passes with multiple attachments.
Agent Prompt
### Issue description
`RenderGraph::execute()` currently:
- Computes `rpBegin.renderArea.extent` from `registry.resources[0]` (not per-pass/framebuffer)
- Supplies only one color `VkClearValue` even if the render pass has multiple color attachments
This can crash (out-of-bounds `resources[0]`) and violates expected clear-value sizing for render passes with multiple attachments.
### Issue Context
A compiled pass may have multiple color attachments (`compiled.attachments.size()` can exceed 2), but `clearValues` is built as `{ one color, optional depth }`.
### Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[300-334]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[151-174]
### Suggested direction
- Store the framebuffer extent per `CompiledPass` during framebuffer creation and use it for `renderArea`.
- Build `clearValues` sized to `compiled.attachments.size()` and fill each entry in attachment order (color attachments first, then depth), or otherwise ensure `clearValueCount` matches `attachmentCount`.
ⓘ 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 de872a38-c33d-412a-94b3-c7df7ffebd4e.