Skip to content

Route swapchain present synchronization through RHIExecutor#157

Closed
Copilot wants to merge 13 commits into
dev_parallel_rhifrom
copilot/simplify-glfw-window-context
Closed

Route swapchain present synchronization through RHIExecutor#157
Copilot wants to merge 13 commits into
dev_parallel_rhifrom
copilot/simplify-glfw-window-context

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 23, 2026

  • 读取 review comment 涉及的 RHIImpl.cppVulkanSwapChain.cpp,确认需要处理的是 Vulkan 头显式包含和重复 assert 两项
  • 检查 stacked PR 的 base branch dev_parallel_rhi workflow runs;当前未发现相关 runs,可继续按代码 review 修正
  • RHIImpl.cpp 显式包含 Vulkan core 头,避免依赖 include 顺序让 VkResult/VkInstance/VkSurfaceKHR 隐式可见
  • VulkanSwapChain.cpp 删除与前置 MOER_ASSERT 重复的 plain assert,统一断言语义
  • 用现有仓库命令做最小化验证:已完成 cmake -B build;当前 Linux 配置下未暴露可用的 render/RHI 目标脚本覆盖这两个 TU,仓库现有验证覆盖存在缺口

Copilot AI and others added 12 commits April 23, 2026 05:56
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/98b0e8f7-c6e3-45ad-b5e4-2236b363bf86

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/98b0e8f7-c6e3-45ad-b5e4-2236b363bf86

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/98b0e8f7-c6e3-45ad-b5e4-2236b363bf86

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/98b0e8f7-c6e3-45ad-b5e4-2236b363bf86

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/98b0e8f7-c6e3-45ad-b5e4-2236b363bf86

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/504f8916-ef36-4b64-bc4d-215c366a9c60

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/504f8916-ef36-4b64-bc4d-215c366a9c60

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/504f8916-ef36-4b64-bc4d-215c366a9c60

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates present-facing lifecycle code (resize/destroy/release) away from caller-owned queue synchronization and toward RHIExecutor-owned synchronization, while also refactoring swapchain surface creation to flow through a new WindowSurfaceSource abstraction and documenting the follow-up design for viewport-scoped present sync.

Changes:

  • Replaced direct GraphicsQueue::Sync() waits in present-facing paths with RHIExecutor::Sync(ERHISyncDepth::Present) (and ERHISyncDepth::RHI for non-present cleanup).
  • Refactored SwapchainSurfaceInfo to carry a WindowSurfaceSourceRef and introduced RenderDevice::CreateSwapchainSurfaceInfo(window) to standardize surface plumbing.
  • Added a checkpoint document describing the next-step contract for per-viewport/swapchain present synchronization.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
source/test/RHITest/rhi_translate_multiqueue_test.cpp Updates tests to use device.CreateSwapchainSurfaceInfo and new SurfaceInitInfo signature.
source/runtime/render/window/glfw/GLFWWindowImpl.h Removes Vulkan-surface-specific API and introduces interop-handle retrieval.
source/runtime/render/window/glfw/GLFWWindowImpl.cpp Implements GetInteropHandle and removes old Vulkan init/surface creation paths.
source/runtime/render/window/WindowContextImpl.h Adds EWindowInteropHandleType and new interop-handle API surface for window impls.
source/runtime/render/window/WindowContext.h Removes SurfaceInitInfo::rhi_type and drops old native window / Vulkan surface helpers.
source/runtime/render/window/WindowContext.cpp Removes old native window/surface wrapper APIs; adds GetWindowInteropHandle free function.
source/runtime/render/rhi/vulkan/VulkanSwapChain.cpp Swapchain recreation now validates and routes surface creation via WindowSurfaceSource.
source/runtime/render/rhi/vulkan/VulkanDevice.h Adds CreateSwapchainSurfaceInfo override.
source/runtime/render/rhi/vulkan/VulkanDevice.cpp Implements CreateSwapchainSurfaceInfo via platform helper.
source/runtime/render/rhi/d3d12/D3D12Device.h Adds CreateSwapchainSurfaceInfo override.
source/runtime/render/rhi/d3d12/D3D12Device.cpp Implements CreateSwapchainSurfaceInfo via platform helper.
source/runtime/render/rhi/RHIResource.h Introduces WindowSurfaceSource + updates SwapchainSurfaceInfo to hold source.
source/runtime/render/rhi/RHIImpl.h Extends device impl interface for swapchain surface creation; adds platform helper declaration.
source/runtime/render/rhi/RHIImpl.cpp Implements GLFW-backed WindowSurfaceSource and platform surface-info factory.
source/runtime/render/rhi/RHI.h Exposes RenderDevice::CreateSwapchainSurfaceInfo.
source/runtime/render/renderer/common/ui/ImGUIRenderer.cpp Routes viewport destroy/resize sync through RHIExecutor::Sync(Present).
source/runtime/render/renderer/common/PresentationSurface.h Removes private surface-info builder helper.
source/runtime/render/renderer/common/PresentationSurface.cpp Uses device.CreateSwapchainSurfaceInfo(window) for swapchain creation.
source/runtime/render/renderer/Renderer.cpp Replaces explicit queue sync with executor-owned present/RHI sync.
source/runtime/Engine.cpp Updates window init for new SurfaceInitInfo ctor signature.
source/profiler/main.cpp Updates window init signature and present resize sync through RHIExecutor.
docs/checkpoints/swapchain_present_sync_checkpoint.json Documents follow-up design for viewport-scoped present synchronization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

#include "window/WindowContextImpl.h"
#include "shader/ShaderResourceManager.h"

#include "rhi/plugin/NrdPlugin.h"
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

This TU uses Vulkan types and glfwCreateWindowSurface but doesn’t explicitly include a Vulkan core header (or define GLFW_INCLUDE_VULKAN before including glfw3.h). That can make VkResult/VkInstance/VkSurfaceKHR undefined depending on include order/build flags. Please add an explicit Vulkan include or the GLFW_INCLUDE_VULKAN define here to guarantee compilation.

Suggested change
#include "rhi/plugin/NrdPlugin.h"
#include "rhi/plugin/NrdPlugin.h"
#define GLFW_INCLUDE_VULKAN

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot fix glfw vulkan include issue

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

已修复,提交在 3b0adf5/home/runner/work/MoerEngine/MoerEngine/source/runtime/render/rhi/RHIImpl.cpp 现在显式包含了 <vulkan/vulkan_core.h>,不再依赖 glfw3.h 或 include 顺序来提供 Vulkan 类型定义。

Comment on lines +145 to +149
const WindowSurfaceSource* existing_surface_source = surface_info.source.get();
const WindowSurfaceSource* incoming_surface_source = _info.surface.source.get();
MOER_ASSERT(
incoming_surface_source != nullptr,
"Vulkan swapchain recreate requires a valid incoming surface source from SwapchainCreateInfo"
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

CreateOrRecreate already hard-asserts incoming_surface_source via MOER_ASSERT here, but later also uses a plain assert(incoming_surface_source && ...) before CreateSurface. It’s redundant and mixes assertion mechanisms; consider removing the duplicate assert (or converting it to MOER_ASSERT) to keep failure behavior consistent across builds.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

已处理,提交在 3b0adf5:保留了 RHIImpl.cpp 的显式 Vulkan core 头包含,并删除了 VulkanSwapChain.cpp 里与前置 MOER_ASSERT 重复的 plain assert

Agent-Logs-Url: https://github.com/NJUCG/MoerEngine/sessions/c174d796-74d4-4c35-92e8-55333bdab298

Co-authored-by: Valer1ie <61175238+Valer1ie@users.noreply.github.com>
Copilot AI requested a review from Valer1ie April 23, 2026 14:32
@Valer1ie Valer1ie marked this pull request as ready for review April 23, 2026 14:35
@Valer1ie Valer1ie closed this Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants