From b239510fd617ed763ce9723723493827e6caf3d1 Mon Sep 17 00:00:00 2001 From: Sylpheed RE agent Date: Fri, 28 Aug 2026 15:59:00 +0200 Subject: [PATCH] viewer: stop the UI Screens browser loading forever Three compounding causes, found by tracing every place a `loading` flag is set against every place it is cleared. THE COST. `compose_screen` inflated EVERY entry in the pak and held every build in memory at once, purely to index into the result -- then did it again on every checkbox click, re-opening the ISO from scratch each time. But the catalog already records each build's pak ENTRY INDEX, so the locator was there all along: compose now reads exactly one entry. `RequestScreenCompose` carries `entry` (the locator) beside `build` (the display ordinal). `build_screen_catalog` had no budget at all, while the pak browser has capped exactly this work since it was written. It now skips oversized entries and stops at a ceiling. That ceiling is 384 MB, not the pak browser's 64 MB, and the difference is the point: GP_HANGAR_ARSENAL inflates past 160 MB and holds ~390 builds, so a 64 MB cap would have quietly hidden most of them -- trading a hang for a wrong answer. When the ceiling IS hit the pak is marked `truncated` and the UI says so. THE LATCH. `poll_loader_channel` treated a disconnected channel exactly like an empty one, so if a worker died every in-flight spinner stayed up for the life of the process -- and the `if loading { return }` guard at the top of each handler then refused every retry. Disconnect now clears the flags and reports it. Verified: the workspace builds, the viewer reaches steady state, and one composite still draws 11/11 elements of the tutorial pause menu. --- crates/sylpheed-viewer/src/iso_loader.rs | 73 +++++++++++++++++++++--- crates/sylpheed-viewer/src/ui.rs | 13 +++++ 2 files changed, 77 insertions(+), 9 deletions(-) diff --git a/crates/sylpheed-viewer/src/iso_loader.rs b/crates/sylpheed-viewer/src/iso_loader.rs index acf9f018..84cba5c1 100644 --- a/crates/sylpheed-viewer/src/iso_loader.rs +++ b/crates/sylpheed-viewer/src/iso_loader.rs @@ -664,6 +664,11 @@ impl Default for ShipBrowser { /// builds it holds. A build is one *(context × language)* variant of the screen. #[derive(Clone)] pub struct ScreenPak { + /// True when the scan hit its decode budget and stopped early, so `builds` + /// is a PREFIX of what the pak holds. Surfaced in the UI rather than + /// silently swallowed — a truncated list that looks complete is worse than + /// a slow one. + pub truncated: bool, /// Disc path, e.g. `dat/GP_PAUSE_MENU.pak`. pub path: String, /// Display name, e.g. `PAUSE MENU`. @@ -738,7 +743,11 @@ pub struct RequestScreenCatalog; #[derive(Event)] pub struct RequestScreenCompose { pub pak: String, + /// Display ordinal within [`ScreenPak::builds`]. pub build: usize, + /// The pak ENTRY index the catalog recorded for this build — the locator + /// that lets a compose read one entry instead of scanning the archive. + pub entry: usize, pub focus: bool, pub animated: bool, /// Element indices to leave out (the per-element toggles). @@ -837,6 +846,11 @@ const PAK_DECODE_BUDGET: usize = 64 * 1024 * 1024; /// Skip decoding any single entry whose stored size exceeds this (cheap guard /// against a pathological entry, since `decompress_entry` has no output cap). const PAK_ENTRY_COMP_CAP: u32 = 16 * 1024 * 1024; +/// Decode ceiling for ONE pak while cataloguing screens. Deliberately larger +/// than `PAK_DECODE_BUDGET`: `GP_HANGAR_ARSENAL` alone inflates past 160 MB and +/// holds ~390 builds, so a 64 MB cap would quietly hide most of them. Hitting +/// this sets `ScreenPak::truncated`, which the UI shows. +const SCREEN_CATALOG_BUDGET: usize = 384 * 1024 * 1024; /// Bounded decoded-frame queue between the ffmpeg reader thread and the main /// thread. Also the back-pressure valve: when we stop draining (paused/behind), @@ -2204,7 +2218,22 @@ fn poll_loader_channel( iso_state.error = Some(msg); browser.loading = false; } - Err(TryRecvError::Empty) | Err(TryRecvError::Disconnected) => break, + Err(TryRecvError::Disconnected) => { + // The loader channel is gone: no result will ever arrive, so + // every in-flight spinner would latch forever. Clear them and + // say why, rather than showing "Scanning…" indefinitely. + iso_state.loading = false; + browser.loading = false; + screens.loading = false; + screens.composing = false; + saves.loading = false; + game_data.loading = false; + ships.loading = false; + voice_lib.loading = false; + iso_state.error = Some("the asset loader stopped responding".into()); + break; + } + Err(TryRecvError::Empty) => break, } } } @@ -4124,15 +4153,30 @@ fn build_screen_catalog(source: &SourceKind, files: &[String]) -> Vec let Ok(ar) = read_pak_archive_blocking(source, path) else { continue; }; + // BUDGETED, like the pak browser at `build_pak_rows`. Without this the + // catalog inflates every entry of every GP_*.pak with no ceiling — + // GP_HANGAR_ARSENAL alone holds ~510 RATC entries — which is most of + // why "Scanning the screen paks…" never finished. let mut builds = Vec::new(); + let mut budget: usize = 0; + let mut truncated = false; for (i, e) in ar.entries().iter().enumerate() { + if e.comp_size > PAK_ENTRY_COMP_CAP { + continue; + } + if budget > SCREEN_CATALOG_BUDGET { + truncated = true; + break; + } let Ok(bytes) = ar.read(e) else { continue }; + budget += bytes.len(); if ui_layout::is_build(&bytes) { builds.push((i, bytes.len())); } } if !builds.is_empty() { out.push(ScreenPak { + truncated, path: path.clone(), label: screen_label(path), builds, @@ -4159,9 +4203,10 @@ fn handle_screen_compose_request( screens.generation = screens.generation.wrapping_add(1); screens.composing = true; let generation = screens.generation; - let (pak, build, focus, animated, hidden) = ( + let (pak, build, entry, focus, animated, hidden) = ( req.pak.clone(), req.build, + req.entry, req.focus, req.animated, req.hidden.clone(), @@ -4169,7 +4214,7 @@ fn handle_screen_compose_request( let source = iso_state.source_kind.clone(); let sender = channels.sender.clone(); std::thread::spawn(move || { - let composed = compose_screen(&source, &pak, build, focus, animated, &hidden); + let composed = compose_screen(&source, &pak, entry, build, focus, animated, &hidden); let _ = sender.send(IsoLoaderMsg::ScreenComposed { generation, composed: Box::new(composed), @@ -4181,6 +4226,7 @@ fn handle_screen_compose_request( fn compose_screen( source: &SourceKind, pak: &str, + entry: usize, build_idx: usize, focus: bool, animated: bool, @@ -4201,13 +4247,22 @@ fn compose_screen( Ok(a) => a, Err(e) => return fail(e), }; - let bundles: Vec> = ar + // Read exactly the entry the catalog recorded for this build. The previous + // version inflated EVERY entry in the pak and held every build in memory at + // once, just to index into the result — repeated on every checkbox click, + // with the ISO re-opened from scratch each time. That, not the compositing, + // was the cost. + let one: Option> = ar .entries() - .iter() - .filter_map(|e| ar.read(e).ok()) - .filter(|b| ui_layout::is_build(b)) - .collect(); - let Some(bundle) = bundles.get(build_idx) else { + .get(entry) + .and_then(|e| ar.read(e).ok()) + .filter(|b| ui_layout::is_build(b)); + let bundles: Vec> = one.into_iter().collect(); + let Some(bundle) = bundles.first().filter(|_| { + // `build_idx` stays the display ordinal; `entry` is the real locator. + let _ = build_idx; + true + }) else { return fail(format!("build {build_idx} not in {pak}")); }; let Some(build) = ui_layout::parse_build(bundle) else { diff --git a/crates/sylpheed-viewer/src/ui.rs b/crates/sylpheed-viewer/src/ui.rs index d31a4e1b..7d762fde 100644 --- a/crates/sylpheed-viewer/src/ui.rs +++ b/crates/sylpheed-viewer/src/ui.rs @@ -1895,6 +1895,12 @@ fn draw_screens_ui( egui::CollapsingHeader::new(&pak.label) .default_open(pak.builds.len() == 1) .show(ui, |ui| { + if pak.truncated { + ui.colored_label( + egui::Color32::from_rgb(224, 168, 86), + "⚠ list truncated (decode budget)", + ); + } for (bi, (entry, size)) in pak.builds.iter().enumerate() { let selected = screens.selected == Some(pi) && screens.build == bi; @@ -2081,6 +2087,13 @@ fn draw_screens_ui( compose.send(RequestScreenCompose { pak: pak.path.clone(), build: screens.build, + // The catalog stored (pak entry index, size) per build; the + // entry index is what lets the worker read one entry. + entry: pak + .builds + .get(screens.build) + .map(|(e, _)| *e) + .unwrap_or(0), focus: screens.show_focus, animated: screens.show_animated, hidden: screens.hidden.clone(),