diff --git a/docs/research/2026-08-06-ap22-review-architecture.md b/docs/research/2026-08-06-ap22-review-architecture.md new file mode 100644 index 00000000..a83fed95 --- /dev/null +++ b/docs/research/2026-08-06-ap22-review-architecture.md @@ -0,0 +1,365 @@ +# AP-22 architecture review — `bc4679cd` + +**Scope:** completeness, blast radius, test quality. Retail fidelity is a +separate reviewer's. **Verdict: PASS**, with one required correction to the +commit record (Finding A) and four minor accuracy items. + +**Worktree:** `.claude/worktrees/peaceful-visvesvaraya-e0a196`, branch +`claude/acdream-physics-divergence-5aa784`, HEAD `bc4679cd`, tree clean at +review start and at review end (the only untracked file is the parallel +retail reviewer's `2026-08-06-ap22-review-retail.md`, untouched). + +--- + +## 1. The crux — do I believe the reachability claim? + +**Yes. Verified by a third route that shares no code with either prior +parser, and the strict-guard variant the contract and the test both left +unverified is also zero.** + +### 1.1 Method + +I wrote a raw `client_portal.dat` parser in Python from scratch — B-tree +directory walk (root at header `0x140 + 0x20` = `0x0232D400`, 1024-byte block +chain), Setup record decode against retail's own `CSetup` field order +(`docs/research/named-retail/acclient.h:31119` + `CSetup::UnPack` @ +`0x00520c50`). It shares no code with `DatReaderWriter`, with +`FlatCollisionAssetBuilder.FlattenSetup`, or with the implementer's C# +scratch parser. Script: scratchpad `ap22_sweep.py` (not committed). + +Self-validation, stronger than the contract's: **5,935/5,935 records consumed +to byte-exact end, residual histogram `{0: 5935}`** — i.e. zero unexplained +bytes on every record, not "a 20 + 48·numLights tail". **608 of the 5,935 +carry a light row**, so the 48-byte `LIGHTINFO` size and the whole post-lights +tail are genuinely exercised rather than trivially satisfied by +`numLights == 0` everywhere (that was worth checking: BN renders the light +allocation as `0x68`-sized at pc:296913, which would have desynchronized 608 +records had it been right). + +### 1.2 Results — every number reproduces + +| Bucket | Contract / test literal | My independent parse | +|---|---|---| +| Total Setups | 5,935 | **5,935** | +| ≥1 CylSphere | 678 | **678** | +| 0 CylSphere, ≥1 Sphere | 3,605 | **3,605** | +| 0 primitives (`ExpectedWithoutAnyPrimitive`) | 1,652 | **1,652** | +| — of those, ≥1 physics-BSP part | 358 | **358** | +| — of those, fully shapeless | 1,294 | **1,294** | +| `Radius > 0.0001` | 4,282 | **4,282** | +| primitive **and** physics-BSP part (AP-152) | 172 | **172** | +| GfxObjs / with physics bit | 15,318 / 1,258 | **15,318 / 1,258** | +| **Fallback reachable** | **0** | **0** | + +Third decoder, spot checks — `dotnet run --project tools/SetupInspect` +(DatReaderWriter, a different implementation again) agrees bit-for-bit with my +parser on all three ids the contract cites: + +- `0x02000001` humanoid — Radius/Height `0.679 / 1.835`, Spheres 2 + (r = 0.48 at z = 0.475 and 1.35), CylSpheres 0, Parts 34, 0 BSP parts. +- `0x02000C9D` Facility Hub door — Radius/Height `0.000 / 0.000`, + 0 primitives, parts `0x01002936`/`0x01002936`/`0x01002937`, 2 BSP parts. + This is a member of the 1,294 shapeless bucket **with Radius exactly 0**. +- `0x020019FF` cottage door — Radius/Height `0.141 / 0.200`, 1 Sphere + (r = 0.100 at z = 0.018), 1 BSP part. + +### 1.3 A gap I closed that neither the contract nor the test covers + +The test's predicate is `flat.Radius > 0.0001f`, and its comment calls that +**"the exact guard the three deleted copies used"** +(`tests/AcDream.Content.Tests/InstalledSetupCollisionReachabilityTests.cs:69`). +It is not. Site 1 used `setup.Radius > 0.0001f`; **sites 2 and 3 used +`setup.Radius > 0f`** (`LandblockPhysicsPublisher.cs:1043` and +`LandblockPhysicsContentBuilder.cs:696` at `bc4679cd~1`). Over the installed +DAT the two predicates genuinely differ: **4,282 Setups satisfy `> 0.0001` +but 4,283 satisfy `> 0`** — Setup `0x02001657` carries a denormal +`Radius = 1.2988e-39` (bits `0x000E246E`). + +I evaluated the strict guard directly: **`!hasCyl && !hasSph && Radius > 0f` +is also 0 of 5,935** (that one denormal Setup carries a primitive). So there +is no hole — but the test does not pin the guard sites 2 and 3 actually used, +and its comment overstates. See Finding C. + +**Conclusion: the deletion removes collision from nothing.** The branch was +unreachable dead code under both guards, for every Setup in the installed +`client_portal.dat`. + +--- + +## 2. Is there a fourth copy? — No. Exhaustively. + +**By symbol.** `new ShadowShape(` appears exactly **8 times** in `src/`: +2 in `LandblockPhysicsPublisher` (CylSphere loop `:1003`, Sphere loop +`:1030`), 2 in `LandblockPhysicsContentBuilder` (`:658`, `:683`), 4 in +`ShadowShapeBuilder` (`:89` CylSphere, `:108` Sphere, `:147` BSP part, `:231` +`FromLandblockBspParts`). **None of the eight reads `Setup.Radius` or +`Setup.Height`.** + +**By concept.** After the deletion the only `setup.Radius`/`setup.Height` +reads left in `src/` are: + +- `src/AcDream.Core/Physics/PhysicsDataCache.cs:391-392` — flatten + pass-through into `FlatSetupCollision`, no synthesis. +- `src/AcDream.App/Physics/LiveEntityMotionRuntimeController.cs:284` + (`GetSetupCylinder`) — the MoveTo/attack-cone consumer, explicitly + out of scope and retail-faithful. +- comments. + +**By registration entry point.** `RegisterMultiPart` / `ReplaceMultiPartPayload` +have exactly four production callers — the three deleted sites' surviving +registration calls plus `LiveEntityCollisionBuilder.Register:175`. Every +*internal* re-registration (`ShadowObjectRegistry.cs:536`, `:707`, `:1582`, +`:2165`, and `RuntimeRemotePhysicsUpdater`'s re-flood) replays a +previously-built shape list and constructs nothing. +`AcDream.Runtime` and `AcDream.Headless` contain **no** `ShadowShape` +reference at all. + +**Adjacent but categorically different — not a fourth copy.** +`src/AcDream.App/Composition/SessionPlayerComposition.cs:544-571` and +`src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs:389-391` derive a +capsule from `GetSetupCylinder` (`Setup.Radius/Height × scale`) with a +hardcoded `0.48 / 1.835` human fallback. That is the **mover's own** sphere/ +capsule — the moving object's shape, not the shape other objects collide +*against* — and is separately governed by #184 / TS-46 / AD-25. Correctly out +of AP-22's scope. + +--- + +## 3. The corrected literal `1,652` + +**Verified independently, and not derived tautologically.** + +- My parse yields `no_primitive = 1,652`, decomposing exactly as the commit + states: 358 BSP-only + 1,294 shapeless. +- The literal is not a re-derivation from the production code under test: + its two components are the contract's §3.2 external measurements, and the + sum is arithmetic. My parse confirms both components and the sum without + touching `FlatCollisionAssetBuilder`. +- The reason 1,294 was wrong for this test is correct as stated: 1,294 is a + three-way condition including "0 physics-BSP parts", and the code path + under test (`FlattenSetup`) resolves no GfxObjs and cannot see that + dimension. + +--- + +## 4. Test quality — 4 sabotages reproduced, 1 claim disproved + +All runs below on a **fully clean** build (all 44 `bin`/`obj` removed first — +see Finding F). + +| # | Sabotage | Claimed | Observed | +|---|---|---|---| +| S1 | Restore the deleted block in `LiveEntityCollisionBuilder.Build` | reddens exactly `ShapelessSetupWithRadius_ProducesNoRegistration`, nothing else | **Confirmed** — App.Tests 4,171 passed / **1 failed** / 3 skipped; the single failure is that test | +| S2 | `Assert.Empty(fallbackReachable)` → `Assert.NotEmpty` | reddens | **Confirmed** — `Assert.NotEmpty() Failure: Collection was empty` | +| S3 | Empty the Setup enumeration | fails on the **controls** at `0 != 5935`, not a vacuous pass | **Confirmed** — `Assert.Equal() Failure … Expected: 5935 / Actual: 0` | +| S4 | Flip a `FromPwdBitfield` bit (`IsPK` `0x20`→`0x40`) | reddens `Build_PropagatesExactStateFlagsScaleAndFullSeedCell` | **Confirmed** — that test and only that test | +| **S1b** *(mine)* | Restore the block in **sites 2 and 3** | *(claimed covered by Headless.Tests 89/89)* | **DISPROVED — entire suite stays green** | + +**Re-hosting of the deleted test's assertions: genuine, nothing dropped.** +`Build_PropagatesExactStateFlagsScaleAndFullSeedCell` +(`tests/AcDream.App.Tests/Physics/LiveEntityCollisionBuilderTests.cs:37-72`) +carries the old test's live assertions verbatim — `registration.State` vs +`record.FinalPhysicsState`, the four PWD-bitfield flags from `0x28`, +`LandblockId` and `SeedCellId` both the full cell, and the `WorldOffsetX/Y` +pass-through (lines 64-71). Only the two fallback-shape assertions were +replaced, by the CylSphere-derived `0.8 / 2.4`, which preserves the *scale* +coverage the old name advertised (`0.4 × 2`, `1.2 × 2`). S4 proves the flag +decode is load-bearing rather than decoration. + +--- + +## 5. Findings + +### DEFECT (record, not code) + +**Finding A — "Headless.Tests 89/89 exercises the site-3 copy" is false; +two of the three production deletions are pinned by no test at all.** +`src/AcDream.Content/LandblockPhysicsContentBuilder.cs:694`, +`src/AcDream.App/Streaming/LandblockPhysicsPublisher.cs:1041`. + +Evidence, three ways: + +1. **Sabotage S1b.** I restored the invented cylinder in *both* static sites + and rebuilt. Result: App.Tests 4,172/3, Content.Tests 125, **Headless.Tests + 89/89**, Core.Tests 4,263/1 — all green. The invented footprint can be put + back into the headless-only copy and every gate the commit cites still + passes. +2. **Grep.** No `.cs` file under `tests/` references + `LandblockPhysicsContentBuilder`, `PublishStaticCollision`, or + `StaticCollisionPublication`. `PublishStaticCollision` has exactly one + caller in the tree: `HeadlessSessionWorldProjection.cs:462`. +3. **The headless suite says so itself.** + `tests/AcDream.Headless.Tests/HeadlessCollisionNeighborhoodServiceWindowTests.cs:72-77` + documents that "no lightweight DAT fixture in this test project can drive + real 3x3 publication through `CenterOn` — its dummy `IDatReaderWriter` + proxy makes `LandblockLoader.Load` fail for every landblock", and seeds + `_resident` by reflection instead. `CreatePublication` + (`HeadlessSessionWorldProjection.cs:367`) returns before line 462 on every + headless test. + +**Failure scenario this permits:** a future edit to the static Setup walk in +either publisher — reordering the Cylinder/Sphere branches, re-adding a +summary-radius shape, changing the `setupShapes.Count` gate — lands green. +The headless bot then collides against an invented footprint, or loses the +Sphere-derived cylinder entirely, and nothing in CI notices; the graphical +client is unaffected, so the divergence is invisible until someone runs a +connected headless route. + +The commit's own contract predicted this exactly (§9 trap 4: *"the inverse +trap: treating 'it's dead code' as licence to skip the headless suite"*), and +proof obligation **P5** required *"`AcDream.Headless.Tests` green **and** a +headless connected run"*. Neither half was satisfied: the suite passing proves +site 3 **compiles**, not that it **runs**, and no headless connected run is +reported. **P3** (connected registration-inventory diff) is likewise not +reported. + +**Why this is not a FAIL of the code:** the deletion at sites 2 and 3 is +textually the same eleven-line removal as site 1, its guard is provably +unsatisfiable over the installed DAT under both `> 0f` and `> 0.0001f`, and +the surrounding empty-payload handling is unchanged (§6 below). The behavior +is correct; the *claim* is not. + +**Required correction:** amend the commit record and the AP-22 retirement text +to say that sites 2 and 3 are covered only by the DAT reachability proof, that +Headless.Tests exercises the site-3 file only by compilation, and that P3/P5 +were consciously substituted by the reachability argument. Optionally file a +follow-up for a Content-layer test that drives `PublishStaticCollision` over a +synthetic `LoadedLandblock` + `FlatSetupCollision` — that would pin sites 2/3 +cheaply and would have caught S1b. + +### LATENT RISK + +**Finding E — retail's `CSetup` declares `step_down_height` before +`step_up_height`; ACE and DatReaderWriter read the opposite order.** +`docs/research/named-retail/acclient.h:31129-31132` gives +`float height; float radius; float step_down_height; float step_up_height;`, +while `references/ACE/Source/ACE.DatLoader/FileTypes/SetupModel.cs:93-94` +reads the 3rd serialized float as `StepUpHeight` and the 4th as +`StepDownHeight`; `SetupInspect` (DatReaderWriter) agrees with ACE. For the +humanoid Setup the two values are `0.600` and `1.500`, and physical +plausibility (step **up** 0.6 m, step **down** 1.5 m) favours the +ACE/DatReaderWriter reading — so this is most likely a +declaration-order-vs-serialization-order artifact, not a bug. **It does not +touch AP-22** (`radius` is field #2 and is unambiguous — 0.679 for the +humanoid on all three decoders). But `FlatSetupCollision.StepUpHeight/ +StepDownHeight` feed TS-46 remote movement and the MoveTo path, so a swap +would be a real physics divergence. Flagging for a 10-minute settle against +the `0x00521240`-region disassembly; not claiming it is wrong. + +**Finding G — the load-bearing reachability test silently passes on a machine +without DATs.** `InstalledSetupCollisionReachabilityTests.cs:47-49`: +`if (datDir is null) return;`. This matches the established suite convention +(`ConformanceDats`, `DatConcurrencyStressTests`, documented as deliberate in +`ContentConformanceDats`'s own doc comment) and is therefore **not** a new +skip or a rule violation. But for the single test the entire zero-risk +argument rests on, a silent green on CI is worth more than convention: `xUnit +v3 Assert.Skip` would make the absence visible in the 4-skip count. Style +preference, not a defect. + +### STYLE / ACCURACY + +**Finding B — "Release build 0 errors / 0 warnings" is inaccurate.** +`dotnet build AcDream.slnx -c Release` from a fully clean tree reports +**21 warnings / 0 errors**. All 21 are pre-existing and all are in *test* +projects (CS8602 ×4, CS0649 ×3, CS8767 ×3, xUnit2000/2013/2017/1025 ×5+, +Core.Tests and App.Tests). **Zero warnings in `src/`**, and this change adds +none. Presumably the claim meant the production build; it is stated as the +solution build. + +**Finding C — the reachability test's guard comment is wrong.** +`InstalledSetupCollisionReachabilityTests.cs:69`: *"The exact guard the three +deleted copies used"*. Two of the three used `Radius > 0f`, not +`> 0.0001f`, and the two predicates differ over the installed DAT by exactly +one Setup (`0x02001657`, denormal radius). The claim happens to be safe — I +verified the strict guard is also 0-reachable — but the comment should say +"site 1's guard; the strict `> 0f` variant used by sites 2 and 3 is also +verified zero", and ideally the test should evaluate both. + +**Finding D — the new AP-152 register row conflates a summary radius with a +sphere radius.** It says the cottage door `0x020019FF` has a *"~14 cm base +Sphere"*. Its base Sphere radius is **0.100 m** (bits `0x3DCCCCCD`, origin +z = 0.018); `0.1414` is the Setup's **summary `Radius`** — precisely the field +AP-152's own retail anchor establishes is not collision geometry. Cosmetic, +but it is the same conflation this commit exists to retire. + +### PROCESS (affects anyone re-verifying, not the commit) + +**Finding F — this worktree's incremental build silently serves stale +referenced assemblies; `--no-incremental` and `-t:Rebuild` are both +insufficient.** My first three Release builds each completed in ~6 s and left +a **stale `AcDream.App.dll`** in `tests/AcDream.App.Tests/bin/Release/net10.0/` +that still contained the deleted fallback. Consequence: on an apparently +"clean rebuilt" tree at `bc4679cd`, +`ShapelessSetupWithRadius_ProducesNoRegistration` failed **deterministically** +(3/3 whole-assembly runs, and also in complete isolation), and +`InstalledSetups_NeverReachTheDeletedRadiusFallback` failed under +full-solution parallelism. I spent a while treating that as a real defect. It +is not: after `rm -rf` of all 44 `bin`/`obj` directories and a fresh build, +everything passes. Note the failure mode is the mirror image of the one the +brief warned about — here a **deleted** block survived in a stale referenced +DLL and reddened the test that proves it is gone. Anyone re-checking this +commit must delete `bin`/`obj` first. + +--- + +## 6. Layering / ownership — clean + +Nothing now depends on a shape that may be absent, and the deletion does not +enlarge the set of shapeless entities (that set is *identical* before and +after, because the guard was unsatisfiable). + +- Site 1: `if (shapes.Count == 0 && !retainEmptyPayload) return null;` + (`LiveEntityCollisionBuilder.cs:146`) — unchanged, already the empty case. +- Site 2: `if (setupShapes.Count > 0)` (`LandblockPhysicsPublisher.cs:1044`) + — unchanged. +- Site 3: `if (setupShapes.Count == 0) { noCollision++; continue; }` + (`LandblockPhysicsContentBuilder.cs:697`) — unchanged; the `noCollision` + counter still accounts for it in `StaticCollisionPublication`. +- `retainEmptyPayload: true` has exactly one production caller, + `LiveEntityAppearanceBinding.cs:100`, and flows only into + `ReconcileAppearance` → `ReplaceMultiPartPayload`, which handles + `shapes.Count == 0` explicitly (`ShadowObjectRegistry.cs:705-707`: no prior + multipart + empty shapes → return). + `LiveEntityCollisionBuilder.Register` is reachable only from + `DatLiveEntityProjectionMaterializer.cs:840` with a default-path result, + which can never be empty. +- The `ShadowShapeBuilder` doc-comment correction is comment-only and does not + change `FromSetup`'s behaviour; `ShadowShapeBuilderTests` is untouched and + still pins the additive AP-152 behaviour as the register row says it does. + +--- + +## 7. Rules compliance + +- **No workaround / suppression flag / grace period / symptom guard.** The + change is a pure deletion plus comment corrections. Nothing was added to + make a symptom go away. +- **No new skips.** 4 skipped before and after (App: `TowerAscent…`, + `RadarLayoutFixtureGenerator`, `ChatLayoutFixtureGenerator`; Core: + `Pvs_CottageInterior…`) — all pre-existing. +- **No test weakened to pass.** The one replaced test gained a + discriminating assertion set (S4 proves it) and the fixture moved from + DAT-impossible to DAT-possible. The two new tests both fail under sabotage. +- **Known flakes #302/#308/#321 not conflated** — none appeared in any run. +- **Divergence register bookkeeping correct.** AP-22 retired with the + corrected three-site list and the reachability evidence; AP-152 filed with + sites, mitigation, risk and retail anchors; #330 filed for the pre-existing + headless live-entity collision gap. Register row count 127. + +## 8. Gates I ran (all on a genuinely clean build) + +| Gate | Result | +|---|---| +| `dotnet build AcDream.slnx -c Release` after deleting all bin/obj | 0 errors, 21 pre-existing test-project warnings, 0 in `src/` | +| Complete solution suite | **11,195 passed / 4 skipped / 0 failed** — matches the claim exactly | +| `AcDream.App.Tests` | **4,172 passed / 3 skipped** — matches the claim | +| `AcDream.Content.Tests` | 125 / 0 | +| `AcDream.Headless.Tests` | 89 / 0 | +| `AcDream.Core.Tests` / `Core.Net` / `Runtime` / `UI` / `Bake` / `Cli` | 4,263+1 skip / 764 / 1,217 / 546 / 15 / 4 | +| Independent raw-DAT sweep | 5,935/5,935 byte-exact, 0 fallback-reachable under both guards | +| `tools/SetupInspect` spot checks | agree bit-for-bit on `0x02000001`, `0x02000C9D`, `0x020019FF` | +| Sabotages S1, S2, S3, S4 | all reproduce as claimed | +| Sabotage S1b (mine) | disproves the site-2/site-3 coverage claim | + +Tree verified clean (`git diff HEAD` empty) after every sabotage and at the +end of the review. diff --git a/docs/research/2026-08-06-ap22-review-retail.md b/docs/research/2026-08-06-ap22-review-retail.md new file mode 100644 index 00000000..e70e9bf2 --- /dev/null +++ b/docs/research/2026-08-06-ap22-review-retail.md @@ -0,0 +1,431 @@ +# 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`. diff --git a/tests/AcDream.Content.Tests/InstalledSetupCollisionReachabilityTests.cs b/tests/AcDream.Content.Tests/InstalledSetupCollisionReachabilityTests.cs index 94d5f6f2..576643a0 100644 --- a/tests/AcDream.Content.Tests/InstalledSetupCollisionReachabilityTests.cs +++ b/tests/AcDream.Content.Tests/InstalledSetupCollisionReachabilityTests.cs @@ -44,6 +44,7 @@ public sealed class InstalledSetupCollisionReachabilityTests int withoutPrimitive = 0; int withRadius = 0; var fallbackReachable = new List(); + var fallbackReachableWideGuard = new List(); foreach (uint id in dats.GetAllIdsOfType()) { @@ -69,9 +70,20 @@ public sealed class InstalledSetupCollisionReachabilityTests if (flat.Radius > 0.0001f) withRadius++; - // The exact guard the three deleted copies used. + // BOTH guard variants the deleted copies used — they are not the + // same predicate. Site 1 (LiveEntityCollisionBuilder) tested + // `Radius > 0.0001f`; sites 2 and 3 (LandblockPhysicsPublisher, + // LandblockPhysicsContentBuilder — the headless-reachable one) + // tested the strictly wider `Radius > 0f`. Corrected 2026-08-06 at + // the AP-22 architecture review, which measured that the two + // genuinely differ over the installed DAT by exactly one Setup: + // 0x02001657, whose radius is the denormal 1.3e-39. Asserting only + // the narrow guard would have claimed coverage of two deletions it + // never evaluated. if (!hasCylinder && !hasSphere && flat.Radius > 0.0001f) fallbackReachable.Add(id); + if (!hasCylinder && !hasSphere && flat.Radius > 0f) + fallbackReachableWideGuard.Add(id); } // (b) Positive controls first: if the enumeration is broken, fail here @@ -84,5 +96,9 @@ public sealed class InstalledSetupCollisionReachabilityTests // (a) The negative claim the deletion rests on. Assert.Empty(fallbackReachable); + // The wider guard sites 2 and 3 actually used. Zero here is what makes + // the headless-reachable deletion safe; the narrow guard above does + // not evaluate it. + Assert.Empty(fallbackReachableWideGuard); } }