Skip to content

Update from task f96c1ed4-2a53-4444-90e8-9b57f386bac1 #25

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 3 commits into
base: chunkkeyubfixsimulationmoduleperformance-ebd4e
Choose a base branch
from
Open

Update from task f96c1ed4-2a53-4444-90e8-9b57f386bac1 #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4693bad
Fix critical RenderGraph and ChunkSystem bugs
qwen-intl Apr 2, 2026
6c5afb7
Update FarmEngine/src/rendering/graph/RenderGraph.cpp
tronpis Apr 2, 2026
65e4332
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
26 changes: 2 additions & 24 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
*.obj
*.a
*.lib
*.dll
*.so
*.dylib
*.dll
*.exe
*.out

Expand All @@ -20,32 +19,11 @@ compile_commands.json

# Dependencies
vendor/
external/

# Logs
*.log

# Temporary files
*.tmp
*.temp

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

# Editors
.vscode/
.idea/
*.swp
*.swo

# Coverage
coverage/
htmlcov/
.coverage

# OS
# OS generated files
.DS_Store
Thumbs.db
Comment on lines +26 to 28
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.

medium

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

```
8 changes: 4 additions & 4 deletions FarmEngine/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@ 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
src/rendering/graph/RenderGraph.h
src/rendering/graph/RenderGraph.cpp
src/rendering/graph/RenderPipeline.h
src/rendering/graph/RenderPipeline.cpp
)

target_include_directories(FarmEngineRenderer PUBLIC
Expand Down
71 changes: 35 additions & 36 deletions FarmEngine/src/rendering/graph/RenderGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,17 +123,14 @@ RenderGraphBuilder& RenderGraphBuilder::addDependency(uint32_t from, uint32_t to
// ============================================================================
// RenderGraph Implementation
// ============================================================================

RenderGraph::~RenderGraph() {
// Destructor - cleanup se hace explícitamente en cleanup()
}

void RenderGraph::compile(RenderGraphBuilder&& builder) {
device = VK_NULL_HANDLE; // Se seteará cuando tengamos contexto Vulkan

// Copiar recursos y passes
compiledRegistry.resources = std::move(builder.resources);

// Limpiar estado previo antes de compilar
compiledPasses.clear();

compiledPasses.resize(builder.passes.size());
Comment thread
tronpis marked this conversation as resolved.
Comment thread
tronpis marked this conversation as resolved.


// Construir mapa nombre->índice
Comment on lines +137 to 149
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. 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

for (size_t i = 0; i < compiledRegistry.resources.size(); ++i) {
Expand Down Expand Up @@ -200,21 +197,9 @@ void RenderGraph::compile(RenderGraphBuilder&& builder) {
// TODO: Implementar análisis de dependencias del grafo
}

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

void RenderGraph::createRenderPasses(VkDevice dev) {
Expand Down Expand Up @@ -251,21 +236,33 @@ void RenderGraph::createFramebuffers(VkDevice dev, VkExtent2D 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 (!res) {
throw std::runtime_error("Color attachment not found: " + colorName);
}
if (res->imageView == VK_NULL_HANDLE) {
throw std::runtime_error("Color attachment has null imageView: " + colorName +
". For external targets, use addExternalImage() instead of addColorTarget().");
}
attachments.push_back(res->imageView);
if (!res->isSwapchain) {
extent = res->extent;
}
Comment on lines +247 to 249
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.

medium

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.

}

if (!compiled.definition.depthAttachment.empty()) {
const ResourceHandle* res = compiledRegistry.getResource(compiled.definition.depthAttachment);
if (res) {
attachments.push_back(res->imageView);
if (!res) {
throw std::runtime_error("Depth attachment not found: " + compiled.definition.depthAttachment);
}
if (res->imageView == VK_NULL_HANDLE) {
throw std::runtime_error("Depth attachment has null imageView: " + compiled.definition.depthAttachment);
}
attachments.push_back(res->imageView);
}

// Guardar el extent calculado en el CompiledPass para usarlo en execute()
compiled.extent = extent;

VkFramebufferCreateInfo fbInfo{};
fbInfo.sType = VK_STRUCTURE_TYPE_FRAMEBUFFER_CREATE_INFO;
fbInfo.renderPass = compiled.vkRenderPass;
Expand Down Expand Up @@ -311,18 +308,20 @@ 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.attachments.empty() ? 0 :
registry.resources[0].extent.width,
compiled.attachments.empty() ? 0 :
registry.resources[0].extent.height};
rpBegin.renderArea.extent = compiled.extent;

Comment on lines 322 to 326
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. 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

// Clear values
// Clear values - uno por cada attachment (color attachments primero, luego depth)
std::vector<VkClearValue> clearValues;
if (!compiled.definition.colorAttachments.empty()) {
clearValues.reserve(compiled.attachments.size());

// Color attachments
for (size_t i = 0; i < compiled.definition.colorAttachments.size(); ++i) {
VkClearValue colorClear{};
colorClear.color = compiled.definition.clearColorValue;
clearValues.push_back(colorClear);
}

// Depth attachment
if (!compiled.definition.depthAttachment.empty()) {
VkClearValue depthClear{};
depthClear.depthStencil = compiled.definition.clearDepthValue;
Expand Down
3 changes: 3 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 @@ -149,6 +149,9 @@ class RenderGraph {
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;
};
Expand Down
1 change: 1 addition & 0 deletions FarmEngine/world/chunks/ChunkSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <mutex>
#include <condition_variable>
#include <atomic>
#include <glm/gtx/norm.hpp>

namespace FarmEngine {

Expand Down