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>
218 lines
12 KiB
Markdown
218 lines
12 KiB
Markdown
# AP-156 fix review — closure record
|
|
|
|
Closes the six findings in
|
|
[`2026-08-06-ap156-fix-review.md`](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:
|
|
|
|
```csharp
|
|
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.
|