Conversation
📝 WalkthroughWalkthroughThe PR refactors the frame synchronization architecture in VulkanKernel from a per-frameSyncs to a frames-based approach with explicit frameIndex/imageIndex tracking. It introduces new fence utility functions in VulkanTools and adjusts render pass synchronization dependencies for the multisampling path. Changes
Sequence Diagram(s)sequenceDiagram
participant VK as Vulkan Device
participant Kernel as VulkanKernel
participant Frame as Frame[N]
participant Sync as Synchronization
rect rgb(230, 245, 250)
Note over Kernel,Frame: Frame Preparation Phase
Kernel->>Frame: Wait on m_frames[frameIndex].inFlightFence
activate Frame
VK->>Sync: Wait for fence completion
deactivate Frame
Kernel->>Frame: Acquire image with m_frames[frameIndex].imageAvailable
Frame->>VK: vkAcquireNextImageKHR()
VK->>Kernel: Return imageIndex
end
rect rgb(245, 240, 230)
Note over Kernel,Frame: Rendering Phase
Kernel->>Kernel: Bind m_frameCmdPools[imageIndex]
Kernel->>Kernel: Record render commands
end
rect rgb(240, 250, 235)
Note over Kernel,Frame: Submission & Presentation
Kernel->>Frame: Queue present with m_frames[frameIndex].renderFinished
Frame->>VK: vkQueuePresentKHR()
Kernel->>Frame: Update m_imagesInFlight[imageIndex] fence
end
rect rgb(250, 235, 235)
Note over Kernel,Sync: Synchronization Recreation (if needed)
Kernel->>Sync: DestroySynchronizations(FrameResult reason)
Kernel->>Sync: ReCreateSynchronizations(reason)
activate Sync
Sync->>Frame: Recreate per-frame primitives
deactivate Sync
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @innerviewer. * #6 (comment) The following files were modified: * `Core/inc/EvoVulkan/Types/RenderPass.h` * `Core/inc/EvoVulkan/VulkanKernel.h` * `Core/src/EvoVulkan/Tools/VulkanTools.cpp` * `Core/src/EvoVulkan/VulkanKernel.cpp`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Core/inc/EvoVulkan/VulkanKernel.h (1)
139-142: Potential type inconsistency between frame indices and max frames.
GetMaxFramesInFlight()returnsuint8_twhilem_frameIndexandm_imageIndexareuint32_t. Consider using consistent types to avoid potential truncation issues when comparing or using modulo operations.🔎 Suggested fix
- EVK_NODISCARD uint8_t GetMaxFramesInFlight() const noexcept; - EVK_NODISCARD uint8_t GetCurrentImageIndex() const noexcept { return m_imageIndex; } + EVK_NODISCARD uint32_t GetMaxFramesInFlight() const noexcept; + EVK_NODISCARD uint32_t GetCurrentImageIndex() const noexcept { return m_imageIndex; }Core/src/EvoVulkan/VulkanKernel.cpp (2)
876-878: Operator precedence issue in condition.The condition
reason != FrameResult::OutOfDate && reason != FrameResult::Suboptimal || frame.inFlightFence == VK_NULL_HANDLEhas ambiguous operator precedence. Due to&&binding tighter than||, this evaluates as:
(reason != OutOfDate && reason != Suboptimal) || (fence == NULL)If the intent is to create fences when the reason is NOT OutOfDate/Suboptimal OR when the fence is null, this is correct. However, if the intent is different, add explicit parentheses for clarity.
🔎 Suggested clarification
- if (reason != FrameResult::OutOfDate && reason != FrameResult::Suboptimal || frame.inFlightFence == VK_NULL_HANDLE) { + if ((reason != FrameResult::OutOfDate && reason != FrameResult::Suboptimal) || frame.inFlightFence == VK_NULL_HANDLE) { frame.inFlightFence = Tools::CreateVulkanFence(*m_device, VK_FENCE_CREATE_SIGNALED_BIT); }
1037-1040: Hardcoded MAX_FRAMES_IN_FLIGHT value.Returning a hardcoded
3works but consider making this a named constant for clarity and maintainability.🔎 Suggested improvement
+namespace { + constexpr uint8_t MAX_FRAMES_IN_FLIGHT = 3; +} + uint8_t EvoVulkan::Core::VulkanKernel::GetMaxFramesInFlight() const noexcept { - return 3; + return MAX_FRAMES_IN_FLIGHT; }Alternatively, define it as a
static constexprmember in the class header.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Core/inc/EvoVulkan/Tools/VulkanTools.hCore/inc/EvoVulkan/Types/RenderPass.hCore/inc/EvoVulkan/VulkanKernel.hCore/src/EvoVulkan/Tools/VulkanTools.cppCore/src/EvoVulkan/VulkanKernel.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
Core/inc/EvoVulkan/Tools/VulkanTools.h (1)
Core/src/EvoVulkan/Tools/VulkanTools.cpp (4)
CreateVulkanFence(169-178)CreateVulkanFence(169-169)DestroyVulkanFence(180-185)DestroyVulkanFence(180-180)
Core/inc/EvoVulkan/VulkanKernel.h (1)
Core/src/EvoVulkan/VulkanKernel.cpp (6)
GetMaxFramesInFlight(1038-1040)GetMaxFramesInFlight(1038-1038)DestroySynchronizations(842-866)DestroySynchronizations(842-842)ReCreateSynchronizations(868-901)ReCreateSynchronizations(868-868)
🔇 Additional comments (7)
Core/inc/EvoVulkan/Types/RenderPass.h (1)
394-402: Improved render pass dependency synchronization.The changes tighten the synchronization scope appropriately:
- Using
VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BITforsrcStageMaskinstead ofTOP_OF_PIPEis more precise and avoids unnecessary pipeline stalls.- Removing
VK_ACCESS_COLOR_ATTACHMENT_READ_BITfromdstAccessMaskis correct since this dependency only needs to ensure writes are visible before the subpass begins writing.Core/inc/EvoVulkan/Tools/VulkanTools.h (1)
354-356: New fence utility functions follow established patterns.The declarations are consistent with the existing semaphore utility functions (
CreateVulkanSemaphore/DestroyVulkanSemaphore) and complement the batch fence functions inVulkanHelper.h(CreateFences/DestroyFences) by providing single-fence management with configurable flags.Core/src/EvoVulkan/Tools/VulkanTools.cpp (1)
169-185: Clean implementation of fence utilities.The implementations correctly:
- Use the
Initializers::FenceCreateInfohelper with the provided flags- Follow the error handling pattern established by
CreateVulkanSemaphore- Null-check and reset the handle in
DestroyVulkanFenceCore/inc/EvoVulkan/VulkanKernel.h (2)
44-49: Well-designed per-frame synchronization structure.The
FrameSyncstruct cleanly encapsulates the per-frame synchronization primitives needed for proper frame-in-flight management. The comments clarify which operations signal each primitive.
219-226: Frame synchronization members are well-organized.The separation between
m_frames(sized to MAX_FRAMES_IN_FLIGHT) andm_imagesInFlight(sized to swapchain image count) correctly implements the standard Vulkan frame-in-flight pattern where the number of in-flight frames can differ from the number of swapchain images.Core/src/EvoVulkan/VulkanKernel.cpp (2)
842-865: Conditional fence destruction logic looks correct.The logic to preserve fences during
OutOfDateandSuboptimalrecreation (while still destroying semaphores) is sensible - fences don't need to be recreated on swapchain resize since they're not tied to swapchain resources, but semaphores used in acquire/present operations should be recreated.
633-674: QueuePresent uses correct frame-based semaphore.Using
m_frames[m_frameIndex].renderFinishedfor the present wait semaphore is correct - it will wait for the GPU work submitted with this frame's command buffers to complete before presenting.
| EvoVulkan::Core::FrameResult EvoVulkan::Core::VulkanKernel::PrepareFrame() { | ||
| EVK_TRACY_ZONE; | ||
|
|
||
| if (m_swapchain->IsDirty()) { | ||
| VK_LOG("VulkanKernel::PrepareFrame() : swapchain is dirty!"); | ||
| } | ||
|
|
||
| /// Acquire the next image from the swap chain | ||
| VkResult result = m_swapchain->AcquireNextImage(m_frameSyncs[m_currentBuffer].m_presentComplete, &m_currentImage); | ||
| /// Recreate the swapchain if it's no longer compatible with the surface (OUT_OF_DATE) or no longer optimal for presentation (SUBOPTIMAL) | ||
| FrameSync& frame = m_frames[m_frameIndex]; | ||
|
|
||
| // 1. Ждём, пока этот sync-slot освободится | ||
| { | ||
| EVK_TRACY_ZONE_N("Wait for in-flight fence"); | ||
| EVK_TRACY_ZONE_COLOR(0xffa500); | ||
| vkWaitForFences(*m_device, 1, &frame.inFlightFence, VK_TRUE, UINT64_MAX); | ||
| } | ||
|
|
||
| // 2. Получаем image от swapchain | ||
| VkResult result = m_swapchain->AcquireNextImage(frame.imageAvailable, &m_imageIndex); | ||
| if (result == VK_ERROR_OUT_OF_DATE_KHR) { | ||
| VK_LOG("VulkanKernel::PrepareFrame() : window has been resized!"); | ||
| return FrameResult::OutOfDate; | ||
| } | ||
| else if (result == VK_SUBOPTIMAL_KHR) { | ||
| VK_LOG("VulkanKernel::PrepareFrame() : window has been suboptimal!"); | ||
| return FrameResult::Suboptimal; | ||
| } | ||
| else if (result != VK_SUCCESS) { | ||
| VK_ERROR("VulkanKernel::PrepareFrame() : failed to acquire next image! Reason: " + | ||
| Tools::Convert::result_to_description(result)); | ||
|
|
||
| VK_ERROR("VulkanKernel::PrepareFrame() : failed to acquire next image! Reason: " + Tools::Convert::result_to_description(result)); | ||
| return FrameResult::Error; | ||
| } | ||
|
|
||
| return FrameResult::Success; | ||
| // 3. Если image уже используется — ждём fence | ||
| if (m_imagesInFlight[m_imageIndex] != VK_NULL_HANDLE) { | ||
| EVK_TRACY_ZONE_N("Wait for image in-flight fence"); | ||
| EVK_TRACY_ZONE_COLOR(0xff4500); | ||
| vkWaitForFences(*m_device, 1, &m_imagesInFlight[m_imageIndex], VK_TRUE, UINT64_MAX); | ||
| } | ||
|
|
||
| // 4. Привязываем image к текущему frame fence | ||
| m_imagesInFlight[m_imageIndex] = frame.inFlightFence; | ||
|
|
||
| return result == VK_SUBOPTIMAL_KHR ? FrameResult::Suboptimal : FrameResult::Success; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for m_frameIndex increment/advancement patterns
echo "=== Searching for m_frameIndex modifications ==="
rg -n "m_frameIndex\s*(=|\+=|\+\+)" --type cpp
echo ""
echo "=== Searching for vkResetFences calls ==="
rg -n "vkResetFences" --type cpp
echo ""
echo "=== Searching for QueuePresent to see post-frame operations ==="
rg -n "QueuePresent" Core/src/EvoVulkan/VulkanKernel.cpp -A 50Repository: SpaRcle-Studio/EvoVulkan
Length of output: 4571
🏁 Script executed:
#!/bin/bash
# Look for where frame submission happens and fence reset
echo "=== Check for vkWaitForFences/vkResetFences pattern ==="
rg -n "vkWaitForFences|vkResetFences" Core/src/EvoVulkan/VulkanKernel.cpp
echo ""
echo "=== Look for all occurrences of m_frameIndex in cpp ==="
rg -n "m_frameIndex" Core/src/EvoVulkan/VulkanKernel.cpp
echo ""
echo "=== Check PrepareFrame and surrounding context (550-610) ==="
sed -n '550,610p' Core/src/EvoVulkan/VulkanKernel.cpp | cat -n
echo ""
echo "=== Check for command buffer submission/fence reset in broader context ==="
rg -n "BeginFrame|vkQueueSubmit|vkResetFences|ResetFences" Core/src/EvoVulkan/VulkanKernel.cppRepository: SpaRcle-Studio/EvoVulkan
Length of output: 3967
🏁 Script executed:
#!/bin/bash
# Check the vkQueueSubmit context around line 629
echo "=== vkQueueSubmit context (lines 620-640) ==="
sed -n '620,640p' Core/src/EvoVulkan/VulkanKernel.cpp | cat -n
echo ""
echo "=== Search for where frame.inFlightFence is used in submission ==="
rg -n "inFlightFence" Core/src/EvoVulkan/VulkanKernel.cpp
echo ""
echo "=== Look for the full SubmitFrame/rendering submission ==="
sed -n '676,690p' Core/src/EvoVulkan/VulkanKernel.cpp | cat -n
echo ""
echo "=== Check header file for frame structure and methods ==="
grep -n "FrameSync\|inFlightFence\|renderFinished" Core/inc/EvoVulkan/VulkanKernel.h | head -20Repository: SpaRcle-Studio/EvoVulkan
Length of output: 3177
Missing frame index advancement and fence reset.
The PrepareFrame implementation has the correct structure for frame-in-flight management but is missing critical pieces:
-
m_frameIndex is never incremented — After a successful frame,
m_frameIndexshould advance:m_frameIndex = (m_frameIndex + 1) % GetMaxFramesInFlight(). Currently only two references exist (lines 565 and 637), both retrieving the frame without any increment. This breaks the ring-buffer pattern for multi-frame synchronization. -
Fence reset is missing — Before submitting work that signals
frame.inFlightFence, you must callvkResetFences(). The fence is created withVK_FENCE_CREATE_SIGNALED_BIT, which is correct for the first iteration, but after the initialvkWaitForFencesreturns, the fence remains unsignaled. Without reset, subsequent waits will not work correctly.
These operations must occur: m_frameIndex should be incremented in QueuePresent() or at the start of the next PrepareFrame(), and vkResetFences() must be called before the graphics command buffer submission that signals the fence.
🤖 Prompt for AI Agents
Core/src/EvoVulkan/VulkanKernel.cpp around lines 558-599: PrepareFrame currently
waits on the in-flight fence and assigns m_imagesInFlight but never resets the
waited fence or advances the ring-buffer index; to fix, after
vkWaitForFences(*m_device, 1, &frame.inFlightFence, VK_TRUE, UINT64_MAX) call
vkResetFences(*m_device, 1, &frame.inFlightFence) to clear the fence before it
will be signaled again, and before returning success/suboptimal increment the
frame index with m_frameIndex = (m_frameIndex + 1) % GetMaxFramesInFlight() (or
equivalent) so the next PrepareFrame uses the next FrameSync slot. Ensure these
two operations are done before any submission that will signal the fence and
that GetMaxFramesInFlight() matches m_frames.size().
3 participants
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.