# AP-152 retail-conformance review — commit `4abd1b5e` **Reviewer role:** adversarial retail-conformance. **Scope:** `ShadowShapeBuilder.FromSetup` step-0 dispatch gate, `ShadowObjectRegistry.BuildFloodSpheres` priority, the AP-152 retirement text, and the AP-153/AP-154/AP-155 filings. **Method:** every retail claim re-derived from the PDB-paired binary (`C:\Users\erikn\Downloads\acclient.exe`, `check_exe_pdb.py` → `=== MATCH ===`, linker 2013-09-06T00:17:56Z, CodeView GUID `9e847e2f-777c-4bd9-886c-22256bb87f32`) with capstone + `pefile`, resolving every address through `named-retail/symbols.json`. Neither the commit body's nor the contract's quoted disassembly was used as input. Population facts re-measured independently against the installed `client_portal.dat` with a scratchpad `Chorizite.DatReaderWriter` sweep (not the repo's test). --- ## VERDICT: **FAIL** The retail port itself is faithful — **every** disassembly claim in the commit and in the AP-152 retirement text checks out byte-exact, and the change makes acdream match `CPhysicsObj::FindObjCollisions` and `CPhysicsObj::calc_cross_cells` where it previously did not. The failure is not in the dispatch port. It is that the commit **moves 172 Setups onto a flood-sphere approximation whose direction the register records backwards**, and the false direction is the stated reason the residual was safe to defer. Measured over the installed DATs: the new flood fails to contain the object's own BSP bounding sphere for **170 of the 172** affected Setups, shortfall up to **9.911 m**, and for **43 of them** the flood is strictly *smaller* than what the code produced before this commit. That is the #98 / #168 under-inclusive membership class — the exact class the commit's thesis says it removes. --- ## Part 1 — Independent retail verification (all PASS) ### 1.1 `CPhysicsObj::calc_cross_cells` @`0x00515230` Symbol resolves exactly (`symbols.json` → `CPhysicsObj::calc_cross_cells`, offset 0). Disassembled: ``` 0x00515285 f786a800000000000100 test dword ptr [esi + 0xa8], 0x10000 0x0051528f 7574 jne 0x515305 ; -> BSP 0x00515291 8b4e10 mov ecx, [esi + 0x10] ; part array 0x00515296 7444 je 0x5152dc ; none -> sorting sphere 0x00515298 e8e32d0000 call 0x518080 ; CPartArray::GetNumCylsphere 0x0051529f 743b je 0x5152dc ; zero -> sorting sphere ... 0x005152d1 e81a670100 call 0x52b9f0 ; CObjCell::find_cell_list (cylsphere) 0x005152da eb35 jmp 0x515311 ; PAST the sorting-sphere branch 0x005152dc ... 0x005152e3 e818380000 call 0x518b00 ; CPartArray::GetSortingSphere 0x005152fb e890660100 call 0x52b990 ; CObjCell::find_cell_list (sorting sphere) 0x00515305 680c3f8400 push 0x843f0c 0x0051530c e8afbcffff call 0x510fc0 ; CPhysicsObj::find_bbox_cell_list ``` - The `test`/`jne` pair is at exactly `0x00515285` / `0x0051528f` as claimed. **PASS.** - `jne` target `0x515305` calls `0x00510fc0` = `CPhysicsObj::find_bbox_cell_list`. **PASS.** - Cylsphere call at `0x005152d1` → `0x0052b9f0`, sorting-sphere call at `0x005152fb` → `0x0052b990`; both **below** the jump and unreachable from it. **PASS.** - **Order (review item 2):** confirmed by fall-through, not merely by "BSP wins". Both `je 0x5152dc` guards (null part array; `GetNumCylsphere == 0`) skip to the sorting-sphere branch, and the cylsphere branch's `jmp 0x515311` at `0x005152da` jumps **past** the sorting-sphere branch. Precedence is therefore **BSP → CylSphere → sorting sphere**, exclusive at every step. **PASS.** `0x0052b9f0` and `0x0052b990` are two overloads of the same symbol `CObjCell::find_cell_list`; the commit distinguishes them correctly by argument shape. Retail's 10-sphere cap is confirmed independently inside the cylsphere overload at `0x0052ba21 cmp eax, 0xa`, matching `RetailSphereCap = 10`. ### 1.2 `CPhysicsObj::FindObjCollisions` @`0x0050f050` (review item 3) Left untouched by the commit; re-confirmed so "already correct" is earned: ``` 0x0050f165 f786a800000000000100 test dword ptr [esi + 0xa8], 0x10000 0x0050f16f 7431 je 0x50f1a2 ; clear -> primitive dispatch 0x0050f18d e8ee8f0000 call 0x518180 ; CPartArray::FindObjCollisions 0x0050f19d e90e010000 jmp 0x50f2b0 ; UNCONDITIONAL 0x0050f1a2 ... ; CylSphere loop head 0x0050f1d6 0f833b010000 jae 0x50f317 ; loop exhausted -> RETURN 0x0050f21d ... ; Sphere loop head 0x0050f22f 0f84e6000000 je 0x50f31b ; zero Spheres -> RETURN seeded OK_TS ``` All five cited instruction addresses are byte-exact and the `jmp 0x50f2b0` is past both `0x50f1a2` and `0x50f21d`. The dispatch is exclusive. **PASS** — the query path genuinely did not need changing. (`ebp` at `0x0050f171`/`0x0050f1b2`/ `0x0050f235` is the ethereal/ignore early-out, not a second shape branch.) ### 1.3 `CacheHasPhysicsBSP` and AP-153's "exactly one caller" (review item 4) Two distinct symbols, both cited correctly in different places: `CPhysicsObj::CacheHasPhysicsBSP` @`0x0050f570` and `CPartArray::CacheHasPhysicsBSP` @`0x00518110`. Full `.text` + `.rdata` scan for `E8`/`E9` rel32 and absolute-dword references: | Target | Refs found | |---|---| | `CPhysicsObj::CacheHasPhysicsBSP` `0x0050f570` | **1** — `call` at `0x0051272e` = `CPhysicsObj::InitPartArrayObject+0x7e` | | `CPartArray::CacheHasPhysicsBSP` `0x00518110` | **1** — `call` at `0x0050f57d` = `CPhysicsObj::CacheHasPhysicsBSP+0xd` | **PASS**, and stronger than filed: `InitPartArrayObject` @`0x005126b0` itself has exactly three callers, all construction — `CPhysicsObj::InitNullObject+0x1f`, `CPhysicsObj::makeObject+0x3b`, `CBuildingObj::makeBuilding+0x3b`. And `CPartArray::SetPart` @`0x00518580` (the `AnimPartChanged` swap site) calls `CPhysicsPart::SetPart` @`0x0050e700` per part and never re-caches. So retail's dispatch flag is derived once at construction and is genuinely stale after a part swap. **AP-153 is honestly scoped** and could legitimately claim more evidence than it does. `CPartArray::CacheHasPhysicsBSP`'s body confirms the derivation the acdream gate imitates: walk `[ecx+0x5c][i]` parts, deref `[part+0x20]` → gfxobj, test `[gfxobj+0x78]` (physics BSP), OR `0x10000` into `[ecx]` on the first hit. ### 1.4 Every cited address resolves to the symbol claimed (review item 5) | Address | Resolves to | Verdict | |---|---|---| | `0x00515230` | `CPhysicsObj::calc_cross_cells` | ✔ | | `0x0050f050` | `CPhysicsObj::FindObjCollisions` | ✔ | | `0x00510fc0` | `CPhysicsObj::find_bbox_cell_list` | ✔ | | `0x0052b9f0` / `0x0052b990` | `CObjCell::find_cell_list` (two overloads) | ✔ | | `0x0050f570` | `CPhysicsObj::CacheHasPhysicsBSP` | ✔ | | `0x00518110` | `CPartArray::CacheHasPhysicsBSP` | ✔ | | `0x00518180` | `CPartArray::FindObjCollisions` | ✔ | | `0x0050d8d0` | `CPhysicsPart::find_obj_collisions` | ✔ | | `0x00537a80` / `0x00537fd0` | `CSphere::intersects_sphere` (two overloads) | ✔ | | `0x00518b00` | `CPartArray::GetSortingSphere` | ✔ | | `0x0051272e` | `CPhysicsObj::InitPartArrayObject+0x7e` | ✔ | | `0x00518060/70/80/90` | `GetNumSphere` / `GetSphere` / `GetNumCylsphere` / `GetCylsphere` | ✔ | No mis-citation found. ### 1.5 The four corrections (all re-measured from the installed DAT) | Correction | Claim | Measured | Verdict | |---|---|---|---| | 2a | max affected primitive = **6.714 m** CylSphere on `0x02001741` | `0x02001741` cyl[0] r = **6.714**, h = 1.476 — the max over the 172 affected | ✔ | | 2b | `0x0200086E` Sphere origin `(0.759, 0.165, 5.842)` | sph[0] origin `(0.758796, 0.165414, 5.842)`, r = 5.842 | ✔ | | 3 | cottage door `0x020019FF` Sphere = **0.100 m**; `0.141` is `Setup.Radius` | sph[0] r = **0.1** @ `(0, 0, 0.018)`; `Setup.Radius` = **0.14142136** | ✔ | | 4 | **two** pinning tests, both corrected, neither deleted | `FromSetup_DoorSetup_ProducesFourShapes` → `_EmitsBspPartsOnly`, and `FromSetup_DoorSetup_SphereAtExpectedLocalOffset` re-hosted `_ => true` → `_ => false` | ✔ | Note the correction is *scoped to the affected 172*, which is legitimate but unstated: the largest primitive anywhere in the DAT is a **15.0 m** Sphere on `0x02000D7D`. Worth one clause in the row so a later reader doesn't re-derive 6.714 as a global bound. ### 1.6 Register bookkeeping - **AP-152 retirement is earned, not asserted.** `~~AP-152~~` struck through, past tense, evidence column populated, corrections enumerated. Sabotage-verified both halves (below). - **AP row count = literal 107.** Parsed the section: 130 AP rows, 23 struck, **107 active**. Baseline at `ec29a732` = 105 active. 105 + 3 (AP-153/154/155) − 1 (AP-152) = **107**. Reconciles. - **AP-154's grep claim is exact.** `grep -rn "PhysicsStateFlags.HasPhysicsBsp" src/` → exactly two hits: `TransitionTypes.cs:1349` (the predicate) and `PhysicsEngine.cs:1614`, which reads `request.MoverPhysicsState` — an unrelated *mover*-state read, as the row says. - **ACE derivation confirmed.** `references/ACE/Source/ACE.Server/WorldObjects/WorldObject_Networking.cs:666-669` — `if (CSetup.HasPhysicsBSP) physicsState |= PhysicsState.HasPhysicsBSP; else &= ~`. - **Static-publisher homogeneity claim earned.** `LandblockPhysicsPublisher.cs:983` gates the Setup-primitive block on `entityBspCount == 0`, so that path already dispatched exclusively and emits Cylinder-typed shapes only. - **Blast-radius claim earned.** `ShadowShapeBuilder.FromSetup`'s only production caller is `src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:126` (`internal sealed`, AcDream.App) — unreachable from Headless. ### 1.7 Build and suite `bin`/`obj` deleted repo-wide, then Release build (0 errors) and full suite: ``` AcDream.Cli.Tests 4 / 0 skip AcDream.Content.Tests 126 / 0 AcDream.UI.Abstractions.Tests 546 / 0 AcDream.Runtime.Tests 1222 / 0 AcDream.Bake.Tests 15 / 0 AcDream.Headless.Tests 89 / 0 AcDream.App.Tests 4173 / 3 skip AcDream.Core.Net.Tests 764 / 0 AcDream.Core.Tests 4264 / 1 skip -------------------------------------------- 11,203 passed / 4 skipped / 0 failed ``` Exactly the commit's stated numbers, including Headless 89/89. ### 1.8 Sabotage (both restored; tree confirmed clean afterwards) | Sabotage | Reddened | |---|---| | A — `if (!anyPhysicsBspPart)` → `\|\| true` (restore the additive union) | `FromSetup_DoorSetup_EmitsBspPartsOnly`, `FromSetup_DispatchGateReadsTheEffectivePartIdentities`, `FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` — 3 failed / 20 passed | | B — `BuildFloodSpheres` `only` → cylinder-first (drop the BSP arm) | `BuildFloodSpheres_BspBearingOwner_FloodsFromBspNotFromCylinder` only — 1 failed / 9 passed | Both facts are load-bearing and neither is over-broad. Sabotage B leaving `FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` green is *correct* and corroborates the commit's own statement that the `BuildFloodSpheres` half is behaviour-neutral against today's producers. --- ## Part 2 — Findings, by severity ### F1 (HIGH) — AP-155(b)'s "over-inclusive, the safe direction" is empirically inverted, and it is the justification for deferring the residual **Register text (AP-155, Risk column):** *"(b)'s bounding-sphere approximation is over-inclusive (a sphere contains the box's inscribed extent but is larger in the diagonal), which floods MORE cells rather than fewer — the safe direction for membership."* That reasoning holds only if the sphere is **concentric** with the geometry. It is not. Production builds the BSP flood sphere from two different sources: - **centre** — `src/AcDream.Core/Physics/ShadowShapeBuilder.cs:194`: `LocalPosition = partFrame.Origin * entScale`, the part's *placement-frame* origin; - **radius** — `src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:137` → `_physicsBspRadius` → `src/AcDream.Core/Physics/FlatCollisionAssetBuilder.cs:393`, `PhysicsBSP.Root.BoundingSphere.**Radius**`. The BSP root sphere's own **`Origin` is discarded**. `BuildFloodSpheres` (`src/AcDream.Core/Physics/ShadowObjectRegistry.cs:661`) then floods from `entityWorldPos + rotate(LocalPosition)` — the part origin — with that radius. **Measured over the installed `client_portal.dat`** (5,935 Setups; 530 carry a physics-BSP part; 973 BSP parts total): - **376 of 973** BSP parts have `|rootSphere.Origin| > radius/2`. Worst: **20.762 m** offset on a 27.708 m sphere (`gfx 0x010036DD`, Setup `0x0200129A`). - Over the **172 Setups this commit moves onto that path**, the emitted flood sphere set fails to contain the true BSP bounding sphere in **170** cases (73 CylSphere-bearing, 97 Sphere-bearing). Only 2 are covered. - Worst shortfall **9.911 m**, Setup `0x02000255`: one part, `gfx 0x01000A90`, BSP root sphere `origin = (0.000, −0.007, 9.911)`, `radius = 10.522` (`Setup.Height = 18.692`). Production floods from a sphere centred at `(0,0,0)` — 9.9 m below the geometry's own collision centre. **Observable in-game consequence.** Outdoor land flood (`AddAllOutsideCells`) is XY-driven and mostly forgives this. Indoor / building flood is not: `CellTransit.BuildShadowCellSet` (`src/AcDream.Core/Physics/CellTransit.cs:601`) routes every candidate cell with `id & 0xFFFF >= 0x0100` through `FindTransitCellsSphere`, a 3-D sphere-vs-portal test. A tall dungeon or building prop whose BSP sphere sits several metres above the part origin will not be registered into the EnvCells it physically occupies. It is then never a broadphase candidate there at all (`TransitionTypes.cs:3763` only iterates entries already in the cell), so it does not collide: **walk through the upper part of a tall indoor prop, or through a door slab from the storey above/below it**. That is precisely the #98 / #168 class. The register does not merely omit this — it records the opposite and uses the recorded direction as the reason the residual is safe to leave open. Under the C4-handoff process finding *"a contract asserting a mechanism that does not exist caused three separate defects"*, this is the same failure mode, in a register row rather than a code contract. Two secondary inaccuracies in the same row's retail characterisation: - *"acdream approximates retail's bounding BOX"* — `find_bbox_cell_list` @`0x00510fc0` adds the object's own cell and then calls `CPartArray::calc_cross_cells_static` @`0x00518160`, which dispatches the virtual at `[cell_vtbl+0x7c]` with `(numParts, parts, cellarray)` — a `find_transit_cells` part-array overload (`CObjCell::find_transit_cells` `0x0052b070`/`0x0052b080`; `CEnvCell`'s pair sits at `.rdata` `0x007c8d14`/ `0x007c8d18`). Retail walks the **actual per-part geometry** through cell portals despite the function's name. The approximation is coarser than the row admits, which widens F1 rather than narrowing it. ### F2 (HIGH) — 43 of the 172 affected Setups get a strictly *smaller* flood than before this commit A direct consequence of F1, but it needs stating separately because it contradicts the commit's own thesis and is unguarded by any test. Modelling the pre-`4abd1b5e` `BuildFloodSpheres` (primitives preferred when any Cylinder exists; otherwise everything, cap 10) against the post-commit set over the same 172 Setups: | | count | |---|---| | new flood ⊇ old flood | 129 | | **new flood ⊉ old flood** | **43** (22 CylSphere-bearing, 21 Sphere-bearing) | Worst: **3.493 m** on `0x0200086E` — `Setup.Height = 11.684`, whose Sphere at `(0.759, 0.165, 5.842) r 5.842` reached `z ≈ 11.68`, while the two surviving BSP flood spheres (`gfx 0x01001B2B` r 9.015, `gfx 0x01001BB2` r 9.254, both centred at their part origins) reach only `z ≈ 9.0`. Next: `0x020015D4` 2.405 m, `0x02000359` 2.083 m, `0x02001761` 1.889 m. The commit says the change removes an under-inclusive membership defect. For these 43 Setups it introduces one. The commit's "NOT yet gated live" note is the right instinct; the connected gate must specifically look for props and doors that stopped blocking, not only for ones that started. **Neither new test covers this.** `BuildFloodSpheres_BspBearingOwner_...` and `FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` both hand-substitute a synthetic `Radius = 14f` at `LocalPosition = Vector3.Zero`, i.e. a *concentric* BSP sphere — exactly the configuration in which F1 cannot fire. ### F3 (MEDIUM) — `RegisterMultiPart`'s own doc comment still states the superseded rule `src/AcDream.Core/Physics/ShadowObjectRegistry.cs:432-439` (the sole caller of `BuildFloodSpheres`, 168 lines above it) still reads: > *"Flood spheres follow retail's rule (Ghidra 0x0052b9f0): when the object has > CylSpheres, they alone drive the flood (base point + cyl radius, capped at 10); > otherwise the BSP parts' bounding spheres stand in for the sorting sphere."* That is the pre-`4abd1b5e` rule and is now false — it is the rule sabotage B restored, and sabotage B reddened a test. The commit rewrote `BuildFloodSpheres`' own doc thoroughly and left its caller's contradicting it. The next reader who greps `RegisterMultiPart` before `BuildFloodSpheres` gets the wrong mental model of the very behaviour this commit changed. ### F4 (MEDIUM) — unregistered divergence: retail's third branch is ONE sorting sphere, acdream's is every Sphere shape Retail's `calc_cross_cells` fall-through calls `CPartArray::GetSortingSphere` @`0x00518b00`, which returns `[partArray+0x54] + 0x70` — a single authored whole-object sphere on the `CSetup` — and floods from that one sphere (`0x005152fb`). acdream's `only == null` branch (`ShadowObjectRegistry.cs:652`) applies no filter and floods from **every** non-BSP, non-Cylinder shape, i.e. the Setup's per-part Sphere array. Different DAT field, different cardinality, different extent. The source comment at `ShadowObjectRegistry.cs:646-648` admits the substitution (*"which acdream approximates with the remaining shapes' bounding spheres"*), and AP-155's anchor column even cites `CPartArray::GetSortingSphere 0x00518b00` — but no AP row states the divergence. The register's own rule ("any commit that introduces a deviation adds its register row IN THE SAME COMMIT") is not retroactive, and this predates `4abd1b5e`; but the commit rewrote this exact method, filed three rows for its neighbours, and stepped over this one. Same class, smaller: `BuildFloodSpheres` collapses a Cylinder to a single sphere at its base point with the cylinder radius and **ignores `CylHeight` entirely**, where retail's `CObjCell::find_cell_list` @`0x0052b9f0` is handed the CCylSphere array `(low_pt, radius, height)`. Also unregistered. ### F5 (LOW) — AP-155 is two divergences in one row; AP-153 and AP-154 are honestly distinct Asked whether one of AP-153/154/155 is the same divergence sliced twice: **no**, but AP-155 has the inverse problem. - **AP-153** (flag cached at construction vs. re-derived live) and **AP-154** (flag taken off the wire vs. derived client-side) concern the same bit but are genuinely different questions — *when* vs. *where from* — with different sites (`ShadowShapeBuilder` step 0 / `LiveEntityCollisionBuilder.ReconcileAppearance` vs. `TransitionTypes.cs:1348` / `LiveEntityCollisionBuilder.cs:161`), different risks, and different gates. Both scoped honestly. - **AP-155** bundles (a) the static publishers' Setup-Sphere→height-capped-Cylinder conversion — a *different* code path, a *different and larger* population, and its own stated gate — with (b) the flood-priority/approximation question, and then files (b) as already half-closed by the same commit. Three different lifecycles under one id. It should be two rows (or three), and (b)'s open remainder needs the F1 correction before it can be reasoned about at all. ### F6 (LOW) — commit-body imprecision *"Priority at both consumers: BSP -> CylSphere -> Sphere -> nothing."* At `FindObjCollisions` the third branch is `CPartArray::GetSphere` @`0x00518070` (the per-part Sphere array, loop head `0x0050f21d`). At `calc_cross_cells` it is `CPartArray::GetSortingSphere` @`0x00518b00` (one authored whole-object sphere, `0x005152e3`). Different arrays; "both consumers" is not true of the third rung. The register row and the source comments state this correctly — only the commit message is loose. Recorded so a future grep of the log doesn't inherit it. --- ## What was checked and found clean (so the PASS half is auditable) - Binary/PDB pairing (`MATCH`), and every disassembly re-derived from that binary rather than Binary Ninja. - `calc_cross_cells` full body: flag test address, jump target, both primitive call sites, and the **fall-through order** including the cylsphere branch's skip-past-sorting-sphere `jmp`. - `FindObjCollisions` full body: all five cited addresses, the unconditional `jmp` past both loops, and identification of `ebp` as the ethereal early-out rather than a shape branch. - `CacheHasPhysicsBSP` (both classes) bodies + exhaustive rel32/absolute xref scan; `InitPartArrayObject` callers; `CPartArray::SetPart` non-recaching. - `find_bbox_cell_list` → `calc_cross_cells_static` → `[vtbl+0x7c]` chain. - `GetSortingSphere` body (`[+0x54]+0x70`). - Retail's 10-sphere cap (`0x0052ba21 cmp eax, 0xa`). - 12 distinct cited addresses → symbol, no mis-citation. - All four register corrections re-measured from the installed DAT by an independent sweep; population 5,935 / 172 / 73 / 99 / 530 reproduced exactly. - AP active-row count parsed (107) and reconciled against the `ec29a732` baseline (105). - AP-154's `src/` grep claim (exactly 2 hits, second is a mover-state read). - ACE's derivation of the wire bit (`WorldObject_Networking.cs:666-669`). - Static publishers' `entityBspCount == 0` exclusivity gate. - `FromSetup`'s sole production caller is App-layer (Headless-unreachable). - `EffectivePartGfxObjId` genuinely shared by step 0 and step 3 — the "gate and emission read the same identity" trap is real and its test discriminates in both directions. - Clean `bin`/`obj` → Release build 0 errors → full suite 11,203 / 4 skip / 0 fail. - Two sabotages, both reddening only in the intended direction; tree restored and `git status --porcelain` empty. ## Recommended before the connected gate 1. Correct AP-155's Risk column: the approximation is **under**-inclusive for 170 of the 172 Setups this commit moved onto it, not over-inclusive. 2. Carry the BSP root sphere's `Origin` through `ShadowShape` (or offset `LocalPosition` by `partFrame.Orientation * rootSphere.Origin`) so the flood sphere is concentric with the geometry it stands for. That is a one-field fix at `ShadowShapeBuilder.cs:192-199` + `LiveEntityCollisionBuilder.cs:135-139`, and it converts F1/F2 from open risk to closed. 3. Add a non-concentric fixture to `ShadowObjectRegistryMultiPartTests` — the two new flood tests both use `LocalPosition = Zero`, which is the one configuration where the defect cannot appear. 4. Fix the stale `RegisterMultiPart` doc (F3) and split AP-155 (F5). 5. Instruct the connected gate to look for props/doors that **stopped** blocking as well as ones that started; 43 Setups shrank.