docs: close the AD-10 review findings — AD-65's magnitude was half the truth
Both AD-10 review lenses PASS; the deletion stands. These are the findings they raised. One production file touched, comment-only. AD-65 WAS UNDERSTATED BY HALF, and it is the finding that matters. The row states the factor as cos^2(theta) and then quantified 1-cos(theta): "13% at 30 degrees, 29% at 45". The correct figures are 25% and 50%. This is not algebra alone — #331's probe in the same push measures 0.0735 m travelled for a 0.1 m request at 30.96 degrees, i.e. 26.5% short, which is exactly cos^2(30.96). AD-65 is a LEAD for #269's slope-slide residual; at the understated magnitude it reads as marginal and could have been dismissed. At 50% short at 45 degrees it is a serious candidate. I repeated the wrong figure in conversation before the review caught it. "VERBATIM/FAITHFUL PORT" of Transition.AdjustOffset was asserted in five places and was false as of the very next commit, which filed AD-65 and AD-66 against that same function. Corrected to "structurally exact, with exactly two filed divergences" in the register row and the production doc comment. RECORDED, and it favours the change: the redundancy measurement is CONTINGENT on AD-65 — the two mechanisms agree today partly because both under-travel downhill. That makes this deletion a PREREQUISITE for fixing AD-65 rather than merely compatible with it; had the projection survived, correcting AdjustOffset would have re-introduced a disagreement between two live projections. The record claimed no such thing and should have. UNTESTED AXIS recorded: the contract's T2 — its mandatory wrong-plane-versus- right-plane discriminator — was dropped without record, breaching the contract's own clause requiring exactly that to be written down. The consequence is precise: the deletion is measured, but the change's only claimed BENEFIT (a walkable non-terrain surface now gets the committed contact plane instead of terrain far below) has zero automated coverage and rests on source reasoning. Stated in the row rather than left implied. #331 SEVERITY RAISED from UNKNOWN — the discriminator is known and it is not the fixture. With `body: null` the same uphill sweep climbs (ok=True, moved (0, -0.0999, +0.060)); with a body supplied it returns ok=False and zero movement, under a call profile identical to the local player's (IsPlayer|EdgeSlide + the human two-sphere Setup). A diagonal request keeps cross-slope X and zeroes only up-slope Y, and it fires on a 1.1 degree ramp. So "confined to the synthetic fixture" is no longer the comfortable default: the failing call shape is the shape production uses. Nothing in the suite asserts uphill progress on a walkable slope, which is why it was invisible — the test that found it passed vacuously, because the body never moved. Also: malformed XML doc on ComposeOffset (duplicate </summary> swallowed the retirement note from tooling) fixed; the placement-cutover plan's item 5 and its stale "After C5" line now record AP-22 and AD-10 as retired. Core builds clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
2223ed1745
commit
7b3e2895cd
6 changed files with 881 additions and 10 deletions
|
|
@ -77,11 +77,31 @@ been vacuous evidence.
|
|||
## #331 — `ResolveWithTransition` refuses ALL uphill motion on a constant-gradient terrain ramp (fixture-or-production unresolved)
|
||||
|
||||
**Status:** OPEN
|
||||
**Severity:** UNKNOWN until the fixture-versus-production question below is
|
||||
settled. If it reproduces on DAT terrain it is severe and affects the local
|
||||
player as much as remotes; if it is confined to the synthetic fixture it is a
|
||||
test-infrastructure defect that silently voids any uphill assertion written
|
||||
against that fixture — which is how it was found.
|
||||
**Severity:** **RAISED from UNKNOWN 2026-08-06 at the AD-10 architecture
|
||||
review — the discriminator is now known and it is NOT the fixture.**
|
||||
|
||||
The deciding variable is the **`body:` parameter**, not terrain publication:
|
||||
|
||||
- `body: null` → the same uphill sweep climbs fine: `ok=True`, moved
|
||||
`(0, −0.0999, +0.060)`.
|
||||
- `body:` supplied → `ok=False` and **zero** movement.
|
||||
- It reproduces under a call profile **identical to the local player's**
|
||||
(`IsPlayer | EdgeSlide` plus the human two-sphere Setup list).
|
||||
- A **diagonal** request keeps its cross-slope X and zeroes only the up-slope
|
||||
Y — i.e. the slope is behaving as a wall in exactly one direction.
|
||||
- It fires on a **1.1° ramp**, not just steep ones.
|
||||
|
||||
So the original first lead (terrain publication / synthetic fixture) is now the
|
||||
*less* likely explanation, and "confined to the fixture" is no longer a
|
||||
comfortable default: the failing call shape is the shape production uses.
|
||||
Nothing in the suite asserts uphill progress on a walkable slope, which is why
|
||||
this has never been caught — the test that found it passed **vacuously**,
|
||||
because the body never moved.
|
||||
|
||||
Prior severity note, retained: if it reproduces on DAT terrain it is severe and
|
||||
affects the local player as much as remotes; if it is confined to the synthetic
|
||||
fixture it is a test-infrastructure defect that silently voids any uphill
|
||||
assertion written against that fixture — which is how it was found.
|
||||
**Filed:** 2026-08-06, while measuring AD-10 (commits `fe6ee877`, `886333a2`).
|
||||
**Not caused by AD-10, and unaffected by its deletion** — the behaviour is
|
||||
identical with the pre-sweep slope projection enabled and disabled.
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -84,6 +84,17 @@ Remaining campaign work, in order:
|
|||
5. Delete the superseded paths, retire AP-1/AD-1/AP-131 and AD-60's legacy
|
||||
half only when the code proves they are gone, then complete AP-22 and
|
||||
AD-10 and close the campaign ledger.
|
||||
**DONE except the ledger close, 2026-08-05/06.** AP-1/AD-1 retired at C5a
|
||||
(`6921a027`); AP-131 and AD-60's legacy half at C5b (`735f0a72`); **AP-22**
|
||||
retired at `bc4679cd` (all three invented-cylinder copies deleted — the row
|
||||
listed one; reachability proved zero over all 5,935 installed Setups by four
|
||||
independent decoders); **AD-10** retired by deletion at `886333a2` (its
|
||||
stated justification was false at HEAD — remotes DO run the sweep, so the
|
||||
projection was an extra non-retail layer, measured bit-identical when
|
||||
removed). Both dual-reviewed, both lenses PASS. Remaining: C5c's gates and
|
||||
the ledger close. Two new divergences were filed out of AD-10's work
|
||||
(AD-65, AD-66) and two issues (#331 uphill-resolve blockage, #332 headless
|
||||
remote dead-reckoning).
|
||||
|
||||
**Inputs (read in order):**
|
||||
1. [`2026-08-02-runtime-continuation-executor-handoff.md`](../research/2026-08-02-runtime-continuation-executor-handoff.md)
|
||||
|
|
@ -627,6 +638,8 @@ same commit) → docs/handoff commit. No workarounds; no fused slices.
|
|||
include a first-login stop, because login shares the same barrier and its
|
||||
gate widened too.
|
||||
|
||||
After C5: ~~AP-22~~ (RETIRED 2026-08-06, bc4679cd) and ~~AD-10~~ (RETIRED
|
||||
2026-08-06 by deletion, 886333a2) are both DONE. Historical text follows.
|
||||
After C5: AP-22 (authored collision shapes), then AD-10 (remote
|
||||
contact-plane projection), then the campaign's final matrix and ledger
|
||||
closeout; vendor Slice 5 resumes.
|
||||
|
|
|
|||
357
docs/research/2026-08-06-ad10-review-architecture.md
Normal file
357
docs/research/2026-08-06-ad10-review-architecture.md
Normal file
|
|
@ -0,0 +1,357 @@
|
|||
# AD-10 retirement-by-deletion — architecture review
|
||||
|
||||
**Reviewer scope:** completeness, blast radius, test quality. Retail fidelity is
|
||||
a separate reviewer's report (`2026-08-06-ad10-review-retail.md`).
|
||||
**Range:** `ef976c6d..2223ed17` on `claude/acdream-physics-divergence-5aa784`
|
||||
(`fe6ee877`, `886333a2`, `fb454b74`, `2223ed17`).
|
||||
**Date:** 2026-08-06.
|
||||
|
||||
## Verdict: **PASS**, with one process defect and one under-scoped issue
|
||||
|
||||
The Stage 0 measurement is **sound and I believe it** — I re-derived both
|
||||
reported numbers independently and added four cases the implementer did not
|
||||
report. The deletion is complete, the blast radius claim is correct, all three
|
||||
reported sabotages reproduce with matching numerals, and the two deleted tests
|
||||
were genuinely obsolete.
|
||||
|
||||
Nothing here is a workaround, a suppression flag, a grace period, a symptom
|
||||
guard, a new skip, or a weakened test. No known flake was conflated.
|
||||
|
||||
The defects are: one **mandatory contract test silently dropped** (D1), a
|
||||
**stale roadmap entry** (D2), and a **malformed doc comment** (D3). None
|
||||
justifies blocking the change; D1 should be settled before the visual gate is
|
||||
called done.
|
||||
|
||||
---
|
||||
|
||||
## Part 1 — Is the Stage 0 measurement sound? (priority 1)
|
||||
|
||||
### Method
|
||||
|
||||
I did not take the implementer's word for it. I deleted all 44 `bin`/`obj`
|
||||
directories, built Release clean (0 errors), byte-scanned
|
||||
`AcDream.Core.dll` to confirm the string `SampleTerrainNormal` is absent from
|
||||
the compiled metadata (0 occurrences; `SampleTerrainWalkable` still present at
|
||||
2, proving the scan works), then drove `RemoteRampHarness` through the
|
||||
production `RuntimeRemotePhysicsUpdater.Tick` for 30 ticks and dumped every
|
||||
position as **raw IEEE-754 bits**.
|
||||
|
||||
I then restored the pre-deletion production files verbatim
|
||||
(`git checkout ef976c6d -- ` the three `src/` files — those files changed only
|
||||
in `886333a2`, so this is an exact revert) and re-ran the identical dump.
|
||||
|
||||
### Result — reproduced, and extended
|
||||
|
||||
| Case | gradient | root motion / tick | pre-deletion vs HEAD |
|
||||
|---|---|---|---|
|
||||
| downhill 31° | 0.6 | (0, +0.10, 0) | **bit-identical, all 30 ticks** |
|
||||
| downhill 8.4° | 0.1477 | (0, +0.10, 0) | X, Y bit-identical; Z differs by **2.9e-5 m** at tick 30 |
|
||||
| **cross-slope 31°** | 0.6 | (+0.10, 0, 0) | **bit-identical** |
|
||||
| **diagonal 31°** | 0.6 | (+0.0707, +0.0707, 0) | **bit-identical** |
|
||||
| **flat** | 0.0 | (0, +0.10, 0) | **bit-identical** |
|
||||
| uphill 31° | 0.6 | (0, −0.10, 0) | bit-identical — **but vacuous, see L1** |
|
||||
|
||||
Both reported numbers land exactly. The last three rows are mine; they close
|
||||
the obvious "only one direction was measured" objection.
|
||||
|
||||
### Is "bit-identical" vacuous — did neither path do anything?
|
||||
|
||||
**No, for the downhill case.** Over 30 ticks the body advances Y by 2.206 m and
|
||||
descends 1.324 m. Those are not noise: they are exactly the analytically
|
||||
predicted projected step. For `N = (0, 0.5145, 0.8575)` and a requested
|
||||
`(0, 0.1, 0)`, the projection `v − N·(v·N)` gives `(0, 0.07353, −0.04412)`;
|
||||
the measured per-tick advance is `0.07353 / −0.04412`. The sweep alone
|
||||
reproduces the deleted projection to the last digit.
|
||||
|
||||
The idempotency argument the commit message gives also holds in the code, not
|
||||
just on paper: `Transition.AdjustOffset`
|
||||
(`src/AcDream.Core/Physics/TransitionTypes.cs:5246-5258`) subtracts the full
|
||||
normal component in **both** its `collisionAngle <= 0` and `> 0` arms, so a
|
||||
vector already on the plane is returned unchanged. That is why the composition
|
||||
collapses.
|
||||
|
||||
**Yes, for the uphill case** — and the implementer said so and refused to ship
|
||||
the test. That refusal was correct (see L1).
|
||||
|
||||
### Does the fixture make it vacuous?
|
||||
|
||||
The concern is real and worth naming: the ramp is one constant-gradient plane
|
||||
over the whole landblock, so `SampleTerrainNormal(x, y)` **equals**
|
||||
`Body.ContactPlane.Normal` everywhere. That is precisely the case where
|
||||
idempotency guarantees no change — the friendliest possible geometry for the
|
||||
"redundant" claim.
|
||||
|
||||
But it is also the **right** geometry for the question actually being asked.
|
||||
Deletion can only regress where the two planes agree (there the old layer was
|
||||
doing correct work that must now come from somewhere else); where they
|
||||
disagree the old layer was applying a *wrong* plane, so removing it cannot be a
|
||||
regression. The measurement covers the regression-capable case exactly, and my
|
||||
cross-slope/diagonal/flat additions cover it in three more directions.
|
||||
|
||||
**Verdict on Stage 0: I believe it.** The redundancy claim is established for
|
||||
terrain. What is *not* established is the claimed improvement — see D1.
|
||||
|
||||
---
|
||||
|
||||
## Part 2 — Findings
|
||||
|
||||
### Defects
|
||||
|
||||
#### D1 — a mandatory contract test was dropped with no record (process)
|
||||
|
||||
`docs/research/2026-08-06-ad10-contract.md:546-577` specifies **T2**, "the
|
||||
discriminating test", on a fixture where the committed contact plane differs
|
||||
from the terrain sample at the same XY. Construction (a), a two-gradient
|
||||
terrain crest, is marked **"Mandatory."** Construction (b), an off-terrain
|
||||
walkable surface, is **"Required, with a documented fallback: if the fixture
|
||||
cannot be built in reasonable time, say so explicitly in the closeout, ship on
|
||||
(a), and record (b) as an untested axis rather than silently dropping it."**
|
||||
Line 690 of the same contract puts T2–T5 in the deletion commit.
|
||||
|
||||
Neither exists. `grep -rn -i "crest|ridge|two-gradient" tests/AcDream.Runtime.Tests/`
|
||||
returns nothing, and no commit message, register row, or doc in the range
|
||||
records the disposition of T2.
|
||||
|
||||
T3, T4 and T5 are defensibly moot under deletion (T3 and T4 test guards that no
|
||||
longer exist; T5's intent is superseded by the stronger compile-error
|
||||
guarantee). **T2 is not moot.** Under deletion it becomes the test for the one
|
||||
behavioural *benefit* the change claims — and that claim is stated as fact in
|
||||
two places without evidence:
|
||||
|
||||
- `docs/architecture/retail-divergence-register.md:122` (the retired AD-10 row);
|
||||
- `886333a2` commit message: *"What deletion does improve is the case #32 never
|
||||
covered: a remote on a WALKABLE non-terrain surface … That surface now gets
|
||||
the body's own committed contact plane."*
|
||||
|
||||
**Concrete failure scenario.** A remote runs along a sloped wooden bridge or a
|
||||
dock over sloped terrain. After deletion the *only* projection is the sweep's,
|
||||
which runs `only when ci.ContactPlaneValid` (`TransitionTypes.cs:5202-5224`
|
||||
takes the no-contact-plane branch otherwise). If `check_contact` ever fails to
|
||||
seed a contact plane on a tick where the body is `OnWalkable`, the offset is
|
||||
not projected at all, where before the terrain sample supplied one. The body
|
||||
holds Z between server updates — the exact ~5 Hz staircase the projection
|
||||
existed to remove — and **nothing in CI goes red**, because every automated
|
||||
assertion about this behaviour lives on a pure-terrain fixture.
|
||||
|
||||
I judge the residual risk *low* (on flat terrain under a bridge the deleted
|
||||
projection was a near-no-op anyway), but "low risk" is a different statement
|
||||
from "measured", and the contract explicitly forbade making it silently.
|
||||
|
||||
**Ask:** either build T2(a) — the harness already supports it, it is a
|
||||
two-gradient heightmap in `RemoteRampHarness.Ramp` — or add one paragraph to
|
||||
the register row recording T2 as a deliberately untested axis, per the
|
||||
contract's own fallback clause.
|
||||
|
||||
#### D2 — stale roadmap entry (docs)
|
||||
|
||||
`docs/plans/2026-04-11-roadmap.md:108` still lists
|
||||
|
||||
> `AP-22 authored object shapes, and AD-10 remote contact-plane projection.`
|
||||
|
||||
as open campaign work. AD-10 is retired. CLAUDE.md's roadmap discipline rule 3
|
||||
requires the roadmap update in the same commit as, or immediately after, the
|
||||
work. The register row, both downstream `docs/ISSUES.md` cross-references
|
||||
(`:10513-10515`, `:10672-10676`) and the historical record were all swept
|
||||
correctly — the roadmap was the one miss.
|
||||
|
||||
#### D3 — malformed XML doc comment (style)
|
||||
|
||||
`src/AcDream.Core/Physics/RemoteMotionCombiner.cs:38-53`. The AD-10 retirement
|
||||
rationale is written as a `<para>` block placed *after* the closing
|
||||
`</summary>` on line 38, and terminated by a second `</summary>` on line 52:
|
||||
|
||||
```
|
||||
38 /// </summary>
|
||||
39 ///
|
||||
40 /// <para><b>AD-10, retired 2026-08-06.</b> …
|
||||
52 /// …unchanged.</para>
|
||||
53 /// </summary> <- unmatched close tag
|
||||
```
|
||||
|
||||
Two closes, one open, one orphaned `<para>`. The build is green only because
|
||||
this project does not generate documentation files; the rationale will not
|
||||
render in IntelliSense, and the file would emit CS1570 the moment
|
||||
`GenerateDocumentationFile` is turned on. Same paragraph reads fine — just move
|
||||
it inside the first `</summary>`.
|
||||
|
||||
### Latent risks
|
||||
|
||||
#### L1 — #331 is real, is under-scoped, and one cheap probe moves it a long way
|
||||
|
||||
The issue as filed (`docs/ISSUES.md:75-150`) is honest and unusually thorough:
|
||||
it rules out gradient, step size, cell boundaries, Z seating, and axis, and it
|
||||
names the vacuous test that found it. I reproduced every one of those
|
||||
exclusions. But the "fixture-versus-production" question it leaves open is
|
||||
answerable more cheaply than it says, and the answer points away from the lead
|
||||
the issue names.
|
||||
|
||||
**What I measured**, calling `PhysicsEngine.ResolveWithTransition` *directly*
|
||||
on the harness's engine (no remote tick involved):
|
||||
|
||||
| call | result |
|
||||
|---|---|
|
||||
| uphill, `body: rm.Body`, `isOnGround: true` | `ok=False`, moved `(0,0,0)` |
|
||||
| uphill, `body: rm.Body`, `isOnGround: false` | `ok=False`, moved `(0,0,0)` |
|
||||
| **uphill, `body: null`** | **`ok=True`, moved `(0, −0.0999, +0.060)`** |
|
||||
| uphill, `body: null`, lifted 0.5 m | `ok=True`, moved `(0, −0.0999, 0)` |
|
||||
| uphill, with `IsPlayer\|EdgeSlide` + the human two-sphere Setup list | `ok=False` |
|
||||
| uphill diagonal `(0.1, −0.1, 0)`, `body:` supplied | `ok=True`, moved **`(0.1, 0, 0)`** |
|
||||
| downhill / cross-slope / straight-up / straight-down, `body:` supplied | `ok=True`, all correct |
|
||||
|
||||
Three things follow that the issue does not yet carry:
|
||||
|
||||
1. **The discriminator is the `body:` parameter**, i.e. the seeded
|
||||
contact-plane / retained-walkable-polygon path (retail `check_contact`).
|
||||
Without a body the same uphill sweep climbs; with one it refuses. Production
|
||||
*always* passes a body — the local player at
|
||||
`src/AcDream.Runtime/Gameplay/PlayerMovementController.cs:2646`, the remote
|
||||
at `src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs:424`.
|
||||
2. **It is not remote-specific and not sphere-list-specific.** It reproduces
|
||||
under a call profile identical to the local player's, including
|
||||
`IsPlayer | EdgeSlide` and the human Setup's two-sphere list. The issue's
|
||||
stated "first thing to check" — the terrain publication path
|
||||
(`AddLandblock` vs `LandblockPhysicsContentBuilder.PublishStaticCollision`)
|
||||
— is now the *less* likely lead of the two.
|
||||
3. **The up-slope component is clipped, not the whole step.** The diagonal case
|
||||
returns `ok=True` and keeps its cross-slope X while zeroing its up-slope Y.
|
||||
A walkable 31° ramp is behaving like a wall for up-slope motion, and a
|
||||
1.1° ramp (gradient 0.02) does the same.
|
||||
|
||||
**Nothing in the repository proves walkable uphill progress works anywhere.**
|
||||
`RetailEdgeResponseOrderingTests.cs:277-312` runs a downhill / uphill /
|
||||
tangential theory and asserts horizontal progress for *downhill and tangential
|
||||
only* — the `uphill` arm is routed to a no-bounce assertion instead. That is
|
||||
correct for its unwalkable steep-roof fixture, but it means the suite has no
|
||||
uphill-progress assertion to lean on.
|
||||
|
||||
**This is not a regression from AD-10** — I confirmed the uphill trajectory is
|
||||
bit-identical before and after the deletion, so nothing here blocks this change.
|
||||
But #331's severity should not stay UNKNOWN: the trigger is now known to be a
|
||||
production-shaped call, and the remaining unknown is narrow enough to settle in
|
||||
one probe.
|
||||
|
||||
#### L2 — the surviving tests cannot see the projection, only the Z seating
|
||||
|
||||
Verified, and quantified. With `Transition.AdjustOffset` short-circuited to
|
||||
`return offset;`:
|
||||
|
||||
- `RuntimeRemoteSlopeProjectionTests` — **all green**, confirming the
|
||||
implementer's rejection of the contract's proposed T1 sabotage;
|
||||
- the downhill Y advance goes from **0.0735 m/tick to 0.1000 m/tick**, i.e. the
|
||||
remote runs **36% faster downhill**, and the tests do not notice;
|
||||
- Z still tracks the surface exactly, because `ValidateWalkable`'s push-out
|
||||
(`TransitionTypes.cs:3445-3452`, reached from `FindEnvCollisions`) re-seats
|
||||
the sphere at its natural resting distance every sub-step. The implementer's
|
||||
stated third mechanism is real, both structurally and empirically.
|
||||
|
||||
This is disclosed, at length and unprompted, in the test's own doc comment
|
||||
(`RuntimeRemoteSlopeProjectionTests.cs:97-113`), including the instruction that
|
||||
it "must not be cited as" a unit test of `adjust_offset`. That is the correct
|
||||
handling of a non-discriminating test.
|
||||
|
||||
It is also not a coverage hole overall: running the full suite under that same
|
||||
sabotage produces **15 failures** — 14 in `AcDream.Core.Tests` plus
|
||||
`RuntimeRemoteSteepContactSlideTests.SteepContactKeepsTheBodySlidingDownhill`
|
||||
("expected a slide, body moved 0.0000 m"), which is the Runtime remote path's
|
||||
own guard. No action required; recorded so nobody later mistakes the new file
|
||||
for `adjust_offset` coverage.
|
||||
|
||||
#### L3 — "bit-identical" is a property of this fixture, not of the change
|
||||
|
||||
The 8.4° ramp differs by 2.9e-5 m in Z because `dot(v', N)` is not exactly
|
||||
zero in float after the first projection. The 31° case lands bit-identical; a
|
||||
third gradient might not. The commit message already reports both numbers and
|
||||
calls the difference float ordering noise, which is the honest framing. Nobody
|
||||
should generalise "bit-identical" into a guarantee.
|
||||
|
||||
---
|
||||
|
||||
## Part 3 — What I checked and found clean (so the PASS is auditable)
|
||||
|
||||
**Deletion completeness (priority 2).** Three independent sweeps:
|
||||
`SampleTerrainNormal` (zero code references — only comments and docs),
|
||||
`terrainNormal` case-insensitive (same), and the formula shape
|
||||
`* Vector3.Dot(` / `-= N *` across all of `src/` (only an unrelated
|
||||
`Vfx/ParticleSystem.cs:1245` axis projection). No fourth copy, no dead
|
||||
diagnostic, no test reference. Both fork branches carried the block verbatim
|
||||
pre-deletion (`:272-282` and `:317-325`) and both are gone — the AP-22 shape was
|
||||
handled. `SampleTerrainWalkable`, the internal the deleted wrapper called, is
|
||||
**not** orphaned: it retains live callers at `TransitionTypes.cs:2909` and
|
||||
`:3432`. `ComputeOffset` confirmed production-dead — the only callers are in
|
||||
`tests/AcDream.Core.Tests/`. Removing the parameter rather than passing null
|
||||
does make a one-site reintroduction a compile error, as claimed.
|
||||
|
||||
**Sabotage reproduction (priority 3).** All three reproduce, from clean builds:
|
||||
|
||||
| sabotage | reported | reproduced |
|
||||
|---|---|---|
|
||||
| `rm.Body.Position = postIntegratePos` (`:457`) | RED at tick 1, 0.05999 m off surface | **RED at tick 1, "body root sits 0.05999 m above the terrain under it"** |
|
||||
| flatten ramp to gradient 0 | RED on the anti-vacuity guard, `dz = 0.0000` | **RED, "fixture is not exercising slope descent: dz = 0.0000 m"** |
|
||||
| short-circuit `Transition.AdjustOffset` | GREEN | **GREEN** (see L2) |
|
||||
|
||||
The anti-vacuity guard is real and load-bearing: note that the *other* fixture
|
||||
test, `TheFixtureRampIsWalkableAndItsPlaneIsTheGeometricOne`, passes happily on
|
||||
a flattened ramp (`RampNormal(0)` is `(0,0,1)` and so is the flat contact
|
||||
plane), so the guard is the only thing standing between this file and a
|
||||
silently-flattened fixture.
|
||||
|
||||
**Deleted tests (priority 4).** Both were genuinely obsolete.
|
||||
`..._SlopedTerrainNormal_ProjectsZOntoSlope` asserted only the deleted formula,
|
||||
against expected values re-derived from that same formula in a comment — it
|
||||
could catch a wrong multiply but never a wrong plane, exactly as the commit
|
||||
message says. `..._FlatTerrainNormal_NoZChange` is a *literal duplicate* of the
|
||||
surviving `ComputeOffset_AnimationOnly_Forward_BodyAdvances`
|
||||
(`RemoteMotionCombinerTests.cs:51-68`): same `dt: 0.1`, same
|
||||
`rootMotionLocalDelta: (0, 0.4, 0)`, same identity orientation, same asserted
|
||||
`(0, 0.4, 0)`. Nothing was asserted by either test that is not still asserted.
|
||||
The tombstone comment left in their place names both and points at the
|
||||
replacement.
|
||||
|
||||
**Harness extraction (`fe6ee877`).** Filtering the diff of
|
||||
`RuntimeRemoteSteepContactSlideTests.cs` to `Assert` / `[Fact]` / `[Theory]`
|
||||
lines yields **zero** added or removed assertions — the extraction moved the
|
||||
private nested `Harness` out verbatim and nothing else. Its start position
|
||||
constants are unchanged (96, 96). All ten Bug B tests still pass.
|
||||
|
||||
**Blast radius (priority 6).** Confirmed exactly as stated.
|
||||
`new RuntimeRemotePhysicsUpdater(` has one production site,
|
||||
`src/AcDream.App/Physics/RemotePhysicsUpdater.cs:46`; every other match is a
|
||||
test fixture. `src/AcDream.Headless/` contains no reference to
|
||||
`RuntimeRemotePhysicsUpdater`, `RemotePhysicsUpdater`, `RemoteMotion` or
|
||||
`RemoteMotionCombiner`. The class is `internal` to `AcDream.Runtime` and reaches
|
||||
production only via `InternalsVisibleTo` into `AcDream.App`
|
||||
(`AcDream.Runtime.csproj:12`). `RemoteMotionCombiner` lives in `AcDream.Core`
|
||||
and is instantiated in `RemoteMotion.cs:242`, but the only things that *drive*
|
||||
it are the three `ComposeOffset` sites in `RuntimeRemotePhysicsUpdater`. The
|
||||
inverse of C5b's lesson does apply here and the implementer got it right:
|
||||
assembly placement is not reachability, and a green headless gate would have
|
||||
been vacuous evidence. #332 records that reasoning correctly.
|
||||
|
||||
**`TickHidden` (priority 7).** Correct to leave untouched. I byte-compared the
|
||||
call at `RuntimeRemotePhysicsUpdater.cs:940-948` against `ef976c6d`: it is
|
||||
character-for-character identical, and it used the named argument
|
||||
`inContact:` while skipping the optional `terrainNormal`, so the hidden-remote
|
||||
path always received `null`. Deleting the parameter is a no-op there by
|
||||
construction.
|
||||
|
||||
**Gates.** From a state with all 44 `bin`/`obj` directories deleted:
|
||||
Release build **0 errors**; full solution suite **11,196 passed / 4 skipped /
|
||||
0 failed**, matching the claim exactly and reconciling with the stated
|
||||
`11,195 / 4 / 0` baseline as +3 new Runtime `[Fact]`s and −2 deleted Core tests.
|
||||
Run twice — once before my sabotage experiments and once after restoring the
|
||||
tree — with identical totals. Skip count unchanged at 4 (3 App, 1 Core); no
|
||||
skip was added.
|
||||
|
||||
**Tree state.** `git status --porcelain` and `git diff HEAD` are clean. The one
|
||||
untracked file, `docs/research/2026-08-06-ad10-review-retail.md`, belongs to the
|
||||
concurrent retail-fidelity reviewer and was not touched. Every temporary
|
||||
sabotage and probe file I introduced was reverted or deleted and verified gone.
|
||||
|
||||
**Rule compliance.** No workaround, no suppression flag, no grace period, no
|
||||
`if (badState) return`, no swallowed exception, no widened tolerance, no new
|
||||
skip. The 5 mm tolerance in the new tracking test is justified in its own doc
|
||||
comment against both the failure it must catch (~1.8 m) and the noise it must
|
||||
tolerate (<1e-4 m), and I confirmed the ratio by measurement. Known flakes
|
||||
#302 / #308 / #321 were not touched or conflated. AD-65 and AD-66 were filed as
|
||||
separate register rows rather than folded into this change, which is the right
|
||||
call — both change local-player feel and need their own gate.
|
||||
481
docs/research/2026-08-06-ad10-review-retail.md
Normal file
481
docs/research/2026-08-06-ad10-review-retail.md
Normal file
|
|
@ -0,0 +1,481 @@
|
|||
# AD-10 retirement — retail-conformance review
|
||||
|
||||
**Verdict: PASS**, with one MEDIUM documentation defect that must be corrected
|
||||
before the register row is trusted (AD-65's quantification), and three LOW
|
||||
items.
|
||||
|
||||
- **Reviewed:** `ef976c6d..2223ed17` on `claude/acdream-physics-divergence-5aa784`
|
||||
in worktree `.claude/worktrees/peaceful-visvesvaraya-e0a196`
|
||||
(`fe6ee877`, `886333a2`, `fb454b74`, `2223ed17`).
|
||||
- **Lens:** retail conformance. Every retail claim below was re-derived from
|
||||
the PDB-paired binary, not from the contract, the commit messages, or the
|
||||
Binary Ninja pseudo-C.
|
||||
- **Binary:** `C:\Users\erikn\Downloads\acclient.exe`,
|
||||
`check_exe_pdb.py` → `=== MATCH ===`, linker UTC 2013-09-06T00:17:56,
|
||||
CodeView GUID `9e847e2f-777c-4bd9-886c-22256bb87f32`. Image base `0x00400000`.
|
||||
Disassembled with capstone x86-32 straight off the file, VA→file-offset
|
||||
through the section table; floats read from `.rdata` as raw bytes.
|
||||
- **Build/test:** all 44 `bin`/`obj` directories deleted, full
|
||||
`dotnet build AcDream.slnx -c Release` → **0 errors**; full
|
||||
`dotnet test --no-build` → **0 failed**. Staleness disproved positively:
|
||||
`SampleTerrainNormal` is **absent** from the freshly built
|
||||
`AcDream.Core.dll` byte image (and `RuntimeRemoteSlopeProjectionTests` is
|
||||
present in the test image), so the runner is genuinely serving the deleted
|
||||
code.
|
||||
|
||||
---
|
||||
|
||||
## 1. Independent verification of the retail claims
|
||||
|
||||
### 1.1 Is `Transition.AdjustOffset` a faithful port of `CTransition::adjust_offset` @0x0050a370?
|
||||
|
||||
**Yes — structurally exact, with exactly the two exceptions the change itself
|
||||
filed as AD-65 and AD-66. No third divergence found.**
|
||||
|
||||
Function extent from the binary: `0x0050a370` (`sub esp,0x24`) to `0x0050a6c7`
|
||||
(`ret 8`), nops to `0x0050a6d0` where `CTransition::cliff_slide` begins.
|
||||
Symbol confirmed from `symbols.json`:
|
||||
`0x0050A370 CTransition::adjust_offset`,
|
||||
mangled `?adjust_offset@CTransition@@IAE?AVVector3@AC1Legacy@@ABV23@@Z`
|
||||
(returns `Vector3`, takes `const Vector3&`).
|
||||
|
||||
Field map recovered from the code: `this+0x2a8` = `sliding_normal_valid`,
|
||||
`this+0x2ac/0x2b0/0x2b4` = `sliding_normal.xyz`, `this+0x288` =
|
||||
`contact_plane_valid`, `esi = this+0x28c` = `contact_plane` (`N.x/N.y/N.z` at
|
||||
`+0/+4/+8`, `D` at `+0xc`), `this+0x2a4` = `contact_plane_is_water`,
|
||||
`this+0x29c` = `contact_plane_cell_id`, `this+0x34` → global sphere
|
||||
(`origin` `+0/+4/+8`, `radius` `+0xc`), `this+0x20` = sphere path.
|
||||
|
||||
**All four x87 flag tests decoded** (the contract's count is right; here is
|
||||
each one, with the FPU condition-code reasoning — `fcom*` sets C0=1 for
|
||||
"ST0 < src", C3=1 for equal, C2=1 for unordered; `fnstsw ax` puts C0 at bit 0
|
||||
of `ah`, C2 at bit 2, C3 at bit 6):
|
||||
|
||||
| # | Address | Instruction | Meaning | acdream |
|
||||
|---|---|---|---|---|
|
||||
| 1 | `0050a3c4` | `test ah,1` after `fcomp [0x795344]` | C0 alone → `slidingAngle < 0` → `checkSlide = true`; else `sliding_normal_valid = 0` | `TransitionTypes.cs:5192-5199` ✅ |
|
||||
| 2 | `0050a502` | `test ah,0x41` / `0050a505 jne 0x50a515` | C0\|C3 → `collisionAngle <= 0` → subtract arm; fall through (`> 0`) → `call 0x509c50` | `:5246-5258` — **diverges, AD-65** |
|
||||
| 3 | `0050a5d3` | `test ah,5` + `jp 0x50a6a8` after `fcompp` | PF set ⇔ C0=C2 ⇔ (ordered) C0=0 ⇔ `dist >= radius - 0.0002` → skip push-out | `:5297` — **diverges in the threshold, AD-66** |
|
||||
| 4 | `0050a5ed` | `test ah,0x41` / `jne 0x50a6a4` | C0\|C3 → `radius <= abs(zDist)` → skip | `:5302` `radius > MathF.Abs(zDist)` ✅ |
|
||||
|
||||
Everything else lines up instruction-for-instruction:
|
||||
|
||||
- **No-contact-plane early return** (`0050a61c`): if `!checkSlide`, returns the
|
||||
offset untouched and jumps past the safety block; else
|
||||
`offset -= sliding_normal * slidingAngle` (`0050a624-0050a69e`) and still
|
||||
skips the safety block. acdream `:5202-5223` ✅ including the skip.
|
||||
- **Crease order.** `0050a412-0050a458` builds
|
||||
`cross(contact_plane.N, sliding_normal)` in that operand order
|
||||
(`[esp+0x14] = sn.z*cpn.y - sn.y*cpn.z` = `cross(cpn, sn).x`). acdream
|
||||
`:5228 Vector3.Cross(ci.ContactPlane.Normal, ci.SlidingNormal)` ✅.
|
||||
- **Degenerate-crease guard.** `0050a45c call 0x452460` =
|
||||
`AC1Legacy::Vector3::normalize_check_small`; disassembled at `0x00452460` it
|
||||
computes `len = sqrt(x²+y²+z²)`, compares against `[0x79b6ac]` = **0.0002f**
|
||||
(raw `17b75139`), returns 1 (and leaves the vector alone) when `len <` that,
|
||||
otherwise normalizes and returns 0. Retail then zeroes the offset on
|
||||
return-1. acdream `:5233-5243` computes the length, compares
|
||||
`slideLen < PhysicsGlobals.EPSILON` (= 0.0002), zeroes on true, else divides
|
||||
by the length and dots — algebraically identical, same threshold, same
|
||||
constant ✅.
|
||||
- **Safety block gating** `contact_plane_is_water == 0 && contact_plane_cell_id != 0`
|
||||
(`0050a569`, `0050a577`) ✅ `:5285`. Retail additionally converts through
|
||||
`LandDefs::get_block_offset` (`0x0043E630`, called at `0050a592`) because its
|
||||
contact plane is cell-local; acdream's sphere is already global — a
|
||||
representational difference, not a behavioural one.
|
||||
- **Push-out vector** `(0, 0, zDist)` and `SPHEREPATH::add_offset_to_check_pos`
|
||||
(`0x00509D10`, called at `0050a612` with `ecx = this+0x20`) ✅ `:5304`.
|
||||
|
||||
Cross-check: **ACE agrees with retail on both divergent points**
|
||||
(`references/ACE/Source/ACE.Server/Physics/Transition.cs:60-88` calls
|
||||
`ContactPlane.SnapToPlane(ref offset)` on the `else` arm and uses the bare
|
||||
`globSphere.Radius` in both the trigger and the numerator). So acdream is the
|
||||
outlier against both oracles, which is what AD-65/AD-66 now record.
|
||||
|
||||
### 1.2 Does retail project against `collision_info.contact_plane`, and who produces it?
|
||||
|
||||
**Yes, and the producer set is broader than claimed — which strengthens rather
|
||||
than weakens the argument.**
|
||||
|
||||
`adjust_offset` reads `this->collision_info.contact_plane` at `edi+0x28c`
|
||||
gated on `contact_plane_valid` at `edi+0x288`. It is called **per sub-step**
|
||||
from inside the step loop of `CTransition::find_transitional_position`
|
||||
(`0x0050BDF0`, call at `0050bf66`, immediately followed by the
|
||||
`WalkInterp = (i+1)/numSteps` computation at `0050bfc2-0050bfe9` that identifies
|
||||
the loop body); the only other caller is `CTransition::find_placement_pos`
|
||||
(`0x0050BA50`, call at `0050bcfd`). Two call sites total, byte-scanned across
|
||||
the whole `.text` for `E8` rel32 targets.
|
||||
|
||||
Byte-scanning `.text` for calls to `COLLISIONINFO::set_contact_plane`
|
||||
(`0x00509D80`) gives **nine** producers:
|
||||
|
||||
```
|
||||
0050ab37 CTransition::validate_transition +0xc7
|
||||
0050acca CTransition::validate_transition +0x25a
|
||||
0050d1c7 OBJECTINFO::validate_walkable +0x1b7
|
||||
0050d2e1 OBJECTINFO::validate_walkable +0x2d1
|
||||
00536ecf CSphere::step_sphere_down +0x1af
|
||||
00537db1 CSphere::intersects_sphere +0x331
|
||||
0053a5ee BSPTREE::find_collisions +0x1ae
|
||||
0053aae2 CCylSphere::step_sphere_down +0x132
|
||||
0053b6b9 CCylSphere::intersects_sphere +0x279
|
||||
```
|
||||
|
||||
`BSPTREE::find_collisions` and both `step_sphere_down` variants are on the
|
||||
list, so the claim that retail's contact plane natively carries building and
|
||||
EnvCell geometry — the gap a terrain-only XY sample structurally cannot cover —
|
||||
**is confirmed**. Two nits, neither material: `BSPTREE::step_sphere_down`
|
||||
(`0x0053A210`) is *not* itself a setter (it reaches the plane through the
|
||||
`CSphere`/`CCylSphere` pair), and the claim omits `intersects_sphere` ×2 and
|
||||
the `validate_*` pair.
|
||||
|
||||
Corroborating the other half of the argument — that retail has **no** pre-sweep
|
||||
projection: `CPhysicsObj::UpdatePositionInternal` (`0x00512C30`, pc:280817) is
|
||||
`CPartArray::Update` → root-frame scale gated on `transient_state & 2`
|
||||
(`0x00512CA1`; `× m_scale` when set, `× 0` when clear) →
|
||||
`PositionManager::adjust_offset` → `Frame::combine` →
|
||||
`UpdatePhysicsInternal` → `process_hooks`. No plane, no normal, no dot product
|
||||
anywhere before the sweep. ✅
|
||||
|
||||
### 1.3 Constants
|
||||
|
||||
Read as raw bytes from `.rdata` at the stated VAs:
|
||||
|
||||
| VA | Bytes | Value |
|
||||
|---|---|---|
|
||||
| `0x00795344` | `00000000` | `0.0f` ✅ |
|
||||
| `0x007c6878` | `17b75139` | `0.00019999999494757503f` ✅ |
|
||||
| `0x0079b6ac` | `17b75139` | same 0.0002f (the `normalize_check_small` threshold) |
|
||||
| `0x007928c0` | `000000000000f03f` | `1.0` (double, `snap_to_plane`'s reciprocal numerator) |
|
||||
|
||||
### 1.4 AD-65 — the `snap_to_plane` substitution
|
||||
|
||||
**Disassembly claim: CONFIRMED exactly. Trigonometric formula: CONFIRMED.
|
||||
Percentages: WRONG — see finding F1.**
|
||||
|
||||
`0050a4fa fcomp [0x795344]` / `0050a502 test ah,0x41` / `0050a505 jne 0x50a515`
|
||||
are byte-for-byte as the row states, and the FPU reasoning is right: `jne`
|
||||
takes the SUBTRACT arm at `0x50a515` (which is literally
|
||||
`result -= N * collisionAngle`, spelled out at `0050a515-0050a565`) when
|
||||
`cAngle <= 0`, and falls through to `call 0x509c50` when `cAngle > 0`.
|
||||
`0x00509C50` is `Plane::snap_to_plane` per `symbols.json`, and disassembling it
|
||||
gives, after an early return when `|N.z| <= 0.0002`:
|
||||
|
||||
```
|
||||
v.z = -(v.x*N.x + v.y*N.y) / N.z ; v.x and v.y are never written
|
||||
```
|
||||
|
||||
(the `+D` and `-D` terms at `0050a58f`/`0050c996` cancel exactly; ACE's
|
||||
`PlaneExtensions.SnapToPlane` writes the same cancelling pair). acdream's
|
||||
`else` arm at `TransitionTypes.cs:5252-5258` is instead
|
||||
`result -= N * collisionAngle`, identical to the `if` arm — the row's core
|
||||
claim, confirmed.
|
||||
|
||||
Geometry, re-derived independently. Slope descending along +X at angle θ has
|
||||
outward normal `N = (sinθ, 0, cosθ)`. For a horizontal step `v = (d, 0, 0)`
|
||||
downhill, `v·N = d·sinθ > 0` → the `snap_to_plane` arm:
|
||||
|
||||
- retail: `(d, 0, -d·tanθ)` — **XY preserved at `d`**, along-plane speed `d/cosθ`;
|
||||
- acdream: `(d·cos²θ, 0, -d·sinθ·cosθ)` — **XY = `d·cos²θ`**, along-plane speed
|
||||
`d·cosθ`.
|
||||
|
||||
So the XY ratio is `cos²θ`. The row's *formula* is right.
|
||||
|
||||
### 1.5 AD-66 — the safety push-out threshold
|
||||
|
||||
**CONFIRMED, all four operand loads, verbatim.**
|
||||
|
||||
```
|
||||
0050a5c4 d9410c fld dword ptr [ecx+0xc] ; bare radius
|
||||
0050a5c7 d82578687c00 fsub dword ptr [0x7c6878] ; - 0.0002f
|
||||
0050a5cd d9c1 fld st(1) ; dist
|
||||
0050a5cf ded9 fcompp ; dist vs radius - eps
|
||||
...
|
||||
0050a5dc d8690c fsubr dword ptr [ecx+0xc] ; radius - dist (bare radius again)
|
||||
0050a5df d87608 fdiv dword ptr [esi+8] ; / contact_plane.N.z
|
||||
```
|
||||
|
||||
Neither site multiplies by `N.z`. acdream substitutes
|
||||
`naturalRestingDist = radius * ci.ContactPlane.Normal.Z` in **both** places
|
||||
(`TransitionTypes.cs:5295` and `:5301`), exactly as the row says, and the row is
|
||||
right that ACE has retail's form too — so "ACE and the published pseudocode
|
||||
have the original threshold" in the code comment does understate it. Filing an
|
||||
argued-but-unrecorded deviation is correct register hygiene.
|
||||
|
||||
### 1.6 Address / anchor precision audit
|
||||
|
||||
Every cited address re-checked as the construct claimed (this is the class of
|
||||
error that produced AP-150's mis-cite):
|
||||
|
||||
| Citation | Verified |
|
||||
|---|---|
|
||||
| `CTransition::adjust_offset` `0x0050a370` | ✅ symbol + `ret 8` at `0x0050a6c7` |
|
||||
| `Plane::snap_to_plane` `0x00509c50` | ✅ symbol; body is the z-only solve |
|
||||
| `0050a4fa` / `0050a502` / `0050a505` | ✅ exact instructions as quoted |
|
||||
| `0050a5c4` / `0050a5c7` / `0050a5dc` / `0050a5df` | ✅ exact instructions as quoted |
|
||||
| `0x795344 = 0.0f`, `0x7c6878 = 0.0002f` | ✅ raw bytes as quoted |
|
||||
| `CTransition::find_transitional_position` `0x0050bdf0` | ✅ symbol; per-step call at `0050bf66` |
|
||||
| pc:272271–272393 | ✅ 272271 is the `adjust_offset` signature line, 272393 the closing `}` — **exact** |
|
||||
| pc:271852 (`snap_to_plane`) | ✅ exact signature line |
|
||||
| old anchor pc:272296–272346 "truncated" | ✅ 272296 = `float __return_1;` (after the sliding-normal gate closes at 272292); 272346 = `if (contact_plane_is_water == 0)` (the safety block's first line). Both truncations real. |
|
||||
|
||||
AD-65's methodological claim — that Binary Ninja "cannot be read for branch
|
||||
direction" — is accurate and correctly narrow. BN *does* render the
|
||||
`snap_to_plane` call plainly (pc:272322) and gets the arm order structurally
|
||||
right; what it cannot express is which FPU condition bits `test ah,0x41`
|
||||
selects, which it emits as
|
||||
`(*(uint8_t*)((char*)eax_4)[1] & 0x41) != 0`. The disassembly was necessary and
|
||||
the row does not overclaim.
|
||||
|
||||
---
|
||||
|
||||
## 2. Is the deletion itself retail-faithful?
|
||||
|
||||
Yes, and more strongly than the commit argues.
|
||||
|
||||
- The sweep genuinely runs for remotes:
|
||||
`RuntimeRemotePhysicsUpdater.cs:414` calls
|
||||
`_physics.Engine.ResolveWithTransition(preIntegratePos, postIntegratePos, …, body: rm.Body, …)`
|
||||
and `:457` assigns `rm.Body.Position = resolveResult.Position` unconditionally,
|
||||
so the sweep is authoritative over the composed root motion. The old row's
|
||||
justification ("remote bodies don't run a full local transition sweep") was
|
||||
indeed false, and the retirement row says so.
|
||||
- `PhysicsEngine.SampleTerrainWalkable` (`PhysicsEngine.cs:1023`) is a pure
|
||||
`(worldX, worldY)` landblock scan — Z-blind, cell-blind, statics-blind. The
|
||||
"wrong surface on a bridge/roof/dungeon ramp" claim is structural, not
|
||||
rhetorical.
|
||||
- Retail has no pre-sweep projection (§1.2). Deleting one is the retail
|
||||
direction regardless of what the measurement had shown.
|
||||
- The composition-idempotence argument holds: after `v -= N·(v·N)`,
|
||||
`dot(v, N) == 0`, so a second projection against the same plane is a no-op —
|
||||
which is why the trajectory came out bit-identical, and why the deletion
|
||||
cannot regress the same-plane (terrain) case.
|
||||
|
||||
Residue check: no production caller of `RemoteMotionCombiner.ComputeOffset`
|
||||
remains (tests only), `SampleTerrainNormal` survives only in comments, and the
|
||||
parameter removal makes a one-site regression a compile error as claimed.
|
||||
|
||||
---
|
||||
|
||||
## 3. Findings
|
||||
|
||||
### F1 — MEDIUM. AD-65's percentages are wrong by ~2×, and contradict its own formula and the project's own measurement
|
||||
|
||||
`docs/architecture/retail-divergence-register.md`, row AD-65:
|
||||
|
||||
> acdream therefore descends slopes SLOWER than retail by cos^2(theta) in XY:
|
||||
> **13% slow at 30 degrees, 29% at 45 degrees**.
|
||||
|
||||
`cos²(30°) = 0.750` → **25% slow**. `cos²(45°) = 0.500` → **50% slow**. The
|
||||
quoted figures are `1 − cos θ` (13.4% and 29.3%), not `1 − cos²θ`; the row
|
||||
states the correct factor and then quantifies a different one.
|
||||
|
||||
This is not a matter of interpretation — the same push measured it. **#331**
|
||||
(`docs/ISSUES.md`, added in `2223ed17`) records a probe on the gradient-0.6
|
||||
ramp (θ = 30.96°): `the XY advance is 0.0735 m for a 0.1 m request`. That is
|
||||
`cos²(30.96°) = 0.7353`, i.e. **26.5% slow** — a direct empirical refutation of
|
||||
"13% at 30 degrees", sitting in a neighbouring file in the same commit series.
|
||||
|
||||
Consequence: AD-65 is filed as a **lead for #269**, and the number is exactly
|
||||
what a future reader will weigh when deciding whether the lead is worth
|
||||
chasing. Halving the magnitude makes a 50%-at-45° downhill speed loss look like
|
||||
a rounding-error feel issue. **Fix the two percentages to 25% and 50% before
|
||||
this row is used for anything.**
|
||||
|
||||
- Retail: `CTransition::adjust_offset` `0x0050a370`, arm select at `0050a505`;
|
||||
`Plane::snap_to_plane` `0x00509c50`.
|
||||
- acdream: `src/AcDream.Core/Physics/TransitionTypes.cs:5252-5258`.
|
||||
- Observable in game: a player or remote running downhill on a 45° face covers
|
||||
half the ground per second that retail does (25% less at 30°); on the same
|
||||
input the body's along-plane speed is `d·cosθ` where retail's is `d/cosθ`.
|
||||
Uphill is correct.
|
||||
|
||||
### F2 — MEDIUM. "Verbatim / faithful port" is asserted about `Transition.AdjustOffset` in five places, and is false as of the very next commit
|
||||
|
||||
The change repeatedly certifies the port it is standing on:
|
||||
|
||||
- `src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs:276` and `:321` —
|
||||
"**ported verbatim** in `Transition.AdjustOffset`";
|
||||
- `src/AcDream.Core/Physics/RemoteMotionCombiner.cs:46` — "acdream ports that
|
||||
**verbatim**";
|
||||
- `tests/AcDream.Runtime.Tests/Physics/RuntimeRemoteSlopeProjectionTests.cs:11` —
|
||||
"acdream ports that **faithfully**";
|
||||
- the retired **AD-10** register row — "runs acdream's **verbatim port** of
|
||||
`CTransition::adjust_offset` once per sub-step";
|
||||
- `fe6ee877`'s commit message — same phrase.
|
||||
|
||||
One commit later, `fb454b74` files **two** divergences inside that exact
|
||||
function. The register now simultaneously asserts that `AdjustOffset` is a
|
||||
verbatim port and that it substitutes a different operation on one of its three
|
||||
arms and a different constant in its safety block. A future maintainer grepping
|
||||
"verbatim" for a trustworthy reference implementation will be misled, and the
|
||||
retired AD-10 row — the permanent record — is the worst place for it.
|
||||
|
||||
Not a code defect; the deletion argument survives untouched (see F3 for why it
|
||||
in fact survives *because* of AD-65). But four comments and one register row
|
||||
should read "ported apart from AD-65 and AD-66" or cite them inline.
|
||||
|
||||
### F3 — LOW/MEDIUM. The "measured redundant" evidence is contingent on AD-65, and the register does not say so
|
||||
|
||||
The bit-identical measurement is a consequence of the divergence filed the same
|
||||
day. acdream's away-plane arm is `v -= N·(v·N)` — *the same operation as the
|
||||
deleted pre-sweep projection* — so the composition is idempotent and deleting
|
||||
one changes nothing. If AD-65 is ever fixed, the away-plane arm becomes
|
||||
`snap_to_plane`, which **preserves** XY; a surviving pre-sweep projection would
|
||||
then have shrunk XY to `cos²θ` before `snap_to_plane` locked it in, and
|
||||
`snap_to_plane` (being a no-op on an already-on-plane vector) would have had no
|
||||
way to recover it.
|
||||
|
||||
So the deletion is not merely safe — it is a **prerequisite** for AD-65's
|
||||
eventual fix, and the ordering (delete first, then fix AD-65) is the right one.
|
||||
That is a point in the change's favour, and it is missing from the record. The
|
||||
retired row reads "measured redundant" flat, which invites a future reader to
|
||||
conclude the pre-sweep layer was always a no-op on principle. Recommend one
|
||||
sentence in AD-65 and/or the retired AD-10 row noting the interaction.
|
||||
|
||||
Related, minor: the surviving measurement is **downhill-only**, because #331
|
||||
made the uphill counterpart vacuous. The commit message and #331 are both
|
||||
candid about this; the register row is not, and simply says "30 ticks down a
|
||||
31-degree ramp" — which is accurate but reads as a choice rather than as a
|
||||
constraint. No action required beyond awareness.
|
||||
|
||||
### F4 — LOW. Malformed XML doc on `ComposeOffset` will hide the AD-10 note from tooling
|
||||
|
||||
`src/AcDream.Core/Physics/RemoteMotionCombiner.cs:34-53`: the summary closes at
|
||||
line 38, the AD-10 `<para>` block sits at document top level from line 40, and a
|
||||
second, unmatched `</summary>` appears at line 52. The project sets no
|
||||
`GenerateDocumentationFile`, so the compiler is silent — but IDE tooltips and
|
||||
any generated docs will show only the first paragraph and drop the retirement
|
||||
note, which is precisely the note a future maintainer of this method needs.
|
||||
Move the `<para>` inside the first `</summary>` and delete the second.
|
||||
|
||||
### F5 — LOW. AD-65 and AD-66 cite files without lines
|
||||
|
||||
Both new rows name `src/AcDream.Core/Physics/TransitionTypes.cs` with a prose
|
||||
description of the block. Adjacent rows (AD-5, AD-12, AD-13, AD-62, AD-64) give
|
||||
`file:line`. The exact anchors are `:5252-5258` (AD-65's `else` arm) and
|
||||
`:5285-5310` (AD-66's safety block). In a 5,000-line file this matters.
|
||||
|
||||
### F6 — informational. A foreign untracked file appeared in the worktree mid-review and must not be committed
|
||||
|
||||
`tests/AcDream.Runtime.Tests/Physics/ZzReviewTrajectoryDump.cs` (self-described
|
||||
"TEMPORARY reviewer probe (2026-08-06 AD-10 architecture review). Delete.",
|
||||
13 test attributes) was **not** present at the start of this review and
|
||||
appeared at 09:49 during it — a concurrent review session writing into the same
|
||||
worktree. It was compiled into the Release build and inflated the raw suite
|
||||
count by 12. It is not part of this change and must be deleted before merge.
|
||||
|
||||
---
|
||||
|
||||
## 4. Bookkeeping audit
|
||||
|
||||
**AD-10's retirement is earned by the code, not asserted.** Positive evidence:
|
||||
`SampleTerrainNormal` is absent from the freshly built `AcDream.Core.dll`
|
||||
image; the `terrainNormal` parameter is gone from both `ComposeOffset` and
|
||||
`ComputeOffset`; both `RuntimeRemotePhysicsUpdater` fork branches are cleared;
|
||||
the two Core tests are deleted; nothing but comments references the removed
|
||||
API. The row follows house style — `~~AD-10~~`, bold **RETIRED 2026-08-06 by
|
||||
deletion**, past tense, evidence inline, `—` in the justification/risk columns,
|
||||
retail anchor retained and corrected — matching the ~~AD-6~~ / ~~AD-11~~
|
||||
precedent. Removing the parameter rather than defaulting it to `null` is the
|
||||
right call and does make a one-site regression a compile error.
|
||||
|
||||
Three stale claims in the old row are recorded rather than dropped
|
||||
(the false "remotes don't run the sweep" justification, the
|
||||
"interpolation-active" mis-description of `if (!interpolationOverwrote`, and the
|
||||
roof clause dead since Bug B gated on `OnWalkable`). All three check out
|
||||
against the code.
|
||||
|
||||
**AD-65 and AD-66 are honestly scoped.** AD-65 explicitly says "Not justified —
|
||||
this is an unexamined substitution, not a decision", and explains why it is
|
||||
filed rather than fixed (it changes local-player feel and needs its own visual
|
||||
gate; folding it into a remote-movement change would put a local-player
|
||||
regression behind the wrong acceptance test). That reasoning is correct.
|
||||
AD-66 is scrupulous in the other direction: "Filed to make the deviation
|
||||
auditable, not to assert it is wrong", and it names the failure mode in both
|
||||
directions. Neither row claims more than the disassembly supports — except for
|
||||
F1's numbers.
|
||||
|
||||
**AD-65 is correctly recorded as a lead, not a diagnosis**, in bold, with
|
||||
"nothing here establishes causation" and "#269 still needs its live cdb A/B".
|
||||
It does **not** overreach into the exonerated area: it states explicitly that
|
||||
"#269's friction and jump chains are byte-exonerated and must not be
|
||||
re-audited; `adjust_offset` is a different function and is not covered by that
|
||||
do-not-retry". That is the correct boundary — `adjust_offset` was never part of
|
||||
the friction/jump byte-verification — and the row draws it itself rather than
|
||||
leaving a future reader to.
|
||||
|
||||
**The #32 corrections are accurate.** Both edited paragraphs were checked
|
||||
against the code:
|
||||
|
||||
- The "even a corrected `OnWalkable` would need a real contact-plane-derived
|
||||
slide" paragraph is correctly marked superseded with an inverted premise —
|
||||
remotes do run the sweep, and on a steep roof `bodyOnWalkableAtTickStart` was
|
||||
false anyway, so the deleted sample never ran on #32's geometry and the roof
|
||||
slide was already contact-plane driven.
|
||||
- The dependency paragraph is correctly struck and replaced with "discharged
|
||||
rather than merely gated".
|
||||
- **The "does not fix #32, and does not partially fix it" claim holds**, and
|
||||
the commit message states it in exactly those words. #32's remote half closed
|
||||
at `204d0ae0`; nothing here touches the local-player edge-slide or the three
|
||||
recorded gaps. The genuine improvement claimed — a remote on a walkable
|
||||
*non-terrain* surface now gets its own committed contact plane rather than
|
||||
the plane of the ground far below — follows directly from
|
||||
`SampleTerrainWalkable` being XY-only, and is correctly described as a case
|
||||
#32 never covered rather than as progress on #32.
|
||||
|
||||
**#331 and #332 are honest filings.** #331 records a probe result that
|
||||
*invalidated a test that had passed*, states severity UNKNOWN on purpose,
|
||||
lists five ruled-out hypotheses with the probe evidence for each, and names the
|
||||
single comparison that decides severity. #332 is scoped as an observation with
|
||||
an instantiation census rather than an inference, and records the reasoning
|
||||
trap (assembly placement ≠ reachability) that would otherwise have produced a
|
||||
vacuous headless gate. Both explicitly disclaim causation by AD-10, correctly.
|
||||
|
||||
**Test-count reconciliation reproduces exactly.** Raw full-suite result on the
|
||||
clean Release build was 11,208 passed / 4 skipped / 0 failed; the foreign probe
|
||||
file (F6) contributes 12 executed tests
|
||||
(`--filter FullyQualifiedName!~ZzReviewTrajectoryDump` → Runtime 1,232 → 1,220).
|
||||
**11,208 − 12 = 11,196 passed / 4 skipped / 0 failed**, matching `886333a2`'s
|
||||
claim to the test. The `+3 Runtime / −2 Core` arithmetic is structurally
|
||||
verified too: `RuntimeRemoteSlopeProjectionTests` carries exactly three
|
||||
`[Fact]`s and the diff deletes exactly two Core tests.
|
||||
|
||||
**Test honesty.** `RuntimeRemoteSlopeProjectionTests` states in its own doc
|
||||
comment that short-circuiting `Transition.AdjustOffset` leaves it GREEN,
|
||||
names the reason (`ValidateWalkable`'s push-out re-seats the sphere every
|
||||
sub-step), and forbids citing itself as a unit test of `adjust_offset`. The
|
||||
anti-vacuity guard (`dz < -1.0 m`) and the per-tick rather than start/end
|
||||
assertion are both the right shape. Expected values come from the fixture's own
|
||||
`TerrainSurface.SampleZ`, i.e. from geometry, not from a re-implementation of
|
||||
the projection formula — the weakness the two deleted Core tests had.
|
||||
|
||||
---
|
||||
|
||||
## 5. What was checked, so the PASS is auditable
|
||||
|
||||
- PDB/EXE pairing (`check_exe_pdb.py` → MATCH) before any address work.
|
||||
- Full disassembly of `CTransition::adjust_offset` `0x0050a370-0x0050a6c7`,
|
||||
arm by arm, against `TransitionTypes.cs:5180-5322` line by line.
|
||||
- All four x87 condition-code tests decoded from `fnstsw`/`test ah` semantics.
|
||||
- `Plane::snap_to_plane` `0x00509c50` and
|
||||
`Vector3::normalize_check_small` `0x00452460` disassembled in full.
|
||||
- Four float/double constants read as raw bytes from `.rdata`.
|
||||
- Whole-`.text` `E8 rel32` scan for callers of `set_contact_plane`
|
||||
(9 producers) and of `adjust_offset` (2 call sites), symbolised against
|
||||
`symbols.json`.
|
||||
- `CPhysicsObj::UpdatePositionInternal` `0x00512C30` read end to end to confirm
|
||||
retail has no pre-sweep projection.
|
||||
- All pc: anchors checked by line number in
|
||||
`named-retail/acclient_2013_pseudo_c.txt`, including the old truncated one.
|
||||
- ACE cross-check (`Transition.cs`, `PlaneExtensions.cs`) on both divergences.
|
||||
- `RemoteMotionCombiner`, `RuntimeRemotePhysicsUpdater`,
|
||||
`PhysicsEngine.SampleTerrainWalkable`, `RemoteRampHarness`,
|
||||
`RuntimeRemoteSlopeProjectionTests` read in full.
|
||||
- Orphan/residue greps for `ComputeOffset`, `SampleTerrainNormal`,
|
||||
`terrainNormal`.
|
||||
- 44 `bin`/`obj` directories deleted; clean Release build (0 errors); full
|
||||
suite (0 failures); staleness disproved by byte-searching the built DLLs.
|
||||
|
||||
**No third retail divergence was found in `AdjustOffset`, no mis-cited address
|
||||
was found, and no claim in the four commits was found to be unsupported by the
|
||||
binary — apart from AD-65's two percentage figures (F1).**
|
||||
|
|
@ -35,7 +35,6 @@ public sealed class RemoteMotionCombiner
|
|||
/// Compose retail's complete per-object delta frame. Interpolation, when
|
||||
/// active, replaces the PartArray frame via
|
||||
/// <c>Position::subtract2</c>; otherwise the authored root frame remains.
|
||||
/// </summary>
|
||||
///
|
||||
/// <para><b>AD-10, retired 2026-08-06.</b> This method used to accept a
|
||||
/// <c>terrainNormal</c> and project the composed world-space root motion
|
||||
|
|
@ -43,8 +42,9 @@ public sealed class RemoteMotionCombiner
|
|||
/// projection: retail projects the per-sub-step offset onto
|
||||
/// <c>collision_info.contact_plane</c> INSIDE the sweep
|
||||
/// (<c>CTransition::adjust_offset</c> <c>0x0050a370</c>,
|
||||
/// pc:272271-272393), acdream ports that verbatim in
|
||||
/// <c>Transition.AdjustOffset</c>, and remote bodies do run that sweep.
|
||||
/// pc:272271-272393), acdream ports that in
|
||||
/// <c>Transition.AdjustOffset</c> (structurally exact, with exactly two
|
||||
/// filed divergences — AD-65 and AD-66), and remote bodies do run that sweep.
|
||||
/// The extra copy also sampled the wrong surface — a single-point
|
||||
/// XY-only terrain lookup, blind to buildings, EnvCells and statics — so
|
||||
/// on a walkable NON-terrain surface it applied the plane of the ground
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue