Launcher UI by 0dm · Pull Request #257 · P0k3rf4ce/SauceEngine

0dm · 2026-04-05T20:23:43Z

i hardly know er

Copilot

Pull request overview

This PR introduces a new in-engine “Launcher” experience that lets developers select local scenes/IBLs or browse/download Poly Haven assets (with caching) before entering the runtime, plus supporting CLI options and dependencies.

Changes:

Add Poly Haven client for asset listing, download, and cache management (curl + nlohmann/json).
Add launcher catalog discovery + command-line preview builder.
Integrate the launcher UI and remote asset selection flow into SauceEngineApp, and extend CLI options to support it.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`vcpkg.json`	Adds `curl` dependency for Poly Haven network requests.
`src/main.cpp`	Enables launcher by default when no args; wires Poly Haven selections into app startup.
`src/launcher/CMakeLists.txt`	Links `CURL::libcurl` and `nlohmann_json` into `launcherLib`.
`src/launcher/optionParser.cpp`	Adds CLI flags for skipping launcher + Poly Haven asset selection.
`include/launcher/optionParser.hpp`	Extends `AppOptions` with Poly Haven options and launcher skip flag.
`src/launcher/PolyHavenClient.cpp`	Implements Poly Haven HTTP fetch/download + cache inspection/mutation.
`include/launcher/PolyHavenClient.hpp`	Declares Poly Haven client API and data structures.
`src/launcher/LauncherCatalog.cpp`	Discovers local assets/IBLs and builds a CLI preview string.
`include/launcher/LauncherCatalog.hpp`	Declares catalog discovery + command preview API.
`src/app/SauceEngineApp.cpp`	Adds the launcher UI, background tasks, cache UI, and runtime handoff logic.
`include/app/SauceEngineApp.hpp`	Adds launcher state, async task plumbing, and Poly Haven selection setters.
`include/app/Camera.hpp`	Adds `setScreenSize()` for updating projection after resolution changes.
`CMakeLists.txt`	Minor cleanup (trailing whitespace removal).

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

Copilot · 2026-04-05T20:28:33Z

+      for (int i = 0; i < static_cast<int>(launcherState.catalog.launchTargets.size()); ++i) {
+        const auto& entry = launcherState.catalog.launchTargets[static_cast<size_t>(i)];
+        const bool selected = launcherState.selectedLaunchTarget == i && polyHavenModelId.empty();
+        if (ImGui::Selectable((entry.label + "##launch-target").c_str(), selected)) {


ImGui IDs collide here because every row uses the same ID suffix ("##launch-target"). In ImGui, the part after "##" is the widget ID, so all entries share an ID which can break selection/hover and cause only one item to be interactable. Make the ID unique per entry (e.g., include the index or a stable unique key like entry.path).

Suggested change

if (ImGui::Selectable((entry.label + "##launch-target").c_str(), selected)) {

const std::string launchTargetLabel = entry.label + "##launch-target-" + entry.path;

if (ImGui::Selectable(launchTargetLabel.c_str(), selected)) {

Copilot · 2026-04-05T20:28:33Z

+      for (int i = 0; i < static_cast<int>(launcherState.catalog.iblMaps.size()); ++i) {
+        const auto& entry = launcherState.catalog.iblMaps[static_cast<size_t>(i)];
+        const bool selected = launcherState.selectedIblMap == i && polyHavenHdriId.empty();
+        if (ImGui::Selectable((entry.label + "##ibl-target").c_str(), selected)) {


ImGui IDs collide here because every row uses the same ID suffix ("##ibl-target"). Since the string after "##" determines the widget ID, all entries end up with the same ID which can break selection and input handling. Include a unique suffix (index or entry.path) in the ID to avoid collisions.

Suggested change

if (ImGui::Selectable((entry.label + "##ibl-target").c_str(), selected)) {

if (ImGui::Selectable((entry.label + "##ibl-target-" + std::to_string(i)).c_str(), selected)) {

Copilot · 2026-04-05T20:28:33Z

+  auto startRemoteLoadIfNeeded = [&](RemoteBrowserState& browserState, sauce::launcher::PolyHavenAssetType type) {
+    if (!browserState.assets.empty() || browserState.pendingLoad.has_value() || !browserState.statusMessage.empty()) {
+      return;
    }

-    float xoffset = xpos - app->lastX;
-    float yoffset = app->lastY - ypos;
+    browserState.statusMessage = kPolyHavenLoadingMessage;
+    browserState.pendingLoad = std::async(std::launch::async, [type]() {
+      return sauce::launcher::fetchPolyHavenAssets(type);
+    });


These background tasks are launched via std::async(std::launch::async) and stored in std::future. If the app is closed while a future is still valid, destruction of the future can block until the async task finishes, which can make quitting the launcher hang during network fetch/download. Consider switching to an explicit worker thread (e.g., std::jthread) or a task system that supports cancellation, or ensure pending futures are completed/cancelled on shutdown without blocking the UI thread.

Copilot · 2026-04-05T20:28:34Z

+  if (recursive) {
+    for (const auto& entry : fs::recursive_directory_iterator(directory)) {
+      pushEntry(entry);
+    }
+  } else {
+    for (const auto& entry : fs::directory_iterator(directory)) {
+      pushEntry(entry);
+    }
+  }


appendMatches uses std::filesystem::recursive_directory_iterator / directory_iterator without an error_code or directory_options, so it can throw (e.g., permission denied, broken symlink) and crash the launcher while scanning. Prefer the iterator overloads that take std::error_code and/or use directory_options::skip_permission_denied, and handle errors gracefully.

Copilot · 2026-04-05T20:28:34Z

+  }
+
+  curl_easy_cleanup(curl);
+  fs::rename(tempPath, destination);


downloadFile renames the temporary .part file onto the destination with std::filesystem::rename. On Windows (and on some platforms/configs), rename fails if the destination already exists (e.g., a previous 0-byte/partial file or an interrupted run), which will make downloads fail even though retry should succeed. Consider deleting an existing destination before renaming, or use a replace/overwrite strategy that is cross-platform safe.

Suggested change

fs::rename(tempPath, destination);

std::error_code removeError;

fs::remove(destination, removeError);

if (removeError) {

fs::remove(tempPath);

throw std::runtime_error(

"Failed to replace existing download target '" + destination.string() +

"': " + removeError.message());

}

std::error_code renameError;

fs::rename(tempPath, destination, renameError);

if (renameError) {

fs::remove(tempPath);

throw std::runtime_error(

"Failed to finalize download to '" + destination.string() +

"': " + renameError.message());

}

Copilot · 2026-04-05T20:28:34Z

+std::filesystem::path getPolyHavenHdriCachePath(const std::string& id, const std::string& resolutionLabel) {
+  return getPolyHavenCacheRoot() / "hdris" / id / resolutionLabel / (id + "_" + resolutionLabel + ".hdr");
+}
+
+std::filesystem::path getPolyHavenModelCachePath(const std::string& id, const std::string& resolutionLabel) {
+  return getPolyHavenCacheRoot() / "models" / id / resolutionLabel / (id + "_" + resolutionLabel + ".gltf");
+}


Cache path helpers build deterministic filenames (e.g., id_resolution.hdr / .gltf), but the downloader saves the primary files using the URL basename (pathBasenameFromUrl). This mismatch means cached assets may not be detected via getPolyHaven*CachePath(), and command previews / subsequent runs may point at a file that was never written. Align these by either downloading/renaming the primary file to match the helper path, or updating the helper logic to match the actual downloaded filenames.

Copilot · 2026-04-05T20:28:34Z

 AppOptions::AppOptions(int argc, char const **argv): desc("Allowed options") {
    namespace po = boost::program_options;

    desc.add_options()
    ("help", "produce help message")
-    ("skip-launcher", "start the engine immediately")
+    ("skip-launcher", po::bool_switch(&(this->skip_launcher)), "start the engine immediately")
    ("width,w", po::value<unsigned int>(&(this->scr_width))->default_value(DEFAULT_SCR_WIDTH), "screen width")
    ("height,h", po::value<unsigned int>(&(this->scr_height))->default_value(DEFAULT_SCR_HEIGHT), "screen height")


AppOptions(int argc, ...) relies on boost::program_options to populate members, but the constructor doesn't initialize scr_width/scr_height/tickrate/scene_file/ibl_file/polyhaven_* (or skip_launcher) before po::notify(). Keeping these members uninitialized is brittle (e.g., if parsing throws before notify, or if future changes read fields earlier). Prefer initializing members in the ctor initializer list (matching the default ctor) so AppOptions is always in a valid state.

… alignment

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

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

Copilot · 2026-04-05T22:14:08Z

    ("tickrate,t", po::value<double>(&(this->tickrate))->default_value(DEFAULT_TICKRATE), "animation tickrate")
+    ("model-rotate-x", po::value<double>(&(this->model_rotate_x_degrees)), "post-import model rotation around world X in degrees")
+    ("model-rotate-y", po::value<double>(&(this->model_rotate_y_degrees)), "post-import model rotation around world Y in degrees")
+    ("model-rotate-z", po::value<double>(&(this->model_rotate_z_degrees)), "post-import model rotation around world Z in degrees")


AppOptions::AppOptions(int argc, ...) doesn't initialize model_rotate_{x,y,z}_degrees, and the program_options entries for --model-rotate-* don't provide default_value(...). If the user doesn't pass these flags, the members remain uninitialized (UB) and are later forwarded into setModelRotationDegrees(). Initialize these members (e.g., in the ctor initializer list, or add ->default_value(DEFAULT_MODEL_ROTATE_*_DEGREES) to the options).

Copilot · 2026-04-05T22:14:08Z

+target_link_libraries(launcherLib PRIVATE imgui::imgui)
+
+find_package(CURL REQUIRED)
+target_link_libraries(launcherLib PRIVATE CURL::libcurl)
+
+find_package(nlohmann_json CONFIG REQUIRED)
+target_link_libraries(launcherLib PRIVATE nlohmann_json::nlohmann_json)


launcherLib is now a STATIC library but links libcurl/imgui/nlohmann_json as PRIVATE. For static libs, those link dependencies typically won't propagate to consumers, and the top-level executable doesn't link CURL at all, so builds will fail with unresolved curl symbols from PolyHavenClient. Make required deps PUBLIC (or INTERFACE) on launcherLib, or link CURL::libcurl (and any other required deps) in the consuming targets.

Suggested change

target_link_libraries(launcherLib PRIVATE imgui::imgui)

find_package(CURL REQUIRED)

target_link_libraries(launcherLib PRIVATE CURL::libcurl)

find_package(nlohmann_json CONFIG REQUIRED)

target_link_libraries(launcherLib PRIVATE nlohmann_json::nlohmann_json)

target_link_libraries(launcherLib PUBLIC imgui::imgui)

find_package(CURL REQUIRED)

target_link_libraries(launcherLib PUBLIC CURL::libcurl)

find_package(nlohmann_json CONFIG REQUIRED)

target_link_libraries(launcherLib PUBLIC nlohmann_json::nlohmann_json)

Copilot · 2026-04-05T22:14:08Z


  sauce::SauceEngineApp mainApp;
  try {
+    const bool shouldShowLauncher = argc <= 1 && !ops.skip_launcher;


shouldShowLauncher is gated by argc <= 1, which makes --skip-launcher effectively redundant (any invocation with --skip-launcher necessarily has argc > 1, so the launcher is disabled regardless of the flag). If the intent is “show launcher by default unless --skip-launcher”, use !ops.skip_launcher (and optionally other explicit criteria) instead of checking argc.

Suggested change

const bool shouldShowLauncher = argc <= 1 && !ops.skip_launcher;

const bool shouldShowLauncher = !ops.skip_launcher;

Copilot · 2026-04-05T22:14:09Z

+  if (fileNode.contains("include")) {
+    for (const auto& [relativePath, includeNode] : fileNode.at("include").items()) {
+      files.push_back({
+          .url = includeNode.at("url").get<std::string>(),
+          .relativePath = relativePath,
+      });


Remote-provided relativePath values from the Poly Haven JSON (include keys) are concatenated directly into assetRoot / file.relativePath. A malicious/compromised response could use paths like ../... or absolute paths to write outside the cache directory (directory traversal). Sanitize/validate relativePath (reject absolute paths and .. segments; consider lexically_normal() + ensuring the result stays within assetRoot).

Copilot · 2026-04-05T22:14:09Z

+    for (const RemoteFile& file : files) {
+      const fs::path destination = assetRoot / file.relativePath;
+      if (fs::exists(destination) && fs::file_size(destination) > 0) {
+        continue;
+      }
+      downloadFile(file.url, destination);


downloadAssetFiles() builds destination = assetRoot / file.relativePath and then writes to it without constraining the final path. Even if relativePath is expected to be safe, it should be validated at the write site too (e.g., after normalization, ensure destination is within assetRoot) to prevent accidental writes outside the cache if a bad path slips through.

Copilot · 2026-04-05T22:14:09Z

+    if (!resolveConfiguredRemoteAssets(remoteError)) {
+      throw std::runtime_error(remoteError);
    }
+    loadConfiguredScene();


In the non-launcher path, loadConfiguredScene()'s return value is ignored. If the scene file is provided but fails to load, the app will continue into the main loop with an empty/cleared scene and no error surfaced. Consider checking the return value and throwing/returning a descriptive error to match the failure handling used in finalizeLauncherLaunch().

Suggested change

loadConfiguredScene();

const bool sceneLoaded = loadConfiguredScene();

if (!sceneLoaded && !sceneFile.empty()) {

throw std::runtime_error("Failed to load configured scene: " + sceneFile);

}

Copilot · 2026-04-05T22:14:09Z

 SauceEngineApp::~SauceEngineApp() {
-    glfwDestroyWindow(window);
-    glfwTerminate();
+  glfwDestroyWindow(window);
+  glfwTerminate();
 }


~SauceEngineApp() unconditionally calls glfwDestroyWindow(window) and glfwTerminate(). Since window is now initialized to nullptr and exceptions can occur during startup, this destructor can be invoked with window == nullptr (or GLFW not initialized). Guard these calls (e.g., if (window) glfwDestroyWindow(window); and only terminate if init succeeded) to avoid undefined behavior/crashes during error paths.

0dm added 2 commits April 5, 2026 15:27

Add no-args launcher flow on main

d2a889e

launcher

b7b2124

Copilot AI review requested due to automatic review settings April 5, 2026 20:23

Copilot started reviewing on behalf of 0dm April 5, 2026 20:24 View session

Copilot AI reviewed Apr 5, 2026

View reviewed changes

rowel-eshan and others added 2 commits April 5, 2026 16:37

windows fix

cfba7a1

refactor launcher into new class, add rotation default, adjust layout…

f9b6c3a

… alignment

0dm requested a review from Copilot April 5, 2026 22:10

Copilot started reviewing on behalf of 0dm April 5, 2026 22:10 View session

Copilot AI reviewed Apr 5, 2026

View reviewed changes

more relaxed file detection

ffa5bac

	if (ImGui::Selectable((entry.label + "##launch-target").c_str(), selected)) {
	const std::string launchTargetLabel = entry.label + "##launch-target-" + entry.path;
	if (ImGui::Selectable(launchTargetLabel.c_str(), selected)) {

-  fs::rename(tempPath, destination);
+  std::error_code removeError;
+  fs::remove(destination, removeError);
+  if (removeError) {
+    fs::remove(tempPath);
+    throw std::runtime_error(
+        "Failed to replace existing download target '" + destination.string() +
+        "': " + removeError.message());
+  }
+  std::error_code renameError;
+  fs::rename(tempPath, destination, renameError);
+  if (renameError) {
+    fs::remove(tempPath);
+    throw std::runtime_error(
+        "Failed to finalize download to '" + destination.string() +
+        "': " + renameError.message());
+  }

	const bool shouldShowLauncher = argc <= 1 && !ops.skip_launcher;
	const bool shouldShowLauncher = !ops.skip_launcher;

-    loadConfiguredScene();
+    const bool sceneLoaded = loadConfiguredScene();
+    if (!sceneLoaded && !sceneFile.empty()) {
+      throw std::runtime_error("Failed to load configured scene: " + sceneFile);
+    }

Conversation

0dm commented Apr 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants