# 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 `continue`s. 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`.