Skip to content

Update from task e23c998a-e103-4ae6-8a9f-0790479ad7c9 #28

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 2 commits into
base: vulkan-rendergraph-barriers-bug-4c669
Choose a base branch
from
Open

Update from task e23c998a-e103-4ae6-8a9f-0790479ad7c9 #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be3e2f4
Fix Vulkan render graph dependency and barrier issues
qwen-intl Apr 2, 2026
c46c1f7
Update FarmEngine/src/rendering/graph/RenderGraph.cpp
tronpis Apr 2, 2026
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
18 changes: 8 additions & 10 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,23 @@ __pycache__/
target/
.gradle/

# Build directories
build/
dist/

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

# Environment
.env
.env.local
*.env.*

# Editors
.vscode/
.idea/

# System files
.DS_Store
Thumbs.db
.env
.env.local
*.env.*

# Coverage
# Coverage reports
coverage/
htmlcov/
.coverage
Expand All @@ -64,4 +58,8 @@ htmlcov/
*.Z
*.lz
*.lzo
*.tar.gz
*.tar.bz2
*.tar.xz
*.tar.zst
```
132 changes: 94 additions & 38 deletions FarmEngine/src/rendering/graph/RenderGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ void RenderGraph::compile(RenderGraphBuilder&& builder) {
// TODO: Implementar análisis de dependencias del grafo
}

// Procesar dependencias explícitas y convertirlas en barreras por-pass
// Process explicit dependencies and convert them to per-pass barriers
for (const auto& dep : builder.explicitDependencies) {
// Validate dependency indices
if (dep.from >= compiledPasses.size()) {
Expand All @@ -245,39 +245,35 @@ void RenderGraph::compile(RenderGraphBuilder&& builder) {
}

if (!dep.resourceName.empty()) {
// Resource-specific dependency: create VkImageMemoryBarrier
// Resource-specific dependency: store as pending barrier for resolution at execution time
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 = dep.oldLayout;
barrier.newLayout = dep.newLayout;
barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
barrier.image = res->image;
barrier.subresourceRange = {
dep.aspectMask,
0, res->mipLevels,
0, res->arrayLayers
};
PendingResourceBarrier pendingBarrier{};
pendingBarrier.resourceName = dep.resourceName;
pendingBarrier.srcStageMask = dep.srcStageMask;
pendingBarrier.dstStageMask = dep.dstStageMask;
pendingBarrier.srcAccessMask = dep.srcAccessMask;
pendingBarrier.dstAccessMask = dep.dstAccessMask;
pendingBarrier.oldLayout = dep.oldLayout;
pendingBarrier.newLayout = dep.newLayout;
pendingBarrier.aspectMask = dep.aspectMask;

compiledPasses[dep.to].prePassBarriers.push_back(barrier);
compiledPasses[dep.to].pendingResourceBarriers.push_back(pendingBarrier);
} 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
});
// Pass-level execution dependency: store for emission as VkMemoryBarrier at execution time
PassLevelDependency passDep{};
passDep.fromPass = dep.from;
passDep.toPass = dep.to;
passDep.srcStageMask = dep.srcStageMask;
passDep.dstStageMask = dep.dstStageMask;
passDep.srcAccessMask = dep.srcAccessMask;
passDep.dstAccessMask = dep.dstAccessMask;
passDep.dependencyFlags = VK_DEPENDENCY_BY_REGION_BIT;

compiledPasses[dep.to].passLevelDependencies.push_back(passDep);
}
}
}
Expand Down Expand Up @@ -305,8 +301,9 @@ void RenderGraph::createRenderPasses(VkDevice dev) {
subpass.pDepthStencilAttachment = compiled.depthRef.attachment != VK_ATTACHMENT_UNUSED ? &compiled.depthRef : nullptr;

rpInfo.pSubpasses = &subpass;
rpInfo.dependencyCount = static_cast<uint32_t>(compiled.dependencies.size());
rpInfo.pDependencies = compiled.dependencies.data();
// No subpass dependencies - all synchronization is done via vkCmdPipelineBarrier
rpInfo.dependencyCount = 0;
rpInfo.pDependencies = nullptr;

if (vkCreateRenderPass(device, &rpInfo, nullptr, &compiled.vkRenderPass) != VK_SUCCESS) {
throw std::runtime_error("Failed to create render pass: " + compiled.definition.name);
Expand Down Expand Up @@ -364,18 +361,72 @@ void RenderGraph::createFramebuffers(VkDevice dev, VkExtent2D swapchainExtent) {
}
}

void RenderGraph::recordBarriers(VkCommandBuffer cmd, const CompiledPass& pass) {
// Ejecutar barreras de imagen pre-pass si existen
if (!pass.prePassBarriers.empty()) {
void RenderGraph::recordBarriers(VkCommandBuffer cmd, const CompiledPass& pass, const ResourceRegistry& registry) {
// Compute combined stage masks from all barriers for vkCmdPipelineBarrier
VkPipelineStageFlags combinedSrcStage = 0;
VkPipelineStageFlags combinedDstStage = 0;

// Process pass-level dependencies (emit as VkMemoryBarrier)
std::vector<VkMemoryBarrier> memoryBarriers;
for (const auto& dep : pass.passLevelDependencies) {
VkMemoryBarrier memBarrier{};
memBarrier.sType = VK_STRUCTURE_TYPE_MEMORY_BARRIER;
memBarrier.srcAccessMask = dep.srcAccessMask;
memBarrier.dstAccessMask = dep.dstAccessMask;
memoryBarriers.push_back(memBarrier);

combinedSrcStage |= dep.srcStageMask;
combinedDstStage |= dep.dstStageMask;
}
Comment on lines +369 to +380
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. Backward deps unsynchronized 🐞 Bug ≡ Correctness

RenderGraph::compile stores pass-level dependencies using fromPass/toPass, but
RenderGraph::recordBarriers never uses fromPass and compile never validates from<to, so a dependency
where from>=to emits a barrier before the consumer pass even though the producer runs later. This
silently breaks the intended ordering/visibility guarantees and can cause rendering hazards/data
races.
Agent Prompt 
### Issue description
`PassLevelDependency` stores `fromPass`/`toPass`, but execution emits barriers purely based on the *current pass* (the `toPass`) and never validates that `fromPass` actually executes earlier. If a caller adds a dependency with `fromPass >= toPass`, the barrier is recorded before the destination pass while the producer pass runs later, so the intended synchronization never happens.

### Issue Context
- Dependencies are appended into `compiledPasses[dep.to].passLevelDependencies`.
- `execute()` iterates passes sequentially in `compiledPasses` order.
- `recordBarriers()` ignores `dep.fromPass`, so the dependency direction is effectively unchecked.

### Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[235-277]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[364-381]

### Suggested fix
Choose one:
1) **Enforce builder order**: in `compile()`, reject invalid direction:
   - if `dep.from >= dep.to`, throw a `std::runtime_error` explaining that passes must be added in dependency order (or that topological sorting is not implemented).

2) **Implement ordering**: perform a topological sort of passes based on explicit dependencies (and later inferred deps), rebuild `compiledPasses` order accordingly, and update stored indices. Then barriers recorded before each pass will correspond to producers that actually ran earlier.

3) (If keeping indices stable) **Attach deps to 'to' but validate**: still validate `from < to` and document it clearly in `RenderGraphBuilder::addDependency`/`addResourceDependency` docs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


// Process resource-specific barriers (resolve image at runtime)
std::vector<VkImageMemoryBarrier> imageBarriers;
for (const auto& pending : pass.pendingResourceBarriers) {
VkImage image = registry.getImage(pending.resourceName);
const ResourceHandle* res = registry.getResource(pending.resourceName);
if (!res) {
throw std::runtime_error("Resource barrier references unknown resource: " + pending.resourceName);
}
if (image == VK_NULL_HANDLE) {
throw std::runtime_error("Resource barrier references resource with VK_NULL_HANDLE: " + pending.resourceName);
}

VkImageMemoryBarrier barrier{};
barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER;
barrier.srcAccessMask = pending.srcAccessMask;
barrier.dstAccessMask = pending.dstAccessMask;
barrier.oldLayout = pending.oldLayout;
barrier.newLayout = pending.newLayout;
barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
barrier.image = image;
barrier.subresourceRange = {
pending.aspectMask,
0, res->mipLevels,
0, res->arrayLayers
};
imageBarriers.push_back(barrier);
Comment thread
tronpis marked this conversation as resolved.

combinedSrcStage |= pending.srcStageMask;
combinedDstStage |= pending.dstStageMask;
}

// Emit barriers if any exist
if (!memoryBarriers.empty() || !imageBarriers.empty()) {
// Use combined stage masks, or fallback to ALL_COMMANDS if no stages were specified
VkPipelineStageFlags srcStage = combinedSrcStage != 0 ? combinedSrcStage : VK_PIPELINE_STAGE_ALL_COMMANDS_BIT;
VkPipelineStageFlags dstStage = combinedDstStage != 0 ? combinedDstStage : VK_PIPELINE_STAGE_ALL_COMMANDS_BIT;

vkCmdPipelineBarrier(
cmd,
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT,
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT,
srcStage,
dstStage,
0,
static_cast<uint32_t>(memoryBarriers.size()),
memoryBarriers.data(),
0, nullptr,
0, nullptr,
static_cast<uint32_t>(pass.prePassBarriers.size()),
pass.prePassBarriers.data()
static_cast<uint32_t>(imageBarriers.size()),
imageBarriers.data()
);
}
}
Expand All @@ -396,8 +447,13 @@ void RenderGraph::execute(VkCommandBuffer cmd, ResourceRegistry& registry, uint3
compiled.definition.name + "'. Call build() before execute().");
}

// Skip rendering if extent is zero (e.g., minimized window)
if (compiled.extent.width == 0 || compiled.extent.height == 0) {
continue;
}

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

// 2. Begin render pass
VkRenderPassBeginInfo rpBegin{};
Expand Down
33 changes: 29 additions & 4 deletions FarmEngine/src/rendering/graph/RenderGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,20 +158,45 @@ class RenderGraph {
void cleanup(VkDevice device);

private:
// Pending barrier info for resource-specific dependencies - resolved at execution time
struct PendingResourceBarrier {
std::string resourceName;
VkPipelineStageFlags srcStageMask;
VkPipelineStageFlags dstStageMask;
VkAccessFlags srcAccessMask;
VkAccessFlags dstAccessMask;
VkImageLayout oldLayout;
VkImageLayout newLayout;
VkImageAspectFlags aspectMask;
};

// Pass-level execution dependency info - emitted as VkMemoryBarrier at execution time
struct PassLevelDependency {
uint32_t fromPass;
uint32_t toPass;
VkPipelineStageFlags srcStageMask;
VkPipelineStageFlags dstStageMask;
VkAccessFlags srcAccessMask;
VkAccessFlags dstAccessMask;
VkDependencyFlags dependencyFlags;
};

struct CompiledPass {
RenderPass definition;
std::vector<VkAttachmentDescription> attachments;
std::vector<VkAttachmentReference> colorRefs;
VkAttachmentReference depthRef = {VK_ATTACHMENT_UNUSED, VK_IMAGE_LAYOUT_UNDEFINED};
std::vector<VkSubpassDependency> dependencies;
VkRenderPass vkRenderPass = VK_NULL_HANDLE;
VkFramebuffer framebuffer = VK_NULL_HANDLE;

// Extent calculado durante la creación del framebuffer para usar en execute()
VkExtent2D extent = {0, 0};

// Barreras de transición antes del pass
std::vector<VkImageMemoryBarrier> prePassBarriers;
// Resource-specific barriers (resolved at execution time)
std::vector<PendingResourceBarrier> pendingResourceBarriers;

// Pass-level dependencies (emitted as memory barriers at execution time)
std::vector<PassLevelDependency> passLevelDependencies;
};

std::vector<CompiledPass> compiledPasses;
Expand All @@ -182,7 +207,7 @@ class RenderGraph {

void createRenderPasses(VkDevice dev);
void createFramebuffers(VkDevice dev, VkExtent2D swapchainExtent);
void recordBarriers(VkCommandBuffer cmd, const CompiledPass& pass);
void recordBarriers(VkCommandBuffer cmd, const CompiledPass& pass, const ResourceRegistry& registry);
};

} // namespace FarmEngine::Render