Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
71 changes: 54 additions & 17 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,29 +1,66 @@
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The .gitignore file starts with triple backticks (```), which appear to be a markdown formatting artifact. This line should be removed as it is not a valid git pattern.

# Build artifacts
# Compiled and binary files
*.o
*.obj
*.a
*.lib
*.so
*.dll
*.exe
*.dll
*.so
*.a
*.out

# CMake build directories
build/
cmake-build-*/
CMakeFiles/
CMakeCache.txt
Makefile
compile_commands.json

# Dependencies
vendor/
node_modules/
venv/
.venv/
__pycache__/
dist/
build/
target/
.gradle/

# Logs
*.log
# Editors/IDE
.vscode/
.idea/
*.swp
*.swo

# OS generated files
# System/Environment
.DS_Store
Thumbs.db
.env
.env.local
*.env.*

# Logs and temp files
*.log
*.tmp

# Coverage
coverage/
htmlcov/
.coverage

# Compressed files
*.zip
*.gz
*.tar
*.tgz
*.bz2
*.xz
*.7z
*.rar
*.zst
*.lz4
*.lzh
*.cab
*.arj
*.rpm
*.deb
*.Z
*.lz
*.lzo
*.tar.gz
*.tar.bz2
*.tar.xz
*.tar.zst
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The .gitignore file ends with triple backticks (```), which should be removed.

53 changes: 49 additions & 4 deletions FarmEngine/src/rendering/graph/RenderGraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ const ResourceHandle* ResourceRegistry::getResource(const std::string& name) con
if (it == nameToIndex.end()) {
return nullptr;
}
// Defensive bounds check to prevent out-of-bounds access with stale indices
if (it->second >= resources.size()) {
return nullptr;
}
return &resources[it->second];
}

Expand Down Expand Up @@ -123,10 +127,13 @@ RenderGraphBuilder& RenderGraphBuilder::addDependency(uint32_t from, uint32_t to
// ============================================================================
// RenderGraph Implementation
// ============================================================================

void RenderGraph::compile(RenderGraphBuilder&& builder) {
// Copiar recursos y passes
compiledRegistry.resources = std::move(builder.resources);

// Limpiar estado previo antes de compilar
compiledRegistry.nameToIndex.clear();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛑 Logic Error: The nameToIndex.clear() is essential but the bounds check added in getResource() won't protect updateResourceState() which also accesses resources[it->second] without bounds checking (line 44). Since updateResourceState() isn't being fixed in this PR, stale indices can still cause crashes through that method. Consider adding the same bounds check to updateResourceState() to fully resolve the stale index issue.

compiledPasses.clear();

compiledPasses.resize(builder.passes.size());
Expand Down Expand Up @@ -197,9 +204,29 @@ RenderGraphBuilder& RenderGraphBuilder::addDependency(uint32_t from, uint32_t to
// TODO: Implementar análisis de dependencias del grafo
}

// Las dependencias entre passes separados se manejan mediante barreras de pipeline
// registradas durante la ejecución, no mediante VkSubpassDependency
// VkSubpassDependency solo es válido para subpasses dentro del mismo VkRenderPass
// Procesar dependencias explícitas y convertirlas en barreras por-pass
for (const auto& dep : builder.explicitDependencies) {
if (dep.to < compiledPasses.size()) {
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;
Comment on lines +214 to +215
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛑 Logic Error: Image memory barriers are created with both oldLayout and newLayout set to VK_IMAGE_LAYOUT_UNDEFINED. This means no actual layout transition will occur, defeating the purpose of the barrier. The oldLayout and newLayout should be set based on the actual resource states and pass requirements, not hardcoded to UNDEFINED.

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};

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

Choose a reason for hiding this comment

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

high

This block creates VkImageMemoryBarrier objects that are invalid for use in vkCmdPipelineBarrier for several reasons:

  1. Null Image Handle: barrier.image is set to VK_NULL_HANDLE. According to the Vulkan specification, an image memory barrier must reference a valid VkImage handle.
  2. Invalid Layouts: Both oldLayout and newLayout are set to VK_IMAGE_LAYOUT_UNDEFINED. This does not perform a valid layout transition and is generally a no-op or invalid for synchronization intended to make data visible.
  3. Hardcoded Aspect Mask: The subresourceRange.aspectMask is hardcoded to VK_IMAGE_ASPECT_COLOR_BIT, which will cause issues if the dependency involves a depth or stencil attachment.

Since the PassDependency structure currently lacks information about which resource is being synchronized, and prePassBarriers is defined as a vector of VkImageMemoryBarrier, this logic cannot function as intended. If these are meant to be general execution dependencies between passes, consider using VkMemoryBarrier instead.

}
Comment on lines +207 to +222
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Null vulkan image barriers 🐞 Bug ≡ Correctness

RenderGraph::compile() builds VkImageMemoryBarrier objects with image=VK_NULL_HANDLE and
UNDEFINED->UNDEFINED layouts, and recordBarriers() submits them unchanged, which will trigger Vulkan
validation errors or fail to synchronize passes. This also ignores the dependency’s stage masks and
hardcodes VK_IMAGE_ASPECT_COLOR_BIT, breaking depth dependencies (e.g., ShadowMap).
Agent Prompt
## Issue description
`RenderGraph::compile()` turns `PassDependency` into `VkImageMemoryBarrier` entries with `image = VK_NULL_HANDLE` and `oldLayout/newLayout = VK_IMAGE_LAYOUT_UNDEFINED`, and `recordBarriers()` submits them as-is. This is invalid Vulkan usage and also ignores the dependency’s `srcStageMask/dstStageMask`.

## Issue Context
`PassDependency` currently contains stage/access masks but does not identify *which* resource/image should be synchronized, so emitting an `VkImageMemoryBarrier` cannot be correct unless you extend the dependency model or compute resource-level transitions.

## Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[207-223]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[308-321]
- FarmEngine/src/rendering/graph/RenderGraph.h[53-60]
- FarmEngine/src/rendering/graph/RenderGraph.h[145-159]

## Suggested fix direction
Choose one:
1) **If dependencies are pass-level only** (no specific resource): replace `VkImageMemoryBarrier` with a `VkMemoryBarrier` and submit it using the dependency’s `srcStageMask/dstStageMask`.
2) **If dependencies are resource-specific**: extend `PassDependency` (or add a new API) to include the resource name and expected layouts/aspect; then at execution time fill `barrier.image`, `subresourceRange.aspectMask` (depth vs color), and use the recorded stage masks in `vkCmdPipelineBarrier`.

Also validate `dep.from`/`dep.to` indices and fail fast on out-of-range dependencies.

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

}
}

void RenderGraph::build(VkDevice dev, VkExtent2D swapchainExtent) {
device = dev;
createRenderPasses(device);
createFramebuffers(device, swapchainExtent);
}
Comment on lines +226 to 230
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

2. Graph build step unused 🐞 Bug ≡ Correctness

VkRenderPass/VkFramebuffer creation has been moved into a new RenderGraph::build(), but existing
render-pipeline code only calls compile(), so execute() will use VK_NULL_HANDLE
renderPass/framebuffer. This will cause vkCmdBeginRenderPass() validation failures/crashes when the
graph is executed.
Agent Prompt
## Issue description
The PR introduces `RenderGraph::build(VkDevice, VkExtent2D)` to create render passes and framebuffers, but no existing code calls it; `execute()` uses `compiled.vkRenderPass` and `compiled.framebuffer` regardless, which remain `VK_NULL_HANDLE`.

## Issue Context
Current renderer examples call only `graph.compile(std::move(builder))`. After this PR, `compile()` compiles attachments/definitions but does not create Vulkan objects.

## Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[131-205]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[226-230]
- FarmEngine/src/rendering/graph/RenderGraph.cpp[324-339]
- FarmEngine/src/rendering/graph/RenderGraph.h[134-137]
- FarmEngine/src/rendering/graph/RenderPipeline.cpp[11-37]

## Suggested fix direction
- Update the owning code path (e.g., the renderer/pipeline builder) to call `graph.build(device, swapchainExtent)` immediately after `compile()`.
- Add a defensive guard in `execute()` that throws/returns if `compiled.vkRenderPass == VK_NULL_HANDLE` or `compiled.framebuffer == VK_NULL_HANDLE`, to fail fast and avoid submitting invalid Vulkan commands.
- If you intended `compile()` to remain a single-step API, move `createRenderPasses/createFramebuffers` back into `compile()` (or have `compile()` call `build()` when `device` and extent are known).

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


void RenderGraph::createRenderPasses(VkDevice dev) {
Expand Down Expand Up @@ -308,7 +335,25 @@ void RenderGraph::execute(VkCommandBuffer cmd, ResourceRegistry& registry, uint3
rpBegin.sType = VK_STRUCTURE_TYPE_RENDER_PASS_BEGIN_INFO;
rpBegin.renderPass = compiled.vkRenderPass;
rpBegin.framebuffer = compiled.framebuffer;
rpBegin.renderArea.extent = compiled.extent;
// 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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛑 Logic Error: The hardcoded fallback extent of 800x600 is arbitrary and may not match the actual framebuffer dimensions, potentially causing rendering artifacts or validation errors. Since compiled.extent is set in createFramebuffers() at line 291, this fallback should never be needed unless there's a logic error. Consider adding validation to ensure extent is always properly set, or throw an error if it's {0,0} rather than using an arbitrary fallback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The fallback extent {800, 600} is a hardcoded magic number. It would be better to use a more appropriate default, such as the swapchainExtent provided during the build phase, or ensure that compiled.extent is always correctly initialized in createFramebuffers.

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;
}

// Clear values - uno por cada attachment (color attachments primero, luego depth)
std::vector<VkClearValue> clearValues;
Expand Down
2 changes: 2 additions & 0 deletions FarmEngine/src/rendering/graph/RenderGraph.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@ class RenderGraph {
RenderGraph() = default;
~RenderGraph();

void setDevice(VkDevice dev) { device = dev; }
void compile(RenderGraphBuilder&& builder);
void build(VkDevice dev, VkExtent2D swapchainExtent);
void execute(VkCommandBuffer cmd, ResourceRegistry& registry, uint32_t frameIndex);

// Acceso a recursos compilados
Expand Down