Skip to content

Update from task a1ce759e-0b74-4564-9c2c-b6370e14c669 #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tronpis wants to merge 1 commit into
base: rendergraph-bugs-15f39
Choose a base branch
from
Open

Update from task a1ce759e-0b74-4564-9c2c-b6370e14c669 #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 18 additions & 17 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
```
# Compiled and binary files
# Compiled and build artifacts
*.o
*.obj
*.exe
Expand All @@ -13,27 +13,32 @@ node_modules/
venv/
.venv/
__pycache__/
dist/
build/
.mypy_cache/
.pytest_cache/
target/
.gradle/

# Editors/IDE
.vscode/
.idea/
# Build directories
build/
dist/

# Logs and temp files
*.log
*.tmp
*.swp
*.swo

# System/Environment
.DS_Store
Thumbs.db
# Environment
.env
.env.local
*.env.*

# Logs and temp files
*.log
*.tmp
# Editors
.vscode/
.idea/

# System files
.DS_Store
Thumbs.db

# Coverage
coverage/
Expand All @@ -59,8 +64,4 @@ htmlcov/
*.Z
*.lz
*.lzo
*.tar.gz
*.tar.bz2
*.tar.xz
*.tar.zst
```
12 changes: 8 additions & 4 deletions FarmEngine/src/rendering/graph/ExampleUsage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace FarmEngine::Render::Examples {
* Este es el caso de uso principal para FarmEngine - renderizar chunks
* voxel con iluminación dinámica y oclusión ambiental.
*/
inline void ChunkRenderingExample(RenderGraph& graph, VkExtent2D swapchainExtent) {
inline void ChunkRenderingExample(RenderGraph& graph, VkDevice device, VkExtent2D swapchainExtent) {
RenderGraphBuilder builder;

// 1. Recursos principales
Expand Down Expand Up @@ -64,14 +64,15 @@ inline void ChunkRenderingExample(RenderGraph& graph, VkExtent2D swapchainExtent
});

graph.compile(std::move(builder));
graph.build(device, swapchainExtent);
}

/**
* @brief Ejemplo: Shadow mapping para iluminación dinámica
*
* Muestra cómo añadir un pass de shadow map antes del geometry pass
*/
inline void ShadowMappingExample(RenderGraph& graph, VkExtent2D extent) {
inline void ShadowMappingExample(RenderGraph& graph, VkDevice device, VkExtent2D extent) {
RenderGraphBuilder builder;

// Shadow map resource (alta resolución para calidad)
Expand Down Expand Up @@ -111,14 +112,15 @@ inline void ShadowMappingExample(RenderGraph& graph, VkExtent2D extent) {
);

graph.compile(std::move(builder));
graph.build(device, extent);
}

/**
* @brief Ejemplo: SSAO (Screen Space Ambient Occlusion)
*
* Técnica post-process para oclusión ambiental en tiempo real
*/
inline void SSAOExample(RenderGraph& graph, VkExtent2D extent) {
inline void SSAOExample(RenderGraph& graph, VkDevice device, VkExtent2D extent) {
RenderGraphBuilder builder;

// GBuffer básico
Expand Down Expand Up @@ -159,14 +161,15 @@ inline void SSAOExample(RenderGraph& graph, VkExtent2D extent) {
});

graph.compile(std::move(builder));
graph.build(device, extent);
}

/**
* @brief Ejemplo: Minimizado para testing
*
* Pipeline más simple posible para validar integración
*/
inline void MinimalExample(RenderGraph& graph, VkExtent2D extent) {
inline void MinimalExample(RenderGraph& graph, VkDevice device, VkExtent2D extent) {
RenderGraphBuilder builder;

builder.addColorTarget("Backbuffer", VK_FORMAT_B8G8R8A8_SRGB, extent, ResourceLifetime::External);
Expand All @@ -184,6 +187,7 @@ inline void MinimalExample(RenderGraph& graph, VkExtent2D extent) {
});

graph.compile(std::move(builder));
graph.build(device, extent);
}

} // namespace FarmEngine::Render::Examples
102 changes: 77 additions & 25 deletions FarmEngine/src/rendering/graph/RenderGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ void ResourceRegistry::updateResourceState(const std::string& name,
VkPipelineStageFlags newStage) {
auto it = nameToIndex.find(name);
if (it != nameToIndex.end()) {
// Defensive bounds check to prevent out-of-bounds access with stale indices
if (it->second >= resources.size()) {
return;
}
resources[it->second].currentLayout = newLayout;
resources[it->second].currentAccess = newAccess;
resources[it->second].currentStage = newStage;
Expand Down Expand Up @@ -124,6 +128,30 @@ RenderGraphBuilder& RenderGraphBuilder::addDependency(uint32_t from, uint32_t to
return *this;
}

RenderGraphBuilder& RenderGraphBuilder::addResourceDependency(uint32_t from, uint32_t to,
const std::string& resourceName,
VkPipelineStageFlags srcStage,
VkPipelineStageFlags dstStage,
VkAccessFlags srcAccess,
VkAccessFlags dstAccess,
VkImageLayout oldLayout,
VkImageLayout newLayout,
VkImageAspectFlags aspectMask) {
PassDependency dep;
dep.from = from;
dep.to = to;
dep.srcStageMask = srcStage;
dep.dstStageMask = dstStage;
dep.srcAccessMask = srcAccess;
dep.dstAccessMask = dstAccess;
dep.resourceName = resourceName;
dep.oldLayout = oldLayout;
dep.newLayout = newLayout;
dep.aspectMask = aspectMask;
explicitDependencies.push_back(dep);
return *this;
}

// ============================================================================
// RenderGraph Implementation
// ============================================================================
Expand Down Expand Up @@ -206,19 +234,50 @@ void RenderGraph::compile(RenderGraphBuilder&& builder) {

// Procesar dependencias explícitas y convertirlas en barreras por-pass
for (const auto& dep : builder.explicitDependencies) {
if (dep.to < compiledPasses.size()) {
// Validate dependency indices
if (dep.from >= compiledPasses.size()) {
throw std::runtime_error("Invalid dependency: 'from' index " + std::to_string(dep.from) +
" is out of range (pass count: " + std::to_string(compiledPasses.size()) + ")");
}
if (dep.to >= compiledPasses.size()) {
throw std::runtime_error("Invalid dependency: 'to' index " + std::to_string(dep.to) +
" is out of range (pass count: " + std::to_string(compiledPasses.size()) + ")");
}

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;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

barrier.subresourceRange = {
dep.aspectMask,
0, res->mipLevels,
0, res->arrayLayers
};

compiledPasses[dep.to].prePassBarriers.push_back(barrier);
Comment on lines +247 to 269
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Comment on lines +247 to 269
Copy link
Copy Markdown

@qodo-code-review qodo-code-review Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

} 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
});
Comment on lines +272 to +280
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Comment on lines +270 to +280
Copy link
Copy Markdown

@qodo-code-review qodo-code-review Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

}
}
}
Expand Down Expand Up @@ -306,11 +365,11 @@ void RenderGraph::createFramebuffers(VkDevice dev, VkExtent2D swapchainExtent) {
}

void RenderGraph::recordBarriers(VkCommandBuffer cmd, const CompiledPass& pass) {
// Ejecutar barreras pre-pass si existen
// Ejecutar barreras de imagen pre-pass si existen
if (!pass.prePassBarriers.empty()) {
vkCmdPipelineBarrier(
cmd,
VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT,
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT,
0,
0, nullptr,
Expand All @@ -327,6 +386,16 @@ void RenderGraph::execute(VkCommandBuffer cmd, ResourceRegistry& registry, uint3
}

for (const auto& compiled : compiledPasses) {
// Defensive guard: ensure render pass and framebuffer are valid
if (compiled.vkRenderPass == VK_NULL_HANDLE) {
throw std::runtime_error("Execute failed: vkRenderPass is VK_NULL_HANDLE for pass '" +
compiled.definition.name + "'. Call build() before execute().");
}
if (compiled.framebuffer == VK_NULL_HANDLE) {
throw std::runtime_error("Execute failed: framebuffer is VK_NULL_HANDLE for pass '" +
compiled.definition.name + "'. Call build() before execute().");
}

// 1. Record barreras de transición
recordBarriers(cmd, compiled);

Expand All @@ -336,24 +405,7 @@ void RenderGraph::execute(VkCommandBuffer cmd, ResourceRegistry& registry, uint3
rpBegin.renderPass = compiled.vkRenderPass;
rpBegin.framebuffer = compiled.framebuffer;
// Usar el extent del CompiledPass (calculado en createFramebuffers)
// Si es {0,0}, calcularlo dinámicamente desde los attachments como fallback
if (compiled.extent.width == 0 || compiled.extent.height == 0) {
VkExtent2D fallbackExtent = {800, 600}; // Default fallback
if (!compiled.definition.colorAttachments.empty()) {
const ResourceHandle* res = compiledRegistry.getResource(compiled.definition.colorAttachments[0]);
if (res && !res->isSwapchain) {
fallbackExtent = res->extent;
}
} else if (!compiled.definition.depthAttachment.empty()) {
const ResourceHandle* res = compiledRegistry.getResource(compiled.definition.depthAttachment);
if (res) {
fallbackExtent = res->extent;
}
}
rpBegin.renderArea.extent = fallbackExtent;
} else {
rpBegin.renderArea.extent = compiled.extent;
}
rpBegin.renderArea.extent = compiled.extent;

// Clear values - uno por cada attachment (color attachments primero, luego depth)
std::vector<VkClearValue> clearValues;
Expand Down
16 changes: 16 additions & 0 deletions FarmEngine/src/rendering/graph/RenderGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ struct PassDependency {
VkPipelineStageFlags dstStageMask;
VkAccessFlags srcAccessMask;
VkAccessFlags dstAccessMask;

// Optional: resource-specific synchronization
// If empty, this is a pass-level execution dependency (uses VkMemoryBarrier)
// If set, this is a resource-specific dependency (uses VkImageMemoryBarrier)
std::string resourceName;
VkImageLayout oldLayout = VK_IMAGE_LAYOUT_UNDEFINED;
VkImageLayout newLayout = VK_IMAGE_LAYOUT_UNDEFINED;
VkImageAspectFlags aspectMask = VK_IMAGE_ASPECT_COLOR_BIT;
};

class RenderPass {
Expand Down Expand Up @@ -100,6 +108,14 @@ class RenderGraphBuilder {
RenderGraphBuilder& addDependency(uint32_t from, uint32_t to,
VkPipelineStageFlags srcStage, VkPipelineStageFlags dstStage,
VkAccessFlags srcAccess, VkAccessFlags dstAccess);

// Definir dependencias específicas de recursos (para barreras de imagen)
RenderGraphBuilder& addResourceDependency(uint32_t from, uint32_t to,
const std::string& resourceName,
VkPipelineStageFlags srcStage, VkPipelineStageFlags dstStage,
VkAccessFlags srcAccess, VkAccessFlags dstAccess,
VkImageLayout oldLayout, VkImageLayout newLayout,
VkImageAspectFlags aspectMask = VK_IMAGE_ASPECT_COLOR_BIT);

private:
friend class RenderGraph;
Expand Down
10 changes: 8 additions & 2 deletions FarmEngine/src/rendering/graph/RenderPipeline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace FarmEngine::Render {

ForwardRenderer::ForwardRenderer() : extent{0, 0} {}

void ForwardRenderer::buildGraph(RenderGraph& graph, VkExtent2D swapchainExtent) {
void ForwardRenderer::buildGraph(RenderGraph& graph, VkDevice device, VkExtent2D swapchainExtent) {
extent = swapchainExtent;

RenderGraphBuilder builder;
Expand All @@ -34,6 +34,9 @@ void ForwardRenderer::buildGraph(RenderGraph& graph, VkExtent2D swapchainExtent)

// Compilar grafo
graph.compile(std::move(builder));

// Construir Vulkan objects (render passes y framebuffers)
graph.build(device, swapchainExtent);
}

void ForwardRenderer::setupPasses() {
Expand All @@ -46,7 +49,7 @@ void ForwardRenderer::setupPasses() {

DeferredRenderer::DeferredRenderer() : extent{0, 0} {}

void DeferredRenderer::buildGraph(RenderGraph& graph, VkExtent2D swapchainExtent) {
void DeferredRenderer::buildGraph(RenderGraph& graph, VkDevice device, VkExtent2D swapchainExtent) {
extent = swapchainExtent;

RenderGraphBuilder builder;
Expand All @@ -61,6 +64,9 @@ void DeferredRenderer::buildGraph(RenderGraph& graph, VkExtent2D swapchainExtent
setupPostProcess(builder);

graph.compile(std::move(builder));

// Construir Vulkan objects (render passes y framebuffers)
graph.build(device, swapchainExtent);
}

void DeferredRenderer::setupGBuffer(RenderGraphBuilder& builder) {
Expand Down
4 changes: 2 additions & 2 deletions FarmEngine/src/rendering/graph/RenderPipeline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class ForwardRenderer {
* 4. Post-Processing
* 5. Present
*/
void buildGraph(RenderGraph& graph, VkExtent2D swapchainExtent);
void buildGraph(RenderGraph& graph, VkDevice device, VkExtent2D swapchainExtent);

/**
* @brief Configura los passes específicos del renderer
Expand All @@ -47,7 +47,7 @@ class DeferredRenderer {
public:
DeferredRenderer();

void buildGraph(RenderGraph& graph, VkExtent2D swapchainExtent);
void buildGraph(RenderGraph& graph, VkDevice device, VkExtent2D swapchainExtent);

/**
* Estructura típica de GBuffer:
Expand Down