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 at4abd1b5e— 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>
This commit is contained in:
Erik
parent
4abd1b5eb7
commit
b52967def3
13 changed files with 1424 additions and 90 deletions
370
docs/research/2026-08-06-ap152-review-architecture.md
Normal file
370
docs/research/2026-08-06-ap152-review-architecture.md
Normal file
|
|
@ -0,0 +1,370 @@
|
|||
# AP-152 architecture review — `4abd1b5e`
|
||||
|
||||
**Reviewer scope:** blast radius, correctness of reach, test quality. Retail
|
||||
fidelity is a separate reviewer's.
|
||||
**Worktree:** `.claude/worktrees/resume-session-e0bd03e1-d5bf45`, branch
|
||||
`claude/resume-session-e0bd03e1-d5bf45`, HEAD `4abd1b5e`.
|
||||
**Method:** read-only, inline, no subagents. All `bin`/`obj` deleted (44
|
||||
directories) before every verdict-deciding build. Five sabotages reproduced;
|
||||
tree restored and verified clean (`git status --porcelain` empty) after each.
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
|
||||
**PASS**, with one high-severity latent risk that must not be treated as
|
||||
landed behaviour until the connected flood-set diff runs, and four
|
||||
documentation/framing residuals.
|
||||
|
||||
No defect found. The two load-bearing claims I was asked to attack — the
|
||||
headless-neutrality claim and the S4 no-op argument — **both hold**, and I
|
||||
verified each independently rather than accepting the implementer's evidence.
|
||||
|
||||
---
|
||||
|
||||
## Gates reproduced
|
||||
|
||||
| Gate | Claimed | Measured | |
|
||||
|---|---|---|---|
|
||||
| Clean Release build | 0 errors / 21 pre-existing warnings | 0 errors / 21 warnings | ✅ |
|
||||
| Full solution suite | 11,203 / 4 / 0 | 11,203 passed / 4 skipped / 0 failed | ✅ |
|
||||
| Headless | 89 / 89 | 89 / 89 | ✅ |
|
||||
| Delta vs `ec29a732` | +5 (3 Core, 1 App, 1 Content) | +5 `[Fact]` added, **0 removed**; the 5 new facts are exactly the 5 that redden under sabotage B | ✅ |
|
||||
| Renames 1:1 | 2 renamed, none deleted | `-0 [Fact]` in the test diff | ✅ |
|
||||
| Register bookkeeping | AP-152 retired, AP-153/154/155 filed | active AP rows 105 → 107 (−1 +3) | ✅ |
|
||||
|
||||
Per-project at HEAD, clean build, `--no-build`:
|
||||
Cli 4, Bake 15, Headless 89, Content 126, UI.Abstractions 546, Core.Net 764,
|
||||
Runtime 1222, App 4173/3 skip, Core 4264/1 skip. **Σ 11,203 / 4 / 0.**
|
||||
|
||||
I did not check out `ec29a732` to re-measure the 11,198 baseline (read-only
|
||||
worktree, and switching would have disturbed the other agent's assumptions).
|
||||
The +5 arithmetic is corroborated independently: sabotage B (gate disabled)
|
||||
reddens exactly five tests solution-wide, and they are exactly the five the
|
||||
commit says it added.
|
||||
|
||||
---
|
||||
|
||||
## 1. Reach across hosts — **the neutrality claim is TRUE, and stronger than stated**
|
||||
|
||||
I did not verify the claim as written; I enumerated the whole producer set.
|
||||
|
||||
`ShadowShape` is constructed at exactly **eight** sites in `src/`:
|
||||
|
||||
| Site | Emits |
|
||||
|---|---|
|
||||
| `ShadowShapeBuilder.cs:134` (step 1) | Cylinder |
|
||||
| `ShadowShapeBuilder.cs:155` (step 2) | Sphere |
|
||||
| `ShadowShapeBuilder.cs:192` (step 3) | BSP |
|
||||
| `ShadowShapeBuilder.cs:276` (`FromLandblockBspParts`) | BSP only — every path in that loop `continue`s or adds `ShadowCollisionType.BSP` |
|
||||
| `LandblockPhysicsContentBuilder.cs:658`, `:683` | Cylinder only (`:683` converts a Setup Sphere to a height-capped Cylinder) |
|
||||
| `LandblockPhysicsPublisher.cs:1003`, `:1030` | Cylinder only (same conversion) |
|
||||
|
||||
Steps 1–3 are the only site that could ever produce a heterogeneous list, and
|
||||
after this change it cannot. So:
|
||||
|
||||
- **Content's two registrations are homogeneous by construction.**
|
||||
`LandblockPhysicsContentBuilder.cs:619` passes `bspShapes` from
|
||||
`FromLandblockBspParts` (all-BSP); `:702` passes `setupShapes`, all
|
||||
`ShadowCollisionType.Cylinder`, with no `ShadowCollisionType.Sphere`
|
||||
reachable. Confirmed by reading both loops, not by trusting the comment.
|
||||
- **Every other `RegisterMultiPart` caller replays a stored list.**
|
||||
`ShadowObjectRegistry.cs:536` (`ReplaceMultiPartPayload`), `:741`
|
||||
(`UpdatePosition`), `:1616` (`RefloodOwnerForLandblock`), `:2199` (mirror)
|
||||
all pass `_entityShapes[entityId]`, which was populated by one of the eight
|
||||
producers above. Homogeneity propagates.
|
||||
- **Runtime constructs no shapes.** `grep "new ShadowShape("` in
|
||||
`src/AcDream.Runtime` returns nothing; every `ShadowObjects.*` call there is
|
||||
`UpdatePosition` / `CommitSetPosition` / `Suspend` / a read.
|
||||
- **Project references confirm the reach boundary.**
|
||||
`AcDream.Headless.csproj` → `AcDream.Runtime` only;
|
||||
`AcDream.Runtime.csproj` → Core, Core.Net, Content, Plugin.Abstractions.
|
||||
`AcDream.App` is unreachable, so `LiveEntityCollisionBuilder` — the sole
|
||||
production caller of `FromSetup` (`LiveEntityCollisionBuilder.cs:126`) —
|
||||
cannot execute in headless.
|
||||
|
||||
**Empirical confirmation, not just structural:** under sabotage B (the step-0
|
||||
gate disabled — emission reverts to additive), `AcDream.Headless.Tests` stays
|
||||
**89/89 green** while Core, App and Content each redden. That is direct
|
||||
evidence the change does not reach headless, which is the C5b lesson applied
|
||||
in the correct direction.
|
||||
|
||||
**I believe the headless-neutrality claim.**
|
||||
|
||||
---
|
||||
|
||||
## 2. Effective-GfxObj identity — **correct, and structurally airtight**
|
||||
|
||||
The gate (`ShadowShapeBuilder.cs:116-124`) and step 3 (`:171-200`) call the
|
||||
same helper `EffectivePartGfxObjId` (`:289-302`), over the same index range
|
||||
(`setup.Parts.Count`), with the same predicate instance (`hasPhysicsBsp`).
|
||||
Therefore
|
||||
|
||||
> `anyPhysicsBspPart == true` ⟺ step 3 emits at least one shape
|
||||
|
||||
is an identity, not a tested property. The trap the contract names — gate
|
||||
suppresses primitives, step 3 emits nothing, `Build` returns null at
|
||||
`LiveEntityCollisionBuilder.cs:146`, collision silently deleted — is
|
||||
unreachable by construction.
|
||||
|
||||
Downstream identity is the same one: `LiveEntityCollisionBuilder.cs:137`
|
||||
resolves the real BSP radius from `shape.GfxObjId`, which step 3 set to the
|
||||
effective id at `ShadowShapeBuilder.cs:193`. The predicate itself
|
||||
(`LiveEntityCollisionBuilder.cs:56`,
|
||||
`physicsData.GetFlatGfxObj(id)?.PhysicsBsp.RootIndex >= 0`) is the same
|
||||
function object passed to the gate. Gate, emission, radius resolution and the
|
||||
collision-time BSP lookup all key on one id.
|
||||
|
||||
**Trap-1 sabotage reproduced.** Replacing the gate body with
|
||||
`hasPhysicsBsp((uint)setup.Parts[i])`:
|
||||
|
||||
```
|
||||
Failed ShadowShapeBuilderTests.FromSetup_DispatchGateReadsTheEffectivePartIdentities
|
||||
Failed! - Failed: 1, Passed: 4263, Skipped: 1, Total: 4265
|
||||
```
|
||||
|
||||
Exactly one test catches it, and it is the one claimed to. Its discriminating
|
||||
power is real: with a `setup.Parts` gate the swapped case yields
|
||||
Cylinder + BSP (2 shapes) and `Assert.Single(swapped)` fails, while the
|
||||
unswapped case still passes — so the test distinguishes *this* wrong gate
|
||||
from *no* gate.
|
||||
|
||||
---
|
||||
|
||||
## 3. Test quality — five sabotages reproduced
|
||||
|
||||
### S4 (flood reverted to cylinder-first) — **the no-op argument holds**
|
||||
|
||||
I reverted `ShadowObjectRegistry.cs:649-652` to cylinder-first and ran the
|
||||
**whole solution**:
|
||||
|
||||
```
|
||||
Failed ShadowObjectRegistryMultiPartTests.BuildFloodSpheres_BspBearingOwner_FloodsFromBspNotFromCylinder
|
||||
Failed! - Failed: 1, Passed: 4263 ... (Core)
|
||||
Passed! - all 8 other test assemblies, including Headless 89/89
|
||||
```
|
||||
|
||||
`FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` — the end-to-end fact —
|
||||
stayed **green**, exactly as claimed. One test out of 11,203 reddens.
|
||||
|
||||
Combined with §1's producer enumeration, this is a proof rather than an
|
||||
anecdote: no production producer can emit a list on which the flood dispatch
|
||||
observably differs, so **the entire measured membership delta is attributable
|
||||
to the emission gate alone**, and the flood half is behaviour-identical today.
|
||||
|
||||
**Is the flood change justified?** Yes, and it is not a workaround. It is a
|
||||
faithful port of `calc_cross_cells`' own dispatch (documented at
|
||||
`ShadowObjectRegistry.cs:600-628`), kept on the same rationale that keeps
|
||||
`Transition.BspOnlyDispatch`. It is, however, honestly dead against current
|
||||
inputs — see latent risk #5. The gate alone would produce identical behaviour;
|
||||
the flood change buys retail-shape correctness for a producer that does not
|
||||
yet exist.
|
||||
|
||||
### S5′ (old `ScaleFactor` body under the same production sabotage) — **reproduced**
|
||||
|
||||
Production sabotage: `ShadowShapeBuilder.cs:161`,
|
||||
`Radius: sph.Radius * entScale` → `Radius: sph.Radius`.
|
||||
|
||||
- Corrected test body → **Failed**
|
||||
(`FromSetup_ScaleFactor_MultipliesAllRadiiAndOffsets`).
|
||||
- Old test body restored verbatim under the *same* sabotage → **Passed**.
|
||||
|
||||
The old test genuinely covered nothing: its radius/offset assertions sat
|
||||
inside `if (s.CollisionType == ShadowCollisionType.Cylinder)` on a fixture
|
||||
with zero CylSpheres. The correction is a strengthening, not a rewrite to
|
||||
make a failing test pass.
|
||||
|
||||
### Sabotage B (step-0 gate disabled → additive emission)
|
||||
|
||||
Reddens exactly the five added facts, across three projects:
|
||||
|
||||
```
|
||||
Core FromSetup_DoorSetup_EmitsBspPartsOnly
|
||||
Core FromSetup_DispatchGateReadsTheEffectivePartIdentities
|
||||
Core FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint
|
||||
App CylSphereAndPhysicsBspPart_EmitsOnlyTheScaledBspShape
|
||||
Content InstalledSetups_WithBothAPrimitiveAndAPhysicsBspPart_EmitOnlyBspShapes
|
||||
Headless 89/89 GREEN
|
||||
```
|
||||
|
||||
### DAT-sweep control (mine, not on the claimed list)
|
||||
|
||||
The installed-DAT sweep completes in ~700 ms, which is fast enough to look
|
||||
like a vacuous early return at
|
||||
`InstalledSetupBspPrimitiveDispatchTests.cs:59-60`. I falsified that:
|
||||
`ExpectedSetups = 5935 → 5936` produces
|
||||
`Assert.Equal() Failure: Expected 5936, Actual 5935`. The sweep really
|
||||
enumerates the installed `client_portal.dat`, and its four external bucket
|
||||
controls are load-bearing rather than derived from the predicate under test.
|
||||
|
||||
### Test-quality notes
|
||||
|
||||
- No test deleted (`-0 [Fact]` in the diff); both renamed tests are
|
||||
strengthened (`Assert.Single` + `Assert.All` + two `DoesNotContain` where
|
||||
there were loose counters).
|
||||
- The `return` on absent DATs is the established Content.Tests convention
|
||||
(`ContentConformanceDats.ResolveDatDir`, used identically by six sibling
|
||||
tests). Not a new skip.
|
||||
- No `Skip =`, no `try/catch`, no `Thread.Sleep`/`Task.Delay`, no new
|
||||
`GetEnvironmentVariable`, no suppression flag introduced anywhere in the
|
||||
diff.
|
||||
- Known flakes #302/#308/#321 untouched and not conflated.
|
||||
|
||||
---
|
||||
|
||||
## 4. Downstream consumers of the (now smaller) shape list — clean
|
||||
|
||||
- No production site indexes a `FromSetup` list positionally. The only
|
||||
positional loops (`LandblockPhysicsPublisher.cs:1083`, `:1098`) walk their
|
||||
own homogeneous list.
|
||||
- No site asserts "at least one primitive". `RegisterMultiPart` handles
|
||||
`shapes.Count == 0` by deregistering (`ShadowObjectRegistry.cs:454`), and
|
||||
§2 proves the count cannot newly become zero.
|
||||
- `TransitionTypes.cs:3759 / 3901 / 4089` branch per shape kind, never on the
|
||||
presence of a kind.
|
||||
- `WorldSceneDiagnosticsController.cs:222` is debug wireframe drawing.
|
||||
- `LiveEntityCollisionBuilder.cs:191-201`'s probe counts `cyl` vs `else`; the
|
||||
contract already documents that its `else` mislabels Sphere as `bsp`. That
|
||||
matters for reading the un-run connected gate, not for behaviour.
|
||||
|
||||
---
|
||||
|
||||
## Findings, ranked
|
||||
|
||||
### Defect
|
||||
None.
|
||||
|
||||
### Latent risk
|
||||
|
||||
**LR-1 [High] — for 99 of the 172 Setups the membership change is a SHRINK, in
|
||||
the same failure class the commit exists to fix, and it is un-gated.**
|
||||
`ShadowShapeBuilder.cs:150-164` stops emitting the Setup Sphere for any
|
||||
Sphere+BSP Setup, so `BuildFloodSpheres` (`ShadowObjectRegistry.cs:654-667`)
|
||||
now floods only from the BSP parts' bounding spheres — whose production radius
|
||||
is the flat BSP root bounding sphere (`LiveEntityCollisionBuilder.cs:137`).
|
||||
Retail's `find_bbox_cell_list` uses a bounding **box** over the whole part
|
||||
array; acdream approximates it with per-part bounding **spheres**, which
|
||||
AP-155 correctly registers. Failure scenario: a Sphere+BSP prop whose Setup
|
||||
sphere is larger than every part's BSP bounding sphere — the contract itself
|
||||
names `0x02001761` "Boulder" (2.00 m sphere + 1 BSP) as a candidate — loses
|
||||
shadow cells, and stops blocking when approached from the landcell it dropped
|
||||
out of while still blocking from another. That is the #98/#168 signature
|
||||
verbatim. The instrument for this is the contract's §8.2 item 3 keyed
|
||||
flood-set diff, and it **has not been run**. The commit's "NOT yet gated live"
|
||||
line covers it; this finding is to make sure the shrink direction, not only
|
||||
the flip direction, is what the connected session measures, and that the
|
||||
acceptance criterion "no affected owner ends with an empty set" is checked.
|
||||
|
||||
**LR-2 [Medium] — the blast radius is understated: this also changes the
|
||||
collision shape set, not only membership, wherever the wire flag is absent.**
|
||||
`Transition.BspOnlyDispatch` (`TransitionTypes.cs:1348-1349`) reads the
|
||||
*server's* `PhysicsState`, copied at `LiveEntityCollisionBuilder.cs:161`. The
|
||||
commit's "the collision half was already inert" is conditional on ACE setting
|
||||
`HAS_PHYSICS_BSP_PS` from `CSetup.HasPhysicsBSP` for every affected Setup. Any
|
||||
live entity where that bit is absent previously had its primitive tested and
|
||||
now does not. Failure scenario: an ACE build (or a different server) that
|
||||
omits the bit for one of the 172 — pre-change the mover collided with the
|
||||
cylinder, post-change it collides only with the slab BSP, and the two are not
|
||||
the same shape. This is retail-*correct* (retail derives the flag from the
|
||||
parts, `CPartArray::CacheHasPhysicsBSP`), and the underlying dependency is
|
||||
registered as AP-154, so nothing is hidden — but the commit message and the
|
||||
"NOT yet gated live" note both say "membership", and the honest statement is
|
||||
"membership, and collision wherever the server omits the bit".
|
||||
|
||||
**LR-3 [Medium] — `RegisterMultiPart`'s own XML doc still states the rule this
|
||||
commit inverted.** `src/AcDream.Core/Physics/ShadowObjectRegistry.cs:432-439`:
|
||||
"when the object has CylSpheres, they alone drive the flood … otherwise the
|
||||
BSP parts' bounding spheres stand in for the sorting sphere." That is now
|
||||
false, and it sits on the **public** method, 165 lines above the corrected
|
||||
`BuildFloodSpheres` block at `:600-628`. A reader who stops at the public
|
||||
API doc gets the pre-change rule. This is the same class of defect the commit
|
||||
was written to correct in AP-152's four false statements, and CLAUDE.md's
|
||||
"never leave them out of sync" applies.
|
||||
|
||||
**LR-4 [Low] — `LiveEntityCollisionBuilder`'s class doc still describes the
|
||||
additive policy.** `src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:32`:
|
||||
"CylSpheres before Spheres, **and** every physics-BSP part." The "and" is
|
||||
precisely the union this commit removed.
|
||||
|
||||
**LR-5 [Low] — the exclusivity invariant is whole-program, not locally
|
||||
enforced, and the flood guard's only live proof is synthetic.** Nothing in
|
||||
`AcDream.Core` prevents a future Content/Runtime producer from handing
|
||||
`RegisterMultiPart` a mixed list; the BSP-first flood branch is the guard, and
|
||||
S4 shows it is exercised by exactly one synthetic test. That is acceptable
|
||||
(it is a faithful port, and it is documented as forward insurance), but it
|
||||
should be understood as inert code with a synthetic-only witness rather than
|
||||
as covered behaviour.
|
||||
|
||||
**LR-6 [Low] — "All eight sabotages run and reported" has no artifact.**
|
||||
`docs/research/2026-08-06-ap152-contract.md` is the pre-implementation
|
||||
contract; no closeout records sabotage outcomes. Five hold under independent
|
||||
reproduction here; the other three rest on the commit message alone.
|
||||
|
||||
**LR-7 [Informational] — headless still has no live-entity collision at all
|
||||
(#330), so graphical and headless now dispatch shapes by different rules for
|
||||
the same world.** Pre-existing and correctly bounded by the commit ("do not
|
||||
fix #330 here"), but it is now load-bearing: the fix improves the graphical
|
||||
host only.
|
||||
|
||||
### Style
|
||||
Covered by LR-3 / LR-4 (both are stale docs on live symbols, so I ranked them
|
||||
as latent risk rather than style).
|
||||
|
||||
---
|
||||
|
||||
## AP-153 / AP-154 / AP-155 — honest residuals, not deferral
|
||||
|
||||
- **AP-153** (retail caches the dispatch flag once at
|
||||
`InitPartArrayObject+0x7e`; acdream's gate is live). A modelling difference
|
||||
the fix itself introduces, unreachable against ACE today because humanoid
|
||||
part swaps involve no physics-BSP GfxObj on either side. Filing it rather
|
||||
than building stale-flag state is the right call — modelling it would be
|
||||
speculative machinery for an unreachable case.
|
||||
- **AP-154** (wire-derived `HAS_PHYSICS_BSP_PS`). **It does not undercut the
|
||||
gate's premise.** The gate is now derived from the parts, which is exactly
|
||||
what `CPartArray::CacheHasPhysicsBSP` does; AP-154 is about the *query-time*
|
||||
guard `BspOnlyDispatch`, which the commit deliberately leaves alone. What it
|
||||
does do is name the assumption the "already inert" argument rests on — which
|
||||
is why LR-2 is a framing problem rather than a hidden one. Filing it is
|
||||
correct; it should not have been resolved inside this commit.
|
||||
- **AP-155** (static paths emit Setup Spheres as height-capped Cylinders; the
|
||||
flood approximates retail's bbox with bounding spheres). Its membership half
|
||||
is genuinely closed here; its static half is explicitly left open. The
|
||||
second clause is the mechanism behind LR-1, and the row states it plainly
|
||||
rather than burying it.
|
||||
|
||||
None of the three defers a part of the defect this commit set out to fix.
|
||||
|
||||
---
|
||||
|
||||
## Rules compliance
|
||||
|
||||
No workaround, suppression flag, grace period, retry loop, or symptom guard.
|
||||
No `if (problematicState) return` at a symptom site — the gate is at the
|
||||
producer, derived from the same data retail derives it from. No new skips
|
||||
(4 skipped, unchanged). No test weakened to pass; both corrected tests are
|
||||
strictly stronger and neither was deleted. Register updated in the same
|
||||
commit, with count arithmetic checked (105 → 107).
|
||||
|
||||
---
|
||||
|
||||
## What I checked, so the PASS is auditable
|
||||
|
||||
1. Full diff of both production files and all five test files.
|
||||
2. Every `ShadowShape` producer in `src/` (8 sites) and every
|
||||
`RegisterMultiPart` caller (6 production sites) read individually.
|
||||
3. `.csproj` reference graph for Headless / Runtime / Content / Core.
|
||||
4. `grep` for `new ShadowShape(` and `ShadowObjects.*` in `AcDream.Runtime`
|
||||
(empty / read-only).
|
||||
5. Downstream consumers keyed on `ShadowCollisionType.Cylinder|Sphere`, on
|
||||
`shapes[0]`, and on `shapes.Count`.
|
||||
6. Clean Release build after deleting all 44 `bin`/`obj` dirs: 0 / 21.
|
||||
7. Full solution suite twice on clean builds: 11,203 / 4 / 0 both times.
|
||||
8. Five sabotages reproduced (trap-1 gate identity, S4 flood revert, S5′
|
||||
scale-factor pair, B gate-disabled, DAT-sweep count control), each with the
|
||||
tree restored and `git status --porcelain` verified empty afterwards.
|
||||
9. Register row count at HEAD vs `ec29a732`.
|
||||
10. Diff scanned for `Skip =`, `try/catch`, sleeps, env-var reads, TODO/HACK.
|
||||
|
||||
Final tree state: **clean**, no modifications left behind. My single write is
|
||||
this file.
|
||||
416
docs/research/2026-08-06-ap152-review-retail.md
Normal file
416
docs/research/2026-08-06-ap152-review-retail.md
Normal file
|
|
@ -0,0 +1,416 @@
|
|||
# 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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue