acdream/docs/research/2026-08-06-ap152-review-retail.md

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.