fix(physics): close the AP-156 fix review — real containment oracle, type-level invariant, AP-158
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>
This commit is contained in:
parent
e2b2d04cb5
commit
e6457cc849
21 changed files with 1454 additions and 298 deletions
|
|
@ -31,8 +31,50 @@ What does NOT go here:
|
|||
downstream of the AP-156 membership fix, so that fix alone may not be enough to
|
||||
make the worst objects block.
|
||||
**Filed:** 2026-08-06 at the AP-156 fix (commit `b52967de`), which surfaced it.
|
||||
**Updated 2026-08-06** at the AP-156 fix review: the retail question below is
|
||||
now ANSWERED, and the filter has its own divergence row, **AP-158**.
|
||||
**Do NOT bundle with AP-156.** Different code path (collision query, not cell
|
||||
membership), and it needs its own retail question answered first.
|
||||
membership).
|
||||
|
||||
### ANSWERED — retail has no distance pre-filter at all
|
||||
|
||||
Disassembled from the PDB-paired binary (`C:\Users\erikn\Downloads\acclient.exe`,
|
||||
`check_exe_pdb.py` → MATCH, CodeView GUID `9e847e2f-777c-4bd9-886c-22256bb87f32`)
|
||||
for this update, not inherited from Binary Ninja:
|
||||
|
||||
```
|
||||
CObjCell::find_obj_collisions @0x0052b750
|
||||
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
|
||||
0x0052b784 jne 0x52b795 ; parented -> skip
|
||||
0x0052b786 cmp ecx,[ebx] ; physobj == mover?
|
||||
0x0052b788 je 0x52b795 ; self -> skip
|
||||
0x0052b78b call 0x50f050 ; CPhysicsObj::FindObjCollisions — UNCONDITIONAL
|
||||
0x0052b79e jb 0x52b773 ; loop, bound = [edi+0xc4]
|
||||
```
|
||||
|
||||
Agrees with `acclient_2013_pseudo_c.txt:308916-308940`. **There is no distance
|
||||
test in the function.** So the `+ 2f` slack and the `movement.Length()` term are
|
||||
acdream's own invention, which is why AP-158 exists. For scale: retail's own
|
||||
cross-cell slack constant is `F_EPSILON` = `1.9999999e-4` — 0.2 mm, read at
|
||||
`0x0052cb5f fld dword [0x7c8c70]` — not 2 m.
|
||||
|
||||
### Measured blast radius
|
||||
|
||||
Over the installed `client_portal.dat`, by an independent scratch sweep outside
|
||||
the repo: **118 of the 477** unique physics-BSP GfxObjs have a root-sphere
|
||||
offset above the filter's roughly 2.5 m walking budget, and **46** above 5 m. At
|
||||
a test scale of 1.75 those offsets become 4.4 m and 8.75 m against an unchanged
|
||||
budget.
|
||||
|
||||
### Consequence for the AP-156 connected gate — read this before running it
|
||||
|
||||
**Tall props may show NO VISIBLE CHANGE at all until this issue is fixed, and a
|
||||
null result there is EXPECTED rather than evidence against AP-156.** AP-156 puts
|
||||
the geometry into the correct cell; this filter then discards it one layer down,
|
||||
for exactly the largest-offset objects AP-156's commit body points the user at.
|
||||
|
||||
### The mechanism
|
||||
|
||||
|
|
@ -78,11 +120,12 @@ finds a tall prop that still does not block after AP-156, look here first.
|
|||
|
||||
### What to establish before fixing
|
||||
|
||||
1. Does retail have this pre-filter at all? `CPhysicsObj::FindObjCollisions`
|
||||
@0x0050f050 walks the cell's object list and dispatches per object; the
|
||||
`+ 2f` slack and the `movement.Length()` term look like acdream's own
|
||||
broadphase rather than a port. If it is acdream's, it also needs a
|
||||
divergence-register row, which it does not currently have.
|
||||
1. ~~Does retail have this pre-filter at all?~~ **ANSWERED above: no.** The
|
||||
register row is filed as **AP-158**. What remains open is a judgement call,
|
||||
not a research question: keep the filter as a deliberate optimisation with a
|
||||
correct measurement point, or delete it and walk the list as retail does.
|
||||
Deleting it is the retail-faithful option and should be costed first —
|
||||
`ShadowEntrySnapshot.Capture` already bounds the per-cell list.
|
||||
2. If it is kept, it must measure from where the geometry is: `ShadowEntry`
|
||||
needs the `BoundsCenter` that `ShadowShape` now carries, and `deltaToCurr`
|
||||
must be taken against `obj.Position + rotate(obj.BoundsCenter, obj.Rotation)`.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue