# AP-156 fix review — `b52967de` + `e2b2d04c` Adversarial dual-lens review of the AP-152 flood-sphere fix, prompted by the FAIL in `docs/research/2026-08-06-ap152-review-retail.md`. Worktree `.claude/worktrees/resume-session-e0bd03e1-d5bf45`, HEAD `e2b2d04c`. All 44 `bin`/`obj` deleted before every verdict-deciding build. Six sabotages applied and restored; `git status --porcelain` empty at the end. ## VERDICTS | Lens | Verdict | |---|---| | **Retail conformance** | **PASS** | | **Architecture** | **PASS** | Three things the brief asked me to settle explicitly: - **The containment claim — BELIEVED, and true more strongly than asserted.** The shipped assertion is algebraically tautological (R1), so I re-derived containment independently against physics-polygon **vertices** rather than against the root sphere: **0 of 530** BSP-bearing Setups fail after the fix, **412** failed before it. - **The cap change — the implementer is RIGHT and both prior reviews were WRONG.** Byte-verified: the `cmp eax,0xa` clamp is inside the cylsphere overload only; the BSP walk is uncapped; `@0x0052b990` pushes a literal `1`. - **The shrink argument — CORRECT, and both prior reviews used the wrong criterion.** `calc_cross_cells`' BSP branch is reached through an unconditional jump past both primitive branches, so retail's flood for these objects contains no primitive contribution at all. "Smaller than before" is not a defect criterion; containment is. My own sweep reproduces the implementer's 143. --- ## Part 1 — Independent retail verification Method: my own Capstone disassembly of the PDB-paired binary (`C:\Users\erikn\Downloads\acclient.exe`, `check_exe_pdb.py` → **MATCH**, CodeView GUID `9e847e2f-777c-4bd9-886c-22256bb87f32`, linker 2013-09-06T00:17:56Z, image base `0x400000`), every address cross-resolved through `named-retail/symbols.json` and every struct offset cross-checked against `named-retail/acclient.h`. Not inherited from the commit, not from the prior reviews. ### 1.1 Every cited address resolves to the construct claimed — all 12 `symbols.json` lookup by address: | Address | Symbol | |---|---| | `0x0050f050` | `CPhysicsObj::FindObjCollisions` | | `0x00510fc0` | `CPhysicsObj::find_bbox_cell_list` | | `0x00515230` | `CPhysicsObj::calc_cross_cells` | | `0x00518070` / `0x00518090` | `CPartArray::GetSphere` / `GetCylsphere` | | `0x00518110` | `CPartArray::CacheHasPhysicsBSP` | | `0x00518160` | `CPartArray::calc_cross_cells_static` | | `0x00518b00` | `CPartArray::GetSortingSphere` | | `0x0052b990` / `0x0052b9f0` | `CObjCell::find_cell_list` (two overloads) | | `0x0052cae0` | `CEnvCell::find_transit_cells` | | `0x005397e0` | `BSPTREE::GetSphere` | | `0x004527f0` | `Position::localtolocal` | No mis-cited neighbour anywhere in this commit, the register rows, or the tests. (I looked specifically, given the mis-cite found earlier this session.) ### 1.2 `BSPTREE::GetSphere` @`0x005397e0` — claim 4b CONFIRMED byte-exact ``` 0x005397e0 8b01 mov eax, dword ptr [ecx] ; BSPTREE::root_node 0x005397e2 83c004 add eax, 4 ; past BSPNODE::vfptr 0x005397e5 c3 ret ``` `acclient.h`: `struct BSPNODE { BSPNODEVtbl *vfptr; CSphere sphere; Plane splitting_plane; ... }` and `struct CSphere { AC1Legacy::Vector3 center; float radius; }` → radius at `+0xc` of the returned pointer. Retail's per-part flood sphere **is** the BSP root bounding sphere, origin included. `acclient_2013_pseudo_c.txt:319128` (`0x00534b5b`): `this->physics_sphere = BSPTREE::GetSphere(this->physics_bsp);` — verbatim. ### 1.3 `physics_sphere` = `[gfxobj+0x74]`, `physics_bsp` = `[+0x78]` — claim 4c CONFIRMED `acclient.h` `CGfxObj` field order: `... use_built_mesh; CSphere *physics_sphere; BSPTREE *physics_bsp; Vector3 sort_center; unsigned num_polygons; CPolygon *polygons; CSphere *drawing_sphere; ...` — which places `physics_sphere` at `+0x74`, `physics_bsp` at `+0x78`, `drawing_sphere` at `+0x90`. Confirmed against code: ``` CPartArray::CacheHasPhysicsBSP @0x00518110 0x00518120 8b32 mov esi,[edx] ; parts[i] 0x00518122 8b7620 mov esi,[esi+0x20] ; CPhysicsPart::gfxobj 0x00518125 8b36 mov esi,[esi] 0x00518127 8b7e78 mov edi,[esi+0x78] ; physics_bsp <-- exact addr ``` and `find_transit_cells` reads `[ecx+0x74]` with a `[ecx+0x90]` fallback — the drawing sphere — which only makes sense with this layout. ### 1.4 `CEnvCell::find_transit_cells` @`0x0052cae0` — claim 4a CONFIRMED, centre-then-radius ``` 0x0052cb31 8b5020 mov edx,[eax+0x20] ; CPhysicsPart::gfxobj 0x0052cb34 8b0a mov ecx,[edx] 0x0052cb36 8b7174 mov esi,[ecx+0x74] ; physics_sphere 0x0052cb39 85f6 test esi,esi 0x0052cb3d 8bb190.. mov esi,[ecx+0x90] ; else drawing_sphere 0x0052cb4b 56 push esi ; -> &sphere->center 0x0052cb4c 83c030 add eax,0x30 ; CPhysicsPart::pos 0x0052cb4f 50 push eax 0x0052cb50 8d442434 lea eax,[esp+0x34] ; out 0x0052cb54 8d5d54 lea ebx,[ebp+0x54] ; cell->pos 0x0052cb5a e8915cf2ff call 0x4527f0 ; Position::localtolocal 0x0052cb5f d905708c7c00 fld dword [0x7c8c70] ; F_EPSILON = 1.9999999e-4 0x0052cb65 d8460c fadd dword [esi+0xc] ; ONLY NOW the radius ``` `acclient.h` `CPhysicsPart`: `CYpt(0) viewer_heading(4) degrades(0x10) deg_level(0x14) deg_mode(0x18) draw_state(0x1c) gfxobj(0x20) gfxobj_scale(0x24) pos(0x30)` — both `+0x20` and `+0x30` are exactly what the commit says they are. The call is `cellPos.localtolocal(&out, &part->pos, &sphere->center)`: the sphere's **centre** is transformed through the part's own `Position` into the cell frame, and the radius is only added afterwards to build the plane test. **Carrying the radius alone is a different sphere — the commit's thesis is right.** ### 1.5 The 10-sphere cap — settled against the implementer's claim **Claim: the cap lives only in the cylsphere overload. VERIFIED.** ``` CObjCell::find_cell_list @0x0052b9f0 (the N-sphere overload) 0x0052ba1d 8b442414 mov eax,[esp+0x14] ; count 0x0052ba21 83f80a cmp eax,0xa 0x0052ba26 760b jbe 0x52ba33 0x0052ba28 bd0a0000 mov ebp,0xa ; clamp ... copy loop into the STATIC array 0x844838..0x8448d8 (10 x 0x10 stride) ``` The clamp is a fixed static-buffer capacity, not a policy: the destination array is exactly ten 16-byte entries and the guard byte at `0x8448d8` sits immediately past it. ``` CObjCell::find_cell_list @0x0052b990 (the sorting-sphere overload) 0x0052b9d6 6a01 push 1 ; literal count of ONE ``` ``` CPhysicsObj::calc_cross_cells @0x00515230 0x00515285 test dword [esi+0xa8],0x10000 0x0051528f jne 0x515305 ------------------> 0x0051530c call 0x510fc0 (BSP) 0x005152d1 call 0x52b9f0 (cylsphere, capped) 0x005152fb call 0x52b990 (sorting sphere, one) ``` ``` CPhysicsObj::find_bbox_cell_list @0x00510fc0 loop over cells [esi+8], no clamp 0x00511012 call 0x518160 CPartArray::calc_cross_cells_static @0x00518160 0x0051816e mov esi,[eax+0x5c] ; parts 0x00518171 mov eax,[eax+0x58] ; num_parts 0x00518176 call dword [edx+0x7c] ; args (num_parts, parts, cellarray) CEnvCell::find_transit_cells @0x0052cae0 inner part loop 0x52cb22 .. 0x52cc76, bound = [esp+0x6c] = num_parts outer portal loop, bound = [ebp+0x108] NO CLAMP ANYWHERE ``` **The implementer is correct on all three counts. Both prior reviews passed a cap that retail does not have on that branch.** 7 installed Setups exceed 10 physics-BSP parts (max 49, Setup `0x02001A91` — reproduced independently), and their tail parts were previously dropped from the flood outright. The change is a correctness improvement in the same direction as the rest of the commit. ### 1.6 `CPartArray::GetSortingSphere` @`0x00518b00` (AP-157's anchor) ``` 0x00518b67 8b4654 mov eax,[esi+0x54] 0x00518b6f 83c070 add eax,0x70 ``` Exactly `[partArray+0x54] + 0x70`. `acclient.h` `CSetup` places `CSphere sorting_sphere` immediately after `step_up_height`. AP-157's citation holds. ### 1.7 Two retail facts the commit did not use, both relevant **`CObjCell::find_obj_collisions` @`0x0052b750` has no distance pre-filter.** Pseudo-C `308916-308940`: it walks `shadow_object_list` and calls `CPhysicsObj::FindObjCollisions` on every non-parented, non-self entry unconditionally. So the `maxReach` early-out at `src/AcDream.Core/Physics/TransitionTypes.cs:3763` — `sphereRadius + obj.Radius + movement.Length() + 2f` — is entirely acdream's own invention with no retail counterpart. **#333's characterisation is correct.** See A2. **Retail's own slack constant is 0.0002 m, not 2 m.** `0x0052cb5f fld dword [0x7c8c70]` reads `1.9999999494757503e-4` (`F_EPSILON`). Anchor for whoever fixes #333. **Retail's cross-cell walk ignores part scale; its collision does not.** `find_transit_cells` uses only `CPhysicsPart::pos` (`+0x30`) and never `gfxobj_scale` (`+0x24`), while `CPhysicsPart::find_obj_collisions` @`0x0050d8d0` explicitly passes `gfxobj_scale.z` into `SPHEREPATH::cache_localspace_sphere`. See R4. --- ## Part 2 — Independent DAT re-derivation Scratch console program **outside the repo**, referencing only `Chorizite.DatReaderWriter 2.1.7` — deliberately **not** `AcDream.Core`, so nothing under test is in the loop. Every number recomputed by hand from the installed `client_portal.dat`. | Quantity | Committed constant | My measurement | |---|---|---| | Setups | 5935 | **5935** | | with CylSphere | 678 | **678** | | Sphere-only, no CylSphere | 3605 | **3605** | | without any primitive | 1652 | **1652** | | with a physics-BSP part | 530 | **530** | | physics-BSP parts | 973 | **973** | | off-centre parts (`\|o\| > r/2`) | 376 | **376** | | affected (primitive + BSP) | 172 (73 cyl / 99 sph) | **172 (73 / 99)** | | deepest BSP part array | 49, `0x02001A91` | **49, `0x02001A91`** | | Setups with > 10 BSP parts | 7 | **7** | | worst part-origin offset | 20.762 m on gfx `0x010036DD` (r 27.708) | **20.762 m, gfx `0x010036DD`, r 27.708** | | affected Setups failing containment pre-fix | 170 | **170**, worst 17.345 m at scale 1.75 = **9.911 m at scale 1**, Setup `0x02000255` | | affected Setups failing post-fix | 0 | **0** | | 143-shrink claim | 143 of 172 | **143** | | max Spheres on any Setup (AP-157) | 5 | **5** (`0x020016F7`) | | non-zero `SortingSphere` (AP-157) | 4154 | **4154** | Every external constant reproduces exactly, including the 9.911 m figure once the 1.75 test scale is divided out. ### The stronger, genuinely independent containment result Instead of comparing the flood sphere to the root sphere (which is circular — see R1), I transformed **every physics-polygon vertex** of every physics-BSP part into the same world frame and asked whether some emitted flood sphere covers it: ``` VERTEX containment failures: after the fix = 0 before the fix = 412 ``` Zero across all 530 BSP-bearing Setups. **I believe the containment claim.** --- ## Part 3 — Sabotage reproduction Six of the nine claimed sabotages reproduced, including C, H and I as requested. Each applied, clean-built, run, reverted. | # | Sabotage | Claimed | Observed | |---|---|---|---| | A | `BuildFloodSpheres`: `world = partWorldPos` | 3 Core | **4 Core** (`FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint`, `_CapsCylSpheresAtTenButNeverTheBspParts`, `_RotatesTheBoundsCentre…`, `_CentresOnTheBoundsCentre…`) | | C | `FromSetup` `boundsCenter = Vector3.Zero` (the shipped defect) | 1 Core + 2 App + 1 Content | **1 Core + 2 App + 1 Content** ✓ | | D | drop `* entScale` on `boundsCenter` | 2 App + 1 Content | **2 App + 1 Content** ✓ | | E | landblock flat branch `localCenter = Zero` | 1 Core | **1 Core** ✓ | | H | `int cap = RetailSphereCap` (all branches) | 1 Core | **1 Core** ✓ | | I | cap removed from the cylsphere branch | 1 Core | **1 Core** ✓ | The claimed counts are honest; A is understated by one, i.e. coverage is slightly better than advertised (A5). **Trap encountered and avoided:** the naive form of sabotage I (`int cap = int.MaxValue`) makes `RetailSphereCap` unused, which is `error CS0219` under `TreatWarningsAsErrors`. The build fails, the stale sabotage-H DLL survives, and the run reports H's failure as I's. I caught it by grepping the build output for `Build succeeded` before every test run. Anyone re-running these must do the same. The fixtures are genuinely off-centre now. `ShadowObjectRegistryMultiPartTests.Bsp()` defaults `BoundsCenter` to `(0, 6, 0)` and requires an explicit `(0.001, 0, 0)` to get a near-concentric sphere; `LiveEntityCollisionBuilderTests.Bsp()` defaults `centerZ = 1.25`; `ShadowRegistrationOverflowTests.BspGfx()` defaults `centerZ = 0.75`. The concentric `Radius = 14f` at `LocalPosition = Zero` configuration the prior review named is gone from every flood fixture. --- ## Part 4 — `FromLandblockBspParts` (neither prior review looked) `src/AcDream.Core/Physics/ShadowShapeBuilder.cs:268-334`. - **The same discard existed.** Pre-fix the method computed only `localRadius = flat.Nodes[flat.RootIndex].BoundingSphere.Radius` (or `phys.BoundingSphere?.Radius`) and emitted the shape at `pPos`. Identical defect, on stair runs, fences and rock clusters. - **Both storage branches are fixed.** Flat (`:310-315`) and graph fallback (`:316-320`) each take `Origin` alongside `Radius`. `ShadowRegistrationOverflowTests.FromLandblockBspParts_CarriesTheScaledRootSphereCentre` asserts both explicitly; sabotage E reddens the flat branch (verified) and the same test carries the graph assertion. - **`partScale` is applied correctly.** `Matrix4x4.Decompose` yields `M = S·R·T`, so a point `v` in the part's own frame maps to root space as `pPos + rotate(v · pScale, pRot)`. The code emits `LocalPosition = pPos` (unscaled — correct, the translation is already absolute in root space), `Radius = localRadius * partScale`, `BoundsCenter = localCenter * partScale`, `Scale = partScale`. That composes exactly as `BuildFloodSpheres` and as `TransitionTypes`' BSP transform (`local * obj.Scale`, rotate, `+ obj.Position`) consume it. Also verified: the two other `new ShadowShape(...)` sites in `src/` (`LandblockPhysicsPublisher.cs:1003,1030` and `LandblockPhysicsContentBuilder.cs:658,683`) emit **Cylinder** shapes only, for which `BoundsCenter == Zero` is correct by definition. There is no third BSP producer. --- ## Part 5 — Is `BoundsCenter` the right seam? **Yes. The implementer's rebuttal of the `LocalPosition`-offset suggestion is correct, and I verified the reason rather than taking it.** `ShadowEntry.Position` is load-bearing in three distinct ways: 1. **BSP world origin.** `TransitionTypes.cs:3862-3898` transforms the mover's spheres by `Vector3.Transform(sphere.Origin - obj.Position, invRot) * invScale` and passes `worldOrigin: obj.Position` into `CollisionTraversal.FindCollisions`. The physics BSP is authored about the **part origin**, so shifting `Position` to the bounding-sphere centre would translate every collision polygon by the offset — up to 20.762 m. 2. **Broadphase reach.** `TransitionTypes.cs:3757`. 3. **Cylinder XY distance.** `TransitionTypes.cs:3760`. Moving `LocalPosition` would break (1) outright. A separate field is the only correct seam. **Consistency audit — every site that composes a shape's world placement:** | Site | Uses | Correct? | |---|---|---| | `ShadowObjectRegistry.cs:474-490` `RegisterMultiPart` → `ShadowEntry` | `LocalPosition` only | ✓ (BSP frame) | | `:578-593` `ReplaceMultiPartPayload` → `ShadowEntry` | `LocalPosition` only | ✓ | | `:1484-1500` suspended-owner restore → `ShadowEntry` | `LocalPosition` only | ✓ | | `:691-693` `BuildFloodSpheres` | `LocalPosition` **+ rotate(BoundsCenter, partWorldRot)** | ✓ | | `:773`, `:1648`, `:2231` reflood / transfer | delegate to `RegisterMultiPart` | ✓ | `BuildFloodSpheres` is the single flood site (`UpdatePosition` routes through `RegisterMultiPart`), and no site reads the old semantics. The one place the new field is *missing* and should eventually be present is `ShadowEntry` itself — that is #333 (A2). `ReplaceMultiPartPayload` deliberately does not re-flood; that matches retail (`CPartArray::SetPart` changes the part read by later tests and does not recalculate cross-cells). Not a finding. --- ## Part 6 — Is the shrink argument right? **Yes. Settle it in favour of the implementer.** Prior review F2 counted 43 Setups whose post-`4abd1b5e` flood was not a superset of the pre-`4abd1b5e` one and called it a new under-inclusive defect. My sweep puts the post-`b52967de` number at **143 of 172** — larger, exactly as the implementer says. That is not a defect, because retail's flood for a BSP-bearing object contains **no primitive contribution at all**: ``` 0x00515285 test dword [esi+0xa8],0x10000 0x0051528f jne 0x515305 ; -> find_bbox_cell_list, and it never returns ; into 0x005152d1 / 0x005152fb ``` The cylsphere branch (`0x005152d1`) and the sorting-sphere branch (`0x005152fb`) are both below that jump and unreachable from it — I read the control flow, not a summary of it. `find_bbox_cell_list` seeds the object's own cell (`0x00510fd5` → `0x00510fe2 call 0x6b4ff0`) and then walks the part array; nothing else contributes. So for `0x0200086E` — F2's worst case, whose Sphere reached `z ≈ 11.68` while its BSP parts reach `z ≈ 9.0` — **retail also reaches only `z ≈ 9.0`.** The pre-commit flood was larger because it included a primitive `calc_cross_cells` never reads. Shrinking toward the BSP set is convergence, not regression. The criterion that *does* discriminate is containment, and it now holds at vertex level for all 530 (Part 2). **What this means for the connected session** (the practical point): "props that stopped blocking" is **not** by itself a bug report. The question to ask of any such observation is whether the object's own BSP geometry still reaches the cell it stopped blocking from. If it does, that is a bug; if it does not, that is retail. Conversely, "a tall prop that still does not block" is the expected symptom of #333, not of this fix (A2). --- ## Findings — retail lens ### R1 (MEDIUM) — the shipped containment assertion is algebraically tautological `tests/AcDream.Content.Tests/InstalledSetupBspPrimitiveDispatchTests.cs:266-341`. `truth` is built at `:282-284` as `(partOrigin + rotate(b.Origin, partRot)) * EntScale` from the **same** `Bounds()` resolver, over the **same** part set (the gate at `:299` is `id => Bounds(id) is not null`), with the **same** placement-frame priority (`:259-264` mirrors `ResolvePlacementFrame`). `flood` is built at `:314-317` as `shape.LocalPosition + rotate(shape.BoundsCenter, shape.LocalRotation)`, and `FromSetup` sets `LocalPosition = partFrame.Origin * entScale`, `BoundsCenter = b.Origin * entScale`, `LocalRotation = partFrame.Orientation`. The two expressions are identical by algebra, so `Shortfall(flood) ≡ 0` for **any** DAT content and any future DAT. Concrete failure scenario: if a later change made `FromSetup` read the wrong GfxObj's bounding sphere in *both* the resolver and the emission — say a `SetupId`/`GfxObjId` mix-up inside `Bounds()` — this assertion stays green while every flood sphere in the game moves. It is still a useful change-detector (sabotages C and D redden it, verified), and `ExpectedWouldFailIfOriginDiscarded = 170` is a real, non-circular measurement of the defect. But the commit body's framing — *"an installed-DAT containment sweep asserting every emitted BSP flood sphere contains that part's real bounding sphere"* — overstates what the code proves. Cheap fix: compare against physics-polygon vertices (`gfx.PhysicsPolygons` → `gfx.VertexArray.Vertices`) instead of against the root sphere. I ran exactly that and it passes 0/530, so adopting it costs nothing and makes the assertion mean what it says. ### R2 (MEDIUM) — the defect population is understated 2.4× The commit, AP-156 and both prior reviews all scope the defect to "170 of the 172" AP-152 Setups. After `4abd1b5e` **every** BSP-bearing Setup floods from its BSP shapes only, so the discarded origin mis-placed the flood for all of them, not just the ones that also carry a primitive. Measured: ``` vertex-containment failures with the origin discarded = 412 of 530 ``` The 172 subset is AP-152's population, not AP-156's. The biggest single mover I found — `gfx 0x010036DD`, 20.762 m offset on a 27.708 m sphere, in Setup `0x0200129A` — is cited in the commit for its offset but its Setup is not necessarily in the 172. Consequence for the live gate: the user should be told the change touches **412 installed Setups**, and that the objects most likely to look different are not confined to the 73 CylSphere + 99 Sphere lists the commit body names. ### R3 (LOW) — the uncapped third branch is *further* from retail than the 10-cap was `src/AcDream.Core/Physics/ShadowObjectRegistry.cs:674`: ```csharp int cap = only == ShadowCollisionType.Cylinder ? RetailSphereCap : int.MaxValue; ``` `only == null` is the sorting-sphere branch. The commit's own justification for lifting the cap there is *"the sorting-sphere overload @0x0052b990 takes one sphere"* — which argues for a cap of **1**, not `int.MaxValue`. The cited evidence does not support the code written. Inert over installed data (I measured max 5 Spheres on any Setup, `0x020016F7`; max 7 CylSpheres), and AP-157 registers the substitution honestly. But the comment block at `:664-673` reads as though it justifies the whole line when it only justifies the BSP half. ### R4 (LOW) — acdream scales the flood sphere; retail does not — unregistered `ShadowShapeBuilder.cs:217` (`boundsCenter * entScale`) and `:330` (`localCenter * partScale`) are new in this commit. Retail's `find_transit_cells` uses only `CPhysicsPart::pos` (`+0x30`) and never touches `gfxobj_scale` (`+0x24`), while `CPhysicsPart::find_obj_collisions` @`0x0050d8d0` explicitly threads `gfxobj_scale.z` into `SPHEREPATH::cache_localspace_sphere`. Retail's cross-cell walk is therefore under-inclusive for scaled parts and acdream's is not. Radius scaling predates this commit; centre scaling does not. Direction is over-inclusive for `scale > 1` (safe) and under-inclusive for `scale < 1` (the #98/#168 direction). Deserves a sentence in AP-156's residual paragraph; the row currently lists only the sphere-vs-portal traversal. ### R5 (INFO) — retail's slack constant, for whoever fixes #333 `0x0052cb5f fld dword [0x7c8c70]` = `1.9999999494757503e-4` — the same `F_EPSILON` AP-30 already byte-confirmed for `Frame::is_equal`. Retail's cross-cell sphere test is `radius + 0.0002`, not `radius + 2`. --- ## Findings — architecture lens ### A1 (MEDIUM) — `BoundsCenter = default` reopens, at the type, the exact hole the commit closed at the seam `src/AcDream.Core/Physics/ShadowShape.cs:61`: ```csharp Vector3 BoundsCenter = default); ``` The commit's central structural claim is that *"the gate and the geometry cannot disagree, and the radius cannot be taken while the origin is dropped. That split is what produced this bug; it no longer exists."* That is true at the `LiveEntityCollisionBuilder`/`FromSetup` seam — genuinely well done, and the single-resolver collapse is the right call. It is **not** true of `ShadowShape` itself. This still compiles today: ```csharp new ShadowShape(gfxId, pos, rot, scale, ShadowCollisionType.BSP, radius, 0f) ``` and silently reproduces AP-156 with a zero centre. Every current BSP producer passes it, so nothing is broken now — but a future BSP producer (a new static publisher, a plugin-facing builder, a bake path) gets the old bug for free, green. Concrete failure scenario: someone adds a BSP branch to `LandblockPhysicsContentBuilder` (which today emits Cylinders at `:658,683` with no `BoundsCenter`), copies the existing 7-argument call shape, and reintroduces a 20 m flood mis-placement with no test failing. Fix: drop the default and pass `Vector3.Zero` explicitly at the two primitive sites in `ShadowShapeBuilder` and the four in the publishers. Six call sites, and the type then enforces the invariant the commit body claims. ### A2 (MEDIUM) — #333 is a real defect, correctly identified, but deferring it can mask this fix's entire visible benefit `e2b2d04c` is honest and well-reasoned: the broadphase at `TransitionTypes.cs:3757-3765` measures `currPos - obj.Position` (the **part origin**) against `sphereRadius + obj.Radius + movement + 2f`, where `obj.Radius` is the root sphere's radius measured about a centre that may be metres away. Same discarded origin, one layer down. I confirmed the two things the issue could not: - **Retail has no such filter at all.** `CObjCell::find_obj_collisions` @`0x0052b750` (pseudo-C 308916-308940) dispatches every non-parented, non-self shadow object to `CPhysicsObj::FindObjCollisions` unconditionally. The `+ 2f` and the movement term are acdream's own. The issue's "looks like acdream's own broadphase rather than a port" is **correct**, and this is an unregistered divergence that predates the commit — it should carry an AP row, not only an issue number. - **Its blast radius is large.** A genuine surface contact is rejected when `|offset| > sphereRadius_mover + movement + 2` — roughly 2.5 m for a walking player. **118 of the 477 unique installed physics-BSP GfxObjs** have a root sphere offset above 2.5 m, and **46** above 5 m. At the test scale 1.75 those become 4.4 m and 8.75 m against an unchanged ~2.5 m budget. The issue's own one-line summary is slightly imprecise — it says contact is admitted "only when the sphere's centre is within about `movement + 2` metres of the part origin", where the correct statement is about the **mover's** distance from the part origin versus `R + r_m + move + 2`. The conclusion is unchanged. Deferral judgement: the split is defensible on attributability grounds (it is literally the AP-155 lesson), and I would not have bundled a *retail-question* fix. But the practical consequence must be stated plainly to the user rather than left in an issue body: **for the tall props with the largest offsets — exactly the objects the commit body tells the user to go look at — the fix may produce no visible change at all**, because the geometry now lands in the right cell and is then discarded by the filter. If the connected gate is run before #333, a null result on tall props is expected, not evidence against AP-156. The minimal correct fix is mechanical (carry `BoundsCenter` on `ShadowEntry` and measure from `obj.Position + rotate(BoundsCenter, obj.Rotation)`); only the `+ 2f` question is genuinely open, and it is open independently of that. ### A3 (LOW) — a cached field became a per-call delegate allocation `src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:136`: ```csharp id => _physicsBspBounds(id) is not null, ``` replaces the cached `_hasPhysicsBsp` field. The lambda captures `this`, so Roslyn allocates a fresh `Func` on every `Build` call. `Build` is per-entity-spawn / per-appearance-rebuild, not per-frame, so this is small — but the project has spent a whole slice (I1) driving physics-path resolves to 0 B, and a `private readonly Func _hasBounds` initialised once in both constructors costs nothing and keeps the single-resolver invariant intact. Also: the new resolver indexes `flat.Nodes[flat.RootIndex]` where the old gate only compared `RootIndex >= 0`. A `FlatPhysicsBsp` with `RootIndex >= 0` and an empty `Nodes` array would now throw where it previously returned `true`. I found no way to construct one from `FlatCollisionAssetBuilder`, so this is a note, not a defect. ### A4 (LOW) — uncapping the BSP branch raises a per-tick cost for moving remotes `RuntimeRemotePhysicsUpdater.cs:482` re-floods a remote through `RegisterMultiPart` on every tick it actually moves, and `RegisterMultiPart` writes every shape row into every flooded cell. Lifting the cap changes the flood input from ≤10 spheres to up to 49, and the row count is `parts × cells` — both factors grow together. Bounded in practice: only 7 installed Setups exceed 10 BSP parts, landblock part arrays are static and register once, and this is what retail does (`add_shadows_to_cells` @`0x00514ae0` → `CPartArray::AddPartsShadow` walks the whole part array into every cross-cell). Worth a glance in the connected session if a dense indoor scene with a many-part animate prop regresses; not worth pre-emptive work. ### A5 (LOW) — sabotage A reddens 4 Core tests, not 3 Reproduced: `FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint`, `BuildFloodSpheres_CapsCylSpheresAtTenButNeverTheBspParts`, `BuildFloodSpheres_BspShape_RotatesTheBoundsCentreByThePartRotation`, `BuildFloodSpheres_BspShape_CentresOnTheBoundsCentreNotThePartOrigin`. Coverage is one better than the commit body claims; the ledger is simply stale by one line. ### A6 (LOW) — the Content test's "cap control" does not test the cap `InstalledSetupBspPrimitiveDispatchTests.cs:302-308` comments that *"EVERY BSP shape contributes … capping here would silently exclude their tail from the containment claim below"*, and `:365` asserts `mostBspShapesOnOneSetup == 49`. But that loop (`:311-319`) is the test's own re-implementation of the composition with **no cap**, so it cannot observe a cap regression in `BuildFloodSpheres`. I confirmed empirically: under sabotage H the Content suite stayed 127/127 green. The cap is genuinely covered — by `BuildFloodSpheres_CapsCylSpheresAtTenButNeverTheBspParts` (sabotages H and I both redden it, verified). The comment just claims coverage that lives elsewhere. --- ## Gates reproduced All after deleting all 44 `bin`/`obj`. | Gate | Claimed | Observed | |---|---|---| | Release build | 0 errors, 21 pre-existing warnings | **0 errors, 21 warnings** ✓ | | Complete suite | 11,208 / 4 skipped / 0 failed | **11,208 / 4 / 0** ✓ | | Core | 4268 | **4268** (1 skip) ✓ | | Content | 127 | **127** (0 skips) ✓ | | App | unchanged, one rename | **4173** (3 skips) ✓ | | Delta vs `4abd1b5e` | +5 | consistent: +4 Core (3 in `ShadowObjectRegistryMultiPartTests`, 1 in `ShadowRegistrationOverflowTests`) +1 Content, App rename only. Baseline **not** independently re-run — I did not check out `4abd1b5e` in a worktree another agent may share. | | Active AP register rows | 109 | **109**, literal count of non-struck `AP-` rows in section 3 ✓ (AD 48, IA 18, UN 4) | Suite run twice — once on the pristine tree before any sabotage, once on a fully clean rebuild after every restore. Identical both times. No flake from #302 / #308 / #321 was hit; nothing was conflated with them. ## Rules compliance - **No workaround, suppression flag, grace period, retry loop or symptom guard** anywhere in the diff. The fix is at the source: the resolver that answers "does this dispatch as BSP?" is now the same one that answers "where and how big is its sphere?". - **No new skips.** 4 skipped, all pre-existing. - **No test weakened.** `BspOnlyPart_UsesRealScaledPhysicsBoundingRadius` → `…BoundingSphere` is a rename that **adds** an assertion; the `LiveAppearanceAnimationTests` and `Builder()` edits drop a now-merged parameter with no behavioural change. Every flood fixture moved from concentric to off-centre, which is strictly stronger. - **Register discipline honoured.** AP-155 narrowed with its false direction corrected, AP-156 and AP-157 filed, AP-152 retired, count reconciles at a literal 109 — all in the same commit as the code. This is the register rule working as designed: AP-155(b)'s recorded direction was the *stated reason* the residual was safe to defer, and it was backwards. ## What I checked and found clean, so the PASS is auditable - All 12 cited retail addresses resolve to the named symbol; no mis-cite. - `BSPTREE::GetSphere`, `find_transit_cells`, `find_bbox_cell_list`, `calc_cross_cells_static`, `calc_cross_cells`, both `find_cell_list` overloads, `CacheHasPhysicsBSP`, `GetSortingSphere` — all disassembled from the PDB-paired binary, not inherited. - `CGfxObj`, `CPhysicsPart`, `BSPNODE`, `CSphere`, `CSetup` field offsets cross-checked against `acclient.h` and against the code that reads them. - Every `new ShadowShape(...)` site in `src/` (8) classified; only two emit BSP and both carry the centre. - Every site that composes a shape's world placement (5) classified; none reads the old semantics. - Every `BuildShadowCellSet` caller (2) classified; the single-shape `Register` overload has no production BSP caller. - `FlatPhysicsBsp.Nodes[RootIndex].BoundingSphere` traced to `gfxObj.PhysicsBSP.Root.BoundingSphere` in both `PhysicsDataCache` population paths (`:216`, `:281-287`) and in `FlatCollisionAssetBuilder` (`:84`, `:393`) — acdream's seam is retail's `physics_sphere`. - `ReplaceMultiPartPayload`'s no-reflood behaviour checked against `CPartArray::SetPart` semantics — retail-faithful. - Quaternion composition order (`entityWorldRot * s.LocalRotation`, then `Vector3.Transform(BoundsCenter, partWorldRot)`) checked against the inverse transform in `TransitionTypes.cs:3859-3876` — consistent. - Six sabotages reproduced; tree confirmed clean by `git status --porcelain` before and after. --- ## Recommended, in priority order 1. Tell the user the live-gate criterion is **containment, not size**, and that "a prop stopped blocking" is only a bug if its own BSP geometry still reaches the cell (Part 6). State the population as **412 Setups** (R2). 2. Warn that tall props with large offsets may show **no change** until #333 lands, and that a null result there is expected (A2). 3. Fix #333 — it is mechanical apart from the `+ 2f` question — and give the `maxReach` filter its own AP row, since retail has no such filter at all (A2). 4. Make `ShadowShape.BoundsCenter` required (A1). 5. Swap the Content containment oracle to physics-polygon vertices (R1) — it passes today, costs nothing, and makes the load-bearing assertion mean what it says. 6. Add the scale sentence to AP-156's residual paragraph (R4).