feat(physics): C4 route 4a — remote steady-state Position through the seam
Routes the classifier's two NO-PLACEMENT remote branches — Interpolate (contact, PlayerDistance < 96 m) and NoPositionOperation (no contact) — through a Runtime-owned seam, and fixes the two divergences they carried. Teleport, far-snap and cell-less stay on the legacy App path; 4b owns them. Route 4 was split into 4a/4b after scoping put the whole route at 1,500-2,500 lines against a ~400 budget. 4a's branches perform no SetPosition, so this slice carries no deferred-cell park, no service-window guard and no allocation exposure — which is what made the split worth doing. Divergences fixed, both previously unfiled: * D1 — the NPC airborne branch hard-snapped Body.Position/Orientation and branched on the client-tracked rmState.Airborne, never consulting the wire IsGrounded bit. Retail's MoveOrTeleport @0x00516330 returns 0 at 0x0051636D and writes nothing. Player remotes were already correct; NPCs were not. * D2 — ConstrainTo was armed before the operation, unconditionally, so it fired on the airborne no-op retail skips and anchored to the PRE-move position. Retail arms it at 0x00454272, only when MoveOrTeleport returns nonzero, anchored to &arg2->m_position read live, i.e. post-move. AP-87 and TS-44 were carried deliberately, not delegated away. AP-87's three conditions — including firstUp, which one round silently dropped — are preserved as an explicit acdream policy layer applied AFTER the classifier commits to Interpolate; the two previously separate player/NPC copies are now one. TS-44 stays an NPC-only caller gate; extending sticky suppression to player remotes has no retail basis and no live evidence, so it was declined rather than absorbed. Landing is explicitly carved out of 4a's ownership on both arms. A landing packet classifies Interpolate, so an ordering slip would ENQUEUE a body that must PLANT and a creature knocked off a ledge would glide down over a packet interval. The carve-out is a named entry point returning AirborneSnap/SteadyStateInterpolate/ Legacy precisely so the PRECEDENCE is observable and testable rather than implied by statement order — that is how the slip happened once and was caught. The player/NPC asymmetry on landing is real and NOT resolved here: retail draws no such distinction, but converging them is a behaviour decision needing its own evidence. Filed into the 4b plan. Register: AP-135 filed for the two bookkeeping writes the airborne branch deliberately retains (rmState.CellId, LastServerPos/Time) — not retail's model, but load-bearing for our catch-up sweep and staleness timer, and verified not to be a canonical cell commit for ordinary remotes. AP-87 and TS-44 rewritten to describe the code. Honest remainder: App still owns branch selection, the airborne return, the cell write, the entity write and the shadow publish, and headless satisfies "both hosts drive the identical entry point" only vacuously since it returns early for remotes. That is written into the 4b bullet rather than left implicit. Cost: 364 non-comment production lines, 91% of the ~400 budget — the split did isolate the cheap half, but not by much. Do not carry "well under" into 4b's scoping. Gates: complete Release solution 10,938 passed / 4 skipped / 0 failed (pre-4a baseline 10,909). Four review rounds; the first three each introduced a new behavioural defect while fixing another, and each left a comment asserting behaviour that no longer matched — the final round's precedence matrix was traced cell-by-cell against HEAD with only the D1-intended difference. App tests call production entry points against a real WorldEntity and real classifier output, closing route 2's #292 gap rather than repeating it. Connected acceptance NOT run — needs a live second character. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
19d9509497
commit
44830a0eb3
12 changed files with 1854 additions and 150 deletions
|
|
@ -81,11 +81,24 @@ a nonzero return, anchored post-move. No register row exists.
|
|||
|
||||
Only the parts reachable from the two 4a branches:
|
||||
|
||||
- The generic tail's remote writes (`entity.SetPosition` / `ParentCellId` /
|
||||
`Rotation` / `RebucketLiveEntity`) for a remote whose classification is
|
||||
`Interpolate` or `NoPositionOperation`. The committed result must reach the
|
||||
render entity through the existing placement-projection sink instead — the
|
||||
same substitution route 2 made.
|
||||
- The generic tail's remote RENDER-POSE writes (`entity.SetPosition` /
|
||||
`ParentCellId` / `Rotation`) for a remote whose classification is
|
||||
`Interpolate` or `NoPositionOperation`.
|
||||
|
||||
> **CORRECTED 2026-08-03 (review finding R1,
|
||||
> [`2026-08-03-c4-route-4a-review-findings.md`](2026-08-03-c4-route-4a-review-findings.md)).**
|
||||
> This bullet originally also listed `RebucketLiveEntity`, and said the
|
||||
> result "must reach the render entity through the existing
|
||||
> placement-projection sink instead — the same substitution route 2 made."
|
||||
> That instruction was copied from route 2 and does not transfer: route 2
|
||||
> performs a placement and therefore has a committed receipt to project,
|
||||
> while NEITHER 4a branch performs a placement, so nothing substitutes for
|
||||
> the bucket transaction. **The rebucket keeps running for both 4a
|
||||
> classifications.** It is the only site that moves an ordinary moving
|
||||
> remote's draw bucket, commits its canonical `FullCellId`, and recovers a
|
||||
> pending bucket promotion (`GpuWorldState`, the 2026-07-03 invisible-player
|
||||
> fix); deleting it produced the #184-class invisible-but-solid creature
|
||||
> through a different door.
|
||||
- The player-remote near/far routing at `:1653-1702` and the NPC copy at
|
||||
`:1826-1871` — **the near half only.** Each has its own duplicated copies of
|
||||
`MaxPhysicsDistance = 96f` and `BodySnapThreshold = 4f`; the far half stays
|
||||
|
|
@ -122,6 +135,19 @@ to the classifier is the failure mode.** Say which you chose.
|
|||
transfer.
|
||||
5. The airborne branch writes NOTHING (retail returns 0). Not the body, not the
|
||||
render entity, not the cell.
|
||||
|
||||
> **AMENDED 2026-08-03/04 (review finding R4).** As shipped, the branch
|
||||
> retains exactly two acdream bookkeeping writes — `rmState.CellId` and
|
||||
> `LastServerPos`/`LastServerPosTime` — on both arms. These are not part of
|
||||
> retail's model (retail's `MoveOrTeleport` has no catch-up sweep and no
|
||||
> staleness timer to keep alive), and dropping them would break the
|
||||
> per-tick free-fall sweep and the staleness timer respectively. Verified
|
||||
> NOT a canonical cell commit for ordinary remotes: `RemoteMotion.CellId`
|
||||
> only delegates to `RuntimePhysicsState` when `_canonicalCellWriter` is
|
||||
> bound, which is projectiles-only, and projectiles return earlier.
|
||||
> Recorded as register row **AP-135**. Everything else the branch used to
|
||||
> do — body pose, queue, leash, render entity, shadow publish, and the
|
||||
> velocity-derived animation cycle — is genuinely skipped.
|
||||
6. `ConstrainTo` moves to after the operation, anchored post-move, and does not
|
||||
run on the airborne branch.
|
||||
7. No behaviour change to the far, teleport, or cell-less branches.
|
||||
|
|
@ -145,6 +171,16 @@ to the classifier is the failure mode.** Say which you chose.
|
|||
AP-87 signature). Then pull a drudge, let it chase, melee it, let it die.
|
||||
- Divergence ledger: D1 and D2 retired by fix in the same commit. AP-131 stays.
|
||||
AP-87 / TS-44 either stay with justification or are retired with evidence.
|
||||
- **Added 2026-08-03 (review finding R15).** D1 makes ACE's wire `IsGrounded`
|
||||
newly load-bearing for NPC remotes: ACE emits it from
|
||||
`TransientState & OnWalkable` (`PositionPack.cs:73`), while acdream's NPC
|
||||
free-fall was gated on the client-tracked `rmState.Airborne` (set only by
|
||||
`0xF74E` VectorUpdate or `!Body.OnWalkable`, never from the wire bit). A
|
||||
creature ACE reports as not-in-contact while the client believes it grounded
|
||||
now receives NO correction where the legacy routing pulled it. **Push a
|
||||
monster off a ledge / pull one down a cliff**, and confirm it falls and
|
||||
lands without hovering, without a mid-air correction, and without an
|
||||
invisible-but-solid body.
|
||||
|
||||
## Budget
|
||||
|
||||
|
|
|
|||
269
docs/research/2026-08-03-c4-route-4a-review-findings.md
Normal file
269
docs/research/2026-08-03-c4-route-4a-review-findings.md
Normal file
|
|
@ -0,0 +1,269 @@
|
|||
# C4 route 4a — dual review findings, and a contract correction (2026-08-03)
|
||||
|
||||
Both mandated reviews returned **FAIL**. Nothing is committed. This supersedes
|
||||
the 4a contract where they disagree, and **corrects an error in that contract
|
||||
that caused one of the findings.**
|
||||
|
||||
Reviews: retail-conformance (Opus) and adversarial/architecture (Opus), run
|
||||
independently against the same uncommitted diff. Suite was green at 10,917/4/0,
|
||||
which again proves nothing.
|
||||
|
||||
## Verified correct — do not churn
|
||||
|
||||
- The Runtime seam `RuntimeRemoteSteadyStatePosition.cs` itself: pure,
|
||||
stateless, correctly decomp-anchored, genuinely well tested. Both reviewers
|
||||
passed it standalone.
|
||||
- Retail truth re-verified independently: `MoveOrTeleport` @0x00516330
|
||||
`arg4 == 0` -> `return 0` writing nothing; `player_distance < 96f` ->
|
||||
`InterpolateTo`; `ConstrainTo` @0x00454272 only inside
|
||||
`if (MoveOrTeleport(...) != 0)`, anchored post-move.
|
||||
- AP-87's two headline conditions survive with identical semantics and no
|
||||
bypass path.
|
||||
- `skipGenericPositionWrite` IS derived purely from classification (the staged-
|
||||
cutover discriminator condition is met).
|
||||
- Per-entity currency on the callee is sound; the caller re-validates
|
||||
`IsCurrentPositionAuthority` between classify and apply.
|
||||
- Budget respected: ~343 production lines against ~400.
|
||||
|
||||
---
|
||||
|
||||
## R0 — CRITICAL, and it is a defect in ALREADY-COMMITTED route 2
|
||||
|
||||
**`AcceptedPhysicsTimestamps.PreviousTeleport` is always 0 on the live Position
|
||||
path.** `InboundPhysicsStateController.cs:636` calls `Current(gate,
|
||||
teleportAdvanced: ...)` and omits the `previousTeleport` argument, which
|
||||
defaults to 0 (`:1119`). The ONLY site that populates it is `:926-941`, the
|
||||
deferred initial-create path — which is why the continuation executor is correct
|
||||
and every newer consumer is not. Verified first-hand.
|
||||
|
||||
Two live consequences:
|
||||
|
||||
**(a) Route 2, at HEAD, silently drops force corrections.**
|
||||
`LiveEntityNetworkUpdateController.cs:1199` and
|
||||
`RuntimeLiveEntitySessionController.cs:268` feed the always-zero value into the
|
||||
route-2 drive, where `ValidAcceptedAuthority` (classifier `:524-525`) requires
|
||||
`Previous == Accepted` for `ForcePosition`. For any local player whose
|
||||
TELEPORT_TS is nonzero — i.e. anyone who has portalled or recalled this session
|
||||
— the authority is rejected and the correction is dropped. The user's `@pklite`
|
||||
acceptance was genuine but narrow: that character had not teleported, so the
|
||||
stamp was still 0.
|
||||
|
||||
**(b) Route 4a misclassifies every previously-teleported remote.**
|
||||
`TeleportAdvanced` becomes `IsNewer(0, T)`, true for any `T` in `[1, 0x7FFF]`,
|
||||
so the classifier returns `SetPosition` — a disposition 4a does not own — and
|
||||
(per R3) the entity falls into the far-snap arm and hard-snaps on EVERY packet
|
||||
at 5-10 Hz. That is worse than the pre-4a behaviour it replaced, and it is
|
||||
exactly the per-packet stepping the connected gate screens for. Reachable the
|
||||
first time an observed character portals or recalls, permanently thereafter.
|
||||
|
||||
**Fix at the source**: capture `previousTeleport = gate.TeleportTimestamp`
|
||||
BEFORE `TryAcceptPositionEvent` mutates it, and pass it into `Current` — the
|
||||
shape `:926` already uses. Then audit every consumer.
|
||||
|
||||
**Commit this separately and first.** It is a shipped-code defect independent of
|
||||
4a, and it needs its own bisectable commit and its own issue.
|
||||
|
||||
---
|
||||
|
||||
## R1 — HIGH — my contract was wrong: the rebucket must NOT be deleted
|
||||
|
||||
The 4a contract listed `RebucketLiveEntity` among the generic-tail writes to
|
||||
delete, and said the result "must reach the render entity through the existing
|
||||
placement-projection sink instead." **That instruction was copied from route 2
|
||||
and does not transfer.** Route 2 performs a placement and therefore has a
|
||||
committed receipt to project. 4a's two branches perform NO placement, so there
|
||||
is no receipt and nothing substitutes for the bucket transaction. The
|
||||
implementer followed the contract precisely; the contract was wrong.
|
||||
|
||||
`skipGenericPositionWrite` currently also gates
|
||||
`_liveEntities.RebucketLiveEntity` (`:1400`), which is the ONLY site that moves
|
||||
an ordinary moving remote's spatial bucket (the nine other call sites are
|
||||
player, projectile, teleport, hydration, materialization, equipped-child and
|
||||
rescue paths; the per-tick DR writers deliberately do not rebucket — see
|
||||
`LiveEntityRuntime.cs:856-858`). So for every grounded remote inside 96 m none
|
||||
of these runs again: the GPU draw bucket, `IsSpatiallyVisible` +
|
||||
`RefreshPresentation`, `CommitRebucket` (the canonical `FullCellId`), the
|
||||
`prepare_to_enter_world` clock rebase, and `PublishProjectionVisibilityChanged`.
|
||||
|
||||
Failure: a creature chasing you across a landblock boundary keeps its body and
|
||||
collision shadow but leaves its draw bucket behind, is frustum-culled, and goes
|
||||
**invisible-but-solid** — the #184 class AP-87 exists to prevent, through a
|
||||
different door. Secondary: the stale `FullCellId` feeds back as the classifier's
|
||||
own `CommittedCellId` and as the `ConstraintDistance` cell key.
|
||||
|
||||
**Required: keep the rebucket running for both 4a classifications.** Delete only
|
||||
the three render-pose writes. Also note R12 below: the per-UP rebucket doubles
|
||||
as the pending-bucket promotion recovery (`GpuWorldState.cs:1113-1126`, the
|
||||
2026-07-03 invisible-player fix) — the cell-change-gated canonical commit does
|
||||
not cover it.
|
||||
|
||||
---
|
||||
|
||||
## R2 — HIGH — the App acceptance tests are tautologies
|
||||
|
||||
`LiveEntityNetworkRemoteSteadyStateIntegrationTests.cs:75-86` recomputes
|
||||
`skipGenericPositionWrite` in the test body, asserts it true, then places the
|
||||
mutation inside `if (!skipGenericPositionWrite)` — a branch the preceding assert
|
||||
proves unreachable. The second test writes the tail sync itself and then asserts
|
||||
its own arithmetic. **Both pass unchanged with the production file reverted to
|
||||
HEAD**, and the fixture supplies a correct `Previous/Accepted` teleport pair
|
||||
that production never produces, so the suite is structurally incapable of
|
||||
catching R0(b).
|
||||
|
||||
This is #292 again, in a WEAKER form than route 2's: a source-text pin at least
|
||||
fails when the source changes; this never observes production at all. The
|
||||
contract's acceptance bullet said "behavioural, not a source-text pin… do not
|
||||
repeat it."
|
||||
|
||||
**Required: a test that fails if the generic tail double-writes a remote.** If
|
||||
`LiveEntityNetworkUpdateController` genuinely cannot be constructed, then extract
|
||||
the decision+mutation into something that CAN be tested and have production call
|
||||
it — do not simulate production in the test body.
|
||||
|
||||
---
|
||||
|
||||
## R3 — HIGH — "not Interpolate" is treated as "far", mis-owning five dispositions
|
||||
|
||||
Player `:1723-1740`, NPC `:1867-1888` use `if (Interpolate) … else <far snap>`.
|
||||
The `else` also swallows `SetPosition` (cell-less half — `remotePlacementRequired`
|
||||
covers only the teleport half), `RejectedAuthority`, `RejectedData`, and `null`.
|
||||
So a wire quaternion failing validation, or a cell-less remote, now hard-snaps
|
||||
every packet where the legacy code did distance-based routing — a behaviour
|
||||
change to branches contract item 7 forbids touching. `ClassifyRemoteAcceptedPosition`'s
|
||||
own doc comment (`:47-50`) claims null leaves the legacy path "completely
|
||||
unchanged"; that is false.
|
||||
|
||||
**Required: test explicitly for the two dispositions 4a owns. Everything else
|
||||
falls through to untouched legacy routing.** One classification, one owner.
|
||||
|
||||
---
|
||||
|
||||
## R4 — HIGH — the NPC airborne branch still writes cell, render entity and shadow
|
||||
|
||||
Contract item 5 says the airborne branch writes NOTHING. The player arm complies
|
||||
(`:1624-1628` returns). The NPC arm only skips the snap and falls through to
|
||||
`rmState.CellId = p.LandblockId` (`:1931` — which commits the canonical cell via
|
||||
`RemoteMotion.cs:169-177` and can rebucket), `LastServerPos/Time`,
|
||||
`RemoteServerControlledVelocityCycle.Apply` (an animation decision from an
|
||||
airborne packet), the `entity.SetPosition`/`ParentCellId`/`Rotation` tail
|
||||
(`:1976-1978`), and `LiveEntityShadowPublisher.TryPublishRemote` (`:1979`).
|
||||
|
||||
Worse than before *because* the body no longer moves: previously body, cell,
|
||||
entity and shadow all agreed; now the body stays put while the cell jumps to the
|
||||
server's and the shadow publishes against a cell that need not contain it.
|
||||
|
||||
(Contract-precision note: `:1616` writes `rmState.CellId` before the PLAYER
|
||||
return too. That is pre-existing free-fall bookkeeping the contract's "not the
|
||||
cell" wording did not intend to forbid — decide deliberately and record it,
|
||||
rather than leaving the contract and the code silently disagreeing.)
|
||||
|
||||
---
|
||||
|
||||
## R5 — MEDIUM — `ConstrainTo` lost on the player-remote landing packet
|
||||
|
||||
The deleted unconditional arm sat before `if (IsPlayerGuid(...))`, so it ran for
|
||||
the landing transition. The new arm at `:1747-1752` is after the landing block's
|
||||
return at `:1710`. A landing UP is a grounded near correction where retail's
|
||||
`MoveOrTeleport` returns nonzero, so retail DOES arm the leash. Behaviour loss
|
||||
on a path 4a was to leave alone. Also: when the route is null or `Rejected*`,
|
||||
`ConstrainAfterRouting` is false so the leash never re-arms, yet the `else` arm
|
||||
still hard-snaps — a hard move with a stale anchor.
|
||||
|
||||
---
|
||||
|
||||
## R6 — MEDIUM — AP-87 and TS-44 changed silently; both register rows now misdescribe the code
|
||||
|
||||
**AP-87:** the NPC variant's third condition `firstUpNpc`
|
||||
(`LastServerPosTime <= 0`) was dropped with no replacement, and the updated row
|
||||
still describes it as a "belt hint". Low blast radius (a UP-created
|
||||
`RemoteMotion` is seeded so `bodyToTarget == 0`), but the contract demanded the
|
||||
choice be stated, and the original code carried an explicit comment about why
|
||||
`LastServerPosTime` is unreliable.
|
||||
|
||||
**TS-44:** the sticky check moved inside `ApplyInterpolate`, which BOTH kinds
|
||||
call, so it now suppresses player-remote near corrections that previously had no
|
||||
sticky check at all (the old `snapSuppressedByStick` gate was structurally
|
||||
unreachable for players). Player remotes are stickable
|
||||
(`LiveEntityMotionRuntimeController.cs:144-145`, `:342-352`). The updated row
|
||||
still says "NPC UpdatePosition enqueue is suppressed" and calls the App gate an
|
||||
"NPC-only caller gate" — both now false. The player arm is also internally
|
||||
inconsistent: near is suppressed, far is not.
|
||||
|
||||
**Required: decide each deliberately, and make the rows true.**
|
||||
|
||||
---
|
||||
|
||||
## R7 — MEDIUM — contract items 1 and 2 are not met; this is a helper extraction, not an ownership transfer
|
||||
|
||||
The Runtime "owner" is two stateless statics. App still owns request assembly
|
||||
(`ClassifyRemoteAcceptedPosition` is private to the App controller), branch
|
||||
selection, the airborne early return, the cell write, the entity write, and the
|
||||
shadow publish. No second host can reuse any of it. The headless claim is
|
||||
factually true (`RuntimeLiveEntitySessionController.cs:212-216` returns early
|
||||
for remotes, so nothing diverges) but it satisfies item 2 by redefinition.
|
||||
|
||||
Note Runtime ALREADY builds this exact request in
|
||||
`RuntimeInitialCreateContinuationExecutor.ApplyPositionAction:1907-1945`. The
|
||||
new App builder is a third copy of the same construction — which is precisely
|
||||
how R0 and R8 diverged from it. **Prefer sharing that builder over maintaining a
|
||||
third copy.**
|
||||
|
||||
---
|
||||
|
||||
## R8 — MEDIUM-LOW — the request builder fabricates `Vector3.Zero`, violating a documented invariant
|
||||
|
||||
`:75-77` uses `_playerController?.Position ?? Vector3.Zero`. `GameRuntime.cs:288-290`
|
||||
states the rule for this exact field: a null controller "must yield null, never a
|
||||
fabricated Vector3.Zero that would misclassify every remote entity as
|
||||
implausibly far." `RuntimeInitialCreateContinuationExecutor.ResolveInputs:510-521`
|
||||
honours it. The new builder does the opposite.
|
||||
|
||||
---
|
||||
|
||||
## R9 — LOW-MEDIUM — the new generation plumbing is decorative
|
||||
|
||||
`RuntimeEntityObjectLifetime.CurrentGeneration()` and `LiveEntityRuntime.Generation`
|
||||
return the lifetime's CURRENT generation, not the classified record's. The
|
||||
classifier only tests `Generation.Value != 0`, never compares it — so the token
|
||||
cannot detect a generation change, and the currency it claims to add is supplied
|
||||
entirely by the surrounding `IsCurrentPositionAuthority` checks. Two new public
|
||||
members for a non-zero placeholder. Also `_generation` is assigned in
|
||||
`BindEventContext` but never cleared in `Dispose`, unlike its siblings.
|
||||
|
||||
Either make it load-bearing or drop it.
|
||||
|
||||
---
|
||||
|
||||
## R10 — LOW — residue
|
||||
|
||||
- **R11:** `ApplyInterpolate`'s return value is discarded at both call sites, and
|
||||
the NPC caller still wraps it in its own `if (!snapSuppressedByStick)` —
|
||||
duplicating the check that moved into the seam.
|
||||
- **R12:** the deleted per-UP rebucket also lost the pending-bucket promotion
|
||||
recovery (`GpuWorldState.cs:1113-1126`, the 2026-07-03 invisible-player fix).
|
||||
Covered by fixing R1, but note it explicitly.
|
||||
- **R13:** HTML entity escapes leaked into plain `//` comments (`>=96 m`) at
|
||||
`:1718`, `:1735`, `:1864`, `:1883`.
|
||||
- **R14:** `preSnapPos` (`:1394`) assigned, never read, with a comment describing
|
||||
behaviour that no longer exists.
|
||||
|
||||
---
|
||||
|
||||
## R15 — live-gate addition, not a code defect
|
||||
|
||||
D1 makes ACE's wire `IsGrounded` newly load-bearing for NPC remotes. ACE emits it
|
||||
from `TransientState & OnWalkable` (`PositionPack.cs:73`), while acdream's NPC
|
||||
free-fall is gated on the client-tracked `rmState.Airborne`, set only by
|
||||
`0xF74E` VectorUpdate or `!Body.OnWalkable` — never from the wire bit. A creature
|
||||
ACE reports as not-in-contact while the client believes it grounded now receives
|
||||
NO correction where the legacy routing pulled it.
|
||||
|
||||
**Add to the connected gate: push a monster off a ledge / pull one down a
|
||||
cliff.**
|
||||
|
||||
---
|
||||
|
||||
## Gate
|
||||
|
||||
Complete Release suite, not a subset. Pre-4a baseline is 10,909/4/0. R2 means the
|
||||
current green number is not evidence for the thing it claims to test.
|
||||
Loading…
Add table
Add a link
Reference in a new issue