acdream/docs/research/2026-08-22-campaign-ar-review.md

# Campaign AR — independent deep review

**Date:** 2026-08-22 · **Reviewer:** Claude (Fable 5), report-only · **Scope:**
`main (6c79d35c) .. c51b07ef` — 378 files, +52,639 / −954 — codex's atmospheric
rendering + terrain fidelity campaign, plus the imported signal-sequencing
commit. Axes: architecture, technical correctness, performance, coverage of
the discussed scope.

Every claim below was checked against the source, the decomp, or a test run
made during this review. Where codex's docs and the evidence disagree, the
evidence is cited.

---

## Verdict

A substantial, technically competent body of work that delivers almost
everything we discussed. The shadow implementation in particular is the
textbook recipe done properly. The architecture is sound and honours the
project's boundaries (Runtime/physics diff is empty, Core changes are additive
metadata, the built-in pack uses the same registry as external packs).

The weaknesses are mostly in **evidence and provenance**, not code:

1. default-path invariance is asserted on a toy oracle, never measured
   against the pre-campaign build;
2. the #226 detail-fade constants are invented and presented as retail;
3. the Tier-1 post stack runs in gamma space;
4. several closeout sentences claim more than their artifacts prove.

**Recommendation:** do not merge to main until the two F1 checks below have
run; decide F2; schedule F3/F4 as follow-ups; then the owner visual gate.

---

## Coverage against the discussed scope

| Discussed | Status | Notes |
|---|---|---|
| Dynamic shadows from trees, monsters, houses | **Delivered** | Camera-relative stable CSM, 2–4 cascades, opaque + alpha-cutout casters, animated casters, terrain + world receivers. Extended to moon shadows (doc records owner approval 2026-08-22). |
| Bloom, ACES tonemap, colour grade, vignette | **Delivered** | Gamma-space caveat (F4). |
| Screen-space sun rays | **Delivered** | Sky-depth occlusion mask, 48-tap radial; sun-only. |
| Volumetric shafts | **Delivered** | Reuses shadow depth; sun-only; no jitter (F8). |
| Rays composited before tonemap | **Delivered** | |
| Ray/shaft intensity from authored weather | **Delivered** | `AtmospherePolicyDeclaration` maps `activeDayGroup` + sun elevation in the pack descriptor. |
| Opt-in, retail default authoritative, via plugin API | **Delivered** | BCL-only `AcDream.Plugin.Abstractions.Rendering`, SDK validator, three samples. |
| #226 detail overlay | **Delivered, with caveats** | Buildings + EnvCells (correct per decomp), through the previously dead checkbox. See F2/F3. |
| Terrain vertex normals (A2) | **Delivered, verified** | Retail `calc_lighting` 0x00531700 port. See F5d for the premise correction. |
| Subdivision (A3) | Rejected | Agree. |
| Tree wind / sway | **Not present** | Raised after codex's brief; not a gap in their work. Still the cheapest item on the list. |

---

## Findings, by severity

### F1 — HIGH (evidence): default-path invariance is unproven

> **CLOSED (VM0, 2026-08-22): PASS.** Exact pixel identity on static content
> vs `6c79d35c` (Holtburg + open field) and a clean production perf A/B with no
> regression — `2026-08-22-vm0-default-path-invariance.md`.

The plan says `NoOpRenderPackProductionIntegrationTests` "pins the
pre-campaign pass list, pipeline set, draw/dispatch tuple, framebuffer
SHA-256". It does — for a **2×2 synthetic framebuffer, one draw call, one
pipeline, on a `RecordingGpuDevice` that does not rasterize**
(`tests/AcDream.App.Tests/Rendering/Packs/NoOpRenderPackProductionIntegrationTests.cs`).
It proves the controller arm adds nothing to a toy composition. It does not
prove the real default frame is unchanged.

Meanwhile the hot default draw path was **refactored, not just extended**:
`WbDrawDispatcher.Rhi.cs` +512/−44 replaces the `MeshPipelineSet`,
`BindRingSection`/`WriteRingSection` instance and indirect-command handling
with variant-aware equivalents. `WbDrawDispatcher.cs` +120, `WorldSceneRenderer`
+127, `TerrainAtlas` +187. And the default path legitimately changed in two
places (A2 normals; the detail overlay, which is **on by default**).

`tools/run-offline-pixel-gate.ps1` has a `-Baseline` mode designed for exactly
this ("capture a baseline at the parent commit, then gate the slice"). No
record shows it was run against a `6c79d35c` capture. The six physical
"retail rows" record zero pack *work* and performance, not pixel identity.

**Ask:**
1. Capture `6c79d35c` pack-off with the pixel gate; capture HEAD pack-off with
   `BuildingDetailTextures=false`; diff. Expected: differences confined to
   terrain shading (A2). Anything outside a terrain mask is a regression.
2. A clean uncapped Release production A/B (no automation observer, no
   validation layers) at the CLAUDE.md profile camera: CPU/GPU p50/p99 and
   `alloc_kb` pre vs post. The connected dense-town numbers cannot serve (see
   F5b).

### F2 — HIGH (process): the #226 fade constants are invented and unregistered

> **Post-review correction (VM1, 2026-08-22):** "invented" was too strong. The
> 10 m → 50 m ramp is a real retail function, `ACRender::get_alpha_for_z`
> (0x006b6230). It is, however, dead for the surfaces #226 targets: retail
> evaluates it only in `D3DPolyRender::DrawPolyInternal` (immediate polygons)
> and only when the static `noFadeDetail` (0x00820e38, initialised 1) is 0;
> built meshes light with `Diffuse.a = 1`. The finding's conclusion (remove
> it; no register row) stands; the provenance claim is corrected.

`RetailDetailTextureContract.FullDetailDistanceMetres = 10f` /
`ZeroDetailDistanceMetres = 50f` drive `mesh_detail.vert:83`
(`clamp((50 − z) / 40, 0, 1)`). The pseudocode note presents "full through
10 m; linear 10–50 m" as part of the retail contract. **Nothing in retail has
a distance gate on detail:** `RenderDeviceD3D::DrawBuilding` (0x0059f2a0),
`DrawEnvCell` (0x0059f170), `D3DPolyRender::RenderMeshSubset` (0x0059ca10) and
`ACRender::SetDetailSurfaceInternal` (0x006b6280) install surface, tiling,
WRAP, LINEAR×3 and the blend — no viewer-distance term. Retail's distance
attenuation is mip averaging (LINEAR mip filter), which for the live category
texture (mean factor 1.033) converges to near-neutral on its own.

This is a guessed AC-specific constant in a class named `Retail…Contract`,
with TS-52 retired and no replacement register row — a double breach of the
workflow rules. **Ask:** either drop the ramp and rely on mips (retail), or
keep it and file the register row naming it an acdream adaptation.

### F3 — MEDIUM (premise): #226 ports retail's two-pass fallback; which path real hardware ran is undetermined

> **ANSWERED (VM2, 2026-08-22):** `m_caps.bCanDoSinglePassDetailing = 1`,
> `trysinglepass = 1` on the owner's GPU — retail runs the single-pass path.
> The reviewer's "probably the fallback" was wrong. See
> `2026-08-22-vm2-retail-detail-path-cdb.md`; re-ported at VM1.

`SetDetailSurfaceInternal` sets the `DSTCOLOR + INVSRCALPHA` framebuffer blend
**only when `stage == 0`** (the two-pass fallback). With
`m_caps.bCanDoSinglePassDetailing`, `RenderMeshSubset` and `landPolyDraw`
call it with stage 1 and the combine is the texture-stage setup in
`D3DPolyRender::SetSurface` (0x0059c4d0): stage 0 alpha `PREMODULATE`, stage 1
colour `BLENDCURRENTALPHA(texture, current)` — a **lerp toward the detail
colour by alpha**, not a multiply. That capability requires
`D3DTEXOPCAPS_BLENDCURRENTALPHA` and `D3DTEXOPCAPS_PREMODULATE` (0x0059f6c6).
PREMODULATE was rarely advertised by consumer drivers, so the fallback is
*probably* what players saw — but that is a recollection of driver caps, not
evidence. One cdb read settles it:
`dt acclient!RenderDevice::render_device->m_caps` on the PDB-paired binary.
Note this caveat also applies to the reviewer's own earlier "retail's detail
pass brightens" finding, which was derived from the same fallback path.

### F4 — MEDIUM (technical): the Tier-1 post stack runs in gamma space

The main world renders retail's fixed-function, gamma-encoded colours into
`Rgba16Float`. Nothing decodes them: bloom thresholds Rec.709 luma of
gamma values, `acesFitted` (Narkowicz) is applied to gamma values, saturation
and the 0.5 contrast pivot assume linear, and the result is written to the
UNORM swapchain without re-encoding (`atmospheric_filmic.frag`,
`atmospheric_bloom_downsample.frag`; no `pow`/sRGB anywhere in the
atmospheric shaders). The default `exposure = 0.80` is the compensation — ACES
maps gamma-0.5 to 0.62 at exposure 1.0, which is the "too bright" the owner
corrected live.

It is an opt-in look and the owner accepted it, so this is not a blocker.
But the fix is cheap (decode `pow(c, 2.2)` where the world colour is read,
encode at the end) and would make threshold/exposure/contrast behave like
every other tool's controls. Schedule as a slice.

### F5 — MEDIUM (docs): closeout claims exceed their evidence

> **CLOSED (VM4, 2026-08-22):** (a)–(d) corrected in place in the AR plan, the
> Stage-2 connected report, the Track A report and the findings doc, each with a
> dated "VM4 correction" note rather than a silent rewrite.

a. "Pins the pre-campaign … framebuffer SHA-256" — see F1.
b. "The user's turning hitch reproduces on the retail path" is drawn from
   `connected-dense-town-20260822-133727` (retail/off: CPU p50 **27.8 ms**,
   `alloc_kb p50 = 9,830`, 31 Gen0 GCs in 216 frames). The Slice H closeout
   already documents that the connected automation observer "intentionally
   allocates in proportion to scene size; 3.9–10.5 MiB/frame outdoor readings
   are automation observer cost". Pre-campaign production dense Caul measured
   3.0/4.9 ms CPU p50/p95. The A/B is valid for the *Medium-minus-retail
   increment* (~0.7–0.9 ms GPU); it says nothing about the owner's hitch or
   about production default-path cost.
c. "14,928/14,928, zero skips" holds only under the hermetic lane filter. A
   raw `dotnet test AcDream.slnx` shows 74 skips and 36 failures across five
   assemblies; re-run in isolation with the repo filter, all five pass
   (7,560 / 0). Fine — but the headline should say "hermetic lanes".
d. The Track A report repeats the findings doc's wrong premise that
   `TerrainUtils.GetNormal` produced "the faceted look". That function only
   orients scenery (`SceneryGenerator.cs:166`). The rendered mesh already had
   smooth central-difference normals (Phase 3b). The real change —
   central-difference → retail's split-aware incident-face average — is a
   genuine, decomp-verified parity port (0x00531700: sum unit plane normals
   per vertex id, normalise, `(0,0,1)` fallback), but a subtler visual change
   than either document implies. The reviewer owns the original error.

### F6 — LOW (architecture): the pack API is heavier than what it can express

42 public types in `AcDream.Plugin.Abstractions.Rendering`.
`RenderPassSemantic` is the built-in pack's pass list (BloomDownsample,
BloomBlur×2, SunOcclusion, SunRays, VolumetricShafts, FilmicComposite) plus
`CustomFullscreen`. A third-party pack can swap shaders within the built-in
pipeline and add fullscreen passes; it cannot introduce a new pass kind. That
is a sensible v1 (Iris/OptiFine are fixed programs with named slots) — the
docs should describe it that way rather than as a general framework. The
SPIR-V validator living in the BCL-only abstractions assembly is unusual but
lets the SDK validator run without App/Vulkan references. The built-in pack
registers through `BufferedRenderPackRegistry.Register(descriptor, assets)`
exactly like externals — good.

### F7 — LOW (perf/architecture): the shadow pass keeps its own transform buffer

`DirectionalShadowTransformBufferSet` uploads a second matrix buffer. The
matrices are recomposed from the same `MeshRef.PartTransform × LocalToWorld`
through the same `WbDrawDispatcher.ComposePartWorldMatrix`, so there is no
second pose (constraint 5 honoured in spirit), but there is a second
composition and upload per animated part per frame, and this buffer is where
the 65,536-matrix ceiling bug lived. Measured cost is small (0.11–0.21 ms CPU
incremental). A future GPU-culling step will want one shared buffer.

### F8 — LOW (quality)

- `atmospheric_volumetric.frag` marches up to 64 steps with no per-pixel
  jitter → visible banding at Low/Medium step counts.
- The sun-occlusion mask is "depth ≈ far plane" only; no disc/sky-radiance
  weighting, so rays are uniform-coloured.
- `SkyDescLoader.ResolveSortCenter` swallows exceptions to `Vector3.Zero`;
  acceptable for enhancement metadata, but note it against the silent-catch
  rule. The other 26 added `catch` sites are cleanup-then-rethrow or
  convert-to-failure-outcome, consistent with the fail-safe contract.

---

## What is good (specifically)

- **Receiver shader** (`directional_shadow_receiver.glsl`): bias in world
  metres (constant + slope-scaled + normal offset), scaled per cascade by
  texel density — the #129 NDC-bias trap is handled by construction; true
  compare-then-interpolate PCF via `textureGather`; cascade blend band and
  terminal reach fade in metres so FOV cannot move seams.
- **Cascade fitter**: bounding-sphere per split, radius quantised to 1/16 m,
  centre snapped to the light-space texel grid — stable CSM done right.
- **Caster pass**: per cascade, one terrain MDI plus the opaque/cutout MDI
  runs over the **full resident set**; no per-cascade CPU reculling, with
  classification counters so the gate is enforced rather than hoped.
  Alpha-cutout casters sample and discard. Low uses multiview.
- **Indoor gating**: `IsOutdoor = RenderSky && !CameraInsideCell`.
- **Lifecycle**: off-side candidate preparation, atomic activation, withdrawal
  at a frame boundary, no-retry per registration, Auto with 180-sample
  hysteresis and a visible reason string. Device loss is correctly treated as
  terminal to the device lifetime.
- **Boundaries**: `git diff main..HEAD -- src/AcDream.Runtime src/*/Physics`
  is empty; Core edits are additive (`TranslucencyFadeManager.Revision`,
  `SkyObjectData.AuthoredSortCenter`); Headless references no pack types.
- **#226 blend**: `src = (d.rgb·f, d.a·f)` under `DSTCOLOR + INVSRCALPHA` gives
  `dest × (1 + f·(d.rgb − d.a))` — exactly neutral at f=0, retail's measured
  factor at f=1. The dead `BuildingDetailTextures` checkbox is now live
  without a second option.
- **A2**: the decomp claim is correct and the port matches it (unit face
  normals summed per shared vertex, per-landblock like retail).
- **Tests**: green under the repo's hermetic filter — 7,560/0 in the five
  assemblies re-run in isolation, plus Runtime 1,818, Core 4,877,
  UI.Abstractions 884, Content 160, MossTank 48, RenderPackValidator 30.

---

## Performance summary

Reference adapter (RX 9070 XT, 1080p capped, fixed dense 9,498-caster scene):
Low 0.108 ms CPU incr / 0.90 ms GPU incl / 40 MiB; Medium 0.116 / 1.00 / 74;
High 0.121 / 1.24 / 114; volumetric +0.19 ms GPU. Credible and inside the
declared budgets. Caveats: one high-end adapter; the integrated-AMD row proves
only safe fallback; the connected dense-town figures carry the observer tax
(F5b); the default path's own pre/post cost is unmeasured (F1).

---

## Recommended order

1. F1 — run both checks (pixel gate vs `6c79d35c`; clean production A/B).
   Blocking for merge.
2. F2 — decide: drop the ramp (retail) or register it. Small.
3. F3 — one cdb read on the retail client. Small; also settles the
   reviewer's earlier finding.
4. F4 — linear-light post stack as a follow-up slice.
5. F5 — correct the four sentences in the plan/reports.
6. Owner visual gate per the plan's own final-gate list; then ship.