acdream/docs/research/2026-08-06-ap156-review-closure.md

AP-156 fix review — closure record

Closes the six findings in 2026-08-06-ap156-fix-review.md (both lenses PASS; this is cleanup before merge, not a rescue). Worktree .claude/worktrees/resume-session-e0bd03e1-d5bf45, base e2b2d04c.

All 44 bin/obj deleted before every verdict-deciding build, and every test run gated on a grep for Build succeeded in the same invocation — the trap the review documented (a sabotage that trips CS0219 under TreatWarningsAsErrors fails the build and lets the previous sabotage's DLL report the wrong failure) was not hit.

---

R1 — the containment oracle now discriminates

Done. InstalledSetupBspPrimitiveDispatchTests no longer compares the emitted flood sphere against a hand-rebuilt copy of the same sphere. It transforms every physics-polygon vertex (GfxObj.PhysicsPolygons → GfxObj.VertexArray.Vertices) of every physics-BSP part into the entity frame and asserts some emitted flood sphere covers it. Vertices are a different DAT field from the bounding sphere, so the two sides of the comparison are no longer the same expression. Renamed to InstalledSetups_BspFloodSpheres_ContainTheirOwnPhysicsPolygons.

New population + defect controls: 973 physics-BSP parts, 530 BSP-bearing Setups, 376 off-centre parts, 91,689 physics vertices, 428 pre-fix failures, deepest part array 49. Every one measured first by a scratch DatReaderWriter console program outside the repo that references no acdream assembly, then reproduced exactly through the production builder.

Sabotage-verified, three ways, each after a full clean and a verified Build succeeded:

#	Sabotage	Result
S1	ShadowShape.Bsp drops the bounds centre (the AP-156 defect itself)	RED — 428 Setups, worst 35.869 m on 0x0200129A (matches the independent sweep exactly)
S2	ShadowShape.Bsp applies the scale to the radius but not the centre	RED — 326 Setups, worst 15.288 m
S3	the TEST's own Bounds() oracle corrupted (bs.Origin negated)	RED — 467 Setups, worst 72.134 m
S1b	same sabotage, run against Core rather than Content	RED — 5 tests: the 4 the review's sabotage A found, plus ShadowRegistrationOverflowTests.FromLandblockBspParts_CarriesTheScaledRootSphereCentre, which sabotage A could not reach because it only touched BuildFloodSpheres. The factory refactor makes both BSP producers share one scaling path, so one sabotage now covers both.
—	restored	GREEN — 127/127 Content, 4,268/1 skip Core

S3 is the one that matters for R1. Under the shipped oracle that same corruption was invisible by algebra: truth was (partOrigin + rotate(O, R))·s and flood was LocalPosition + rotate(BoundsCenter, R) = partOrigin·s + rotate(O·s, R), so negating O moved both sides identically and the shortfall stayed ≡ 0. The vertex oracle catches it. That is the tenth green-test-covering-nothing this campaign, and this is the direction of sabotage that proves it is closed — breaking the production code was never the hard half.

R2 — population corrected, and refined

Done, with a refinement the review will want. The review says 412, not "170 of 172". Both figures are right about different things, and neither is the number to quote to the user:

Question	Answer
Setups carrying both a primitive and a physics-BSP part (AP-152's DISPATCH population)	172 of 5,935
BSP-bearing Setups (AP-156's population)	530
…whose flood sphere actually MOVES because of the fix	525
…failing vertex-level containment before the fix, 1 mm tolerance	428
…failing at a 1 cm tolerance	412 ← the review's figure
…failing after the fix, at any tolerance down to zero	0

The 412/428 gap is 16 Setups whose pre-fix shortfall lands between 1.4 mm and 10 mm — real geometry, three orders of magnitude above float noise at these radii, not a measurement artifact. The test keeps its existing 1 mm tolerance and therefore pins 428; the register records both and names the tolerance, because a bare "412" is unreproducible without it.

The number to tell the user is 525 — Setups whose behaviour changes — not 172, and not 412. Corrected in the AP-156 row, the section-3 header, the C5c handoff, and the two stale test docstrings. The dated review documents (2026-08-06-ap152-review-retail.md, and the fix review itself) are left as written: "170 of 172" was correct for what those reviews measured (root-sphere containment over the 172), and rewriting a dated evidence artifact to match a later measurement is how provenance gets lost.

The commit's own record cannot be corrected in place — b52967de is HEAD~1 and amending it would rewrite published history. This document plus the register row is that correction.

A1 — the hole is closed at the type

Done, and deliberately stronger than the review's recommendation. The review suggested dropping the = default and passing Vector3.Zero explicitly at six call sites. That does not close the review's own failure scenario: someone adding a BSP branch to LandblockPhysicsContentBuilder by copying the adjacent Cylinder call would then write Vector3.Zero explicitly and get the same 20 m mis-placement, still green.

Instead ShadowShape's constructor is now private, and the three factories are the only way to build one:

ShadowShape.Bsp(gfxObjId, localPosition, localRotation, scale, FlatCollisionSphere localBounds)
ShadowShape.Cylinder(gfxObjId, localPosition, localRotation, scale, radius, cylHeight)
ShadowShape.Sphere(gfxObjId, localPosition, localRotation, scale, radius)

Bsp takes the radius and the centre as one FlatCollisionSphere value and scales them together inside the factory. There is no longer an expression a caller can write that carries one and drops the other — which is the commit body's own claim ("the radius cannot be taken while the origin is dropped"), now true of the type and not only of the FromSetup seam. 22 construction sites converted; S1 and S2 above are exactly the two ways the factory could still be got wrong, and both are red.

Residual, recorded rather than fixed: default(ShadowShape) remains constructible because C# structs always have a parameterless constructor. It yields CollisionType = BSP, Radius = 0 — inert, and no production path produces one.

A2 — #333 updated, AP-158 filed, and the retail claim independently verified

Done. I disassembled CObjCell::find_obj_collisions @0x0052b750 myself from the PDB-paired binary (check_exe_pdb.py → MATCH, CodeView GUID 9e847e2f-777c-4bd9-886c-22256bb87f32, linker 2013-09-06T00:17:56Z) rather than inheriting the claim, because Binary Ninja drops flag tests and that has bitten this campaign repeatedly:

0x0052b759  cmp dword [ebx+0x174], 2   ; sphere_path.insert_type
0x0052b765  je  0x52b7a0               ; INITIAL_PLACEMENT_INSERT -> return OK_TS
0x0052b773  mov ecx,[edi+0xc8]         ; shadow_object_list.data
0x0052b77f  mov edx,[ecx+0x40]         ; physobj->parent -> skip if set
0x0052b786  cmp ecx,[ebx]              ; self -> skip
0x0052b78b  call 0x50f050              ; FindObjCollisions — UNCONDITIONAL
0x0052b79e  jb  0x52b773               ; loop, bound [edi+0xc4]

The single early-out is a transition-state test, not a distance test — the whole function is 0x54 bytes and there is no float comparison in it. Confirms the review: retail has no distance pre-filter at all, so acdream's sphereRadius + obj.Radius + movement.Length() + 2f is an invention. [ebx+0x174] resolves to sphere_path.insert_type against acclient.h's CTransition layout and the pseudo-C at 308916-308940 agrees.

Blast radius reproduced independently: 118 of the 477 unique installed physics-BSP GfxObjs exceed the ~2.5 m budget, 46 exceed 5 m.

Filed as AP-158 — the filter had no register row at all — carrying the disassembly, the F_EPSILON = 1.9999999e-4 m contrast (R5), the measured blast radius, and the "this can mask AP-156's entire visible benefit" consequence. #333 updated: its research question is struck through and answered, and what remains is a judgement call (keep the filter with a correct measurement point, or delete it and walk the list as retail does — the retail-faithful option, which should be costed first).

The consequence that had to be recorded prominently

Tall props may show no visible change at all until #333 lands, and a null result at the connected gate is EXPECTED, not evidence against AP-156. Recorded in three places a reader will actually hit: the AP-156 register row's WHAT REMAINS OPEN, the AP-158 row's risk column, and item 7 of the C5c handoff's §3.1 gate list — alongside the other half of the same warning, that "a prop stopped blocking" is only a bug if the object's own BSP geometry still reaches the cell it stopped blocking from.

LOW items

R3 — int.MaxValue on the sorting-sphere branch. Comment corrected; code deliberately unchanged, and I think the review's implied fix is wrong. The review is right that the cited evidence (0x0052b9d6 push 1) argues for a cap of 1, not int.MaxValue, and the comment block claimed to justify the whole line when it justified only the BSP half — that is fixed, and the comment now says plainly which half is a port and which is not. But capping at 1 would take Spheres[0], and retail's one sphere is CSetup::sorting_sphere, a different DAT field. Capping would not move toward retail; it would keep the wrong field and additionally make the substitution under-inclusive, which is the #98/#168 direction. int.MaxValue keeps it over-inclusive until AP-157 ports the real field, which is the honest state. Inert over installed data either way — max 5 Spheres on any Setup (0x020016F7).

R4 — acdream scales the flood sphere, retail does not. Recorded as a second residual on the AP-156 row. find_transit_cells @0x0052cae0 reads only CPhysicsPart::pos (+0x30) and never gfxobj_scale (+0x24), while CPhysicsPart::find_obj_collisions @0x0050d8d0 does thread gfxobj_scale.z into SPHEREPATH::cache_localspace_sphere. Direction: over-inclusive for scale > 1, under-inclusive for scale < 1. Radius scaling predates the commit; centre scaling did not, which is why it needed the row.

R5 — retail's slack is 0.0002 m, not 2 m. Carried into AP-158's anchor column and into #333, where whoever fixes the filter will hit it.

A3 — per-call delegate allocation. Fixed: LiveEntityCollisionBuilder now holds private readonly Func<uint,bool> _hasPhysicsBsp, initialised once in the single real constructor and still derived from _physicsBspBounds, so the single-resolver invariant is intact and Build allocates no closure.

A6 — the Content test's "cap control" does not test the cap. Comment corrected: the loop is the test's own uncapped re-implementation and cannot observe a cap regression; the cap is covered by BuildFloodSpheres_CapsCylSpheresAtTenButNeverTheBspParts, which reddens under both cap sabotages. What mostBspShapesOnOneSetup == 49 genuinely proves — that the containment claim reaches Setups past the retired clamp rather than stopping short of them — is now what the comment claims.

A5 — sabotage A reddens 4 Core tests, not 3. Noted; b52967de's message cannot be amended. Coverage is one better than advertised, which is the benign direction.

---

Gates

Gate	Baseline e2b2d04c	Observed
Release build, all 44 bin/obj deleted	0 errors, 21 warnings	0 errors, 21 warnings
Complete suite, -m:1, ACDREAM_PAK_PATH set	11,208 / 4 skipped / 0 failed	11,208 / 4 / 0 — reconciles exactly; one test RENAMED, none added or removed
Content	127 / 0 skips	127 / 0 skips
Active AP register rows	109	110 (AP-158 filed)

No new skips. Nothing was conflated with the known load-sensitive flakes #302 / #308 / #321.