fix(test): evaluate BOTH deleted guards; correct AP-22's overstated coverage claim
From the AP-22 dual review (both lenses PASS). No production change.
THE RECORD WAS WRONG. bc4679cd claimed "Headless.Tests 89/89 exercises the
site-3 copy". The architecture review disproved it by sabotage: restoring the
invented cylinder in BOTH static sites left the entire suite green. No test
anywhere references PublishStaticCollision, and the headless suite's dummy DAT
proxy makes LandblockLoader.Load fail for every landblock, so CreatePublication
returns before reaching it. Two of the three deletions — including the
headless-only one — are pinned by the installed-DAT reachability proof ALONE.
The deletion is still correct; the evidence claim was not, and a successor
trusting it would think those sites had regression cover they do not have.
THE TEST NOW COVERS WHAT IT CLAIMED. Its comment said "the exact guard the
three deleted copies used", but site 1 guarded on `Radius > 0.0001f` while
sites 2 and 3 used the strictly wider `Radius > 0f`. Those are not the same
predicate: the review measured that they differ over the installed DAT by
exactly one Setup, 0x02001657, whose radius is the denormal 1.3e-39. The test
now evaluates BOTH and asserts each is empty, so the wider guard the
headless-reachable deletion actually used is no longer asserted by proxy.
Sabotage-verified: widening the new guard to `>= 0f` reddens it (1,652
zero-radius Setups appear), so the assertion is live rather than vacuously
empty over real DAT data.
AP-22's row also corrected for two precisions the reviews surfaced: the
load-bearing fact is that all 1,652 no-primitive Setups carry Radius exactly 0
(not the 1,294 first cited), and retail's `report_object_collision` DOES read
GetHeight for the quadrant field — recorded so a future reader does not mistake
it for a refutation of "never collision geometry", which is a claim about
FindObjCollisions' shape dispatch only.
Reachability now independently reproduced by four decoders — the contract's
sweep, the implementer's parser, and both reviewers' from-scratch parsers —
plus tools/SetupInspect agreeing bit-for-bit on the cited ids.
Content.Tests 125/125. No new skips; #302/#308/#321 did not fire.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
bc4679cda5
commit
619de97ad1
3 changed files with 813 additions and 1 deletions
365
docs/research/2026-08-06-ap22-review-architecture.md
Normal file
365
docs/research/2026-08-06-ap22-review-architecture.md
Normal file
|
|
@ -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.
|
||||
431
docs/research/2026-08-06-ap22-review-retail.md
Normal file
431
docs/research/2026-08-06-ap22-review-retail.md
Normal file
|
|
@ -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`.
|
||||
|
|
@ -44,6 +44,7 @@ public sealed class InstalledSetupCollisionReachabilityTests
|
|||
int withoutPrimitive = 0;
|
||||
int withRadius = 0;
|
||||
var fallbackReachable = new List<uint>();
|
||||
var fallbackReachableWideGuard = new List<uint>();
|
||||
|
||||
foreach (uint id in dats.GetAllIdsOfType<Setup>())
|
||||
{
|
||||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue