From 8c08c211a369ff936397fbb16db39e9267b67c81 Mon Sep 17 00:00:00 2001 From: Sylpheed RE agent Date: Wed, 19 Aug 2026 02:45:48 +0000 Subject: [PATCH] formats: narrow the ghost skip, and withdraw the claim that it fixed a bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checking the blast radius of the previous commit's `kind = 0x4` skip found 174 elements on the disc that are 0x4 with no non-0x4 element of the same sprite — GP_READY_ROOM pak entry 75 is 56 elements and every one is 0x4, a list of real icons a blanket skip would erase. So the rule is narrowed: skip an instance only when its template is present, which is the case the title capture covers. The title render is byte-identical before and after, so the narrowing changes nothing the evidence covers. And the claim that this "caught a regression" is WITHDRAWN, because measuring it refuted it: none of those 174 elements is in a bundle `is_build` accepts, so none of them ever reaches the compositor. The blanket skip would have been harmless in practice. The narrow rule is a precaution, not a fix, and the comment says so. The test that was going to assert the regression could not find such a build — correctly, since none is composable. It is replaced by one that pins the fact which makes the narrow rule safe: no composable build on the disc has a 0x4 element without its template, checked across 500+ builds. If that stops being true it fails, instead of a screen quietly going empty. --- crates/sylpheed-formats/src/ui_layout.rs | 31 +++++++--- .../tests/ui_paint_order_disc.rs | 58 +++++++++++++++++++ 2 files changed, 81 insertions(+), 8 deletions(-) diff --git a/crates/sylpheed-formats/src/ui_layout.rs b/crates/sylpheed-formats/src/ui_layout.rs index 1034b3a..f7d2c79 100644 --- a/crates/sylpheed-formats/src/ui_layout.rs +++ b/crates/sylpheed-formats/src/ui_layout.rs @@ -569,14 +569,29 @@ pub fn compose( if (el.animated && !opts.include_animated) || (el.focused && !opts.include_focus) { continue; } - // `kind = 0x4` elements are the repeated instances of a template — the - // motion-trail ghosts a wordmark leaves while it flies in. They are NOT - // on screen at rest: the title's draw capture shows exactly ONE quad at - // each wordmark's position, though the bundle declares three instances - // of each, and their keyframe groups end at alpha 0. Drawing them at - // their resting keyframe is what put three oversized copies of - // PROJECT SYLPHEED across the composite. - if el.kind & 0x4 != 0 { + // A `kind = 0x4` element is a repeated instance of a template. On the + // title screen those are motion-trail ghosts and are NOT on screen at + // rest — the draw capture shows exactly ONE quad at each wordmark's + // position though the bundle declares three instances of each, and + // drawing them put three oversized PROJECT SYLPHEED copies across the + // composite. + // + // But `0x4` alone does not mean "ghost". 174 elements on the disc are + // `0x4` with **no** non-`0x4` element of the same sprite — e.g. + // `GP_READY_ROOM` pak entry 75, 56 elements and every one of them `0x4` + // (`pbb_destroyer` ×5, `pb_w_line` ×5, …), a list of real icons that a + // blanket skip would erase. Measured afterwards, and worth stating + // plainly: **none** of those 174 is in a bundle `is_build` accepts, so + // today they never reach this function and a blanket skip would have + // been harmless in practice. The condition below is therefore a + // precaution, not a bug fix — it keeps the rule to the case the capture + // actually covers, an instance whose template is also present. + if el.kind & 0x4 != 0 + && build + .elements + .iter() + .any(|o| o.kind & 0x4 == 0 && o.name == el.name) + { continue; } let Some(kf) = el.rest() else { continue }; diff --git a/crates/sylpheed-formats/tests/ui_paint_order_disc.rs b/crates/sylpheed-formats/tests/ui_paint_order_disc.rs index 129663a..c2d573c 100644 --- a/crates/sylpheed-formats/tests/ui_paint_order_disc.rs +++ b/crates/sylpheed-formats/tests/ui_paint_order_disc.rs @@ -275,3 +275,61 @@ fn title_composites_in_the_measured_order_without_ghosts() { } assert!(found, "the 24-element GP_TITLE build was not found"); } + +/// The ghost skip is inert everywhere except where the capture licenses it. +/// +/// `0x4` means "repeated instance of a template", and on the title those are +/// motion-trail ghosts the game does not show at rest. Skipping every `0x4` +/// element for that reason would be over-broad: 174 elements on the disc are +/// `0x4` with no non-`0x4` element of the same sprite (`GP_READY_ROOM` pak entry +/// 75 is 56 elements, all of them `0x4` — a list of real icons). +/// +/// This pins the measurement that makes the narrow rule safe: **no bundle the +/// compositor accepts contains such an element**, so the skip only ever drops a +/// ghost whose template is right there beside it. If that ever stops being true, +/// this fails and the rule needs re-deriving rather than quietly erasing a +/// screen. +#[test] +fn no_composable_build_has_an_instance_without_its_template() { + skip_without_disc!(root); + let mut paks: Vec = std::fs::read_dir(root.join("dat")) + .expect("dat/") + .flatten() + .map(|e| e.path()) + .filter(|p| p.extension().and_then(|s| s.to_str()) == Some("pak")) + .collect(); + paks.sort(); + let mut orphans = Vec::new(); + let mut builds_seen = 0usize; + // one pak at a time: holding every build on the disc at once OOM-kills the + // test process when it runs alongside the others. + for p in &paks { + let name = p.file_name().unwrap().to_string_lossy().to_string(); + for bundle in pak_builds(&root, &name) { + let Some(build) = ui_layout::parse_build(&bundle) else { + continue; + }; + builds_seen += 1; + for el in &build.elements { + if el.kind & 0x4 == 0 { + continue; + } + if !build + .elements + .iter() + .any(|o| o.kind & 0x4 == 0 && o.name == el.name) + { + orphans.push(format!("{name}: {} (kind {:#x})", el.name, el.kind)); + } + } + } + } + assert!(builds_seen > 500, "expected the disc's builds, saw {builds_seen}"); + assert!( + orphans.is_empty(), + "{} composable elements are kind=0x4 with no template present, so the \ + ghost skip would erase real content: {:?}", + orphans.len(), + &orphans[..orphans.len().min(8)] + ); +}