refactor: replace untested SpriteBank slot-33 port with panic, trap leaves

Two related changes addressing the SpriteBank-side concern raised after
the slot 33 port:

1. **Replace `sprite_bank_get_frame_for_blit` body with a panic.** The
port shipped in 973f234 was disassembly-derived but never executed
in any test (a `panic!()` planted there during headful play was
never triggered). Per the project rule against keeping unverified
code, the body has been removed; the function is now a stub that
panics with a clear "should never be reached" message pointing
back at 973f234 for the unverified Ghidra translation if banks
ever become live.

Why we believe the path is structurally dead in shipping WA:
`SpriteBank` objects are constructed only by `SpriteBank__Constructor`
(0x4F9450, verified by xrefs to vtable 0x664180), whose only caller
is `DisplayGfx::LoadSpriteEx` (slot 30, 0x523310), which has zero
callers in WA.exe and zero in Rust and is itself trapped via
`install_trap!("DisplayGfx__LoadSpriteEx", ...)`. Therefore
`DisplayBase::sprite_banks[id]` is always null and the bank branch
of slot 33 is unreachable. Confirmed by the panic-test above.

`bitgrid_set_external_buffer` (the helper used only by the bank
path) is removed alongside the body — git history preserves it.

2. **Trap the four slot-33 leaf functions on the WA side**:
- `Sprite__GetFrameForBlit` (0x4FAD30)
- `SpriteBank__GetFrameForBlit` (0x4F9710)
- `FrameCache__Allocate` (0x4FA950)
- `DisplayBitGrid::SetExternalBuffer` (0x4F6470)

Each one's only callers (verified via xrefs) were inside the slot
33 dispatch chain, which is now bypassed by the
`vtable_replace!(get_sprite_frame_for_blit => ...)` from the
previous commit. If any of these traps fires in normal play, it
means we missed a still-live caller path and need to investigate.

`Sprite_LZSS_Decode` (0x5B29E0) is intentionally NOT trapped — it
has independent callers `IMG_Decode` (0x4F5F80) and `FUN_0048EE32`
reachable from `DDGame__Constructor`, `SoundEmitter`, and
`LoadHudAndWeaponSprites`, all still bridged.

Two new address constants added for the trap targets:
- `FRAME_CACHE_ALLOCATE` in `render::sprite::frame_cache`
- `DISPLAY_BIT_GRID_SET_EXTERNAL_BUFFER` in `bitgrid`

Both re-exported via `address::va`.

Verification:
- 135/135 unit tests pass.
- 15/15 headless replay tests pass (traps installed but headless
doesn't exercise the render path, so this only confirms
install_display succeeds and nothing else broke).
- Headful verification with traps in place is **still required** to
confirm none of the four traps fire during actual frame rendering.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

crates/openwa-core/src/address.rs

@@ -73,7 +73,7 @@ pub mod va {
     pub use crate::bitgrid::{
         BitGridBaseVtable, BitGridCollisionVtable, BitGridDisplayVtable, BIT_GRID_BASE_VTABLE,
         BIT_GRID_COLLISION_VTABLE, BIT_GRID_DISPLAY_VTABLE, BIT_GRID_INIT, BLIT_SPRITE_RECT,
-        DRAW_LINE_CLIPPED, DRAW_LINE_TWO_COLOR,
+        DISPLAY_BIT_GRID_SET_EXTERNAL_BUFFER, DRAW_LINE_CLIPPED, DRAW_LINE_TWO_COLOR,
     };
     pub use crate::frontend::map_view::MAP_VIEW_VTABLE;
     pub use crate::input::controller::INPUT_CTRL_VTABLE;
@@ -90,6 +90,7 @@ pub mod va {
     pub use crate::render::palette::{
         PALETTE_CONTEXT_INIT, PALETTE_CONTEXT_INIT_RANGE, PALETTE_CONTEXT_MAP_COLOR,
     };
+    pub use crate::render::sprite::frame_cache::FRAME_CACHE_ALLOCATE;
     pub use crate::render::sprite::sprite::SPRITE_BANK_VTABLE;
     pub use crate::render::sprite::sprite::{
         CBITMAP_VTABLE_MAYBE, CONSTRUCT_SPRITE, DESTROY_SPRITE, FREE_SPRITE_OBJECT,

crates/openwa-core/src/bitgrid.rs

@@ -50,6 +50,12 @@ crate::define_addresses! {
         fn DRAW_LINE_CLIPPED = 0x004F_7500;
         /// Two-color line draw on 8bpp BitGrid (0x4F7A60)
         fn DRAW_LINE_TWO_COLOR = 0x004F_7A60;
+        /// `DisplayBitGrid::SetExternalBuffer` (FUN_004F6470) — fastcall
+        /// `(ECX=height, EDX=width, stack=(bitgrid, data, row_stride))`.
+        /// Updates an external-buffer bitgrid's data pointer + dimensions
+        /// + clip rect. Only known caller is `SpriteBank__GetFrameForBlit`,
+        /// which is itself unreachable in shipping WA — see slot 33.
+        fn/Fastcall DISPLAY_BIT_GRID_SET_EXTERNAL_BUFFER = 0x004F_6470;
     }
 }
 

crates/openwa-core/src/render/display/vtable.rs

@@ -317,18 +317,16 @@ pub struct DisplayGfxVtable {
     /// | `out_left`/`top`/`right`/`bot` | frame bounding box within the cell               |
     /// | `out_anim_frac`                | sub-frame interpolation value (Fixed16) or 0     |
     ///
-    /// **Ported.** Native Rust impl is [`get_sprite_frame_for_blit`]
-    /// in this file (Sprite path → [`sprite_get_frame_for_blit`],
-    /// SpriteBank path → [`sprite_bank_get_frame_for_blit`]). Wired via
-    /// `vtable_replace!` in `install_display`. With this slot ported,
-    /// the entire in-game render pipeline of a normal Worms match runs
-    /// on Rust code with zero bridges into WA's render functions.
-    ///
-    /// **SpriteBank path is suspected dead in normal gameplay** — adding
-    /// a `panic!()` to `sprite_bank_get_frame_for_blit` and playing
-    /// several turns of a normal match did not trigger it. The path is
-    /// still ported (per the disassembly) as a defensive measure for any
-    /// obscure game state we haven't sampled.
+    /// **Ported (Sprite path only).** Native Rust impl is
+    /// [`get_sprite_frame_for_blit`] in this file: Sprite path →
+    /// [`sprite_get_frame_for_blit`], SpriteBank path →
+    /// [`sprite_bank_get_frame_for_blit`] (a `panic!()` stub — see its
+    /// docstring for the structural-deadness argument). Wired via
+    /// `vtable_replace!` in `install_display`.
+    ///
+    /// With this slot ported, the entire in-game render pipeline of a
+    /// normal Worms match runs on Rust code with zero bridges into WA's
+    /// render functions.
     ///
     /// # Porting blueprint (kept for archival reference)
     ///
@@ -1664,187 +1662,56 @@ unsafe fn sprite_get_frame_for_blit(
     bitgrid
 }
 
-/// Pure-Rust port of `DisplayBitGrid::SetExternalBuffer` (0x4F6470).
-///
-/// Sets a `DisplayBitGrid`'s data pointer, dimensions, row stride, and
-/// clip rect (full-rect: `(0, 0, width, height)`). Returns `false` if the
-/// bitgrid's `external_buffer` flag is currently zero (i.e. it owns its
-/// own buffer and the caller has no business swapping it out).
-///
-/// Used by the SpriteBank path of slot 33 to point the bank's embedded
-/// `frame_bitgrid` (at `bank + 0x130`) at the freshly-decompressed
-/// subframe pixels. Inlined manually in the Sprite path because that one
-/// uses `frame_w` for both `width` and `row_stride`.
-#[inline]
-unsafe fn bitgrid_set_external_buffer(
-    bitgrid: *mut DisplayBitGrid,
-    data: *mut u8,
-    row_stride: u32,
-    width: u32,
-    height: u32,
-) -> bool {
-    if (*bitgrid).external_buffer == 0 {
-        return false;
-    }
-    (*bitgrid).data = data;
-    (*bitgrid).row_stride = row_stride;
-    (*bitgrid).width = width;
-    (*bitgrid).height = height;
-    (*bitgrid).clip_left = 0;
-    (*bitgrid).clip_top = 0;
-    (*bitgrid).clip_right = width;
-    (*bitgrid).clip_bottom = height;
-    true
-}
-
-/// Pure-Rust port of `SpriteBank__GetFrameForBlit` (0x4F9710).
-///
-/// SpriteBank-side counterpart of [`sprite_get_frame_for_blit`]: walks
-/// the bank's `index_table` to map `sprite_id` to a frame in
-/// `frame_table`, resolves the animation value to a sub-frame index in
-/// `bbox_table`, lazily decompresses the corresponding subframe via
-/// [`frame_cache_allocate`] + [`sprite_lzss_decode`], updates the
-/// embedded `frame_bitgrid` (at `bank + 0x130`), and returns a pointer
-/// to it. Returns null on any validation failure (null `frame_table` or
-/// out-of-range `entry_idx`).
-///
-/// Note the structural differences from the Sprite path:
-/// - Animation metadata lives in [`SpriteBankFrame`]
-///   (`flags/width/height/base_frame_idx/scale_or_count`) instead of
-///   `Sprite::flags`/`max_frames`/`scale_*`.
-/// - Out-`w`/`h` come from the per-frame `width`/`height`, NOT from a
-///   sprite-wide cell dimension.
-/// - The bbox table is a separate array of [`SpriteBankBboxEntry`] at
-///   `bank + 0x18`, with `subframe_idx` at `+0` (low u16 of what would
-///   be `SpriteFrame::bitmap_offset`) and `decoded_offset` at `+2`.
-/// - The subframe cache uses [`SpriteBankSubframeCache`] with
-///   `decoded_ptr` at `+8` (vs `+4` in `SpriteSubframeCache`).
-/// - The LUT for LZSS is the inline 256-byte `palette_lut` at
-///   `bank + 0x30`, not a separately-allocated palette pointer.
-/// - The bitgrid update goes through `bitgrid_set_external_buffer`
-///   instead of inline writes.
-///
-/// # Safety
-///
-/// `bank` must point at a valid, fully-parsed [`SpriteBank`]. Output
-/// pointers must be writable.
+/// Stub for `SpriteBank__GetFrameForBlit` (0x4F9710).
+///
+/// **Structurally unreachable in shipping WA.** The only function that
+/// constructs a `SpriteBank` object is `SpriteBank__Constructor`
+/// (0x4F9450), and the only caller of *that* is
+/// `DisplayGfx::LoadSpriteEx` (slot 30, 0x523310), which has zero
+/// callers in WA.exe and zero in Rust and is currently trapped via
+/// `install_trap!("DisplayGfx__LoadSpriteEx", ...)` in `install_display`.
+///
+/// Therefore `DisplayBase::sprite_banks[id]` is always null in shipping
+/// WA, and the `bank.is_null()` check in [`get_sprite_frame_for_blit`]
+/// always succeeds. The bank branch of slot 33 is dead code in normal
+/// gameplay — confirmed by adding a `panic!()` here and playing several
+/// turns of a regular Worms match without triggering it.
+///
+/// A previous revision of this function (commit `973f234`) contained a
+/// disassembly-derived port of the bank-side logic, but it was untested
+/// because no replay or live play could reach it. Per the project rule
+/// against keeping unverified code, the body has been replaced with a
+/// panic — if any future change to WA, mods, or our ports starts
+/// producing live `SpriteBank` objects, this panic will fire and we'll
+/// need to revive the port (see git history for the unverified Ghidra
+/// translation, which would then need bridge-and-compare validation
+/// against the original WA function at 0x4F9710).
+///
+/// The slot 33 dispatcher still calls this stub when `sprite_banks[id]`
+/// is non-null so we get a clear, source-located error rather than
+/// silently returning null (which would just present as missing
+/// graphics with no diagnostic).
 #[allow(clippy::too_many_arguments)]
 unsafe fn sprite_bank_get_frame_for_blit(
-    bank: *mut SpriteBank,
+    _bank: *mut SpriteBank,
     sprite_id: u32,
-    mut anim_value: u32,
-    out_anim_frac: *mut u32,
-    out_w: *mut i32,
-    out_h: *mut i32,
-    out_left: *mut i32,
-    out_top: *mut i32,
-    out_right: *mut i32,
-    out_bottom: *mut i32,
+    _anim_value: u32,
+    _out_anim_frac: *mut u32,
+    _out_w: *mut i32,
+    _out_h: *mut i32,
+    _out_left: *mut i32,
+    _out_top: *mut i32,
+    _out_right: *mut i32,
+    _out_bottom: *mut i32,
 ) -> *mut DisplayBitGrid {
-    // ── Index lookup + bounds check ──────────────────────────────────
-    let entry_idx = *(*bank)
-        .index_table
-        .offset((sprite_id as i32 - (*bank).base_id) as isize);
-    if (*bank).frame_table.is_null() || entry_idx < 0 || entry_idx >= (*bank).frame_count {
-        return core::ptr::null_mut();
-    }
-    let frame = (*bank).frame_table.add(entry_idx as usize);
-    let frame_flags = (*frame).flags;
-
-    // ── Clamp anim_value ─────────────────────────────────────────────
-    if frame_flags & 1 != 0 {
-        anim_value &= 0xFFFF;
-    } else {
-        let signed = anim_value as i32;
-        if signed < 0 {
-            anim_value = 0;
-        } else if signed >= 0x10000 {
-            anim_value = 0xFFFF;
-        }
-    }
-
-    // ── Ping-pong ────────────────────────────────────────────────────
-    if frame_flags & 2 != 0 {
-        anim_value &= 0xFFFF;
-        anim_value = anim_value.wrapping_mul(2);
-        if anim_value >= 0x10000 {
-            anim_value = 0x1FFFE - anim_value;
-        }
-    }
-
-    // ── Frame index resolution ───────────────────────────────────────
-    let scale_or_count = (*frame).scale_or_count;
-    let base_frame_idx = (*frame).base_frame_idx;
-
-    let resolved_idx: u32 = if scale_or_count & 0x8000 != 0 {
-        // Scaled mode: low/high 7-bit nibbles of `scale_or_count` form a
-        // signed Fixed16 pair. The asm extracts both via:
-        //   `value = (low_or_high_7 << 16) >> 5` (signed)
-        // i.e. shift the 7-bit value into the high half, sign-extend
-        // from bit 22, then shift right by 5 → effective `<< 11` with
-        // sign extension from bit 6 of the original 7-bit value.
-        let low7 = scale_or_count & 0x7F;
-        let high7 = (scale_or_count >> 8) & 0x7F;
-        let low_val = (((low7 as u32) << 16) as i32) >> 5;
-        let high_val = (((high7 as u32) << 16) as i32) >> 5;
-        // Note: the asm computes `EAX = low - high` (NOT high - low) —
-        // verified at 0x4F97AD `SUB EAX, EDI` where EAX = low_val and
-        // EDI = high_val.
-        let diff = low_val.wrapping_sub(high_val);
-        let prod = (diff as i64).wrapping_mul((anim_value as i32) as i64);
-        let interp = ((prod >> 16) as i32).wrapping_add(high_val);
-        *out_anim_frac = interp as u32;
-        // Scaled mode uses `base_frame_idx` directly as the bbox index.
-        base_frame_idx as u32
-    } else {
-        // Non-scaled mode: subframe_idx = ((scale_or_count * anim_value) >> 16)
-        // + base_frame_idx. *out_anim_frac = 0.
-        let prod = (scale_or_count as i32).wrapping_mul(anim_value as i32);
-        *out_anim_frac = 0;
-        ((prod >> 16) as u32).wrapping_add(base_frame_idx as u32)
-    };
-
-    // ── Read bbox metadata ───────────────────────────────────────────
-    let bbox = (*bank).bbox_table.add(resolved_idx as usize);
-    *out_left = (*bbox).start_x as i32;
-    *out_top = (*bbox).start_y as i32;
-    *out_right = (*bbox).end_x as i32;
-    *out_bottom = (*bbox).end_y as i32;
-    // out_w/out_h come from the per-frame width/height, NOT the bbox.
-    *out_w = (*frame).width as i32;
-    *out_h = (*frame).height as i32;
-    let frame_w = (*out_right - *out_left) as u32;
-    let frame_h = (*out_bottom - *out_top) as u32;
-
-    // ── Subframe cache lookup + lazy decompress ──────────────────────
-    let subframe_idx = (*bbox).subframe_idx as u32;
-    let entry = (*bank).subframe_cache_table.add(subframe_idx as usize);
-
-    if (*entry).decoded_ptr.is_null() {
-        let context_ptr = (*bank).context_ptr;
-        let decoded_size = (*entry).decoded_size;
-        let decoded = frame_cache_allocate(
-            decoded_size,
-            context_ptr,
-            bank as *mut core::ffi::c_void,
-            subframe_idx,
-        );
-        (*entry).decoded_ptr = decoded;
-        let src = (*bank)
-            .bitmap_data_ptr
-            .add((*entry).compressed_offset as usize) as *mut u8;
-        // Bank LUT is the inline 256-byte palette_lut at bank+0x30.
-        let lut = (&raw const (*bank).palette_lut) as *const u8;
-        sprite_lzss_decode(decoded, src, lut);
-    }
-
-    // ── Update embedded bitgrid via SetExternalBuffer ────────────────
-    //
-    // Surface address = bbox.decoded_offset + entry.decoded_ptr.
-    let surface = (*entry).decoded_ptr.add((*bbox).decoded_offset as usize);
-    let bitgrid = &raw mut (*bank).frame_bitgrid;
-    bitgrid_set_external_buffer(bitgrid, surface, frame_w, frame_w, frame_h);
-    bitgrid
+    panic!(
+        "SpriteBank::GetFrameForBlit reached for sprite_id={sprite_id}, \
+         but `DisplayBase::sprite_banks` was supposed to be unreachable \
+         (LoadSpriteEx slot 30 is trapped). The bank-side port was \
+         removed because it was unverifiable; revive it from commit \
+         973f234 if banks have become live and validate against \
+         0x4F9710 by bridge-and-compare."
+    );
 }
 
 /// Port of `DisplayGfx::GetSpriteFrameForBlit` (vtable slot 33, 0x5237C0).

crates/openwa-core/src/render/sprite/frame_cache.rs

@@ -23,6 +23,18 @@ use crate::render::display::base::FrameCache;
 use crate::render::display::base::FrameCacheEntry;
 use crate::render::SpriteCache;
 
+crate::define_addresses! {
+    class "FrameCache" {
+        /// `FrameCache__Allocate` (FUN_004FA950) — usercall LRU
+        /// allocator. `EAX = entry_size`, stack args
+        /// `(context_ptr, owner, frame_idx)`. Ported to
+        /// [`frame_cache_allocate`]; only callers were
+        /// `Sprite__GetFrameForBlit` and `SpriteBank__GetFrameForBlit`,
+        /// both bypassed by the slot 33 vtable_replace.
+        fn/Usercall FRAME_CACHE_ALLOCATE = 0x004F_A950;
+    }
+}
+
 /// Eviction callback signature — `vtable[1]` of both `Sprite` and
 /// `SpriteBank`. Called when the oldest entry is being dropped from the
 /// LRU ring buffer; the callback clears the owner's per-subframe

crates/openwa-dll/src/replacements/render.rs

@@ -1370,6 +1370,37 @@ fn install_display() -> Result<(), String> {
         // own Rust code (no `bind_DisplayVtable!` invocation, no direct call).
         // Trap it so we'll be alerted the moment any future caller appears.
         hook::install_trap!("DisplayGfx__LoadSpriteEx", va::DISPLAY_GFX_LOAD_SPRITE_EX);
+
+        // ── Slot 33 leaf functions: trap the WA-side originals ────────
+        //
+        // After `vtable_replace!` swapped slot 33 to our Rust impl, the
+        // original `DisplayGfx::GetSpriteFrameForBlit` (0x5237C0) is no
+        // longer reached, which means its leaves have no remaining
+        // callers in shipping WA either:
+        //
+        // - `Sprite__GetFrameForBlit` (0x4FAD30): only caller was the
+        //   original slot 33 dispatcher.
+        // - `SpriteBank__GetFrameForBlit` (0x4F9710): same. Also bank
+        //   objects are themselves unreachable (LoadSpriteEx is trapped).
+        // - `FrameCache__Allocate` (0x4FA950): only callers were the
+        //   two GetFrameForBlit helpers above.
+        // - `DisplayBitGrid::SetExternalBuffer` (0x4F6470): only caller
+        //   was `SpriteBank__GetFrameForBlit`.
+        //
+        // `Sprite_LZSS_Decode` (0x5B29E0) is NOT trapped — it has
+        // independent callers (`IMG_Decode`, `FUN_0048EE32`) reachable
+        // from `DDGame__Constructor`/`SoundEmitter`/`LoadHudAndWeaponSprites`,
+        // all still bridged. Trapping it would break those.
+        hook::install_trap!("Sprite__GetFrameForBlit", va::SPRITE_GET_FRAME_FOR_BLIT);
+        hook::install_trap!(
+            "SpriteBank__GetFrameForBlit",
+            va::SPRITE_BANK_GET_FRAME_FOR_BLIT
+        );
+        hook::install_trap!("FrameCache__Allocate", va::FRAME_CACHE_ALLOCATE);
+        hook::install_trap!(
+            "DisplayBitGrid__SetExternalBuffer",
+            va::DISPLAY_BIT_GRID_SET_EXTERNAL_BUFFER
+        );
     }
 
     Ok(())