diff --git a/apps/lightcraft/src/main.rs b/apps/lightcraft/src/main.rs index 4bb1c6ff..f30fed29 100644 --- a/apps/lightcraft/src/main.rs +++ b/apps/lightcraft/src/main.rs @@ -93,10 +93,23 @@ fn gpu_marker_path() -> Option { /// Arm the GPU crash sentinel; if the previous launch left it behind (it died inside the GPU /// driver while creating the device), the notice to show — GPU rendering then starts off. -fn gpu_crash_check() -> Option { - let marker = gpu_marker_path(); - let left = marker.as_deref().and_then(lightcraft_engine::gpu::backend::take_init_marker); - lightcraft_engine::gpu::backend::set_init_marker(marker); +/// +/// A `--memory` session writes nothing (issue #169): it neither arms the sentinel — which would +/// create the settings folder — nor removes a marker it finds. It still honours one, so the GPU +/// starts off there too, and the next ordinary launch reports and clears it. +fn gpu_crash_check(in_memory: bool) -> Option { + gpu_crash_check_at(gpu_marker_path(), in_memory) +} + +fn gpu_crash_check_at(marker: Option, in_memory: bool) -> Option { + use lightcraft_engine::gpu::backend; + let left = if in_memory { + marker.as_deref().and_then(backend::read_init_marker) + } else { + let left = marker.as_deref().and_then(backend::take_init_marker); + backend::set_init_marker(marker); + left + }; left.map(|what| gpu_crash_notice(&what)) } @@ -112,19 +125,28 @@ fn config_dir() -> Option { lightcraft_engine::camera_profiles::config_dir() } +/// `/ui.json`, the saved UI state and app settings — `None` when nothing is read or +/// written: `LIGHTCRAFT_NO_PREFS` (tests, scripts) or no home folder. +fn prefs_path() -> Option { + if std::env::var_os("LIGHTCRAFT_NO_PREFS").is_some() { + return None; + } + config_dir().map(|d| d.join("ui.json")) +} + /// The saved UI state and app settings (`/ui.json`), if any, and a warning for the user /// when the file exists but can't be used (issue #103): a damaged file is kept as /// `ui.json.corrupt-` first, so the next save can't lose the library location in it; /// one that can't be read at all is not written this session (`keep_file`). -fn load_prefs() -> (Option, Option, bool) { - if std::env::var_os("LIGHTCRAFT_NO_PREFS").is_some() { - return (None, None, false); - } - let Some(path) = config_dir().map(|d| d.join("ui.json")) else { return (None, None, false) }; - load_prefs_at(&path) +/// +/// A `--memory` session reads the settings (so it looks like the user's app) but writes nothing +/// (issue #164), so a damaged file is left as it is there, not set aside. +fn load_prefs(in_memory: bool) -> (Option, Option, bool) { + let Some(path) = prefs_path() else { return (None, None, false) }; + load_prefs_at(&path, !in_memory) } -fn load_prefs_at(path: &std::path::Path) -> (Option, Option, bool) { +fn load_prefs_at(path: &std::path::Path, set_aside_damaged: bool) -> (Option, Option, bool) { let bytes = match std::fs::read(path) { Ok(b) => b, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return (None, None, false), @@ -135,6 +157,10 @@ fn load_prefs_at(path: &std::path::Path) -> (Option, Option, bo }; match serde_json::from_slice::(&bytes) { Ok(ui) => (Some(ui.sanitized()), None, false), + Err(e) if !set_aside_damaged => { + let msg = format!("The app settings ({}) are damaged ({e}). Defaults are used; nothing is saved in this session.", path.display()); + (None, Some(msg), true) + } Err(e) => { let secs = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); let keep = path.with_file_name(format!("ui.json.corrupt-{secs}")); @@ -177,8 +203,15 @@ fn write_atomic(path: &std::path::Path, bytes: &[u8]) -> std::io::Result<()> { /// Saves `ui.json` (app settings, incl. the library to open at launch): as soon as the library /// changes, every few seconds when anything else changed, and at exit. +/// +/// Nothing is written while the session is one that promises to save nothing: `--memory` +/// (`path` is `None`; issue #164), or the one running because the library couldn't be opened — the +/// question window and Continue Without Saving (issue #100; `app.openLibrary` ends it, and the +/// library it opened is saved then). #[derive(Default)] struct PrefsWriter { + /// Where to write; `None` writes nothing (`--memory`, `LIGHTCRAFT_NO_PREFS`, no home folder). + path: Option, written: Vec, library: String, checked: f64, @@ -190,19 +223,26 @@ struct PrefsWriter { impl PrefsWriter { fn save(&mut self, app: &LightcraftApp) -> Result<(), String> { - if self.keep_file || std::env::var_os("LIGHTCRAFT_NO_PREFS").is_some() { + self.save_ui(&app.ui, app.library_problem.is_some()) + } + + /// Write `ui` if it changed since the last write; `temporary` (the library couldn't be + /// opened) saves nothing. + fn save_ui(&mut self, ui: &UiState, temporary: bool) -> Result<(), String> { + if self.keep_file || temporary { return Ok(()); } - let Some(d) = config_dir() else { return Ok(()) }; - let bytes = serde_json::to_vec_pretty(&app.ui).map_err(|e| e.to_string())?; + let Some(path) = self.path.clone() else { return Ok(()) }; + let bytes = serde_json::to_vec_pretty(ui).map_err(|e| e.to_string())?; if bytes == self.written { return Ok(()); } - std::fs::create_dir_all(&d) - .and_then(|()| write_atomic(&d.join("ui.json"), &bytes)) + path.parent() + .map_or(Ok(()), std::fs::create_dir_all) + .and_then(|()| write_atomic(&path, &bytes)) .map_err(|e| format!("saving the app settings failed: {e}"))?; self.written = bytes; - self.library = app.ui.settings.library_path.clone(); + self.library = ui.settings.library_path.clone(); Ok(()) } @@ -470,7 +510,7 @@ fn main() -> eframe::Result { _ => files.push(a), } } - let (prefs, prefs_warning, keep_prefs_file) = load_prefs(); + let (prefs, prefs_warning, keep_prefs_file) = load_prefs(in_memory); // --library, else the library last opened from Settings, else the default location let library_dir = library_dir.or_else(|| { prefs @@ -482,7 +522,7 @@ fn main() -> eframe::Result { let library_dir = library_dir.or_else(lightcraft_engine::library::default_dir); // GPU compute: off if the preference says so, or if the last launch died creating the device // (issue #136) — before anything can create it - let gpu_crash = gpu_crash_check(); + let gpu_crash = gpu_crash_check(in_memory); let gpu_on = prefs.as_ref().is_none_or(|u| u.settings.gpu) && gpu_crash.is_none(); lightcraft_engine::gpu::set_enabled(gpu_on); let options = eframe::NativeOptions { @@ -520,6 +560,7 @@ fn main() -> eframe::Result { app.notices.extend(prefs_warning); // what's on disk now: only changes are written let writer = PrefsWriter { + path: if in_memory { None } else { prefs_path() }, written: serde_json::to_vec_pretty(&app.ui).unwrap_or_default(), library: app.ui.settings.library_path.clone(), keep_file: keep_prefs_file, @@ -575,7 +616,7 @@ mod tests { let d = dir("damaged"); let path = d.join("ui.json"); std::fs::write(&path, br#"{"settings": {"library_path": "/Volumes/Photos/Lib"#).unwrap(); - let (ui, warning, keep) = load_prefs_at(&path); + let (ui, warning, keep) = load_prefs_at(&path, true); assert!(ui.is_none() && !keep); let warning = warning.unwrap(); assert!(warning.contains("damaged") && warning.contains("ui.json.corrupt-"), "{warning}"); @@ -583,7 +624,80 @@ mod tests { let kept: Vec<_> = std::fs::read_dir(&d).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().to_string()).collect(); assert!(kept.len() == 1 && kept[0].starts_with("ui.json.corrupt-"), "{kept:?}"); // a missing file is just "no settings yet" - assert_eq!(load_prefs_at(&path).1, None); + assert_eq!(load_prefs_at(&path, true).1, None); + let _ = std::fs::remove_dir_all(&d); + } + + /// Issue #164: a `--memory` session reads the settings but writes nothing — a damaged file is + /// reported and left where it is, not set aside. + #[test] + fn memory_session_leaves_a_damaged_ui_json_alone() { + let d = dir("damaged-memory"); + let path = d.join("ui.json"); + let bytes = br#"{"settings": {"library_path": "/Volumes/Photos/Lib"#; + std::fs::write(&path, bytes).unwrap(); + let (ui, warning, keep) = load_prefs_at(&path, false); + assert!(ui.is_none() && keep); + let warning = warning.unwrap(); + assert!(warning.contains("damaged") && warning.contains("nothing is saved"), "{warning}"); + assert_eq!(std::fs::read(&path).unwrap(), bytes); + assert_eq!(std::fs::read_dir(&d).unwrap().count(), 1); + let _ = std::fs::remove_dir_all(&d); + } + + /// Issue #164: the writer saves changed settings to its file — and nothing at all without one + /// (`--memory`) or while the session is the temporary one offered when the library can't open. + #[test] + fn prefs_writer_writes_nothing_in_a_memory_or_temporary_session() { + let d = dir("writer"); + let path = d.join("sub").join("ui.json"); + let mut ui = UiState::default(); + // an ordinary session: a change is written, the folder created on the way + let mut w = PrefsWriter { path: Some(path.clone()), ..Default::default() }; + ui.thumb_size += 50.0; + w.save_ui(&ui, false).unwrap(); + let saved = std::fs::read(&path).unwrap(); + assert!(String::from_utf8_lossy(&saved).contains("thumbSize"), "{}", String::from_utf8_lossy(&saved)); + // the temporary session (library problem): a further change is not written… + ui.thumb_size += 50.0; + w.save_ui(&ui, true).unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), saved); + // …until a library opens + w.save_ui(&ui, false).unwrap(); + assert_ne!(std::fs::read(&path).unwrap(), saved); + // --memory: no path, so no file and no folder, whatever changes + let d2 = dir("writer-memory"); + let _ = std::fs::remove_dir_all(&d2); + let mut w = PrefsWriter { path: None, ..Default::default() }; + ui.thumb_size += 50.0; + w.save_ui(&ui, false).unwrap(); + assert!(w.written.is_empty() && !d2.exists()); + let _ = std::fs::remove_dir_all(&d); + } + + /// Issue #169: a `--memory` session neither arms the GPU crash sentinel (which would create + /// the settings folder) nor removes a marker a crashed launch left; it still reports it, and + /// the next ordinary launch finds it, reports it and clears it. + #[test] + fn memory_session_leaves_the_gpu_marker_and_folder_alone() { + let d = dir("gpu-marker-memory"); + let _ = std::fs::remove_dir_all(&d); + let m = d.join("gpu-init.marker"); + // nothing left behind: no notice, and the folder is not created + assert_eq!(gpu_crash_check_at(Some(m.clone()), true), None); + assert!(!d.exists(), "a --memory session must not create the settings folder"); + // a marker left by a crashed launch: reported, kept + std::fs::create_dir_all(&d).unwrap(); + std::fs::write(&m, "GPU device creation started (backends METAL)").unwrap(); + let notice = gpu_crash_check_at(Some(m.clone()), true).unwrap(); + assert!(notice.contains("GPU rendering is now off"), "{notice}"); + assert!(m.exists(), "the marker stays for the next ordinary launch"); + // the next ordinary launch reports it once and removes it + assert!(gpu_crash_check_at(Some(m.clone()), false).is_some()); + lightcraft_engine::gpu::backend::set_init_marker(None); + assert!(!m.exists()); + assert_eq!(gpu_crash_check_at(Some(m.clone()), false), None); + lightcraft_engine::gpu::backend::set_init_marker(None); let _ = std::fs::remove_dir_all(&d); } @@ -630,7 +744,7 @@ mod tests { assert_eq!(std::fs::read(&path).unwrap(), b"{\"new\": true}"); let ui = UiState::default(); write_atomic(&path, &serde_json::to_vec_pretty(&ui).unwrap()).unwrap(); - let (loaded, warning, _) = load_prefs_at(&path); + let (loaded, warning, _) = load_prefs_at(&path, true); assert!(loaded.is_some() && warning.is_none()); let _ = std::fs::remove_dir_all(&d); } diff --git a/crates/gpu/src/backend.rs b/crates/gpu/src/backend.rs index 5c50ca04..fcdea55f 100644 --- a/crates/gpu/src/backend.rs +++ b/crates/gpu/src/backend.rs @@ -155,8 +155,16 @@ pub(crate) fn with_init_marker(backends: Backends, f: impl FnOnce() -> T) -> /// If the init marker `path` is present — the last process died while creating the GPU device — /// remove it and return what it recorded. pub fn take_init_marker(path: &std::path::Path) -> Option { - let text = std::fs::read_to_string(path).ok()?; + let text = read_init_marker(path)?; let _ = std::fs::remove_file(path); + Some(text) +} + +/// What the init marker `path` recorded, if it is present — without removing it. For sessions +/// that must leave the disk as they found it (`--memory`, issues #164 and #169): GPU rendering +/// still starts off, and the marker stays for the next ordinary launch to report and clear. +pub fn read_init_marker(path: &std::path::Path) -> Option { + let text = std::fs::read_to_string(path).ok()?; Some(text.trim().to_string()) } @@ -225,10 +233,13 @@ mod tests { set_init_marker(None); assert!(seen, "the marker exists while the device is created"); assert!(!m.exists(), "and is removed afterwards"); - // a marker left behind by a crashed process is reported once + // a marker left behind by a crashed process is reported once; reading it leaves it in place std::fs::write(&m, "GPU device creation started (backends DX12)").unwrap(); + assert!(read_init_marker(&m).unwrap().contains("DX12")); + assert!(m.exists(), "read_init_marker leaves the marker for the next launch"); assert!(take_init_marker(&m).unwrap().contains("DX12")); assert_eq!(take_init_marker(&m), None); + assert_eq!(read_init_marker(&m), None); let _ = std::fs::remove_dir_all(&dir); } } diff --git a/docs/gpu-pipeline.md b/docs/gpu-pipeline.md index 794b3aa8..a32814ef 100644 --- a/docs/gpu-pipeline.md +++ b/docs/gpu-pipeline.md @@ -55,7 +55,9 @@ Settings ▸ Performance ▸ *Use the GPU for rendering* unchecked (applied befo returns, successfully or not. If the marker is still there at the next launch, the process died inside the driver: LightCraft starts with GPU rendering off (the preference is saved unchecked), removes the marker and says so in a notice. Checking *Use the GPU for rendering* again tries the GPU once more -(and re-arms the sentinel). Not with `LIGHTCRAFT_NO_PREFS` (tests, scripts). Killing the app during +(and re-arms the sentinel). Not with `LIGHTCRAFT_NO_PREFS` (tests, scripts), and not in a `--memory` session, +which writes nothing: it neither arms the sentinel nor removes a marker it finds (it still starts with GPU +rendering off when one is there, and the next ordinary launch reports and clears it). Killing the app during the ~0.3 s of device creation, or two instances starting at the same moment, can trip it falsely — harmless: rendering is then on the CPU until the box is checked again. The sentinel only covers the compute device; a crash while the window's renderer starts is avoided by the backend defaults above diff --git a/docs/whats-new.md b/docs/whats-new.md index ea0243f4..5d292008 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -14,6 +14,9 @@ - 23 new built-in presets: Portrait, Landscape, Urban, Food, Seasons, Vintage and B&W toners. ### Reliability +- `--memory` sessions keep their promise to save nothing (issues #164, #169): UI changes made in one no longer + land in `ui.json` (where they replaced the saved settings), and the GPU crash sentinel no longer creates the + settings folder there. The same goes for the temporary session offered when the library can't be opened. - LightCraft no longer crashes at launch on Windows PCs whose Vulkan driver is broken (issue #136, e.g. some Intel UHD 630 drivers): on Windows the window and GPU rendering use DirectX 12 only and never load the Vulkan driver unless asked to. `LIGHTCRAFT_GPU_BACKEND=dx12 | vulkan | metal | off` (or wgpu's `WGPU_BACKEND`, which GPU rendering