formats: narrow the ghost skip, and withdraw the claim that it fixed a bug

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.
This commit is contained in:
Sylpheed RE agent
2026-08-19 02:45:48 +00:00
parent f66e9de430
commit 8c08c211a3
2 changed files with 81 additions and 8 deletions

View File

@@ -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 };

View File

@@ -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::path::PathBuf> = 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)]
);
}