Audit du sous-système SpeedTree

Audit du crate byroredux-spt : analyse des flux .spt en TLV et émission de billboards de remplacement. Vérifie la comptabilité des octets, le fallback, le câblage TREE et les variations par jeu.

Spar Skills Guide Bot
DeveloppementAvancé
11016/08/2026
Claude Code
#audit#speedtree#rust#tlv-parser#billboard-fallback

Recommandé pour


description: "Audit the SpeedTree (.spt) TLV walker + placeholder-billboard fallback shipped in Session 33 Phase 1" argument-hint: "--focus <dimensions> --depth shallow|deep"

SpeedTree Subsystem Audit

Audit the byroredux-spt crate — a young, deliberately small subsystem (Session 33 Phase 1, "S1"). It does two things: (1) walks the .spt parameter section as a tag-length-value stream for FNV / FO3 / Oblivion, and (2) emits a placeholder billboard ImportedScene so TREE cells render something instead of panicking or going treeless. The real geometry tail is not decoded — everything past tail_offset is left on the floor by design.

Keep the audit proportionate. The highest-risk surface is the walker's byte accounting (one mis-sized payload desyncs the whole stream), then the placeholder fallback correctness, then TREE record → billboard wiring, then per-game .spt differences. The tag dictionary is large but each entry is a fixed-size decode — low risk individually.

Architecture: Single-pass — small enough to run all dimensions inline rather than spawning Tasks.

See .claude/commands/_audit-common.md for project layout, methodology, deduplication, context rules, severity, and finding format. Do not re-derive any of those here.

Scope

Crate: crates/spt/src/parser.rs, tag.rs, version.rs, stream.rs, scene.rs, crates/spt/src/import/mod.rs, plus the feature-gated crates/spt/src/recon/mod.rs. Public surface re-exported from crates/spt/src/lib.rs: parse_spt, import_spt_scene, SptImportParams, SptScene, detect_variant, dispatch_tag. (compute_billboard_size is a private helper in the import module, not an entry point.)

Cross-cuts (the wiring that actually invokes the crate):

  • byroredux/src/cell_loader/references/mod.rs — the production route. An is_spt extension check (model_path … eq_ignore_ascii_case("spt")) dispatches to parse_and_import_spt (implemented in the sibling byroredux/src/cell_loader/references/import.rs), which looks up the matching TREE record from record_index.trees and threads its metadata into SptImportParams. byroredux/src/cell_loader/refr.rs does not carry a .spt route.
  • byroredux/src/scene/nif_loader.rs — the --tree / loose-file direct-visualiser route (parse_import_and_merge, is_spt branch). This is a parallel path: it calls import_spt_scene with SptImportParams::default()no TREE metadata (no ICON override, no OBND/MODB/BNAM sizing). Verify the two routes don't silently diverge in ways that matter (Dimension 4).
  • crates/plugin/src/esm/records/tree.rsparse_treeTreeRecord. Captures OBND / ICON / MODB / SNAM / CNAM / BNAM / PFIG. has_speedtree_binary() is the case-insensitive .spt predicate. Pre-S1 TREE fell into the generic MODL-only path and dropped every field but MODL.
  • byroredux/src/systems/billboard.rsmake_billboard_system rotates any entity carrying a Billboard component. BsRotateAboutUp (the mode the spt placeholder uses) currently falls back to the world-up yaw lock — confirm that's still the behaviour and that the -Z front-face convention in import/mod.rs matches the rotation arc used here.
  • byroredux/src/cell_loader/spawn.rs — attaches the Billboard component from cached.placement_root_billboard, and routes the placeholder ImportedMesh through the NIFAL boundary (crate::material_translate::translate_material).

Phase 1 ("S1") acceptance (ground truth — verify before reporting):

  • TLV walker recovered against the FNV/FO3/Oblivion .spt corpus (133 vanilla files; Oblivion ≈ 113).
  • Acceptance gate ≥ 95 % unknown-tag-clean rate, asserted in crates/spt/tests/parse_real_spt.rs (env-var gated, #[ignore]).
  • Placeholder fallback: un-decoded trees render as a billboard card, never an Err out of the cell loader.
  • .spt REFRs route to the SpeedTree importer, not NIF.
  • --tree smoke path parses + imports.

Future phases (NOT shipped — do not flag as missing unless --focus explicitly includes them): real branch/leaf mesh recovery from the geometry tail, wind-bone animation from SNAM/CNAM, distance-LOD swap, baked-shadow lookup. SNAM/CNAM are parsed but not consumed (TD5-011 gate) — that's intentional, not a drop.

Parameters (from $ARGUMENTS)

  • --focus <dimensions>: comma-separated dimension numbers. Default: all.
  • --depth shallow|deep: shallow = walker contract + wiring review from source only; deep = also run the corpus harness against on-disk BSAs. Default: deep.

Extra Per-Finding Fields

  • Dimension: Walker Byte-Accounting | Placeholder Fallback | TREE→Billboard Wiring | Per-Game Variants | Tag Dictionary | NIFAL Material Translation

Phase 1: Setup

  1. mkdir -p /tmp/audit/speedtree
  2. Dedup query (per _audit-common.md): gh issue list --repo matiaszanolli/ByroRedux --limit 200 --json number,title,state,labels --search "speedtree OR spt OR TREE" > /tmp/audit/speedtree/issues.json
  3. Read the most recent docs/audits/AUDIT_SPEEDTREE_*.md report (sort by date — do not hardcode a filename here, it rots every cycle) and diff direction against it rather than re-deriving from scratch. Findings SPT-D4-01/02/03/04, SPT-D5-01/02, SPT-D2-01, SPT-D3-01, SPT-D1-01 are all closed (#994/#995/#996/#997/#998/#999/#1000/#1001/#1002). Of the later SPT-NEW batch, SPT-NEW-01 (dead-code detect_variant, #1820) and SPT-NEW-06 (format-notes byte-align, #1821) are also closed — only SPT-NEW-07 (MaybeStringElseBare misparse risk on a bare tag-13005 immediately before the geometry tail, #1822) remains open. Treat closed findings as regression guards, not open items; only re-flag if a guard has actually broken.
  4. deep only — corpus + recon harness:
    • corpus location: Fallout - Meshes.bsa (FNV/FO3), Oblivion - Meshes.bsa.
    • acceptance run: BYROREDUX_FNV_DATA=… cargo test -p byroredux-spt --release --test parse_real_spt -- --ignored --nocapture (and _FO3_DATA / _OBL_DATA).
    • per-file dumps: the recon examples are feature-gatedcargo run -p byroredux-spt --features recon --example spt_walk (also spt_tagmap, spt_transitions, spt_dissect, spt_recon). Findings log to crates/spt/docs/format-notes.md.

Phase 2: Dimensions

Ordered by SpeedTree risk.

Dimension 1: Walker Byte-Accounting

Highest risk. The walker has no length-prefixed framing for most tags — it advances by the payload kind's fixed size. A single wrong size in the dictionary desyncs every subsequent read. Entry points: crates/spt/src/parser.rs (parse_spt, read_payload, TAG_MIN/TAG_MAX), crates/spt/src/stream.rs (SptStream LE readers + is_eof/remaining guards), crates/spt/src/parser.rs tests. Checklist:

  • Each SptTagKind decode advances the cursor by exactly the bytes it claims: U8=1, U32=4, Vec3=12, FixedBytes(n)=n, String=4+len, ArrayBytes{stride}=4+count×stride, Bare=0. Cross-check read_payload against the dictionary in tag.rs for any size mismatch.
  • The MaybeStringElseBare branch (tag 13005, #999) consumes the tag, then peeks the next u32 to decide Bare vs String. Confirm it re-syncs cleanly on both arms and that a None peek (EOF) can't panic — the tag_13005_at_eof_does_not_panic regression guard.
  • Walker stops cleanly at is_eof() (sets reached_eof) and at an out-of-range / Unknown tag (records tail_offset + unknown_tags), never reads one tag past EOF.
  • Pathological lengths: read_string_lp and ArrayBytes both cap at 64 KiB and bail with Err, not OOM. Confirm the cap is on the byte count (count × stride), not just count.
  • parse_spt returns Err only on the two fatal conditions (missing magic, mid-payload underflow). In-range-but-unknown tags are non-fatal (recorded, walker stops) — this is the contract the placeholder relies on. Any new fatal-error path is a HIGH finding (it kills the cell-loader fallback).
  • Endian: LE-only, unconditional (no version-gated readers — every .spt is __IdvSpt_02_). Flag any big-endian assumption or host-endian read.

Dimension 2: Placeholder Fallback Correctness

Entry points: crates/spt/src/import/mod.rs (import_spt_scene, compute_billboard_size, placeholder_billboard_mesh, placeholder_root_node, DEFAULT_BILLBOARD_WIDTH/_HEIGHT), byroredux/src/systems/billboard.rs. Checklist:

  • import_spt_scene always returns a one-node / one-mesh ImportedScene — there is no Err path out of it. The only way the cell loader gets None is parse_spt returning Err (magic / underflow); confirm that path logs + skips the REFR without aborting the rest of the cell (graceful-degradation contract).
  • Leaf-texture precedence: TREE.ICON override → .spt tag 4003 (first wins, #997) → unset (renderer magenta placeholder). Regression guards: leaf_texture_override_wins_over_spt_tag, empty_texture_leaves_path_unset_for_renderer_placeholder.
  • Billboard sizing precedence in compute_billboard_size: OBND → BNAM → MODB → 256×512 default, every path clamped to [16, 8192] (#1001/#1002). OBND beats BNAM intentionally (BNAM clamps tall trees, e.g. WhiteOak01 OBND 802×1567 vs BNAM 768×768). Vanilla Oblivion ships MODB-only / no OBND, so an OBND-first-or-default path would render Cyrodiil pines at half scale. Guard the ordering.
  • Normal / winding convention (#1000): front-face normal is -Z, indices [0, 3, 2, 2, 1, 0]. The billboard system rotates via Quat::from_rotation_arc(-Z, look_dir), so object -Z ends up facing the camera — the textured face must be -Z. Pre-#1000 normals were +Z and two_sided: true masked it. Guards: placeholder_normals_point_negative_z_for_billboard_arc, placeholder_index_winding_produces_negative_z_geometric_normal.
  • bs_bound Z-up → Y-up swap (#995): center via byroredux_core::math::coord::zup_to_yup_pos, half-extents reshuffled (hx, hz, hy). Guard: placeholder_uses_obnd_bounds_when_present.
  • Two-sided + alpha-test cutout: two_sided: true, alpha_test: true, alpha_threshold: 0.5, alpha_test_func: 6 (GREATEREQUAL), has_alpha: false (cutout and blend are exclusive).
  • BsRotateAboutUp handling in billboard.rs::compute_billboard_rotation currently falls back to the world-up yaw lock (it lacks the local frame). Verify that approximation is still acceptable for tree imposters and documented — drifting it silently into pitch would tilt every tree.

Dimension 3: TREE → Billboard Wiring

Entry points: byroredux/src/cell_loader/references/mod.rs (is_spt dispatch) + byroredux/src/cell_loader/references/import.rs (parse_and_import_spt), byroredux/src/cell_loader/spawn.rs (placement_root_billboardBillboard::new), crates/plugin/src/esm/records/tree.rs (parse_tree, TreeRecord, has_speedtree_binary). Checklist:

  • The .spt route fires when the REFR's TREE base's MODL ends in .spt; TREE record is fetched from record_index.trees by the same form id resolved against index.statics. Mixed .nif + .spt REFRs in one cell must coexist.
  • parse_and_import_spt returns the same CachedNifImport shape as every other model, with synthetic defaults the generic spawn path must not mis-read as NIF-rooted: placement_root_billboard = Some(BsRotateAboutUp) (#994), bsx_flags = 0 (#1214), root_flags = 0 (#1235), flame_attach_offset = None. Confirm the spawn site never assumes the placeholder carries a real NiAVObject root / BSXFlags / flame marker.
  • spawn.rs actually inserts the Billboard component when placement_root_billboard.is_some() — without it the quad spawns static (this was SPT-D4-01, now closed; it's the regression guard for the whole dimension).
  • TreeRecord field capture is lossless for the fields the importer reads (OBND→bounds, ICON→leaf_texture, MODB→bound_radius, BNAM→billboard_size). SNAM/CNAM are parsed-but-not-consumed (TD5-011) — don't flag as a drop, but DO flag if they're silently mis-parsed (e.g. CNAM length not shape-tolerant across the 5-float Oblivion vs 8-float FO3/FNV split).
  • .spt files in BSAs resolve through the same extract_mesh lookup chain as .nif (sibling-BSA auto-load, AE pipeline-path strip if relevant) — no parallel "spt resolver".
  • Cell unload despawns the placeholder entities cleanly; no leaked BLAS entries for the billboard quad.

Dimension 4: Per-Game Variants & Route Divergence

Entry points: crates/spt/src/version.rs (detect_variant, SpeedTreeVariant, MAGIC_HEAD), byroredux/src/scene/nif_loader.rs (parse_import_and_merge is_spt branch). Checklist:

  • detect_variant recognises any __IdvSpt_02_-prefixed file but cannot tell V4Oblivion from V5Fnv at the magic level — it defaults to V5Fnv and the caller is meant to override via game context. Verify nothing downstream actually depends on the variant being correct today (the placeholder path is variant-agnostic); if a consumer branches on it, that's a real bug. Guards: detect_variant_recognises_idvspt_magic, detect_variant_unknown_for_non_speedtree_inputs.
  • MAGIC_HEAD is the exact 20 bytes (u32 1000, u32 12, "__IdvSpt_02_"). A one-byte flip must reject → placeholder. Confirm no partial-prefix leakage (input shorter than 20 bytes rejects).
  • Route divergence (the real per-game risk here): the cell-loader route threads TREE metadata; the --tree / loose route uses SptImportParams::default(). That means a loose-loaded Oblivion .spt gets the 256×512 default (no MODB sizing) and no ICON override. Confirm this is understood/documented and isn't masking a sizing bug that would also bite the cell route. Flag if the two routes have drifted in the parse call (they must both call parse_spt + import_spt_scene).
  • Oblivion (SpeedTree 4.x) vs FO3/FNV (5.x): the parameter walker is assumed unified across all three (same magic, same tag dictionary). The geometry-tail layout is not confirmed unified — but the tail is out-of-scope for Phase 1, so only flag a tail-decode assumption if --focus includes the future phases.

Dimension 5: Tag Dictionary

Lower risk (fixed-size decodes), but a wrong size here is the Dimension-1 desync trigger, so spot-check rather than skip. Entry points: crates/spt/src/tag.rs (SptTagKind, dispatch_tag), crates/spt/docs/format-notes.md, recon examples (spt_tagmap, spt_transitions). Checklist:

  • dispatch_tag currently maps ~120 tag values across the payload kinds. This is conservative-by-design: any tag not in the table → Unknown → walker stops cleanly. The old "~14 tags / 40-tag aspirational target" framing is stale — do not report dictionary size as a gap.
  • Cross-check a sample of fixed-size assignments against the format-notes.md 2026-05-09 table and the tag.rs unit tests (fixed_byte_payload_tags, string_payload_tags, etc.): e.g. 8003/8005/8009 = 52 B, 13008 = 11 B, 13013 = 7 B, 12002 = 16 B, 12003 = 20 B, ArrayBytes 10002 stride 1 / 10003 stride 8. A size that contradicts the observed histogram is a Dimension-1 desync waiting to happen → MEDIUM.
  • Confounder tags (4096, 5376, string-length values that fell in the tag band) must stay Unknown so the walker bails rather than misparses (unknown_for_out_of_dictionary_tags).
  • Any tag observed in the corpus at ≥1 % frequency that's still Unknown should have a format-notes.md rationale; an undocumented common-tag bail is a LOW finding.

Dimension 6: NIFAL Material Translation for Placeholders

The placeholder ImportedMesh flows through the single NIFAL boundary like any other mesh. Cross-cuts /audit-nifal — report single-boundary / no-fabrication findings there, not here. Entry points: the material defaults in crates/spt/src/import/mod.rs (placeholder_billboard_mesh), byroredux/src/material_translate.rs (translate_material, consumed at the spawn.rs call site), crates/core/src/ecs/components/material.rs (Material::resolve_pbr). Checklist:

  • The placeholder is canonicalised at the single translate_material boundary — no parallel "spt material" path that bypasses it (the --tree loose route and the cell route must both land here via spawn.rs).
  • Non-PBR defaults survive translation: is_pbr: false, from_bgsm: false (#1076/#1077); metalness_override: Some(0.0) / roughness_override: Some(0.85) — explicit foliage defaults set at import (#1819/SPT-NEW-05, placeholder_billboard_mesh in crates/spt/src/import/mod.rs), NOT None. A regression to None re-opens the keyword-classifier substring collision (Boxwood→wood, Elderberry→glass). Guard: placeholder_billboard_sets_foliage_pbr_overrides_regardless_of_texture_path. resolve_pbr must fill the canonical metalness/roughness f32 from the non-PBR keyword path, never promote the billboard to metallic-roughness. SpeedTree never resolves a BGSM/BGEM (#1241/#1353) — guard that import-side PBR plumbing (a82366e9-style) left the billboard non-PBR.
  • emissive_source: EmissiveSource::None (#1280) holds — a tree billboard must not pick up an emissive lobe.
  • The two-sided alpha-test cutout maps to the correct canonical Material flags after translation (foliage silhouette preserved, not opaque-blitted).

Phase 3: Output

Write findings to docs/audits/AUDIT_SPEEDTREE_<TODAY>.md using the base finding format from _audit-common.md. Mark anything already covered by #994–#1002 as a regression guard, not a new finding. Suggest /audit-publish on completion.

Skills similaires