From 0fa8070647e612e0a0f219b966931bd8d679c098 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Tue, 13 Oct 2026 02:33:10 +0200 Subject: [PATCH] AI Denoise: Install from file says why a model file is refused (fixes #1381) --- crates/engine/src/cmd/denoise.rs | 17 +++++++++--- crates/engine/src/tests_denoise.rs | 23 ++++++++++++++++ crates/ui-egui/src/headless.rs | 39 ++++++++++++++++++++++++++++ crates/ui-egui/src/panels/denoise.rs | 22 ++++++++++++++-- 4 files changed, 96 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd/denoise.rs b/crates/engine/src/cmd/denoise.rs index 8802460a..f93a2434 100644 --- a/crates/engine/src/cmd/denoise.rs +++ b/crates/engine/src/cmd/denoise.rs @@ -303,9 +303,20 @@ fn inspect(s: &mut Session, p: &Value) -> Result { if !src.extension().is_some_and(|e| e.eq_ignore_ascii_case("onnx")) { return Err(bad(C, "choose an .onnx file with denoise-model.json beside it")); } - let bytes = read_capped(&src.with_file_name("denoise-model.json"), MAX_MANIFEST_BYTES) - .ok_or_else(|| bad(C, "denoise-model.json is missing or too large"))?; - let m = manifest::parse(&bytes).map_err(|e| bad(C, e.to_string()))?; + let sibling = src.with_file_name("denoise-model.json"); + let Some(bytes) = read_capped(&sibling, MAX_MANIFEST_BYTES) else { + let why = if sibling.is_file() { + "denoise-model.json could not be read or is too large" + } else if src.with_file_name("model.json").is_file() { + // other apps ship their models with a model.json of their own (#1381) + "No denoise-model.json beside this .onnx file. The model.json there is another app's description, which LightCraft \ + does not read: it needs its own denoise-model.json (see docs/denoise.md)" + } else { + "No denoise-model.json beside this .onnx file: LightCraft needs one that says how to use the model (see docs/denoise.md)" + }; + return Err(bad(C, why)); + }; + let m = manifest::parse(&bytes).map_err(|e| bad(C, format!("denoise-model.json: {e} (see docs/denoise.md)")))?; let dir = models_dir(s, C)?; Ok(json!({"kind": "model", "domain": "denoise", "path": path, "fileName": src.file_name().map(|n| n.to_string_lossy()), "model": m, "sizeBytes": std::fs::metadata(src).ok().map(|m| m.len()), "alreadyInstalled": dir.join(&m.id).join("model.onnx").is_file()})) diff --git a/crates/engine/src/tests_denoise.rs b/crates/engine/src/tests_denoise.rs index ae202f19..1cba9f64 100644 --- a/crates/engine/src/tests_denoise.rs +++ b/crates/engine/src/tests_denoise.rs @@ -277,6 +277,29 @@ fn a_manifest_that_does_not_validate_or_name_the_file_is_refused() { let _ = std::fs::remove_dir_all(&x.dir); } +/// Another app's model folder (#1381): an `.onnx` with no manifest, then with that app's `model.json` beside it, then +/// with the `model.json` renamed. Each refusal says what is wrong and where LightCraft's manifest is described. +#[test] +fn inspecting_another_apps_model_folder_says_what_is_missing() { + let dir = temp_dir("denoise-foreign"); + let mut s = Session::new().with_fs(); + s.set_denoise_models_dir(Some(dir.join("denoise-models"))); + let src = dir.join("rawdenoise"); + std::fs::create_dir_all(&src).unwrap(); + let onnx = src.join("rawdenoise.onnx"); + std::fs::write(&onnx, b"not an onnx file").unwrap(); + let mut inspect = || s.execute("denoise.models.inspect", &json!({"path": onnx.to_string_lossy()})).unwrap_err().to_string(); + let e = inspect(); + assert!(e.contains("No denoise-model.json beside") && e.contains("docs/denoise.md") && !e.contains("model.json there"), "{e}"); + std::fs::write(src.join("model.json"), json!({"name": "rawdenoise", "input": {"channels": 4}, "scale": 2}).to_string()).unwrap(); + let e = inspect(); + assert!(e.contains("No denoise-model.json beside") && e.contains("The model.json there") && e.contains("docs/denoise.md"), "{e}"); + std::fs::rename(src.join("model.json"), src.join("denoise-model.json")).unwrap(); + let e = inspect(); + assert!(e.contains("not a denoise-model manifest") && e.contains("docs/denoise.md"), "{e}"); + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn a_photo_is_denoised_in_the_background_and_the_amount_mixes_it_in() { let mut x = setup("background", true); diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index 8cb91b9b..0f56d79b 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -508,6 +508,45 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// Install from file… with a model the engine rejects says why in the AI Denoise section (#1381), instead of + /// leaving the button looking dead. + #[test] + fn a_rejected_model_file_says_why_in_settings() { + use crate::state::Dialog; + let mut h = demo([1400.0, 900.0]); + let t = Duration::from_secs(10); + let dir = std::env::temp_dir().join(format!("lc-ui-denoise-rejected-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + h.app.session.set_denoise_models_dir(Some(dir.join("denoise-models"))); + // without the runtime the button is disabled (the workspace build turns it on through the desktop app) + if h.app.session.execute("denoise.models.list", &json!({})).unwrap()["runtime"] != true { + let _ = std::fs::remove_dir_all(&dir); + return; + } + // another app's model folder: the .onnx with that app's model.json, and no denoise-model.json + let onnx = dir.join("rawdenoise.onnx"); + std::fs::write(&onnx, b"not a model").unwrap(); + std::fs::write(dir.join("model.json"), br#"{"name": "rawdenoise", "scale": 2}"#).unwrap(); + let picked = onnx.to_string_lossy().into_owned(); + h.app.services.pick_denoise_model = Some(Box::new(move || vec![picked.clone()])); + h.app.ui.dialog = Some(Dialog::Settings { tab: "denoise".into() }); + h.settle(SETTLE); + let r = h.request("ui.clickWidget", json!({"id": "denoise:installFile"}), t); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + h.step(); + assert_eq!(h.app.ui.dialog, Some(Dialog::Settings { tab: "denoise".into() })); + let painted = h.painted_text(); + assert!( + painted + .iter() + .any(|s| s.starts_with("No denoise-model.json beside this .onnx file. The model.json there") && s.ends_with("(see docs/denoise.md)")), + "{painted:?}" + ); + let _ = std::fs::remove_dir_all(&dir); + } + use super::*; /// The pretend host has a clipboard, as a desktop does: text the UI copies lands in it, and a diff --git a/crates/ui-egui/src/panels/denoise.rs b/crates/ui-egui/src/panels/denoise.rs index bf89acd2..75488f76 100644 --- a/crates/ui-egui/src/panels/denoise.rs +++ b/crates/ui-egui/src/panels/denoise.rs @@ -35,6 +35,8 @@ pub struct Ui { /// Models the user pressed Download for that have not been installed yet. dl_watch: Vec, list: Option<(u64, f64, Value)>, + /// Why the last file picked with Install from file… was refused, shown under the button. + rejected: Option, } /// Called every frame: keeps the engine's denoise work going and asks for a redraw when a photo's picture appears (the @@ -247,9 +249,17 @@ pub fn settings_tab(app: &mut LightcraftApp, ui: &mut egui::Ui, t: &Tokens) { register(ui.ctx(), "denoise:installFile", r.rect); if r.clicked() && let Some(path) = app.services.pick_denoise_model.as_mut().and_then(|f| f().into_iter().next()) - && let Ok(info) = app.run("denoise.models.inspect", json!({"path": path})) { - app.ui.dialog = Some(crate::state::Dialog::DenoiseModel { info, accepted: false }); + match app.session.execute("denoise.models.inspect", &json!({"path": path})) { + Ok(info) => { + app.caches.denoise.rejected = None; + app.ui.dialog = Some(crate::state::Dialog::DenoiseModel { info, accepted: false }); + } + Err(e) => app.caches.denoise.rejected = Some(plain(&e)), + } + } + if let Some(why) = &app.caches.denoise.rejected { + hint(ui, t, why); } if all.is_empty() { hint(ui, t, "Install your own ONNX model with denoise-model.json beside it. This build offers no model downloads."); @@ -550,6 +560,14 @@ pub fn install(app: &mut LightcraftApp, info: &Value, accepted: bool) -> Result< r } +/// What an engine error says, without the command it came from (the install job's errors read the same way). +fn plain(e: &lightcraft_engine::EngineError) -> String { + match e { + lightcraft_engine::EngineError::BadParams { msg, .. } => msg.clone(), + other => other.to_string(), + } +} + /// "1 photo" / "3 photos": the count with the right noun. fn count(n: u64, one: &str, many: &str) -> String { tr_format!("{n} {noun}", n = n, noun = tr(if n == 1 { one } else { many }))