From 7b3e2895cda5151bcf0b982e142d4b96070673f2 Mon Sep 17 00:00:00 2001 From: Erik Date: Thu, 6 Aug 2026 10:08:53 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20close=20the=20AD-10=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20AD-65's=20magnitude=20was=20half=20the=20truth?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 --- docs/ISSUES.md | 30 +- .../retail-divergence-register.md | 4 +- docs/plans/2026-08-02-placement-cutover.md | 13 + .../2026-08-06-ad10-review-architecture.md | 357 +++++++++++++ .../research/2026-08-06-ad10-review-retail.md | 481 ++++++++++++++++++ .../Physics/RemoteMotionCombiner.cs | 6 +- 6 files changed, 881 insertions(+), 10 deletions(-) create mode 100644 docs/research/2026-08-06-ad10-review-architecture.md create mode 100644 docs/research/2026-08-06-ad10-review-retail.md diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 55e0b5bd..7c5db839 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -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. diff --git a/docs/architecture/retail-divergence-register.md b/docs/architecture/retail-divergence-register.md index 19e0ddb1..b9f70298 100644 --- a/docs/architecture/retail-divergence-register.md +++ b/docs/architecture/retail-divergence-register.md @@ -119,7 +119,7 @@ readiness/requeue adaptation. See | AD-2 | Async readiness gates replace retail's synchronous destination cell load. **#229 refinement (2026-07-20):** login and F751 portal-space exit now share `WorldRevealReadinessBarrier`, so neither path can expose the normal viewport until the same render-publication, composite-texture, and collision domains converge. A hydratable indoor claim requires its owning Near-tier static/EnvCell mesh set, destination composites, and exact EnvCell physics (`IsSpawnCellReady`); an outdoor claim requires those render domains plus terrain/collision residency across the DERIVED reveal window. **#280 amendment (2026-08-05):** that outdoor window is no longer a hardcoded radius-1 neighbourhood. Retail has exactly ONE landscape square — `LScape::mid_radius`, assigned directly from the `Render.LandscapeDrawDistance` preference (`SmartBox::SetRegion` @0x004531F0; values `Render_LandscapeDrawDistance_Values` @0x007CA988 = {3,5,8,11,15,25}, default 8, byte-verified) — and that same square is simultaneously the loaded set, the drawn set, and the set `LScape::PreFetchCells` @0x00505660 blocks on, so retail structurally cannot stream farther than it gates. acdream now DERIVES the outdoor radius from the live streaming window (`QualitySettings.FarRadius`, read per evaluation from `StreamingController` so a mid-hold Settings change re-arms the gate the way `SmartBox::set_mid_radius` @0x00453180 does), and the render-completeness predicate is TIER-AWARE to match acdream's two-tier landscape: inside `NearRadius`, full Near publication (`IsNearTier && IsRenderReady`); out to `FarRadius`, terrain publication only (`IsRenderReady`, which a Far-tier landblock satisfies through an empty spawn-adapter registration installed after its terrain upload crossed the render-thread barrier). **#280 review correction (2026-08-06):** as originally written this row asserted that property of a `PublicationKind.Far` *publication* only, which was true but not exhaustive — a landblock also reaches Far tier by Near→Far DEMOTE, and the demote's `LandblockRetirementStage.MeshReferences` left `LandblockSpawnAdapter.WantsLoaded == false` on a landblock that stays loaded and drawn, with no path that re-publishes it. Both review lenses found the same defect: one demoted member anywhere in the far ring made the gate unsatisfiable for the life of the streaming window (permanent portal-space hang, no recovery short of relog), reachable by two consecutive recalls to the same landblock with walking in between, or by a mid-hold quality-preset drop. The two routes are now genuinely equivalent — `GpuWorldState.ReleaseLandblockMeshReferences` re-asserts the empty Far registration after retiring the Near layer — rather than the predicate being taught to tolerate two meanings of "ready". Composite-texture warmup stays `NearRadius`-scoped because it is entity-scoped and Far builds carry no entities, and (same correction) its TRIGGER is scoped the same way: gating warmup on the whole widened gate serialised every composite upload behind the last outer-ring landblock, which is a hold longer than the streaming work requires. The destination reservation opens at exactly the gate's radius, since retail has one square for both. Runtime's readiness invariant is correspondingly a SHAPE check (`indoor ⇒ 0`, `outdoor ⇒ ≥1`), never a re-encoded value — Runtime does not own the graphical host's streaming configuration. Hard-recenter generations and tier-aware completion application prevent stale overlapping loads/unloads or Far/Near jobs from opening or erasing the gate; mesh upload remains separate from balanced landblock ownership. Claims beyond NumCells still take the loud unhydratable-placement path. `RuntimeWorldTransitState` owns the shared reveal generation, accepted readiness, transit correlation, and exact generation/cell-scoped host-acknowledgement suffix. `WorldRevealCoordinator` is a graphical adapter holding only App resource receipts; normalized Runtime checkpoints observe ownership without defining another readiness path. **Slice E3 refinement (2026-07-24):** the same generation now publishes an immediate `WorldGenerationQuiescence` edge: old-world drawing/spatial queries, simulation/effect clocks, reconciliation, targeting, and 3-D audio stop while retained physical teardown advances through metered cursors and destination network/UI/streaming/readiness remain live. **Slice E4 refinement (2026-07-24):** accepted render/physics/static publication may span update frames through retained exact cursors, but reveal still consumes only the completed spatial/render-ready generation; building and EnvCell snapshots remain invisible until complete and the final spatial identity swap stays observer-atomic. **Slice E5 refinement (2026-07-24):** the reveal generation owns one exact destination reservation across every typed budget dimension. Stale completion cannot consume or clear its replacement, and hydratable incomplete content is never force-revealed; portal transit retains the DAT tunnel and centered retail wait cue until readiness converges. The hold→materialize→regain-control lifecycle remains owned by `TeleportAnimSequencer`. **C4 route 3 refinement (2026-08-04):** retail places the local player IMMEDIATELY on the accepted destination Position (`SmartBox::TeleportPlayer` @0x00453910) and blocks SIMULATION on DAT prefetch (`CellManager::blocking_for_cells`; `SmartBox::UseTime` @0x00455410 runs only `CheckPrefetchStatus`) behind the portal viewport; acdream defers the PLACEMENT itself to this reveal-ready Place edge, executed by the canonical `RuntimeAcceptedPositionDriveController` portal arm (`TryExecuteAcceptedPortalArrival`). Two load-bearing notes from that route: (1) every accepted local Apply — including the portal destination Position itself — still writes the raw wire pose onto the local player's `WorldEntity` via the ordinary generic-remote-render-pose path while portal space covers the viewport (`LiveEntityNetworkUpdateController.cs`, `OwnsSteadyState` false for the local player's null route); the committed Place receipt's presentation suffix overwrites it with the resolved pose — tolerated, not suppressed, since suppressing it would be an unowned behaviour change on the ordinary local Apply path (AP-131/#275 territory). (2) The constraint-leash re-arm on a committed portal placement anchors at the RESOLVED post-placement body position (`PlayerMovementController.CommitCanonicalTeleportFrame` → `RearmConstraintLeashAtCurrentPosition`), where retail's `ConstrainTo` @0x0045418A anchors at the received WIRE destination; the two differ by at most the placement adjustment (ring search/floor snap) and the anchor is write-only downstream, so the delta is not user-observable — switching to the wire-destination anchor is a deliberately deferred decision, not adopted here. **B4 round-3 review refinement (2026-08-05):** the wait cue's trigger predicate (`LocalPlayerTeleportController.Tick`'s `placementReady = dataReady && TryAdvancePortalCommit(sequence)`, gating the cue at `haveDestination && !placementReady`) now covers a SECOND, distinct cause beyond the original streaming/DAT-readiness gate this row described: `TryAdvancePortalCommit` returning false while a DeferredCell park is outstanding or a fresh placement attempt has not yet succeeded (B1's `TryConsumePortalCommit` gate). The cue's five-second trigger and centered-tunnel behavior are unchanged (that trigger is an acdream divergence in its own right — AP-150, filed 2026-08-06); only the SET of conditions that can hold it open grew from "world data not ready" to "world data not ready OR canonical placement not yet committed" — a slow-publishing destination-landblock collision generation now presents identically to a slow asset stream, which is the correct retail-faithful degradation (both are `blocking_for_cells` causes retail itself does not distinguish), but is worth naming here since a future debugging session seeing the cue must not assume streaming is the only possible cause. | `src/AcDream.Runtime/World/RuntimeWorldTransitState.cs`; `src/AcDream.App/Streaming/WorldRevealCoordinator.cs`; `src/AcDream.App/Streaming/WorldGenerationQuiescence.cs`; `src/AcDream.App/Streaming/WorldRevealReadinessBarrier.cs`; `src/AcDream.App/Streaming/StreamingOriginRecenterCoordinator.cs`; `src/AcDream.App/Streaming/LandblockPresentationPipeline.cs`; `src/AcDream.App/Streaming/StreamingController.cs`; `src/AcDream.App/Rendering/PortalTunnelPresentation.cs`; `src/AcDream.App/UI/PortalWaitNoticeController.cs`; `src/AcDream.App/Streaming/GpuWorldState.cs` (`IsRenderReady`); `src/AcDream.App/Rendering/Wb/LandblockSpawnAdapter.cs`; `src/AcDream.Core/Physics/PhysicsEngine.cs` (`IsSpawnCellReady`, `IsNeighborhoodTerrainResident`) | This is the asynchronous equivalent of retail leaving `SmartBox::position_update_complete` false while `CellManager::blocking_for_cells` is set: neither initial login nor portal arrival may reveal or continue simulating an old/partial collision world, a terrain-only Far shell, or a published-but-not-drawable GPU landblock. Indoor does not require a terrain heightmap, only the owning render landblock and exact EnvCell. | Gate opens early → grey/untextured first login or portal reveal, free-fall, wrong-cell rooting, missing scenery, or a still-active old generation; predicate never satisfies (streamer/DAT/upload failure) → login remains behind the world render gate, while portal transit remains in the authored tunnel and presents the centered wait cue after five seconds — that five-second arming is acdream's own and is NOT retail's trigger; see AP-150. | `SmartBox::UseTime` 0x00455410; `gmSmartBoxUI::UseTime` 0x004D6E30; `gmSmartBoxUI::EndTeleportAnimation` 0x004D65A0; `LScape::PreFetchCells` 0x00505660; `LScape::SetMidRadius` 0x00504C00; `SmartBox::set_mid_radius` 0x00453180; `Render_LandscapeDrawDistance_Values` 0x007CA988 | | AD-5 | Outdoor `point_in_cell` is an identity compare against the global XY-column cell from `LandDefs.AdjustToOutside` (no per-cell containment test) | `src/AcDream.Core/Physics/CellTransit.cs:865` | Landcells are disjoint 24 m columns — identity-compare against the column under the sphere centre is exactly equivalent to retail's per-candidate test | If block-origin/lcoord math is wrong at a landblock seam, the compare silently never matches — outdoor membership freezes at boundaries (the pre-#106 symptom) | `find_cell_list` pick pc:308788-308825; `CLandCell::point_in_cell` (get_block_offset pc:308804) | | ~~AD-6~~ | **RETIRED 2026-07-31 (placement/streaming Slice 3B).** Cell/cache/topology/building/static-shadow publication plus every retained non-suspended owner touching or withdrawn from the prefix is one Runtime-owned collision generation. Retained includes dynamics and adjacent-root statics; only target-root statics are superseded by the authored replacement. App and Headless build one shared off-side `CollisionWorldState` through one-work-unit preparation/capture/seal cursors. Admission captures the active root in O(1); a stable landblock/owner slot suffix materializes non-target leaves incrementally, so resident-world size cannot become a synchronous clone spike. Reusable per-prefix owner slots and one Runtime-scoped versioned journal replace event-time exact-copy fanout: repeated live mutations coalesce by owner, every draft reconciles only that owner's latest exact state one owner per seal call, discovered relevant owners receive scoped exact updates, and visited unrelated owners receive only a cheap coalesced dirty notification before metered replay. Once topology sealing finishes, observed owners temporarily write through exactly until same-call activation; the finite pre-seal queue therefore drains even under continuous multi-owner movement. New drafts start at their captured journal suffix; old slots are superseded rather than reused behind live cursors and compact through the same meter. Unrelated churn therefore never restarts or starves target capture/sealing. Deterministically ordered concurrent preparations receive committed—not merely sealed—peer deltas and rebase one cache, graph, landblock, or owner leaf per seal step; cancellation therefore cannot leak unpublished topology. Demotion/withdrawal cancels a matching queued or active rebase, suppresses the prefix in unfinished source scans, and retires one owner/cache/graph/outdoor leaf per seal call. The complete previous generation remains queryable until one zero-managed-byte volatile root transfer in the same update-thread call as final reconciliation; that preserves PhysicsDataCache, CellGraph, PhysicsEngine, and ShadowObjectRegistry facade identity, revokes staging, and requires no quiet frame. A stale admission or staging failure disposes only that private generation and cannot withdraw the active world or invalidate a newer admission. Authored same-ID target statics, live-current-cell changes, owner departure/reuse, newly relevant seam-crossing statics, and teardown remain coherent across drafts; empty per-prefix owner containers are reclaimed without invalidating captured seal cursors. The commit clears repaired withdrawal markers before its single notification/readiness acknowledgement, so no optional hydration callback can omit reflood and no observer sees mixed old/new cells. | `src/AcDream.Runtime/Physics/RuntimePhysicsState.cs` (`PrepareCollisionGeneration`, `AdvanceCollisionGenerationPreparation`, `AdvanceCollisionGenerationSeal`, `CommitCollisionGeneration`); `src/AcDream.Core/Physics/CollisionWorldState.cs`; `PhysicsDataCache.cs`; `PhysicsEngine.cs`; `ShadowObjectRegistry.cs`; `src/AcDream.App/Streaming/LandblockPhysicsPublisher.cs`; `src/AcDream.Headless/Hosting/HeadlessSessionWorldProjection.cs`; `tests/AcDream.Runtime.Tests/Physics/RuntimePhysicsStateTests.cs`; `tests/AcDream.App.Tests/Streaming/LandblockPhysicsPublisherTests.cs`; `tests/AcDream.Headless.Tests/HeadlessSessionHostTests.cs` | — | — | `CObjCell::init_objects` → `CPhysicsObj::recalc_cross_cells`, 0x0052b420 / 0x00515a30; `CPhysicsObj::SetPositionInternal` shadow replacement tail 0x00515330 | -| ~~AD-10~~ | **RETIRED 2026-08-06 by deletion.** The row's justification ("remote bodies don't run a full local transition sweep") was false at HEAD: `RuntimeRemotePhysicsUpdater.Tick` calls `PhysicsEngine.ResolveWithTransition` with the remote's own body, and that sweep runs acdream's verbatim port of `CTransition::adjust_offset` once per sub-step. So this was never a relocation of a missing mechanism — it was an EXTRA pre-sweep projection layered on top of the faithful one, against a surface retail never uses (`SampleTerrainNormal(x, y)`, an XY-only landblock lookup blind to the body's Z, its cell, buildings, EnvCells and statics). Measured before deleting: with the projection forced null at both fork sites, the production trajectory of a remote running 30 ticks down a 31-degree ramp is BIT-IDENTICAL, a 8.4-degree ramp differs by at most 2.8e-5 m in Z, and the whole `AcDream.Runtime.Tests` suite is unchanged. Deleted: both `RuntimeRemotePhysicsUpdater` sample sites, the `terrainNormal` parameter and projection block on `RemoteMotionCombiner.ComposeOffset` AND on the production-dead `ComputeOffset`, and the now-callerless `PhysicsEngine.SampleTerrainNormal`. Removing the parameter is what makes an AP-22-shaped one-site-only regression a compile error. Two claims in the old row were also stale/backwards and did not survive: it described `ComposeOffset`'s guard as "interpolation-active" when the code is `if (!interpolationOverwrote ...)`, and its second cited site (`ComputeOffset` ~:163-168) had zero production callers. The roof clause was stale too — since Bug B (`204d0ae0`) the sample was gated on `OnWalkable`, and a steep roof is `OnWalkable == false`, so the path did not run on #32's geometry at all. | `src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs`; `src/AcDream.Core/Physics/RemoteMotionCombiner.cs`; `src/AcDream.Core/Physics/PhysicsEngine.cs` (deletion); `tests/AcDream.Runtime.Tests/Physics/RuntimeRemoteSlopeProjectionTests.cs`; `tests/AcDream.Runtime.Tests/Physics/RemoteRampHarness.cs` | — | — | `CTransition::adjust_offset` 0x0050a370, pc:272271-272393 (the old anchor pc:272296-272346 truncated both the sliding-normal validity gate at the head and the entire safety push-out block at the tail); per-step call from `CTransition::find_transitional_position` 0x0050bdf0 | +| ~~AD-10~~ | **RETIRED 2026-08-06 by deletion.** The row's justification ("remote bodies don't run a full local transition sweep") was false at HEAD: `RuntimeRemotePhysicsUpdater.Tick` calls `PhysicsEngine.ResolveWithTransition` with the remote's own body, and that sweep runs acdream's port of `CTransition::adjust_offset` once per sub-step. **(Wording corrected 2026-08-06 at the retail review, F2: this row and four other places called that port "verbatim"/"faithful". It is structurally exact BUT carries exactly two divergences, filed the same day as AD-65 and AD-66 -- so the unqualified word was false from the very next commit. It is a STRUCTURALLY EXACT port with two filed exceptions.)** So this was never a relocation of a missing mechanism — it was an EXTRA pre-sweep projection layered on top of the faithful one, against a surface retail never uses (`SampleTerrainNormal(x, y)`, an XY-only landblock lookup blind to the body's Z, its cell, buildings, EnvCells and statics). Measured before deleting: with the projection forced null at both fork sites, the production trajectory of a remote running 30 ticks down a 31-degree ramp is BIT-IDENTICAL, a 8.4-degree ramp differs by at most 2.8e-5 m in Z, and the whole `AcDream.Runtime.Tests` suite is unchanged. Deleted: both `RuntimeRemotePhysicsUpdater` sample sites, the `terrainNormal` parameter and projection block on `RemoteMotionCombiner.ComposeOffset` AND on the production-dead `ComputeOffset`, and the now-callerless `PhysicsEngine.SampleTerrainNormal`. Removing the parameter is what makes an AP-22-shaped one-site-only regression a compile error. **Noted 2026-08-06 at the retail review (F3): this 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.** Two claims in the old row were also stale/backwards and did not survive: it described `ComposeOffset`'s guard as "interpolation-active" when the code is `if (!interpolationOverwrote ...)`, and its second cited site (`ComputeOffset` ~:163-168) had zero production callers. The roof clause was stale too — since Bug B (`204d0ae0`) the sample was gated on `OnWalkable`, and a steep roof is `OnWalkable == false`, so the path did not run on #32's geometry at all. **UNTESTED AXIS, recorded 2026-08-06 at the AD-10 architecture review: the contract's T2 -- its mandatory wrong-plane-versus-right-plane discriminator -- was dropped without record, in breach of the contract's own "record it as an untested axis rather than silently dropping it" clause. Consequence: this change's only claimed BENEFIT (a walkable NON-TERRAIN surface -- bridge, dock, dungeon ramp -- now gets the committed contact plane instead of the terrain plane far below) has ZERO automated coverage and rests on source reasoning alone. The deletion itself is measured; the benefit is not.** | `src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs`; `src/AcDream.Core/Physics/RemoteMotionCombiner.cs`; `src/AcDream.Core/Physics/PhysicsEngine.cs` (deletion); `tests/AcDream.Runtime.Tests/Physics/RuntimeRemoteSlopeProjectionTests.cs`; `tests/AcDream.Runtime.Tests/Physics/RemoteRampHarness.cs` | — | — | `CTransition::adjust_offset` 0x0050a370, pc:272271-272393 (the old anchor pc:272296-272346 truncated both the sliding-normal validity gate at the head and the entire safety push-out block at the tail); per-step call from `CTransition::find_transitional_position` 0x0050bdf0 | | ~~AD-11~~ | **RETIRED 2026-07-23** — the matching binary disproved the old nonzero interpretation: `ItemUses::IsUseable` executes `not bitfield; and eax,1`, so absent/reset zero is usable and only `USEABLE_NO` disables use. Toolbar, item policy, and world interaction now share that exact Core predicate. | `src/AcDream.Core/Items/ClientObject.cs` (`ItemUseability.IsUseable`); `src/AcDream.Core/Items/ItemInteractionPolicy.cs`; `src/AcDream.App/Interaction/WorldSelectionQuery.cs` | — | — | `ItemUses::IsUseable @ 0x004FCCC0`; matching v11.4186 instructions recorded in `docs/research/2026-07-23-retail-item-use-and-autowear-pseudocode.md` | | AD-12 | SecondaryAttributeTable coefficients hardcoded (Health=End×0.5, Stam=End×1.0, Mana=Self×1.0) instead of dat-read; unknown attributes contribute 0 | `src/AcDream.Core/Player/LocalPlayerState.cs:279` | Coefficients never vary across retail dat versions; re-confirmed by ACE AttributeFormula.cs + holtburger; dat port can replace later | A customized portal.dat with modified vital formulas silently yields wrong max-vitals; a missing attribute snapshot underestimates max | SecondaryAttributeTable portal.dat 0x0E0..0x0E2; `CreatureVital::GetMaxValue` 0x0058F2DD | | AD-13 | 1-second dedup window for identical system chat messages (retail has none) | `src/AcDream.Core/Chat/ChatLog.cs:29` | ACE dual-sends the same system text (0xF7E0 + 0x02EB) for back-compat; without dedup every line doubled (Phase J compromise) | Two genuinely distinct but textually identical system messages within 1 s collapse to one line where retail shows both | ACE dual-send 0xF7E0 + 0x02EB | @@ -157,7 +157,7 @@ readiness/requeue adaptation. See | AD-62 | **Filed 2026-08-03 (C4 route 2, round 2); rewritten round 3.** General rule: an accepted local-player ForcePosition that this route does not carry through to a committed canonical placement is never re-applied. That half matches retail — `SmartBox::BlipPlayer` attempts the placement exactly once and never retries. What diverges is that acdream has non-commit outcomes retail cannot reach at all, because retail's world is fully resident and its placement synchronous. Round 3 narrowed the loss to the re-apply alone wherever the packet's placement was actually BEGUN: the retail position event now fires at that packet's terminal outcome whether or not the placement committed (`SettlePending`'s `positionEventOwed` path), matching `BlipPlayer` discarding `SetPositionSimple`'s `enum SetPositionError` and `HandleReceivedPosition` acking unconditionally @0x00454091. Shapes losing ONLY the re-apply: (i) the destination landblock's collision generation is unpublished so the placement parks (`DeferredCell`) and is then retired by a non-position cause (collision-generation retirement, the lost-cell deadline, `ParkCollisionResidents`) with the accepted authority unmoved — the funnel's EQUAL branch; (ii) the same park superseded by a newer ordinary `Apply` Position which now owns the pose — the ADVANCED+ordinary branch; (iii) any OTHER `PositionAuthorityVersion` advance moving the record out from under the funnel's re-issue test — `TryApplyPickup` (`RuntimeEntityObjectLifetime.cs:1116`), `CommitPositionChannelUpdate` (`:2041`), `AdvanceCreateAuthority` (`:2466`) — effectively unreachable for a live local player, but they fail silently in the same direction and the funnel cannot tell them from (ii). Shapes still losing BOTH the re-apply and the ack because no placement was ever begun for that packet: (iv) a `Contention` whose blocking operation is EXTERNAL to this drive (a concurrent portal/teleport placement owns the entity) — nothing is recorded in `_pending`, so nothing pumps it and the packet is dropped outright; (vi) a re-issue retry marker whose re-issue never manages to begin before the funnel clears it. Losing BOTH for a DIFFERENT reason — the placement WAS begun, but the descriptor was displaced before reaching its own terminal settle: (v) a packet superseded by a newer force whose own placement begins cleanly — `SettlePending` opens by nulling `_pending` without reading it, so the older descriptor's owed ack is discarded. Replaying it would be worse than losing it (a stale-sequence report carrying the newer packet's committed pose), and the displacing packet always acks, so ACE always receives a report for the newest force. The `DeferredCell` park is NOT a precondition of this row: shapes (iv)-(vi) never park. In every shape the body stays where the last successful placement left it and the next accepted Position (ACE broadcasts at 5-10 Hz) carries the corrected pose forward. | `src/AcDream.Runtime/Session/RuntimeAcceptedPositionDriveController.cs` (`SettlePending` — the single terminal-outcome funnel: its `positionEventOwed` ack and its two non-reissuing branches; and `TryExecuteAcceptedLocalPosition`'s `Contention` return) | Retail has no park and no external placement authority: `SmartBox::BlipPlayer` runs synchronously against a fully resident world, so "arrived but not yet placeable" and "another placement owns this entity" are both unrepresentable there. Those are our async collision-publication and single-placement-authority adaptations. Re-issuing a retired force instead would be worse than not: shape (ii) would stamp the force route's `Teleport\|Slide` flags and an unconditional ack onto an ordinary echo's pose while skipping the `ConstrainTo` the ordinary branch runs (`RuntimeAuthoritativePositionRouteClassifier.cs:368-388`), and shape (i) can re-issue into the same persistent cancellation cause indefinitely. The drive still owns at most one in-flight placement and still re-issues whenever the newest accepted event IS a still-unserved ForcePosition. | A server correction whose destination collision is slow to publish, or which lands while another placement authority owns the entity, can be silently skipped: the player stays at the pre-correction pose for one broadcast interval (~100-200 ms). Sustained (a slow-publishing destination correcting repeatedly) this reads as rubber-banding that does not take. In shapes (iv)-(vi) ACE additionally receives one fewer `AutonomousPosition` than retail would have sent, so the server cannot tell its force was not applied. | `SmartBox::HandleReceivedPosition` @0x00453FD0 FORCE_POSITION branch (`SendPositionEvent` @0x00454091, early return @0x0045409D); `SmartBox::BlipPlayer` @0x00453940 (discards the error, returns void); `CPhysicsObj::SetPositionSimple` @0x005162B0 (returns `enum SetPositionError`; other callers test `== OK_SPE` @0x0055605D/@0x00556021); `CommandInterpreter::SendPositionEvent` @0x006B4770 | | AD-63 | **Filed 2026-08-04 (cancelled-park presentation rollback).** When a cancelled restorable park is rolled back, the entity's presentation is restored EXCEPT the player's selection. `ParkDeferred`'s Withdraw receipt makes the host sink clear the selection if the parked entity was the selected object (`_clearSelectionForUnavailableEntity`), and the `WithdrawalRestored` receipt that rolls that withdrawal back deliberately does not re-select it. Every other registration the withdrawal removed — the graphical bucket, projection visibility, plugin world state, the world-event replay set, the effect-pose registry, the local-player shadow, the presentation visibility sinks — IS restored exactly. | `src/AcDream.App/World/RuntimePlacementPresentationSink.cs` (`TryApplyWithdrawalRestoration` vs `TryPublishWithdrawal`'s `_clearSelectionForUnavailableEntity` call) | Selection is user intent, not a projection registration. Retail clears the selection when its target becomes unavailable (`SelectionChangeReason.SelectedObjectRemoved` is acdream's name for the same edge) and never re-selects on the object's behalf; re-selecting here would invent input the player did not give. Retail also cannot reach this state at all — it has no cancel for a lost-cell park (AP-136) — so there is no retail behaviour to match, only two acdream choices, and "do not act for the player" is the conservative one. | The player loses their target for the ~150 ms park window if the selected object happened to park, and must re-click it. No other state is affected: the object is visible, on the radar, collidable, and assessable again as soon as the restoration receipt drains. Retire together with AP-136 by making the park survive cancellation (issue #309), which removes the withdrawal — and therefore the selection clear — entirely. | AP-136 (the park rollback this rides on); no retail anchor — retail has no cancellable lost-cell park | | AD-64 | **Filed 2026-08-05 at the C5b architecture review's D1 fix.** The graphical and no-window hosts run parallel, non-shared inbound entity routes — `LiveEntitySessionController` → `LiveEntityNetworkUpdateController.OnPosition` versus `RuntimeLiveEntitySessionController.OnPositionUpdated` — and AD-60's W2 wire-cell commit is therefore expressed TWICE. The committed VALUE is shared exactly (one owner, `RuntimeEntityObjectLifetime.CommitWireCellRebucket`, including the landblock-vs-cell preserve branch); what is duplicated is the REACHABILITY decision — which packets may reach it. The graphical host encodes that decision implicitly, as the set of early returns strewn through a 400-line `OnPosition` (authority gate on `Rejected`, the local force arm on every drive status except `NotApplicable`, the missile arm, the `ChildUnparentDisposition` Superseded/Pending arm, the initial-create residence gate inside `RebucketLiveEntity`). The no-window host encodes it explicitly, in one method, `TryCommitAcceptedWireCell`, whose gates were derived from those returns one by one. Two of the graphical gates have no no-window analogue and are deliberately absent rather than reproduced: the `ChildUnparentDisposition` arm is presentation recovery this host does not perform, and the residence gate's `MaterializationResidence is AwaitRuntimePlacement` half is App presentation bookkeeping whose no-window equivalent is unconditionally true for a residence-backed record. **CORRECTED 2026-08-05 at the C5b closeout (architecture finding L-B): "deliberately absent" was presented as the complete list of differences and it was not — there are three more, and the row's own "derived from those returns one by one" phrasing was the claim that made them invisible.** (a) **The residence gate is WEAKER than the merge's own.** Both hosts' wire-cell commits gate on `TryGetInitialCreateResidence` (= `RuntimeInitialCreateResidenceState.TryGetCurrent`), while `RuntimeEntityObjectLifetime.TryApplyPosition`'s FIFO enqueue branch gates on `TryGetPendingInitialResidence` (= `TryGetTransaction` = `TryGetCurrent` OR a completed-but-unretired lease, `RuntimeInitialCreateResidenceState.cs:729-748`). In that window the merge enqueues the packet as a continuation while the commit reads "no residence" and writes the wire cell AHEAD of the continuation that will replay it. Host-symmetric and pre-existing — the graphical `RebucketLiveEntity` has the identical pair — but this row previously claimed the `AwaitRuntimePlacement` half was the only deliberately-absent piece of the residence gate, which is false. (b) **The missile gates are two different expressions.** The graphical route PREFERS `earlyRemoteRoute.OperationKind is RuntimeSetPositionOperationKind.ProjectileAuthoritative` and falls back to the `Missile`-flag / bound-projectile conjunction only when the classification is null; the no-window route ALWAYS uses the conjunction, because it classifies nothing for a remote. They agree today (the conjunction is what the classifier's own projectile test is built from), but they are separately maintained and only the conjunction is reachable on one side — a change to the classifier's projectile predicate moves one host and not the other. (c) **The pre-merge PAYLOAD gate was absent entirely, and is now present.** The graphical route validates the wire payload before the merge (`LiveEntityNetworkUpdateController.OnPosition`'s `payloadIsValid` from `ProjectileController.CanAcceptPositionPayload` — despite the name, not projectile-scoped; it runs for every guid — consumed by `LiveEntityInboundAuthorityGate.TryAcceptPosition`'s `!payloadIsValid` return). The no-window route had no equivalent, so since D1 an unvalidated `update.Position.LandblockId` reached `CommitWireCellRebucket`, whose own doc calls `0` "the withdrawal shape" (cell 0 + landblock 0) — silently de-residencing the entity in the exact field `RuntimeEntityObjectViews.Snapshot` hands every bot as `CellId` and `RuntimeSetPositionState.IsAffectedCollisionResident` reads. Fixed at the closeout by applying the same predicate at the same point: `RuntimeAuthoritativePositionRouteClassifier.IsValidCreateWirePosition` plus the finite-velocity term, the pair `RuntimeEntityObjectLifetime.TryApplyPosition` already applies on its initial-residence branch. Rejecting BEFORE the merge (not merely before the commit) is what makes the hosts symmetric — neither lets an invalid payload advance the timestamp gate — and is pinned by `RuntimeLiveEntitySessionControllerTests.InvalidPositionPayload_IsRefusedBeforeTheMerge_InANoWindowHost`, sabotage-verified in both directions (gate removed -> red at the withdrawal-shape assertion; gate moved to guard only the commit -> red at the pose assertion). The no-window host also has no W3 (`TryAdoptWireCellAfterRouting`) analogue and needs none — it performs no remote contact routing at all. | `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs` (`OnPosition`, the implicit gate set); `src/AcDream.App/World/LiveEntityRuntime.cs` (`RebucketLiveEntity`'s residence early return); `src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs` (`TryCommitAcceptedWireCell`, `IsMissilePacket`) | Retail has one client and therefore one route; there is no retail shape to match, only acdream's own two-host structure. The alternative — unifying the two session controllers so the decision exists once — is the genuinely correct fix and is filed as issue #324, but it is campaign-sized: it has to reconcile presentation recovery, hydration, the equipped-child renderer, and the remote routing arms that only one of the two hosts has. Duplicating a small, individually test-gated decision is the cheaper correct thing meanwhile; duplicating it SILENTLY, which is what the pre-D1 state amounted to (one host simply had none of it), is what this row exists to stop. | The two decisions can drift: a future change to one host's reachability rules will not be caught by the other host's tests. Concretely, if the graphical route later adds an early return, the no-window host keeps committing on that packet shape, and vice versa. Bounded by the eight-sabotage gate the D1 fix left behind, plus the closeout's ninth (the payload gate, red in both directions) — every arm of `TryCommitAcceptedWireCell` and both directions of the force rule are individually red-verified — so drift shows up as a test that must be deliberately changed, not as a silent divergence. **That bound does NOT cover the three differences added at the closeout**: the weaker residence predicate (a) and the missile-expression split (b) have no discriminating test on either side, because in both cases the two hosts currently AGREE and the divergence is structural rather than behavioural. They are recorded here precisely because nothing else will catch them. Retire with #324. | No retail anchor — acdream-only host-structure deviation. Adjacent rows: AD-60 (the W2/W3 channel list), AP-146/#320 (the local player's cell edges) | -| AD-65 | **Filed 2026-08-06 (found while retiring AD-10; NOT fixed here).** `Transition.AdjustOffset`'s `collisionAngle > 0` arm — the body moving AWAY from its contact plane — substitutes `result -= N * collisionAngle` for retail's `Plane::snap_to_plane` call, making the `if` and the `else` arms byte-identical. Retail's two arms are genuinely different: `snap_to_plane` (0x00509c50) writes ONLY `v.z = -(v.x*N.x + v.y*N.y) / N.z` and leaves X and Y untouched, while the into-plane arm subtracts the full normal component. So for a horizontal step of length d on a slope of angle theta, retail DESCENDS with XY preserved at d and Z dropping d*tan(theta) (speed along the plane d/cos theta), whereas acdream shrinks XY to d*cos^2(theta) (speed along the plane d*cos theta). acdream therefore descends slopes SLOWER than retail by cos^2(theta) in XY: 13% slow at 30 degrees, 29% at 45 degrees. Uphill (`collisionAngle <= 0`) is correct and identical to retail. | `src/AcDream.Core/Physics/TransitionTypes.cs` (`AdjustOffset`, the `else` arm commented "Moving away from contact plane: snap to plane surface" — the comment names snap_to_plane but the code does not call it) | Not justified — this is an unexamined substitution, not a decision. It is filed rather than fixed because it changes LOCAL-PLAYER movement feel and so needs its own visual gate; folding it into a remote-movement change would put a local-player regression behind the wrong acceptance test. | Downhill locomotion is 13-29% slow in XY across the walkable slope range, for every mover that runs the sweep (local player, remotes, projectiles). **Recorded as a LEAD, not a diagnosis, for the open #269 slope-slide feel residual** (Campaign P): the direction is right (downhill-only, XY-shortening) but nothing here establishes causation, and #269 still needs its live cdb A/B. Note #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. | `CTransition::adjust_offset` 0x0050a370, pc:272271-272393; the branch at `0050a4fa fcomp [0x795344]` / `0050a502 test ah,0x41` / `0050a505 jne 0x50a515` — disassembled from the PDB-paired v11.4186 binary (GUID 9e847e2f-777c-4bd9-886c-22256bb87f32), 0x795344 = 0.0f (bytes 00000000). FPU C0 is "less" and C3 is "equal", so `jne` on `ah & 0x41` takes the SUBTRACT branch at 0x50a515 when `cAngle <= 0` and falls through to `call 0x509c50` (`Plane::snap_to_plane`, pc:271852) when `cAngle > 0`. Binary Ninja renders all four comparisons in this function as the `fnstsw`/`test ah` mush and cannot be read for direction. | +| AD-65 | **Filed 2026-08-06 (found while retiring AD-10; NOT fixed here).** `Transition.AdjustOffset`'s `collisionAngle > 0` arm — the body moving AWAY from its contact plane — substitutes `result -= N * collisionAngle` for retail's `Plane::snap_to_plane` call, making the `if` and the `else` arms byte-identical. Retail's two arms are genuinely different: `snap_to_plane` (0x00509c50) writes ONLY `v.z = -(v.x*N.x + v.y*N.y) / N.z` and leaves X and Y untouched, while the into-plane arm subtracts the full normal component. So for a horizontal step of length d on a slope of angle theta, retail DESCENDS with XY preserved at d and Z dropping d*tan(theta) (speed along the plane d/cos theta), whereas acdream shrinks XY to d*cos^2(theta) (speed along the plane d*cos theta). acdream therefore descends slopes SLOWER than retail by cos^2(theta) in XY: **25% slow at 30 degrees, 50% at 45 degrees**. **MAGNITUDE CORRECTED 2026-08-06 at the AD-10 retail review (F1): this row first said 13%/29%, which is 1-cos(theta) -- the wrong formula for its own stated cos^2(theta) factor, and half the true value.** The correction is confirmed by measurement, not just algebra: #331's probe records 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). This matters because the row is a LEAD for #269's slope-slide residual -- at the understated magnitude the lead reads as marginal and could be dismissed. Uphill (`collisionAngle <= 0`) is correct and identical to retail. | `src/AcDream.Core/Physics/TransitionTypes.cs` (`AdjustOffset`, the `else` arm commented "Moving away from contact plane: snap to plane surface" — the comment names snap_to_plane but the code does not call it) | Not justified — this is an unexamined substitution, not a decision. It is filed rather than fixed because it changes LOCAL-PLAYER movement feel and so needs its own visual gate; folding it into a remote-movement change would put a local-player regression behind the wrong acceptance test. | Downhill locomotion is 25-50% slow in XY across the walkable slope range (corrected 2026-08-06; was understated as 13-29%), for every mover that runs the sweep (local player, remotes, projectiles). **Recorded as a LEAD, not a diagnosis, for the open #269 slope-slide feel residual** (Campaign P): the direction is right (downhill-only, XY-shortening) but nothing here establishes causation, and #269 still needs its live cdb A/B. Note #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. | `CTransition::adjust_offset` 0x0050a370, pc:272271-272393; the branch at `0050a4fa fcomp [0x795344]` / `0050a502 test ah,0x41` / `0050a505 jne 0x50a515` — disassembled from the PDB-paired v11.4186 binary (GUID 9e847e2f-777c-4bd9-886c-22256bb87f32), 0x795344 = 0.0f (bytes 00000000). FPU C0 is "less" and C3 is "equal", so `jne` on `ah & 0x41` takes the SUBTRACT branch at 0x50a515 when `cAngle <= 0` and falls through to `call 0x509c50` (`Plane::snap_to_plane`, pc:271852) when `cAngle > 0`. Binary Ninja renders all four comparisons in this function as the `fnstsw`/`test ah` mush and cannot be read for direction. | | AD-66 | **Filed 2026-08-06 (found while retiring AD-10; NOT fixed here).** `Transition.AdjustOffset`'s safety push-out substitutes `naturalRestingDist = radius * ContactPlane.Normal.Z` for retail's bare `radius` in BOTH the trigger comparison and the `zDist` numerator. The substitution is deliberate and carries a written rationale in the code (the LocalSphere origin sits at (0, 0, radius) along WORLD Z, so a sphere resting on a tilted plane is `radius * N.z` from it, and the bare threshold would fire spuriously on every slope and lift the feet by r*(sec theta - 1) — 7 cm at 30 degrees, 48 cm at 60). The rationale may well be correct. What is missing is the register row: an intentional deviation from a byte-confirmed retail constant with no row is precisely what this register exists to catch, and the code comment's claim that "ACE and the published pseudocode have the original threshold" understates it — the retail BINARY has it. | `src/AcDream.Core/Physics/TransitionTypes.cs` (`AdjustOffset`, the `ci.ContactPlaneCellId != 0 && !ci.ContactPlaneIsWater` block) | Argued at length in the code comment and empirically motivated (the uncorrected threshold reportedly broke ValidateWalkable's contact check on steep slopes and flickered the Falling animation while running uphill). Filed to make the deviation auditable, not to assert it is wrong. | If the sphere-origin premise is mistaken, the push-out under-fires on slopes and a genuinely penetrating sphere is left below its contact plane. Conversely, if the premise is right, retail itself has the spurious lift and acdream is deliberately smoother than retail on slopes — a feel divergence in the same family as, and possibly interacting with, AD-65 and #269. | `CTransition::adjust_offset` 0x0050a370; disassembled from the PDB-paired v11.4186 binary: `0050a5c4 fld [ecx+0xc]` loads the bare `global_sphere->radius` and `0050a5c7 fsub [0x7c6878]` subtracts 0.00019999999494757503f (bytes 17b75139) for the trigger; `0050a5dc fsubr [ecx+0xc]` reloads the bare radius for the numerator before `0050a5df fdiv [esi+8]` divides by `contact_plane.N.z`. Neither site multiplies by N.z. | --- diff --git a/docs/plans/2026-08-02-placement-cutover.md b/docs/plans/2026-08-02-placement-cutover.md index b0359985..03e2d83c 100644 --- a/docs/plans/2026-08-02-placement-cutover.md +++ b/docs/plans/2026-08-02-placement-cutover.md @@ -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. diff --git a/docs/research/2026-08-06-ad10-review-architecture.md b/docs/research/2026-08-06-ad10-review-architecture.md new file mode 100644 index 00000000..f44eab6a --- /dev/null +++ b/docs/research/2026-08-06-ad10-review-architecture.md @@ -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 `` block placed *after* the closing +`` on line 38, and terminated by a second `` on line 52: + +``` +38 /// +39 /// +40 /// AD-10, retired 2026-08-06. … +52 /// …unchanged. +53 /// <- unmatched close tag +``` + +Two closes, one open, one orphaned ``. 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 ``. + +### 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. diff --git a/docs/research/2026-08-06-ad10-review-retail.md b/docs/research/2026-08-06-ad10-review-retail.md new file mode 100644 index 00000000..f31d5fb6 --- /dev/null +++ b/docs/research/2026-08-06-ad10-review-retail.md @@ -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 `` block sits at document top level from line 40, and a +second, unmatched `` 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 `` inside the first `` 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).** diff --git a/src/AcDream.Core/Physics/RemoteMotionCombiner.cs b/src/AcDream.Core/Physics/RemoteMotionCombiner.cs index d80cd29b..22b6a8ac 100644 --- a/src/AcDream.Core/Physics/RemoteMotionCombiner.cs +++ b/src/AcDream.Core/Physics/RemoteMotionCombiner.cs @@ -35,7 +35,6 @@ public sealed class RemoteMotionCombiner /// Compose retail's complete per-object delta frame. Interpolation, when /// active, replaces the PartArray frame via /// Position::subtract2; otherwise the authored root frame remains. - /// /// /// AD-10, retired 2026-08-06. This method used to accept a /// terrainNormal 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 /// collision_info.contact_plane INSIDE the sweep /// (CTransition::adjust_offset 0x0050a370, - /// pc:272271-272393), acdream ports that verbatim in - /// Transition.AdjustOffset, and remote bodies do run that sweep. + /// pc:272271-272393), acdream ports that in + /// Transition.AdjustOffset (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