fabi/Syplheed-Reborn
Archived
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

viewer: stop the UI Screens browser loading forever

Browse Source 
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.
This commit is contained in:
Sylpheed RE agent
2026-08-28 15:59:00 +02:00
parent 1054d43676
commit 47e4e43310
2 changed files with 77 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

73
crates/sylpheed-viewer/src/iso_loader.rs
View File

@@ -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<ScreenPak>
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<Vec<u8>> = 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<Vec<u8>> = 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<Vec<u8>> = 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 {

13
crates/sylpheed-viewer/src/ui.rs
View File

@@ -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(),