From 7376eef424fdf94ddf7f59d76e58fd684d48a04a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Tue, 30 Jan 2024 14:15:15 +0100 Subject: [PATCH 1/6] [NFC] Simplify window size handling This change is the first requirement for cleaning up knobs initialization: BigWheels has settings, which are used to initialize default values, which in turn are used to initialize knobs. But then those settings becomes RW values used at runtime (like window.width, window.height). The ultimate goal is to sanitize this whole initialization step, and only rely on knobs. This first commit removes `window.width` and `window.height` from the settings, and rely on the `Window` class to store its size. This should be a NFC. --- .../GraphicsBenchmarkApp.cpp | 3 +- include/ppx/application.h | 26 ++- include/ppx/window.h | 9 + projects/async_compute/main.cpp | 4 +- projects/gltf/main.cpp | 4 +- src/ppx/application.cpp | 189 +++++++----------- src/ppx/window.cpp | 2 +- src/ppx/window_android.cpp | 2 + src/ppx/window_glfw.cpp | 11 +- 9 files changed, 118 insertions(+), 132 deletions(-) diff --git a/benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp b/benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp index 620e6e1fc..cb54a9d07 100644 --- a/benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp +++ b/benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp @@ -179,8 +179,6 @@ void GraphicsBenchmarkApp::Config(ppx::ApplicationSettings& settings) { settings.appName = "graphics_pipeline"; settings.enableImGui = true; - settings.window.width = 1920; - settings.window.height = 1080; settings.grfx.api = kApi; settings.grfx.numFramesInFlight = 1; settings.grfx.swapchain.depthFormat = grfx::FORMAT_D32_FLOAT; @@ -192,6 +190,7 @@ void GraphicsBenchmarkApp::Config(ppx::ApplicationSettings& settings) #endif settings.standardKnobsDefaultValue.enableMetrics = true; settings.standardKnobsDefaultValue.overwriteMetricsFile = true; + settings.standardKnobsDefaultValue.resolution = std::make_pair(1920, 1080); } void GraphicsBenchmarkApp::Setup() diff --git a/include/ppx/application.h b/include/ppx/application.h index 3099327e2..bb98782c9 100644 --- a/include/ppx/application.h +++ b/include/ppx/application.h @@ -129,8 +129,6 @@ struct ApplicationSettings struct { - uint32_t width = 0; - uint32_t height = 0; std::string title = ""; bool resizable = false; } window; @@ -198,7 +196,7 @@ struct ApplicationSettings bool listGpus = false; std::string metricsFilename = "report_@.json"; bool overwriteMetricsFile = false; - std::pair resolution = std::make_pair(0, 0); + std::pair resolution = std::make_pair(1280, 720); uint32_t runTimeMs = 0; int screenshotFrameNumber = -1; std::string screenshotPath = "screenshot_frame_#.ppm"; @@ -288,15 +286,27 @@ class Application const ApplicationSettings* GetSettings() const { return &mSettings; } const StandardOptions& GetStandardOptions() const { return mStandardOpts; } - uint32_t GetWindowWidth() const { return mSettings.window.width; } - uint32_t GetWindowHeight() const { return mSettings.window.height; } bool IsWindowIconified() const; bool IsWindowMaximized() const; uint32_t GetUIWidth() const; uint32_t GetUIHeight() const; - float GetWindowAspect() const { return static_cast(mSettings.window.width) / static_cast(mSettings.window.height); } - grfx::Rect GetScissor() const; - grfx::Viewport GetViewport(float minDepth = 0.0f, float maxDepth = 1.0f) const; + + std::pair GetWindowSize() const + { + return mWindow != nullptr ? mWindow->Size() : mStandardOpts.pResolution->GetValue(); + } + + uint32_t GetWindowWidth() const { return GetWindowSize().first; } + uint32_t GetWindowHeight() const { return GetWindowSize().second; } + + float GetWindowAspect() const + { + const auto size = GetWindowSize(); + return static_cast(size.first) / static_cast(size.second); + } + + grfx::Rect GetScissor() const; + grfx::Viewport GetViewport(float minDepth = 0.0f, float maxDepth = 1.0f) const; // Loads a DXIL or SPV shader from baseDir. // diff --git a/include/ppx/window.h b/include/ppx/window.h index 83289e7ca..df3ac014b 100644 --- a/include/ppx/window.h +++ b/include/ppx/window.h @@ -42,6 +42,15 @@ struct WindowSize WindowSize() = default; WindowSize(uint32_t width_, uint32_t height_) : width(width_), height(height_) {} + WindowSize(const std::pair&& size) + : WindowSize(size.first, size.second) {} + WindowSize(const std::pair&& size) + : WindowSize(size.first, size.second) {} + + operator std::pair() + { + return {width, height}; + } bool operator==(const WindowSize& other) const { return width == other.width && height == other.height; }; }; diff --git a/projects/async_compute/main.cpp b/projects/async_compute/main.cpp index 58cc16a8a..1345f2390 100644 --- a/projects/async_compute/main.cpp +++ b/projects/async_compute/main.cpp @@ -147,11 +147,11 @@ void ProjApp::Config(ppx::ApplicationSettings& settings) settings.appName = "async_compute"; settings.enableImGui = true; settings.grfx.api = kApi; - settings.window.width = 1920; - settings.window.height = 1080; settings.grfx.swapchain.imageCount = mNumFramesInFlight; settings.grfx.device.computeQueueCount = 1; settings.grfx.numFramesInFlight = mNumFramesInFlight; + + settings.standardKnobsDefaultValue.resolution = std::make_pair(1920, 1080); } void ProjApp::Setup() diff --git a/projects/gltf/main.cpp b/projects/gltf/main.cpp index 4da90f15f..83952e5f4 100644 --- a/projects/gltf/main.cpp +++ b/projects/gltf/main.cpp @@ -161,10 +161,10 @@ void ProjApp::Config(ppx::ApplicationSettings& settings) { settings.appName = "gltf"; settings.enableImGui = true; - settings.window.width = 1920; - settings.window.height = 1080; settings.grfx.api = kApi; settings.grfx.swapchain.depthFormat = grfx::FORMAT_D32_FLOAT; + + settings.standardKnobsDefaultValue.resolution = std::make_pair(1920, 1080); } void ProjApp::LoadTexture( diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 51c45bc60..6c6bd522c 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -44,20 +44,16 @@ Application::Application() { InternalCtor(); - mSettings.appName = kDefaultAppName; - mSettings.window.width = kDefaultWindowWidth; - mSettings.window.height = kDefaultWindowHeight; - mSettings.window.title = kDefaultAppName; + mSettings.appName = kDefaultAppName; + mSettings.window.title = kDefaultAppName; } Application::Application(uint32_t windowWidth, uint32_t windowHeight, const char* windowTitle) { InternalCtor(); - mSettings.appName = windowTitle; - mSettings.window.width = windowWidth; - mSettings.window.height = windowHeight; - mSettings.window.title = windowTitle; + mSettings.appName = windowTitle; + mSettings.window.title = windowTitle; } Application::~Application() @@ -226,8 +222,8 @@ Result Application::CreateSwapchains() grfx::SwapchainCreateInfo ci = {}; ci.pQueue = mDevice->GetGraphicsQueue(); ci.pSurface = nullptr; - ci.width = mSettings.window.width; - ci.height = mSettings.window.height; + ci.width = mWindow->Size().width; + ci.height = mWindow->Size().height; ci.colorFormat = mXrComponent.GetColorFormat(); ci.depthFormat = mXrComponent.GetDepthFormat(); ci.imageCount = 0; // This will be derived from XrSwapchain. @@ -263,10 +259,7 @@ Result Application::CreateSwapchains() || (mSettings.xr.enable && mSettings.xr.enableDebugCapture)) #endif { - PPX_LOG_INFO("Creating application swapchain"); - PPX_LOG_INFO(" resolution : " << mSettings.window.width << "x" << mSettings.window.height); - PPX_LOG_INFO(" image count : " << mSettings.grfx.swapchain.imageCount); - + std::pair swapchainSize = {mWindow->Size().width, mWindow->Size().height}; if (mSurface) { const uint32_t surfaceMinImageCount = mSurface->GetMinImageCount(); if (mSettings.grfx.swapchain.imageCount < surfaceMinImageCount) { @@ -285,29 +278,34 @@ Result Application::CreateSwapchains() // const uint32_t surfaceMaxImageWidth = mSurface->GetMaxImageWidth(); const uint32_t surfaceMaxImageHeight = mSurface->GetMaxImageHeight(); - if ((mSettings.window.width > surfaceMaxImageWidth) || (mSettings.window.height > surfaceMaxImageHeight)) { - PPX_LOG_WARN("readjusting swapchain/window size from " << mSettings.window.width << "x" << mSettings.window.height << " to " << surfaceMaxImageWidth << "x" << surfaceMaxImageHeight << " to match surface requirements"); - mSettings.window.width = std::min(mSettings.window.width, surfaceMaxImageWidth); - mSettings.window.height = std::min(mSettings.window.height, surfaceMaxImageHeight); + if ((swapchainSize.first > surfaceMaxImageWidth) || (swapchainSize.second > surfaceMaxImageHeight)) { + PPX_LOG_WARN("readjusting swapchain/window size from " << swapchainSize.first << "x" << swapchainSize.second << " to " << surfaceMaxImageWidth << "x" << surfaceMaxImageHeight << " to match surface requirements"); + swapchainSize = { + std::min(swapchainSize.first, surfaceMaxImageWidth), + std::min(swapchainSize.second, surfaceMaxImageHeight)}; } const uint32_t surfaceCurrentImageWidth = mSurface->GetCurrentImageWidth(); const uint32_t surfaceCurrentImageHeight = mSurface->GetCurrentImageHeight(); if ((surfaceCurrentImageWidth != grfx::Surface::kInvalidExtent) && (surfaceCurrentImageHeight != grfx::Surface::kInvalidExtent)) { - if ((mSettings.window.width != surfaceCurrentImageWidth) || - (mSettings.window.height != surfaceCurrentImageHeight)) { - PPX_LOG_WARN("window size " << mSettings.window.width << "x" << mSettings.window.height << " does not match current surface extent " << surfaceCurrentImageWidth << "x" << surfaceCurrentImageHeight); + if ((swapchainSize.first != surfaceCurrentImageWidth) || + (swapchainSize.second != surfaceCurrentImageHeight)) { + PPX_LOG_WARN("window size " << swapchainSize.first << "x" << swapchainSize.second << " does not match current surface extent " << surfaceCurrentImageWidth << "x" << surfaceCurrentImageHeight); } PPX_LOG_WARN("surface current extent " << surfaceCurrentImageWidth << "x" << surfaceCurrentImageHeight); } } + PPX_LOG_INFO("Creating application swapchain"); + PPX_LOG_INFO(" resolution : " << swapchainSize.first << "x" << swapchainSize.second); + PPX_LOG_INFO(" image count : " << mSettings.grfx.swapchain.imageCount); + grfx::SwapchainCreateInfo ci = {}; ci.pQueue = mDevice->GetGraphicsQueue(); ci.pSurface = mSurface; - ci.width = mSettings.window.width; - ci.height = mSettings.window.height; + ci.width = swapchainSize.first; + ci.height = swapchainSize.second; ci.colorFormat = mSettings.grfx.swapchain.colorFormat; ci.depthFormat = mSettings.grfx.swapchain.depthFormat; ci.imageCount = mSettings.grfx.swapchain.imageCount; @@ -322,12 +320,6 @@ Result Application::CreateSwapchains() #if defined(PPX_BUILD_XR) if (mSettings.xr.enable && mSettings.xr.enableDebugCapture) { mDebugCaptureSwapchainIndex = static_cast(mSwapchains.size()); - // The window size could be smaller than the requested one in glfwCreateWindow - // So the final swapchain size for window needs to be adjusted - // In the case of debug capture, we don't care about the window size after creating the dummy window - // restore width and heigh in the settings since they are used by some other systems in the renderer - mSettings.window.width = mXrComponent.GetWidth(); - mSettings.window.height = mXrComponent.GetHeight(); } #endif mSwapchains.push_back(swapchain); @@ -421,22 +413,11 @@ Result Application::CreatePlatformWindow() { // Decorated window title std::stringstream windowTitle; - windowTitle << mSettings.window.title << " | " << ToString(mSettings.grfx.api) << " | " << mDevice->GetDeviceName() << " | " << Platform::GetPlatformString(); - - { - Result ppxres = mWindow->Create(windowTitle.str().c_str()); - if (ppxres != ppx::SUCCESS) { - return ppxres; - } - } + windowTitle << mSettings.window.title << " | " << ToString(mSettings.grfx.api) + << " | " << mDevice->GetDeviceName() + << " | " << Platform::GetPlatformString(); - // Update window size to the actual size. - if (!IsXrEnabled()) { - auto windowSize = mWindow->Size(); - mSettings.window.width = windowSize.width; - mSettings.window.height = windowSize.height; - } - return ppx::SUCCESS; + return mWindow->Create(windowTitle.str().c_str()); } void Application::DestroyPlatformWindow() @@ -786,55 +767,50 @@ void Application::MoveCallback(int32_t x, int32_t y) void Application::ResizeCallback(uint32_t width, uint32_t height) { - bool widthChanged = (width != mSettings.window.width); - bool heightChanged = (height != mSettings.window.height); - if (widthChanged || heightChanged) { - // Update the configuration's width and height - mSettings.window.width = width; - mSettings.window.height = height; - mWindowSurfaceInvalid = ((width == 0) || (height == 0)); - - // Vulkan will return an error if either dimension is 0 - if (!mWindowSurfaceInvalid) { - // D3D12 swapchain needs resizing - if ((mDevice->GetApi() == grfx::API_DX_12_0) || (mDevice->GetApi() == grfx::API_DX_12_1)) { - // Wait for device to idle - mDevice->WaitIdle(); - - PPX_ASSERT_MSG((mSwapchains.size() == 1), "Invalid number of swapchains for D3D12"); - - auto ppxres = mSwapchains[0]->Resize(mSettings.window.width, mSettings.window.height); - if (Failed(ppxres)) { - PPX_ASSERT_MSG(false, "D3D12 swapchain resize failed"); - // Signal the app to quit if swapchain recreation fails - mWindow->Quit(); - } + mWindowSurfaceInvalid = ((width == 0) || (height == 0)); + // Vulkan will return an error if either dimension is 0 + if (mWindowSurfaceInvalid) { + DispatchResize(width, height); + return; + } + + // D3D12 swapchain needs resizing + if ((mDevice->GetApi() == grfx::API_DX_12_0) || (mDevice->GetApi() == grfx::API_DX_12_1)) { + // Wait for device to idle + mDevice->WaitIdle(); + + PPX_ASSERT_MSG((mSwapchains.size() == 1), "Invalid number of swapchains for D3D12"); + + auto ppxres = mSwapchains[0]->Resize(width, height); + if (Failed(ppxres)) { + PPX_ASSERT_MSG(false, "D3D12 swapchain resize failed"); + // Signal the app to quit if swapchain recreation fails + mWindow->Quit(); + } #if defined(PPX_MSW) - mForceInvalidateClientArea = true; + mForceInvalidateClientArea = true; #endif - PPX_LOG_INFO("Resized application swapchain"); - PPX_LOG_INFO(" resolution : " << mSettings.window.width << "x" << mSettings.window.height); - PPX_LOG_INFO(" image count : " << mSettings.grfx.swapchain.imageCount); - } - // Vulkan swapchain needs recreation - else { - // This function will wait for device to idle - DestroySwapchains(); - - auto ppxres = CreateSwapchains(); - if (Failed(ppxres)) { - PPX_ASSERT_MSG(false, "Vulkan swapchain recreate failed"); - // Signal the app to quit if swapchain recreation fails - mWindow->Quit(); - } - } - } + PPX_LOG_INFO("Resized application swapchain"); + PPX_LOG_INFO(" resolution : " << width << "x" << height); + PPX_LOG_INFO(" image count : " << mSettings.grfx.swapchain.imageCount); + } + // Vulkan swapchain needs recreation + else { + // This function will wait for device to idle + DestroySwapchains(); - // Dispatch resize event - DispatchResize(mSettings.window.width, mSettings.window.height); + auto ppxres = CreateSwapchains(); + if (Failed(ppxres)) { + PPX_ASSERT_MSG(false, "Vulkan swapchain recreate failed"); + // Signal the app to quit if swapchain recreation fails + mWindow->Quit(); + } } + + // Dispatch resize event + DispatchResize(width, height); } void Application::WindowIconifyCallback(bool iconified) @@ -929,16 +905,9 @@ void Application::UpdateStandardSettings() { mSettings.headless = mStandardOpts.pHeadless->GetValue(); - // If command line argument provided width and height - auto resolution = mStandardOpts.pResolution->GetValue(); - const bool hasResolutionFlag = (resolution.first > 0 && resolution.second > 0); - if (hasResolutionFlag) { - mSettings.window.width = resolution.first; - mSettings.window.height = resolution.second; - } - #if defined(PPX_BUILD_XR) - resolution = mStandardOpts.pXrUiResolution->GetValue(); + auto resolution = mStandardOpts.pResolution->GetValue(); + resolution = mStandardOpts.pXrUiResolution->GetValue(); if (resolution.first > 0 && resolution.second > 0) { mSettings.xr.uiWidth = resolution.first; mSettings.xr.uiHeight = resolution.second; @@ -983,15 +952,11 @@ void Application::InitializeXRComponentBeforeGrfxDeviceInit() createInfo.enableDebug = mSettings.grfx.enableDebug; createInfo.enableQuadLayer = mSettings.enableImGui; createInfo.enableDepthSwapchain = mSettings.xr.enableDepthSwapchain; - const auto resolution = mStandardOpts.pResolution->GetValue(); - const bool hasResolutionFlag = (resolution.first > 0 && resolution.second > 0); - if (hasResolutionFlag) { - createInfo.resolution.width = mSettings.window.width; - createInfo.resolution.height = mSettings.window.height; - } - createInfo.uiResolution.width = mSettings.xr.uiWidth; - createInfo.uiResolution.height = mSettings.xr.uiHeight; - createInfo.requiredExtensions = mStandardOpts.pXrRequiredExtensions->GetValue(); + createInfo.resolution.width = GetWindowWidth(); + createInfo.resolution.height = GetWindowHeight(); + createInfo.uiResolution.width = mSettings.xr.uiWidth; + createInfo.uiResolution.height = mSettings.xr.uiHeight; + createInfo.requiredExtensions = mStandardOpts.pXrRequiredExtensions->GetValue(); mXrComponent.InitializeBeforeGrfxDeviceInit(createInfo); } @@ -1001,8 +966,6 @@ void Application::InitializeXRComponentAndUpdateSettingsAfterGrfxDeviceInit() { if (mSettings.xr.enable) { mXrComponent.InitializeAfterGrfxDeviceInit(mInstance); - mSettings.window.width = mXrComponent.GetWidth(); - mSettings.window.height = mXrComponent.GetHeight(); } } @@ -1322,10 +1285,6 @@ int Application::Run(int argc, char** argv) return EXIT_FAILURE; } - if (!mSettings.xr.enable) { - mWindow->Resize({mSettings.window.width, mSettings.window.height}); - } - // Setup ImGui if (mSettings.enableImGui) { ppxres = InitializeImGui(); @@ -1406,17 +1365,17 @@ bool Application::IsWindowMaximized() const uint32_t Application::GetUIWidth() const { #if defined(PPX_BUILD_XR) - return (mSettings.xr.enable && mSettings.xr.uiWidth > 0) ? mSettings.xr.uiWidth : mSettings.window.width; + return (mSettings.xr.enable && mSettings.xr.uiWidth > 0) ? mSettings.xr.uiWidth : GetWindowWidth(); #else - return mSettings.window.width; + return GetWindowWidth(); #endif } uint32_t Application::GetUIHeight() const { #if defined(PPX_BUILD_XR) - return (mSettings.xr.enable && mSettings.xr.uiHeight > 0) ? mSettings.xr.uiHeight : mSettings.window.height; + return (mSettings.xr.enable && mSettings.xr.uiHeight > 0) ? mSettings.xr.uiHeight : GetWindowHeight(); #else - return mSettings.window.height; + return GetWindowHeight(); #endif } diff --git a/src/ppx/window.cpp b/src/ppx/window.cpp index 32ff22bf7..86a507d6d 100644 --- a/src/ppx/window.cpp +++ b/src/ppx/window.cpp @@ -49,7 +49,7 @@ Window::Window(Application* pApp) WindowSize Window::Size() const { - return {mApp->GetSettings()->window.width, mApp->GetSettings()->window.height}; + return App()->GetStandardOptions().pResolution->GetValue(); } } // namespace ppx diff --git a/src/ppx/window_android.cpp b/src/ppx/window_android.cpp index a7094d2e9..88563b799 100644 --- a/src/ppx/window_android.cpp +++ b/src/ppx/window_android.cpp @@ -130,10 +130,12 @@ WindowSize WindowImplAndroid::Size() const // xrComponent is taking care of window size on XR builds. return Window::Size(); } + if (mSize.width == 0 || mSize.height == 0) { // Return a default size if the window has not been initialized. return Window::Size(); } + return mSize; } diff --git a/src/ppx/window_glfw.cpp b/src/ppx/window_glfw.cpp index 8625800a4..b652ad68c 100644 --- a/src/ppx/window_glfw.cpp +++ b/src/ppx/window_glfw.cpp @@ -416,8 +416,8 @@ Result WindowImplGLFW::Create(const char* title) glfwWindowHint(GLFW_RESIZABLE, settings->window.resizable ? GLFW_TRUE : GLFW_FALSE); GLFWwindow* pWindow = glfwCreateWindow( - static_cast(settings->window.width), - static_cast(settings->window.height), + static_cast(App()->GetStandardOptions().pResolution->GetValue().first), + static_cast(App()->GetStandardOptions().pResolution->GetValue().second), title, nullptr, nullptr); @@ -452,6 +452,9 @@ Result WindowImplGLFW::Destroy() bool WindowImplGLFW::IsRunning() const { + if (IsNull(mNative)) + return false; + int value = glfwWindowShouldClose(mNative); bool isRunning = (value == 0); return isRunning; @@ -472,6 +475,10 @@ Result WindowImplGLFW::Resize(const WindowSize& size) WindowSize WindowImplGLFW::Size() const { + if (IsNull(mNative)) { + return App()->GetStandardOptions().pResolution->GetValue(); + } + int width = 0; int height = 0; glfwGetWindowSize(mNative, &width, &height); From 3fab909dce219e4d55be98ea0f14cfc7a2ed345d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Tue, 27 Feb 2024 16:58:58 +0100 Subject: [PATCH 2/6] pr review --- include/ppx/application.h | 2 +- src/ppx/application.cpp | 11 +++++------ src/ppx/window_glfw.cpp | 3 ++- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/ppx/application.h b/include/ppx/application.h index bb98782c9..fb0b622ec 100644 --- a/include/ppx/application.h +++ b/include/ppx/application.h @@ -217,7 +217,7 @@ class Application { public: Application(); - Application(uint32_t windowWidth, uint32_t windowHeight, const char* windowTitle); + Application(const char* windowTitle); virtual ~Application(); static Application* Get(); diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 6c6bd522c..5463ab29b 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -48,7 +48,7 @@ Application::Application() mSettings.window.title = kDefaultAppName; } -Application::Application(uint32_t windowWidth, uint32_t windowHeight, const char* windowTitle) +Application::Application(const char* windowTitle) { InternalCtor(); @@ -222,8 +222,8 @@ Result Application::CreateSwapchains() grfx::SwapchainCreateInfo ci = {}; ci.pQueue = mDevice->GetGraphicsQueue(); ci.pSurface = nullptr; - ci.width = mWindow->Size().width; - ci.height = mWindow->Size().height; + ci.width = GetWindowWidth(); + ci.height = GetWindowHeight(); ci.colorFormat = mXrComponent.GetColorFormat(); ci.depthFormat = mXrComponent.GetDepthFormat(); ci.imageCount = 0; // This will be derived from XrSwapchain. @@ -259,7 +259,7 @@ Result Application::CreateSwapchains() || (mSettings.xr.enable && mSettings.xr.enableDebugCapture)) #endif { - std::pair swapchainSize = {mWindow->Size().width, mWindow->Size().height}; + std::pair swapchainSize = {GetWindowWidth(), GetWindowHeight()}; if (mSurface) { const uint32_t surfaceMinImageCount = mSurface->GetMinImageCount(); if (mSettings.grfx.swapchain.imageCount < surfaceMinImageCount) { @@ -906,8 +906,7 @@ void Application::UpdateStandardSettings() mSettings.headless = mStandardOpts.pHeadless->GetValue(); #if defined(PPX_BUILD_XR) - auto resolution = mStandardOpts.pResolution->GetValue(); - resolution = mStandardOpts.pXrUiResolution->GetValue(); + auto resolution = mStandardOpts.pXrUiResolution->GetValue(); if (resolution.first > 0 && resolution.second > 0) { mSettings.xr.uiWidth = resolution.first; mSettings.xr.uiHeight = resolution.second; diff --git a/src/ppx/window_glfw.cpp b/src/ppx/window_glfw.cpp index b652ad68c..3edca3c2f 100644 --- a/src/ppx/window_glfw.cpp +++ b/src/ppx/window_glfw.cpp @@ -452,8 +452,9 @@ Result WindowImplGLFW::Destroy() bool WindowImplGLFW::IsRunning() const { - if (IsNull(mNative)) + if (IsNull(mNative)) { return false; + } int value = glfwWindowShouldClose(mNative); bool isRunning = (value == 0); From 114434df6e78640f9f41380f2fe75a2a253268e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Wed, 17 Apr 2024 17:31:59 +0200 Subject: [PATCH 3/6] pr feedback --- include/ppx/command_line_parser.h | 6 ++++++ include/ppx/knob.h | 9 +++++++-- include/ppx/xr_component.h | 10 +++++----- src/ppx/application.cpp | 5 +++-- 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/include/ppx/command_line_parser.h b/include/ppx/command_line_parser.h index b13f6f244..595597510 100644 --- a/include/ppx/command_line_parser.h +++ b/include/ppx/command_line_parser.h @@ -55,6 +55,12 @@ class CliOptions // not counting multiple appearances of the same flag such as: --assets-path a --assets-path b size_t GetNumUniqueOptions() const { return mAllOptions.size(); } + // Returns true if the given option is present in the commandline. False otherwise. + bool HasOption(const std::string& optionName) const + { + return mAllOptions.cend() != mAllOptions.find(optionName); + } + // Tries to parse the option string into the type of the default value and return it. // If the value fails to be converted, return the specified default value. // Warning: If this is called instead of the vector overload for multiple-value flags, diff --git a/include/ppx/knob.h b/include/ppx/knob.h index 31382266a..4222ef2e6 100644 --- a/include/ppx/knob.h +++ b/include/ppx/knob.h @@ -376,13 +376,13 @@ class KnobFlag final { public: KnobFlag(const std::string& flagName, T defaultValue) - : Knob(flagName, false) + : Knob(flagName, false), mDirty(false) { SetValue(defaultValue); } KnobFlag(const std::string& flagName, T defaultValue, T minValue, T maxValue) - : Knob(flagName, false) + : Knob(flagName, false), mDirty(false) { static_assert(std::is_arithmetic_v, "KnobFlag can only be defined with min/max when it's of arithmetic type"); PPX_ASSERT_MSG(minValue < maxValue, "invalid range to initialize KnobFlag"); @@ -400,6 +400,8 @@ class KnobFlag final T GetValue() const { return mValue; } + bool IsDefaultValue() const { return mDirty; } + void SetValidator(std::function validatorFunc) { mValidatorFunc = validatorFunc; } private: @@ -418,6 +420,8 @@ class KnobFlag final void UpdateFromFlags(const CliOptions& opts) override { + if (opts.HasOption(mFlagName)) + mDirty = true; SetValue(opts.GetOptionValueOrDefault(mFlagName, mValue)); } @@ -438,6 +442,7 @@ class KnobFlag final private: T mValue; std::function mValidatorFunc; + bool mDirty; }; // KnobManager holds the knobs in an application diff --git a/include/ppx/xr_component.h b/include/ppx/xr_component.h index 261a5a37e..5a03ffa1f 100644 --- a/include/ppx/xr_component.h +++ b/include/ppx/xr_component.h @@ -133,7 +133,7 @@ struct XrComponentCreateInfo bool enableDebug = false; bool enableQuadLayer = false; bool enableDepthSwapchain = false; - XrComponentResolution resolution = {0, 0}; + std::optional resolution = (XrComponentResolution){0, 0}; XrComponentResolution uiResolution = {0, 0}; std::vector requiredExtensions = {}; @@ -174,16 +174,16 @@ class XrComponent { if (mConfigViews.empty()) return 0; - if (mCreateInfo.resolution.width > 0) - return mCreateInfo.resolution.width; + if (mCreateInfo.resolution.has_value()) + return mCreateInfo.resolution->width; return mConfigViews[0].recommendedImageRectWidth; } uint32_t GetHeight() const { if (mConfigViews.empty()) return 0; - if (mCreateInfo.resolution.height > 0) - return mCreateInfo.resolution.height; + if (mCreateInfo.resolution.has_value()) + return mCreateInfo.resolution->height; return mConfigViews[0].recommendedImageRectHeight; } uint32_t GetUIWidth() const diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 5463ab29b..9197c6937 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -951,8 +951,9 @@ void Application::InitializeXRComponentBeforeGrfxDeviceInit() createInfo.enableDebug = mSettings.grfx.enableDebug; createInfo.enableQuadLayer = mSettings.enableImGui; createInfo.enableDepthSwapchain = mSettings.xr.enableDepthSwapchain; - createInfo.resolution.width = GetWindowWidth(); - createInfo.resolution.height = GetWindowHeight(); + if (!mStandardOpts.pResolution->IsDefaultValue()) { + createInfo.resolution = (XrComponentResolution){GetWindowWidth(), GetWindowHeight()}; + } createInfo.uiResolution.width = mSettings.xr.uiWidth; createInfo.uiResolution.height = mSettings.xr.uiHeight; createInfo.requiredExtensions = mStandardOpts.pXrRequiredExtensions->GetValue(); From 35181b0a1916b56cb8ef44ed7315a143c1dd8f53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Thu, 18 Apr 2024 18:29:35 +0200 Subject: [PATCH 4/6] mDirty flip --- include/ppx/knob.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/ppx/knob.h b/include/ppx/knob.h index 4222ef2e6..65216577d 100644 --- a/include/ppx/knob.h +++ b/include/ppx/knob.h @@ -400,7 +400,7 @@ class KnobFlag final T GetValue() const { return mValue; } - bool IsDefaultValue() const { return mDirty; } + bool IsDefaultValue() const { return !mDirty; } void SetValidator(std::function validatorFunc) { mValidatorFunc = validatorFunc; } From a8067874251891e9a80d99e1555bb690783dcdaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Thu, 18 Apr 2024 18:36:15 +0200 Subject: [PATCH 5/6] clang-format --- src/ppx/application.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 9197c6937..95d89239a 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -954,9 +954,9 @@ void Application::InitializeXRComponentBeforeGrfxDeviceInit() if (!mStandardOpts.pResolution->IsDefaultValue()) { createInfo.resolution = (XrComponentResolution){GetWindowWidth(), GetWindowHeight()}; } - createInfo.uiResolution.width = mSettings.xr.uiWidth; - createInfo.uiResolution.height = mSettings.xr.uiHeight; - createInfo.requiredExtensions = mStandardOpts.pXrRequiredExtensions->GetValue(); + createInfo.uiResolution.width = mSettings.xr.uiWidth; + createInfo.uiResolution.height = mSettings.xr.uiHeight; + createInfo.requiredExtensions = mStandardOpts.pXrRequiredExtensions->GetValue(); mXrComponent.InitializeBeforeGrfxDeviceInit(createInfo); } From 76cde130876b77b05512f8e116cf69d20b202211 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Thu, 18 Apr 2024 18:38:35 +0200 Subject: [PATCH 6/6] clang-format --- include/ppx/xr_component.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/include/ppx/xr_component.h b/include/ppx/xr_component.h index 5a03ffa1f..4bb87916d 100644 --- a/include/ppx/xr_component.h +++ b/include/ppx/xr_component.h @@ -127,14 +127,14 @@ struct XrComponentCreateInfo #else grfx::Format colorFormat = grfx::FORMAT_B8G8R8A8_SRGB; #endif - grfx::Format depthFormat = grfx::FORMAT_D32_FLOAT; - XrRefSpace refSpaceType = XrRefSpace::XR_STAGE; - XrViewConfigurationType viewConfigType = XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO; - bool enableDebug = false; - bool enableQuadLayer = false; - bool enableDepthSwapchain = false; + grfx::Format depthFormat = grfx::FORMAT_D32_FLOAT; + XrRefSpace refSpaceType = XrRefSpace::XR_STAGE; + XrViewConfigurationType viewConfigType = XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO; + bool enableDebug = false; + bool enableQuadLayer = false; + bool enableDepthSwapchain = false; std::optional resolution = (XrComponentResolution){0, 0}; - XrComponentResolution uiResolution = {0, 0}; + XrComponentResolution uiResolution = {0, 0}; std::vector requiredExtensions = {}; };