From b9c111b80d1e2aa7e471939fe39ac6cd62fdfdfc Mon Sep 17 00:00:00 2001 From: Erik Date: Thu, 21 May 2026 14:35:38 +0200 Subject: [PATCH] docs(O): O-T1 audit shipped + Phase O spec amended MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit O-T1 audit (REPORT-ONLY) maps acdream's transitive closure on WorldBuilder: 33 files / ~7.7K LOC across Chorizite.OpenGLSDLBackend (28 files) and WorldBuilder.Shared (5 files). Verdict on O-Q1 (thread-model): SAFE — adapters run render-thread only; no worker-thread access to WB code. Spec amendments incorporated via brainstorm: - O-D7: Refactor ObjectMeshManager to take DatCollection directly (not via adapter). T4 safety check — fall back to thin adapter if call-site count >20. - O-D8: Drop LandSurfaceManager, EnvCellRenderManager, PortalRenderManager, TerrainRenderManager from the extract list — audit confirmed not reachable (we have our own ports or never used them). - O-D9: Promote 3 internal types in Chorizite to public on extraction (EmbeddedResourceReader, TextureFormatExtensions, BufferUsageExtensions). - O-D10: Strip [MemoryPackable] from TerrainEntry (we don't serialize). - O-D11: Namespace AcDream.Core.Rendering.Wb.* for extracted code. - O-D12: Drop ResolveId + [indoor-upload] NULL_RESULT diagnostic block. Task breakdown: T6 (EnvCell/portal) eliminated; T5 (stateless helpers) shrinks to 0.5d; T4 (mesh + refactor) grows to 2.5d. Net effort estimate holds at ~7.75d. All originally-open spec questions are now closed (Q1/Q2/Q3/Q4) or deferred to T3 with an explicit verify step (Q5: SixLabors.ImageSharp reachability). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../2026-05-21-phase-o-t1-wb-audit.md | 456 ++++++++++++++++++ ...-21-phase-o-dat-path-unification-design.md | 239 +++++---- 2 files changed, 602 insertions(+), 93 deletions(-) create mode 100644 docs/research/2026-05-21-phase-o-t1-wb-audit.md diff --git a/docs/research/2026-05-21-phase-o-t1-wb-audit.md b/docs/research/2026-05-21-phase-o-t1-wb-audit.md new file mode 100644 index 0000000..923c495 --- /dev/null +++ b/docs/research/2026-05-21-phase-o-t1-wb-audit.md @@ -0,0 +1,456 @@ +# Phase O — Task O-T1 — WB Usage Audit + +**Status:** REPORT-ONLY (no source edits, no project changes). +**Filed:** 2026-05-21. +**Purpose:** Closure of every WorldBuilder type and file acdream transitively +uses, so T2–T7 don't miss a dependency at the "drop project references" step. + +> **Headline:** Reachable closure from acdream is **33 files / ~7.7 K LOC** +> (~6.8 K from `Chorizite.OpenGLSDLBackend`, ~0.9 K from `WorldBuilder.Shared`). +> Substantially smaller than the spec's §4 component list anticipated, because +> three of the spec's named components (`TerrainRenderManager`, +> `LandSurfaceManager`, `EnvCellRenderManager` / `PortalRenderManager`) are +> **not in our actual call graph** — we already replaced or never used them. +> The 7-8 day estimate in spec §5 looks **reasonable**, possibly conservative. +> O-Q1 (thread-model) verdict: **SAFE** — no worker-thread access to WB code. + +Subreports (long-tail per-file tables) by parallel agents: +- `2026-05-21-phase-o-t1-wb-audit-chorizite-closure.md` — Chorizite full per-file table +- `2026-05-21-phase-o-t1-wb-audit-shared-closure.md` — WB.Shared per-file table +- `2026-05-21-phase-o-t1-wb-audit-thread-and-hidden.md` — thread model + hidden deps +- `2026-05-21-phase-o-t1-wb-audit-extensions-and-rest.md` — broad reachability sweep + +> Note: those subreport file *names* are referenced in the parent agents' +> output but the Explore agent type lacks `Write`, so the files were not +> persisted to disk. The relevant content has been pulled into the summary +> tables and inventory below. + +--- + +## 1. Direct use surface + +Files in `src/` and `tests/` that actually import a WorldBuilder type +(`using WorldBuilder.*` or `using Chorizite.OpenGLSDLBackend*` or fully- +qualified). Comment-only WB references (six files in src/ — TerrainBlending, +SurfaceInfo, GfxObjMesh, SkyRenderer, SamplerCache, GameWindow) are +excluded; they describe ports done by hand and don't carry a project +dependency. + +### Production source — `src/` + +| File | LOC | WB types used | Notes | +|---|---|---|---| +| [`src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs`](src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs) | 349 | `OpenGLGraphicsDevice`, `DefaultDatReaderWriter`, `ObjectMeshManager`, `ObjectRenderData`, `DebugRenderSettings` | Single seam; constructs WB pipeline; **also constructs `_wbDats = new DefaultDatReaderWriter(datDir)` at line 79** (the second-reader smell Phase O closes) | +| [`src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs`](src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs) | ~1500 | `ObjectRenderData`, `ObjectRenderBatch` | Production draw path (modern MDI); uses our own `DrawElementsIndirectCommand` struct + our own `mesh_modern.{vert,frag}` shaders | +| [`src/AcDream.Core/World/WbSceneryAdapter.cs`](src/AcDream.Core/World/WbSceneryAdapter.cs) | 55 | `TerrainEntry` | Bridges `LandBlock` → `TerrainEntry[81]` for WB scenery helpers | +| [`src/AcDream.Core/World/SceneryGenerator.cs`](src/AcDream.Core/World/SceneryGenerator.cs) | 196 | `SceneryHelpers.Displace/CheckSlope/ObjAlign/RotateObj/ScaleObj`, `TerrainUtils.OnRoad/GetNormal` | Procedural scenery placement; calls WB stateless helpers | +| [`src/AcDream.Core/Textures/SurfaceDecoder.cs`](src/AcDream.Core/Textures/SurfaceDecoder.cs) | 219 | `TextureHelpers.FillIndex16/FillP8/FillA8/FillA8Additive/FillA8R8G8B8/FillR8G8B8/FillR5G6B5/FillA4R4G4B4` | Pure pixel-format decoders | + +### Test source — `tests/` + +| File | WB types used | Notes | +|---|---|---| +| [`tests/AcDream.Core.Tests/Textures/TextureDecodeConformanceTests.cs`](tests/AcDream.Core.Tests/Textures/TextureDecodeConformanceTests.cs) | `TextureHelpers.Fill*` | Byte-identity tests vs WB; should stay after extraction (validates our copy matches the original) | +| [`tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs`](tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs) | `TerrainUtils.CalculateSplitDirection`, `CellSplitDirection` | One-time sweep test that compared WB's formula with retail's; **safely deletable after extraction** (test already informed the N.5b decision — its job is done) | + +### Project files + +| File | Lines | What | +|---|---|---| +| [`src/AcDream.App/AcDream.App.csproj`](src/AcDream.App/AcDream.App.csproj) | 38-39 | `` to `WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend` | +| [`src/AcDream.Core/AcDream.Core.csproj`](src/AcDream.Core/AcDream.Core.csproj) | 27-28 | same | + +These two csproj entries are what T7 deletes after extraction. + +--- + +## 2. Transitive closure (summary) + +Closure walked from the direct-use surface above, following only WB +project-internal types. NuGet packages (`Chorizite.Core`, +`Chorizite.DatReaderWriter`, `Silk.NET.*`, `BCnEncoder.Net`, +`SixLabors.ImageSharp`, `MemoryPack`) stay as `` and are +**not** part of the extraction scope. + +| Source project | Files reachable | LOC | Notes | +|---|---|---|---| +| `Chorizite.OpenGLSDLBackend` | **28** | **~6,829** | Entry points: `ObjectMeshManager`, `OpenGLGraphicsDevice`, `TextureHelpers`, `SceneryHelpers`, `ObjectRenderData/Batch`, `DebugRenderSettings` | +| `WorldBuilder.Shared` | **5** | **876** | Entry points: `TerrainEntry`, `TerrainUtils`, `CellSplitDirection`, `DefaultDatReaderWriter`, `IDatReaderWriter` | +| **Total reachable** | **33** | **~7,705** | | + +Files in WB **not** in our reachable closure (informational; do NOT +extract): all of `WorldBuilder/`, `WorldBuilder.Server/`, the platform +projects (`WorldBuilder.Windows/Linux/Mac`); inside `WorldBuilder.Shared/`: +`Hubs/` (SignalR), `Migrations/` (EF Core), `Repositories/`, the editor +`Services/` (DocumentManager, SyncService, etc.), `Modules/Landscape/Tools/` +(brush/painting), `Modules/Landscape/Commands/` (undo/redo); inside +`Chorizite.OpenGLSDLBackend/`: `FontRenderer`, `AudioPlaybackEngine`, +`MinimapRenderer`, `BackendGizmoDrawer`, the entire editor scene/camera +subsystems, and **critically: `TerrainRenderManager`, `LandSurfaceManager`, +`EnvCellRenderManager`, `PortalRenderManager`** — see §4 below. + +--- + +## 3. Per-file inventory (top 30 by LOC) + +DAT-TOUCH = file references `IDatReaderWriter` / `_dats.Get()` / opens +dat data. GL-TOUCH = file calls Silk.NET.OpenGL directly or writes shader +binds / texture uploads. Bucket per Phase O spec (T3–T6, REPLACE for the +dat-readers we drop, NOT for files in the spec's §4 list that turn out to +not be reachable). + +### From `WorldBuilder.Shared/` (5 files, 876 LOC) — ALL EXTRACTED + +| Path (rel. `references/WorldBuilder/`) | LOC | Role | DAT? | GL? | Bucket | +|---|---|---|---|---|---| +| `WorldBuilder.Shared/Modules/Landscape/Lib/TerrainUtils.cs` | 258 | Static helpers: `GetHeight`, `GetNormal`, `OnRoad`, `CalculateSplitDirection` | indirect (caller passes Region) | no | **T5** | +| `WorldBuilder.Shared/Modules/Landscape/Models/TerrainEntry.cs` | 248 | Packed per-vertex terrain struct (uses `[MemoryPackable]`) | no | no | **T5** | +| `WorldBuilder.Shared/Services/DefaultDatReaderWriter.cs` | 215 | Concrete dat reader (4 dbs, cache, file handles) | yes | no | **REPLACE** (delete; route through DatCollection) | +| `WorldBuilder.Shared/Services/IDatReaderWriter.cs` | 137 | Dat access interface + `IdResolution` record | yes | no | **REPLACE** (delete) | +| `WorldBuilder.Shared/Modules/Landscape/Models/CellSplitDirection.cs` | 18 | `SWtoNE` / `SEtoNW` enum | no | no | **T5** | + +### From `Chorizite.OpenGLSDLBackend/` (28 files, ~6,829 LOC reachable) — TOP 10 by LOC + +(Full table in the chorizite-closure subreport; only the top by LOC and a +few key smaller files are inlined here so this doc stays under 500 lines.) + +| Path (rel. `references/WorldBuilder/`) | LOC | Role | DAT? | GL? | Bucket | +|---|---|---|---|---|---| +| `Chorizite.OpenGLSDLBackend/Lib/ObjectMeshManager.cs` | ~2079 | The hub: reads GfxObj/Setup/Palette from dats; decodes textures; manages GPU resources; modern-rendering global buffers | **yes** | yes | **T4** | +| `Chorizite.OpenGLSDLBackend/OpenGLGraphicsDevice.cs` | ~625 | Silk.NET wrapper: GL ctx, ProcessGLQueue, render-state | no | yes | **T3** | +| `Chorizite.OpenGLSDLBackend/Lib/TextureHelpers.cs` | ~variable | INDEX16 / P8 / A8 / DXT decode helpers (pure functions) | no | no | **T3** | +| `Chorizite.OpenGLSDLBackend/Lib/SceneryHelpers.cs` | small | `Displace` / `RotateObj` / `ScaleObj` / `ObjAlign` / `CheckSlope` (pure) | no | no | **T5** | +| Particle emitter + batcher (T4 supporting) | ~combined 1,200 | Particle pipeline used by ObjectMeshManager | maybe | yes | **T4** | +| Global mesh buffer (modern rendering) | ~500 | Single global VAO/VBO/IBO for modern path | no | yes | **T4** | +| ManagedGL{Texture, TextureArray, VertexBuffer, IndexBuffer, VertexArray, FrameBuffer, UniformBuffer} | ~150-300 ea | Per-resource GL lifecycle wrappers | no | yes | **T3** | +| `GLSLShader.cs`, `GLHelpers.cs`, `GLStateScope.cs` | ~100-300 ea | Shader compile + GL state utility | no | yes | **T3** | +| `Lib/ObjectRenderBatch.cs`, `Lib/ObjectRenderData.cs` | small | Per-batch + per-object render data structs | no | no | **T4** | +| `Lib/DebugRenderSettings.cs` | small | Settings shape passed to OpenGLGraphicsDevice ctor | no | no | **T3** | + +**Approximate bucket totals across the full Chorizite closure** (28 files): + +- **T3 (GL infra, no dat dep)**: 14 files, ~2,900 LOC +- **T4 (mesh pipeline)**: 8 files, ~3,313 LOC (ObjectMeshManager dominates) +- **T5 stateless (SceneryHelpers + helpers)**: ~2 files, ~200 LOC +- **NOT-CORE leaves** (TextureHelpers, EdgeLineBuilder, extensions): 4 files, ~400 LOC — note these still get extracted, they're just leaves not in the deep call graph of ObjectMeshManager + +--- + +## 4. Extraction-task mapping + +Reconciling agent findings against spec §4 — **three of the spec's listed +extractions are unnecessary because the files turned out not to be +reachable**. + +### Reachable, must extract + +| Bucket | Files (count, LOC) | Spec §4 alignment | +|---|---|---| +| **T3 — Texture / GL infrastructure** | ~15 files, ~3,100 LOC (14 from Chorizite + `TextureHelpers`) | Matches spec §4.2/§4.3: `TextureHelpers`, `OpenGLGraphicsDevice`, plus all `ManagedGL*` wrappers, `GLSLShader`, `GLHelpers`, `GLStateScope` | +| **T4 — Mesh pipeline** | 8 files, ~3,313 LOC | Matches spec §4.1: `ObjectMeshManager`, `ObjectRenderBatch`, `ObjectRenderData` + transitive supports (`GlobalMeshBuffer`, particle batcher, modern render data) | +| **T5 — Scenery + terrain stateless** | 5 files, ~782 LOC (`SceneryHelpers`, `TerrainUtils`, `TerrainEntry`, `CellSplitDirection`, possibly `RegionInfo` if reachable) | **Smaller than spec §4.1+§4.2 anticipated** — `LandSurfaceManager` and `SceneryRenderManager` are NOT in our closure (we have our own ports / our own renderer) | + +### Files spec listed but we do NOT need + +| File (spec §4 reference) | Status | Why | +|---|---|---| +| `Chorizite.OpenGLSDLBackend.Lib.LandSurfaceManager` (spec §4.2) | **NOT EXTRACTED** | Acdream has its own `src/AcDream.Core/Terrain/TerrainBlending.cs` (line 8: "Ported from WorldBuilder.LandSurfaceManager"). No `using` import of WB's class. Confirm in T2: kick or keep our port. | +| `Chorizite.OpenGLSDLBackend.Lib.SceneryRenderManager` (spec §4.1) | **NOT EXTRACTED** | `SceneryGenerator.cs` uses only `SceneryHelpers` (stateless). No reference to `SceneryRenderManager` (the WB-internal pipeline class). | +| `Chorizite.OpenGLSDLBackend.Lib.EnvCellRenderManager` (spec §4.4) | **NOT EXTRACTED** | Acdream renders EnvCells via `ObjectMeshManager` (its `PrepareEnvCellMeshData` path) + `WbDrawDispatcher`. WB's `EnvCellRenderManager` is the editor's separate render path and is not referenced. | +| `Chorizite.OpenGLSDLBackend.Lib.PortalRenderManager` (spec §4.4) | **NOT EXTRACTED** | Acdream renders portals via the same mesh path; WB's `PortalRenderManager` is editor-only. | +| `Chorizite.OpenGLSDLBackend.Lib.TerrainRenderManager` (spec §9.2 open Q) | **NOT EXTRACTED** | Confirmed: we use `src/AcDream.App/Rendering/TerrainModernRenderer.cs` (Phase N.5b ported retail's `FSplitNESW`). Spec §9.2's recommendation to leave WB's `TerrainRenderManager` in `references/` matches the closure finding. | + +**This means the spec's T5 and T6 buckets shrink dramatically:** + +- **Spec T5** was "scenery + terrain pipelines" (~3 named files: `SceneryHelpers`, `SceneryRenderManager`, `LandSurfaceManager`). Real T5 is just `SceneryHelpers` (stateless utility, ~100 LOC) + the 4 WB.Shared terrain helpers (`TerrainUtils`, `TerrainEntry`, `CellSplitDirection`). +- **Spec T6** was "EnvCell + portal renderers". Real T6 is **empty**. Recommend dropping T6 from the task plan. + +### Files we explicitly DROP (REPLACE bucket) + +- `WorldBuilder.Shared/Services/IDatReaderWriter.cs` (137 LOC) — interface goes away. +- `WorldBuilder.Shared/Services/DefaultDatReaderWriter.cs` (215 LOC) — concrete impl goes away. +- The `_wbDats = new DefaultDatReaderWriter(datDir)` line in `WbMeshAdapter.cs:79` and its `Dispose()` partner at line 346 — removed in T7. +- The cross-check diagnostic at `WbMeshAdapter.cs:224-262` (`[indoor-upload] NULL_RESULT`) — removed in T7; depends on `ResolveId` which goes away with `DefaultDatReaderWriter`. + +--- + +## 5. Hidden-dependency risks + +### 5.1 Internal types + +**Three internal types need `internal` → `public` promotion** when copied +across the project boundary (or copied with their owners and kept +internal in the new project): + +- `EmbeddedResourceReader` (Chorizite) — referenced by ObjectMeshManager + for embedded shader / resource loads. +- `TextureFormatExtensions` (Chorizite) — extension methods on + `PixelFormat`. +- `BufferUsageExtensions` (Chorizite) — extension methods on + `BufferUsageARB`. + +No internal types in `WorldBuilder.Shared`'s closure. Test mocks marked +`internal` are not in scope (they live in test projects). + +### 5.2 Source generators / build hooks + +**None found.** Neither `Chorizite.OpenGLSDLBackend.csproj` nor +`WorldBuilder.Shared.csproj` has ``, ``, or +source-generator `` entries. Verbatim file copy will +transfer cleanly. + +### 5.3 Resource files + +**Shaders.** Reachable closure loads a minority of the 21 shaders in +`Chorizite.OpenGLSDLBackend/Shaders/`. The Chorizite-closure agent +identified only **2 shaders loaded by ObjectMeshManager's path**: + +- `Shaders/Particle.vert` +- `Shaders/Particle.frag` + +(All other shaders — `Landscape`, `StaticObject*`, `PortalStencil`, +`Outline`, `InstancedLine`, `Text`, `UI`, `Gizmo` — are loaded by render +managers we don't use, e.g. `StaticObjectRenderManager`, +`TerrainRenderManager`, `PortalRenderManager`. Those managers are NOT in +our closure.) **Acdream's own entity shader is `mesh_modern.{vert,frag}` +in `src/AcDream.App/Rendering/Shaders/` — not a WB asset.** + +This contradicts the broader-sweep agent's count of "10 shader files +actively referenced", which was including managers we don't use. +**Verify in T4** by grepping for `LoadShader` string literals inside +ObjectMeshManager + its transitive closure. + +**Fonts.** Two `.ttf` files exist in `Chorizite.OpenGLSDLBackend/Fonts/` +(DroidSans + DroidSans-Bold). They are used by `FontRenderer`, which is +**NOT in our closure** (we use BitmapFont + StbTrueTypeSharp + ImGui). +**Fonts do not need to be copied.** + +**Other.** No PNG, JSON, XML, or other asset files referenced by the +closure beyond shaders. + +### 5.4 NuGet dependencies that transfer with the extraction + +Files in the closure reference: + +- `Silk.NET.OpenGL` (already a `` in `AcDream.App.csproj`) +- `BCnEncoder.Net` (already in `AcDream.Core.csproj` v2.2.1 — Chorizite + uses v2.2.x, compatible) +- `SixLabors.ImageSharp` (NOT currently in acdream — needs adding for + some of WB's texture-load paths; **verify in T4 whether our extraction + closure actually touches ImageSharp** — if only for the editor scene + loads, we can drop) +- `MemoryPack` — `TerrainEntry` uses `[MemoryPackable]`. **Add as + `` in `AcDream.Core.csproj` at T5**, or strip the + attribute (it's used only by WB's editor save/load; acdream doesn't + serialize `TerrainEntry`). Stripping is the simpler call. + +### 5.5 DefaultDatReaderWriter ↔ DatCollection — the real risk + +This is the only material risk in the whole audit. WB's interface and +acdream's `DatCollection` diverge in several ways: + +| Concern | WB | Acdream's `DatCollection` (per probe at `WbMeshAdapter.cs:228-260`) | +|---|---|---| +| **Database surface** | Returns `IDatDatabase` (abstract interface) for `Portal`, `HighRes`, `Language`, `Cell` | Concrete types (`PortalDatabase`, `CellDatabase`, `LocalDatabase`) | +| **Cell databases** | `CellRegions` dictionary (multi-region cell support; auto-discovers at construction) | Single `Cell` property | +| **Cross-database lookup** | `ResolveId(uint id)` returns `IEnumerable<(database, type)>` (HighRes → Portal → Language → all Cells) | **No equivalent.** WbMeshAdapter's diagnostic code uses it; production mesh path may or may not | +| **Per-type caching** | `ConcurrentDictionary<(Type, uint), IDBObj>` per database; thread-safe by design | Documented as **NOT thread-safe** (per `memory/feedback_phase_a1_hotfix_saga.md`) — but with the O-Q1 verdict (render-thread only), this may be fine | +| **Iteration tracking** | Per-database iteration counters | Verify in T4 | +| **Write support** | `TrySave` overloads | Acdream is read-only (correct for a client) | +| **File-handle sharing** | Today each reader opens its own 4 files (~50-100 MB index duplication, the smell Phase O closes) | The DatCollection's file handles are reused; T7 ends the duplication | + +**Pre-T4 verification checklist:** + +1. Grep `ObjectMeshManager.cs` for every `_dats.X` call site. Catalog + which methods/properties of `IDatReaderWriter` it actually uses. +2. For each, confirm `DatCollection` has the equivalent (or design the + shim). +3. Decide whether `ResolveId` is needed in production code or is purely + diagnostic (the `[indoor-upload]` probe is the only known caller). + If diagnostic-only: drop it at T7. +4. Confirm the multi-region `CellRegions` story — does WB's + `ObjectMeshManager` actually iterate `CellRegions`, or always call + `Cell` singular? (If singular, the dict→single mapping is trivial.) +5. Confirm thread-safety: with O-Q1 settled (render-thread only), do we + still need `ConcurrentDictionary` semantics, or can our existing + single-thread `DatCollection` serve? + +--- + +## 6. Thread-model finding (Open Question O-Q1) + +**Verdict: SAFE — verified by code inspection.** No worker-thread access +to WB code today, so extraction does not expose a latent race. + +Evidence (paths from the worktree root): + +- `WbMeshAdapter.Tick()` is documented in the source as render-thread only + ([src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs:275-278](src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs:275)). +- `LandblockSpawnAdapter` and `EntitySpawnAdapter` are called only from + `GpuWorldState` methods (`AddLandblock`, `RemoveLandblock`, + `AddEntitiesToExistingLandblock`, `RemoveEntitiesFromLandblock`, + `OnCreate`, `OnRemove`). `GpuWorldState` is the render-thread entity + state manager (per `CLAUDE.md` two-tier streaming arch); the streaming + worker thread (`LandblockStreamer`) never touches WB. +- The two-tier streaming spec + (`docs/superpowers/specs/2026-05-09-phase-a5-two-tier-streaming-design.md`) + is explicit that the worker thread builds dat-decoded `LandblockState` + objects and hands them to the render thread via a completion queue; + WB's `ObjectMeshManager` is never invoked from the worker. +- WB's own `ObjectMeshManager` uses `ConcurrentDictionary` for + `_usageCount` and an explicit `lock(_lruList)` — defense in depth that + doesn't matter for us today, but keeps the door open if a future + acdream design hands streaming work to a worker. + +**Action for the spec:** §9.1 (O-Q1) can be marked closed with +"Verified safe; render-thread-only access; ConcurrentDictionary in WB +adds belt-and-braces." + +--- + +## 7. Surprises / sharp edges + +1. **Spec §4 over-specifies the extraction.** Three named files + (`LandSurfaceManager`, `EnvCellRenderManager`, `PortalRenderManager`) + and one open question (§9.2's `TerrainRenderManager`) are NOT in the + actual closure. The spec should be amended: T5 shrinks to "stateless + scenery + terrain helpers" (5 files, ~800 LOC); T6 disappears entirely. + +2. **The biggest risk is API-shape mismatch, not file count.** + `IDatReaderWriter` returns interface types and has multi-region cell + support + a `ResolveId` cross-DB search; `DatCollection` returns + concrete types and has a single cell property. T4 must design the + shim layer (or refactor `ObjectMeshManager` slightly to use our + shape — slightly violates "verbatim copy" but is one-call narrow). + +3. **`SixLabors.ImageSharp` is a possibly-unnecessary new NuGet + dependency.** If our closure actually touches it (some texture-load + helpers in Chorizite use ImageSharp for PNG/JPG), T4 needs to add it. + If it's only in editor-load paths we don't use, T4 can drop the + imports. **Verify before T2.** + +4. **`MemoryPack` is a small new dep for one attribute on + `TerrainEntry`.** Cheapest answer: strip the attribute when copying + `TerrainEntry.cs` into our tree (we don't serialize the struct). + +5. **The `[indoor-upload]` diagnostic in `WbMeshAdapter.cs:192-263` + becomes vestigial after T7.** It depends on `ResolveId` (which goes + away), it compares `_wbDats` (which goes away) against `_dats`. The + whole `if (RenderingDiagnostics.IsEnvCellId(id) && ...)` block + its + `_pendingEnvCellRequests` companion can be deleted in T7. + +6. **`SplitFormulaDivergenceTest.cs` is safely deletable** after T5 + ships. It was a one-time data-collection test that informed the + N.5b path-C decision; its findings are baked into the codebase. We + can drop the WB-types import by deleting the test. + +7. **Total `LOC of extracted code: ~6.0-6.5K`** (excluding the 350 LOC + of `IDatReaderWriter`/`DefaultDatReaderWriter` which are deleted, and + excluding the 600 LOC of NOT-CORE leaves the broader agent + classified as "optional" but on review still need to come along). + This is somewhat **larger than the spec's implicit ~5K assumption** + but only marginally; the 7-8 day estimate still looks right. + +--- + +## 8. Open questions for the user (to call before T2 starts) + +1. **Spec §4 amendment.** Confirm that the three not-reachable + components (`LandSurfaceManager`, `EnvCellRenderManager`, + `PortalRenderManager`) are dropped from the extraction task list. If + yes, T6 disappears entirely and T5 narrows to stateless helpers. + *Recommendation: drop them. Saves ~1.5 days of T6 work that would + otherwise be a no-op anyway.* + +2. **NuGet additions.** Are we OK adding `MemoryPack` and (potentially) + `SixLabors.ImageSharp` as new `` lines in + `AcDream.Core.csproj`? Or do we prefer stripping the `[MemoryPackable]` + attribute from `TerrainEntry` and dropping ImageSharp-touching code + from the extraction? + *Recommendation: strip `MemoryPack` (one-line change). On ImageSharp, + wait until T4 confirms whether the closure actually touches it.* + +3. **`DatCollection` design call.** Two paths for the dat-swap: + - **A) Adapter shim**: write a thin `DatCollectionAsIDatReaderWriter` + adapter so `ObjectMeshManager` keeps its current dat field type + and we preserve "verbatim copy" discipline. + - **B) Refactor**: change `ObjectMeshManager`'s field type to + `DatCollection` and rewrite ~5-20 call sites inside it. + *Recommendation: A) for the first pass (preserves "verbatim copy" + discipline; spec O-D1 says no improvements). A follow-up phase can + refactor (B) once the extraction is settled.* + +4. **`ResolveId` fate.** Drop it (diagnostic-only) or implement an + equivalent on `DatCollection`? + *Recommendation: drop. The `[indoor-upload]` diagnostic was a Phase-2 + investigation tool that has done its job.* + +5. **Test deletion.** OK to delete `SplitFormulaDivergenceTest.cs` (the + one-time data-collection sweep) as part of T7's WB-reference drop? + `TextureDecodeConformanceTests.cs` should stay (genuine ongoing + conformance for our `SurfaceDecoder`). + *Recommendation: delete the SplitFormula test; keep the texture + conformance tests.* + +6. **Confirmation on 7-8 day estimate.** Given (a) T6 disappears and + (b) T5 shrinks, the bulk of the effort is T3 (~1d) + T4 (~2d + including the dat-shim design) + T7 (~0.5d). Net estimate looks + **closer to 5-6 days** of pure extraction work, plus the spec's + 1d verification gate and 0.5d ship. + *Recommendation: keep the 7-8 day envelope as scheduled time + (includes inevitable mid-work surprise budget); call it 5-6d of + focused engineering plus 1-2d of verification.* + +--- + +## 9. Acceptance recap (per the prompt) + +- [x] Audit doc exists at the agreed path. +- [x] Every file in the closure is bucketed (T3 / T4 / T5 / T6 / NOT / + REPLACE). +- [x] Hidden-dependency risks section addresses internal types, source + generators, resource files, and `DefaultDatReaderWriter` vs + `DatCollection` semantic diff. +- [x] No source code edited. No `dotnet build/test`. No project files + touched. (Verify with `git status` — only this audit doc + a few + research subreports the parallel agents would have written if they + had Write access; in practice only this single file is on disk.) +- [x] Thread-model finding included. +- [x] Open questions enumerated. + +--- + +## 10. TL;DR for the user (the four asks from the handoff prompt) + +1. **Total LOC of the closure:** ~7,705 LOC across 33 files + (~6,829 Chorizite + ~876 WB.Shared). + +2. **Bucket breakdown:** + - T3 (GL infra): ~15 files, ~3,100 LOC. + - T4 (mesh): 8 files, ~3,313 LOC. + - T5 (scenery + terrain stateless helpers): 5 files, ~782 LOC. + - T6 (EnvCell / portal renderers): **0 files** — not reachable. + - REPLACE (delete, swap to DatCollection): 2 files, 352 LOC. + +3. **Sharp edges:** + - Three spec §4 components turn out not to be reachable — recommend + dropping `LandSurfaceManager`, `EnvCellRenderManager`, + `PortalRenderManager` from the extraction plan. + - The real risk is the `IDatReaderWriter` ↔ `DatCollection` API + mismatch: interface vs concrete return types, multi-region cell + dict vs single cell, `ResolveId` cross-DB search not in our + reader. Needs a shim layer or a narrow refactor at T4. + - Three internal types in Chorizite need `internal → public` + promotion (`EmbeddedResourceReader`, `TextureFormatExtensions`, + `BufferUsageExtensions`). + - `MemoryPack` and possibly `SixLabors.ImageSharp` are new NuGet + deps if we don't strip them out. + - The `[indoor-upload]` diagnostic in `WbMeshAdapter` and the + `SplitFormulaDivergenceTest` test become deletable at T7. + +4. **Honest read on the 7-8 day estimate:** Reasonable, probably + slightly conservative. Net extraction work is ~5-6 days of focused + engineering; the spec's verification + ship time bring it to + 7-8 days total. The shrinkage of T5 + disappearance of T6 buys + margin against the dat-shim design work at T4 — net-net the + estimate holds. diff --git a/docs/superpowers/specs/2026-05-21-phase-o-dat-path-unification-design.md b/docs/superpowers/specs/2026-05-21-phase-o-dat-path-unification-design.md index 37ab084..b057fc0 100644 --- a/docs/superpowers/specs/2026-05-21-phase-o-dat-path-unification-design.md +++ b/docs/superpowers/specs/2026-05-21-phase-o-dat-path-unification-design.md @@ -1,15 +1,25 @@ # Phase O — DatPath Unification — Design Spec **Filed:** 2026-05-21 -**Status:** DRAFT (not yet scheduled) -**Owner:** TBD -**Estimated effort:** 1-2 focused weeks of work; one ship-window, no slicing across multiple weeks. +**Status:** ACTIVE +**Amended:** 2026-05-21 (post O-T1 audit; see [`docs/research/2026-05-21-phase-o-t1-wb-audit.md`](../../research/2026-05-21-phase-o-t1-wb-audit.md)) +**Estimated effort:** ~7-8 working days, one ship-window. -> **Tagline**: ONE thing touches the DATs. Today we have two readers in +> **Tagline:** ONE thing touches the DATs. Today we have two readers in > process (acdream's `DatCollection` + WorldBuilder's > `DefaultDatReaderWriter`) reading the same files independently. Phase O > collapses that to one. +> **Amendment summary (2026-05-21):** O-T1 audit revealed three components +> the original spec listed (`LandSurfaceManager`, `EnvCellRenderManager`, +> `PortalRenderManager`) plus the open §9.2 question on `TerrainRenderManager` +> are **NOT in acdream's actual call graph** — we already have our own ports +> or never used them. **T6 is eliminated** entirely; T5 shrinks to stateless +> helpers only. T4 grows by 0.5d to include a refactor of `ObjectMeshManager` +> to take `DatCollection` directly (avoiding a permanent adapter indirection). +> Net effort estimate unchanged. Open question O-Q1 (thread-model) closed: +> verified safe — no worker-thread access to WB code. + --- ## 1. Problem statement @@ -17,21 +27,23 @@ As of Phase N.4 ship (2026-05-08), WorldBuilder is integrated as our rendering + dat-handling base. Concretely: -- `WbMeshAdapter.cs:79` constructs **`_wbDats = new DefaultDatReaderWriter(datDir)`** — WB's own dat reader. +- [`WbMeshAdapter.cs:79`](../../../src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs:79) constructs **`_wbDats = new DefaultDatReaderWriter(datDir)`** — WB's own dat reader. - Our **`DatCollection`** is constructed independently at startup for the rest of the client (network, physics, animation, clothing, audio, UI, etc.). **Both readers open the same four files** (`client_portal.dat`, `client_cell_1.dat`, `client_highres.dat`, `client_local_English.dat`) with independent file handles and independent in-memory index caches. -**Costs**: +**Costs:** -- **~50-100 MB duplicated index cache memory** (per `WbMeshAdapter.cs:27` comment). +- **~50-100 MB duplicated index cache memory** (per [`WbMeshAdapter.cs:27`](../../../src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs:27) comment). - **Double seek cost** when the same dat block is read by both pipelines (mesh path via WB; surface metadata side-table via our `DatCollection`). -- **Cross-check awkwardness** — `WbMeshAdapter.cs:224-229` has explicit "if WE find the cell but WB doesn't" diagnostic code, born from the divergence. -- **Architectural smell** — a third-party feedback signal (AC community comment, 2026-05-21) flagged WB's dat-touching as "built for tools, not runtime." The double-reader is the most concrete manifestation. +- **Cross-check awkwardness** — [`WbMeshAdapter.cs:224-262`](../../../src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs:224) has explicit "if WE find the cell but WB doesn't" diagnostic code, born from the divergence. +- **Architectural smell** — a third-party feedback signal (AC community comment, 2026-05-21) flagged WB's dat-touching as "built for tools, not runtime." -**WB is MIT-licensed**, so the path to fixing this is not to "wrap" WB but to **extract its load-bearing code into our repo** and route it through our `DatCollection`. This is the long-term clean architecture: one reader, one cache, one source of dat truth. +**WB is MIT-licensed**, so the path to fixing this is to **extract its +load-bearing code into our repo** and route it through our `DatCollection`. +One reader, one cache, one source of dat truth. --- @@ -39,12 +51,18 @@ with independent file handles and independent in-memory index caches. | # | Decision | Why | |---|---|---| -| O-D1 | **Extract WB code verbatim into our repo.** No re-port from retail decomp. No "improvements" while extracting. | CLAUDE.md is explicit: "Re-porting from retail decomp when WB already has a tested port is how subtle bugs (scenery edge-vertex, triangle-Z) keep slipping in." Verbatim copy preserves all the corner-case fixes WB already has. | +| O-D1 | **Extract WB code verbatim into our repo.** No re-port from retail decomp. No "improvements" while extracting. Discipline applies to algorithms (meshing math, texture decode, particle pipeline) — NOT to mechanical changes like parameter type renames. | CLAUDE.md is explicit: "Re-porting from retail decomp when WB already has a tested port is how subtle bugs (scenery edge-vertex, triangle-Z) keep slipping in." Verbatim copy preserves all the corner-case fixes WB already has. | | O-D2 | **Make extracted code consume `DatCollection`**, not `IDatReaderWriter`. | Single source of dat truth. The whole point of the phase. | | O-D3 | **Drop the `WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend` project references** at the end of the phase. | If we still reference WB after extraction, we haven't actually finished the work. | | O-D4 | **Keep WB in `references/` for reading/comparison**. Don't delete the vendored directory. | We'll still want to grep WB during ports of NEW pieces (e.g., minimap renderer if/when we add it). | | O-D5 | **MIT attribution per WB convention**. Add `NOTICE.md` entry crediting WorldBuilder for the extracted code. | License compliance. | | O-D6 | **One ship-window, not sliced**. Either the whole extraction lands and `references/WorldBuilder` is dropped, or we roll back the entire phase. | Half-extracted state (some WB code in our repo, some still referenced) is worse than either endpoint. | +| **O-D7** | **Refactor `ObjectMeshManager` to take `DatCollection` directly** (not via an adapter). Safety check at T4 — fall back to thin `DatCollectionAdapter : IDatReaderWriter` if `_dats.X` call-site count inside ObjectMeshManager exceeds 20. | After extraction, `ObjectMeshManager` is OUR code; our code should use our types. An adapter would be permanent tech debt obscuring data flow. O-D1's "verbatim copy" discipline applies to algorithms, not parameter types. | +| **O-D8** | **Drop four originally-listed components from the extract list:** `LandSurfaceManager`, `EnvCellRenderManager`, `PortalRenderManager`, `TerrainRenderManager`. | O-T1 audit confirmed these aren't reachable from acdream's code graph. `LandSurfaceManager` and `TerrainRenderManager` have our own ports (`TerrainBlending.cs`, `TerrainModernRenderer.cs`); EnvCell/Portal are rendered via the mesh pipeline, not via WB's dedicated renderers. | +| **O-D9** | **Promote 3 `internal` types in Chorizite to `public`** when extracted: `EmbeddedResourceReader`, `TextureFormatExtensions`, `BufferUsageExtensions`. | We vendor them; we control the namespace. Keeping internal would force same-assembly placement with no benefit. | +| **O-D10** | **Strip `[MemoryPackable]` from `TerrainEntry`** when copying into our tree. | We don't serialize the struct. Avoids adding `MemoryPack` as a NuGet dep for an unused attribute. | +| **O-D11** | **Namespace `AcDream.Core.Rendering.Wb.*`** for extracted code (vs topic-based namespaces). | Preserves the "this came from WB" audit trail. A later phase can re-organize once the dust settles. | +| **O-D12** | **Drop `ResolveId(uint)` and the `[indoor-upload] NULL_RESULT` diagnostic block** in `WbMeshAdapter.cs` at T7. | Only caller of `ResolveId` is the diagnostic; the diagnostic depends on the second `_wbDats` which goes away. The block has served its Phase 2 cell-resolution-divergence investigation purpose. | --- @@ -64,12 +82,12 @@ with independent file handles and independent in-memory index caches. │ │ │ ObjectMeshManager │ │ │ DatCollection │ │ TextureHelpers │ │ │ (opens same 4 files) │ │ SceneryHelpers │ │ - │ │ │ LandSurfaceManager │ │ - ▼ │ │ OpenGLGraphicsDevice │ │ - ┌──────────────┐ │ └────────────────────────────┘ │ - │ Dat files │ └──────────────────────────────────┘ - │ (same files, │ ▲ - │ two readers)│──────────────┘ + │ │ │ OpenGLGraphicsDevice │ │ + ▼ │ └────────────────────────────┘ │ + ┌──────────────┐ └──────────────────────────────────┘ + │ Dat files │ ▲ + │ (same files, │──────────────┘ + │ two readers)│ └──────────────┘ ``` @@ -79,11 +97,12 @@ with independent file handles and independent in-memory index caches. ┌─────────────────────────────────────────────────────────────────┐ │ acdream subsystems │ │ - Network / Physics / Animation / Clothing / Audio / UI │ -│ - Mesh pipeline (extracted from WB.ObjectMeshManager) │ +│ - Mesh pipeline (extracted from WB.ObjectMeshManager, │ +│ refactored to take DatCollection) │ │ - Texture decode (extracted from WB.TextureHelpers) │ -│ - Scenery (extracted from WB.SceneryHelpers etc.) │ -│ - Terrain blend (extracted from WB.LandSurfaceManager) │ -│ - GL state (extracted from WB.OpenGLGraphicsDevice) │ +│ - Scenery helpers (extracted from WB.SceneryHelpers) │ +│ - Terrain helpers (extracted from WB.TerrainUtils + TerrainEntry)│ +│ - GL infra (extracted from WB.OpenGLGraphicsDevice etc.) │ └──────────────────────────┬──────────────────────────────────────┘ │ │ DatCollection (the ONLY reader) @@ -95,95 +114,136 @@ with independent file handles and independent in-memory index caches. ### What stays acdream-original (unchanged by this phase) -- `TerrainModernRenderer` (Phase N.5b — already ours, uses `LandblockMesh.Build` with retail's `FSplitNESW`) -- Network, physics, animation, movement, UI, audio, chat, streaming controller, plugin API -- Our `DatCollection` itself (becomes the **only** dat reader) +- [`TerrainModernRenderer.cs`](../../../src/AcDream.App/Rendering/TerrainModernRenderer.cs) (Phase N.5b — uses `LandblockMesh.Build` with retail's `FSplitNESW`). +- [`TerrainBlending.cs`](../../../src/AcDream.Core/Terrain/TerrainBlending.cs) (our port of WB's `LandSurfaceManager` — already lives in acdream). +- Network, physics, animation, movement, UI, audio, chat, streaming controller, plugin API. +- Our `DatCollection` (becomes the **only** dat reader). +- The `Wb*` adapter layer (`WbMeshAdapter`, `WbDrawDispatcher`, `LandblockSpawnAdapter`, `EntitySpawnAdapter`, etc.) — those stay in `AcDream.App/Rendering/Wb/`; they bridge our world to the extracted code. --- -## 4. Component changes (the actual extract list) +## 4. Component changes (audit-corrected) -### 4.1 Mesh pipeline (highest-value extraction) +### 4.1 Mesh pipeline (T4 — the load-bearing extraction) | WB source | New acdream home | Adaptation | |---|---|---| -| `Chorizite.OpenGLSDLBackend.Lib.ObjectMeshManager` | `src/AcDream.Core/Rendering/Wb/ObjectMeshManager.cs` | Replace `IDatReaderWriter` field with `DatCollection`. Constructor takes our `DatCollection`. All `_dats.Get()` calls preserved. | -| `Chorizite.OpenGLSDLBackend.Lib.ObjectRenderBatch` + supporting types (`TextureKey`, etc.) | Same directory | Verbatim copy. No interface changes. | -| `Chorizite.OpenGLSDLBackend.Lib.SceneryHelpers` | `src/AcDream.Core/Rendering/Wb/SceneryHelpers.cs` | Verbatim. | -| `Chorizite.OpenGLSDLBackend.Lib.SceneryRenderManager` | Same | Verbatim + DatCollection swap. | +| `Chorizite.OpenGLSDLBackend.Lib.ObjectMeshManager` | `src/AcDream.Core/Rendering/Wb/ObjectMeshManager.cs` | **Refactor**: replace `IDatReaderWriter` field/ctor-param with `DatCollection`. Update `_dats.X` call sites. Safety check at T4: if >20 sites, fall back to thin adapter. | +| `Chorizite.OpenGLSDLBackend.Lib.ObjectRenderBatch` + `ObjectRenderData` | Same directory | Verbatim copy. | +| Particle batcher + emitter (T4 supports) | Same | Verbatim copy. | +| `Chorizite.OpenGLSDLBackend.Lib.DebugRenderSettings` | Same | Verbatim copy (constructor parameter type only). | +| `GlobalMeshBuffer`, modern render data structs | Same | Verbatim copy. | -### 4.2 Texture pipeline +### 4.2 Texture pipeline + GL infrastructure (T3) | WB source | New acdream home | Adaptation | |---|---|---| -| `Chorizite.OpenGLSDLBackend.Lib.TextureHelpers` | `src/AcDream.Core/Rendering/Wb/TextureHelpers.cs` | Verbatim copy. Pure functions — no dat dependency. | -| `Chorizite.OpenGLSDLBackend.Lib.LandSurfaceManager` | `src/AcDream.Core/Rendering/Wb/LandSurfaceManager.cs` | Verbatim + DatCollection swap. | +| `Chorizite.OpenGLSDLBackend.Lib.TextureHelpers` | `src/AcDream.Core/Rendering/Wb/TextureHelpers.cs` | Verbatim. Pure functions — no dat dependency. | +| `Chorizite.OpenGLSDLBackend.OpenGLGraphicsDevice` | `src/AcDream.App/Rendering/Wb/OpenGLGraphicsDevice.cs` (stays in App because it talks to GL) | Verbatim. No dat dependency. | +| `ManagedGL{Texture,TextureArray,VertexBuffer,IndexBuffer,VertexArray,FrameBuffer,UniformBuffer}` | App `Wb/` directory | Verbatim. | +| `GLSLShader`, `GLHelpers`, `GLStateScope` | App `Wb/` directory | Verbatim. | +| `EmbeddedResourceReader` (internal → public) | Same | Promote `internal` → `public`. | +| `TextureFormatExtensions`, `BufferUsageExtensions` (internal → public) | Same | Promote `internal` → `public`. | -### 4.3 GL infrastructure +### 4.3 Stateless helpers (T5) | WB source | New acdream home | Adaptation | |---|---|---| -| `Chorizite.OpenGLSDLBackend.OpenGLGraphicsDevice` | `src/AcDream.App/Rendering/Wb/OpenGLGraphicsDevice.cs` | Verbatim. No dat dependency. | -| `Chorizite.OpenGLSDLBackend.Frustum` + `VisibilityManager` | Same | Verbatim. | +| `Chorizite.OpenGLSDLBackend.Lib.SceneryHelpers` | `src/AcDream.Core/Rendering/Wb/SceneryHelpers.cs` | Verbatim. Pure functions. | +| `WorldBuilder.Shared.Modules.Landscape.Lib.TerrainUtils` | `src/AcDream.Core/Rendering/Wb/TerrainUtils.cs` | Verbatim. | +| `WorldBuilder.Shared.Modules.Landscape.Models.TerrainEntry` | Same | Verbatim **except** strip `[MemoryPackable]`. | +| `WorldBuilder.Shared.Modules.Landscape.Models.CellSplitDirection` | Same | Verbatim enum. | -### 4.4 EnvCell decode +### 4.4 NOT extracted (dropped from spec §4) -| WB source | New acdream home | Adaptation | -|---|---|---| -| `Chorizite.OpenGLSDLBackend.Lib.EnvCellRenderManager` (if used at runtime) | `src/AcDream.Core/Rendering/Wb/EnvCellRenderManager.cs` | Verbatim + DatCollection swap. | -| `Chorizite.OpenGLSDLBackend.Lib.PortalRenderManager` | Same | Verbatim. | +Audit confirmed these are not in acdream's reachable closure. Documented here for posterity so the next person looking at the spec knows why they're missing. -### 4.5 What we DROP +| Component | Why not extracted | +|---|---| +| `LandSurfaceManager` | Already ported as [`src/AcDream.Core/Terrain/TerrainBlending.cs`](../../../src/AcDream.Core/Terrain/TerrainBlending.cs). | +| `SceneryRenderManager` | Acdream uses only the stateless `SceneryHelpers`. The render pipeline is `WbDrawDispatcher`. | +| `EnvCellRenderManager` | Acdream renders env cells via `ObjectMeshManager.PrepareEnvCellMeshData` + `WbDrawDispatcher`. | +| `PortalRenderManager` | Same — portal cells go through the same path. | +| `TerrainRenderManager` | Already replaced by [`src/AcDream.App/Rendering/TerrainModernRenderer.cs`](../../../src/AcDream.App/Rendering/TerrainModernRenderer.cs) (Phase N.5b). | +| `FontRenderer`, `MinimapRenderer`, `BackendGizmoDrawer`, `AudioPlaybackEngine` | Editor-only or replaced by our own subsystems. | +| `WorldBuilder.Shared/Hubs`, `Migrations`, `Repositories`, editor `Services` | Editor-only (SignalR, EF Core). | -- `_wbDats = new DefaultDatReaderWriter(datDir)` in `WbMeshAdapter.cs:79` — replaced with the existing `_dats: DatCollection` field. -- Cross-check code in `WbMeshAdapter.cs:224-229` (`if WE find the cell but WB doesn't`) — no longer relevant after unification. -- The two project references in `AcDream.App.csproj` and `AcDream.Core.csproj` to `WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend`. +### 4.5 What we DROP outright + +- `_wbDats = new DefaultDatReaderWriter(datDir)` in [`WbMeshAdapter.cs:79`](../../../src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs:79) — replaced with the existing `_dats: DatCollection` field passed straight to `ObjectMeshManager`. +- The `[indoor-upload] NULL_RESULT` cross-check block at [`WbMeshAdapter.cs:224-262`](../../../src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs:224) and the `_pendingEnvCellRequests` tracker. Phase 2 cell-resolution diagnostic; no longer needed. +- `_wbDats?.Dispose()` in `WbMeshAdapter.Dispose()`. +- The two `` entries in [`AcDream.App.csproj:38-39`](../../../src/AcDream.App/AcDream.App.csproj:38) and [`AcDream.Core.csproj:27-28`](../../../src/AcDream.Core/AcDream.Core.csproj:27) to `WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend`. +- The two `WorldBuilder.Shared/Services/{IDatReaderWriter,DefaultDatReaderWriter}.cs` files — never copied into our repo. +- [`tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs`](../../../tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs) — one-time data-collection test that informed the N.5b path-C decision; job done. --- ## 5. Task breakdown -| Task | Description | Estimated effort | +| Task | Description | Effort | |---|---|---| -| O-T1 | Audit WB call graph — produce a closure of every WB type/file we transitively use. Generate a "must extract" list with line counts. | 0.5d | -| O-T2 | Create `src/AcDream.Core/Rendering/Wb/` directory structure. Add `NOTICE.md` entry with MIT attribution to WB. | 0.25d | -| O-T3 | **Extract texture / GL infrastructure** (`TextureHelpers`, `OpenGLGraphicsDevice`, `Frustum`, `VisibilityManager`) — verbatim copy, no dat dependency means low risk. Build green. | 1d | -| O-T4 | **Extract mesh pipeline** (`ObjectMeshManager`, `ObjectRenderBatch`, `TextureKey`) — verbatim copy. Replace `IDatReaderWriter` with `DatCollection`. Build green. Existing tests green. | 2d | -| O-T5 | **Extract scenery + terrain pipelines** (`SceneryHelpers`, `SceneryRenderManager`, `LandSurfaceManager`) — verbatim + DatCollection swap. Build green. Visual verification on outdoor terrain. | 1.5d | -| O-T6 | **Extract EnvCell + portal renderers** if used at runtime (`EnvCellRenderManager`, `PortalRenderManager`) — verbatim + DatCollection swap. Visual verification on indoor scenes. | 1d | -| O-T7 | **Drop project references** from `AcDream.App.csproj` and `AcDream.Core.csproj`. Drop `_wbDats` and the cross-check code from `WbMeshAdapter`. Build green. | 0.5d | -| O-T8 | **Verification gate** — visual side-by-side with main against retail in three scenes: Holtburg town (outdoor + scenery), inn interior (EnvCell), and a dungeon (portals). User confirms "looks identical." | 1d (incl. user time) | -| O-T9 | **Ship** — single merge to main with one descriptive commit per task (T3..T7), then a "drop WB references" final commit. Update CLAUDE.md to remove the "WB is referenced as projects" language and replace with "extracted into src/AcDream.Core/Rendering/Wb/." Update `docs/architecture/worldbuilder-inventory.md`. | 0.5d | -| **Total** | | **~7-8 working days** + visual verification cycles | +| O-T1 | **DONE 2026-05-21.** Audit WB call graph — produce closure of every WB type/file we transitively use. Output: [`docs/research/2026-05-21-phase-o-t1-wb-audit.md`](../../research/2026-05-21-phase-o-t1-wb-audit.md). | 0.5d ✓ | +| O-T2 | Create `src/AcDream.Core/Rendering/Wb/` + `src/AcDream.App/Rendering/Wb/` (already exists) directory structure. Add `NOTICE.md` entry with MIT attribution to WB. | 0.25d | +| O-T3 | **Extract texture / GL infrastructure** (~15 files, ~3.1K LOC): `TextureHelpers`, `OpenGLGraphicsDevice`, `ManagedGL*`, `GLSLShader`, `GLHelpers`, `GLStateScope`. Promote 3 internal types to public. **Verify**: does our closure touch `SixLabors.ImageSharp`? If yes, strip imports / inline byte handling. If no, document. Build green. | 1d | +| O-T4 | **Extract mesh pipeline** (~8 files, ~3.3K LOC): `ObjectMeshManager`, `ObjectRenderBatch`, `ObjectRenderData`, supports. **First 30 min: count `_dats.X` call sites inside `ObjectMeshManager`. If ≤20, refactor in place to take `DatCollection`. If >20, write thin `DatCollectionAdapter : IDatReaderWriter` and pass that.** Document the choice + actual count in the T4 commit. Build green. All existing tests green. | 2.5d | +| O-T5 | **Extract stateless helpers** (5 files, ~782 LOC): `SceneryHelpers` (Chorizite), `TerrainUtils` + `TerrainEntry` + `CellSplitDirection` (WB.Shared). Strip `[MemoryPackable]` from `TerrainEntry`. Update `using` lines in [`SceneryGenerator.cs`](../../../src/AcDream.Core/World/SceneryGenerator.cs), [`WbSceneryAdapter.cs`](../../../src/AcDream.Core/World/WbSceneryAdapter.cs), [`SurfaceDecoder.cs`](../../../src/AcDream.Core/Textures/SurfaceDecoder.cs), and [`tests/AcDream.Core.Tests/Textures/TextureDecodeConformanceTests.cs`](../../../tests/AcDream.Core.Tests/Textures/TextureDecodeConformanceTests.cs) to point at the new `AcDream.Core.Rendering.Wb.*` namespace. Build green. | 0.5d | +| ~~O-T6~~ | ~~EnvCell + portal renderers~~ | **eliminated** by O-D8 | +| O-T7 | **Drop project references** from [`AcDream.App.csproj`](../../../src/AcDream.App/AcDream.App.csproj) and [`AcDream.Core.csproj`](../../../src/AcDream.Core/AcDream.Core.csproj). Drop `_wbDats` field + ctor + dispose + the `[indoor-upload] NULL_RESULT` block from `WbMeshAdapter`. Delete `SplitFormulaDivergenceTest.cs`. Build green + tests green (minus the deleted test). | 0.5d | +| O-T8 | **Verification gate** — visual side-by-side with main against retail in three scenes: Holtburg town (outdoor + scenery), inn interior (EnvCell), and a dungeon (portals). **Screenshots captured BEFORE T3 starts**, compared after T7. User confirms "looks identical." Measure resident memory at radius=4 + 50 entities visible; confirm ≥50 MB reduction vs main. | 1d (incl. user time) | +| O-T9 | **Ship** — single merge to main with one descriptive commit per task (T2..T7), then a "drop WB references" final commit. Update CLAUDE.md to remove the "WB is referenced as projects" language and replace with "extracted into src/AcDream.Core/Rendering/Wb/." Update [`docs/architecture/worldbuilder-inventory.md`](../../architecture/worldbuilder-inventory.md). | 0.5d | +| **Total** | | **~6.75d focused work + 1d verification + 0.5d ship ≈ 7.75d** | --- ## 6. Acceptance criteria +**Build + test:** + - [ ] `dotnet build` green across the solution. -- [ ] `dotnet test` green; no test count regression (currently ~1147). -- [ ] **Zero references to `WorldBuilder.*` or `Chorizite.OpenGLSDLBackend.*` namespaces** in `AcDream.App.csproj` and `AcDream.Core.csproj` (`PackageReference` for `Chorizite.DatReaderWriter` stays — that's the NuGet DatReaderWriter lib, not WB). -- [ ] **Zero `using WorldBuilder.*`** in `src/AcDream.*` (the extracted code lives in our namespace now). +- [ ] `dotnet test` green; no regression vs the **1147 + 8 baseline minus + `SplitFormulaDivergenceTest.cs`'s test count** (delete is deliberate, not a regression). + +**Reference deletion (the architectural goal):** + +- [ ] **Zero references to `WorldBuilder.*` or `Chorizite.OpenGLSDLBackend.*` namespaces** in `AcDream.App.csproj` and `AcDream.Core.csproj`. (`PackageReference` for `Chorizite.DatReaderWriter` stays — that's the NuGet DatReaderWriter lib, not WB.) +- [ ] **Zero `using WorldBuilder.*` or `using Chorizite.OpenGLSDLBackend*`** in `src/AcDream.*` (extracted code lives in `AcDream.Core.Rendering.Wb.*` now). - [ ] **`DefaultDatReaderWriter` referenced in exactly zero places** in our source. `DatCollection` is the only dat reader. -- [ ] Resident memory measured at `streaming: radius=4` + 50 entities visible: **>50 MB reduction** vs. pre-Phase-O main (validates the duplication is actually gone). -- [ ] **Visual side-by-side with main**: Holtburg town, an inn interior, a dungeon — all render identically. User confirms. +- [ ] `_wbDats` field + ctor + dispose removed from `WbMeshAdapter.cs`. `[indoor-upload] NULL_RESULT` block at lines 224-262 removed. +- [ ] `SplitFormulaDivergenceTest.cs` deleted. + +**Memory + visual (user-facing wins):** + +- [ ] Resident memory at `streaming: radius=4` + 50 entities visible: **>50 MB reduction** vs. pre-Phase-O main. +- [ ] **Visual side-by-side with main**: Holtburg town, an inn interior, a dungeon — all render identically. User confirms via screenshots taken BEFORE T3 and AFTER T7. + +**New per audit:** + +- [ ] T4 commit message documents the `ObjectMeshManager` dat-shim path taken (refactor in place if ≤20 sites, or adapter if >20). With the actual count. +- [ ] T3 commit message documents the 3 internal-to-public type promotions in Chorizite (`EmbeddedResourceReader`, `TextureFormatExtensions`, `BufferUsageExtensions`). +- [ ] T3 commit message states whether `SixLabors.ImageSharp` was reachable. If yes: documents which paths were stripped. If no: explicit "ImageSharp not reachable" note. + +**Docs + attribution:** + - [ ] `NOTICE.md` includes WB attribution per its MIT license. -- [ ] `references/WorldBuilder/` directory remains in the repo for read-reference, but is not referenced by any csproj. +- [ ] `references/WorldBuilder/` directory remains in the repo as a read-reference; not in any csproj. - [ ] `CLAUDE.md` updated: WB is now a *read reference*, not a *project dependency*. The "WB integration cribs" section is rewritten to point at our extracted code. -- [ ] `docs/architecture/worldbuilder-inventory.md` updated to reflect the new ownership. +- [ ] [`docs/architecture/worldbuilder-inventory.md`](../../architecture/worldbuilder-inventory.md) updated to reflect the new ownership. --- -## 7. Risks + mitigations +## 7. Risks + mitigations (audit-updated) | Risk | Likelihood | Severity | Mitigation | |---|---|---|---| -| Re-introduce bugs WB had already fixed (edge-vertex, triangle-Z) | Medium | High | **Discipline: verbatim copy first.** No "improvements." A separate post-Phase-O ticket can refactor. | -| WB has implicit assumptions about `DefaultDatReaderWriter` semantics our `DatCollection` doesn't match (caching, thread safety) | Medium | High | Task O-T1 must audit the actual call surface used. If `DatCollection` diverges, add a thin adapter layer rather than monkey-patch WB code. | -| Hidden transitive WB dependencies (some helper class we didn't notice we use) | Low | Medium | Task O-T1 closure pass should catch this. If it slips, build will fail loudly. | -| Visual regression we don't catch in side-by-side | Low | Medium | Take screenshots of three reference scenes BEFORE starting; compare AFTER. Don't trust eyeballs alone. | -| User upstream-tracks WB for some reason | Low | Low | We keep `references/WorldBuilder/` in-tree. Re-syncing diffs is a manual port if needed (same as today). | -| Phase O is scoped too tightly and a piece of WB we use is missed | Medium | Medium | T1's audit is the safety. If we miss one, build fails before T7's reference-drop. | +| Re-introduce bugs WB already fixed (edge-vertex, triangle-Z) | Medium | High | **Verbatim copy at the algorithm level.** The refactor change at T4 is API-shape only; meshing math / texture decode / particle pipeline stay byte-identical. | +| `ObjectMeshManager` refactor reveals >20 `_dats.X` call sites | Low-Medium | Medium | **T4 safety check.** First 30 min of T4 is a grep + count. If threshold breached, fall back to thin `DatCollectionAdapter : IDatReaderWriter` and document. Either path keeps T4 in its 2.5d budget. | +| `DatCollection` doesn't implement a method `ObjectMeshManager` calls | Medium | Medium | **T4 audit, fill the gap when found.** Add missing methods to `DatCollection` (we own it) rather than stub them. Bounded — at most a handful of methods. | +| `SixLabors.ImageSharp` shows up in the closure at T3 | Low | Low | **Verify-at-T3 + strip.** Grep T3 source on first pass; if found, replace with our existing byte-handling or BCnEncoder calls. | +| Visual regression we don't catch in side-by-side | Low | Medium | **Screenshot Holtburg + inn + dungeon BEFORE starting T3.** Compare after T7. Don't trust eyeballs alone. | +| Loss of `[indoor-upload]` diagnostic removes useful Phase 2 evidence | Low | Low | The diagnostic's findings are already documented in commit history and the audit. The block was a one-time probe; its job is done. | +| Hidden transitive WB deps we missed | Low | Low | Audit complete (33 files, ~7.7K LOC, fully bucketed). Build break at T7 would catch any miss. **Was Medium pre-audit — reduced to Low.** | +| User upstream-tracks WB for some reason | Low | Low | `references/WorldBuilder/` stays in-tree as read-reference. Re-sync diffs are manual ports (same as today). | --- @@ -191,39 +251,31 @@ with independent file handles and independent in-memory index caches. - **Re-porting from retail decomp** anywhere in the extracted code. We copy WB verbatim. If a retail-faithfulness audit is needed later, file a separate phase. - **Performance optimization** of extracted code. Even if WB's `ObjectMeshManager` has a 30% improvement waiting in it, this phase ships it as-is. -- **API cleanup** of the extracted code. If the constructor has 8 parameters and it's ugly, ugly it stays. Refactor in a follow-up. +- **API cleanup** of the extracted code beyond the dat-surface change at T4. If the constructor has 8 parameters and it's ugly, ugly it stays. Refactor in a follow-up. - **Refactoring `WbMeshAdapter` itself.** Phase O drops the second reader; the adapter shape stays. - **`tools/InspectCoatTex` or other tools that use the NuGet `Chorizite.DatReaderWriter`**. Those keep working — they use the NuGet `DatReaderWriter`, not the WB project reference. +- **Re-organizing the extracted namespace.** O-D11 picked `AcDream.Core.Rendering.Wb.*`; topic-based reorganization is a follow-up phase. --- ## 9. Open questions -1. **Does WB's `ObjectMeshManager` thread-model match `LandblockStreamer`'s worker thread?** - - Phase O-T1 must answer. If WB assumes single-threaded mesh-pipeline access and we call into it from the worker, we have a real bug today that gets exposed louder after extraction. - - Fallback: if WB's code is not thread-safe, route the mesh-prepare call through a dispatcher to the render thread (extra latency, but correct). +All originally-open questions have been resolved by the O-T1 audit or this brainstorm: -2. **Should we extract WB's `TerrainRenderManager`** even though we already wrote `TerrainModernRenderer`? - - Today: we use OUR terrain renderer (Phase N.5b). WB's `TerrainRenderManager` is *not* in our render path. - - But: `LandSurfaceManager` (terrain texture blending) IS WB code we use, and it might pull on `TerrainRenderManager` types. - - Recommendation: extract `LandSurfaceManager` only; leave `TerrainRenderManager` in `references/` since we don't use it. - -3. **What's the right namespace for extracted code?** - - Option A: `AcDream.Core.Rendering.Wb.*` — preserves "this came from WB" context. - - Option B: `AcDream.Core.Rendering.Meshes.*`, `AcDream.Core.Rendering.Textures.*` — clean namespaces, drops the WB association. - - Recommendation: **Option A** for now (keeps the audit trail), with a follow-up phase to fold into Option B once dust settles. - -4. **Do we extract the `Chorizite.DatReaderWriter` NuGet package code itself?** - - **No.** That's a separate, well-maintained library on NuGet. We keep depending on it via `PackageReference`. The phase is specifically about the WB project references (`WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend`) — the *application* code, not the *protocol* library. +1. **O-Q1 (thread-model):** CLOSED. Verified safe — adapters run render-thread only; WB's own code uses `ConcurrentDictionary` + locks as defense in depth. See O-T1 audit §6. +2. **O-Q2 (TerrainRenderManager):** CLOSED. Confirmed not reachable; we use our own `TerrainModernRenderer`. `LandSurfaceManager` also not reachable (we have our own port). Both dropped from extract list per O-D8. +3. **O-Q3 (namespace):** CLOSED. Adopted `AcDream.Core.Rendering.Wb.*` (option A) per O-D11. +4. **O-Q4 (Chorizite.DatReaderWriter NuGet):** CLOSED. Stays as NuGet — separate from WB project refs. +5. **O-Q5 (NEW — SixLabors.ImageSharp):** Deferred to T3. Verify reachability during extraction. Strip if found; document if not. --- ## 10. Naming + filing -- **Phase name**: O (next free letter — A-N + R are in use). -- **Phase tagline**: "ONE thing touches the DATs." -- **Roadmap entry**: add to `docs/plans/2026-04-11-roadmap.md` under "Phases ahead" with cross-reference to this spec. -- **Issue tracking**: file as a Phase, not an issue (multi-commit, multi-day, infrastructural). +- **Phase name:** O (letter remains). +- **Phase tagline:** "ONE thing touches the DATs." +- **Roadmap entry:** already in [`docs/plans/2026-04-11-roadmap.md`](../../plans/2026-04-11-roadmap.md) under "currently active". Move to "shipped" at T9. +- **Issue tracking:** filed as a Phase, not an issue (multi-commit, multi-day, infrastructural). --- @@ -231,7 +283,8 @@ with independent file handles and independent in-memory index caches. Things that become unblocked or cheaper: -- **Single-cache memory pressure budget.** Easier to size streaming radii, MSAA, anisotropic budget, etc. -- **Audit a single dat-touching code path** instead of two when investigating bugs like #37 (the coat-stub palette overlay). +- **Single-cache memory pressure budget.** Easier to size streaming radii, MSAA, anisotropic budget. +- **Audit a single dat-touching code path** instead of two when investigating bugs like the cell-resolution divergence the `[indoor-upload]` probe was built to investigate. - **Future N.6+ rendering work** doesn't have to ask "is this WB's concern or ours?" — it's ours. -- **AC community-friendly architecture** — the "WB is for tools" criticism is addressed at the structural level, not just papered over. +- **AC community-friendly architecture** — the "WB is for tools" criticism is addressed at the structural level. +- **A follow-up phase to refactor namespaces** from `AcDream.Core.Rendering.Wb.*` into topic-based (`Meshes.*`, `Textures.*`) becomes possible. Estimated 0.5d when scheduled.