From 3f6e27a91481520c7738f27df4286527c0afde52 Mon Sep 17 00:00:00 2001
From: Karn Kaul <karnkaul@gmail.com>
Date: Sun, 11 May 2025 12:53:36 -0700
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20present=20semaphore=20sync?=
 =?UTF-8?q?=20=20(#25)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Refactor build scripts and presets (#24)

- Move compile flags to presets
- Update CI script

* :bug: Fix present semaphore sync

Discovered on SDK 1.4.313
---
 .github/workflows/ci.yml                |  29 ++---
 CMakePresets.json                       | 156 ++++++++++++++++++++++--
 guide/src/rendering/render_sync.md      |   3 -
 guide/src/rendering/swapchain_loop.md   |   4 +-
 guide/src/rendering/swapchain_update.md |  42 ++++++-
 src/app.cpp                             |   6 +-
 src/app.hpp                             |   6 +-
 src/swapchain.cpp                       |  21 +++-
 src/swapchain.hpp                       |   6 +-
 9 files changed, 229 insertions(+), 44 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 985eec1..bc12bb7 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -1,8 +1,9 @@
-name: ci-push
+name: ci-pr
 on: 
-  push:
+  pull_request:
     branches-ignore:
       - staging
+  workflow_dispatch:
 jobs:
   format-check:
     runs-on: ubuntu-latest
@@ -25,11 +26,11 @@ jobs:
       - name: init
         run: uname -m; sudo apt update -yqq && sudo apt install -yqq ninja-build mesa-common-dev libwayland-dev libxkbcommon-dev wayland-protocols extra-cmake-modules
       - name: configure
-        run: export CXX=g++-14; cmake -S . --preset=default -B build -DGLFW_BUILD_X11=OFF
+        run: cmake -S . --preset=ninja-gcc -B build -DGLFW_BUILD_X11=OFF -DCMAKE_C_COMPILER=gcc-14 -DCMAKE_CXX_COMPILER=g++-14
       - name: build debug
-        run: cmake --build build --config=Debug
+        run: cmake --build build --config=Debug -- -v
       - name: build release
-        run: cmake --build build --config=Release
+        run: cmake --build build --config=Release -- -v
       - name: test debug
         run: cd build && ctest -V -C Debug
       - name: test release
@@ -43,9 +44,9 @@ jobs:
       - name: configure
         run: cmake -S . --preset=ninja-clang -B build -DGLFW_BUILD_X11=OFF
       - name: build debug
-        run: cmake --build build --config=Debug
+        run: cmake --build build --config=Debug -- -v
       - name: build release
-        run: cmake --build build --config=Release
+        run: cmake --build build --config=Release -- -v
       - name: test debug
         run: cd build && ctest -V -C Debug
       - name: test release
@@ -57,11 +58,11 @@ jobs:
       - name: init
         run: uname -m; sudo apt update -yqq && sudo apt install -yqq ninja-build mesa-common-dev libwayland-dev libxkbcommon-dev wayland-protocols extra-cmake-modules
       - name: configure
-        run: export CXX=g++-14; cmake -S . --preset=default -B build -DGLFW_BUILD_X11=OFF
+        run: cmake -S . --preset=ninja-gcc -B build -DGLFW_BUILD_X11=OFF -DCMAKE_C_COMPILER=gcc-14 -DCMAKE_CXX_COMPILER=g++-14
       - name: build debug
-        run: cmake --build build --config=Debug
+        run: cmake --build build --config=Debug -- -v
       - name: build release
-        run: cmake --build build --config=Release
+        run: cmake --build build --config=Release -- -v
       - name: test debug
         run: cd build && ctest -V -C Debug
       - name: test release
@@ -75,9 +76,9 @@ jobs:
       - name: configure
         run: cmake -S . --preset=ninja-clang -B build -DGLFW_BUILD_X11=OFF
       - name: build debug
-        run: cmake --build build --config=Debug
+        run: cmake --build build --config=Debug -- -v
       - name: build release
-        run: cmake --build build --config=Release
+        run: cmake --build build --config=Release -- -v
       - name: test debug
         run: cd build && ctest -V -C Debug
       - name: test release
@@ -105,9 +106,9 @@ jobs:
       - name: configure
         run: cmake -S . --preset=ninja-clang -B clang
       - name: build debug
-        run: cmake --build clang --config=Debug
+        run: cmake --build clang --config=Debug -- -v
       - name: build release
-        run: cmake --build clang --config=Release
+        run: cmake --build clang --config=Release -- -v
       - name: test debug
         run: cd clang && ctest -V -C Debug
       - name: test release
diff --git a/CMakePresets.json b/CMakePresets.json
index 8ddf102..a56321d 100644
--- a/CMakePresets.json
+++ b/CMakePresets.json
@@ -1,14 +1,15 @@
 {
-  "version": 2,
+  "version": 5,
   "cmakeMinimumRequired": {
     "major": 3,
-    "minor": 20,
+    "minor": 24,
     "patch": 0
   },
   "configurePresets": [
     {
       "name": "default",
-      "description": "Build configuration using Ninja Multi-config",
+      "displayName": "Default Config",
+      "description": "Base configuration using Ninja Multi-config",
       "generator": "Ninja Multi-Config",
       "binaryDir": "${sourceDir}/out/default",
       "cacheVariables": {
@@ -16,50 +17,115 @@
       }
     },
     {
-      "name": "ninja-clang",
-      "description": "Build configuration using Ninja Multi-config / clang",
+      "name": "base-gcc",
+      "hidden": true,
+      "inherits": "default",
+      "cacheVariables": {
+        "CMAKE_C_COMPILER": "gcc",
+        "CMAKE_CXX_COMPILER": "g++"
+      }
+    },
+    {
+      "name": "base-clang",
+      "hidden": true,
       "inherits": "default",
-      "binaryDir": "${sourceDir}/out/clang",
       "cacheVariables": {
         "CMAKE_C_COMPILER": "clang",
         "CMAKE_CXX_COMPILER": "clang++"
       }
     },
+    {
+      "name": "base-msvc",
+      "hidden": true,
+      "inherits": "default",
+      "cacheVariables": {
+        "CMAKE_C_COMPILER": "cl",
+        "CMAKE_CXX_COMPILER": "cl"
+      }
+    },
+    {
+      "name": "ninja-gcc",
+      "displayName": "Ninja GCC",
+      "description": "Build configuration using Ninja Multi-config / GCC",
+      "inherits": "base-gcc",
+      "binaryDir": "${sourceDir}/out/gcc",
+      "cacheVariables": {
+        "CMAKE_CXX_FLAGS_DEBUG_INIT": "-Wall -Wextra -Wpedantic -Werror=return-type",
+        "CMAKE_CXX_FLAGS_INIT": "-Wall -Wextra -Wpedantic -Werror"
+      }
+    },
+    {
+      "name": "ninja-clang",
+      "displayName": "Ninja Clang",
+      "description": "Build configuration using Ninja Multi-config / Clang",
+      "inherits": "base-clang",
+      "binaryDir": "${sourceDir}/out/clang",
+      "cacheVariables": {
+        "CMAKE_CXX_FLAGS_DEBUG_INIT": "-Wall -Wextra -Wpedantic -Werror=return-type",
+        "CMAKE_CXX_FLAGS_INIT": "-Wall -Wextra -Wpedantic -Werror"
+      }
+    },
+    {
+      "name": "ninja-msvc",
+      "displayName": "Ninja MSVC",
+      "description": "Build configuration using Ninja Multi-config / MSVC",
+      "inherits": "base-msvc",
+      "binaryDir": "${sourceDir}/out/msvc",
+      "cacheVariables": {
+        "CMAKE_CXX_FLAGS_INIT": "/WX"
+      }
+    },
     {
       "name": "ninja-ubsan",
+      "displayName": "Ninja UBSan",
       "description": "UBSan build configuration using Ninja Multi-config",
       "inherits": "default",
       "binaryDir": "${sourceDir}/out/ubsan",
       "cacheVariables": {
-        "CMAKE_C_FLAGS": "-fsanitize=undefined",
-        "CMAKE_CXX_FLAGS": "-fsanitize=undefined"
+        "CMAKE_CXX_FLAGS_DEBUG_INIT": "-fsanitize=undefined -Wall -Wextra -Wpedantic -Werror=return-type",
+        "CMAKE_CXX_FLAGS_INIT": "-fsanitize=undefined -Wall -Wextra -Wpedantic -Werror"
       }
     },
     {
       "name": "ninja-asan",
+      "displayName": "Ninja ASan",
       "description": "ASan build configuration using Ninja Multi-config",
       "inherits": "default",
       "binaryDir": "${sourceDir}/out/asan",
       "cacheVariables": {
-        "CMAKE_C_FLAGS": "-fsanitize=address",
-        "CMAKE_CXX_FLAGS": "-fsanitize=address"
+        "CMAKE_CXX_FLAGS_DEBUG_INIT": "-fsanitize=address -Wall -Wextra -Wpedantic -Werror=return-type",
+        "CMAKE_CXX_FLAGS_INIT": "-fsanitize=address -Wall -Wextra -Wpedantic -Werror"
+      }
+    },
+    {
+      "name": "ninja-msvc-asan",
+      "displayName": "Ninja MSVC ASan",
+      "description": "ASan build configuration using Ninja Multi-config",
+      "inherits": "base-msvc",
+      "binaryDir": "${sourceDir}/out/asan",
+      "cacheVariables": {
+        "CMAKE_CXX_FLAGS_INIT": "-fsanitize=address"
       }
     },
     {
       "name": "ninja-tsan",
+      "displayName": "Ninja TSan",
       "description": "TSan build configuration using Ninja Multi-config",
       "inherits": "default",
       "binaryDir": "${sourceDir}/out/tsan",
       "cacheVariables": {
-        "CMAKE_C_FLAGS": "-fsanitize=thread",
-        "CMAKE_CXX_FLAGS": "-fsanitize=thread"
+        "CMAKE_CXX_FLAGS_INIT": "-fsanitize=thread -Wall -Wextra -Wpedantic -Werror=return-type"
       }
     },
     {
       "name": "vs22",
+      "displayName": "Visual Studio 2022",
       "description": "Build configuration using Visual Studio 17 (2022)",
       "generator": "Visual Studio 17 2022",
       "binaryDir": "${sourceDir}/out/vs",
+      "cacheVariables": {
+        "CMAKE_CXX_FLAGS_INIT": "/WX"
+      },
       "architecture": {
         "value": "x64",
         "strategy": "external"
@@ -82,6 +148,36 @@
       "configurePreset": "default",
       "configuration": "RelWithDebInfo"
     },
+    {
+      "name": "GCC Debug",
+      "configurePreset": "ninja-gcc",
+      "configuration": "Debug"
+    },
+    {
+      "name": "GCC RelWithDebInfo",
+      "configurePreset": "ninja-gcc",
+      "configuration": "RelWithDebInfo"
+    },
+    {
+      "name": "Clang Debug",
+      "configurePreset": "ninja-clang",
+      "configuration": "Debug"
+    },
+    {
+      "name": "Clang RelWithDebInfo",
+      "configurePreset": "ninja-clang",
+      "configuration": "RelWithDebInfo"
+    },
+    {
+      "name": "MSVC Debug",
+      "configurePreset": "ninja-msvc",
+      "configuration": "Debug"
+    },
+    {
+      "name": "MSVC Release",
+      "configurePreset": "ninja-msvc",
+      "configuration": "Release"
+    },
     {
       "name": "UBSan Debug",
       "configurePreset": "ninja-ubsan",
@@ -107,6 +203,42 @@
       "configuration": "RelWithDebInfo",
       "inheritConfigureEnvironment": true
     },
+    {
+      "name": "GCC Debug",
+      "configurePreset": "ninja-gcc",
+      "configuration": "Debug",
+      "inheritConfigureEnvironment": true
+    },
+    {
+      "name": "GCC RelWithDebInfo",
+      "configurePreset": "ninja-gcc",
+      "configuration": "RelWithDebInfo",
+      "inheritConfigureEnvironment": true
+    },
+    {
+      "name": "Clang Debug",
+      "configurePreset": "ninja-clang",
+      "configuration": "Debug",
+      "inheritConfigureEnvironment": true
+    },
+    {
+      "name": "Clang RelWithDebInfo",
+      "configurePreset": "ninja-clang",
+      "configuration": "RelWithDebInfo",
+      "inheritConfigureEnvironment": true
+    },
+    {
+      "name": "MSVC Debug",
+      "configurePreset": "ninja-msvc",
+      "configuration": "Debug",
+      "inheritConfigureEnvironment": true
+    },
+    {
+      "name": "MSVC Release",
+      "configurePreset": "ninja-msvc",
+      "configuration": "Release",
+      "inheritConfigureEnvironment": true
+    },
     {
       "name": "UBSan Debug",
       "configurePreset": "ninja-ubsan",
diff --git a/guide/src/rendering/render_sync.md b/guide/src/rendering/render_sync.md
index f18023d..d713863 100644
--- a/guide/src/rendering/render_sync.md
+++ b/guide/src/rendering/render_sync.md
@@ -17,8 +17,6 @@ Add a private `struct RenderSync` to `App`:
 struct RenderSync {
   // signaled when Swapchain image has been acquired.
   vk::UniqueSemaphore draw{};
-  // signaled when image is ready to be presented.
-  vk::UniqueSemaphore present{};
   // signaled with present Semaphore, waited on before next render.
   vk::UniqueFence drawn{};
   // used to record rendering commands.
@@ -68,7 +66,6 @@ void App::create_render_sync() {
      std::views::zip(m_render_sync, command_buffers)) {
     sync.command_buffer = command_buffer;
     sync.draw = m_device->createSemaphoreUnique({});
-    sync.present = m_device->createSemaphoreUnique({});
     sync.drawn = m_device->createFenceUnique(fence_create_info_v);
   }
 }
diff --git a/guide/src/rendering/swapchain_loop.md b/guide/src/rendering/swapchain_loop.md
index bf9b8a8..46060fc 100644
--- a/guide/src/rendering/swapchain_loop.md
+++ b/guide/src/rendering/swapchain_loop.md
@@ -18,7 +18,9 @@ Additionally, the number of swapchain images can vary, whereas the engine should
 
 ## Virtual Frames
 
-All the dynamic resources used during the rendering of a frame comprise a virtual frame. The application has a fixed number of virtual frames which it cycles through on each render pass. Each frame will be associated with a `vk::Fence` which will be waited on before rendering to it again. It will also have a pair of `vk::Semaphore`s to synchronize the acquire, render, and present calls on the GPU (we don't need to wait for them in the code). Lastly, there will be a Command Buffer per virtual frame, where all rendering commands for that frame (including layout transitions) will be recorded.
+All the dynamic resources used during the rendering of a frame comprise a virtual frame. The application has a fixed number of virtual frames which it cycles through on each render pass. Each frame will be associated with a `vk::Fence` which will be waited on before rendering to it again. It will also have a `vk::Semaphore` to synchronize the acquire and render calls on the GPU (we don't need to wait for them in the code). Lastly, there will be a Command Buffer per virtual frame, where all rendering commands for that frame (including layout transitions) will be recorded.
+
+Presentation will require a semaphore for synchronization too, but since the swapchain loop waits on the _drawn_ fence, which will be pre-signaled for each virtual frame on first use, present semaphores cannot be part of the virtual frame. It is possible to acquire another image and submit commands with a present semaphore that has not been signaled yet - this is invalid. Thus, these semaphores will be tied to the swapchain images (associated with their indices), and recreated with it.
 
 ## Image Layouts
 
diff --git a/guide/src/rendering/swapchain_update.md b/guide/src/rendering/swapchain_update.md
index 20191d9..5ecf7cf 100644
--- a/guide/src/rendering/swapchain_update.md
+++ b/guide/src/rendering/swapchain_update.md
@@ -1,5 +1,41 @@
 # Swapchain Update
 
+Add a vector of semaphores and populate them in `recreate()`:
+
+```cpp
+void create_present_semaphores();
+
+// ...
+// signaled when image is ready to be presented.
+std::vector<vk::UniqueSemaphore> m_present_semaphores{};
+
+// ...
+auto Swapchain::recreate(glm::ivec2 size) -> bool {
+  // ...
+  populate_images();
+  create_image_views();
+  // recreate present semaphores as the image count might have changed.
+  create_present_semaphores();
+  // ...
+}
+
+void Swapchain::create_present_semaphores() {
+  m_present_semaphores.clear();
+  m_present_semaphores.resize(m_images.size());
+  for (auto& semaphore : m_present_semaphores) {
+    semaphore = m_device.createSemaphoreUnique({});
+  }
+}
+```
+
+Add a function to get the present semaphore corresponding to the acquired image, this will be signaled by render command submission:
+
+```cpp
+auto Swapchain::get_present_semaphore() const -> vk::Semaphore {
+  return *m_present_semaphores.at(m_image_index.value());
+}
+```
+
 Swapchain acquire/present operations can have various results. We constrain ourselves to the following:
 
 - `eSuccess`: all good
@@ -59,13 +95,15 @@ auto Swapchain::acquire_next_image(vk::Semaphore const to_signal)
 Similarly, present:
 
 ```cpp
-auto Swapchain::present(vk::Queue const queue, vk::Semaphore const to_wait)
+auto Swapchain::present(vk::Queue const queue)
   -> bool {
   auto const image_index = static_cast<std::uint32_t>(m_image_index.value());
+  auto const wait_semaphore =
+    *m_present_semaphores.at(static_cast<std::size_t>(image_index));
   auto present_info = vk::PresentInfoKHR{};
   present_info.setSwapchains(*m_swapchain)
     .setImageIndices(image_index)
-    .setWaitSemaphores(to_wait);
+    .setWaitSemaphores(wait_semaphore);
   // avoid VulkanHPP ErrorOutOfDateKHR exceptions by using alternate API.
   auto const result = queue.presentKHR(&present_info);
   m_image_index.reset();
diff --git a/src/app.cpp b/src/app.cpp
index 35537cc..faf2e77 100644
--- a/src/app.cpp
+++ b/src/app.cpp
@@ -132,7 +132,6 @@ void App::create_render_sync() {
 		 std::views::zip(m_render_sync, command_buffers)) {
 		sync.command_buffer = command_buffer;
 		sync.draw = m_device->createSemaphoreUnique({});
-		sync.present = m_device->createSemaphoreUnique({});
 		sync.drawn = m_device->createFenceUnique(fence_create_info_v);
 	}
 }
@@ -256,7 +255,7 @@ void App::submit_and_present() {
 	wait_semaphore_info.setSemaphore(*render_sync.draw)
 		.setStageMask(vk::PipelineStageFlagBits2::eColorAttachmentOutput);
 	auto signal_semaphore_info = vk::SemaphoreSubmitInfo{};
-	signal_semaphore_info.setSemaphore(*render_sync.present)
+	signal_semaphore_info.setSemaphore(m_swapchain->get_present_semaphore())
 		.setStageMask(vk::PipelineStageFlagBits2::eColorAttachmentOutput);
 	submit_info.setCommandBufferInfos(command_buffer_info)
 		.setWaitSemaphoreInfos(wait_semaphore_info)
@@ -270,8 +269,7 @@ void App::submit_and_present() {
 	// framebuffer size does not match the Swapchain image size, check it
 	// explicitly.
 	auto const fb_size_changed = m_framebuffer_size != m_swapchain->get_size();
-	auto const out_of_date =
-		!m_swapchain->present(m_queue, *render_sync.present);
+	auto const out_of_date = !m_swapchain->present(m_queue);
 	if (fb_size_changed || out_of_date) {
 		m_swapchain->recreate(m_framebuffer_size);
 	}
diff --git a/src/app.hpp b/src/app.hpp
index 2391c5b..95d6bfa 100644
--- a/src/app.hpp
+++ b/src/app.hpp
@@ -13,11 +13,9 @@ class App {
 
   private:
 	struct RenderSync {
-		// signalled when Swapchain image has been acquired.
+		// signaled when Swapchain image has been acquired.
 		vk::UniqueSemaphore draw{};
-		// signalled when image is ready to be presented.
-		vk::UniqueSemaphore present{};
-		// signalled with present Semaphore, waited on before next render.
+		// signaled with present Semaphore, waited on before next render.
 		vk::UniqueFence drawn{};
 		// used to record rendering commands.
 		vk::CommandBuffer command_buffer{};
diff --git a/src/swapchain.cpp b/src/swapchain.cpp
index fd08af0..aa32c82 100644
--- a/src/swapchain.cpp
+++ b/src/swapchain.cpp
@@ -120,6 +120,8 @@ auto Swapchain::recreate(glm::ivec2 size) -> bool {
 
 	populate_images();
 	create_image_views();
+	// recreate present semaphores as the image count might have changed.
+	create_present_semaphores();
 
 	size = get_size();
 	std::println("[lvk] Swapchain [{}x{}]", size.x, size.y);
@@ -155,13 +157,18 @@ auto Swapchain::base_barrier() const -> vk::ImageMemoryBarrier2 {
 	return ret;
 }
 
-auto Swapchain::present(vk::Queue const queue, vk::Semaphore const to_wait)
-	-> bool {
+auto Swapchain::get_present_semaphore() const -> vk::Semaphore {
+	return *m_present_semaphores.at(m_image_index.value());
+}
+
+auto Swapchain::present(vk::Queue const queue) -> bool {
 	auto const image_index = static_cast<std::uint32_t>(m_image_index.value());
+	auto const wait_semaphore =
+		*m_present_semaphores.at(static_cast<std::size_t>(image_index));
 	auto present_info = vk::PresentInfoKHR{};
 	present_info.setSwapchains(*m_swapchain)
 		.setImageIndices(image_index)
-		.setWaitSemaphores(to_wait);
+		.setWaitSemaphores(wait_semaphore);
 	// avoid VulkanHPP ErrorOutOfDateKHR exceptions by using alternate API.
 	auto const result = queue.presentKHR(&present_info);
 	m_image_index.reset();
@@ -195,4 +202,12 @@ void Swapchain::create_image_views() {
 		m_image_views.push_back(m_device.createImageViewUnique(image_view_ci));
 	}
 }
+
+void Swapchain::create_present_semaphores() {
+	m_present_semaphores.clear();
+	m_present_semaphores.resize(m_images.size());
+	for (auto& semaphore : m_present_semaphores) {
+		semaphore = m_device.createSemaphoreUnique({});
+	}
+}
 } // namespace lvk
diff --git a/src/swapchain.hpp b/src/swapchain.hpp
index a9aa8ac..a7e5c4c 100644
--- a/src/swapchain.hpp
+++ b/src/swapchain.hpp
@@ -22,11 +22,13 @@ class Swapchain {
 
 	[[nodiscard]] auto base_barrier() const -> vk::ImageMemoryBarrier2;
 
-	[[nodiscard]] auto present(vk::Queue queue, vk::Semaphore to_wait) -> bool;
+	[[nodiscard]] auto get_present_semaphore() const -> vk::Semaphore;
+	[[nodiscard]] auto present(vk::Queue queue) -> bool;
 
   private:
 	void populate_images();
 	void create_image_views();
+	void create_present_semaphores();
 
 	vk::Device m_device{};
 	Gpu m_gpu{};
@@ -35,6 +37,8 @@ class Swapchain {
 	vk::UniqueSwapchainKHR m_swapchain{};
 	std::vector<vk::Image> m_images{};
 	std::vector<vk::UniqueImageView> m_image_views{};
+	// signaled when image is ready to be presented.
+	std::vector<vk::UniqueSemaphore> m_present_semaphores{};
 	std::optional<std::size_t> m_image_index{};
 };
 } // namespace lvk
