From 3b35c48fce28e13856d6a2b87bb65f62148da742 Mon Sep 17 00:00:00 2001 From: "Claude (auto-RE)" Date: Thu, 13 Aug 2026 12:27:56 +0000 Subject: [PATCH] fix(xbg7): grouped selection prefers the candidate explaining the whole pool; per-sub-mesh decls on by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With per-sub-mesh declarations enabled, n201_01 decoded as a 2-part fragment 4 bytes off. Both starts validate for the pivot — 0x32BA718 at pad 2 (earlier in file order, so first-match took it) and the capture-proven 0x32BA71C at pad 0 — so the pivot alone cannot separate them; at the early one two of four sub-meshes fall out as out-of-range. anchor_grouped_meshes now builds each accepted candidate and keeps the one that explains the most of the declared pool: it returns immediately when a candidate explains all n sub-meshes, else keeps the best partial, so it can never decode less than first-match did. n201_01 lands on all four capture-proven offsets (0x32BA71C / 0x32BEFF4 / 0x32C416C / 0x32C536C) and its two sibling copies take their own pools, so the twin collapse is gone. XBG7_SUBMESH_DECLS is therefore on by default (=0 reverts): resources that never decode 85 -> 47 resources decoding in no container 63 -> 30 degenerate index runs 1 -> 1 (unchanged) cross-container minority decodes 96 -> 96 (unchanged) captured index runs, stage-02 93/93 (unchanged) captured index runs, stage-05 124/128 -> 128/128 The last line is the point: the buffers the capture could not name are the n201 family, and they now decode and match the GPU's indices byte for byte. Suite green including twin_pairs_do_not_share_a_buffer, apart from the pre-existing known-failing cross-container consistency test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NptfmpjdpNCKEez6d2xvA9 --- crates/sylpheed-formats/src/mesh.rs | 130 ++++++++++++++-------------- docs/re/structures/xbg7-mesh.md | 50 ++++++++++- 2 files changed, 115 insertions(+), 65 deletions(-) diff --git a/crates/sylpheed-formats/src/mesh.rs b/crates/sylpheed-formats/src/mesh.rs index 6d40708..84fa4fb 100644 --- a/crates/sylpheed-formats/src/mesh.rs +++ b/crates/sylpheed-formats/src/mesh.rs @@ -1143,23 +1143,23 @@ fn pad0_consistency() -> f32 { /// [`anchor_pool_mesh`] takes the FIRST pad that validates (the pre-fix /// behaviour) instead of the pad whose index run is cleanest. Kept so the two /// behaviours can be diffed on the disc; see docs/re/structures/xbg7-mesh.md. -/// Use the **per-sub-mesh** vertex declarations in a grouped pool -/// (`XBG7_SUBMESH_DECLS=1`, default off). +/// Use the **per-sub-mesh** vertex declarations in a grouped pool — **on by +/// default**; `XBG7_SUBMESH_DECLS=0` restores the single-declaration reading. /// -/// The format truth is not in question: each index marker is followed by its own -/// element triples and they can differ — `n201_01` (`Stage_S02.xpr`) declares -/// strides 24, 24, 24, **28**, which a runtime capture confirms draw-for-draw. With -/// this on, the disc-wide misses fall **85 → 47** and the resources that decode in -/// no container at all fall **63 → 30**. +/// Each index marker is followed by its own element triples and they can differ: +/// `n201_01` (`Stage_S02.xpr`) declares strides 24, 24, 24, **28**, which a runtime +/// capture confirms draw-for-draw (`stride=28` on the fourth draw, and a distinct +/// vertex shader per sub-mesh). Reading only the first declaration walked the last +/// buffer out of phase and declined the whole resource. /// -/// It is off by default because selection has not caught up: the three `n201_0x` -/// copies then land on ONE pool (`tests/mesh_consistency_disc.rs::twin_pairs_do_not_share_a_buffer` -/// fails), and four newly decoded `ptc_pack` `.dat` composites carry degenerate -/// triangles. The capture-proven pool start now VALIDATES (`debug_grouped_report` -/// reports `pad 0: ACCEPTED` where it used to report a NaN position), so what -/// remains is choosing it — see docs/re/structures/xbg7-mesh.md. +/// With this and the completeness-based candidate choice in +/// [`anchor_grouped_meshes`], `n201_01` anchors at the capture-proven pool start +/// with all four sub-meshes at the captured offsets, its two sibling copies take +/// their own pools, disc-wide misses fall **85 → 47**, and the captured index runs +/// of the stage-05 mission rise **124 → 128** identical. See +/// docs/re/structures/xbg7-mesh.md. fn submesh_decls() -> bool { - std::env::var("XBG7_SUBMESH_DECLS").map(|v| v == "1").unwrap_or(false) + std::env::var("XBG7_SUBMESH_DECLS").map(|v| v != "0").unwrap_or(true) } fn pad_first_match() -> bool { @@ -1710,6 +1710,49 @@ fn anchor_grouped_meshes( let kmax = (0..n).max_by_key(|&i| markers[i].1).unwrap_or(0); let (vck, ick) = markers[kmax]; + // Build the pool at a candidate (vb0, pad). Sub-meshes that fail the + // structural requirements (every index inside its own buffer, indices + // reaching its end) are skipped, so the returned length says how much of the + // declared pool this candidate actually explains — which is what selects + // between candidates below. + let build = |vb0: usize, pad: usize| -> Vec { + let ib0 = vb0 - span - pad; + let mut meshes = Vec::with_capacity(n); + let mut vb = vb0; + for i in 0..n { + let (vc, ic) = markers[i]; + let ib = ib0 + rel_ib[i]; + if ib + ic * 2 > bytes.len() + || vc.checked_mul(decls[i].stride).map_or(true, |b| vb + b > bytes.len()) + { + break; + } + let ok = validate_block(bytes, ib, vb, vc, ic, &decls[i], 0.85, false); + if !ok && i > kmax { + break; // chain diverged — emit the validated prefix, no garbage + } + let mut max_idx = 0usize; + let in_range = (0..ic).all(|k| { + let i = be16(bytes, ib + k * 2) as usize; + max_idx = max_idx.max(i); + i < vc + }); + if in_range && max_idx + cover_slack() >= vc { + meshes.push(read_pool_mesh(bytes, ib, vb, ic, vc, &decls[i])); + } + vb += vc * decls[i].stride; + } + meshes + }; + + // A candidate that explains the WHOLE pool beats one that explains part of it, + // however early it sits in file order. `n201_01` is the case that forced this: + // a `vb0` **4 bytes before** the capture-proven start also validates for the + // pivot (at pad 2) and, being earlier in the scan, used to win — then two of + // the four sub-meshes fell out as out-of-range and the resource decoded as a + // 2-part fragment 4 bytes off. The proven start explains all four. + let mut partial: Option> = None; + for &vb0 in starts { // Distinct assignment: a pool another resource already claimed is not a // candidate (see the collision resolution in `anchor_models_filtered`). @@ -1749,58 +1792,19 @@ fn anchor_grouped_meshes( } } if let Some((_, _, pad)) = best { - let ib0 = vb0 - span - pad; - - // Pivot confirmed the exact alignment ⇒ every marker up to the pivot - // is correctly placed; read those unconditionally (a legitimately - // tiny/flat lead part may fail the quality gates yet still be real). - // Markers after the pivot are validated so a stray trailing marker - // ends the chain instead of appending garbage. - let mut meshes = Vec::with_capacity(n); - let mut vb = vb0; - for i in 0..n { - let (vc, ic) = markers[i]; - let ib = ib0 + rel_ib[i]; - if ib + ic * 2 > bytes.len() - || vc.checked_mul(decls[i].stride).map_or(true, |b| vb + b > bytes.len()) - { - break; - } - // Parts are placed deterministically; in-range + consistency pins - // them, so the connectivity heuristic (which mis-rejects small - // flat fins) is relaxed here. - let ok = validate_block(bytes, ib, vb, vc, ic, &decls[i], 0.85, false); - if !ok && i > kmax { - break; // chain diverged — emit the validated prefix, no garbage - } - // Sub-meshes BEFORE the pivot are emitted even when they fail the - // quality gates (a tiny flat lead part is legitimately poor), but - // an index that addresses past its own vertex buffer is not a - // quality question — it is unusable. Measured 2026-08-12: 18 - // sub-meshes disc-wide carried indices up to 364 vertices past - // the end (`coverage_audit`), which any renderer would fault on. - // Same two structural requirements the searched path enforces: - // every index inside the buffer, and the indices reaching the - // end of it. Real geometry covers its pool exactly — 8 586 of - // 8 636 decoded sub-meshes reference their last vertex, none - // more than 3 short (`coverage_audit`) — so a sub-mesh whose - // indices stop well short is reading the wrong block, not a - // sparse one. - let mut max_idx = 0usize; - let in_range = (0..ic).all(|k| { - let i = be16(bytes, ib + k * 2) as usize; - max_idx = max_idx.max(i); - i < vc - }); - if in_range && max_idx + cover_slack() >= vc { - meshes.push(read_pool_mesh(bytes, ib, vb, ic, vc, &decls[i])); - } - vb += vc * decls[i].stride; + // Pivot confirmed the alignment; how much of the pool does it explain? + let meshes = build(vb0, pad); + if meshes.len() == n { + return meshes; + } + if partial.as_ref().map_or(true, |p| meshes.len() > p.len()) { + partial = Some(meshes); } - return meshes; } } - Vec::new() + // No candidate explained the whole pool — keep the best partial one, so this + // can never decode less than the previous first-match behaviour. + partial.unwrap_or_default() } /// How clean is the triangle list at `ib` against the pool at `vb`? diff --git a/docs/re/structures/xbg7-mesh.md b/docs/re/structures/xbg7-mesh.md index ac4bc18..ae7fa60 100644 --- a/docs/re/structures/xbg7-mesh.md +++ b/docs/re/structures/xbg7-mesh.md @@ -1603,7 +1603,7 @@ Flying stage 16 did **not** get the `e901` wings drawn (the boss appears later i the mission), which is the next lesson: choosing the mission puts a container in memory, but the unit still has to be **on screen** for a draw to exist. -### 🔎 The descriptor carries a declaration PER SUB-MESH — worth 38 misses, held behind a knob (2026-08-13) +### ✅ The descriptor carries a declaration PER SUB-MESH — worth 38 misses (2026-08-13) Following the `n201` mixed-stride finding: the descriptor was dumped around every index marker (`examples/desc_dump.rs`), and the layout is unambiguous — **each @@ -1637,7 +1637,7 @@ And `debug_grouped_report` at `n201_01`'s **capture-proven** pool start now read `pad 0: ACCEPTED`, where it used to read `position component NaN` — the block the engine draws from is finally acceptable to the decoder. -**Why it is off by default.** Selection has not caught up: +**Selection had to catch up first** (it now has — see the next section): * the three `n201_0x` copies all settle on ONE pool, so `tests/mesh_consistency_disc.rs::twin_pairs_do_not_share_a_buffer` fails — a twin @@ -1654,3 +1654,49 @@ So the format question is **settled** (and capture-confirmed), the coverage win real and measured, and what stands between the two is the same selection/distinct-assignment machinery that the pad work already improved once. That is the next step, with `n201`'s proven offsets as the acceptance test. + +### ✅ …and the selection bug it exposed: prefer the candidate that explains the WHOLE pool + +With per-sub-mesh declarations on, `n201_01` decoded as a 2-part fragment **4 bytes** +off. The reason, from `debug_grouped_report` at both offsets: + +``` +vb0 0x32BA718 (4 bytes early) pad 2: ACCEPTED ← earlier in file order, so first-match took it +vb0 0x32BA71C (capture-proven) pad 0: ACCEPTED +``` + +Both validate for the **pivot** — the pivot alone cannot separate them. At the early +one, two of the four sub-meshes then fall out as out-of-range, so the resource decoded +as a fragment whose first sub-mesh sat at `0x32BEFF0` (= the early `vb0` + 777·24). + +`anchor_grouped_meshes` now **builds** each accepted candidate and keeps the one that +explains the most of the declared pool, returning immediately when a candidate +explains all `n` sub-meshes and falling back to the best partial otherwise (so it can +never decode less than first-match did). The result is exact: + +| `n201_01` sub-mesh | decoded vertex offset | capture-proven | +|---|---|---| +| #0 | `0x32BA71C` | `0x32BA71C` ✅ | +| #1 | `0x32BEFF4` | `0x32BEFF4` ✅ | +| #2 | `0x32C416C` | `0x32C416C` ✅ | +| #3 | `0x32C536C` | `0x32C536C` ✅ | + +and `n201_02` / `n201_03` take their own distinct pools (`0x3353BEC`, `0x3388BEC`), so +the twin collapse is gone. + +**Both changes are now the default** (`XBG7_SUBMESH_DECLS=0` reverts the declaration +reading): + +| | before | after | +|---|---|---| +| resources that never decode | 85 | **47** | +| resources decoding in **no** container | 63 | **30** | +| decoded index runs with a degenerate triangle | 1 | **1** | +| cross-container minority decodes | 96 | 96 | +| captured index runs identical, stage-02 capture | 93/93 | **93/93** | +| captured index runs identical, stage-05 mission capture | 124/128 (4 undecoded) | **128/128** | + +The last row is the one that matters most: the four buffers that capture could not +name are the `n201` family, and now they decode **and** their index runs match the +GPU byte for byte. Suite green (`twin_pairs_do_not_share_a_buffer` included) apart +from the pre-existing known-failing cross-container consistency test.