erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2
acdream/docs/research/2026-08-06-ap152-review-retail.md
Erik b52967def3 fix(physics): AP-156 — flood the BSP sphere where the geometry is, not at the part origin 
The AP-152 retail review (docs/research/2026-08-06-ap152-review-retail.md)
FAILED `4abd1b5e` and is right. `ShadowObjectRegistry.BuildFloodSpheres` took
each physics-BSP part's ROOT BOUNDING SPHERE RADIUS
(FlatCollisionAssetBuilder.cs:393 -> LiveEntityCollisionBuilder.cs:137) and
centred it on the PART ORIGIN (ShadowShapeBuilder.cs:194), discarding the root
sphere's own Origin.

Re-measured independently against the installed client_portal.dat, reproducing
the reviewer's numbers exactly: 376 of 973 physics-BSP parts have
|origin| > radius/2, worst 20.762 m on a 27.708 m sphere (gfx 0x010036DD,
Setup 0x0200129A). Over the 172 Setups AP-152 moved onto that path the emitted
flood FAILED TO CONTAIN the object's own BSP sphere for 170 of them (73
CylSphere-bearing, 97 Sphere-bearing), worst shortfall 9.911 m on Setup
0x02000255 — whose one part's sphere sits 9.911 m above the part origin — and
for 43 the post-AP-152 flood was strictly SMALLER than the pre-AP-152 one.
Indoor flooding is 3-D (CellTransit.cs:601 routes every id & 0xFFFF >= 0x0100
candidate through FindTransitCellsSphere), so a tall prop or door slab was
absent from EnvCells it physically occupies and therefore never a broadphase
candidate there (TransitionTypes.cs:3763 iterates only entries already in the
cell). That is the #98 / #168 class AP-152 exists to remove.

Retail, re-disassembled from the PDB-paired binary (check_exe_pdb.py MATCH,
CodeView GUID 9e847e2f-777c-4bd9-886c-22256bb87f32), every address resolved
back through named-retail/symbols.json:

  CGfxObj::physics_sphere is [gfxobj+0x74] (physics_bsp is [+0x78], as
  CPartArray::CacheHasPhysicsBSP @0x00518110 reads at 0x00518127), and
  acclient pseudo-C 0x00534b5b assigns it BSPTREE::GetSphere(physics_bsp).

  BSPTREE::GetSphere @0x005397e0
    8b01        mov eax,[ecx]     ; BSPTREE::root_node
    83c004      add eax,4         ; past BSPNODE::vfptr -> CSphere sphere
  So retail's per-part flood sphere IS the BSP root bounding sphere,
  ORIGIN INCLUDED (acclient.h: BSPNODE { vfptr; CSphere sphere; ... },
  CSphere { Vector3 center; float radius; } -> radius at +0xc).

  CPhysicsObj::find_bbox_cell_list @0x00510fc0 adds the object's own cell and
  then walks the PART ARRAY: 0x00511012 call 0x518160
  (CPartArray::calc_cross_cells_static), which dispatches [edx+0x7c] with
  (num_parts, parts, cellarray). Its EnvCell body,
  CEnvCell::find_transit_cells @0x0052cae0:
    0x0052cb31  mov edx,[eax+0x20]   ; CPhysicsPart::gfxobj (CGfxObj**)
    0x0052cb36  mov esi,[ecx+0x74]   ; physics_sphere (else +0x90 drawing)
    0x0052cb4c  add eax,0x30         ; CPhysicsPart::pos
    0x0052cb5a  call Position::localtolocal   ; transform the sphere CENTRE
    0x0052cb65  fadd [esi+0xc]       ; only NOW the radius
  Retail transforms the centre through the part's own Position before it ever
  touches the radius. Carrying the radius alone is not an approximation of
  that; it is a different sphere.

Changes:

* `ShadowShape` gains `BoundsCenter` — the bounding sphere's centre in the
  shape's own local frame, scaled like LocalPosition and Radius. Zero for
  Cylinder/Sphere shapes, whose LocalPosition already IS their centre.

* `ShadowShapeBuilder.FromSetup` gains a `physicsBspBounds` resolver that
  supplies radius AND centre from ONE call, replacing the placeholder radius
  plus a downstream substitution. `LiveEntityCollisionBuilder` now holds a
  single `Func<uint, FlatCollisionSphere?>` and derives its dispatch predicate
  from it, so the gate and the geometry cannot disagree and the radius cannot
  be taken while the origin is dropped. That split is what produced this bug;
  it no longer exists.

* `FromLandblockBspParts` carries the centre too. A landblock-baked part array
  is the same CPartArray walk, so stair runs, fences and rock clusters had the
  identical defect. Both storage forms (flat BSP and the graph fallback) are
  covered.

* `BuildFloodSpheres` places each sphere at
  partWorldPos + rotate(BoundsCenter, partWorldRot), composed exactly as the
  ShadowEntry rows are.

* The 10-sphere clamp now applies to the CYLSPHERE branch only. Retail's clamp
  is inside CObjCell::find_cell_list @0x0052b9f0
  (0x0052ba21 cmp eax,0xa / 0x0052ba28 mov ebp,0xa); the BSP walk has none and
  the sorting-sphere overload @0x0052b990 takes one sphere. 7 installed Setups
  carry more than 10 physics-BSP parts (max 49, Setup 0x02001A91) and their
  tail parts were dropped from the flood entirely. Without this the new
  containment assertion would have covered shapes production never floods
  from.

Register. AP-155 was two divergences with different code paths, populations
and gates under one id; it is NARROWED to its static-publication half and its
flood half is split out as AP-156 WITH ITS DIRECTION CORRECTED. AP-155(b)
recorded the approximation as over-inclusive — "floods MORE cells rather than
fewer, the safe direction for membership" — and that false direction was the
stated reason the residual was safe to defer. It was under-inclusive for 170
of 172. AP-156 records the correction, this fix, and the one genuine residual:
acdream's sphere-vs-portal traversal where retail walks each part's sphere
against the cell's own portal planes. AP-155(b)'s "acdream approximates
retail's bounding BOX" was wrong too — find_bbox_cell_list forms no box.
AP-157 filed for the review's F4: retail's third branch floods from ONE
CPartArray::GetSortingSphere @0x00518b00 ([partArray+0x54]+0x70 =
CSetup::sorting_sphere; 4,154 of 5,935 installed Setups carry a non-zero one)
where acdream floods from every Sphere shape, and acdream's cylinder flood
ignores CylHeight. Deliberately NOT bundled here: different branch, disjoint
population, different live gate. Active AP rows 107 -> 109, literal count.

Tests. Both flood tests the review named substituted a CONCENTRIC Radius = 14f
at LocalPosition = Zero — the one configuration in which the defect cannot
appear. Every fixture is now off-centre by default, and
`FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` drives the production
`physicsBspBounds` seam instead of hand-substituting. Five new facts: the
flood centres on BoundsCenter not the part origin; it rotates BoundsCenter by
the part rotation; it caps cylspheres at ten but never the BSP parts; the
landblock path carries the scaled centre in both storage forms; and an
installed-DAT containment sweep asserting every emitted BSP flood sphere
contains that part's real bounding sphere at entity scale 1.75, behind four
external controls — 973 parts, 376 off-centre, 172 affected, and 170
would-fail-if-the-origin-were-discarded, the last of which fails if the
population ever stops exercising the field.

Nine sabotages, each reverted and re-verified:
  A drop BoundsCenter from the flood       -> 3 Core
  B rotate by entity rot, not part rot     -> 1 Core (the rotation fact only)
  C FromSetup discards the origin          -> 1 Core + 2 App + 1 Content
     (the shipped defect, now caught in three projects)
  D drop entScale on BoundsCenter          -> 2 App + 1 Content
  E landblock flat branch drops the centre -> 1 Core
  F landblock graph branch drops it        -> 1 Core
  G drop partScale on the landblock centre -> 1 Core
  H re-apply the 10-cap to every branch    -> 1 Core
  I remove the cylsphere cap               -> 1 Core
AP-152's own two sabotages re-run against this tree: the step-0 gate disabled
still reddens exactly its five facts with Headless 89/89 green, and
cylinder-first flooding still reddens exactly one.

Clean Release build after deleting all 44 bin/obj: 0 errors, 21 pre-existing
warnings. Complete suite 11,208 passed / 4 skipped / 0 failed, +5 on the
11,203 baseline at 4abd1b5e — Core 4264 -> 4268, Content 126 -> 127, App
unchanged (one rename, not an addition). No new skips.

NOT yet gated live. This moves shadow-cell membership for real objects, in
both directions, and the connected session must look for both: props and doors
that START blocking from a neighbouring cell (the 73 CylSphere+BSP Setups),
AND ones that STOP blocking (the 99 Sphere+BSP Setups can shrink; 43 shrink
below their pre-4abd1b5e size, which is the regression this fixes). Tall
indoor props and door slabs — the ones whose sphere sits metres above the part
origin — are where the change is largest.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-06 15:52:04 +02:00

416 lines
23 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# AP-152 retail-conformance review — commit `4abd1b5e`
**Reviewer role:** adversarial retail-conformance.
**Scope:** `ShadowShapeBuilder.FromSetup` step-0 dispatch gate,
`ShadowObjectRegistry.BuildFloodSpheres` priority, the AP-152 retirement text,
and the AP-153/AP-154/AP-155 filings.
**Method:** every retail claim re-derived from the PDB-paired binary
(`C:\Users\erikn\Downloads\acclient.exe`, `check_exe_pdb.py` → `=== MATCH ===`,
linker 2013-09-06T00:17:56Z, CodeView GUID
`9e847e2f-777c-4bd9-886c-22256bb87f32`) with capstone + `pefile`, resolving every
address through `named-retail/symbols.json`. Neither the commit body's nor the
contract's quoted disassembly was used as input. Population facts re-measured
independently against the installed `client_portal.dat` with a scratchpad
`Chorizite.DatReaderWriter` sweep (not the repo's test).
---
## VERDICT: **FAIL**
The retail port itself is faithful — **every** disassembly claim in the commit
and in the AP-152 retirement text checks out byte-exact, and the change makes
acdream match `CPhysicsObj::FindObjCollisions` and `CPhysicsObj::calc_cross_cells`
where it previously did not. The failure is not in the dispatch port. It is that
the commit **moves 172 Setups onto a flood-sphere approximation whose direction
the register records backwards**, and the false direction is the stated reason
the residual was safe to defer. Measured over the installed DATs: the new flood
fails to contain the object's own BSP bounding sphere for **170 of the 172**
affected Setups, shortfall up to **9.911 m**, and for **43 of them** the flood is
strictly *smaller* than what the code produced before this commit. That is the
#98 / #168 under-inclusive membership class — the exact class the commit's
thesis says it removes.
---
## Part 1 — Independent retail verification (all PASS)
### 1.1 `CPhysicsObj::calc_cross_cells` @`0x00515230`
Symbol resolves exactly (`symbols.json` → `CPhysicsObj::calc_cross_cells`,
offset 0). Disassembled:
```
0x00515285 f786a800000000000100 test dword ptr [esi + 0xa8], 0x10000
0x0051528f 7574 jne 0x515305 ; -> BSP
0x00515291 8b4e10 mov ecx, [esi + 0x10] ; part array
0x00515296 7444 je 0x5152dc ; none -> sorting sphere
0x00515298 e8e32d0000 call 0x518080 ; CPartArray::GetNumCylsphere
0x0051529f 743b je 0x5152dc ; zero -> sorting sphere
...
0x005152d1 e81a670100 call 0x52b9f0 ; CObjCell::find_cell_list (cylsphere)
0x005152da eb35 jmp 0x515311 ; PAST the sorting-sphere branch
0x005152dc ...
0x005152e3 e818380000 call 0x518b00 ; CPartArray::GetSortingSphere
0x005152fb e890660100 call 0x52b990 ; CObjCell::find_cell_list (sorting sphere)
0x00515305 680c3f8400 push 0x843f0c
0x0051530c e8afbcffff call 0x510fc0 ; CPhysicsObj::find_bbox_cell_list
```
- The `test`/`jne` pair is at exactly `0x00515285` / `0x0051528f` as claimed. **PASS.**
- `jne` target `0x515305` calls `0x00510fc0` = `CPhysicsObj::find_bbox_cell_list`. **PASS.**
- Cylsphere call at `0x005152d1` → `0x0052b9f0`, sorting-sphere call at
`0x005152fb` → `0x0052b990`; both **below** the jump and unreachable from it. **PASS.**
- **Order (review item 2):** confirmed by fall-through, not merely by "BSP wins".
Both `je 0x5152dc` guards (null part array; `GetNumCylsphere == 0`) skip to the
sorting-sphere branch, and the cylsphere branch's `jmp 0x515311` at `0x005152da`
jumps **past** the sorting-sphere branch. Precedence is therefore
**BSP → CylSphere → sorting sphere**, exclusive at every step. **PASS.**
`0x0052b9f0` and `0x0052b990` are two overloads of the same symbol
`CObjCell::find_cell_list`; the commit distinguishes them correctly by argument
shape. Retail's 10-sphere cap is confirmed independently inside the cylsphere
overload at `0x0052ba21 cmp eax, 0xa`, matching `RetailSphereCap = 10`.
### 1.2 `CPhysicsObj::FindObjCollisions` @`0x0050f050` (review item 3)
Left untouched by the commit; re-confirmed so "already correct" is earned:
```
0x0050f165 f786a800000000000100 test dword ptr [esi + 0xa8], 0x10000
0x0050f16f 7431 je 0x50f1a2 ; clear -> primitive dispatch
0x0050f18d e8ee8f0000 call 0x518180 ; CPartArray::FindObjCollisions
0x0050f19d e90e010000 jmp 0x50f2b0 ; UNCONDITIONAL
0x0050f1a2 ... ; CylSphere loop head
0x0050f1d6 0f833b010000 jae 0x50f317 ; loop exhausted -> RETURN
0x0050f21d ... ; Sphere loop head
0x0050f22f 0f84e6000000 je 0x50f31b ; zero Spheres -> RETURN seeded OK_TS
```
All five cited instruction addresses are byte-exact and the `jmp 0x50f2b0` is
past both `0x50f1a2` and `0x50f21d`. The dispatch is exclusive. **PASS** — the
query path genuinely did not need changing. (`ebp` at `0x0050f171`/`0x0050f1b2`/
`0x0050f235` is the ethereal/ignore early-out, not a second shape branch.)
### 1.3 `CacheHasPhysicsBSP` and AP-153's "exactly one caller" (review item 4)
Two distinct symbols, both cited correctly in different places:
`CPhysicsObj::CacheHasPhysicsBSP` @`0x0050f570` and
`CPartArray::CacheHasPhysicsBSP` @`0x00518110`.
Full `.text` + `.rdata` scan for `E8`/`E9` rel32 and absolute-dword references:
| Target | Refs found |
|---|---|
| `CPhysicsObj::CacheHasPhysicsBSP` `0x0050f570` | **1** — `call` at `0x0051272e` = `CPhysicsObj::InitPartArrayObject+0x7e` |
| `CPartArray::CacheHasPhysicsBSP` `0x00518110` | **1** — `call` at `0x0050f57d` = `CPhysicsObj::CacheHasPhysicsBSP+0xd` |
**PASS**, and stronger than filed: `InitPartArrayObject` @`0x005126b0` itself has
exactly three callers, all construction — `CPhysicsObj::InitNullObject+0x1f`,
`CPhysicsObj::makeObject+0x3b`, `CBuildingObj::makeBuilding+0x3b`. And
`CPartArray::SetPart` @`0x00518580` (the `AnimPartChanged` swap site) calls
`CPhysicsPart::SetPart` @`0x0050e700` per part and never re-caches. So retail's
dispatch flag is derived once at construction and is genuinely stale after a part
swap. **AP-153 is honestly scoped** and could legitimately claim more evidence
than it does.
`CPartArray::CacheHasPhysicsBSP`'s body confirms the derivation the acdream gate
imitates: walk `[ecx+0x5c][i]` parts, deref `[part+0x20]` → gfxobj, test
`[gfxobj+0x78]` (physics BSP), OR `0x10000` into `[ecx]` on the first hit.
### 1.4 Every cited address resolves to the symbol claimed (review item 5)
| Address | Resolves to | Verdict |
|---|---|---|
| `0x00515230` | `CPhysicsObj::calc_cross_cells` | ✔ |
| `0x0050f050` | `CPhysicsObj::FindObjCollisions` | ✔ |
| `0x00510fc0` | `CPhysicsObj::find_bbox_cell_list` | ✔ |
| `0x0052b9f0` / `0x0052b990` | `CObjCell::find_cell_list` (two overloads) | ✔ |
| `0x0050f570` | `CPhysicsObj::CacheHasPhysicsBSP` | ✔ |
| `0x00518110` | `CPartArray::CacheHasPhysicsBSP` | ✔ |
| `0x00518180` | `CPartArray::FindObjCollisions` | ✔ |
| `0x0050d8d0` | `CPhysicsPart::find_obj_collisions` | ✔ |
| `0x00537a80` / `0x00537fd0` | `CSphere::intersects_sphere` (two overloads) | ✔ |
| `0x00518b00` | `CPartArray::GetSortingSphere` | ✔ |
| `0x0051272e` | `CPhysicsObj::InitPartArrayObject+0x7e` | ✔ |
| `0x00518060/70/80/90` | `GetNumSphere` / `GetSphere` / `GetNumCylsphere` / `GetCylsphere` | ✔ |
No mis-citation found.
### 1.5 The four corrections (all re-measured from the installed DAT)
| Correction | Claim | Measured | Verdict |
|---|---|---|---|
| 2a | max affected primitive = **6.714 m** CylSphere on `0x02001741` | `0x02001741` cyl[0] r = **6.714**, h = 1.476 — the max over the 172 affected | ✔ |
| 2b | `0x0200086E` Sphere origin `(0.759, 0.165, 5.842)` | sph[0] origin `(0.758796, 0.165414, 5.842)`, r = 5.842 | ✔ |
| 3 | cottage door `0x020019FF` Sphere = **0.100 m**; `0.141` is `Setup.Radius` | sph[0] r = **0.1** @ `(0, 0, 0.018)`; `Setup.Radius` = **0.14142136** | ✔ |
| 4 | **two** pinning tests, both corrected, neither deleted | `FromSetup_DoorSetup_ProducesFourShapes` → `_EmitsBspPartsOnly`, and `FromSetup_DoorSetup_SphereAtExpectedLocalOffset` re-hosted `_ => true` → `_ => false` | ✔ |
Note the correction is *scoped to the affected 172*, which is legitimate but
unstated: the largest primitive anywhere in the DAT is a **15.0 m** Sphere on
`0x02000D7D`. Worth one clause in the row so a later reader doesn't re-derive
6.714 as a global bound.
### 1.6 Register bookkeeping
- **AP-152 retirement is earned, not asserted.** `~~AP-152~~` struck through,
past tense, evidence column populated, corrections enumerated. Sabotage-verified
both halves (below).
- **AP row count = literal 107.** Parsed the section: 130 AP rows, 23 struck, **107
active**. Baseline at `ec29a732` = 105 active. 105 + 3 (AP-153/154/155) − 1
(AP-152) = **107**. Reconciles.
- **AP-154's grep claim is exact.** `grep -rn "PhysicsStateFlags.HasPhysicsBsp" src/`
→ exactly two hits: `TransitionTypes.cs:1349` (the predicate) and
`PhysicsEngine.cs:1614`, which reads `request.MoverPhysicsState` — an unrelated
*mover*-state read, as the row says.
- **ACE derivation confirmed.** `references/ACE/Source/ACE.Server/WorldObjects/WorldObject_Networking.cs:666-669`
— `if (CSetup.HasPhysicsBSP) physicsState |= PhysicsState.HasPhysicsBSP; else &= ~`.
- **Static-publisher homogeneity claim earned.** `LandblockPhysicsPublisher.cs:983`
gates the Setup-primitive block on `entityBspCount == 0`, so that path already
dispatched exclusively and emits Cylinder-typed shapes only.
- **Blast-radius claim earned.** `ShadowShapeBuilder.FromSetup`'s only production
caller is `src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:126`
(`internal sealed`, AcDream.App) — unreachable from Headless.
### 1.7 Build and suite
`bin`/`obj` deleted repo-wide, then Release build (0 errors) and full suite:
```
AcDream.Cli.Tests 4 / 0 skip
AcDream.Content.Tests 126 / 0
AcDream.UI.Abstractions.Tests 546 / 0
AcDream.Runtime.Tests 1222 / 0
AcDream.Bake.Tests 15 / 0
AcDream.Headless.Tests 89 / 0
AcDream.App.Tests 4173 / 3 skip
AcDream.Core.Net.Tests 764 / 0
AcDream.Core.Tests 4264 / 1 skip
--------------------------------------------
11,203 passed / 4 skipped / 0 failed
```
Exactly the commit's stated numbers, including Headless 89/89.
### 1.8 Sabotage (both restored; tree confirmed clean afterwards)
| Sabotage | Reddened |
|---|---|
| A — `if (!anyPhysicsBspPart)` → `\|\| true` (restore the additive union) | `FromSetup_DoorSetup_EmitsBspPartsOnly`, `FromSetup_DispatchGateReadsTheEffectivePartIdentities`, `FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` — 3 failed / 20 passed |
| B — `BuildFloodSpheres` `only` → cylinder-first (drop the BSP arm) | `BuildFloodSpheres_BspBearingOwner_FloodsFromBspNotFromCylinder` only — 1 failed / 9 passed |
Both facts are load-bearing and neither is over-broad. Sabotage B leaving
`FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` green is *correct* and
corroborates the commit's own statement that the `BuildFloodSpheres` half is
behaviour-neutral against today's producers.
---
## Part 2 — Findings, by severity
### F1 (HIGH) — AP-155(b)'s "over-inclusive, the safe direction" is empirically inverted, and it is the justification for deferring the residual
**Register text (AP-155, Risk column):** *"(b)'s bounding-sphere approximation is
over-inclusive (a sphere contains the box's inscribed extent but is larger in the
diagonal), which floods MORE cells rather than fewer — the safe direction for
membership."*
That reasoning holds only if the sphere is **concentric** with the geometry. It
is not. Production builds the BSP flood sphere from two different sources:
- **centre** — `src/AcDream.Core/Physics/ShadowShapeBuilder.cs:194`:
`LocalPosition = partFrame.Origin * entScale`, the part's *placement-frame*
origin;
- **radius** — `src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:137` →
`_physicsBspRadius` → `src/AcDream.Core/Physics/FlatCollisionAssetBuilder.cs:393`,
`PhysicsBSP.Root.BoundingSphere.**Radius**`.
The BSP root sphere's own **`Origin` is discarded**. `BuildFloodSpheres`
(`src/AcDream.Core/Physics/ShadowObjectRegistry.cs:661`) then floods from
`entityWorldPos + rotate(LocalPosition)` — the part origin — with that radius.
**Measured over the installed `client_portal.dat`** (5,935 Setups; 530 carry a
physics-BSP part; 973 BSP parts total):
- **376 of 973** BSP parts have `|rootSphere.Origin| > radius/2`. Worst:
**20.762 m** offset on a 27.708 m sphere (`gfx 0x010036DD`, Setup `0x0200129A`).
- Over the **172 Setups this commit moves onto that path**, the emitted flood
sphere set fails to contain the true BSP bounding sphere in **170** cases
(73 CylSphere-bearing, 97 Sphere-bearing). Only 2 are covered.
- Worst shortfall **9.911 m**, Setup `0x02000255`: one part, `gfx 0x01000A90`,
BSP root sphere `origin = (0.000, −0.007, 9.911)`, `radius = 10.522`
(`Setup.Height = 18.692`). Production floods from a sphere centred at
`(0,0,0)` — 9.9 m below the geometry's own collision centre.
**Observable in-game consequence.** Outdoor land flood (`AddAllOutsideCells`) is
XY-driven and mostly forgives this. Indoor / building flood is not:
`CellTransit.BuildShadowCellSet` (`src/AcDream.Core/Physics/CellTransit.cs:601`)
routes every candidate cell with `id & 0xFFFF >= 0x0100` through
`FindTransitCellsSphere`, a 3-D sphere-vs-portal test. A tall dungeon or building
prop whose BSP sphere sits several metres above the part origin will not be
registered into the EnvCells it physically occupies. It is then never a broadphase
candidate there at all (`TransitionTypes.cs:3763` only iterates entries already in
the cell), so it does not collide: **walk through the upper part of a tall indoor
prop, or through a door slab from the storey above/below it**. That is precisely
the #98 / #168 class.
The register does not merely omit this — it records the opposite and uses the
recorded direction as the reason the residual is safe to leave open. Under the
C4-handoff process finding *"a contract asserting a mechanism that does not exist
caused three separate defects"*, this is the same failure mode, in a register row
rather than a code contract.
Two secondary inaccuracies in the same row's retail characterisation:
- *"acdream approximates retail's bounding BOX"* — `find_bbox_cell_list`
@`0x00510fc0` adds the object's own cell and then calls
`CPartArray::calc_cross_cells_static` @`0x00518160`, which dispatches the
virtual at `[cell_vtbl+0x7c]` with `(numParts, parts, cellarray)` — a
`find_transit_cells` part-array overload (`CObjCell::find_transit_cells`
`0x0052b070`/`0x0052b080`; `CEnvCell`'s pair sits at `.rdata` `0x007c8d14`/
`0x007c8d18`). Retail walks the **actual per-part geometry** through cell
portals despite the function's name. The approximation is coarser than the row
admits, which widens F1 rather than narrowing it.
### F2 (HIGH) — 43 of the 172 affected Setups get a strictly *smaller* flood than before this commit
A direct consequence of F1, but it needs stating separately because it
contradicts the commit's own thesis and is unguarded by any test.
Modelling the pre-`4abd1b5e` `BuildFloodSpheres` (primitives preferred when any
Cylinder exists; otherwise everything, cap 10) against the post-commit set over
the same 172 Setups:
| | count |
|---|---|
| new flood ⊇ old flood | 129 |
| **new flood ⊉ old flood** | **43** (22 CylSphere-bearing, 21 Sphere-bearing) |
Worst: **3.493 m** on `0x0200086E` — `Setup.Height = 11.684`, whose Sphere at
`(0.759, 0.165, 5.842) r 5.842` reached `z ≈ 11.68`, while the two surviving BSP
flood spheres (`gfx 0x01001B2B` r 9.015, `gfx 0x01001BB2` r 9.254, both centred at
their part origins) reach only `z ≈ 9.0`. Next: `0x020015D4` 2.405 m,
`0x02000359` 2.083 m, `0x02001761` 1.889 m.
The commit says the change removes an under-inclusive membership defect. For
these 43 Setups it introduces one. The commit's "NOT yet gated live" note is the
right instinct; the connected gate must specifically look for props and doors
that stopped blocking, not only for ones that started.
**Neither new test covers this.** `BuildFloodSpheres_BspBearingOwner_...` and
`FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` both hand-substitute a
synthetic `Radius = 14f` at `LocalPosition = Vector3.Zero`, i.e. a *concentric*
BSP sphere — exactly the configuration in which F1 cannot fire.
### F3 (MEDIUM) — `RegisterMultiPart`'s own doc comment still states the superseded rule
`src/AcDream.Core/Physics/ShadowObjectRegistry.cs:432-439` (the sole caller of
`BuildFloodSpheres`, 168 lines above it) still reads:
> *"Flood spheres follow retail's rule (Ghidra 0x0052b9f0): when the object has
> CylSpheres, they alone drive the flood (base point + cyl radius, capped at 10);
> otherwise the BSP parts' bounding spheres stand in for the sorting sphere."*
That is the pre-`4abd1b5e` rule and is now false — it is the rule sabotage B
restored, and sabotage B reddened a test. The commit rewrote
`BuildFloodSpheres`' own doc thoroughly and left its caller's contradicting it.
The next reader who greps `RegisterMultiPart` before `BuildFloodSpheres` gets the
wrong mental model of the very behaviour this commit changed.
### F4 (MEDIUM) — unregistered divergence: retail's third branch is ONE sorting sphere, acdream's is every Sphere shape
Retail's `calc_cross_cells` fall-through calls `CPartArray::GetSortingSphere`
@`0x00518b00`, which returns `[partArray+0x54] + 0x70` — a single authored
whole-object sphere on the `CSetup` — and floods from that one sphere
(`0x005152fb`). acdream's `only == null` branch
(`ShadowObjectRegistry.cs:652`) applies no filter and floods from **every**
non-BSP, non-Cylinder shape, i.e. the Setup's per-part Sphere array.
Different DAT field, different cardinality, different extent. The source comment
at `ShadowObjectRegistry.cs:646-648` admits the substitution
(*"which acdream approximates with the remaining shapes' bounding spheres"*), and
AP-155's anchor column even cites `CPartArray::GetSortingSphere 0x00518b00` — but
no AP row states the divergence. The register's own rule ("any commit that
introduces a deviation adds its register row IN THE SAME COMMIT") is not
retroactive, and this predates `4abd1b5e`; but the commit rewrote this exact
method, filed three rows for its neighbours, and stepped over this one.
Same class, smaller: `BuildFloodSpheres` collapses a Cylinder to a single sphere
at its base point with the cylinder radius and **ignores `CylHeight` entirely**,
where retail's `CObjCell::find_cell_list` @`0x0052b9f0` is handed the CCylSphere
array `(low_pt, radius, height)`. Also unregistered.
### F5 (LOW) — AP-155 is two divergences in one row; AP-153 and AP-154 are honestly distinct
Asked whether one of AP-153/154/155 is the same divergence sliced twice: **no**,
but AP-155 has the inverse problem.
- **AP-153** (flag cached at construction vs. re-derived live) and **AP-154**
(flag taken off the wire vs. derived client-side) concern the same bit but are
genuinely different questions — *when* vs. *where from* — with different sites
(`ShadowShapeBuilder` step 0 / `LiveEntityCollisionBuilder.ReconcileAppearance`
vs. `TransitionTypes.cs:1348` / `LiveEntityCollisionBuilder.cs:161`), different
risks, and different gates. Both scoped honestly.
- **AP-155** bundles (a) the static publishers' Setup-Sphere→height-capped-Cylinder
conversion — a *different* code path, a *different and larger* population, and
its own stated gate — with (b) the flood-priority/approximation question, and
then files (b) as already half-closed by the same commit. Three different
lifecycles under one id. It should be two rows (or three), and (b)'s open
remainder needs the F1 correction before it can be reasoned about at all.
### F6 (LOW) — commit-body imprecision
*"Priority at both consumers: BSP -> CylSphere -> Sphere -> nothing."* At
`FindObjCollisions` the third branch is `CPartArray::GetSphere` @`0x00518070`
(the per-part Sphere array, loop head `0x0050f21d`). At `calc_cross_cells` it is
`CPartArray::GetSortingSphere` @`0x00518b00` (one authored whole-object sphere,
`0x005152e3`). Different arrays; "both consumers" is not true of the third rung.
The register row and the source comments state this correctly — only the commit
message is loose. Recorded so a future grep of the log doesn't inherit it.
---
## What was checked and found clean (so the PASS half is auditable)
- Binary/PDB pairing (`MATCH`), and every disassembly re-derived from that binary
rather than Binary Ninja.
- `calc_cross_cells` full body: flag test address, jump target, both primitive
call sites, and the **fall-through order** including the cylsphere branch's
skip-past-sorting-sphere `jmp`.
- `FindObjCollisions` full body: all five cited addresses, the unconditional
`jmp` past both loops, and identification of `ebp` as the ethereal early-out
rather than a shape branch.
- `CacheHasPhysicsBSP` (both classes) bodies + exhaustive rel32/absolute xref scan;
`InitPartArrayObject` callers; `CPartArray::SetPart` non-recaching.
- `find_bbox_cell_list` → `calc_cross_cells_static` → `[vtbl+0x7c]` chain.
- `GetSortingSphere` body (`[+0x54]+0x70`).
- Retail's 10-sphere cap (`0x0052ba21 cmp eax, 0xa`).
- 12 distinct cited addresses → symbol, no mis-citation.
- All four register corrections re-measured from the installed DAT by an
independent sweep; population 5,935 / 172 / 73 / 99 / 530 reproduced exactly.
- AP active-row count parsed (107) and reconciled against the `ec29a732` baseline (105).
- AP-154's `src/` grep claim (exactly 2 hits, second is a mover-state read).
- ACE's derivation of the wire bit (`WorldObject_Networking.cs:666-669`).
- Static publishers' `entityBspCount == 0` exclusivity gate.
- `FromSetup`'s sole production caller is App-layer (Headless-unreachable).
- `EffectivePartGfxObjId` genuinely shared by step 0 and step 3 — the
"gate and emission read the same identity" trap is real and its test
discriminates in both directions.
- Clean `bin`/`obj` → Release build 0 errors → full suite 11,203 / 4 skip / 0 fail.
- Two sabotages, both reddening only in the intended direction; tree restored and
`git status --porcelain` empty.
## Recommended before the connected gate
1. Correct AP-155's Risk column: the approximation is **under**-inclusive for
170 of the 172 Setups this commit moved onto it, not over-inclusive.
2. Carry the BSP root sphere's `Origin` through `ShadowShape` (or offset
`LocalPosition` by `partFrame.Orientation * rootSphere.Origin`) so the flood
sphere is concentric with the geometry it stands for. That is a one-field fix
at `ShadowShapeBuilder.cs:192-199` + `LiveEntityCollisionBuilder.cs:135-139`,
and it converts F1/F2 from open risk to closed.
3. Add a non-concentric fixture to `ShadowObjectRegistryMultiPartTests` — the two
new flood tests both use `LocalPosition = Zero`, which is the one configuration
where the defect cannot appear.
4. Fix the stale `RegisterMultiPart` doc (F3) and split AP-155 (F5).
5. Instruct the connected gate to look for props/doors that **stopped** blocking
as well as ones that started; 43 Setups shrank.
Reference in a new issue View git blame Copy permalink