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

AP-22 retail-conformance review — commit bc4679cd

Date: 2026-08-06 Reviewer role: retail-conformance (adversarial), read-only Subject: bc4679cd — fix(physics): delete the invented Setup-radius collision cylinder (AP-22) Worktree: C:\Users\erikn\source\repos\acdream\.claude\worktrees\peaceful-visvesvaraya-e0a196 Branch / HEAD: claude/acdream-physics-divergence-5aa784 @ bc4679cd

Verdict

PASS.

Every load-bearing claim was re-derived from primary source without reading the contract's or the commit's quotations of it. The retail disassembly was produced independently (Capstone over the PE .text, section-mapped from the file, symbol names resolved from docs/research/named-retail/symbols.json), and the DAT reachability sweep was produced by an independently written B-tree walk and Setup decoder in Python, validated by byte accounting. Both reproduce the commit's numbers exactly. The deletion is correct, complete, and behaviour-preserving on the installed data.

Five findings follow. None is a defect in the shipped code. They are one documentation-precision error inside the change (F1), one incompleteness in the register row (F2), one pre-existing stale comment the change made stale-er (F3), and two informational notes (F4, F5) plus one un-audited claim (F6).

---

1. Binary provenance

py tools/pdb-extract/check_exe_pdb.py "C:/Users/erikn/Downloads/acclient.exe"
  timestamp = 0x52291f34  -> 2013-09-06T00:17:56Z
  GUID      = {9e847e2f-777c-4bd9-886c-22256bb87f32}   age = 1
  === MATCH: this exe pairs with our acclient.pdb ===

image_base = 0x00400000. All addresses below are VAs in that image.

---

2. Claim 1 — exclusive dispatch in CPhysicsObj::FindObjCollisions @0x0050f050

VERIFIED, all four cited anchors byte-exact.

Independent disassembly (extent 0x0050f050 – 0x0050f32e, next symbol 0x0050f340). The four specifics the commit and register rest on:

Claim	Bytes at address	Verdict
OK_TS seed	0x0050f13b bf 01 00 00 00 mov edi, 1	exact
BSP dispatch	0x0050f165 f7 86 a8 00 00 00 00 00 01 00 test dword ptr [esi+0xa8], 0x10000 then 0x0050f16f 74 31 je 0x50f1a2	exact
BSP branch cannot reach the primitive branches	0x0050f19d e9 0e 01 00 00 jmp 0x50f2b0 — unconditional	exact
zero spheres → epilogue	0x0050f22f 0f 84 e6 00 00 00 je 0x50f31b	exact

Control-flow reading (mine, not the contract's):

• BSP branch (0x0050f171–0x0050f19d): guarded by HAS_PHYSICS_BSP_PS; null part-array → je 0x50f31b (epilogue); otherwise call 0x518180 CPartArray::FindObjCollisions, edi = eax; edi == 1 → je 0x50f31b; else the unconditional jmp 0x50f2b0 into the collision-report tail. It cannot fall into either primitive loop.
• CylSphere branch (0x0050f1a2–0x0050f21b): entered only when the BSP bit is clear. Null part-array or GetNumCylsphere() == 0 → je 0x50f21d (the Sphere branch). With cylspheres present the loop runs; loop completion exits at 0x0050f1d6 jae 0x50f317 → 0x50f31b epilogue, i.e. a CylSphere-bearing object that survives its loop returns and never reaches the Sphere loop. A non-OK result leaves at 0x0050f211 jne 0x50f2ac → the 0x50f2b0 report tail.
• Sphere branch (0x0050f21d–0x0050f2aa): null part-array → epilogue; GetNumSphere() == 0 → the cited 0x0050f22f je 0x50f31b.
• Epilogue 0x0050f31b: mov eax, edi / mov dword ptr [ebx+0x1cc], 0 / ret 4. With no BSP, no cylspheres and no spheres, edi is still the 1 seeded at 0x0050f13b, and COLLISIONINFO::add_object is never reached.

So the dispatch is BSP, else CylSpheres if any, else Spheres if any, else nothing — exclusive, and a shapeless object returns OK_TS with no shape synthesized and no collision recorded.

Two corroborations that the operand decode is right rather than merely plausible: 0x0050f061 test al, 4 and 0x0050f095 test al, 4 on the same [esi+0xa8] are ETHEREAL_PS = 0x4, and HAS_PHYSICS_BSP_PS = 0x10000 — both from docs/research/named-retail/acclient.h:2815–2833. [esi+0xa8] is therefore CPhysicsObj::state, not an adjacent field.

---

3. Claim 2 — CPartArray::GetRadius/GetHeight absent from the call set

VERIFIED, at three levels.

(a) Direct calls in the function. The complete E8-encoded call set of 0x0050f050–0x0050f32e is exactly the nine symbols the register row lists:

OBJECTINFO::missile_ignore 0x0050ceb0 (×3) · CPartArray::FindObjCollisions 0x00518180 · CPartArray::GetNumCylsphere 0x00518080 (×2) · CPartArray::GetCylsphere 0x00518090 · CCylSphere::intersects_sphere 0x0053b8f0 · CPartArray::GetNumSphere 0x00518060 (×2) · CPartArray::GetSphere 0x00518070 · CSphere::intersects_sphere 0x00537fd0 · COLLISIONINFO::add_object 0x006b4e20.

Neither 0x005180a0 nor 0x005180b0 appears. (The six indirect call dword ptr [reg+N] sites are all vtable calls on [esi+0x12c], the weenie object — not the part array. The register row omits them; that omission is correct in substance and worth nothing further.)

(b) Whole-binary xref, the other direction. Scanning every E8/E9 rel32 in .text for targets 0x005180a0 / 0x005180b0 yields six callers each, and none lies inside [0x0050f050, 0x0050f340):

CPartArray::GetRadius  0x005180a0 <- CPhysicsObj::GetRadius+0x7 (tail jmp),
    check_attack+0x47, get_distance_to_object+0x38, get_distance_to_object+0x6c,
    stick_to_object+0x4e, MoveToObject+0x9e
CPartArray::GetHeight  0x005180b0 <- CPhysicsObj::GetHeight+0x7 (tail jmp),
    check_attack+0x2d, get_distance_to_object+0x1e, get_distance_to_object+0x52,
    stick_to_object+0x34, MoveToObject+0x84

(c) Transitively, through the one branch that delegates. CPartArray::FindObjCollisions 0x00518180 is a bare loop over [this+0x5c][i] calling CPhysicsPart::find_obj_collisions and breaking on a non-OK result — no radius/height. See §5 for the callee.

Conclusion: the justification for deletion holds. Retail never converts Setup.Radius/Height into collision geometry.

---

4. Claim 3 — where retail does use setup radius/height

VERIFIED; the deleted values have a real, non-collision home.

CPartArray::GetRadius @0x005180a0 is four instructions:

mov  eax, dword ptr [ecx+0x54]     ; this->setup
fld  dword ptr [eax+0x64]          ; setup->radius
fmul dword ptr [ecx+0x68]          ; * this->scale
ret

GetHeight @0x005180b0 is the same shape. That is exactly acdream's LiveEntityMotionRuntimeController.GetSetupCylinder (src/AcDream.App/Physics/LiveEntityMotionRuntimeController.cs:284 — return (setup.Radius * scale, setup.Height * scale);), which the AP-22 row calls retail-faithful and untouched. It is.

Consumers, spot-checked to the call:

• Attack cones — CPhysicsObj::check_attack 0x0050ec80 reads GetHeight @0x0050ecad and GetRadius @0x0050ecc7, then call 0x00536ab0 CSphere::attack.
• cylinder_distance — CPhysicsObj::get_distance_to_object 0x0050f7a0 reads both for self and target (0x0050f7be / 0x0050f7d8 / 0x0050f7f2 / 0x0050f80c) then call 0x005a97f0 Position::cylinder_distance.
• MoveTo / sticky family — CPhysicsObj::stick_to_object 0x005127e0, CPhysicsObj::MoveToObject 0x00512860, and via the CPhysicsObj::GetRadius (0x0050e9c0) / GetHeight (0x0050e9e0) thunks: DetectionManager::CheckDetection, MoveToManager::GetCurrentDistance, StickyManager::adjust_offset.

See F4 for the one consumer that is collision-adjacent and not named in the row.

---

5. Claim 4 — the corrected ShadowShapeBuilder anchor

VERIFIED. The previous comment was false; the replacement is exact.

CPhysicsPart::find_obj_collisions @0x0050d8d0, whole body (0x50d8d0–0x50d8e0 is the extent to the next symbol at 0x50d920):

0x0050d8d3  mov ecx, [esi+0x20]      ; CGfxObj **gfxobj
0x0050d8d6  mov ecx, [ecx]           ; CGfxObj *
0x0050d8d8  test ecx, ecx
0x0050d8da  mov eax, 1               ; OK_TS
0x0050d8df  je  0x50d90d             ; -> return 1
0x0050d8e1  mov edx, [ecx+0x78]      ; gfxobj->physics_bsp
0x0050d8e4  test edx, edx
0x0050d8e6  je  0x50d90d             ; -> return 1
            ... call 0x50c9d0 SPHEREPATH::cache_localspace_sphere
            ... call 0x534700 CGfxObj::find_obj_collisions

There is no CylSphere test inside a part, so the old comment ("each part's find_obj_collisions tests CylSpheres + GfxObj BSP") was wrong and its correction is warranted.

Both struct offsets were verified rather than assumed, from docs/research/named-retail/acclient.h:

• CPhysicsPart (h:31151): CYpt 0 · viewer_heading 4–16 · degrades 16 · deg_level 20 · deg_mode 24 · draw_state 28 · CGfxObj **gfxobj at 32 = +0x20 · gfxobj_scale 36–48 · pos at 48 = +0x30, which is exactly the lea eax,[esi+0x30] pushed to cache_localspace_sphere.
• CGfxObj : DBObj (h:31712) with DBObj : Interface (h:27570) and CVertexArray (h:31300): DBObj 48 (vfptr 4, category 4, bool+pad 8, long double 8, 4 pointers/ints 16, DID 4, bool+pad 4) + material/num_surfaces/ m_rgSurfaces 12 + CVertexArray 40 + num_physics_polygons/physics_polygons/ constructed_mesh/use_built_mesh 16 + physics_sphere 4 = 120 = +0x78 = BSPTREE *physics_bsp.

---

6. Claim 5 — every cited address is the construct claimed

VERIFIED. Every address appearing in the AP-22 row, the AP-152 row, and both code comments was resolved against symbols.json by nearest-preceding-symbol so an off-by-one landing inside a neighbour would surface (the AP-150 failure mode):

0x00518060 CPartArray::GetNumSphere        EXACT
0x00518070 CPartArray::GetSphere           EXACT
0x00518080 CPartArray::GetNumCylsphere     EXACT
0x00518090 CPartArray::GetCylsphere        EXACT
0x00518110 CPartArray::CacheHasPhysicsBSP  EXACT
0x0050f570 CPhysicsObj::CacheHasPhysicsBSP EXACT
0x0053b8f0 CCylSphere::intersects_sphere   EXACT
0x00537fd0 CSphere::intersects_sphere      EXACT
0x006b4e20 COLLISIONINFO::add_object       EXACT
0x0050ceb0 OBJECTINFO::missile_ignore      EXACT
0x0050ec80 CPhysicsObj::check_attack       EXACT
0x0050f7a0 CPhysicsObj::get_distance_to_object EXACT
0x005180a0 CPartArray::GetRadius           EXACT
0x005180b0 CPartArray::GetHeight           EXACT
0x0050d8d0 CPhysicsPart::find_obj_collisions EXACT
0x0050f050 CPhysicsObj::FindObjCollisions  EXACT

No mis-cited line. (0x00537a80 in the pre-existing comment at tests/AcDream.Core.Tests/Physics/ShadowShapeBuilderTests.cs:81 also resolves EXACT — it is a second overload of CSphere::intersects_sphere, not an error.)

---

7. The empirical claim — independent DAT sweep

Written from scratch: DAT header at 0x140, 1024-byte block chain walk, B-tree directory traversal (62 branches / 61 entries / 24-byte rows), Setup record decode per the format cross-read from references/DatReaderWriter Setup.generated.cs. Self-validating: each of the 5,935 records must land on a residual tail of exactly 20 + 48 * numLights.

parsed OK: 5935 / 5935   (bad accounting: 0)
cylsphere = 678   sphere-only = 3605   no primitive = 1652
radius > 0.0001 = 4282   radius > 0 = 4283
GUARD (no cyl & no sph & radius > 0.0001) = 0
GUARD (no cyl & no sph & radius > 0.0)    = 0
no-primitive Setups with radius != 0 exactly: 0

Every number matches the commit and the test's five control constants (5935 / 678 / 3605 / 1652 / 4282). The deleted branch was unreachable for all 5,935 installed Setups, at both guard thresholds. Nothing in game loses a collision shape; no visual gate is required, as claimed.

---

8. Completeness of the deletion

• Three sites and no fourth. A whole-src grep for setup.Radius / setup.Height outside tests returns only LiveEntityMotionRuntimeController.cs:284 (the retail-faithful GetSetupCylinder), PhysicsDataCache.cs:391–392 (verbatim storage), and comments. No surviving copy.
• Site 3 really is headless-only. LandblockPhysicsContentBuilder.PublishStaticCollision (src/AcDream.Content/LandblockPhysicsContentBuilder.cs:592) has exactly one production caller: src/AcDream.Headless/Hosting/HeadlessSessionWorldProjection.cs:462.
• FlattenSetup copies Radius verbatim (FlatCollisionAssetBuilder.cs:277), so the sweep's flat.Radius is the same float the deleted guards read.
• Both static paths are genuinely exclusive, as AP-152 asserts: LandblockPhysicsPublisher.cs:985 gates the whole Setup walk on entityBspCount == 0; LandblockPhysicsContentBuilder.cs:617–632 registers BSP shapes and continues. Sphere emission is gated on Cylinders.Length == 0 in both — matching retail's fall-through order.

---

9. Bookkeeping

Item	Verdict
AP-22 retirement earned by the code, not asserted	Yes. Struck-through, past tense, three-site list correct, byte anchors correct, and its stated nine-symbol call set matches my disassembly exactly.
AP-152 scoped honestly	Yes. Its structural claim, its three cited addresses, and the two CacheHasPhysicsBSP anchors all verify. It correctly notes ShadowShapeBuilderTests.FromSetup_DoorSetup_ProducesFourShapes PINS the additive behaviour — verified: that test asserts 1 Sphere + 3 BSP shapes from one Setup and would have to be rewritten. It also correctly notes the internal inconsistency with the two static paths.
#330 distinguished from #291	Yes. docs/ISSUES.md:1260 shows #291 is the headless 3×3 collision window wanting a divergence-register row; #330 is "no live-entity shape is ever built on the headless host". Different objects, no overlap. Both premises re-checked: ShadowShapeBuilder.FromSetup has one production caller in AcDream.App, and AcDream.Headless.csproj references only AcDream.Runtime.
Section-3 header count 105	Reconciles. bcb66ccd said 105; bc4679cd says 105; a mechanical count of active (non-struck) AP rows in section 3 returns exactly 105, with no duplicate ids and max id AP-152.

---

10. Gates reproduced

Gate	Result
dotnet build -c Release AcDream.slnx	0 errors, 0 warnings
AcDream.Content.Tests	125 / 125
AcDream.App.Tests	4,171 passed / 3 skipped (with sabotage in place; 4,172/3 clean)
AcDream.Core.Tests — ShadowShapeBuilderTests	8 / 8
AcDream.Headless.Tests	89 / 89

Sabotage checks, both restored:

1. ExpectedWithCylinder 678 → 679 in InstalledSetupCollisionReachabilityTests reddens with Expected: 679 / Actual: 678. The test genuinely enumerates the installed DAT through FlatCollisionAssetBuilder.FlattenSetup and measures 678 — it is not vacuous on this machine, and its measurement equals my independent parse.
2. Restoring the deleted block into LiveEntityCollisionBuilder.Build reddens exactly one test across the full 4,175-test App suite: LiveEntityCollisionBuilderTests.ShapelessSetupWithRadius_ProducesNoRegistration. The commit's claim that the new fact is precisely inverse to the deleted one holds.

git status --porcelain is empty at HEAD bc4679cd after both restorations.

---

11. Findings

F1 — LOW (documentation precision, inside the change)

tests/AcDream.Content.Tests/InstalledSetupCollisionReachabilityTests.cs:76 comments the predicate !hasCylinder && !hasSphere && flat.Radius > 0.0001f as "The exact guard the three deleted copies used." It is not exact. Only site 1 (LiveEntityCollisionBuilder) used > 0.0001f; sites 2 and 3 (LandblockPhysicsPublisher.cs, LandblockPhysicsContentBuilder.cs) used the strictly wider setup.Radius > 0f. The test therefore does not evaluate the guard of two of the three sites it claims to pin.

Harmless in fact — I measured the > 0f variant myself and it is also 0, because every no-primitive Setup has Radius exactly 0.0 — but the sentence asserts a coverage identity the code does not have. In-game consequence: none. Risk: a future reader trusting the comment could relax a > 0f site believing it was pinned. Cheapest correction is to evaluate flat.Radius > 0f (the wider guard, still Empty) or to reword.

F2 — LOW (register incompleteness)

The AP-22 row states "all 1,294 genuinely shapeless Setups have Radius exactly 0." True, but it is the weaker of the two available statements and it leaves the 358 BSP-only Setups unaddressed — and those are precisely the ones sites 2/3's Cylinders.Length == 0 && Spheres.Length == 0 && Radius > 0f guard would have fired on had their radii been nonzero. Sites 2/3 were saved twice over: by the outer entityBspCount == 0 / BSP-continue gate, and by those 358 radii also being exactly zero. My sweep establishes the stronger fact: all 1,652 Setups with no CylSphere and no Sphere have Radius exactly 0.0. One clause in the row would close it. docs/architecture/retail-divergence-register.md:200.

F3 — LOW (stale doc, pre-existing, made stale-er here)

src/AcDream.Core/World/WorldEntity.cs:154–164 documents Scale as being "used by the collision registration path to scale CylSphere / Sphere / Setup.Radius shapes." After bc4679cd there is no Setup.Radius shape on any path. A three-word doc fix that would naturally have ridden along with the deletion. No behavioural effect.

F4 — INFORMATIONAL (a precision the row would benefit from)

AP-22's blanket "Setup.Radius/Height serve attack cones, cylinder_distance and MoveTo, never collision geometry" is true of retail's shape dispatch, but retail does read CPhysicsObj::GetHeight twice inside CPhysicsObj::report_object_collision — at 0x005130f4 and 0x005131c2, each immediately feeding call 0x005a9580 Position::determine_quadrant for the AtkCollisionProfile / ObjCollisionProfile. That is a post-collision report field (which body quadrant was struck), not geometry, so the row's conclusion is unaffected; but a future reader who finds GetHeight in a function named report_object_collision may believe they have refuted the row. One clause ("…and the collision report's quadrant, never its geometry") pre-empts that.

Related, same axis, out of AP-22's scope: acdream still derives a mover sphere from setup.Radius/Height as the TS-46 fallback (src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs:376–392, the deR/deH reconstruction used only when the Setup carries no sphere rows). That is the mover's own sweep volume, not the target's collision shape, so it is a different question — but it lives under the same sentence and is worth knowing about before someone reads AP-22 as a blanket prohibition.

F5 — INFORMATIONAL (evidence is machine-local)

InstalledSetupCollisionReachabilityTests returns silently when ContentConformanceDats.ResolveDatDir() finds no DAT directory, so on a DAT-less CI it passes vacuously — including its five positive controls, whose whole purpose is to prevent vacuous passes. This is the documented suite convention (ContentConformanceDats says so explicitly and cites the Core.Tests precedent), so it is not a new sin and I am not asking for a change. It does mean the strongest evidence for this commit only actually runs on a machine with the client installed.

F6 — NOT AUDITED (declared, not disputed)

AP-152's "172 of 5,935 Setups (73 CylSphere+BSP, 99 Sphere+BSP)" and AP-22's 1,652 → 1,294 + 358 split both require GfxObj physics-BSP presence, which a Setup-only parse cannot see; I did not build a GfxObj decoder. Nothing I measured contradicts them (1,652 no-primitive is exactly 1,294 + 358, and 358 + 172 = 530 Setups with ≥1 BSP part is internally consistent), and AP-152 is a filed divergence rather than a code change, so the stakes are low. The cottage-door 0x020019FF example and its ~14 cm base sphere are likewise unverified. Flagged so the PASS is not read as covering them.

---

12. What was checked, so the PASS is auditable

1. EXE↔PDB pairing via check_exe_pdb.py → MATCH.
2. Full independent disassembly of CPhysicsObj::FindObjCollisions (0x0050f050, 0x2f0 bytes) — control flow, all four cited anchors, the epilogue, and the CylSphere loop-completion exit.
3. PhysicsState bit values cross-read from acclient.h to confirm the operand decode (ETHEREAL_PS 0x4, HAS_PHYSICS_BSP_PS 0x10000).
4. Complete direct call set of that function; whole-.text rel32 xref of CPartArray::GetRadius / GetHeight in the reverse direction.
5. Disassembly of CPartArray::GetRadius/GetHeight, CPartArray::FindObjCollisions, CPhysicsPart::find_obj_collisions.
6. CGfxObj / CPhysicsPart field-offset arithmetic from acclient.h to confirm +0x78 = physics_bsp and +0x20 = gfxobj.
7. Call-site inspection of check_attack, get_distance_to_object, report_object_collision to place the deleted values in a legitimate home.
8. Nearest-preceding-symbol resolution of all 16 addresses cited across the two register rows and both code comments.
9. Independent Python DAT B-tree walk + Setup decode of all 5,935 Setups, byte accounted, at both guard thresholds.
10. Diff read of all four production files and both test files; grep sweep for a fourth copy; caller counts for PublishStaticCollision and ShadowShapeBuilder.FromSetup; FlattenSetup radius provenance; exclusivity of both static paths.
11. Register row text (AP-22, AP-152), section-3 header count vs. a mechanical count of active rows, retire-next shortlist, ISSUES #330 vs. #291.
12. Release build; Content / App / Core-ShadowShapeBuilder / Headless suites; two sabotage experiments, both restored; git status clean at bc4679cd.