e6457cc849
Both review lenses PASSED; this is the cleanup, not a rescue. Evidence:
docs/research/2026-08-06-ap156-review-closure.md (the review itself is
committed alongside it as the received artifact).
R1 — the load-bearing containment test could not fail. Its truth and flood
values were two hand-copies of the same expression over the same part set,
so the shortfall was algebraically identically zero for any DAT input. The
oracle is now PHYSICS-POLYGON VERTICES — a different DAT field from the
bounding sphere the builder emits, so the two sides can genuinely disagree.
Sabotage-verified three ways after full cleans: dropping the bounds centre
in production reddens it (428 Setups, worst 35.869 m on 0x0200129A, matching
an independent out-of-repo sweep exactly); dropping only the scale on the
centre reddens it (326); and corrupting the TEST's own bounds oracle reddens
it (467) where under the shipped oracle that same corruption was invisible
by algebra. Renamed accordingly. A6's stale "cap control" comment corrected:
that loop is the test's own uncapped re-implementation and cannot observe a
cap regression — the cap is covered in Core.
R2 — the population was understated. 172 is AP-152's DISPATCH population;
AP-156's is 530 BSP-bearing Setups, of which 525 have a flood sphere move
and 428 fail vertex containment before the fix (412 at a 1 cm tolerance —
the review's figure; the gap is 16 Setups between 1.4 mm and 10 mm, real
geometry). 0 fail after, at any tolerance down to zero. Corrected in the
AP-156 row, the section-3 header, the C5c handoff and two test docstrings.
Dated review artifacts are left as written — "170 of 172" was correct for
what they measured, and rewriting evidence to match a later measurement
loses provenance.
A1 — BoundsCenter = default reopened at the type what the commit closed at
the seam. Dropping the default alone would NOT have closed the review's own
scenario (a copied Cylinder call site would write Vector3.Zero explicitly
and stay green), so ShadowShape's constructor is now private and BSP shapes
are built only through ShadowShape.Bsp(..., FlatCollisionSphere localBounds),
which takes radius and centre as ONE value and scales them together. There
is no expression a caller can write that carries one and drops the other.
22 construction sites converted; the same sabotage now reddens 5 Core tests
where the review's sabotage A reached 4, because both BSP producers share
one scaling path.
A2 — #333 is real and bigger than filed, and its retail question is
answered. I disassembled CObjCell::find_obj_collisions @0x0052b750 from the
PDB-paired binary myself (check_exe_pdb.py MATCH) rather than inheriting the
claim: its only early-out is sphere_path.insert_type == INITIAL_PLACEMENT_
INSERT, then it calls FindObjCollisions on every unparented non-self shadow
object UNCONDITIONALLY. Retail has NO distance pre-filter, so acdream's
"+ movement + 2f" reach filter is an invention with no register row — filed
as AP-158, carrying the disassembly, the F_EPSILON = 0.0002 m contrast, and
the measured blast radius (118 of 477 unique installed physics-BSP GfxObjs
exceed its ~2.5 m budget, 46 exceed 5 m). Active AP rows 109 -> 110.
Recorded prominently in three places a reader will hit: TALL PROPS MAY SHOW
NO VISIBLE CHANGE UNTIL #333 LANDS, and a null result at the connected gate
is EXPECTED, not evidence against AP-156.
LOW items. R3: the comment claiming the cited evidence justified the whole
cap line is corrected, but int.MaxValue on the sorting-sphere branch stays —
capping at 1 would take Spheres[0], and retail's one sphere is
CSetup::sorting_sphere, a different DAT field; capping keeps the wrong field
AND flips the substitution under-inclusive (#98/#168 direction). AP-157
already owns it. R4: acdream scales the flood sphere where retail's
find_transit_cells never reads gfxobj_scale — added as a second residual on
AP-156. R5: retail's slack constant carried into AP-158 and #333. A3: the
per-call delegate allocation is back to a cached field, still derived from
the single bounds resolver. A5: noted; b52967de's message cannot be amended.
Gates: all 44 bin/obj deleted before every verdict-deciding build, each test
run gated on a verified "Build succeeded" in the same invocation. Release
build 0 errors / 21 pre-existing warnings. Complete suite 11,208 passed /
4 skipped / 0 failed — reconciles exactly with the e2b2d04c baseline; one
test renamed, none added, removed or skipped. Nothing conflated with the
known load-sensitive flakes #302 / #308 / #321.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
719 lines
34 KiB
Markdown
719 lines
34 KiB
Markdown
# AP-156 fix review — `b52967de` + `e2b2d04c`
|
||
|
||
Adversarial dual-lens review of the AP-152 flood-sphere fix, prompted by the
|
||
FAIL in `docs/research/2026-08-06-ap152-review-retail.md`.
|
||
|
||
Worktree `.claude/worktrees/resume-session-e0bd03e1-d5bf45`, HEAD `e2b2d04c`.
|
||
All 44 `bin`/`obj` deleted before every verdict-deciding build. Six sabotages
|
||
applied and restored; `git status --porcelain` empty at the end.
|
||
|
||
## VERDICTS
|
||
|
||
| Lens | Verdict |
|
||
|---|---|
|
||
| **Retail conformance** | **PASS** |
|
||
| **Architecture** | **PASS** |
|
||
|
||
Three things the brief asked me to settle explicitly:
|
||
|
||
- **The containment claim — BELIEVED, and true more strongly than asserted.**
|
||
The shipped assertion is algebraically tautological (R1), so I re-derived
|
||
containment independently against physics-polygon **vertices** rather than
|
||
against the root sphere: **0 of 530** BSP-bearing Setups fail after the fix,
|
||
**412** failed before it.
|
||
- **The cap change — the implementer is RIGHT and both prior reviews were
|
||
WRONG.** Byte-verified: the `cmp eax,0xa` clamp is inside the cylsphere
|
||
overload only; the BSP walk is uncapped; `@0x0052b990` pushes a literal `1`.
|
||
- **The shrink argument — CORRECT, and both prior reviews used the wrong
|
||
criterion.** `calc_cross_cells`' BSP branch is reached through an
|
||
unconditional jump past both primitive branches, so retail's flood for these
|
||
objects contains no primitive contribution at all. "Smaller than before" is
|
||
not a defect criterion; containment is. My own sweep reproduces the
|
||
implementer's 143.
|
||
|
||
---
|
||
|
||
## Part 1 — Independent retail verification
|
||
|
||
Method: my own Capstone disassembly of the PDB-paired binary
|
||
(`C:\Users\erikn\Downloads\acclient.exe`, `check_exe_pdb.py` → **MATCH**,
|
||
CodeView GUID `9e847e2f-777c-4bd9-886c-22256bb87f32`, linker
|
||
2013-09-06T00:17:56Z, image base `0x400000`), every address cross-resolved
|
||
through `named-retail/symbols.json` and every struct offset cross-checked
|
||
against `named-retail/acclient.h`. Not inherited from the commit, not from the
|
||
prior reviews.
|
||
|
||
### 1.1 Every cited address resolves to the construct claimed — all 12
|
||
|
||
`symbols.json` lookup by address:
|
||
|
||
| Address | Symbol |
|
||
|---|---|
|
||
| `0x0050f050` | `CPhysicsObj::FindObjCollisions` |
|
||
| `0x00510fc0` | `CPhysicsObj::find_bbox_cell_list` |
|
||
| `0x00515230` | `CPhysicsObj::calc_cross_cells` |
|
||
| `0x00518070` / `0x00518090` | `CPartArray::GetSphere` / `GetCylsphere` |
|
||
| `0x00518110` | `CPartArray::CacheHasPhysicsBSP` |
|
||
| `0x00518160` | `CPartArray::calc_cross_cells_static` |
|
||
| `0x00518b00` | `CPartArray::GetSortingSphere` |
|
||
| `0x0052b990` / `0x0052b9f0` | `CObjCell::find_cell_list` (two overloads) |
|
||
| `0x0052cae0` | `CEnvCell::find_transit_cells` |
|
||
| `0x005397e0` | `BSPTREE::GetSphere` |
|
||
| `0x004527f0` | `Position::localtolocal` |
|
||
|
||
No mis-cited neighbour anywhere in this commit, the register rows, or the
|
||
tests. (I looked specifically, given the mis-cite found earlier this session.)
|
||
|
||
### 1.2 `BSPTREE::GetSphere` @`0x005397e0` — claim 4b CONFIRMED byte-exact
|
||
|
||
```
|
||
0x005397e0 8b01 mov eax, dword ptr [ecx] ; BSPTREE::root_node
|
||
0x005397e2 83c004 add eax, 4 ; past BSPNODE::vfptr
|
||
0x005397e5 c3 ret
|
||
```
|
||
|
||
`acclient.h`: `struct BSPNODE { BSPNODEVtbl *vfptr; CSphere sphere; Plane
|
||
splitting_plane; ... }` and `struct CSphere { AC1Legacy::Vector3 center; float
|
||
radius; }` → radius at `+0xc` of the returned pointer. Retail's per-part flood
|
||
sphere **is** the BSP root bounding sphere, origin included.
|
||
|
||
`acclient_2013_pseudo_c.txt:319128` (`0x00534b5b`):
|
||
`this->physics_sphere = BSPTREE::GetSphere(this->physics_bsp);` — verbatim.
|
||
|
||
### 1.3 `physics_sphere` = `[gfxobj+0x74]`, `physics_bsp` = `[+0x78]` — claim 4c CONFIRMED
|
||
|
||
`acclient.h` `CGfxObj` field order: `... use_built_mesh; CSphere
|
||
*physics_sphere; BSPTREE *physics_bsp; Vector3 sort_center; unsigned
|
||
num_polygons; CPolygon *polygons; CSphere *drawing_sphere; ...` — which places
|
||
`physics_sphere` at `+0x74`, `physics_bsp` at `+0x78`, `drawing_sphere` at
|
||
`+0x90`. Confirmed against code:
|
||
|
||
```
|
||
CPartArray::CacheHasPhysicsBSP @0x00518110
|
||
0x00518120 8b32 mov esi,[edx] ; parts[i]
|
||
0x00518122 8b7620 mov esi,[esi+0x20] ; CPhysicsPart::gfxobj
|
||
0x00518125 8b36 mov esi,[esi]
|
||
0x00518127 8b7e78 mov edi,[esi+0x78] ; physics_bsp <-- exact addr
|
||
```
|
||
|
||
and `find_transit_cells` reads `[ecx+0x74]` with a `[ecx+0x90]` fallback — the
|
||
drawing sphere — which only makes sense with this layout.
|
||
|
||
### 1.4 `CEnvCell::find_transit_cells` @`0x0052cae0` — claim 4a CONFIRMED, centre-then-radius
|
||
|
||
```
|
||
0x0052cb31 8b5020 mov edx,[eax+0x20] ; CPhysicsPart::gfxobj
|
||
0x0052cb34 8b0a mov ecx,[edx]
|
||
0x0052cb36 8b7174 mov esi,[ecx+0x74] ; physics_sphere
|
||
0x0052cb39 85f6 test esi,esi
|
||
0x0052cb3d 8bb190.. mov esi,[ecx+0x90] ; else drawing_sphere
|
||
0x0052cb4b 56 push esi ; -> &sphere->center
|
||
0x0052cb4c 83c030 add eax,0x30 ; CPhysicsPart::pos
|
||
0x0052cb4f 50 push eax
|
||
0x0052cb50 8d442434 lea eax,[esp+0x34] ; out
|
||
0x0052cb54 8d5d54 lea ebx,[ebp+0x54] ; cell->pos
|
||
0x0052cb5a e8915cf2ff call 0x4527f0 ; Position::localtolocal
|
||
0x0052cb5f d905708c7c00 fld dword [0x7c8c70] ; F_EPSILON = 1.9999999e-4
|
||
0x0052cb65 d8460c fadd dword [esi+0xc] ; ONLY NOW the radius
|
||
```
|
||
|
||
`acclient.h` `CPhysicsPart`: `CYpt(0) viewer_heading(4) degrades(0x10)
|
||
deg_level(0x14) deg_mode(0x18) draw_state(0x1c) gfxobj(0x20)
|
||
gfxobj_scale(0x24) pos(0x30)` — both `+0x20` and `+0x30` are exactly what the
|
||
commit says they are.
|
||
|
||
The call is `cellPos.localtolocal(&out, &part->pos, &sphere->center)`: the
|
||
sphere's **centre** is transformed through the part's own `Position` into the
|
||
cell frame, and the radius is only added afterwards to build the plane test.
|
||
**Carrying the radius alone is a different sphere — the commit's thesis is
|
||
right.**
|
||
|
||
### 1.5 The 10-sphere cap — settled against the implementer's claim
|
||
|
||
**Claim: the cap lives only in the cylsphere overload. VERIFIED.**
|
||
|
||
```
|
||
CObjCell::find_cell_list @0x0052b9f0 (the N-sphere overload)
|
||
0x0052ba1d 8b442414 mov eax,[esp+0x14] ; count
|
||
0x0052ba21 83f80a cmp eax,0xa
|
||
0x0052ba26 760b jbe 0x52ba33
|
||
0x0052ba28 bd0a0000 mov ebp,0xa ; clamp
|
||
... copy loop into the STATIC array 0x844838..0x8448d8 (10 x 0x10 stride)
|
||
```
|
||
|
||
The clamp is a fixed static-buffer capacity, not a policy: the destination
|
||
array is exactly ten 16-byte entries and the guard byte at `0x8448d8` sits
|
||
immediately past it.
|
||
|
||
```
|
||
CObjCell::find_cell_list @0x0052b990 (the sorting-sphere overload)
|
||
0x0052b9d6 6a01 push 1 ; literal count of ONE
|
||
```
|
||
|
||
```
|
||
CPhysicsObj::calc_cross_cells @0x00515230
|
||
0x00515285 test dword [esi+0xa8],0x10000
|
||
0x0051528f jne 0x515305 ------------------> 0x0051530c call 0x510fc0 (BSP)
|
||
0x005152d1 call 0x52b9f0 (cylsphere, capped)
|
||
0x005152fb call 0x52b990 (sorting sphere, one)
|
||
```
|
||
|
||
```
|
||
CPhysicsObj::find_bbox_cell_list @0x00510fc0
|
||
loop over cells [esi+8], no clamp
|
||
0x00511012 call 0x518160
|
||
|
||
CPartArray::calc_cross_cells_static @0x00518160
|
||
0x0051816e mov esi,[eax+0x5c] ; parts
|
||
0x00518171 mov eax,[eax+0x58] ; num_parts
|
||
0x00518176 call dword [edx+0x7c] ; args (num_parts, parts, cellarray)
|
||
|
||
CEnvCell::find_transit_cells @0x0052cae0
|
||
inner part loop 0x52cb22 .. 0x52cc76, bound = [esp+0x6c] = num_parts
|
||
outer portal loop, bound = [ebp+0x108]
|
||
NO CLAMP ANYWHERE
|
||
```
|
||
|
||
**The implementer is correct on all three counts. Both prior reviews passed a
|
||
cap that retail does not have on that branch.** 7 installed Setups exceed 10
|
||
physics-BSP parts (max 49, Setup `0x02001A91` — reproduced independently), and
|
||
their tail parts were previously dropped from the flood outright. The change is
|
||
a correctness improvement in the same direction as the rest of the commit.
|
||
|
||
### 1.6 `CPartArray::GetSortingSphere` @`0x00518b00` (AP-157's anchor)
|
||
|
||
```
|
||
0x00518b67 8b4654 mov eax,[esi+0x54]
|
||
0x00518b6f 83c070 add eax,0x70
|
||
```
|
||
|
||
Exactly `[partArray+0x54] + 0x70`. `acclient.h` `CSetup` places
|
||
`CSphere sorting_sphere` immediately after `step_up_height`. AP-157's citation
|
||
holds.
|
||
|
||
### 1.7 Two retail facts the commit did not use, both relevant
|
||
|
||
**`CObjCell::find_obj_collisions` @`0x0052b750` has no distance pre-filter.**
|
||
Pseudo-C `308916-308940`: it walks `shadow_object_list` and calls
|
||
`CPhysicsObj::FindObjCollisions` on every non-parented, non-self entry
|
||
unconditionally. So the `maxReach` early-out at
|
||
`src/AcDream.Core/Physics/TransitionTypes.cs:3763` — `sphereRadius +
|
||
obj.Radius + movement.Length() + 2f` — is entirely acdream's own invention with
|
||
no retail counterpart. **#333's characterisation is correct.** See A2.
|
||
|
||
**Retail's own slack constant is 0.0002 m, not 2 m.** `0x0052cb5f fld dword
|
||
[0x7c8c70]` reads `1.9999999494757503e-4` (`F_EPSILON`). Anchor for whoever
|
||
fixes #333.
|
||
|
||
**Retail's cross-cell walk ignores part scale; its collision does not.**
|
||
`find_transit_cells` uses only `CPhysicsPart::pos` (`+0x30`) and never
|
||
`gfxobj_scale` (`+0x24`), while `CPhysicsPart::find_obj_collisions`
|
||
@`0x0050d8d0` explicitly passes `gfxobj_scale.z` into
|
||
`SPHEREPATH::cache_localspace_sphere`. See R4.
|
||
|
||
---
|
||
|
||
## Part 2 — Independent DAT re-derivation
|
||
|
||
Scratch console program **outside the repo**, referencing only
|
||
`Chorizite.DatReaderWriter 2.1.7` — deliberately **not** `AcDream.Core`, so
|
||
nothing under test is in the loop. Every number recomputed by hand from the
|
||
installed `client_portal.dat`.
|
||
|
||
| Quantity | Committed constant | My measurement |
|
||
|---|---|---|
|
||
| Setups | 5935 | **5935** |
|
||
| with CylSphere | 678 | **678** |
|
||
| Sphere-only, no CylSphere | 3605 | **3605** |
|
||
| without any primitive | 1652 | **1652** |
|
||
| with a physics-BSP part | 530 | **530** |
|
||
| physics-BSP parts | 973 | **973** |
|
||
| off-centre parts (`\|o\| > r/2`) | 376 | **376** |
|
||
| affected (primitive + BSP) | 172 (73 cyl / 99 sph) | **172 (73 / 99)** |
|
||
| deepest BSP part array | 49, `0x02001A91` | **49, `0x02001A91`** |
|
||
| Setups with > 10 BSP parts | 7 | **7** |
|
||
| worst part-origin offset | 20.762 m on gfx `0x010036DD` (r 27.708) | **20.762 m, gfx `0x010036DD`, r 27.708** |
|
||
| affected Setups failing containment pre-fix | 170 | **170**, worst 17.345 m at scale 1.75 = **9.911 m at scale 1**, Setup `0x02000255` |
|
||
| affected Setups failing post-fix | 0 | **0** |
|
||
| 143-shrink claim | 143 of 172 | **143** |
|
||
| max Spheres on any Setup (AP-157) | 5 | **5** (`0x020016F7`) |
|
||
| non-zero `SortingSphere` (AP-157) | 4154 | **4154** |
|
||
|
||
Every external constant reproduces exactly, including the 9.911 m figure once
|
||
the 1.75 test scale is divided out.
|
||
|
||
### The stronger, genuinely independent containment result
|
||
|
||
Instead of comparing the flood sphere to the root sphere (which is circular —
|
||
see R1), I transformed **every physics-polygon vertex** of every physics-BSP
|
||
part into the same world frame and asked whether some emitted flood sphere
|
||
covers it:
|
||
|
||
```
|
||
VERTEX containment failures: after the fix = 0 before the fix = 412
|
||
```
|
||
|
||
Zero across all 530 BSP-bearing Setups. **I believe the containment claim.**
|
||
|
||
---
|
||
|
||
## Part 3 — Sabotage reproduction
|
||
|
||
Six of the nine claimed sabotages reproduced, including C, H and I as
|
||
requested. Each applied, clean-built, run, reverted.
|
||
|
||
| # | Sabotage | Claimed | Observed |
|
||
|---|---|---|---|
|
||
| A | `BuildFloodSpheres`: `world = partWorldPos` | 3 Core | **4 Core** (`FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint`, `_CapsCylSpheresAtTenButNeverTheBspParts`, `_RotatesTheBoundsCentre…`, `_CentresOnTheBoundsCentre…`) |
|
||
| C | `FromSetup` `boundsCenter = Vector3.Zero` (the shipped defect) | 1 Core + 2 App + 1 Content | **1 Core + 2 App + 1 Content** ✓ |
|
||
| D | drop `* entScale` on `boundsCenter` | 2 App + 1 Content | **2 App + 1 Content** ✓ |
|
||
| E | landblock flat branch `localCenter = Zero` | 1 Core | **1 Core** ✓ |
|
||
| H | `int cap = RetailSphereCap` (all branches) | 1 Core | **1 Core** ✓ |
|
||
| I | cap removed from the cylsphere branch | 1 Core | **1 Core** ✓ |
|
||
|
||
The claimed counts are honest; A is understated by one, i.e. coverage is
|
||
slightly better than advertised (A5).
|
||
|
||
**Trap encountered and avoided:** the naive form of sabotage I
|
||
(`int cap = int.MaxValue`) makes `RetailSphereCap` unused, which is
|
||
`error CS0219` under `TreatWarningsAsErrors`. The build fails, the stale
|
||
sabotage-H DLL survives, and the run reports H's failure as I's. I caught it by
|
||
grepping the build output for `Build succeeded` before every test run. Anyone
|
||
re-running these must do the same.
|
||
|
||
The fixtures are genuinely off-centre now. `ShadowObjectRegistryMultiPartTests.Bsp()`
|
||
defaults `BoundsCenter` to `(0, 6, 0)` and requires an explicit `(0.001, 0, 0)`
|
||
to get a near-concentric sphere;
|
||
`LiveEntityCollisionBuilderTests.Bsp()` defaults `centerZ = 1.25`;
|
||
`ShadowRegistrationOverflowTests.BspGfx()` defaults `centerZ = 0.75`. The
|
||
concentric `Radius = 14f` at `LocalPosition = Zero` configuration the prior
|
||
review named is gone from every flood fixture.
|
||
|
||
---
|
||
|
||
## Part 4 — `FromLandblockBspParts` (neither prior review looked)
|
||
|
||
`src/AcDream.Core/Physics/ShadowShapeBuilder.cs:268-334`.
|
||
|
||
- **The same discard existed.** Pre-fix the method computed only
|
||
`localRadius = flat.Nodes[flat.RootIndex].BoundingSphere.Radius` (or
|
||
`phys.BoundingSphere?.Radius`) and emitted the shape at `pPos`. Identical
|
||
defect, on stair runs, fences and rock clusters.
|
||
- **Both storage branches are fixed.** Flat (`:310-315`) and graph fallback
|
||
(`:316-320`) each take `Origin` alongside `Radius`.
|
||
`ShadowRegistrationOverflowTests.FromLandblockBspParts_CarriesTheScaledRootSphereCentre`
|
||
asserts both explicitly; sabotage E reddens the flat branch (verified) and
|
||
the same test carries the graph assertion.
|
||
- **`partScale` is applied correctly.** `Matrix4x4.Decompose` yields
|
||
`M = S·R·T`, so a point `v` in the part's own frame maps to root space as
|
||
`pPos + rotate(v · pScale, pRot)`. The code emits
|
||
`LocalPosition = pPos` (unscaled — correct, the translation is already
|
||
absolute in root space), `Radius = localRadius * partScale`,
|
||
`BoundsCenter = localCenter * partScale`, `Scale = partScale`. That composes
|
||
exactly as `BuildFloodSpheres` and as `TransitionTypes`' BSP transform
|
||
(`local * obj.Scale`, rotate, `+ obj.Position`) consume it.
|
||
|
||
Also verified: the two other `new ShadowShape(...)` sites in `src/`
|
||
(`LandblockPhysicsPublisher.cs:1003,1030` and
|
||
`LandblockPhysicsContentBuilder.cs:658,683`) emit **Cylinder** shapes only, for
|
||
which `BoundsCenter == Zero` is correct by definition. There is no third BSP
|
||
producer.
|
||
|
||
---
|
||
|
||
## Part 5 — Is `BoundsCenter` the right seam?
|
||
|
||
**Yes. The implementer's rebuttal of the `LocalPosition`-offset suggestion is
|
||
correct, and I verified the reason rather than taking it.**
|
||
|
||
`ShadowEntry.Position` is load-bearing in three distinct ways:
|
||
|
||
1. **BSP world origin.** `TransitionTypes.cs:3862-3898` transforms the mover's
|
||
spheres by `Vector3.Transform(sphere.Origin - obj.Position, invRot) *
|
||
invScale` and passes `worldOrigin: obj.Position` into
|
||
`CollisionTraversal.FindCollisions`. The physics BSP is authored about the
|
||
**part origin**, so shifting `Position` to the bounding-sphere centre would
|
||
translate every collision polygon by the offset — up to 20.762 m.
|
||
2. **Broadphase reach.** `TransitionTypes.cs:3757`.
|
||
3. **Cylinder XY distance.** `TransitionTypes.cs:3760`.
|
||
|
||
Moving `LocalPosition` would break (1) outright. A separate field is the only
|
||
correct seam.
|
||
|
||
**Consistency audit — every site that composes a shape's world placement:**
|
||
|
||
| Site | Uses | Correct? |
|
||
|---|---|---|
|
||
| `ShadowObjectRegistry.cs:474-490` `RegisterMultiPart` → `ShadowEntry` | `LocalPosition` only | ✓ (BSP frame) |
|
||
| `:578-593` `ReplaceMultiPartPayload` → `ShadowEntry` | `LocalPosition` only | ✓ |
|
||
| `:1484-1500` suspended-owner restore → `ShadowEntry` | `LocalPosition` only | ✓ |
|
||
| `:691-693` `BuildFloodSpheres` | `LocalPosition` **+ rotate(BoundsCenter, partWorldRot)** | ✓ |
|
||
| `:773`, `:1648`, `:2231` reflood / transfer | delegate to `RegisterMultiPart` | ✓ |
|
||
|
||
`BuildFloodSpheres` is the single flood site (`UpdatePosition` routes through
|
||
`RegisterMultiPart`), and no site reads the old semantics. The one place the
|
||
new field is *missing* and should eventually be present is `ShadowEntry` itself
|
||
— that is #333 (A2).
|
||
|
||
`ReplaceMultiPartPayload` deliberately does not re-flood; that matches retail
|
||
(`CPartArray::SetPart` changes the part read by later tests and does not
|
||
recalculate cross-cells). Not a finding.
|
||
|
||
---
|
||
|
||
## Part 6 — Is the shrink argument right?
|
||
|
||
**Yes. Settle it in favour of the implementer.**
|
||
|
||
Prior review F2 counted 43 Setups whose post-`4abd1b5e` flood was not a
|
||
superset of the pre-`4abd1b5e` one and called it a new under-inclusive defect.
|
||
My sweep puts the post-`b52967de` number at **143 of 172** — larger, exactly as
|
||
the implementer says.
|
||
|
||
That is not a defect, because retail's flood for a BSP-bearing object contains
|
||
**no primitive contribution at all**:
|
||
|
||
```
|
||
0x00515285 test dword [esi+0xa8],0x10000
|
||
0x0051528f jne 0x515305 ; -> find_bbox_cell_list, and it never returns
|
||
; into 0x005152d1 / 0x005152fb
|
||
```
|
||
|
||
The cylsphere branch (`0x005152d1`) and the sorting-sphere branch
|
||
(`0x005152fb`) are both below that jump and unreachable from it — I read the
|
||
control flow, not a summary of it. `find_bbox_cell_list` seeds the object's own
|
||
cell (`0x00510fd5` → `0x00510fe2 call 0x6b4ff0`) and then walks the part array;
|
||
nothing else contributes.
|
||
|
||
So for `0x0200086E` — F2's worst case, whose Sphere reached `z ≈ 11.68` while
|
||
its BSP parts reach `z ≈ 9.0` — **retail also reaches only `z ≈ 9.0`.** The
|
||
pre-commit flood was larger because it included a primitive `calc_cross_cells`
|
||
never reads. Shrinking toward the BSP set is convergence, not regression.
|
||
|
||
The criterion that *does* discriminate is containment, and it now holds at
|
||
vertex level for all 530 (Part 2).
|
||
|
||
**What this means for the connected session** (the practical point):
|
||
"props that stopped blocking" is **not** by itself a bug report. The question
|
||
to ask of any such observation is whether the object's own BSP geometry still
|
||
reaches the cell it stopped blocking from. If it does, that is a bug; if it
|
||
does not, that is retail. Conversely, "a tall prop that still does not block"
|
||
is the expected symptom of #333, not of this fix (A2).
|
||
|
||
---
|
||
|
||
## Findings — retail lens
|
||
|
||
### R1 (MEDIUM) — the shipped containment assertion is algebraically tautological
|
||
|
||
`tests/AcDream.Content.Tests/InstalledSetupBspPrimitiveDispatchTests.cs:266-341`.
|
||
|
||
`truth` is built at `:282-284` as
|
||
`(partOrigin + rotate(b.Origin, partRot)) * EntScale` from the **same**
|
||
`Bounds()` resolver, over the **same** part set (the gate at `:299` is
|
||
`id => Bounds(id) is not null`), with the **same** placement-frame priority
|
||
(`:259-264` mirrors `ResolvePlacementFrame`). `flood` is built at `:314-317` as
|
||
`shape.LocalPosition + rotate(shape.BoundsCenter, shape.LocalRotation)`, and
|
||
`FromSetup` sets `LocalPosition = partFrame.Origin * entScale`,
|
||
`BoundsCenter = b.Origin * entScale`, `LocalRotation = partFrame.Orientation`.
|
||
The two expressions are identical by algebra, so `Shortfall(flood) ≡ 0` for
|
||
**any** DAT content and any future DAT.
|
||
|
||
Concrete failure scenario: if a later change made `FromSetup` read the wrong
|
||
GfxObj's bounding sphere in *both* the resolver and the emission — say a
|
||
`SetupId`/`GfxObjId` mix-up inside `Bounds()` — this assertion stays green
|
||
while every flood sphere in the game moves.
|
||
|
||
It is still a useful change-detector (sabotages C and D redden it, verified),
|
||
and `ExpectedWouldFailIfOriginDiscarded = 170` is a real, non-circular
|
||
measurement of the defect. But the commit body's framing — *"an installed-DAT
|
||
containment sweep asserting every emitted BSP flood sphere contains that part's
|
||
real bounding sphere"* — overstates what the code proves.
|
||
|
||
Cheap fix: compare against physics-polygon vertices (`gfx.PhysicsPolygons` →
|
||
`gfx.VertexArray.Vertices`) instead of against the root sphere. I ran exactly
|
||
that and it passes 0/530, so adopting it costs nothing and makes the assertion
|
||
mean what it says.
|
||
|
||
### R2 (MEDIUM) — the defect population is understated 2.4×
|
||
|
||
The commit, AP-156 and both prior reviews all scope the defect to "170 of the
|
||
172" AP-152 Setups. After `4abd1b5e` **every** BSP-bearing Setup floods from
|
||
its BSP shapes only, so the discarded origin mis-placed the flood for all of
|
||
them, not just the ones that also carry a primitive. Measured:
|
||
|
||
```
|
||
vertex-containment failures with the origin discarded = 412 of 530
|
||
```
|
||
|
||
The 172 subset is AP-152's population, not AP-156's. The biggest single mover I
|
||
found — `gfx 0x010036DD`, 20.762 m offset on a 27.708 m sphere, in Setup
|
||
`0x0200129A` — is cited in the commit for its offset but its Setup is not
|
||
necessarily in the 172.
|
||
|
||
Consequence for the live gate: the user should be told the change touches
|
||
**412 installed Setups**, and that the objects most likely to look different
|
||
are not confined to the 73 CylSphere + 99 Sphere lists the commit body names.
|
||
|
||
### R3 (LOW) — the uncapped third branch is *further* from retail than the 10-cap was
|
||
|
||
`src/AcDream.Core/Physics/ShadowObjectRegistry.cs:674`:
|
||
|
||
```csharp
|
||
int cap = only == ShadowCollisionType.Cylinder ? RetailSphereCap : int.MaxValue;
|
||
```
|
||
|
||
`only == null` is the sorting-sphere branch. The commit's own justification for
|
||
lifting the cap there is *"the sorting-sphere overload @0x0052b990 takes one
|
||
sphere"* — which argues for a cap of **1**, not `int.MaxValue`. The cited
|
||
evidence does not support the code written.
|
||
|
||
Inert over installed data (I measured max 5 Spheres on any Setup, `0x020016F7`;
|
||
max 7 CylSpheres), and AP-157 registers the substitution honestly. But the
|
||
comment block at `:664-673` reads as though it justifies the whole line when it
|
||
only justifies the BSP half.
|
||
|
||
### R4 (LOW) — acdream scales the flood sphere; retail does not — unregistered
|
||
|
||
`ShadowShapeBuilder.cs:217` (`boundsCenter * entScale`) and `:330`
|
||
(`localCenter * partScale`) are new in this commit. Retail's
|
||
`find_transit_cells` uses only `CPhysicsPart::pos` (`+0x30`) and never touches
|
||
`gfxobj_scale` (`+0x24`), while `CPhysicsPart::find_obj_collisions`
|
||
@`0x0050d8d0` explicitly threads `gfxobj_scale.z` into
|
||
`SPHEREPATH::cache_localspace_sphere`. Retail's cross-cell walk is therefore
|
||
under-inclusive for scaled parts and acdream's is not.
|
||
|
||
Radius scaling predates this commit; centre scaling does not. Direction is
|
||
over-inclusive for `scale > 1` (safe) and under-inclusive for `scale < 1`
|
||
(the #98/#168 direction). Deserves a sentence in AP-156's residual paragraph;
|
||
the row currently lists only the sphere-vs-portal traversal.
|
||
|
||
### R5 (INFO) — retail's slack constant, for whoever fixes #333
|
||
|
||
`0x0052cb5f fld dword [0x7c8c70]` = `1.9999999494757503e-4` — the same
|
||
`F_EPSILON` AP-30 already byte-confirmed for `Frame::is_equal`. Retail's
|
||
cross-cell sphere test is `radius + 0.0002`, not `radius + 2`.
|
||
|
||
---
|
||
|
||
## Findings — architecture lens
|
||
|
||
### A1 (MEDIUM) — `BoundsCenter = default` reopens, at the type, the exact hole the commit closed at the seam
|
||
|
||
`src/AcDream.Core/Physics/ShadowShape.cs:61`:
|
||
|
||
```csharp
|
||
Vector3 BoundsCenter = default);
|
||
```
|
||
|
||
The commit's central structural claim is that *"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."* That is true at
|
||
the `LiveEntityCollisionBuilder`/`FromSetup` seam — genuinely well done, and
|
||
the single-resolver collapse is the right call.
|
||
|
||
It is **not** true of `ShadowShape` itself. This still compiles today:
|
||
|
||
```csharp
|
||
new ShadowShape(gfxId, pos, rot, scale, ShadowCollisionType.BSP, radius, 0f)
|
||
```
|
||
|
||
and silently reproduces AP-156 with a zero centre. Every current BSP producer
|
||
passes it, so nothing is broken now — but a future BSP producer (a new static
|
||
publisher, a plugin-facing builder, a bake path) gets the old bug for free,
|
||
green.
|
||
|
||
Concrete failure scenario: someone adds a BSP branch to
|
||
`LandblockPhysicsContentBuilder` (which today emits Cylinders at `:658,683`
|
||
with no `BoundsCenter`), copies the existing 7-argument call shape, and
|
||
reintroduces a 20 m flood mis-placement with no test failing.
|
||
|
||
Fix: drop the default and pass `Vector3.Zero` explicitly at the two
|
||
primitive sites in `ShadowShapeBuilder` and the four in the publishers. Six
|
||
call sites, and the type then enforces the invariant the commit body claims.
|
||
|
||
### A2 (MEDIUM) — #333 is a real defect, correctly identified, but deferring it can mask this fix's entire visible benefit
|
||
|
||
`e2b2d04c` is honest and well-reasoned: the broadphase at
|
||
`TransitionTypes.cs:3757-3765` measures `currPos - obj.Position` (the **part
|
||
origin**) against `sphereRadius + obj.Radius + movement + 2f`, where
|
||
`obj.Radius` is the root sphere's radius measured about a centre that may be
|
||
metres away. Same discarded origin, one layer down.
|
||
|
||
I confirmed the two things the issue could not:
|
||
|
||
- **Retail has no such filter at all.** `CObjCell::find_obj_collisions`
|
||
@`0x0052b750` (pseudo-C 308916-308940) dispatches every non-parented,
|
||
non-self shadow object to `CPhysicsObj::FindObjCollisions` unconditionally.
|
||
The `+ 2f` and the movement term are acdream's own. The issue's "looks like
|
||
acdream's own broadphase rather than a port" is **correct**, and this is an
|
||
unregistered divergence that predates the commit — it should carry an AP row,
|
||
not only an issue number.
|
||
- **Its blast radius is large.** A genuine surface contact is rejected when
|
||
`|offset| > sphereRadius_mover + movement + 2` — roughly 2.5 m for a walking
|
||
player. **118 of the 477 unique installed physics-BSP GfxObjs** have a root
|
||
sphere offset above 2.5 m, and **46** above 5 m. At the test scale 1.75 those
|
||
become 4.4 m and 8.75 m against an unchanged ~2.5 m budget.
|
||
|
||
The issue's own one-line summary is slightly imprecise — it says contact is
|
||
admitted "only when the sphere's centre is within about `movement + 2` metres
|
||
of the part origin", where the correct statement is about the **mover's**
|
||
distance from the part origin versus `R + r_m + move + 2`. The conclusion is
|
||
unchanged.
|
||
|
||
Deferral judgement: the split is defensible on attributability grounds (it is
|
||
literally the AP-155 lesson), and I would not have bundled a *retail-question*
|
||
fix. But the practical consequence must be stated plainly to the user rather
|
||
than left in an issue body: **for the tall props with the largest offsets —
|
||
exactly the objects the commit body tells the user to go look at — the fix may
|
||
produce no visible change at all**, because the geometry now lands in the right
|
||
cell and is then discarded by the filter. If the connected gate is run before
|
||
#333, a null result on tall props is expected, not evidence against AP-156.
|
||
|
||
The minimal correct fix is mechanical (carry `BoundsCenter` on `ShadowEntry`
|
||
and measure from `obj.Position + rotate(BoundsCenter, obj.Rotation)`); only the
|
||
`+ 2f` question is genuinely open, and it is open independently of that.
|
||
|
||
### A3 (LOW) — a cached field became a per-call delegate allocation
|
||
|
||
`src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:136`:
|
||
|
||
```csharp
|
||
id => _physicsBspBounds(id) is not null,
|
||
```
|
||
|
||
replaces the cached `_hasPhysicsBsp` field. The lambda captures `this`, so
|
||
Roslyn allocates a fresh `Func<uint,bool>` on every `Build` call. `Build` is
|
||
per-entity-spawn / per-appearance-rebuild, not per-frame, so this is small —
|
||
but the project has spent a whole slice (I1) driving physics-path resolves to
|
||
0 B, and a `private readonly Func<uint,bool> _hasBounds` initialised once in
|
||
both constructors costs nothing and keeps the single-resolver invariant intact.
|
||
|
||
Also: the new resolver indexes `flat.Nodes[flat.RootIndex]` where the old gate
|
||
only compared `RootIndex >= 0`. A `FlatPhysicsBsp` with `RootIndex >= 0` and an
|
||
empty `Nodes` array would now throw where it previously returned `true`. I
|
||
found no way to construct one from `FlatCollisionAssetBuilder`, so this is a
|
||
note, not a defect.
|
||
|
||
### A4 (LOW) — uncapping the BSP branch raises a per-tick cost for moving remotes
|
||
|
||
`RuntimeRemotePhysicsUpdater.cs:482` re-floods a remote through
|
||
`RegisterMultiPart` on every tick it actually moves, and `RegisterMultiPart`
|
||
writes every shape row into every flooded cell. Lifting the cap changes the
|
||
flood input from ≤10 spheres to up to 49, and the row count is
|
||
`parts × cells` — both factors grow together.
|
||
|
||
Bounded in practice: only 7 installed Setups exceed 10 BSP parts, landblock
|
||
part arrays are static and register once, and this is what retail does
|
||
(`add_shadows_to_cells` @`0x00514ae0` → `CPartArray::AddPartsShadow` walks the
|
||
whole part array into every cross-cell). Worth a glance in the connected
|
||
session if a dense indoor scene with a many-part animate prop regresses; not
|
||
worth pre-emptive work.
|
||
|
||
### A5 (LOW) — sabotage A reddens 4 Core tests, not 3
|
||
|
||
Reproduced: `FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint`,
|
||
`BuildFloodSpheres_CapsCylSpheresAtTenButNeverTheBspParts`,
|
||
`BuildFloodSpheres_BspShape_RotatesTheBoundsCentreByThePartRotation`,
|
||
`BuildFloodSpheres_BspShape_CentresOnTheBoundsCentreNotThePartOrigin`.
|
||
Coverage is one better than the commit body claims; the ledger is simply stale
|
||
by one line.
|
||
|
||
### A6 (LOW) — the Content test's "cap control" does not test the cap
|
||
|
||
`InstalledSetupBspPrimitiveDispatchTests.cs:302-308` comments that *"EVERY BSP
|
||
shape contributes … capping here would silently exclude their tail from the
|
||
containment claim below"*, and `:365` asserts
|
||
`mostBspShapesOnOneSetup == 49`. But that loop (`:311-319`) is the test's own
|
||
re-implementation of the composition with **no cap**, so it cannot observe a
|
||
cap regression in `BuildFloodSpheres`. I confirmed empirically: under sabotage
|
||
H the Content suite stayed 127/127 green.
|
||
|
||
The cap is genuinely covered — by
|
||
`BuildFloodSpheres_CapsCylSpheresAtTenButNeverTheBspParts` (sabotages H and I
|
||
both redden it, verified). The comment just claims coverage that lives
|
||
elsewhere.
|
||
|
||
---
|
||
|
||
## Gates reproduced
|
||
|
||
All after deleting all 44 `bin`/`obj`.
|
||
|
||
| Gate | Claimed | Observed |
|
||
|---|---|---|
|
||
| Release build | 0 errors, 21 pre-existing warnings | **0 errors, 21 warnings** ✓ |
|
||
| Complete suite | 11,208 / 4 skipped / 0 failed | **11,208 / 4 / 0** ✓ |
|
||
| Core | 4268 | **4268** (1 skip) ✓ |
|
||
| Content | 127 | **127** (0 skips) ✓ |
|
||
| App | unchanged, one rename | **4173** (3 skips) ✓ |
|
||
| Delta vs `4abd1b5e` | +5 | consistent: +4 Core (3 in `ShadowObjectRegistryMultiPartTests`, 1 in `ShadowRegistrationOverflowTests`) +1 Content, App rename only. Baseline **not** independently re-run — I did not check out `4abd1b5e` in a worktree another agent may share. |
|
||
| Active AP register rows | 109 | **109**, literal count of non-struck `AP-` rows in section 3 ✓ (AD 48, IA 18, UN 4) |
|
||
|
||
Suite run twice — once on the pristine tree before any sabotage, once on a
|
||
fully clean rebuild after every restore. Identical both times. No flake from
|
||
#302 / #308 / #321 was hit; nothing was conflated with them.
|
||
|
||
## Rules compliance
|
||
|
||
- **No workaround, suppression flag, grace period, retry loop or symptom
|
||
guard** anywhere in the diff. The fix is at the source: the resolver that
|
||
answers "does this dispatch as BSP?" is now the same one that answers "where
|
||
and how big is its sphere?".
|
||
- **No new skips.** 4 skipped, all pre-existing.
|
||
- **No test weakened.** `BspOnlyPart_UsesRealScaledPhysicsBoundingRadius` →
|
||
`…BoundingSphere` is a rename that **adds** an assertion; the
|
||
`LiveAppearanceAnimationTests` and `Builder()` edits drop a now-merged
|
||
parameter with no behavioural change. Every flood fixture moved from
|
||
concentric to off-centre, which is strictly stronger.
|
||
- **Register discipline honoured.** AP-155 narrowed with its false direction
|
||
corrected, AP-156 and AP-157 filed, AP-152 retired, count reconciles at a
|
||
literal 109 — all in the same commit as the code. This is the register rule
|
||
working as designed: AP-155(b)'s recorded direction was the *stated reason*
|
||
the residual was safe to defer, and it was backwards.
|
||
|
||
## What I checked and found clean, so the PASS is auditable
|
||
|
||
- All 12 cited retail addresses resolve to the named symbol; no mis-cite.
|
||
- `BSPTREE::GetSphere`, `find_transit_cells`, `find_bbox_cell_list`,
|
||
`calc_cross_cells_static`, `calc_cross_cells`, both `find_cell_list`
|
||
overloads, `CacheHasPhysicsBSP`, `GetSortingSphere` — all disassembled from
|
||
the PDB-paired binary, not inherited.
|
||
- `CGfxObj`, `CPhysicsPart`, `BSPNODE`, `CSphere`, `CSetup` field offsets
|
||
cross-checked against `acclient.h` and against the code that reads them.
|
||
- Every `new ShadowShape(...)` site in `src/` (8) classified; only two emit BSP
|
||
and both carry the centre.
|
||
- Every site that composes a shape's world placement (5) classified; none reads
|
||
the old semantics.
|
||
- Every `BuildShadowCellSet` caller (2) classified; the single-shape
|
||
`Register` overload has no production BSP caller.
|
||
- `FlatPhysicsBsp.Nodes[RootIndex].BoundingSphere` traced to
|
||
`gfxObj.PhysicsBSP.Root.BoundingSphere` in both `PhysicsDataCache`
|
||
population paths (`:216`, `:281-287`) and in
|
||
`FlatCollisionAssetBuilder` (`:84`, `:393`) — acdream's seam is retail's
|
||
`physics_sphere`.
|
||
- `ReplaceMultiPartPayload`'s no-reflood behaviour checked against
|
||
`CPartArray::SetPart` semantics — retail-faithful.
|
||
- Quaternion composition order (`entityWorldRot * s.LocalRotation`, then
|
||
`Vector3.Transform(BoundsCenter, partWorldRot)`) checked against the inverse
|
||
transform in `TransitionTypes.cs:3859-3876` — consistent.
|
||
- Six sabotages reproduced; tree confirmed clean by `git status --porcelain`
|
||
before and after.
|
||
|
||
---
|
||
|
||
## Recommended, in priority order
|
||
|
||
1. Tell the user the live-gate criterion is **containment, not size**, and that
|
||
"a prop stopped blocking" is only a bug if its own BSP geometry still
|
||
reaches the cell (Part 6). State the population as **412 Setups** (R2).
|
||
2. Warn that tall props with large offsets may show **no change** until #333
|
||
lands, and that a null result there is expected (A2).
|
||
3. Fix #333 — it is mechanical apart from the `+ 2f` question — and give the
|
||
`maxReach` filter its own AP row, since retail has no such filter at all
|
||
(A2).
|
||
4. Make `ShadowShape.BoundsCenter` required (A1).
|
||
5. Swap the Content containment oracle to physics-polygon vertices (R1) — it
|
||
passes today, costs nothing, and makes the load-bearing assertion mean
|
||
what it says.
|
||
6. Add the scale sentence to AP-156's residual paragraph (R4).
|