d13d63d0a5
Dual-lens Opus review of 7e0c1303 (reports committed under
docs/research/). The law, gate, and vertical application are CONFIRMED
at instruction-byte level against the PDB-paired acclient.exe (the BN
text FPU-elides this whole area); the fix round addresses the findings:
- Blast MUST-FIX 1: real schema migration instead of a hand-edited dev
file. SettingsStore v2->v3: a pre-v3 display.fieldOfView was the
applied vertical FOV in degrees; v3 means retail's m_fGameFOV.
LoadDisplay migrates on read - the untouched old default 60 maps to
the retail default 90; a deliberate other value preserves its visible
16:9 framing (x (16/9 - 0.1)), clamped to the registered [10,160];
the next save stamps v3 and migration never reruns. The dev
settings.json hand-edit was reverted so the migration owns it.
- Blast MUST-FIX 2 / mechanism M2: the Field of View now applies LIVE on
Save (retail: Render::GRPCallback_OnRenderPreferenceChanged @0x0054d999
-> SmartBox::SetDefaultFov). RuntimeSettingsTargets gains the camera
graph and applies through ApplyDisplayWindowState - the update-phase
seam, deliberately NOT the render-phase preview path (the review's
WATCH-3 cull-vs-raster landmine).
- Mechanism M1 -> register row AD-90: retail's divisor aspect runs
through the Render.AspectRatio preference (ComputeAspectForViewport
@0x0054f150, (w/h) x pref x 0.75) - exactly raw w/h at the registered
default, which is what acdream assumes; retail's NaN-through-the-gate
quirk (M3) is folded into the same row as deliberately not reproduced.
- Docs: RetailFieldOfView now cites the decisive vertical proof
(D3DXMatrixPerspectiveFovLH fovy slot @0x0059ab71), the unconditional
SmartBox::RenderNormalMode site, and M4's exact horizontal numbers
(89.0/83.9/80.6 deg); the Config FOV row comment updated to LIVE.
- Blast WATCH 4 disposition: the 15 replay-harness PI/3 constants stay -
they are CAPTURE-TIME camera parameters for recorded fixtures, not
production framing; changing them would invalidate the replays.
Tests: +6 SettingsStore migration facts, +1 live-apply fact.
App suite 4,962/3 skips; UI.Abstractions 922.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
386 lines
22 KiB
Markdown
386 lines
22 KiB
Markdown
# Blast-radius review — commit `7e0c1303` (#389 SmartboxFOV port)
|
||
|
||
**Reviewer lens:** BLAST RADIUS (consumers of the changed surfaces), 2026-08-13.
|
||
**Commit:** `7e0c1303` — "fix #389: port retail's SmartboxFOV law; retire AD-89 (display slice 1)".
|
||
**Scope:** report only. No files edited outside this document.
|
||
|
||
## Verdict summary
|
||
|
||
The mechanism is right and the consumer sweep is largely clean — every camera
|
||
computes `Projection` on demand, nothing caches a projection or a FOV across a
|
||
`SetAspect`/`SetGameFov`, and the three "writes `.Aspect` directly" suspects are
|
||
all private cameras outside the controller. Two real defects survive, both on the
|
||
axis the commit did *not* traverse (the **stored value**, not the code):
|
||
|
||
| # | Verdict | Surface | One-line |
|
||
|---|---------|---------|----------|
|
||
| 1 | **MUST-FIX** | `SettingsStore.LoadDisplay` | The stored `fieldOfView` changed MEANING with no schema bump and no migration; existing users silently get 35.8° vertical where they had 60°. Migration machinery + precedent already exist and were not used. |
|
||
| 2 | **MUST-FIX** | divergence register / Config apply | Retail's FOV preference is LIVE (`SmartBox::SetDefaultFov` from the preference callback, re-applied every render); acdream's is next-launch-only. AD-89 was retired without a replacement row for the half that did not land. |
|
||
| 3 | **WATCH (conditional MUST-FIX)** | `WorldRenderFrameBuilder.Build` ordering | The per-frame preview path mutates camera FOV *after* the frame snapshotted its frustum. Dead in production today (`HasDraftPreview => false`); becomes a live cull-vs-raster mismatch the moment anyone makes the slider live — i.e. while fixing #2. |
|
||
| 4 | WATCH | 15 test harness sites + 4 comments | Still pin `MathF.PI / 3f` as "the production / RetailChaseCamera projection". Production is now 0.9363 rad. |
|
||
|
||
Reference values for the rest of this document (law = `gameFOV / (aspect − 0.1)`,
|
||
`gameFOV = π/2`):
|
||
|
||
| aspect | applied FovY | vs. old constant |
|
||
|---|---|---|
|
||
| 4:3 | 1.2736 rad = **72.97°** | +12.97° |
|
||
| 16:9 | 0.9363 rad = **53.64°** | −6.36° |
|
||
| 21:9 | 0.7033 rad = **40.30°** | −19.70° |
|
||
| 16:9, stored `60` reinterpreted | 0.6242 rad = **35.76°** | −24.24° ← MUST-FIX 1 |
|
||
|
||
---
|
||
|
||
## MUST-FIX 1 — the persisted `fieldOfView` was reinterpreted with no migration
|
||
|
||
**Evidence**
|
||
|
||
- `src/AcDream.UI.Abstractions/Panels/Settings/SettingsStore.cs:73`
|
||
```csharp
|
||
FieldOfView: ReadFloat (disp, "fieldOfView", d.FieldOfView),
|
||
```
|
||
Straight read. No range check, no version check — `LoadDisplay` (`:56-95`)
|
||
never inspects `root["version"]` at all.
|
||
- `src/AcDream.UI.Abstractions/Panels/Settings/SettingsStore.cs:596` —
|
||
`["fieldOfView"] = d.FieldOfView` is written unconditionally by
|
||
`BuildDisplayObject`, so **every** `settings.json` that has ever had a
|
||
`display` section carries the key. It is not an "only if the user touched the
|
||
slider" case.
|
||
- The schema-version machinery the fix needs already exists and is already used
|
||
for exactly this class of change:
|
||
- `SettingsStore.cs:42` — `private const int CurrentSchemaVersion = 2;`
|
||
- `SettingsStore.cs:397` — `// Version-1 migration: radar stored only X/Y and had no resolution key.`
|
||
The commit bumped neither the constant nor added a `display` migration.
|
||
- The commit message acknowledges the problem and solves it for one machine:
|
||
*"user settings.json migrated 60→90 by hand (stale pre-port default)"*.
|
||
|
||
**Consequence.** A pre-`7e0c1303` `settings.json` holds `fieldOfView: 60` (the
|
||
old `DisplaySettings.Default`). Post-commit that 60 is read as retail
|
||
`m_fGameFOV` degrees, so at 16:9 the applied vertical FOV is
|
||
`60° / (1.7778 − 0.1) = 35.76°` — a 40 % reduction from the 60° the same file
|
||
used to produce. The visible symptom is a hard zoom-in that survives restarts and
|
||
looks nothing like either the old build or retail, and the user has no way to
|
||
know the number in their file changed meaning. It is the exact inverse of the
|
||
"squished" report that opened #389.
|
||
|
||
**Shape of the fix (not applied).** Bump `CurrentSchemaVersion` to 3 and, in
|
||
`LoadDisplay`, when the file's `version` is `< 3` and a `display.fieldOfView`
|
||
key is present, replace it with `DisplaySettings.Default.FieldOfView` (or map it
|
||
through the inverse law if preserving the user's framing is preferred:
|
||
`gameFOV = storedVerticalDegrees × (aspect − 0.1)` — but the honest choice is the
|
||
retail registered default, since the old number never had a retail meaning to
|
||
preserve). Note the round-trip preservation contract in the class doc
|
||
(`SettingsStore.cs:31-37`): unknown top-level keys are carried forward, so the
|
||
version bump must be written on save through the existing `root["version"] =
|
||
CurrentSchemaVersion` sites (`:296`, `:339`, `:446`, `:500`, `:683`).
|
||
|
||
---
|
||
|
||
## MUST-FIX 2 — AD-89 retired while half its own required scope is still unported
|
||
|
||
AD-89's deleted text (from the commit's own diff) said the port *"must also
|
||
re-map the Config Field of View slider to retail's degree semantics in the same
|
||
change"*. The **semantics** landed. The **liveness** did not, and there is now no
|
||
register row for the gap.
|
||
|
||
**Retail is live.** Three decomp facts, two of which the commit already cites:
|
||
|
||
- `docs/research/named-retail/acclient_2013_pseudo_c.txt:344363-344365`
|
||
(`Render::GRPCallback_OnRenderPreferenceChanged` @ `0x0054d982`-`0x0054d999`):
|
||
```
|
||
0054d984 float FieldOfView = Render::m_RenderPrefs.FieldOfView;
|
||
0054d98c Current_Render_FieldOfView = FieldOfView;
|
||
0054d999 SmartBox::SetDefaultFov(SmartBox::smartbox, FieldOfView);
|
||
```
|
||
The preference-changed callback pushes the new value immediately.
|
||
- `acclient_2013_pseudo_c.txt:90988` (`SmartBox::SetDefaultFov` @ `0x00451e60`)
|
||
writes `this->m_fGameFOV` and nothing else.
|
||
- `:91727` / `:92660` (`0x00452b2f` / `0x00453b14`) re-evaluate
|
||
`m_fGameFOV / (m_ViewportAspectRatio − 0.1)` **inside the render path**, so the
|
||
new `m_fGameFOV` takes effect on the very next frame. There is no restart.
|
||
|
||
**acdream is next-launch-only.**
|
||
|
||
- `src/AcDream.App/Settings/RuntimeSettingsController.cs:171-176` — `Startup` is
|
||
snapshotted in the constructor.
|
||
- `:208-218` — `ApplyStartup` calls `target.ApplyDisplay(Startup.Display)`, i.e.
|
||
the ctor-time snapshot, and is guarded by `_startupApplied` so it runs once.
|
||
- `src/AcDream.App/UI/Layout/ConfigOptionsPageController.cs:571` — the FOV row's
|
||
`apply` only calls `bindings.SaveDisplay(...)`. Nothing on that path reaches
|
||
`CameraController`.
|
||
- `ConfigOptionsPageController.cs:563-566` documents this honestly
|
||
(`// Field of View: NEXT-LAUNCH ...`), and `DisplaySettings.cs:27-31` repeats
|
||
it — but neither is a register row, and a search of
|
||
`docs/architecture/retail-divergence-register.md` for `next launch` /
|
||
`NEXT-LAUNCH` / `next-launch` returns **zero** hits.
|
||
|
||
**Why this is not "pre-existing, therefore out of scope".** Before this commit
|
||
the slider was a no-op on the camera the player actually looks through
|
||
(`CameraDiagnostics.UseRetailChaseCamera` defaults **true** —
|
||
`src/AcDream.Core/Rendering/CameraDiagnostics.cs:27-28` — and the old
|
||
`ApplyFieldOfView` wrote only `Orbit`/`Fly`/`Chase`, never `RetailChase`). The
|
||
setting was inert, so its latency was immaterial. This commit made the value
|
||
matter, which is precisely when the register rule bites: the deviation is now
|
||
observable. Either add the AD row, or make it live — the live fix is one call
|
||
(`cameras.SetGameFov(value * MathF.PI / 180f)`) alongside the existing
|
||
`SaveDisplay` in the apply lambda. **If you take the live route, read MUST-FIX 3
|
||
first — do not route it through the per-frame preview seam.**
|
||
|
||
---
|
||
|
||
## MUST-FIX 3 (conditional) — the preview seam mutates cameras *after* the frame snapshot
|
||
|
||
`src/AcDream.App/Rendering/WorldRenderFrameBuilder.cs:162-164`:
|
||
|
||
```csharp
|
||
WorldCameraFrame camera = _camera.Resolve(); // snapshots Projection/ViewProjection/Frustum
|
||
_visibility.Begin(in camera, waitingForLogin);
|
||
_settings.Apply(in camera); // ← calls ApplyFieldOfView → SetGameFov → ApplyProjection
|
||
```
|
||
|
||
`Resolve()` (`:196-211`) freezes `Projection`, `ViewProjection` and `Frustum`.
|
||
`RuntimeWorldFrameSettingsPreview.Apply` (`:368-380`) then mutates every camera's
|
||
`FovY` **and now also `Aspect`**. Downstream:
|
||
|
||
- **Culling / visibility use the snapshot**: `_visibility.Begin` →
|
||
`RetailSelectionScene.SetViewFrustum(camera.Frustum)` (`:335`),
|
||
`_buildings.Gather(..., in frustum)` (`:170-173`),
|
||
`_environmentFrustum.Update(camera.ViewProjection)` (`:346`).
|
||
- **Rasterization re-reads the camera live**: `WorldScenePassExecutor.cs:128,
|
||
158, 171, 186, 208, 218, 227, 250, 280` all pass `camera.Camera` (the live
|
||
`ICamera`), and `TerrainModernRenderer.cs:207`, `Wb/WbDrawDispatcher.cs:2011`,
|
||
`ParticleRenderer.cs:253`, `ParticleRenderer.Rhi.cs:296`,
|
||
`Sky/SkyRenderer.cs:189` each recompute `camera.View * camera.Projection`.
|
||
|
||
So a FOV change applied at line 164 produces one frame culled at the old frustum
|
||
and rasterized at the new one — pop-in / holes at the screen edge on every frame
|
||
the value moves.
|
||
|
||
**Why this is only a WATCH today.** `RuntimeSettingsController.cs:202` —
|
||
`public bool HasDraftPreview => false;`, with the OP9 comment at `:195-201`
|
||
stating it is *"always false in production"* since `SettingsVM` was retired. The
|
||
branch is unreachable outside tests.
|
||
|
||
**Why it is a landmine.** MUST-FIX 2's natural-looking fix is "make the preview
|
||
path live". Doing that turns this dormant ordering bug into a live artifact on
|
||
the retail chase camera. The safe fixes are (a) drive `SetGameFov` from the
|
||
Config apply lambda, outside the frame graph entirely, or (b) move
|
||
`_settings.Apply` **above** `_camera.Resolve()` in `Build`.
|
||
|
||
---
|
||
|
||
## WATCH 4 — harness constants and comments still pin the deleted 60°
|
||
|
||
These compile and pass (explicit initializers override the changed defaults), but
|
||
they now describe a projection no production camera uses. Two of them are
|
||
load-bearing *replay* harnesses whose whole point is to reproduce the production
|
||
frustum, and a 60° test frustum is 6.4° **wider** than production at 16:9 — a
|
||
flood/clip regression that only shows at the production frustum can hide here.
|
||
|
||
Constructed at `MathF.PI / 3f`:
|
||
|
||
| File | Lines |
|
||
|---|---|
|
||
| `tests/AcDream.App.Tests/Rendering/Issue177StairDescentCameraFloodTests.cs` | 194, 250, 272, 378, 423, 478 |
|
||
| `tests/AcDream.App.Tests/Rendering/PortalProjectionTests.cs` | 121, 154, 266, 418 — each tagged `// RetailChaseCamera` |
|
||
| `tests/AcDream.App.Tests/Rendering/HouseExitWalkReplayTests.cs` | 479 |
|
||
| `tests/AcDream.App.Tests/Rendering/Issue181VisFlapReplayTests.cs` | 38 |
|
||
| `tests/AcDream.App.Tests/Rendering/OutdoorCellNodeTests.cs` | 41 |
|
||
| `tests/AcDream.App.Tests/Rendering/TerrainCullOrientationTests.cs` | 42 |
|
||
| `tests/AcDream.App.Tests/Rendering/TowerAscentReplayTests.cs` | 352 |
|
||
|
||
Comment-only drift (claims a production FOV that no longer exists):
|
||
|
||
- `CornerFloodReplayTests.cs:174` — *"Production projection: ChaseCamera/FlyCamera use FovY ~1.2"*
|
||
- `Issue130DoorwayStripTests.cs:49` — *"FovY 1.2 rad"*
|
||
- `Issue95DungeonFloodDiagnosticTests.cs:35` — *"FovY ~1.2"*
|
||
- `TerrainCullOrientationTests.cs:15` — cites `RetailChaseCamera.cs:203 / :52` for the projection convention (line numbers also stale after the edit)
|
||
|
||
Recommended: leave the numeric constants (changing them re-baselines flood
|
||
fixtures, which is its own gate) but retag them as *"pinned pre-#389 constant,
|
||
not the live production FOV"*, and point the replay harnesses at
|
||
`RetailFieldOfView.DefaultAppliedFovY` where the value is not fixture-pinned.
|
||
|
||
---
|
||
|
||
## CLEAN — per-surface enumeration
|
||
|
||
### Axis 1: readers of `ChaseCamera` / `RetailChaseCamera` / `OrbitCamera` / `FlyCamera` `.FovY` / `.Aspect` / `.Projection`
|
||
|
||
**No projection or FOV is cached anywhere.** All four cameras compute
|
||
`Projection` in a get-accessor from live `FovY`/`Aspect`:
|
||
`ChaseCamera.cs:66`, `OrbitCamera.cs:29`, `FlyCamera.cs:38`,
|
||
`RetailChaseCamera.cs:56-57`. A grep for cached-projection fields
|
||
(`_cachedProjection` / `_lastProjection` / `Matrix4x4 _projection`) returns
|
||
nothing in `src/AcDream.App`. There is no stale-matrix surface to fix.
|
||
|
||
The named suspects:
|
||
|
||
- **`PortalTunnelPresentation.cs:313`** — **CLEAN, and now more faithful.**
|
||
`_camera` is a private `PortalTunnelCamera` (declared `:87`), not one of the
|
||
four and never attached to `CameraController`; `ApplyProjection` cannot reach
|
||
it and it cannot fight back. Its FOV comes from
|
||
`UseSmartBoxFov(smartBoxProjection)` (`:314`), where the projection is
|
||
`_camera.Active.Projection` read live at `PrivatePresentationRenderer.cs:128`.
|
||
That is exactly retail's `CreatureMode::UseSmartboxFOV` reading the active
|
||
SmartBox projection, so the tunnel now inherits the smartbox law for free —
|
||
previously it inherited the invented 60°.
|
||
- **`PrivateEntityViewportRenderer.cs:203`** — **CLEAN.** `_camera` is an
|
||
`IPrivateEntityViewportCamera` (`:15`, `:73`); the implementations are
|
||
`DollViewportCamera` (`DollCamera.cs:59`, wrapping a private `DollCamera` with
|
||
its own `FovRadians = MathF.PI / 4f`, `DollCamera.cs:41`) and
|
||
`CreatureAppraisalPresentation` (`:315`, `:343`). Both are paperdoll/portrait
|
||
surfaces, outside the law by design, and `DollCamera`'s own doc already cites
|
||
retail's `UseSharpMode` branch — which the decomp confirms at
|
||
`acclient_2013_pseudo_c.txt:91718` (`Render::SetFOVRad(this->m_fFOVRadians)`,
|
||
the *non*-smartbox arm of the same function that carries the smartbox site at
|
||
`0x00452b2f`).
|
||
- **`TeleportViewPlaneController.cs:145-146`** — **CLEAN, with a noted footgun.**
|
||
`ProjectionOverrideCamera.Aspect` is a write-through into `_source.Aspect`, and
|
||
`_source` *is* the active world camera (`ApplyTo(_cameras.Active)` via
|
||
`WorldRenderFrameBuilder.cs:198`). Nothing in production writes it — the only
|
||
writer anywhere is `tests/.../PortalTunnelAssetTests.cs:229`, against its own
|
||
fixture source. Its `Apply` (`:94-118`) recovers aspect as
|
||
`baseProjection.M22 / baseProjection.M11`, which for
|
||
`CreatePerspectiveFieldOfView` is exactly `aspect` regardless of FOV — the
|
||
changed FovY does not perturb it. *Footgun:* a future caller that sets `.Aspect`
|
||
on the wrapper would be silently reverted by the next `ApplyProjection`, since
|
||
the controller is now the sole aspect authority. Worth a one-line comment on
|
||
the setter.
|
||
- **Picking / `WorldSelectionQuery`** — **CLEAN.**
|
||
`SelectionCameraSource.Snapshot()`
|
||
(`Composition/InteractionUiRuntimeSources.cs:553-559`) reads
|
||
`_camera.Active.Projection` **live at pick time**, and
|
||
`WorldSelectionQuery.PickAt` (`:145-154`) feeds that same snapshot's `View` +
|
||
`Projection` into `_selectionScene.Pick`. There is no cached pick projection,
|
||
so the pick ray cannot desync from the camera. (Pre-existing, unchanged: the
|
||
candidate frustum handed to `RetailSelectionScene.SetViewFrustum` is the
|
||
previous frame's snapshot — a one-frame skew across a resize, not introduced
|
||
here.)
|
||
- **`FrustumCuller` / `FrustumPlanes`** — **CLEAN.**
|
||
`FrustumPlanes.FromViewProjection` (`FrustumCuller.cs:34-66`) is a pure
|
||
function of the passed matrix; the struct stores planes, never a FOV.
|
||
- **`RetailChaseCamera` consumers (Phase W single-viewpoint)** — **CLEAN.**
|
||
`ViewerCellId` (`:41`) is the single viewpoint and is untouched.
|
||
`CameraFrameController.cs:75`, `MouseLookController.cs:184`,
|
||
`CameraPointerInputController.cs:304, 399` and
|
||
`WorldRenderFrameBuilder.cs:255-259` all read pose / cell / the diagnostics
|
||
flag — none reads `FovY` or `Aspect`. The near plane (0.1 m) and its long
|
||
correctness note (`RetailChaseCamera.cs:46-55`) are unaffected: the commit
|
||
changed only the FOV argument to the same `CreatePerspectiveFieldOfView` call.
|
||
- **Name-collision false positives** (unrelated `.Projection`, entity-placement
|
||
domain): `Rendering/Scene/LiveRenderProjectionJournal.cs`,
|
||
`Rendering/Scene/CurrentRenderSceneOracle.cs`,
|
||
`Runtime/Physics/RuntimeSetPositionState.cs`,
|
||
`Runtime/Entities/*`, `Headless/Hosting/*`. **CLEAN — not this surface.**
|
||
|
||
### Axis 2: readers of `DisplaySettings.FieldOfView`
|
||
|
||
- **`ConfigOptionsPageController.cs:567-573`** — **CLEAN, improved.**
|
||
`min: 10f, max: 160f, defaultValue: 90.0f` already matched retail's registered
|
||
row (`UIPreferences::SetPreferenceRange(&Render_FieldOfView, 10f, 160f)`
|
||
@ `0x004043b2`, `acclient_2013_pseudo_c.txt:3261`). Under the *old* semantics
|
||
the row's `defaultValue: 90` silently contradicted
|
||
`DisplaySettings.Default.FieldOfView = 60` — clicking "Defaults" wrote a
|
||
different number than a fresh install. The commit made them agree. No slider
|
||
change is needed.
|
||
- **`WorldRenderFrameBuilder.cs:370-379`** (preview path) — **WATCH**, see
|
||
MUST-FIX 3.
|
||
- **`RuntimeSettingsTargets.cs:130` / `:143-148`** (startup) — **CLEAN.** The
|
||
degree→radian conversion matches retail's own option setter
|
||
(`SmartBox::SetDefaultFov` @ `0x00451e6a`, literal
|
||
`0.017453292519943295`).
|
||
- **`RuntimeSettingsController.cs:165, 171-176, 208-218`** — **CLEAN as code**,
|
||
but is the mechanism behind MUST-FIX 2 (ctor-snapshot + once-only apply).
|
||
- **Headless / Runtime / Cli** — **CLEAN.** `grep DisplaySettings|SettingsStore`
|
||
over `src/AcDream.Headless`, `src/AcDream.Cli`, `src/AcDream.Runtime` returns
|
||
nothing. Headless has no FOV surface to reinterpret.
|
||
- **Launcher / UI Studio / fixtures** — **CLEAN.** No launcher project exists
|
||
(`src/` is App, Bake, Cli, Content, Core, Core.Net, Headless,
|
||
Plugin.Abstractions, Plugins.Smoke, Runtime, UI.Abstractions). No `ui-studio`
|
||
settings surface reads `FieldOfView`. No `settings.json` fixture is committed
|
||
(the only two in the tree are `.claude/` and `.vscode/`). MUST-FIX 1's blast
|
||
radius is therefore exactly "every real user's
|
||
`%LOCALAPPDATA%\acdream\settings.json`" and nothing else.
|
||
- **`tests/.../SettingsStoreTests.cs:45, 86`** — **CLEAN.** Uses `100f` and
|
||
`DisplaySettings.Default.FieldOfView`; both semantics-neutral.
|
||
|
||
### Axis 3: `CameraController` lifecycle
|
||
|
||
- **Constructor (`CameraController.cs:57-62`)** — **CLEAN.** C# runs
|
||
field/property initializers before the constructor body, so `GameFovRadians`
|
||
(`:53`) and `_aspect` (`:55`) are both set when `ApplyProjection()` runs.
|
||
- **Aspect seeding** — **CLEAN.** `HostInputCameraComposition.cs:317-321` binds
|
||
the camera target and then immediately calls
|
||
`FramebufferResize.Resize(InitialFramebufferSize)`, so `_aspect` is the real
|
||
viewport before any camera is read. There is no window in which the controller's
|
||
16:9 default could stomp a correct non-16:9 aspect (the concern raised by
|
||
`FramebufferResizeController.cs:124` — *"Late binding never replays an earlier
|
||
resize transition"* — is answered by that explicit initial `Resize`).
|
||
- **`EnterChaseMode` (`:74-88`)** — **CLEAN.** `ApplyProjection()` runs *before*
|
||
`_mode` is assigned and before `ModeChanged` fires, so no listener can observe
|
||
a half-converged camera. Both construction sites call it immediately after
|
||
`new`: `PlayerModeController.cs:350-357` (first entry) and `:185-190`
|
||
(`ToggleFlyOrChase`'s `_chase.Retail ??= new ...`). **No camera escapes the
|
||
law.** Note `:350` / `:353` still seed `Aspect = _viewport.Aspect` and
|
||
`EnterChaseMode` immediately overwrites it from `CameraController._aspect`;
|
||
harmless because `FramebufferResizeController.Resize:126-127` feeds both from
|
||
the same width/height in the same method, but the seed is now dead code.
|
||
- **`RestoreState` (`:148-161`)** — **CLEAN.** `ApplyProjection()` runs after the
|
||
camera fields are assigned but before `_mode` and before `ModeChanged`, and it
|
||
null-guards both chase slots (`:130-131`, `:137-138`), so restoring a
|
||
`Chase = null` prior state is safe. Its only caller is the entry-failure
|
||
rollback at `PlayerModeController.cs:385`;
|
||
`CameraControllerTests.RestoreState_ReestablishesPriorCameraAfterNotificationFailure`
|
||
(`:126-150`) still covers the throwing-listener path unchanged.
|
||
- **`ExitChaseMode` (`:90-100`)** — **CLEAN.** Deliberately does not re-apply;
|
||
`Orbit`/`Fly` are already converged and the cleared chase cameras are garbage.
|
||
- **Re-entrancy** — **CLEAN.** `ApplyProjection` invokes no callbacks, so
|
||
`SetAspect`/`SetGameFov` cannot re-enter.
|
||
|
||
### Bonus verification the commit did not cite: the divisor's aspect orientation
|
||
|
||
Worth recording because it is the one input that could have silently inverted the
|
||
whole law. If retail's `m_ViewportAspectRatio` were height/width, then at 16:9 the
|
||
divisor would be `0.5625 − 0.1 = 0.4625`, giving `1.5708 / 0.4625 = 3.396 rad`
|
||
— **greater than π**, so `TryAppliedVerticalFov` would return `false` on every
|
||
modern screen, the cameras would stay pinned at `DefaultAppliedFovY` forever, and
|
||
the slider would be inert. The unit tests (which feed `16f/9f` directly) would
|
||
not catch it.
|
||
|
||
It is width/height, proved by the consumer rather than by the decomp of the
|
||
producer: `acclient_2013_pseudo_c.txt:423669` (`PrimD3DRender::SetFOVInternal`
|
||
@ `0x0059ab71`) passes `m_ViewportAspectRatio` straight into
|
||
`D3DXMatrixPerspectiveFovLH`'s `Aspect` parameter, which is width/height by the
|
||
D3D contract. Note that the producer itself,
|
||
`RenderDevice::ComputeAspectForViewport` @ `0x0054f150`
|
||
(`acclient_2013_pseudo_c.txt:345715-345722`), decompiles to a bare
|
||
`return arg4;` / `return arg5;` — the classic Binary Ninja FPU-elision artifact
|
||
(see `memory/reference_pe_byte_decode.md`), so it cannot be read directly.
|
||
**Recommend adding the `0x0059ab71` citation to `RetailFieldOfView`'s class
|
||
doc** — it is the only evidence in the tree that the divisor is not inverted, and
|
||
the next reader will otherwise land on the mangled `ComputeAspectForViewport` and
|
||
have to redo this.
|
||
|
||
---
|
||
|
||
## Ranked action list
|
||
|
||
1. **MUST-FIX** — `SettingsStore`: bump `CurrentSchemaVersion` to 3 and migrate
|
||
`display.fieldOfView` on load for files written at version ≤ 2.
|
||
(`SettingsStore.cs:42, 56-95, 397, 596`)
|
||
2. **MUST-FIX** — either make the Config FOV row live
|
||
(`ConfigOptionsPageController.cs:571` → also `cameras.SetGameFov(...)`) to
|
||
match `Render::GRPCallback_OnRenderPreferenceChanged @0x0054d999`, **or** file
|
||
the AD row for the next-launch divergence. Retiring AD-89 without one leaves
|
||
an unrowed deviation.
|
||
3. **WATCH → MUST-FIX if #2 is taken live** — do not route the live apply through
|
||
`WorldRenderFrameBuilder`'s preview seam; either call `SetGameFov` from the
|
||
Config apply lambda or move `_settings.Apply` above `_camera.Resolve()`
|
||
(`WorldRenderFrameBuilder.cs:162-164`).
|
||
4. **WATCH** — retag the 15 `MathF.PI / 3f` harness sites and 4 stale comments as
|
||
pinned pre-#389 constants (§WATCH 4 table).
|
||
5. **WATCH (documentation)** — add the `0x0059ab71` D3DX citation to
|
||
`RetailFieldOfView`'s class doc; add a one-line note on
|
||
`TeleportViewPlaneController.ProjectionOverrideCamera.Aspect` that the
|
||
controller is now the sole aspect authority and a write here will be reverted.
|