Update from task f96c1ed4-2a53-4444-90e8-9b57f386bac1#25
Open
tronpis wants to merge 3 commits into
Open
Conversation
- Fix CMakeLists.txt to use correct paths for new RenderGraph source files - Remove invalid VkSubpassDependency logic from RenderGraph compilation - Add validation for null imageView handles in RenderGraph framebuffers - Correct render area extent and clear value handling in RenderGraph execution - Initialize CompiledPass state correctly during RenderGraph compilation - Include glm/gtx/norm.hpp header for glm::length2 function - Fix chunk key generation logic to handle negative coordinates correctly - Optimize chunk sorting using squared distance to avoid sqrt operations
Review Summary by QodoFix RenderGraph bugs and ChunkSystem compilation issues
WalkthroughsDescription• Fix RenderGraph compilation state initialization and framebuffer validation • Remove invalid VkSubpassDependency logic for separate render passes • Correct render area extent and clear value handling in execution • Fix CMakeLists.txt paths and add missing glm header for chunk sorting • Optimize chunk distance calculations using squared distance Diagramflowchart LR
A["RenderGraph Compilation"] -->|Clear previous state| B["Initialize CompiledPass"]
B -->|Remove invalid logic| C["Remove VkSubpassDependency"]
D["Framebuffer Creation"] -->|Validate imageView| E["Add null checks"]
E -->|Store extent| F["Save for execution"]
F -->|Use stored extent| G["Execute render pass"]
H["CMakeLists.txt"] -->|Fix paths| I["Update to src/rendering/graph"]
J["ChunkSystem"] -->|Add header| K["Include glm/gtx/norm.hpp"]
K -->|Optimize sorting| L["Use squared distance"]
File Changes1. FarmEngine/src/rendering/graph/RenderGraph.cpp
|
Code Review by Qodo
1. Stale resource index map
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR introduces render graph system enhancements and updates file paths in the build system. I've identified critical issues that must be addressed before merge.
Critical Issues Found:
- RenderGraph.cpp (Lines 137-147): Logic error in state cleanup where individual CompiledPass fields are cleared in a loop, then the vector is resized. This is inefficient and potentially incorrect if the vector size changes. The loop may iterate over passes that will be destroyed or miss newly created ones.
Changes Summary:
- .gitignore: Cleanup of ignored file patterns
- CMakeLists.txt: Updated paths from
rendering/graph/tosrc/rendering/graph/ - RenderGraph.cpp: Added state cleanup before compilation, improved error handling with null checks, fixed render area extent calculation
- RenderGraph.h: Added
extentfield toCompiledPassstruct - ChunkSystem.cpp: Added missing
glm/gtx/norm.hppinclude
Please address the critical logic error 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 updates project configuration and enhances the RenderGraph system. Key changes include adjusting source paths in CMakeLists.txt, refining .gitignore patterns, and improving RenderGraph by adding robust error checking for attachments, better extent management, and corrected clear value handling for multiple attachments. Feedback highlights a potential resource leak in RenderGraph.cpp where Vulkan handles are reset without being destroyed, a regression in .gitignore due to the removal of standard environment and IDE ignores, and a logic flaw in the extent calculation that may fail for depth-only passes.
Comment on lines
138
to
147
| for (auto& compiled : compiledPasses) { | ||
| compiled.attachments.clear(); | ||
| compiled.colorRefs.clear(); | ||
| compiled.dependencies.clear(); | ||
| compiled.depthRef = {}; | ||
| compiled.vkRenderPass = VK_NULL_HANDLE; | ||
| compiled.framebuffer = VK_NULL_HANDLE; | ||
| } | ||
|
|
||
| compiledPasses.resize(builder.passes.size()); |
Contributor
There was a problem hiding this comment.
This loop resets Vulkan handles (vkRenderPass, framebuffer) to VK_NULL_HANDLE without destroying the actual Vulkan objects. This causes a resource leak every time the graph is re-compiled. Additionally, manually clearing members before a resize is less idiomatic than simply clearing the vector.
Note: Ensure that cleanup() is called before re-compiling to properly destroy existing Vulkan resources.
compiledPasses.clear();
compiledPasses.resize(builder.passes.size());
Comment on lines
+26
to
28
| # OS generated files | ||
| .DS_Store | ||
| Thumbs.db |
Contributor
There was a problem hiding this comment.
Removing standard ignore patterns for IDEs (.vscode/, .idea/), environment files (.env*), and coverage reports is a regression in project hygiene. These files are local to a developer's environment and should not be tracked in the repository to avoid accidental commits of sensitive or environment-specific data.
# Temporary files
*.tmp
*.temp
# Environment
.env
.env.local
.env.*
# Editors
.vscode/
.idea/
*.swp
*.swo
# Coverage
coverage/
htmlcov/
.coverage
# OS generated files
.DS_Store
Thumbs.db
Comment on lines
+261
to
263
| if (!res->isSwapchain) { | ||
| extent = res->extent; | ||
| } |
Contributor
There was a problem hiding this comment.
The extent calculation only considers color attachments and defaults to swapchainExtent. If a pass only has a depth attachment, or if the depth attachment has different dimensions than the color attachments, the resulting framebuffer extent might be incorrect. It is safer to determine the extent by validating all participating attachments.
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
+137
to
149
| // Limpiar estado previo antes de compilar | ||
| for (auto& compiled : compiledPasses) { | ||
| compiled.attachments.clear(); | ||
| compiled.colorRefs.clear(); | ||
| compiled.dependencies.clear(); | ||
| compiled.depthRef = {}; | ||
| compiled.vkRenderPass = VK_NULL_HANDLE; | ||
| compiled.framebuffer = VK_NULL_HANDLE; | ||
| } | ||
|
|
||
| compiledPasses.resize(builder.passes.size()); | ||
|
|
||
| // Construir mapa nombre->índice |
There was a problem hiding this comment.
1. Stale resource index map 🐞 Bug ≡ Correctness
RenderGraph::compile() rebuilds compiledRegistry.nameToIndex without clearing previous entries, so recompiling with a different resource set can make getResource() return a pointer to the wrong resource (or out-of-bounds). This can also defeat the intended "not found" checks during compilation by returning a stale hit.
Agent Prompt
### Issue description
`RenderGraph::compile()` rebuilds `compiledRegistry.nameToIndex` without clearing it first. After recompiling with a different resource set, stale keys can remain and `ResourceRegistry::getResource()` will index `resources[it->second]` with a stale index, returning the wrong resource or going out-of-bounds.
### Issue Context
This is especially risky now that `compile()` explicitly tries to "clean previous state" for recompilation.
### Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[131-152]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[11-17]
### Suggested fix
- Add `compiledRegistry.nameToIndex.clear();` (and consider clearing other compiled-registry state that must not persist) before rebuilding the map.
- Optionally add a defensive bounds check in `getResource()` to ensure `it->second < resources.size()` and return `nullptr` otherwise.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
322
to
326
| 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}; | ||
| rpBegin.renderArea.extent = compiled.extent; | ||
|
|
There was a problem hiding this comment.
2. Zero render area extent 🐞 Bug ≡ Correctness
RenderGraph::execute() uses compiled.extent for rpBegin.renderArea.extent, but CompiledPass::extent
defaults to {0,0} and is only set in createFramebuffers(). Since createFramebuffers() is private and
not called anywhere in this module, execute() can begin render passes with a 0x0 render area.
Agent Prompt
### Issue description
`RenderGraph::execute()` uses `compiled.extent`, but `CompiledPass::extent` defaults to `{0,0}` and is only assigned in `createFramebuffers()`. In the current code, `createFramebuffers()` is private and there are no call sites, so `execute()` can run with a 0x0 render area.
### Issue Context
This change was introduced by switching from a computed extent to `compiled.extent`.
### Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[311-326]
- FarmEngine/src/rendering/graph/RenderGraph.h[143-167]
### Suggested fix options
Pick one consistent API direction:
1. **Expose an initialization/build step** (public) that must be called before `execute()` and that calls `createRenderPasses()` + `createFramebuffers()` and sets extents.
2. **Pass the render extent into `execute()`** (or store a default graph extent) and use it when `compiled.extent` is `{0,0}`.
3. **Compute extent from attachments/registry at execute-time** (less ideal, but avoids uninitialized state).
ⓘ 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 f96c1ed4-2a53-4444-90e8-9b57f386bac1.