docs(plans): Campaign VM — VisualMaster plan + the Campaign AR review it closes
Adds the independent Campaign AR review (docs/research/2026-08-22-campaign-
ar-review.md): architecture, technical correctness, performance, coverage.
Findings F1-F8; F1 (default-path invariance proven only on a 2x2 recording-
device oracle) blocks merge; F2 (10 m/50 m detail fade has no retail anchor);
F3 (the port is retail's two-pass fallback; single-pass stage path
undetermined); F4 (post stack runs in gamma space); F5 (four overclaiming
sentences, one premise error the reviewer owns).
Campaign VM plans the closure: VM0 masked pixel + production perf A/B vs
6c79d35c; VM1 removes the fade (retail attenuates by mips); VM2 one cdb read
of m_caps.bCanDoSinglePassDetailing; VM3 linear-light post stack with a
numerically neutral preset; VM4 truthful docs; VM5 volumetric jitter; VM6
weather-driven foliage wind - procedural-scenery (bit-31 id namespace) cutout
subsets only, three motions (lean/branch/flutter) with gusts and per-tree
phase, shadows share the displacement include; VM7 closeout + merge.
Roadmap gains the Campaign VM pointer beside Campaign AR.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
c51b07ef95
commit
4dbbe8fead
3 changed files with 638 additions and 0 deletions
254
docs/research/2026-08-22-campaign-ar-review.md
Normal file
254
docs/research/2026-08-22-campaign-ar-review.md
Normal file
|
|
@ -0,0 +1,254 @@
|
|||
# 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
|
||||
|
||||
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
|
||||
|
||||
`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
|
||||
|
||||
`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
|
||||
|
||||
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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue